fix(lambda-nodejs): NodejsFunction construct incompatible with lambda@edge #9562

jogold · 2020-08-10T11:23:20Z

Check version and function compatibility when a Lambda is used for
Lambda@Edge. Environment variables can be marked as "removable" when
used for Lambda@Edge.

Closes #9328
Closes #9453

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

…at edge Automatically remove environment variables of a Lambda function when it's used at edge. A warning is also shown. Works with singleton functions. Closes aws#9328 Closes aws#9453

nija-at · 2020-08-11T08:35:20Z

packages/@aws-cdk/aws-cloudfront/lib/private/cache-behavior.ts

+          if (edgeLambda.functionVersion.lambda.removeEnvironment()) {
+            edgeLambda.functionVersion.lambda.node.addWarning(`Removed environment variables from function ${edgeLambda.functionVersion.lambda.node.path} because Lambda@Edge does not support environment variables`);
+          }


This is going to surprise customers who are not aware of this limitation and expect their environment variables to show up.

How about removing environment variables that the CDK inserts, and throwing an error when the customer has environment variables configured?

Also (FYI), the addWarning API was just deprecated yesterday (#9563) as part of prep work for CDK 2.0 (Design).

In the future, you'd use:

Suggested change

if (edgeLambda.functionVersion.lambda.removeEnvironment()) {

edgeLambda.functionVersion.lambda.node.addWarning(`Removed environment variables from function ${edgeLambda.functionVersion.lambda.node.path} because Lambda@Edge does not support environment variables`);

}

if (edgeLambda.functionVersion.lambda.removeEnvironment()) {

Annotations.of(edgeLambda.functionVersion.lambda).addWarning(`Removed environment variables from function ${edgeLambda.functionVersion.lambda.node.path} because Lambda@Edge does not support environment variables`);

}

@nija-at
This means that we need to identify env vars inserted by CDK? You would like to add a special method (addCdkEnvironment()) and prop (cdkEnvironment) to identify those? Doesn't it require "developer discipline" inside the CDK?

Also consider the following use case outside the CDK: you have a construct that creates your special Lambda with a defined lists of env vars (whether used in the runtime code or not, like STAGE for example) and other good defaults. You want to use it both at edge and not.

@njlynch
saw #9563 but unfortunately Annotations has not been exported in core/lib/index.ts in this PR. Will be done when all node.addXxx() methods will be updated in the constructs library I presume.

That's fair @jogold.

In your example, if I understand the lambda@edge limitation correctly, the special lambda function in the construct may not work at edge if it relies on certain env vars being present.

In this case, does it make sense instead to have a property allowInEdgeEnv or edgeEnvOptimized or something similar (false by default), that strips out env vars when used in edge environment, and errors out otherwise?

This can be used to encode similar behaviour for other lambda@edge limitations.

@nija-at how about the following?

add a get edgeArn() getter on IVersion: in there we can check that the version is "compatible" for Lambda@Edge and throw otherwise, let's start with env vars only in this PR (+ the check that it's not $LATEST)

call this get edgeArn() in aws-cloudfront instead of doing lambdaFunctionArn: edgeLambda.functionVersion.functionArn => lambdaFunctionArn: edgeLambda.functionVersion.edgeArn.

add a third parameter allowEdgeRemoval to addEnvironment() so that we can loop over the env vars in get edgeArn(), remove the env vars that we're allowed to and throw if anything remains (with a clear message saying that addEnvironment() can be used to overcome it). allowEdgeRemoval will be set to true in the call to addEnvironment() in aws-lambda-nodejs

This approach looks reasonable to me.

Minor tweaks - make the third parameter to addEnvironment() - EnvironmentOptions - which holds the property removeInEdge (or something like that).

Pull request has been modified.

packages/@aws-cdk/aws-lambda/lib/function.ts

nija-at · 2020-08-17T13:18:46Z

packages/@aws-cdk/aws-lambda/lib/function-base.ts

+  /**
+   * Checks whether this function is compatible for Lambda@Edge.
+   */
+  checkEdgeCompatibility(): void;


Why have you added this API to IFunction? Is there value in exposing this to users?

Because the underlying Lambda of a version is an IFunction. For the same reason, I had to make checkEdgeCompatibility() public. Do you see a workaround?

Sorry, I was not able to follow your response. Can you expand a bit more?

If we wanted it to be on a base class so that it's available on all implementations, we could make it a protected abstract method on FunctionBase and mark it as @internal?
At the calling point check on instanceof and call this internal method. We do that in a few places in the CDK, like here.

OK with instanceof I can move this method to Function but it will remain public since I need to call it from a Version on its underlying Lambda?

We don't need this method on FunctionBase because an imported Lambda will always be considered to be compatible?

If it's moved to FunctionBase and marked @internal, keeping it as public is fine.

Also do you prefer the Symbol implementation (vs instanceof Function)?

That's my preference but not strongly.
The value of the symbol based implementation will diminish over the next few months with our upcoming move to monolithic packaging.

That's my preference but not strongly.

Went for the instanceof implementation because FunctionBase has currently no constructor and didn't want to add one.

_checkEdgeCompatibility() is on FunctionBase to handle singleton functions.

packages/@aws-cdk/aws-lambda/lib/function.ts

nija-at · 2020-08-17T13:24:03Z

packages/@aws-cdk/aws-lambda/lib/function.ts

@@ -760,27 +762,43 @@ export class Function extends FunctionBase {
    return this._logGroup;
  }

+  public checkEdgeCompatibility(): void {


Make this private instead?

packages/@aws-cdk/aws-lambda/lib/function.ts

Co-authored-by: Niranjan Jayakar <nija@amazon.com>

Pull request has been modified.

jogold · 2020-08-18T10:58:12Z

packages/@aws-cdk/aws-cloudfront/test/web_distribution.test.ts

@@ -426,8 +426,7 @@ nodeunitShim({
    const stack = new cdk.Stack();
    const sourceBucket = new s3.Bucket(stack, 'Bucket');

-    const lambdaFunction = new lambda.SingletonFunction(stack, 'Lambda', {


Had to move from a SingletonFunction to Function because SingletonFunction has no addVersion() it's a FunctionBase. This test was actually incorrect because it associated a latest version with a CF distribution.

SingletonFunction now has a _checkEdgeCompatibility() but no addVersion() yet, for this PR?

nija-at

Thanks @jogold - looks good. Just a couple of things below.

I've also reworded the commit title to focus on the NodejsFunction construct since that seems to be the primary benefit of this change.

nija-at · 2020-08-19T10:18:22Z

packages/@aws-cdk/aws-cloudfront/lib/private/cache-behavior.ts

-          };
-        })
+        ? this.props.edgeLambdas.map(edgeLambda => ({
+          lambdaFunctionArn: Lazy.stringValue({ produce: () => edgeLambda.functionVersion.edgeArn }),


edgeArn must always be lazily processed. Would be better to move the Lazy.stringValue() block into the getter.

Indeed better, 👍

packages/@aws-cdk/aws-lambda/lib/function-base.ts

Pull request has been modified.

aws-cdk-automation · 2020-08-19T14:14:42Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: deed339
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-08-19T14:15:13Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…@edge (aws#9562) Check version and function compatibility when a Lambda is used for Lambda@Edge. Environment variables can be marked as "removable" when used for Lambda@Edge. Closes aws#9328 Closes aws#9453 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Add `currentVersion` for singleton functions. This makes it possible to use them for Lambda@Edge. Also remove deprecated calls to `addVersion()` introduced in aws#9562.

Add `currentVersion` for singleton functions. This makes it possible to use them for Lambda@Edge. To achieve this, make `ensureLambda()` return a `Function` and not an `IFunction` (which now allows to remove the default implementation of `_checkEdgeCompatibilty()` in `FunctionBase`). Also remove deprecated calls to `addVersion()` introduced in #9562. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

jogold added 3 commits August 10, 2020 13:23

fix(cloudfront,lambda): remove env vars when Lambda function is used …

625d49b

…at edge Automatically remove environment variables of a Lambda function when it's used at edge. A warning is also shown. Works with singleton functions. Closes aws#9328 Closes aws#9453

Merge branch 'master' into lambda-edge-remove-env

0bde106

warning on version

5dcc8d5

SomayaB assigned nija-at Aug 10, 2020

nija-at previously requested changes Aug 11, 2020

View reviewed changes

nija-at assigned njlynch Aug 11, 2020

jogold added 2 commits August 13, 2020 15:13

Merge branch 'master' into lambda-edge-remove-env

3f55a28

edgeArn

7a05847

jogold changed the title ~~fix(cloudfront,lambda): remove env vars when Lambda function is used at edge~~ fix(cloudfront,lambda): version and function compatibility with Lambda@Edge Aug 13, 2020

remove dev dep

1ab542b

nija-at previously requested changes Aug 17, 2020

View reviewed changes

Apply suggestions from code review

b538779

Co-authored-by: Niranjan Jayakar <nija@amazon.com>

jogold added 6 commits August 17, 2020 16:03

Merge branch 'master' into lambda-edge-remove-env

9465e60

review feedback

2ff9840

env

c1dc747

fix test

5455085

Merge branch 'master' into lambda-edge-remove-env

006206b

_checkEdgeCompatibility() on FunctionBase

1ce04e1

jogold commented Aug 18, 2020

View reviewed changes

jogold mentioned this pull request Aug 19, 2020

docs(lambda-nodejs): Add note on usage as lambda@edge #9453

Closed

nija-at changed the title ~~fix(cloudfront,lambda): version and function compatibility with Lambda@Edge~~ fix(lambda-nodejs): NodejsFunction construct incompatible with lambda@edge Aug 19, 2020

nija-at previously requested changes Aug 19, 2020

View reviewed changes

jogold added 2 commits August 19, 2020 14:55

Merge branch 'master' into lambda-edge-remove-env

efd2327

move Lazy to getter

6c9fdc7

jogold added 2 commits August 19, 2020 15:43

Merge branch 'master' into lambda-edge-remove-env

b878a9b

minimize changes

deed339

nija-at approved these changes Aug 19, 2020

View reviewed changes

mergify bot merged commit dfe2c5c into aws:master Aug 19, 2020

jogold deleted the lambda-edge-remove-env branch August 19, 2020 14:15

jogold mentioned this pull request Aug 19, 2020

[aws-lambda,aws-cloudfront] Lambda@Edge compatibilty #9833

Closed

2 tasks

jogold mentioned this pull request Aug 21, 2020

chore(lambda): current version for singleton functions #9892

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(lambda-nodejs): NodejsFunction construct incompatible with lambda@edge #9562

fix(lambda-nodejs): NodejsFunction construct incompatible with lambda@edge #9562

jogold commented Aug 10, 2020 •

edited

Loading

nija-at Aug 11, 2020

njlynch Aug 11, 2020

jogold Aug 11, 2020

nija-at Aug 11, 2020

jogold Aug 12, 2020

nija-at Aug 13, 2020 •

edited

Loading

nija-at Aug 17, 2020

jogold Aug 17, 2020

nija-at Aug 18, 2020 •

edited

Loading

jogold Aug 18, 2020 •

edited

Loading

nija-at Aug 18, 2020

jogold Aug 18, 2020 •

edited

Loading

nija-at Aug 18, 2020

jogold Aug 18, 2020 •

edited

Loading

nija-at Aug 17, 2020

jogold Aug 18, 2020 •

edited

Loading

nija-at left a comment

nija-at Aug 19, 2020

jogold Aug 19, 2020

aws-cdk-automation commented Aug 19, 2020

mergify bot commented Aug 19, 2020

fix(lambda-nodejs): NodejsFunction construct incompatible with lambda@edge #9562

fix(lambda-nodejs): NodejsFunction construct incompatible with lambda@edge #9562

Conversation

jogold commented Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

jogold Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold Aug 18, 2020 • edited Loading

Choose a reason for hiding this comment

nija-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Aug 19, 2020

AWS CodeBuild CI Report

mergify bot commented Aug 19, 2020

jogold commented Aug 10, 2020 •

edited

Loading

nija-at Aug 13, 2020 •

edited

Loading

nija-at Aug 18, 2020 •

edited

Loading

jogold Aug 18, 2020 •

edited

Loading

jogold Aug 18, 2020 •

edited

Loading

jogold Aug 18, 2020 •

edited

Loading

jogold Aug 18, 2020 •

edited

Loading