chore: do not use "synthesize" and "prepare" in the cdk (aws#9410)

Elad Ben-Israel · Curtis Eppel · commit 8a5d7f4b6836 · 2020-08-11T12:00:10.000+10:00
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: aws/aws-cdk-rfcs#192 [RFC]: https://github.com/aws/aws-cdk-rfcs/blob/master/text/0192-remove-constructs-compat.md [here]: aws#9410 (comment) 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*
diff --git 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<any>();
-  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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
