From f2636e53119d9fcb72142811d87f46d623f11227 Mon Sep 17 00:00:00 2001 From: Elad Ben-Israel Date: Thu, 30 May 2019 20:16:17 +0300 Subject: [PATCH] fix(core): apply overrides after rendering properties (#2685) Resource overrides (`addOverride` and `addPropertyOverride`) should be applied after rendering properties at the L1 level. Otherwise, validation and capitalization changes would be applied to overrides and this contradicts the idea of being able to specify arbitrary overrides as "patches" to the synthesized resource. The previous behavior had two adverse effects: 1. If a property was unknown, it would be omitted from the resource 2. Properties names would need to be capitalized in camel case instead of 1:1 with the CFN schema. Fixes #2677 BREAKING CHANGE: Properties passed to `addPropertyOverride` should match in capitalization to the CloudFormation schema (normally pascal case). For example, `addPropertyOverride('accessControl', 'xxx')` should now be `addPropertyOverride('AccessControl', 'xxx')`. --- .../aws-iam/test/test.escape-hatch.ts | 98 +++++++++++++++++++ packages/@aws-cdk/cdk/lib/cfn-resource.ts | 8 +- packages/@aws-cdk/cdk/test/test.resource.ts | 32 ++++++ 3 files changed, 135 insertions(+), 3 deletions(-) create mode 100644 packages/@aws-cdk/aws-iam/test/test.escape-hatch.ts diff --git a/packages/@aws-cdk/aws-iam/test/test.escape-hatch.ts b/packages/@aws-cdk/aws-iam/test/test.escape-hatch.ts new file mode 100644 index 0000000000000..942dd55127ed8 --- /dev/null +++ b/packages/@aws-cdk/aws-iam/test/test.escape-hatch.ts @@ -0,0 +1,98 @@ +// tests for the L1 escape hatches (overrides). those are in the IAM module +// because we want to verify them end-to-end, as a complement to the unit +// tests in the @aws-cdk/cdk module +import { expect } from '@aws-cdk/assert'; +import { Stack } from '@aws-cdk/cdk'; +import { Test } from 'nodeunit'; +import iam = require('../lib'); + +// tslint:disable:object-literal-key-quotes + +export = { + 'addPropertyOverride should allow overriding supported properties'(test: Test) { + const stack = new Stack(); + const user = new iam.User(stack, 'user', { + userName: 'MyUserName' + }); + + const cfn = user.node.findChild('Resource') as iam.CfnUser; + cfn.addPropertyOverride('UserName', 'OverriddenUserName'); + + expect(stack).toMatch({ + "Resources": { + "user2C2B57AE": { + "Type": "AWS::IAM::User", + "Properties": { + "UserName": "OverriddenUserName" + } + } + } + }); + test.done(); + }, + 'addPropertyOverrides should allow specifying arbitrary properties'(test: Test) { + // GIVEN + const stack = new Stack(); + const user = new iam.User(stack, 'user', { userName: 'MyUserName' }); + const cfn = user.node.findChild('Resource') as iam.CfnUser; + + // WHEN + cfn.addPropertyOverride('Hello.World', 'Boom'); + + // THEN + expect(stack).toMatch({ + "Resources": { + "user2C2B57AE": { + "Type": "AWS::IAM::User", + "Properties": { + "UserName": "MyUserName", + "Hello": { + "World": "Boom" + } + } + } + } + }); + + test.done(); + }, + 'addOverride should allow overriding properties'(test: Test) { + // GIVEN + const stack = new Stack(); + const user = new iam.User(stack, 'user', { userName: 'MyUserName' }); + const cfn = user.node.findChild('Resource') as iam.CfnUser; + cfn.options.updatePolicy = { useOnlineResharding: true }; + + // WHEN + cfn.addOverride('Properties.Hello.World', 'Bam'); + cfn.addOverride('Properties.UserName', 'HA!'); + cfn.addOverride('Joob.Jab', 'Jib'); + cfn.addOverride('Joob.Jab', 'Jib'); + cfn.addOverride('UpdatePolicy.UseOnlineResharding.Type', 'None'); + + // THEN + expect(stack).toMatch({ + "Resources": { + "user2C2B57AE": { + "Type": "AWS::IAM::User", + "Properties": { + "UserName": "HA!", + "Hello": { + "World": "Bam" + } + }, + "Joob": { + "Jab": "Jib" + }, + "UpdatePolicy": { + "UseOnlineResharding": { + "Type": "None" + } + } + } + } + }); + + test.done(); + } +}; \ No newline at end of file diff --git a/packages/@aws-cdk/cdk/lib/cfn-resource.ts b/packages/@aws-cdk/cdk/lib/cfn-resource.ts index d04eac39c5c12..9f7e46546f4a1 100644 --- a/packages/@aws-cdk/cdk/lib/cfn-resource.ts +++ b/packages/@aws-cdk/cdk/lib/cfn-resource.ts @@ -234,9 +234,11 @@ export class CfnResource extends CfnRefElement { Metadata: ignoreEmpty(this.options.metadata), Condition: this.options.condition && this.options.condition.logicalId }, props => { - const r = deepMerge(props, this.rawOverrides); - r.Properties = this.renderProperties(r.Properties); - return r; + // let derived classes to influence how properties are rendered (e.g. change capitalization) + props.Properties = this.renderProperties(props.Properties); + + // merge overrides *after* rendering + return deepMerge(props, this.rawOverrides); }) } }; diff --git a/packages/@aws-cdk/cdk/test/test.resource.ts b/packages/@aws-cdk/cdk/test/test.resource.ts index 2cdcc03509aad..976186301d319 100644 --- a/packages/@aws-cdk/cdk/test/test.resource.ts +++ b/packages/@aws-cdk/cdk/test/test.resource.ts @@ -549,6 +549,38 @@ export = { test.done(); }, + 'overrides are applied after render'(test: Test) { + // GIVEN + class MyResource extends CfnResource { + public renderProperties() { + return { Fixed: 123 }; + } + } + const stack = new Stack(); + const cfn = new MyResource(stack, 'rr', { type: 'AWS::Resource::Type' }); + + // WHEN + cfn.addPropertyOverride('Boom', 'Hi'); + cfn.addOverride('Properties.Foo.Bar', 'Bar'); + + // THEN + test.deepEqual(stack._toCloudFormation(), { + Resources: { + rr: { + Type: 'AWS::Resource::Type', + Properties: { + Fixed: 123, + Boom: 'Hi', + Foo: { + Bar: 'Bar' + } + } + } + } + }); + test.done(); + }, + 'untypedPropertyOverrides': { 'can be used by derived classes to specify overrides before render()'(test: Test) {