From 3d4fcb5ab72ca0777f3abfa2c4aa10e0d7deba6b Mon Sep 17 00:00:00 2001 From: Elad Ben-Israel Date: Tue, 14 Jul 2020 14:05:15 +0300 Subject: [PATCH] chore: prepare Remove usage of the `prepare()` hook from across the library. --- .../@aws-cdk/aws-apigateway/lib/deployment.ts | 85 ++++++++----------- .../@aws-cdk/aws-apigateway/lib/restapi.ts | 13 +++ .../integ.restapi.multistack.expected.json | 4 +- .../aws-apigateway/test/test.deployment.ts | 8 +- packages/@aws-cdk/aws-iam/lib/policy.ts | 32 ++++--- packages/@aws-cdk/aws-lambda/lib/function.ts | 33 +++---- packages/@aws-cdk/pipelines/lib/pipeline.ts | 9 +- packages/@aws-cdk/pipelines/lib/stage.ts | 6 +- 8 files changed, 95 insertions(+), 95 deletions(-) diff --git a/packages/@aws-cdk/aws-apigateway/lib/deployment.ts b/packages/@aws-cdk/aws-apigateway/lib/deployment.ts index c0c013d3f60ff..07dd38fe49781 100644 --- a/packages/@aws-cdk/aws-apigateway/lib/deployment.ts +++ b/packages/@aws-cdk/aws-apigateway/lib/deployment.ts @@ -78,6 +78,15 @@ export class Deployment extends Resource { this.api = props.api; this.deploymentId = Lazy.stringValue({ produce: () => this.resource.ref }); + + // 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. + if (this.api instanceof RestApi) { + this.node.addDependency(this.api._methodsDependencyGroup); + } } /** @@ -91,30 +100,6 @@ export class Deployment extends Resource { public addToLogicalId(data: any) { this.resource.addToLogicalId(data); } - - /** - * Hook into synthesis before it occurs and make any final adjustments. - */ - 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)); - } - } } interface LatestDeploymentResourceProps { @@ -124,7 +109,7 @@ interface LatestDeploymentResourceProps { class LatestDeploymentResource extends CfnDeployment { private hashComponents = new Array(); - private originalLogicalId: string; + private api: IRestApi; constructor(scope: Construct, id: string, props: LatestDeploymentResourceProps) { @@ -134,7 +119,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; + }})); } /** @@ -150,28 +159,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/lib/restapi.ts b/packages/@aws-cdk/aws-apigateway/lib/restapi.ts index 1d3eff6619328..4de5db061b084 100644 --- a/packages/@aws-cdk/aws-apigateway/lib/restapi.ts +++ b/packages/@aws-cdk/aws-apigateway/lib/restapi.ts @@ -1,6 +1,7 @@ import { IVpcEndpoint } from '@aws-cdk/aws-ec2'; import * as iam from '@aws-cdk/aws-iam'; import { CfnOutput, IResource as IResourceBase, Resource, Stack } from '@aws-cdk/core'; +import { Construct, DependencyGroup } from 'constructs'; import { ApiDefinition } from './api-definition'; import { ApiKey, ApiKeyOptions, IApiKey } from './api-key'; import { CfnAccount, CfnRestApi } from './apigateway.generated'; @@ -543,9 +544,17 @@ export class RestApi extends RestApiBase { */ public readonly methods = new Array(); + /** + * A dependency group that represents all methods attached to this API GW (lazy). + * @internal + */ + public readonly _methodsDependencyGroup: DependencyGroup; + constructor(scope: Construct, id: string, props: RestApiProps = { }) { super(scope, id, props); + this._methodsDependencyGroup = new DependencyGroup(); + const resource = new CfnRestApi(this, 'Resource', { name: this.physicalName, description: props.description, @@ -622,6 +631,10 @@ export class RestApi extends RestApiBase { */ public _attachMethod(method: Method) { this.methods.push(method); + + // we want to take a direct dependency on the CfnMethod resource because + // Method itself has other children and depending on them will cause cyclic deps. + this._methodsDependencyGroup.add(method.node.findChild('Resource')); } /** 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-iam/lib/policy.ts b/packages/@aws-cdk/aws-iam/lib/policy.ts index 94f97958af715..91c4f0f541f5c 100644 --- a/packages/@aws-cdk/aws-iam/lib/policy.ts +++ b/packages/@aws-cdk/aws-iam/lib/policy.ts @@ -1,4 +1,5 @@ -import { Construct, IResource, Lazy, Resource } from '@aws-cdk/core'; +import { IResource, Lazy, Resource } from '@aws-cdk/core'; +import { Construct, Dependable, IDependable } from 'constructs'; import { IGroup } from './group'; import { CfnPolicy } from './iam.generated'; import { PolicyDocument } from './policy-document'; @@ -124,7 +125,18 @@ export class Policy extends Resource implements IPolicy { Lazy.stringValue({ produce: () => generatePolicyName(scope, resource.logicalId) }), }); - const resource = new CfnPolicy(this, 'Resource', { + // 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. + const shouldExist = () => this.force || this.referenceTaken || (!this.document.isEmpty && this.isAttached); + + class CfnPolicyConditional extends CfnPolicy implements IDependable { + protected toCloudFormation(): object { + return shouldExist() ? super.toCloudFormation() : {}; + } + } + + const resource = new CfnPolicyConditional(this, 'Resource', { policyDocument: this.document, policyName: this.physicalName, roles: undefinedIfEmpty(() => this.roles.map(r => r.roleName)), @@ -132,6 +144,14 @@ export class Policy extends Resource implements IPolicy { groups: undefinedIfEmpty(() => this.groups.map(g => g.groupName)), }); + // implement custom Dependable logic which returns an empty array if the + // policy document is empty (and force is `false`). + Dependable.implement(this, { + get dependencyRoots() { + return shouldExist() ? [resource] : []; + }, + }); + this._policyName = this.physicalName!; this.force = props.force !== undefined ? props.force : false; @@ -224,14 +244,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/aws-lambda/lib/function.ts b/packages/@aws-cdk/aws-lambda/lib/function.ts index dbd77a2a9d793..b1d9b304219ab 100644 --- a/packages/@aws-cdk/aws-lambda/lib/function.ts +++ b/packages/@aws-cdk/aws-lambda/lib/function.ts @@ -3,7 +3,8 @@ import * as ec2 from '@aws-cdk/aws-ec2'; import * as iam from '@aws-cdk/aws-iam'; import * as logs from '@aws-cdk/aws-logs'; import * as sqs from '@aws-cdk/aws-sqs'; -import { CfnResource, Construct, Duration, Fn, Lazy, Stack } from '@aws-cdk/core'; +import { CfnResource, Duration, Fn, Lazy, Stack } from '@aws-cdk/core'; +import { Construct } from 'constructs'; import { Code, CodeConfig } from './code'; import { EventInvokeConfigOptions } from './event-invoke-config'; import { IEventSource } from './event-source'; @@ -296,7 +297,6 @@ export interface FunctionProps extends FunctionOptions { * library. */ export class Function extends FunctionBase { - /** * Returns a `lambda.Version` which represents the current version of this * Lambda function. A new version will be created every time the function's @@ -315,6 +315,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: string = Stack.of(this).resolve(cfn.logicalId); + + cfn.overrideLogicalId(Lazy.stringValue({ produce: _ => { + const hash = calculateFunctionHash(this); + const logicalId = trimFromStart(originalLogicalId, 255 - 32); + return `${logicalId}${hash}`; + }})); + return this._currentVersion; } @@ -700,23 +712,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/pipelines/lib/pipeline.ts b/packages/@aws-cdk/pipelines/lib/pipeline.ts index 45f7cae6d2925..7fe245c32b80e 100644 --- a/packages/@aws-cdk/pipelines/lib/pipeline.ts +++ b/packages/@aws-cdk/pipelines/lib/pipeline.ts @@ -104,6 +104,7 @@ export class CdkPipeline extends Construct { }); this.node.addValidation({ validate: () => this.validatePipeline() }); + Aspects.of(this).apply({ visit: () => this._assets.removeAssetsStageIfEmpty() }); } /** @@ -180,14 +181,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 index 437a7c0ff8668..6119da97669a1 100644 --- a/packages/@aws-cdk/pipelines/lib/stage.ts +++ b/packages/@aws-cdk/pipelines/lib/stage.ts @@ -1,6 +1,6 @@ import * as codepipeline from '@aws-cdk/aws-codepipeline'; import * as cpactions from '@aws-cdk/aws-codepipeline-actions'; -import { Construct, Stage } from '@aws-cdk/core'; +import { Stage, Aspects } from '@aws-cdk/core'; import * as cxapi from '@aws-cdk/cx-api'; import { Construct } from 'constructs'; import { AssetType, DeployCdkStackAction } from './actions'; @@ -55,6 +55,8 @@ export class CdkStage extends Construct { this.pipelineStage = props.pipelineStage; this.cloudAssemblyArtifact = props.cloudAssemblyArtifact; this.host = props.host; + + Aspects.of(this).apply({ visit: () => this.prepare() }); } /** @@ -176,7 +178,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 prepare() { // 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.