diff --git a/packages/@aws-cdk/aws-apigateway/lib/deployment.ts b/packages/@aws-cdk/aws-apigateway/lib/deployment.ts index cf2f159025288..404ba1855bd68 100644 --- 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(); - - private api: IRestApi; + private readonly hashComponents = new Array(); + 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 index 41119d51f316e..f027a2f7424be 100644 --- 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(); + /** + * This list of deployments bound to this RestApi + */ + private readonly deployments = new Array(); + 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 index 6f852caec1a23..792f0f5861186 100644 --- 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 index 39b60a3d248b7..9af3aa5c16a21 100644 --- 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 index 4d87c148958f0..7e52427467039 100644 --- 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 index 41d360296ec1d..5033af4b21598 100644 --- 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 index 03120bf7c9866..f20021b34d1a8 100644 --- 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 index 2441da072cede..6b379f686fe2e 100644 --- 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.