From 08f9739fede99cacabc172039624fc8c567eac4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=91=A8=F0=9F=8F=BC=E2=80=8D=F0=9F=92=BB=20Romain=20M?= =?UTF-8?q?arcadier-Muller?= Date: Mon, 1 Jul 2019 15:28:02 +0200 Subject: [PATCH 1/2] fix(cli): Genertate CDK Metadata resource for region-independent stacks Fixes a bug that caused the CDK Metadata resource to not be emitted if a stack was not bound to a particular supported region. The fixed behavior will generate the resource for supported regions as well as for region independent stacks. Fixes #3142 --- packages/aws-cdk/lib/api/cxapp/stacks.ts | 2 +- packages/aws-cdk/test/api/test.stacks.ts | 94 +++++++++++++++++------- packages/aws-cdk/test/util.ts | 5 +- 3 files changed, 73 insertions(+), 28 deletions(-) diff --git a/packages/aws-cdk/lib/api/cxapp/stacks.ts b/packages/aws-cdk/lib/api/cxapp/stacks.ts index 3fd78ed659354..cbc12dfe1273b 100644 --- a/packages/aws-cdk/lib/api/cxapp/stacks.ts +++ b/packages/aws-cdk/lib/api/cxapp/stacks.ts @@ -223,7 +223,7 @@ export class AppStacks { if (!stack.template.Resources) { stack.template.Resources = {}; } - const resourcePresent = stack.environment.region === 'default-region' + const resourcePresent = stack.environment.region === cxapi.UNKNOWN_REGION || regionInfo.Fact.find(stack.environment.region, regionInfo.FactName.CDK_METADATA_RESOURCE_AVAILABLE) === 'YES'; if (resourcePresent) { if (!stack.template.Resources.CDKMetadata) { diff --git a/packages/aws-cdk/test/api/test.stacks.ts b/packages/aws-cdk/test/api/test.stacks.ts index ceb172ea18c9a..c5d4b69763ec5 100644 --- a/packages/aws-cdk/test/api/test.stacks.ts +++ b/packages/aws-cdk/test/api/test.stacks.ts @@ -1,28 +1,11 @@ import cxapi = require('@aws-cdk/cx-api'); -import { Test } from 'nodeunit'; +import { Test, testCase } from 'nodeunit'; import { SDK } from '../../lib'; import { AppStacks, DefaultSelection } from '../../lib/api/cxapp/stacks'; import { Configuration } from '../../lib/settings'; import { testAssembly } from '../util'; -const FIXED_RESULT = testAssembly({ - stackName: 'withouterrors', - template: { resource: 'noerrorresource' }, -}, -{ - stackName: 'witherrors', - template: { resource: 'errorresource' }, - metadata: { - '/resource': [ - { - type: cxapi.ERROR_METADATA_KEY, - data: 'this is an error' - } - ] - }, -}); - -export = { +export = testCase({ async 'do not throw when selecting stack without errors'(test: Test) { // GIVEN const stacks = testStacks(); @@ -95,14 +78,75 @@ export = { // THEN test.ok(thrown && thrown.includes('Since this app includes more than a single stack, specify which stacks to use (wildcards are supported)')); test.done(); - } + }, + + 'AWS::CDK::Metadata': { + async 'is generated for relocatable stacks'(test: Test) { + const stacks = testStacks({ env: `aws://${cxapi.UNKNOWN_ACCOUNT}/${cxapi.UNKNOWN_REGION}`, versionReporting: true }); + + const result = await stacks.synthesizeStack('withouterrors'); + const metadata = result.template.Resources && result.template.Resources.CDKMetadata; + test.deepEqual(metadata, { + Type: 'AWS::CDK::Metadata', + Properties: { + Modules: `${require('../../package.json').name}=${require('../../package.json').version}` + } + }); + + test.done(); + }, + + async 'is generated for stacks in supported regions'(test: Test) { + const stacks = testStacks({ env: 'aws://012345678912/us-east-1', versionReporting: true }); + + const result = await stacks.synthesizeStack('withouterrors'); + const metadata = result.template.Resources && result.template.Resources.CDKMetadata; + test.deepEqual(metadata, { + Type: 'AWS::CDK::Metadata', + Properties: { + Modules: `${require('../../package.json').name}=${require('../../package.json').version}` + } + }); + + test.done(); + }, + + async 'is not generated for stacks in unsupported regions'(test: Test) { + const stacks = testStacks({ env: 'aws://012345678912/bermuda-triangle-1337', versionReporting: true }); + + const result = await stacks.synthesizeStack('withouterrors'); + const metadata = result.template.Resources && result.template.Resources.CDKMetadata; + test.equal(metadata, undefined); + + test.done(); + } + }, +}); -}; +function testStacks({ env, versionReporting = true }: { env?: string, versionReporting?: boolean } = {}) { + const configuration = new Configuration(); + configuration.settings.set(['versionReporting'], versionReporting); -function testStacks() { return new AppStacks({ - configuration: new Configuration(), + configuration, aws: new SDK(), - synthesizer: async () => FIXED_RESULT, + synthesizer: async () => testAssembly({ + stackName: 'withouterrors', + env, + template: { resource: 'noerrorresource' }, + }, + { + stackName: 'witherrors', + env, + template: { resource: 'errorresource' }, + metadata: { + '/resource': [ + { + type: cxapi.ERROR_METADATA_KEY, + data: 'this is an error' + } + ] + }, + }), }); -} \ No newline at end of file +} diff --git a/packages/aws-cdk/test/util.ts b/packages/aws-cdk/test/util.ts index 2e1c11d97693b..710341356c526 100644 --- a/packages/aws-cdk/test/util.ts +++ b/packages/aws-cdk/test/util.ts @@ -5,6 +5,7 @@ import path = require('path'); export interface TestStackArtifact { stackName: string; template: any; + env?: string, depends?: string[]; metadata?: cxapi.StackMetadata; assets?: cxapi.AssetMetadataEntry[]; @@ -27,7 +28,7 @@ export function testAssembly(...stacks: TestStackArtifact[]): cxapi.CloudAssembl builder.addArtifact(stack.stackName, { type: cxapi.ArtifactType.AWS_CLOUDFORMATION_STACK, - environment: 'aws://12345/here', + environment: stack.env || 'aws://12345/here', dependencies: stack.depends, metadata, @@ -43,4 +44,4 @@ export function testAssembly(...stacks: TestStackArtifact[]): cxapi.CloudAssembl export function testStack(stack: TestStackArtifact) { const assembly = testAssembly(stack); return assembly.getStack(stack.stackName); -} \ No newline at end of file +} From a604330b0a2899e7ffe651b36764a88193328f9a Mon Sep 17 00:00:00 2001 From: Romain Marcadier-Muller Date: Mon, 1 Jul 2019 15:31:54 +0200 Subject: [PATCH 2/2] fix(code): make CfnResource#_toCloudFormation null-safe (#3121) The `CfnResource#_toCloudFormation` method creates a `PostResolveToken` with a post-processor that was not ready to handle the absence of `Properties` on the resolved value. It is however possible that `Properties` are missing when an object is created with the default configuration (e.g: by `new sqs.CfnQueue(this, 'Queue');`). This change makes the post-processor function correctly handle `undefined` in this case. Related #3093 --- packages/@aws-cdk/core/lib/cfn-resource.ts | 3 +- .../@aws-cdk/core/test/test.cfn-resource.ts | 29 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) create mode 100644 packages/@aws-cdk/core/test/test.cfn-resource.ts diff --git a/packages/@aws-cdk/core/lib/cfn-resource.ts b/packages/@aws-cdk/core/lib/cfn-resource.ts index edb7d3f87a8ea..31dc062f5ce43 100644 --- a/packages/@aws-cdk/core/lib/cfn-resource.ts +++ b/packages/@aws-cdk/core/lib/cfn-resource.ts @@ -230,7 +230,8 @@ export class CfnResource extends CfnRefElement { Metadata: ignoreEmpty(this.cfnOptions.metadata), Condition: this.cfnOptions.condition && this.cfnOptions.condition.logicalId }, props => { - props.Properties = this.renderProperties(props.Properties); + const renderedProps = this.renderProperties(props.Properties || {}); + props.Properties = renderedProps && (Object.values(renderedProps).find(v => !!v) ? renderedProps : undefined); return deepMerge(props, this.rawOverrides); }) } diff --git a/packages/@aws-cdk/core/test/test.cfn-resource.ts b/packages/@aws-cdk/core/test/test.cfn-resource.ts new file mode 100644 index 0000000000000..db83634ecccd3 --- /dev/null +++ b/packages/@aws-cdk/core/test/test.cfn-resource.ts @@ -0,0 +1,29 @@ +import nodeunit = require('nodeunit'); +import core = require('../lib'); + +export = nodeunit.testCase({ + '._toCloudFormation': { + 'does not call renderProperties with an undefined value'(test: nodeunit.Test) { + const app = new core.App(); + const stack = new core.Stack(app, 'TestStack'); + const resource = new core.CfnResource(stack, 'DefaultResource', { type: 'Test::Resource::Fake' }); + + let called = false; + (resource as any).renderProperties = (val: any) => { + called = true; + test.notEqual(val, null); + }; + + test.deepEqual(app.synth().getStack(stack.stackName).template, { + Resources: { + DefaultResource: { + Type: 'Test::Resource::Fake' + } + } + }); + test.ok(called, `renderProperties must be called called`); + + test.done(); + } + } +});