From e3ae645f636a9f08566435799b7f55d50f5298bb Mon Sep 17 00:00:00 2001 From: Elad Ben-Israel Date: Tue, 4 Aug 2020 18:02:15 +0300 Subject: [PATCH] chore: do not use "synthesize" and "prepare" in the cdk (#9410) In 2.x we plan to deprecate support for the `synthesize()` and `prepare()` hooks in `Construct`. See [RFC] for motivation. This change does not remove support for these hooks, but it does remove any usage of these hooks from the AWS Construct Library. - aws-apigateway: the calculated logical ID of Deployment resources is now done through a Lazy instead of in `prepare()`. - aws-lambda: the calculated logical ID of Version resources is now done through a Lazy instead of in `prepare()`. - core: `Stack.synthesize()` is now called `_synthesizeTemplate()` and is explicitly called from `app.synth()`. - core: `TreeEtadata.synthesize()` is now called `_synthesizeTree()` and is explicitly called from `app.synth()`. The logical IDs of Lambda Version and API Gateway Deployment resources will be different after this change due to a latent bug in the previous implementation. The `prepare()` methods are called _before_ resource dependencies are resolved, which means that the hash in the logical ID did not include dependencies. Now it includes dependencies and therefore these IDs have changed. Since both of these resources are stateless, this does not introduce risk to production systems. See more details [here]. Furthermore: all calls to `ConstructNode.prepare()` were converted to `app.synth()`. Related: https://github.com/aws/aws-cdk-rfcs/issues/192 [RFC]: https://github.com/aws/aws-cdk-rfcs/blob/master/text/0192-remove-constructs-compat.md [here]: https://github.com/aws/aws-cdk/pull/9410#issuecomment-668552361 BREAKING CHANGE: `lambda.Version` and `apigateway.Deployment` resources with auto-generated IDs will be replaced as we fixed a bug which ignored resource dependencies when generating these logical IDs. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../@aws-cdk/aws-apigateway/lib/deployment.ts | 52 +++++++++---------- .../integ.restapi.multistack.expected.json | 4 +- .../aws-apigateway/test/test.deployment.ts | 8 +-- .../aws-cloudfront/test/distribution.test.ts | 4 +- .../cloudformation/test.pipeline-actions.ts | 2 +- .../@aws-cdk/aws-ec2/test/connections.test.ts | 8 +-- packages/@aws-cdk/aws-lambda/lib/function.ts | 29 +++++------ .../test/integ.current-version.expected.json | 6 +-- .../aws-lambda/test/test.lambda-version.ts | 2 +- .../test/notifications.test.ts | 2 - .../@aws-cdk/core/lib/private/synthesis.ts | 22 ++++++-- .../core/lib/private/tree-metadata.ts | 6 ++- packages/@aws-cdk/core/lib/stack.ts | 48 +++++++++-------- packages/@aws-cdk/core/test/test.stack.ts | 21 +++++++- 14 files changed, 124 insertions(+), 90 deletions(-) diff --git a/packages/@aws-cdk/aws-apigateway/lib/deployment.ts b/packages/@aws-cdk/aws-apigateway/lib/deployment.ts index c72dba724f878..cf2f159025288 100644 --- a/packages/@aws-cdk/aws-apigateway/lib/deployment.ts +++ b/packages/@aws-cdk/aws-apigateway/lib/deployment.ts @@ -123,7 +123,7 @@ interface LatestDeploymentResourceProps { class LatestDeploymentResource extends CfnDeployment { private hashComponents = new Array(); - private originalLogicalId: string; + private api: IRestApi; constructor(scope: Construct, id: string, props: LatestDeploymentResourceProps) { @@ -133,7 +133,31 @@ class LatestDeploymentResource extends CfnDeployment { }); this.api = props.restApi; - this.originalLogicalId = Stack.of(this).getLogicalId(this); + + const originalLogicalId = Stack.of(this).getLogicalId(this); + + this.overrideLogicalId(Lazy.stringValue({ produce: ctx => { + const hash = [ ...this.hashComponents ]; + + if (this.api instanceof RestApi || this.api instanceof SpecRestApi) { // Ignore IRestApi that are imported + + // Add CfnRestApi to the logical id so a new deployment is triggered when any of its properties change. + const cfnRestApiCF = (this.api.node.defaultChild as any)._toCloudFormation(); + hash.push(ctx.resolve(cfnRestApiCF)); + } + + let lid = originalLogicalId; + + // if hash components were added to the deployment, we use them to calculate + // a logical ID for the deployment resource. + if (hash.length > 0) { + const md5 = crypto.createHash('md5'); + hash.map(x => ctx.resolve(x)).forEach(c => md5.update(JSON.stringify(c))); + lid += md5.digest('hex'); + } + + return lid; + }})); } /** @@ -149,28 +173,4 @@ class LatestDeploymentResource extends CfnDeployment { this.hashComponents.push(data); } - - /** - * Hooks into synthesis to calculate a logical ID that hashes all the components - * add via `addToLogicalId`. - */ - protected prepare() { - if (this.api instanceof RestApi || this.api instanceof SpecRestApi) { // Ignore IRestApi that are imported - - // Add CfnRestApi to the logical id so a new deployment is triggered when any of its properties change. - const cfnRestApiCF = (this.api.node.defaultChild as any)._toCloudFormation(); - this.addToLogicalId(Stack.of(this).resolve(cfnRestApiCF)); - } - - const stack = Stack.of(this); - - // if hash components were added to the deployment, we use them to calculate - // a logical ID for the deployment resource. - if (this.hashComponents.length > 0) { - const md5 = crypto.createHash('md5'); - this.hashComponents.map(x => stack.resolve(x)).forEach(c => md5.update(JSON.stringify(c))); - this.overrideLogicalId(this.originalLogicalId + md5.digest('hex')); - } - super.prepare(); - } } diff --git a/packages/@aws-cdk/aws-apigateway/test/integ.restapi.multistack.expected.json b/packages/@aws-cdk/aws-apigateway/test/integ.restapi.multistack.expected.json index 92ca8ad632bfc..5aa57732955bc 100644 --- a/packages/@aws-cdk/aws-apigateway/test/integ.restapi.multistack.expected.json +++ b/packages/@aws-cdk/aws-apigateway/test/integ.restapi.multistack.expected.json @@ -120,7 +120,7 @@ "BooksApi60AC975F" ] }, - "BooksApiDeployment86CA39AFc1570c78b1ea90526c0309cd74b7b8d0": { + "BooksApiDeployment86CA39AF4ff82f86c127f53c9de94d266b1906be": { "Type": "AWS::ApiGateway::Deployment", "Properties": { "RestApiId": { @@ -141,7 +141,7 @@ "Ref": "BooksApi60AC975F" }, "DeploymentId": { - "Ref": "BooksApiDeployment86CA39AFc1570c78b1ea90526c0309cd74b7b8d0" + "Ref": "BooksApiDeployment86CA39AF4ff82f86c127f53c9de94d266b1906be" }, "StageName": "prod" } diff --git a/packages/@aws-cdk/aws-apigateway/test/test.deployment.ts b/packages/@aws-cdk/aws-apigateway/test/test.deployment.ts index 47118594132a5..0f5ce4a37732d 100644 --- a/packages/@aws-cdk/aws-apigateway/test/test.deployment.ts +++ b/packages/@aws-cdk/aws-apigateway/test/test.deployment.ts @@ -150,16 +150,16 @@ export = { // the logical ID changed const template = synthesize(); test.ok(!template.Resources.deployment33381975bba46c5132329b81e7befcbbba5a0e75, 'old resource id is not deleted'); - test.ok(template.Resources.deployment33381975075f46a4503208d69fcffed2f263c48c, - `new resource deployment33381975075f46a4503208d69fcffed2f263c48c is not created, instead found ${Object.keys(template.Resources)}`); + test.ok(template.Resources.deployment333819758aa4cdb9d204502b959c4903f4d5d29f, + `new resource deployment333819758aa4cdb9d204502b959c4903f4d5d29f is not created, instead found ${Object.keys(template.Resources)}`); // tokens supported, and are resolved upon synthesis const value = 'hello hello'; deployment.addToLogicalId({ foo: Lazy.stringValue({ produce: () => value }) }); const template2 = synthesize(); - test.ok(template2.Resources.deployment33381975b6d7672e4c9afd0b741e41d07739786b, - `resource deployment33381975b6d7672e4c9afd0b741e41d07739786b not found, instead found ${Object.keys(template2.Resources)}`); + test.ok(template2.Resources.deployment333819758d91bed959c6bd6268ba84f6d33e888e, + `resource deployment333819758d91bed959c6bd6268ba84f6d33e888e not found, instead found ${Object.keys(template2.Resources)}`); test.done(); diff --git a/packages/@aws-cdk/aws-cloudfront/test/distribution.test.ts b/packages/@aws-cdk/aws-cloudfront/test/distribution.test.ts index e4f91e7bcd58d..29942711ff781 100644 --- a/packages/@aws-cdk/aws-cloudfront/test/distribution.test.ts +++ b/packages/@aws-cdk/aws-cloudfront/test/distribution.test.ts @@ -307,7 +307,7 @@ describe('with Lambda@Edge functions', () => { { EventType: 'origin-request', LambdaFunctionARN: { - Ref: 'FunctionCurrentVersion4E2B22619c0305f954e58f25575548280c0a3629', + Ref: 'FunctionCurrentVersion4E2B2261477a5ae8059bbaa7813f752292c0f65e', }, }, ], @@ -341,7 +341,7 @@ describe('with Lambda@Edge functions', () => { { EventType: 'viewer-request', LambdaFunctionARN: { - Ref: 'FunctionCurrentVersion4E2B22619c0305f954e58f25575548280c0a3629', + Ref: 'FunctionCurrentVersion4E2B2261477a5ae8059bbaa7813f752292c0f65e', }, }, ], diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/cloudformation/test.pipeline-actions.ts b/packages/@aws-cdk/aws-codepipeline-actions/test/cloudformation/test.pipeline-actions.ts index 9a6b99351ee7b..043e42952e7c0 100644 --- a/packages/@aws-cdk/aws-codepipeline-actions/test/cloudformation/test.pipeline-actions.ts +++ b/packages/@aws-cdk/aws-codepipeline-actions/test/cloudformation/test.pipeline-actions.ts @@ -26,7 +26,7 @@ export = nodeunit.testCase({ actions: [action], }); - cdk.ConstructNode.prepare(stack.node); + app.synth(); _assertPermissionGranted(test, stack, pipelineRole.statements, 'iam:PassRole', action.deploymentRole.roleArn); diff --git a/packages/@aws-cdk/aws-ec2/test/connections.test.ts b/packages/@aws-cdk/aws-ec2/test/connections.test.ts index 9c30e5f104cda..591d543827084 100644 --- a/packages/@aws-cdk/aws-ec2/test/connections.test.ts +++ b/packages/@aws-cdk/aws-ec2/test/connections.test.ts @@ -1,5 +1,5 @@ import { expect, haveResource } from '@aws-cdk/assert'; -import { App, ConstructNode, Stack } from '@aws-cdk/core'; +import { App, Stack } from '@aws-cdk/core'; import { nodeunitShim, Test } from 'nodeunit-shim'; import { @@ -185,7 +185,7 @@ nodeunitShim({ sg2.connections.allowFrom(sg1, Port.tcp(100)); // THEN -- both rules are in Stack2 - ConstructNode.prepare(app.node); + app.synth(); expect(stack2).to(haveResource('AWS::EC2::SecurityGroupIngress', { GroupId: { 'Fn::GetAtt': [ 'SecurityGroupDD263621', 'GroupId' ] }, @@ -216,7 +216,7 @@ nodeunitShim({ sg2.connections.allowTo(sg1, Port.tcp(100)); // THEN -- both rules are in Stack2 - ConstructNode.prepare(app.node); + app.synth(); expect(stack2).to(haveResource('AWS::EC2::SecurityGroupIngress', { GroupId: { 'Fn::ImportValue': 'Stack1:ExportsOutputFnGetAttSecurityGroupDD263621GroupIdDF6F8B09' }, @@ -249,7 +249,7 @@ nodeunitShim({ sg2.connections.allowFrom(sg1b, Port.tcp(100)); // THEN -- both egress rules are in Stack2 - ConstructNode.prepare(app.node); + app.synth(); expect(stack2).to(haveResource('AWS::EC2::SecurityGroupEgress', { GroupId: { 'Fn::ImportValue': 'Stack1:ExportsOutputFnGetAttSecurityGroupAED40ADC5GroupId1D10C76A' }, diff --git a/packages/@aws-cdk/aws-lambda/lib/function.ts b/packages/@aws-cdk/aws-lambda/lib/function.ts index d450e85ca913a..92b6d64774751 100644 --- a/packages/@aws-cdk/aws-lambda/lib/function.ts +++ b/packages/@aws-cdk/aws-lambda/lib/function.ts @@ -332,6 +332,18 @@ export class Function extends FunctionBase { ...this.currentVersionOptions, }); + // override the version's logical ID with a lazy string which includes the + // hash of the function itself, so a new version resource is created when + // the function configuration changes. + const cfn = this._currentVersion.node.defaultChild as CfnResource; + const originalLogicalId = this.stack.resolve(cfn.logicalId) as string; + + cfn.overrideLogicalId(Lazy.stringValue({ produce: _ => { + const hash = calculateFunctionHash(this); + const logicalId = trimFromStart(originalLogicalId, 255 - 32); + return `${logicalId}${hash}`; + }})); + return this._currentVersion; } @@ -739,23 +751,6 @@ export class Function extends FunctionBase { return this._logGroup; } - protected prepare() { - super.prepare(); - - // if we have a current version resource, override it's logical id - // so that it includes the hash of the function code and it's configuration. - if (this._currentVersion) { - const stack = Stack.of(this); - const cfn = this._currentVersion.node.defaultChild as CfnResource; - const originalLogicalId: string = stack.resolve(cfn.logicalId); - - const hash = calculateFunctionHash(this); - - const logicalId = trimFromStart(originalLogicalId, 255 - 32); - cfn.overrideLogicalId(`${logicalId}${hash}`); - } - } - private renderEnvironment() { if (!this.environment || Object.keys(this.environment).length === 0) { return undefined; diff --git a/packages/@aws-cdk/aws-lambda/test/integ.current-version.expected.json b/packages/@aws-cdk/aws-lambda/test/integ.current-version.expected.json index 1877a683a1740..bf269c8f6d555 100644 --- a/packages/@aws-cdk/aws-lambda/test/integ.current-version.expected.json +++ b/packages/@aws-cdk/aws-lambda/test/integ.current-version.expected.json @@ -85,7 +85,7 @@ "MyLambdaServiceRole4539ECB6" ] }, - "MyLambdaCurrentVersionE7A382CC86b18af374d6e380aa07074d2490e2df": { + "MyLambdaCurrentVersionE7A382CC721de083c6b4b6360a9c534b79eb610e": { "Type": "AWS::Lambda::Version", "Properties": { "FunctionName": { @@ -103,7 +103,7 @@ }, "Qualifier": { "Fn::GetAtt": [ - "MyLambdaCurrentVersionE7A382CC86b18af374d6e380aa07074d2490e2df", + "MyLambdaCurrentVersionE7A382CC721de083c6b4b6360a9c534b79eb610e", "Version" ] }, @@ -118,7 +118,7 @@ }, "FunctionVersion": { "Fn::GetAtt": [ - "MyLambdaCurrentVersionE7A382CC86b18af374d6e380aa07074d2490e2df", + "MyLambdaCurrentVersionE7A382CC721de083c6b4b6360a9c534b79eb610e", "Version" ] }, diff --git a/packages/@aws-cdk/aws-lambda/test/test.lambda-version.ts b/packages/@aws-cdk/aws-lambda/test/test.lambda-version.ts index 69e0f62e9c668..b7f283164b715 100644 --- a/packages/@aws-cdk/aws-lambda/test/test.lambda-version.ts +++ b/packages/@aws-cdk/aws-lambda/test/test.lambda-version.ts @@ -134,7 +134,7 @@ export = { }, 'FunctionVersion': { 'Fn::GetAtt': [ - 'FnCurrentVersion17A89ABB19ed45993ff69fd011ae9fd4ab6e2005', + 'FnCurrentVersion17A89ABBab5c765f3c55e4e61583b51b00a95742', 'Version', ], }, diff --git a/packages/@aws-cdk/aws-s3-notifications/test/notifications.test.ts b/packages/@aws-cdk/aws-s3-notifications/test/notifications.test.ts index 7a5a3cf25b96d..cadf2609692f0 100644 --- a/packages/@aws-cdk/aws-s3-notifications/test/notifications.test.ts +++ b/packages/@aws-cdk/aws-s3-notifications/test/notifications.test.ts @@ -290,8 +290,6 @@ test('a notification destination can specify a set of dependencies that must be bucket.addObjectCreatedNotification(dest); - cdk.ConstructNode.prepare(stack.node); - expect(SynthUtils.synthesize(stack).template.Resources.BucketNotifications8F2E257D).toEqual({ Type: 'Custom::S3BucketNotifications', Properties: { diff --git a/packages/@aws-cdk/core/lib/private/synthesis.ts b/packages/@aws-cdk/core/lib/private/synthesis.ts index 34aa158740242..296ba171efde9 100644 --- a/packages/@aws-cdk/core/lib/private/synthesis.ts +++ b/packages/@aws-cdk/core/lib/private/synthesis.ts @@ -1,8 +1,10 @@ import * as cxapi from '@aws-cdk/cx-api'; import * as constructs from 'constructs'; import { Construct, IConstruct, SynthesisOptions, ValidationError } from '../construct-compat'; +import { Stack } from '../stack'; import { Stage, StageSynthesisOptions } from '../stage'; import { prepareApp } from './prepare-app'; +import { TreeMetadata } from './tree-metadata'; export function synthesize(root: IConstruct, options: SynthesisOptions = { }): cxapi.CloudAssembly { // we start by calling "synth" on all nested assemblies (which will take care of all their children) @@ -103,10 +105,22 @@ function prepareTree(root: IConstruct) { * Stop at Assembly boundaries. */ function synthesizeTree(root: IConstruct, builder: cxapi.CloudAssemblyBuilder) { - visit(root, 'post', construct => construct.onSynthesize({ - outdir: builder.outdir, - assembly: builder, - })); + visit(root, 'post', construct => { + const session = { + outdir: builder.outdir, + assembly: builder, + }; + + if (construct instanceof Stack) { + construct._synthesizeTemplate(session); + } else if (construct instanceof TreeMetadata) { + construct._synthesizeTree(session); + } + + // this will soon be deprecated and removed in 2.x + // see https://github.com/aws/aws-cdk-rfcs/issues/192 + construct.onSynthesize(session); + }); } /** diff --git a/packages/@aws-cdk/core/lib/private/tree-metadata.ts b/packages/@aws-cdk/core/lib/private/tree-metadata.ts index a40481faa9b2d..b2f288d97b4b4 100644 --- a/packages/@aws-cdk/core/lib/private/tree-metadata.ts +++ b/packages/@aws-cdk/core/lib/private/tree-metadata.ts @@ -20,7 +20,11 @@ export class TreeMetadata extends Construct { super(scope, 'Tree'); } - protected synthesize(session: ISynthesisSession) { + /** + * Create tree.json + * @internal + */ + public _synthesizeTree(session: ISynthesisSession) { const lookup: { [path: string]: Node } = { }; const visit = (construct: IConstruct): Node => { diff --git a/packages/@aws-cdk/core/lib/stack.ts b/packages/@aws-cdk/core/lib/stack.ts index c7c20e8bb0c73..3f79c43aec18e 100644 --- a/packages/@aws-cdk/core/lib/stack.ts +++ b/packages/@aws-cdk/core/lib/stack.ts @@ -700,6 +700,32 @@ export class Stack extends Construct implements ITaggable { } } + /** + * Synthesizes the cloudformation template into a cloud assembly. + * @internal + */ + public _synthesizeTemplate(session: ISynthesisSession): void { + // In principle, stack synthesis is delegated to the + // StackSynthesis object. + // + // However, some parts of synthesis currently use some private + // methods on Stack, and I don't really see the value in refactoring + // this right now, so some parts still happen here. + const builder = session.assembly; + + // write the CloudFormation template as a JSON file + const outPath = path.join(builder.outdir, this.templateFile); + const text = JSON.stringify(this._toCloudFormation(), undefined, 2); + fs.writeFileSync(outPath, text); + + for (const ctx of this._missingContext) { + builder.addMissing(ctx); + } + + // Delegate adding artifacts to the Synthesizer + this.synthesizer.synthesizeStackArtifacts(session); + } + /** * Returns the naming scheme used to allocate logical IDs. By default, uses * the `HashedAddressingScheme` but this method can be overridden to customize @@ -761,28 +787,6 @@ export class Stack extends Construct implements ITaggable { } } - protected synthesize(session: ISynthesisSession): void { - // In principle, stack synthesis is delegated to the - // StackSynthesis object. - // - // However, some parts of synthesis currently use some private - // methods on Stack, and I don't really see the value in refactoring - // this right now, so some parts still happen here. - const builder = session.assembly; - - // write the CloudFormation template as a JSON file - const outPath = path.join(builder.outdir, this.templateFile); - const text = JSON.stringify(this._toCloudFormation(), undefined, 2); - fs.writeFileSync(outPath, text); - - for (const ctx of this._missingContext) { - builder.addMissing(ctx); - } - - // Delegate adding artifacts to the Synthesizer - this.synthesizer.synthesizeStackArtifacts(session); - } - /** * Returns the CloudFormation template for this stack by traversing * the tree and invoking _toCloudFormation() on all Entity objects. diff --git a/packages/@aws-cdk/core/test/test.stack.ts b/packages/@aws-cdk/core/test/test.stack.ts index 7ae1adc077798..c6a3be31af013 100644 --- a/packages/@aws-cdk/core/test/test.stack.ts +++ b/packages/@aws-cdk/core/test/test.stack.ts @@ -2,7 +2,7 @@ import * as cxapi from '@aws-cdk/cx-api'; import { Test } from 'nodeunit'; import { App, CfnCondition, CfnInclude, CfnOutput, CfnParameter, - CfnResource, Construct, Lazy, ScopedAws, Stack, Tag, validateString } from '../lib'; + CfnResource, Construct, Lazy, ScopedAws, Stack, Tag, validateString, ISynthesisSession } from '../lib'; import { Intrinsic } from '../lib/private/intrinsic'; import { resolveReferences } from '../lib/private/refs'; import { PostResolveToken } from '../lib/util'; @@ -868,6 +868,25 @@ export = { test.done(); }, + + 'users can (still) override "synthesize()" in stack'(test: Test) { + let called = false; + + class MyStack extends Stack { + synthesize(session: ISynthesisSession) { + called = true; + test.ok(session.outdir); + test.equal(session.assembly.outdir, session.outdir); + } + } + + const app = new App(); + new MyStack(app, 'my-stack'); + + app.synth(); + test.ok(called, 'synthesize() not called for Stack'); + test.done(); + }, }; class StackWithPostProcessor extends Stack {