Skip to content

Commit

Permalink
fix(core): apply overrides after rendering properties (#2685)
Browse files Browse the repository at this point in the history
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')`.
  • Loading branch information
Elad Ben-Israel authored May 30, 2019
1 parent 50d71bf commit f2636e5
Show file tree
Hide file tree
Showing 3 changed files with 135 additions and 3 deletions.
98 changes: 98 additions & 0 deletions packages/@aws-cdk/aws-iam/test/test.escape-hatch.ts
Original file line number Diff line number Diff line change
@@ -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();
}
};
8 changes: 5 additions & 3 deletions packages/@aws-cdk/cdk/lib/cfn-resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
})
}
};
Expand Down
32 changes: 32 additions & 0 deletions packages/@aws-cdk/cdk/test/test.resource.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit f2636e5

Please sign in to comment.