From 7db44e9975e246ffe045961b95b5d78cf7537136 Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Fri, 8 Jan 2021 10:38:44 +0000 Subject: [PATCH 1/4] feat(lambda): AssetCode instance can be used in multiple stacks Although the constructor of the AssetCode doesn't imply it's real dependencies, an AssetCode currently can only be associated to consumers within a single stack. Attempting to use them across stacks results in the conspicuous error - "Cannot reference across apps. Consuming and producing stacks must be defined within the same CDK app." The correct solution here would have been to model this such that the stack dependency is made explicit on the customer facing API, perhaps by taking a 'scope' parameter. This solves the problem by caching the bound asset per stack. --- packages/@aws-cdk/aws-lambda/lib/code.ts | 26 +++++++++++------ .../@aws-cdk/aws-lambda/test/code.test.ts | 28 +++++++++++++++++++ 2 files changed, 45 insertions(+), 9 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda/lib/code.ts b/packages/@aws-cdk/aws-lambda/lib/code.ts index fef200a9a6c9c..36efa797fd8e3 100644 --- a/packages/@aws-cdk/aws-lambda/lib/code.ts +++ b/packages/@aws-cdk/aws-lambda/lib/code.ts @@ -232,7 +232,7 @@ export class InlineCode extends Code { */ export class AssetCode extends Code { public readonly isInline = false; - private asset?: s3_assets.Asset; + private assets: Record = {}; /** * @param path The path to the asset file or directory. @@ -242,35 +242,43 @@ export class AssetCode extends Code { } public bind(scope: cdk.Construct): CodeConfig { - // If the same AssetCode is used multiple times, retain only the first instantiation. - if (!this.asset) { - this.asset = new s3_assets.Asset(scope, 'Code', { + const stackPath = cdk.Stack.of(scope).node.path; // use path instead of name to account for Stage. + let asset: s3_assets.Asset; + if (stackPath in this.assets) { + // If the same AssetCode is used multiple times, retain only the first instantiation. + asset = this.assets[stackPath]; + } else { + asset = new s3_assets.Asset(scope, 'Code', { path: this.path, ...this.options, }); + this.assets[stackPath] = asset; } - if (!this.asset.isZipArchive) { + if (!asset.isZipArchive) { throw new Error(`Asset must be a .zip file or a directory (${this.path})`); } return { s3Location: { - bucketName: this.asset.s3BucketName, - objectKey: this.asset.s3ObjectKey, + bucketName: asset.s3BucketName, + objectKey: asset.s3ObjectKey, }, }; } public bindToResource(resource: cdk.CfnResource, options: ResourceBindOptions = { }) { - if (!this.asset) { + const stackPath = cdk.Stack.of(resource).node.path; + const asset = this.assets[stackPath]; + + if (!asset) { throw new Error('bindToResource() must be called after bind()'); } const resourceProperty = options.resourceProperty || 'Code'; // https://github.com/aws/aws-cdk/issues/1432 - this.asset.addResourceMetadata(resource, resourceProperty); + asset.addResourceMetadata(resource, resourceProperty); } } diff --git a/packages/@aws-cdk/aws-lambda/test/code.test.ts b/packages/@aws-cdk/aws-lambda/test/code.test.ts index a822ba698697e..5d949a0d8e0d9 100644 --- a/packages/@aws-cdk/aws-lambda/test/code.test.ts +++ b/packages/@aws-cdk/aws-lambda/test/code.test.ts @@ -77,6 +77,34 @@ describe('code', () => { }, }, ResourcePart.CompleteDefinition); }); + + test('can be used in multiple stacks', () => { + // GIVEN + const app = new cdk.App(); + const stack1 = new cdk.Stack(app, 'Stack1'); + const stack2 = new cdk.Stack(app, 'Stack2'); + const asset = lambda.Code.fromAsset(path.join(__dirname, 'my-lambda-handler')); + + // WHEN + new lambda.Function(stack1, 'Func', { + code: asset, + runtime: lambda.Runtime.NODEJS_12_X, + handler: 'foom', + }); + new lambda.Function(stack2, 'Func', { + code: asset, + runtime: lambda.Runtime.NODEJS_12_X, + handler: 'foom', + }); + + // THEN + const assembly = app.synth(); + const synthesized1 = assembly.getStackArtifact(stack1.artifactId); + const synthesized2 = assembly.getStackArtifact(stack2.artifactId); + + expect(synthesized1.assets.length).toEqual(1); + expect(synthesized2.assets.length).toEqual(1); + }); }); describe('lambda.Code.fromCfnParameters', () => { From 38169eb807bbdda5725279c61d72d4637241adcf Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Mon, 11 Jan 2021 11:17:15 +0000 Subject: [PATCH 2/4] Revert "feat(lambda): AssetCode instance can be used in multiple stacks" This reverts commit 7db44e9975e246ffe045961b95b5d78cf7537136. --- packages/@aws-cdk/aws-lambda/lib/code.ts | 26 ++++++----------- .../@aws-cdk/aws-lambda/test/code.test.ts | 28 ------------------- 2 files changed, 9 insertions(+), 45 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda/lib/code.ts b/packages/@aws-cdk/aws-lambda/lib/code.ts index 36efa797fd8e3..fef200a9a6c9c 100644 --- a/packages/@aws-cdk/aws-lambda/lib/code.ts +++ b/packages/@aws-cdk/aws-lambda/lib/code.ts @@ -232,7 +232,7 @@ export class InlineCode extends Code { */ export class AssetCode extends Code { public readonly isInline = false; - private assets: Record = {}; + private asset?: s3_assets.Asset; /** * @param path The path to the asset file or directory. @@ -242,43 +242,35 @@ export class AssetCode extends Code { } public bind(scope: cdk.Construct): CodeConfig { - const stackPath = cdk.Stack.of(scope).node.path; // use path instead of name to account for Stage. - let asset: s3_assets.Asset; - if (stackPath in this.assets) { - // If the same AssetCode is used multiple times, retain only the first instantiation. - asset = this.assets[stackPath]; - } else { - asset = new s3_assets.Asset(scope, 'Code', { + // If the same AssetCode is used multiple times, retain only the first instantiation. + if (!this.asset) { + this.asset = new s3_assets.Asset(scope, 'Code', { path: this.path, ...this.options, }); - this.assets[stackPath] = asset; } - if (!asset.isZipArchive) { + if (!this.asset.isZipArchive) { throw new Error(`Asset must be a .zip file or a directory (${this.path})`); } return { s3Location: { - bucketName: asset.s3BucketName, - objectKey: asset.s3ObjectKey, + bucketName: this.asset.s3BucketName, + objectKey: this.asset.s3ObjectKey, }, }; } public bindToResource(resource: cdk.CfnResource, options: ResourceBindOptions = { }) { - const stackPath = cdk.Stack.of(resource).node.path; - const asset = this.assets[stackPath]; - - if (!asset) { + if (!this.asset) { throw new Error('bindToResource() must be called after bind()'); } const resourceProperty = options.resourceProperty || 'Code'; // https://github.com/aws/aws-cdk/issues/1432 - asset.addResourceMetadata(resource, resourceProperty); + this.asset.addResourceMetadata(resource, resourceProperty); } } diff --git a/packages/@aws-cdk/aws-lambda/test/code.test.ts b/packages/@aws-cdk/aws-lambda/test/code.test.ts index 5d949a0d8e0d9..a822ba698697e 100644 --- a/packages/@aws-cdk/aws-lambda/test/code.test.ts +++ b/packages/@aws-cdk/aws-lambda/test/code.test.ts @@ -77,34 +77,6 @@ describe('code', () => { }, }, ResourcePart.CompleteDefinition); }); - - test('can be used in multiple stacks', () => { - // GIVEN - const app = new cdk.App(); - const stack1 = new cdk.Stack(app, 'Stack1'); - const stack2 = new cdk.Stack(app, 'Stack2'); - const asset = lambda.Code.fromAsset(path.join(__dirname, 'my-lambda-handler')); - - // WHEN - new lambda.Function(stack1, 'Func', { - code: asset, - runtime: lambda.Runtime.NODEJS_12_X, - handler: 'foom', - }); - new lambda.Function(stack2, 'Func', { - code: asset, - runtime: lambda.Runtime.NODEJS_12_X, - handler: 'foom', - }); - - // THEN - const assembly = app.synth(); - const synthesized1 = assembly.getStackArtifact(stack1.artifactId); - const synthesized2 = assembly.getStackArtifact(stack2.artifactId); - - expect(synthesized1.assets.length).toEqual(1); - expect(synthesized2.assets.length).toEqual(1); - }); }); describe('lambda.Code.fromCfnParameters', () => { From 98b1bc13e2196134dddf29faef793d00bbb85d00 Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Mon, 11 Jan 2021 11:24:18 +0000 Subject: [PATCH 3/4] throw an error instead --- packages/@aws-cdk/aws-lambda/lib/code.ts | 2 ++ .../@aws-cdk/aws-lambda/test/code.test.ts | 20 +++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/packages/@aws-cdk/aws-lambda/lib/code.ts b/packages/@aws-cdk/aws-lambda/lib/code.ts index fef200a9a6c9c..b2c7407f904fe 100644 --- a/packages/@aws-cdk/aws-lambda/lib/code.ts +++ b/packages/@aws-cdk/aws-lambda/lib/code.ts @@ -248,6 +248,8 @@ export class AssetCode extends Code { path: this.path, ...this.options, }); + } else if (cdk.Stack.of(this.asset) !== cdk.Stack.of(scope)) { + throw new Error(`Asset is already associated with another stack '${cdk.Stack.of(this.asset).stackName}'.`); } if (!this.asset.isZipArchive) { diff --git a/packages/@aws-cdk/aws-lambda/test/code.test.ts b/packages/@aws-cdk/aws-lambda/test/code.test.ts index a822ba698697e..9b99c095c2467 100644 --- a/packages/@aws-cdk/aws-lambda/test/code.test.ts +++ b/packages/@aws-cdk/aws-lambda/test/code.test.ts @@ -77,6 +77,26 @@ describe('code', () => { }, }, ResourcePart.CompleteDefinition); }); + + test('fails if asset is bound with a second stack', () => { + // GIVEN + const asset = lambda.Code.fromAsset(path.join(__dirname, 'my-lambda-handler')); + + const app = new cdk.App(); + const stack1 = new cdk.Stack(app, 'Stack1'); + new lambda.Function(stack1, 'Func', { + code: asset, + runtime: lambda.Runtime.NODEJS_10_X, + handler: 'foom', + }); + + const stack2 = new cdk.Stack(app, 'Stack2'); + expect(() => new lambda.Function(stack2, 'Func', { + code: asset, + runtime: lambda.Runtime.NODEJS_10_X, + handler: 'foom', + })).toThrow(/already associated/); + }); }); describe('lambda.Code.fromCfnParameters', () => { From 6be0a4e2ecb9e17304a9c46bbb23db0065c335ad Mon Sep 17 00:00:00 2001 From: Niranjan Jayakar Date: Mon, 11 Jan 2021 14:43:26 +0000 Subject: [PATCH 4/4] better message --- packages/@aws-cdk/aws-lambda/lib/code.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-lambda/lib/code.ts b/packages/@aws-cdk/aws-lambda/lib/code.ts index b2c7407f904fe..b2323008bb5fd 100644 --- a/packages/@aws-cdk/aws-lambda/lib/code.ts +++ b/packages/@aws-cdk/aws-lambda/lib/code.ts @@ -249,7 +249,8 @@ export class AssetCode extends Code { ...this.options, }); } else if (cdk.Stack.of(this.asset) !== cdk.Stack.of(scope)) { - throw new Error(`Asset is already associated with another stack '${cdk.Stack.of(this.asset).stackName}'.`); + throw new Error(`Asset is already associated with another stack '${cdk.Stack.of(this.asset).stackName}'. ` + + 'Create a new Code instance for every stack.'); } if (!this.asset.isZipArchive) {