Initial lambda wrapper 2 by rjlohan · Pull Request #4 · aws-cloudformation/cloudformation-cli-java-plugin

rjlohan · 2018-11-16T21:47:28Z

Initial Lambda Wrapper Implementation

This commit includes a basic framework for a Java Lambda wrapper for RPDK handlers.
It includes some temporary files and types which need to be refactored out in favour
of the actual RPDK handler implementations.

No tests exist yet, given the expected refactoring which will happen.

This PR also drops an initial implementation for #2

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

tobywf

some initial feedback, mainly about dates

src/com/aws/cfn/Action.java

src/com/aws/cfn/GenericHandler.java

src/com/aws/cfn/LambdaWrapper.java

src/com/aws/cfn/cron/CronHelper.java

src/com/aws/cfn/LambdaWrapper.java

src/com/aws/cfn/Action.java

aygold92 · 2018-11-16T22:09:07Z

src/com/aws/cfn/GenericHandler.java

+        Action action,
+        RequestContext context) {
+
+        Gson gson = new GsonBuilder().create();


just wondering, what was the reasoning for picking Gson over Jackson?

No specific reason. But it works.

pom.xml

aygold92 · 2018-11-16T22:59:59Z

src/com/aws/cfn/LambdaWrapper.java

+            request.getAction(),
+            (endTime.getTime() - startTime.getTime()));
+
+        // When the handler responses InProgress with a callback delay, we trigger a callback to re-invoke


if the handler wants to record additional progress events, how would it do that?

TBD in future PR.

aygold92 · 2018-11-16T23:04:32Z

src/com/aws/cfn/LambdaWrapper.java

+       return null;
+    }
+
+    public void handleRequest(InputStream inputStream, OutputStream outputStream, Context context) throws IOException {


where is the POJO which contains the ResourceRequest we get from CloudFormation?
I don't see clientRequestToken or credentials anywhere.

Also, resourceTypeVersion, nextToken for List calls, previousResourceProperties, behaviorSpec

This needs iteration and refactoring to consume the actual handler interface and context object. So far, just a Lambda wrapper for a basic function.

aygold92 · 2018-11-16T23:12:52Z

src/com/aws/cfn/metrics/MetricsPublisherImpl.java

+
+    public void publishInvocationMetric(final Date timestamp, final Action action) {
+        publishMetric(
+            Metrics.METRIC_NAME_HANDLER_INVOCATION_COUNT,


how is this meant to work? invocations by the handler or by cloudwatch? if both, how would we keep track?

This metric is indicating the handler is running. CloudFormation will perform the initial invocation, but since the wrapper re-invokes itself, there may be many more actual invokes are CFN hands off.

aygold92 · 2018-11-16T23:13:41Z

src/com/aws/rpdk/RequestContext.java

+     * If the request was the result of a CloudWatchEvents re-invoke trigger the
+     * CloudWatchEvents Trigger Id is stored to allow cleanup
+     */
+    private String cloudWatchEventsTargetId;


why would the handler implementation care about the cloudWatch stuff or the invocation count?

This RequestContext object comes unto Lambda, it's not passed to the Handlers.

your example takes in RequestContext and it appears you're passing it here? https://github.com/awslabs/aws-cloudformation-rpdk-java-plugin/blob/a83bb79089e5604144d91aa18bd9a2cf68875a13/src/com/aws/cfn/LambdaWrapper.java#L174

This commit includes a basic framework for a Java Lambda wrapper for RPDK handlers. It includes some temporary files and types which need to be refactored out in favour of the actual RPDK handler implementations. No tests exist yet, given the expected refactoring which will happen.

aygold92

looks fine for initial PR, other than the try/catch

aygold92 · 2018-11-19T18:18:51Z

src/com/aws/cfn/GenericHandler.java

+
+        final Gson gson = new GsonBuilder().create();
+        final JsonObject jsonObject = gson.toJsonTree(request.getResourceModel()).getAsJsonObject();
+        final ResourceModel model = gson.fromJson(jsonObject.toString(), ResourceModel.class);


was going to ask if we can do this stuff for them; it looks like you half way did it when you declared the argument as HandlerRequest<ResourceModel>

aygold92 · 2018-11-19T18:22:37Z

src/com/aws/cfn/LambdaWrapper.java

+    public void handleRequest(final InputStream inputStream,
+                              final OutputStream outputStream,
+                              final Context context) throws IOException {
+        this.logger = context.getLogger();


this whole function should be surrounded in try/catch/finally and return some response to CloudFormation to know when it fails (like with an NPE or some other bug of ours), and record metrics (I'd say metrics stuff can be done in a later PR, but I think the try/catch and write response should be in this one)

I've raised #8 to do this later, once the interface back to the caller is cleaned up a bit.

churn

…brianlao-github-fort Fix resource targetting for a stack level hook

rjlohan added the enhancement New feature or request label Nov 16, 2018

rjlohan requested review from aygold92, johnttompkins and tobywf November 16, 2018 21:47

tobywf previously requested changes Nov 16, 2018

View reviewed changes

johnttompkins reviewed Nov 16, 2018

View reviewed changes

src/com/aws/cfn/LambdaWrapper.java Outdated Show resolved Hide resolved

src/com/aws/cfn/LambdaWrapper.java Show resolved Hide resolved

rjlohan force-pushed the initial-lambda-wrapper-2 branch 2 times, most recently from 433368c to 6432429 Compare November 16, 2018 23:09

aygold92 reviewed Nov 16, 2018

View reviewed changes

rjlohan force-pushed the initial-lambda-wrapper-2 branch from 6432429 to a83bb79 Compare November 16, 2018 23:15

johnttompkins approved these changes Nov 19, 2018

View reviewed changes

aygold92 suggested changes Nov 19, 2018

View reviewed changes

aygold92 approved these changes Nov 19, 2018

View reviewed changes

rjlohan merged commit 7f465b4 into master Nov 19, 2018

tobywf deleted the initial-lambda-wrapper-2 branch November 27, 2018 22:13

brianlaoaws added a commit to brianlaoaws/cloudformation-cli-java-plugin that referenced this pull request Nov 25, 2024

Merge pull request aws-cloudformation#4 from brianlaoaws/master-from-…

a74a040

…brianlao-github-fort Fix resource targetting for a stack level hook

Conversation

rjlohan commented Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobywf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aygold92 Nov 16, 2018 • edited by rjlohan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aygold92 Nov 16, 2018 • edited by rjlohan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aygold92 Nov 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aygold92 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aygold92 Nov 19, 2018 • edited by rjlohan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rjlohan commented Nov 16, 2018 •

edited

Loading

aygold92 Nov 16, 2018 •

edited by rjlohan

Loading

aygold92 Nov 16, 2018 •

edited by rjlohan

Loading

aygold92 Nov 16, 2018 •

edited

Loading

aygold92 Nov 19, 2018 •

edited by rjlohan

Loading