From 9c6ff2a76018aab6629ccb14d5ee96dcdb0e04fe Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 24 Oct 2018 10:26:03 +0200 Subject: [PATCH] fix(cloudformation-diff): track replacements 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. --- lerna.json | 5 + .../cloudformation-diff/lib/diff-template.ts | 108 ++++++++++++++++-- .../test/test.diff-template.ts | 73 ++++++++++-- 3 files changed, 168 insertions(+), 18 deletions(-) diff --git a/lerna.json b/lerna.json index 8c20c4987a516..8d739ce6fddf5 100644 --- a/lerna.json +++ b/lerna.json @@ -6,6 +6,11 @@ "packages/@aws-cdk/*", "tools/*" ], + "command": { + "bootstrap": { + "npmClientArgs": ["--no-package-lock"] + } + }, "rejectCycles": "true", "version": "0.13.0" } diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts index f2e36251a43eb..9a0d6ae88039b 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts @@ -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 } = {}; - 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 } = {}; + 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); } /** @@ -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; +} \ No newline at end of file diff --git a/packages/@aws-cdk/cloudformation-diff/test/test.diff-template.ts b/packages/@aws-cdk/cloudformation-diff/test/test.diff-template.ts index d3eab88f42260..32c264bead1b1 100644 --- a/packages/@aws-cdk/cloudformation-diff/test/test.diff-template.ts +++ b/packages/@aws-cdk/cloudformation-diff/test/test.diff-template.ts @@ -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: { @@ -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: { @@ -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: { @@ -160,7 +166,9 @@ exports.diffTemplate = { const newTemplate = { Resources: { - BucketPolicyResource: BUCKET_POLICY_RESOURCE, + QueueResource: { + Type: 'AWS::SQS::Queue' + }, BucketResource: { Type: 'AWS::S3::Bucket' } @@ -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' } @@ -192,7 +202,9 @@ exports.diffTemplate = { const newTemplate = { Resources: { - BucketPolicyResource: BUCKET_POLICY_RESOURCE, + QueueResource: { + Type: 'AWS::SQS::Queue' + }, BucketResource: { Type: 'AWS::S3::Bucket', Properties: { @@ -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(); + }, };