chore: remove an additional "prepare" usage (#9428)

Follow up on #9410 and remove a few additional usages of `prepare`: - In the API Gateway library we leveraged `prepare()` to record dependencies between all Deployment resources and all Method resources in the APIGW. The solution is to perform two-sided bookkeeping while methods/deployments are defined and record their dependencies in-band. We also refactored the way the code in `LatestDeployment` to be slightly more readable. - In the Pipelines library prepare was replaced with an aspect (which is technically the drop-in alternative to `prepare()` in v2.0, for lack of a better solution at the moment). - In the IAM library, the `Policy` resource needs to be conditionally created only if the document contains statements. To address that, we added a new protected API to `CfnResource` which is called `shouldSynthesize()`. By default it returns `true` but if it returns `false` (in a subclass), the resource will not be rendered into the cloudformation template. Related: aws/aws-cdk-rfcs#192 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws · Aug 5, 2020 · 53cb749 · 53cb749
1 parent 998c0f7
commit 53cb749
Show file tree

Hide file tree

Showing 8 changed files with 162 additions and 70 deletions.
diff --git a/packages/@aws-cdk/aws-apigateway/lib/deployment.ts b/packages/@aws-cdk/aws-apigateway/lib/deployment.ts
@@ -1,7 +1,8 @@
-import { CfnResource, Construct, Lazy, RemovalPolicy, Resource, Stack } from '@aws-cdk/core';
 import * as crypto from 'crypto';
+import { Construct, Lazy, RemovalPolicy, Resource, CfnResource } from '@aws-cdk/core';
 import { CfnDeployment } from './apigateway.generated';
-import { IRestApi, RestApi, SpecRestApi } from './restapi';
+import { IRestApi, RestApi, SpecRestApi, RestApiBase } from './restapi';
+import { Method } from './method';
 
 export interface DeploymentProps  {
   /**
@@ -77,6 +78,10 @@ export class Deployment extends Resource {
 
     this.api = props.api;
     this.deploymentId = Lazy.stringValue({ produce: () => this.resource.ref });
+
+    if (props.api instanceof RestApiBase) {
+      props.api._attachDeployment(this);
+    }
   }
 
   /**
@@ -92,27 +97,28 @@ export class Deployment extends Resource {
   }
 
   /**
-   * Hook into synthesis before it occurs and make any final adjustments.
+   * Quoting from CloudFormation's docs:
+   *
+   *   If you create an AWS::ApiGateway::RestApi resource and its methods (using
+   *   AWS::ApiGateway::Method) in the same template as your deployment, the
+   *   deployment must depend on the RestApi's methods. To create a dependency,
+   *   add a DependsOn attribute to the deployment. If you don't, AWS
+   *   CloudFormation creates the deployment right after it creates the RestApi
+   *   resource that doesn't contain any methods, and AWS CloudFormation
+   *   encounters the following error: The REST API doesn't contain any methods.
+   *
+   * @param method The method to add as a dependency of the deployment
+   * @see https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-apigateway-deployment.html
+   * @see https://github.com/aws/aws-cdk/pull/6165
+   * @internal
    */
-  protected prepare() {
-    if (this.api instanceof RestApi) {
-      // Ignore IRestApi that are imported
-
-      /*
-       * https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-apigateway-deployment.html
-       * Quoting from CloudFormation's docs - "If you create an AWS::ApiGateway::RestApi resource and its methods (using AWS::ApiGateway::Method) in
-       * the same template as your deployment, the deployment must depend on the RestApi's methods. To create a dependency, add a DependsOn attribute
-       * to the deployment. If you don't, AWS CloudFormation creates the deployment right after it creates the RestApi resource that doesn't contain
-       * any methods, and AWS CloudFormation encounters the following error: The REST API doesn't contain any methods."
-       */
-
-      /*
-       * Adding a dependency between LatestDeployment and Method construct, using ConstructNode.addDependencies(), creates additional dependencies
-       * between AWS::ApiGateway::Deployment and the AWS::Lambda::Permission nodes (children under Method), causing cyclic dependency errors. Hence,
-       * falling back to declaring dependencies between the underlying CfnResources.
-       */
-      this.api.methods.map(m => m.node.defaultChild as CfnResource).forEach(m => this.resource.addDependsOn(m));
-    }
+  public _addMethodDependency(method: Method) {
+    // adding a dependency between the constructs using `node.addDependency()`
+    // will create additional dependencies between `AWS::ApiGateway::Deployment`
+    // and the `AWS::Lambda::Permission` resources (children under Method),
+    // causing cyclic dependency errors. Hence, falling back to declaring
+    // dependencies between the underlying CfnResources.
+    this.node.addDependency(method.node.defaultChild as CfnResource);
   }
 }
 
@@ -122,9 +128,9 @@ interface LatestDeploymentResourceProps {
 }
 
 class LatestDeploymentResource extends CfnDeployment {
-  private hashComponents = new Array<any>();
-
-  private api: IRestApi;
+  private readonly hashComponents = new Array<any>();
+  private readonly originalLogicalId: string;
+  private readonly api: IRestApi;
 
   constructor(scope: Construct, id: string, props: LatestDeploymentResourceProps) {
     super(scope, id, {
@@ -133,31 +139,8 @@ class LatestDeploymentResource extends CfnDeployment {
     });
 
     this.api = props.restApi;
-
-    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;
-    }}));
+    this.originalLogicalId = this.stack.getLogicalId(this);
+    this.overrideLogicalId(Lazy.stringValue({ produce: () => this.calculateLogicalId() }));
   }
 
   /**
@@ -173,4 +156,27 @@ class LatestDeploymentResource extends CfnDeployment {
 
     this.hashComponents.push(data);
   }
+
+  private calculateLogicalId() {
+    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(this.stack.resolve(cfnRestApiCF));
+    }
+
+    let lid = this.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 => this.stack.resolve(x)).forEach(c => md5.update(JSON.stringify(c)));
+      lid += md5.digest('hex');
+    }
+
+    return lid;
+  }
 }
diff --git a/packages/@aws-cdk/aws-apigateway/lib/restapi.ts b/packages/@aws-cdk/aws-apigateway/lib/restapi.ts
@@ -247,7 +247,6 @@ export interface SpecRestApiProps extends RestApiBaseProps {
  * Base implementation that are common to various implementations of IRestApi
  */
 export abstract class RestApiBase extends Resource implements IRestApi {
-
   /**
    * Checks if the given object is an instance of RestApiBase.
    * @internal
@@ -383,6 +382,15 @@ export abstract class RestApiBase extends Resource implements IRestApi {
     ignore(method);
   }
 
+  /**
+   * Associates a Deployment resource with this REST API.
+   *
+   * @internal
+   */
+  public _attachDeployment(deployment: Deployment) {
+    ignore(deployment);
+  }
+
   protected configureCloudWatchRole(apiResource: CfnRestApi) {
     const role = new iam.Role(this, 'CloudWatchRole', {
       assumedBy: new iam.ServicePrincipal('apigateway.amazonaws.com'),
@@ -569,6 +577,11 @@ export class RestApi extends RestApiBase {
    */
   public readonly methods = new Array<Method>();
 
+  /**
+   * This list of deployments bound to this RestApi
+   */
+  private readonly deployments = new Array<Deployment>();
+
   constructor(scope: Construct, id: string, props: RestApiProps = { }) {
     super(scope, id, props);
 
@@ -646,6 +659,29 @@ export class RestApi extends RestApiBase {
    */
   public _attachMethod(method: Method) {
     this.methods.push(method);
+
+    // add this method as a dependency to all deployments defined for this api
+    // when additional deployments are added, _attachDeployment is called and
+    // this method will be added there.
+    for (const dep of this.deployments) {
+      dep._addMethodDependency(method);
+    }
+  }
+
+  /**
+   * Attaches a deployment to this REST API.
+   *
+   * @internal
+   */
+  public _attachDeployment(deployment: Deployment) {
+    this.deployments.push(deployment);
+
+    // add all methods that were already defined as dependencies of this
+    // deployment when additional methods are added, _attachMethod is called and
+    // it will be added as a dependency to this deployment.
+    for (const method of this.methods) {
+      deployment._addMethodDependency(method);
+    }
   }
 
   /**

diff --git a/packages/@aws-cdk/aws-iam/lib/policy.ts b/packages/@aws-cdk/aws-iam/lib/policy.ts
@@ -124,7 +124,20 @@ export class Policy extends Resource implements IPolicy {
         Lazy.stringValue({ produce: () => generatePolicyName(scope, resource.logicalId) }),
     });
 
-    const resource = new CfnPolicy(this, 'Resource', {
+    const self = this;
+
+    class CfnPolicyConditional extends CfnPolicy {
+      /**
+       * This function returns `true` if the CFN resource should be included in
+       * the cloudformation template unless `force` is `true`, if the policy
+       * document is empty, the resource will not be included.
+       */
+      protected shouldSynthesize() {
+        return self.force || self.referenceTaken || (!self.document.isEmpty && self.isAttached);
+      }
+    }
+
+    const resource = new CfnPolicyConditional(this, 'Resource', {
       policyDocument: this.document,
       policyName: this.physicalName,
       roles: undefinedIfEmpty(() => this.roles.map(r => r.roleName)),
@@ -222,14 +235,6 @@ export class Policy extends Resource implements IPolicy {
     return result;
   }
 
-  protected prepare() {
-    // Remove the resource if it shouldn't exist. This will prevent it from being rendered to the template.
-    const shouldExist = this.force || this.referenceTaken || (!this.document.isEmpty && this.isAttached);
-    if (!shouldExist) {
-      this.node.tryRemoveChild('Resource');
-    }
-  }
-
   /**
    * Whether the policy resource has been attached to any identity
    */

diff --git a/packages/@aws-cdk/core/lib/cfn-resource.ts b/packages/@aws-cdk/core/lib/cfn-resource.ts
@@ -233,6 +233,11 @@ export class CfnResource extends CfnRefElement {
    * and the dependency will automatically be transferred to the relevant scope.
    */
   public addDependsOn(target: CfnResource) {
+    // skip this dependency if the target is not part of the output
+    if (!target.shouldSynthesize()) {
+      return;
+    }
+
     addDependency(this, target, `"${this.node.path}" depends on "${target.node.path}"`);
   }
 
@@ -278,6 +283,10 @@ export class CfnResource extends CfnRefElement {
    * @internal
    */
   public _toCloudFormation(): object {
+    if (!this.shouldSynthesize()) {
+      return { };
+    }
+
     try {
       const ret = {
         Resources: {
@@ -362,6 +371,17 @@ export class CfnResource extends CfnRefElement {
   protected validateProperties(_properties: any) {
     // Nothing
   }
+
+  /**
+   * Can be overridden by subclasses to determine if this resource will be rendered
+   * into the cloudformation template.
+   *
+   * @returns `true` if the resource should be included or `false` is the resource
+   * should be omitted.
+   */
+  protected shouldSynthesize() {
+    return true;
+  }
 }
 
 export enum TagType {

diff --git a/packages/@aws-cdk/core/lib/nested-stack.ts b/packages/@aws-cdk/core/lib/nested-stack.ts
@@ -187,7 +187,7 @@ export class NestedStack extends Stack {
       return false;
     }
 
-    const cfn = JSON.stringify((this as any)._toCloudFormation());
+    const cfn = JSON.stringify(this._toCloudFormation());
     const templateHash = crypto.createHash('sha256').update(cfn).digest('hex');
 
     const templateLocation = this._parentStack.addFileAsset({

diff --git a/packages/@aws-cdk/core/test/test.cfn-resource.ts b/packages/@aws-cdk/core/test/test.cfn-resource.ts
@@ -97,4 +97,33 @@ export = nodeunit.testCase({
 
     test.done();
   },
+
+  'subclasses can override "shouldSynthesize" to lazy-determine if the resource should be included'(test: nodeunit.Test) {
+    // GIVEN
+    class HiddenCfnResource extends core.CfnResource {
+      protected shouldSynthesize() {
+        return false;
+      }
+    }
+
+    const app = new core.App();
+    const stack = new core.Stack(app, 'TestStack');
+    const subtree = new core.Construct(stack, 'subtree');
+
+    // WHEN
+    new HiddenCfnResource(subtree, 'R1', { type: 'Foo::R1' });
+    const r2 = new core.CfnResource(stack, 'R2', { type: 'Foo::R2' });
+
+    // also try to take a dependency on the parent of `r1` and expect the dependency not to materialize
+    r2.node.addDependency(subtree);
+
+    // THEN - only R2 is synthesized
+    test.deepEqual(app.synth().getStackByName(stack.stackName).template, {
+      Resources: { R2: { Type: 'Foo::R2' } },
+
+      // No DependsOn!
+    });
+
+    test.done();
+  },
 });
diff --git a/packages/@aws-cdk/pipelines/lib/pipeline.ts b/packages/@aws-cdk/pipelines/lib/pipeline.ts
@@ -102,6 +102,8 @@ export class CdkPipeline extends Construct {
       pipeline: this._pipeline,
       projectName: maybeSuffix(props.pipelineName, '-publish'),
     });
+
+    this.node.applyAspect({ visit: () => this._assets.removeAssetsStageIfEmpty() });
   }
 
   /**
@@ -178,14 +180,6 @@ export class CdkPipeline extends Construct {
     return ret;
   }
 
-  protected onPrepare() {
-    super.onPrepare();
-
-    // TODO: Support this in a proper way in the upstream library. For now, we
-    // "un-add" the Assets stage if it turns out to be empty.
-    this._assets.removeAssetsStageIfEmpty();
-  }
-
   /**
    * Return all StackDeployActions in an ordered list
    */

diff --git a/packages/@aws-cdk/pipelines/lib/stage.ts b/packages/@aws-cdk/pipelines/lib/stage.ts
@@ -54,6 +54,8 @@ export class CdkStage extends Construct {
     this.pipelineStage = props.pipelineStage;
     this.cloudAssemblyArtifact = props.cloudAssemblyArtifact;
     this.host = props.host;
+
+    this.node.applyAspect({ visit: () => this.prepareStage() });
   }
 
   /**
@@ -175,7 +177,7 @@ export class CdkStage extends Construct {
    * after creation, nor is there a way to specify relative priorities, which
    * is a limitation that we should take away in the base library.
    */
-  protected prepare() {
+  private prepareStage() {
     // FIXME: Make sure this only gets run once. There seems to be an issue in the reconciliation
     // loop that may trigger this more than once if it throws an error somewhere, and the exception
     // that gets thrown here will then override the actual failure.