Skip to content

Commit

Permalink
fix(cloudformation-diff): track replacements (aws#1003)
Browse files Browse the repository at this point in the history 
Track second-order effects of resource replacements through references.

If a resource gets replaced and it's referenced by some other resource
in an immutable field, that field gets replaced as well, and so on.

ALSO IN THIS COMMIT

- Configure lerna to not update package-lock.json on every bootstrap.

Fixes aws#1001
  • Loading branch information
@rix0rrr
rix0rrr authored Oct 25, 2018
1 parent 8c90350 commit a83ac5f
Show file tree
Hide file tree
Showing 3 changed files with 168 additions and 18 deletions.
5 changes: 5 additions & 0 deletions lerna.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@
"packages/@aws-cdk/*",
"tools/*"
],
"command": {
"bootstrap": {
"npmClientArgs": ["--no-package-lock"]
}
},
"rejectCycles": "true",
"version": "0.13.0"
}
108 changes: 97 additions & 11 deletions packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,18 +39,40 @@ const DIFF_HANDLERS: HandlerRegistry = {
* the template +newTemplate+.
*/
export function diffTemplate(currentTemplate: { [key: string]: any }, newTemplate: { [key: string]: any }): types.TemplateDiff {
const differences: types.ITemplateDiff = {};
const unknown: { [key: string]: types.Difference<any> } = {};
for (const key of unionOf(Object.keys(currentTemplate), Object.keys(newTemplate)).sort()) {
const oldValue = currentTemplate[key];
const newValue = newTemplate[key];
if (deepEqual(oldValue, newValue)) { continue; }
const handler: DiffHandler = DIFF_HANDLERS[key]
|| ((_diff, oldV, newV) => unknown[key] = impl.diffUnknown(oldV, newV));
handler(differences, oldValue, newValue);
// We're going to modify this in-place
newTemplate = deepCopy(newTemplate);

while (true) {
const differences: types.ITemplateDiff = {};
const unknown: { [key: string]: types.Difference<any> } = {};
for (const key of unionOf(Object.keys(currentTemplate), Object.keys(newTemplate)).sort()) {
const oldValue = currentTemplate[key];
const newValue = newTemplate[key];
if (deepEqual(oldValue, newValue)) { continue; }
const handler: DiffHandler = DIFF_HANDLERS[key]
|| ((_diff, oldV, newV) => unknown[key] = impl.diffUnknown(oldV, newV));
handler(differences, oldValue, newValue);

}
if (Object.keys(unknown).length > 0) { differences.unknown = new types.DifferenceCollection(unknown); }

// Propagate replacements for replaced resources
let didPropagateReferenceChanges = false;
if (differences.resources) {
differences.resources.forEach((logicalId, change) => {
if (change.changeImpact === types.ResourceImpact.WILL_REPLACE) {
if (propagateReplacedReferences(newTemplate, logicalId)) {
didPropagateReferenceChanges = true;
}
}
});
}

// We're done only if we didn't have to propagate any more replacements.
if (!didPropagateReferenceChanges) {
return new types.TemplateDiff(differences);
}
}
if (Object.keys(unknown).length > 0) { differences.unknown = new types.DifferenceCollection(unknown); }
return new types.TemplateDiff(differences);
}

/**
Expand All @@ -59,3 +81,67 @@ export function diffTemplate(currentTemplate: { [key: string]: any }, newTemplat
export function diffResource(oldValue: types.Resource, newValue: types.Resource): types.ResourceDifference {
return impl.diffResource(oldValue, newValue);
}

/**
* Replace all references to the given logicalID on the given template, in-place
*
* Returns true iff any references were replaced.
*/
function propagateReplacedReferences(template: object, logicalId: string): boolean {
let ret = false;

function recurse(obj: any) {
if (Array.isArray(obj)) {
obj.forEach(recurse);
}

if (typeof obj === 'object' && obj !== null) {
if (!replaceReference(obj)) {
Object.values(obj).forEach(recurse);
}
}
}

function replaceReference(obj: any) {
const keys = Object.keys(obj);
if (keys.length !== 1) { return false; }
const key = keys[0];

if (key === 'Ref') {
if (obj.Ref === logicalId) {
obj.Ref = logicalId + '(replaced)';
ret = true;
}
return true;
}

if (key.startsWith('Fn::')) {
if (Array.isArray(obj[key]) && obj[key].length > 0 && obj[key][0] === logicalId) {
obj[key][0] = logicalId + '(replaced)';
ret = true;
}
return true;
}

return false;
}

recurse(template);
return ret;
}

function deepCopy(x: any): any {
if (Array.isArray(x)) {
return x.map(deepCopy);
}

if (typeof x === 'object' && x !== null) {
const ret: any = {};
for (const key of Object.keys(x)) {
ret[key] = deepCopy(x[key]);
}
return ret;
}

return x;
}
73 changes: 66 additions & 7 deletions packages/@aws-cdk/cloudformation-diff/test/test.diff-template.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,9 @@ exports.diffTemplate = {
const bucketName = 'ShineyBucketName';
const currentTemplate = {
Resources: {
BucketPolicyResource: BUCKET_POLICY_RESOURCE,
QueueResource: {
Type: 'AWS::SQS::Queue'
},
BucketResource: {
Type: 'AWS::S3::Bucket',
Properties: {
Expand All @@ -122,7 +124,9 @@ exports.diffTemplate = {
const newBucketName = `${bucketName}-v2`;
const newTemplate = {
Resources: {
BucketPolicyResource: BUCKET_POLICY_RESOURCE,
QueueResource: {
Type: 'AWS::SQS::Queue'
},
BucketResource: {
Type: 'AWS::S3::Bucket',
Properties: {
Expand All @@ -148,7 +152,9 @@ exports.diffTemplate = {
const bucketName = 'ShineyBucketName';
const currentTemplate = {
Resources: {
BucketPolicyResource: BUCKET_POLICY_RESOURCE,
QueueResource: {
Type: 'AWS::SQS::Queue'
},
BucketResource: {
Type: 'AWS::S3::Bucket',
Properties: {
Expand All @@ -160,7 +166,9 @@ exports.diffTemplate = {

const newTemplate = {
Resources: {
BucketPolicyResource: BUCKET_POLICY_RESOURCE,
QueueResource: {
Type: 'AWS::SQS::Queue'
},
BucketResource: {
Type: 'AWS::S3::Bucket'
}
Expand All @@ -183,7 +191,9 @@ exports.diffTemplate = {
const bucketName = 'ShineyBucketName';
const currentTemplate = {
Resources: {
BucketPolicyResource: BUCKET_POLICY_RESOURCE,
QueueResource: {
Type: 'AWS::SQS::Queue'
},
BucketResource: {
Type: 'AWS::S3::Bucket'
}
Expand All @@ -192,7 +202,9 @@ exports.diffTemplate = {

const newTemplate = {
Resources: {
BucketPolicyResource: BUCKET_POLICY_RESOURCE,
QueueResource: {
Type: 'AWS::SQS::Queue'
},
BucketResource: {
Type: 'AWS::S3::Bucket',
Properties: {
Expand Down Expand Up @@ -247,5 +259,52 @@ exports.diffTemplate = {
'the difference reflects the type change');
test.equal(difference && difference.changeImpact, ResourceImpact.WILL_REPLACE, 'the difference reflects that the resource will be replaced');
test.done();
}
},

'resource replacement is tracked through references': (test: Test) => {
// If a resource is replaced, then that change shows that references are
// going to change. This may lead to replacement of downstream resources
// if the reference is used in an immutable property, and so on.

// GIVEN
const currentTemplate = {
Resources: {
Bucket: {
Type: 'AWS::S3::Bucket',
Properties: { BucketName: 'Name1', }, // Immutable prop
},
Queue: {
Type: 'AWS::SQS::Queue',
Properties: { QueueName: { Ref: 'Bucket' }}, // Immutable prop
},
Topic: {
Type: 'AWS::SNS::Topic',
Properties: { TopicName: { Ref: 'Queue' }}, // Immutable prop
}
}
};

// WHEN
const newTemplate = {
Resources: {
Bucket: {
Type: 'AWS::S3::Bucket',
Properties: { BucketName: 'Name2', },
},
Queue: {
Type: 'AWS::SQS::Queue',
Properties: { QueueName: { Ref: 'Bucket' }},
},
Topic: {
Type: 'AWS::SNS::Topic',
Properties: { TopicName: { Ref: 'Queue' }},
}
}
};
const differences = diffTemplate(currentTemplate, newTemplate);

// THEN
test.equal(differences.resources.count, 3, 'all resources are replaced');
test.done();
},
};

0 comments on commit a83ac5f

Please sign in to comment.