Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 17 commits into from
Aug 19, 2020
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions packages/@aws-cdk/aws-cloudfront/lib/private/cache-behavior.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { Lazy } from '@aws-cdk/core';
import { CfnDistribution } from '../cloudfront.generated';
import { AddBehaviorOptions, ViewerProtocolPolicy } from '../distribution';

Expand Down Expand Up @@ -48,15 +49,10 @@ export class CacheBehavior {
smoothStreaming: this.props.smoothStreaming,
viewerProtocolPolicy: this.props.viewerProtocolPolicy ?? ViewerProtocolPolicy.ALLOW_ALL,
lambdaFunctionAssociations: this.props.edgeLambdas
? this.props.edgeLambdas.map(edgeLambda => {
if (edgeLambda.functionVersion.version === '$LATEST') {
throw new Error('$LATEST function version cannot be used for Lambda@Edge');
}
return {
lambdaFunctionArn: edgeLambda.functionVersion.functionArn,
eventType: edgeLambda.eventType.toString(),
};
})
? this.props.edgeLambdas.map(edgeLambda => ({
lambdaFunctionArn: Lazy.stringValue({ produce: () => edgeLambda.functionVersion.edgeArn }),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed better, 👍

eventType: edgeLambda.eventType.toString(),
}))
: undefined,
};
}
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-cloudfront/lib/web_distribution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -894,7 +894,7 @@ export class CloudFrontWebDistribution extends cdk.Resource implements IDistribu
lambdaFunctionAssociations: input.lambdaFunctionAssociations
.map(fna => ({
eventType: fna.eventType,
lambdaFunctionArn: fna.lambdaFunction && fna.lambdaFunction.functionArn,
lambdaFunctionArn: fna.lambdaFunction && cdk.Lazy.stringValue({ produce: () => fna.lambdaFunction.edgeArn }),
})),
});

Expand Down
80 changes: 66 additions & 14 deletions packages/@aws-cdk/aws-cloudfront/test/distribution.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -500,20 +500,72 @@ describe('with Lambda@Edge functions', () => {
});
});

test('fails creation when attempting to add the $LATEST function version as an edge Lambda to the default behavior', () => {
expect(() => {
new Distribution(stack, 'MyDist', {
defaultBehavior: {
origin,
edgeLambdas: [
{
functionVersion: lambdaFunction.latestVersion,
eventType: LambdaEdgeEventType.ORIGIN_RESPONSE,
},
],
},
});
}).toThrow(/\$LATEST function version cannot be used for Lambda@Edge/);
test('fails synthesis when attempting to add the $LATEST function version as an edge Lambda to the default behavior', () => {
new Distribution(stack, 'MyDist', {
defaultBehavior: {
origin,
edgeLambdas: [
{
functionVersion: lambdaFunction.latestVersion,
eventType: LambdaEdgeEventType.ORIGIN_RESPONSE,
},
],
},
});
expect(() => app.synth()).toThrow(/\$LATEST function version cannot be used for Lambda@Edge/);
});

test('with removable env vars', () => {
const envLambdaFunction = new lambda.Function(stack, 'EnvFunction', {
runtime: lambda.Runtime.NODEJS,
code: lambda.Code.fromInline('whateverwithenv'),
handler: 'index.handler',
});
envLambdaFunction.addEnvironment('KEY', 'value', { removeInEdge: true });

new Distribution(stack, 'MyDist', {
defaultBehavior: {
origin,
edgeLambdas: [
{
functionVersion: envLambdaFunction.currentVersion,
eventType: LambdaEdgeEventType.ORIGIN_REQUEST,
},
],
},
});

expect(stack).toHaveResource('AWS::Lambda::Function', {
Environment: ABSENT,
Code: {
ZipFile: 'whateverwithenv',
},
});
});

test('with incompatible env vars', () => {
const envLambdaFunction = new lambda.Function(stack, 'EnvFunction', {
runtime: lambda.Runtime.NODEJS,
code: lambda.Code.fromInline('whateverwithenv'),
handler: 'index.handler',
environment: {
KEY: 'value',
},
});

new Distribution(stack, 'MyDist', {
defaultBehavior: {
origin,
edgeLambdas: [
{
functionVersion: envLambdaFunction.currentVersion,
eventType: LambdaEdgeEventType.ORIGIN_REQUEST,
},
],
},
});

expect(() => app.synth()).toThrow(/KEY/);
});
});

Expand Down
91 changes: 80 additions & 11 deletions packages/@aws-cdk/aws-cloudfront/test/web_distribution.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { expect, haveResource, haveResourceLike } from '@aws-cdk/assert';
import { ABSENT, expect, haveResource, haveResourceLike } from '@aws-cdk/assert';
import * as certificatemanager from '@aws-cdk/aws-certificatemanager';
import * as lambda from '@aws-cdk/aws-lambda';
import * as s3 from '@aws-cdk/aws-s3';
Expand Down Expand Up @@ -426,8 +426,7 @@ nodeunitShim({
const stack = new cdk.Stack();
const sourceBucket = new s3.Bucket(stack, 'Bucket');

const lambdaFunction = new lambda.SingletonFunction(stack, 'Lambda', {
Copy link
Contributor Author

@jogold jogold Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

uuid: 'xxxx-xxxx-xxxx-xxxx',
const lambdaFunction = new lambda.Function(stack, 'Lambda', {
code: lambda.Code.inline('foo'),
handler: 'index.handler',
runtime: lambda.Runtime.NODEJS_10_X,
Expand All @@ -444,7 +443,7 @@ nodeunitShim({
isDefaultBehavior: true,
lambdaFunctionAssociations: [{
eventType: LambdaEdgeEventType.ORIGIN_REQUEST,
lambdaFunction: lambdaFunction.latestVersion,
lambdaFunction: lambdaFunction.addVersion('1'),
}],
},
],
Expand All @@ -459,13 +458,7 @@ nodeunitShim({
{
'EventType': 'origin-request',
'LambdaFunctionARN': {
'Fn::Join': [
'',
[
{ 'Fn::GetAtt': [ 'SingletonLambdaxxxxxxxxxxxxxxxx69D4268A', 'Arn' ] },
':$LATEST',
],
],
'Ref': 'LambdaVersion1BB7548E1',
},
},
],
Expand All @@ -476,6 +469,82 @@ nodeunitShim({
test.done();
},

'associate a lambda with removable env vars'(test: Test) {
const app = new cdk.App();
const stack = new cdk.Stack(app, 'Stack');
const sourceBucket = new s3.Bucket(stack, 'Bucket');

const lambdaFunction = new lambda.Function(stack, 'Lambda', {
code: lambda.Code.inline('foo'),
handler: 'index.handler',
runtime: lambda.Runtime.NODEJS_10_X,
});
lambdaFunction.addEnvironment('KEY', 'value', { removeInEdge: true });

new CloudFrontWebDistribution(stack, 'AnAmazingWebsiteProbably', {
originConfigs: [
{
s3OriginSource: {
s3BucketSource: sourceBucket,
},
behaviors: [
{
isDefaultBehavior: true,
lambdaFunctionAssociations: [{
eventType: LambdaEdgeEventType.ORIGIN_REQUEST,
lambdaFunction: lambdaFunction.addVersion('1'),
}],
},
],
},
],
});

expect(stack).to(haveResource('AWS::Lambda::Function', {
Environment: ABSENT,
}));

test.done();
},

'throws when associating a lambda with incompatible env vars'(test: Test) {
const app = new cdk.App();
const stack = new cdk.Stack(app, 'Stack');
const sourceBucket = new s3.Bucket(stack, 'Bucket');

const lambdaFunction = new lambda.Function(stack, 'Lambda', {
code: lambda.Code.inline('foo'),
handler: 'index.handler',
runtime: lambda.Runtime.NODEJS_10_X,
environment: {
KEY: 'value',
},
});

new CloudFrontWebDistribution(stack, 'AnAmazingWebsiteProbably', {
originConfigs: [
{
s3OriginSource: {
s3BucketSource: sourceBucket,
},
behaviors: [
{
isDefaultBehavior: true,
lambdaFunctionAssociations: [{
eventType: LambdaEdgeEventType.ORIGIN_REQUEST,
lambdaFunction: lambdaFunction.addVersion('1'),
}],
},
],
},
],
});

test.throws(() => app.synth(), /KEY/);

test.done();
},

'distribution has a defaultChild'(test: Test) {
const stack = new cdk.Stack();
const sourceBucket = new s3.Bucket(stack, 'Bucket');
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-lambda-nodejs/lib/function.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export class NodejsFunction extends lambda.Function {

// Enable connection reuse for aws-sdk
if (props.awsSdkConnectionReuse ?? true) {
this.addEnvironment('AWS_NODEJS_CONNECTION_REUSE_ENABLED', '1');
this.addEnvironment('AWS_NODEJS_CONNECTION_REUSE_ENABLED', '1', { removeInEdge: true });
}
} finally {
// We can only restore after the code has been bound to the function
Expand Down
13 changes: 13 additions & 0 deletions packages/@aws-cdk/aws-lambda/lib/function-base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,11 @@ export interface IFunction extends IResource, ec2.IConnectable, iam.IGrantable {
* Configures options for asynchronous invocation.
*/
configureAsyncInvoke(options: EventInvokeConfigOptions): void;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@nija-at nija-at Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jogold jogold Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jogold jogold Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jogold jogold Aug 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
Expand Down Expand Up @@ -318,6 +323,10 @@ export abstract class FunctionBase extends Resource implements IFunction {
});
}

public checkEdgeCompatibility(): void {
return;
}

/**
* Returns the construct tree node that corresponds to the lambda function.
* For use internally for constructs, when the tree is set up in non-standard ways. Ex: SingletonFunction.
Expand Down Expand Up @@ -417,4 +426,8 @@ class LatestVersion extends FunctionBase implements IVersion {
public addAlias(aliasName: string, options: AliasOptions = {}) {
return addAlias(this, this, aliasName, options);
}

public get edgeArn(): never {
throw new Error('$LATEST function version cannot be used for Lambda@Edge');
}
}
Loading