Skip to content

Commit 10ed194

Browse files
authored
fix(cli): cdk diff falsely reports resource replacements on trivial template changes (#28336)
Adds a new flag to diff, `--change-set`, that creates a new changeset and uses it to determine resource replacement. This new flag is on by default. When the flag is set, the following happens: * Resource metadata changes are obscured * Resource changes that do not appear in the changeset are obscured from the diff When the flag is unset, yaml Fn::GetAtt short-form uses are considered equivalent to their long-form counterpart. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent a74aacf commit 10ed194

22 files changed

+1137
-335
lines changed

packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts

+116-2
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
// The SDK is only used to reference `DescribeChangeSetOutput`, so the SDK is added as a devDependency.
2+
// The SDK should not make network calls here
3+
// eslint-disable-next-line import/no-extraneous-dependencies
4+
import { CloudFormation } from 'aws-sdk';
15
import * as impl from './diff';
26
import * as types from './diff/types';
37
import { deepEqual, diffKeyedEntities, unionOf } from './diff/util';
@@ -33,12 +37,33 @@ const DIFF_HANDLERS: HandlerRegistry = {
3337
*
3438
* @param currentTemplate the current state of the stack.
3539
* @param newTemplate the target state of the stack.
40+
* @param changeSet the change set for this stack.
3641
*
3742
* @returns a +types.TemplateDiff+ object that represents the changes that will happen if
3843
* a stack which current state is described by +currentTemplate+ is updated with
3944
* the template +newTemplate+.
4045
*/
41-
export function diffTemplate(currentTemplate: { [key: string]: any }, newTemplate: { [key: string]: any }): types.TemplateDiff {
46+
export function fullDiff(
47+
currentTemplate: { [key: string]: any },
48+
newTemplate: { [key: string]: any },
49+
changeSet?: CloudFormation.DescribeChangeSetOutput,
50+
): types.TemplateDiff {
51+
52+
normalize(currentTemplate);
53+
normalize(newTemplate);
54+
const theDiff = diffTemplate(currentTemplate, newTemplate);
55+
if (changeSet) {
56+
filterFalsePositivies(theDiff, changeSet);
57+
}
58+
59+
return theDiff;
60+
}
61+
62+
function diffTemplate(
63+
currentTemplate: { [key: string]: any },
64+
newTemplate: { [key: string]: any },
65+
): types.TemplateDiff {
66+
4267
// Base diff
4368
const theDiff = calculateTemplateDiff(currentTemplate, newTemplate);
4469

@@ -105,7 +130,6 @@ function calculateTemplateDiff(currentTemplate: { [key: string]: any }, newTempl
105130
const handler: DiffHandler = DIFF_HANDLERS[key]
106131
|| ((_diff, oldV, newV) => unknown[key] = impl.diffUnknown(oldV, newV));
107132
handler(differences, oldValue, newValue);
108-
109133
}
110134
if (Object.keys(unknown).length > 0) {
111135
differences.unknown = new types.DifferenceCollection(unknown);
@@ -184,3 +208,93 @@ function deepCopy(x: any): any {
184208

185209
return x;
186210
}
211+
212+
function filterFalsePositivies(diff: types.TemplateDiff, changeSet: CloudFormation.DescribeChangeSetOutput) {
213+
const replacements = findResourceReplacements(changeSet);
214+
diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => {
215+
change.forEachDifference((type: 'Property' | 'Other', name: string, value: types.Difference<any> | types.PropertyDifference<any>) => {
216+
if (type === 'Property') {
217+
if (!replacements[logicalId]) {
218+
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE;
219+
(value as types.PropertyDifference<any>).isDifferent = false;
220+
return;
221+
}
222+
switch (replacements[logicalId].propertiesReplaced[name]) {
223+
case 'Always':
224+
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.WILL_REPLACE;
225+
break;
226+
case 'Never':
227+
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.WILL_UPDATE;
228+
break;
229+
case 'Conditionally':
230+
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.MAY_REPLACE;
231+
break;
232+
case undefined:
233+
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE;
234+
(value as types.PropertyDifference<any>).isDifferent = false;
235+
break;
236+
// otherwise, defer to the changeImpact from `diffTemplate`
237+
}
238+
} else if (type === 'Other') {
239+
switch (name) {
240+
case 'Metadata':
241+
change.setOtherChange('Metadata', new types.Difference<string>(value.newValue, value.newValue));
242+
break;
243+
}
244+
}
245+
});
246+
});
247+
}
248+
249+
function findResourceReplacements(changeSet: CloudFormation.DescribeChangeSetOutput): types.ResourceReplacements {
250+
const replacements: types.ResourceReplacements = {};
251+
for (const resourceChange of changeSet.Changes ?? []) {
252+
const propertiesReplaced: { [propName: string]: types.ChangeSetReplacement } = {};
253+
for (const propertyChange of resourceChange.ResourceChange?.Details ?? []) {
254+
if (propertyChange.Target?.Attribute === 'Properties') {
255+
const requiresReplacement = propertyChange.Target.RequiresRecreation === 'Always';
256+
if (requiresReplacement && propertyChange.Evaluation === 'Static') {
257+
propertiesReplaced[propertyChange.Target.Name!] = 'Always';
258+
} else if (requiresReplacement && propertyChange.Evaluation === 'Dynamic') {
259+
// If Evaluation is 'Dynamic', then this may cause replacement, or it may not.
260+
// see 'Replacement': https://docs.aws.amazon.com/AWSCloudFormation/latest/APIReference/API_ResourceChange.html
261+
propertiesReplaced[propertyChange.Target.Name!] = 'Conditionally';
262+
} else {
263+
propertiesReplaced[propertyChange.Target.Name!] = propertyChange.Target.RequiresRecreation as types.ChangeSetReplacement;
264+
}
265+
}
266+
}
267+
replacements[resourceChange.ResourceChange?.LogicalResourceId!] = {
268+
resourceReplaced: resourceChange.ResourceChange?.Replacement === 'True',
269+
propertiesReplaced,
270+
};
271+
}
272+
273+
return replacements;
274+
}
275+
276+
function normalize(template: any) {
277+
if (typeof template === 'object') {
278+
for (const key of (Object.keys(template ?? {}))) {
279+
if (key === 'Fn::GetAtt' && typeof template[key] === 'string') {
280+
template[key] = template[key].split('.');
281+
continue;
282+
} else if (key === 'DependsOn') {
283+
if (typeof template[key] === 'string') {
284+
template[key] = [template[key]];
285+
} else if (Array.isArray(template[key])) {
286+
template[key] = template[key].sort();
287+
}
288+
continue;
289+
}
290+
291+
if (Array.isArray(template[key])) {
292+
for (const element of (template[key])) {
293+
normalize(element);
294+
}
295+
} else {
296+
normalize(template[key]);
297+
}
298+
}
299+
}
300+
}

packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts

+27-2
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,15 @@ import { SecurityGroupChanges } from '../network/security-group-changes';
66

77
export type PropertyMap = {[key: string]: any };
88

9+
export type ResourceReplacements = { [logicalId: string]: ResourceReplacement };
10+
11+
export interface ResourceReplacement {
12+
resourceReplaced: boolean,
13+
propertiesReplaced: { [propertyName: string]: ChangeSetReplacement };
14+
}
15+
16+
export type ChangeSetReplacement = 'Always' | 'Never' | 'Conditionally';
17+
918
/** Semantic differences between two CloudFormation templates. */
1019
export class TemplateDiff implements ITemplateDiff {
1120
public awsTemplateFormatVersion?: Difference<string>;
@@ -296,7 +305,7 @@ export class Difference<ValueType> implements IDifference<ValueType> {
296305
*
297306
* isDifferent => (isUpdate | isRemoved | isUpdate)
298307
*/
299-
public readonly isDifferent: boolean;
308+
public isDifferent: boolean;
300309

301310
/**
302311
* @param oldValue the old value, cannot be equal (to the sense of +deepEqual+) to +newValue+.
@@ -327,7 +336,7 @@ export class Difference<ValueType> implements IDifference<ValueType> {
327336
}
328337

329338
export class PropertyDifference<ValueType> extends Difference<ValueType> {
330-
public readonly changeImpact?: ResourceImpact;
339+
public changeImpact?: ResourceImpact;
331340

332341
constructor(oldValue: ValueType | undefined, newValue: ValueType | undefined, args: { changeImpact?: ResourceImpact }) {
333342
super(oldValue, newValue);
@@ -352,6 +361,10 @@ export class DifferenceCollection<V, T extends IDifference<V>> {
352361
return ret;
353362
}
354363

364+
public remove(logicalId: string): void {
365+
delete this.diffs[logicalId];
366+
}
367+
355368
public get logicalIds(): string[] {
356369
return Object.keys(this.changes);
357370
}
@@ -621,6 +634,18 @@ export class ResourceDifference implements IDifference<Resource> {
621634
this.propertyDiffs[propertyName] = change;
622635
}
623636

637+
/**
638+
* Replace a OtherChange in this object
639+
*
640+
* This affects the property diff as it is summarized to users, but it DOES
641+
* NOT affect either the "oldValue" or "newValue" values; those still contain
642+
* the actual template values as provided by the user (they might still be
643+
* used for downstream processing).
644+
*/
645+
public setOtherChange(otherName: string, change: PropertyDifference<any>) {
646+
this.otherDiffs[otherName] = change;
647+
}
648+
624649
public get changeImpact(): ResourceImpact {
625650
// Check the Type first
626651
if (this.resourceTypes.oldType !== this.resourceTypes.newType) {

packages/@aws-cdk/cloudformation-diff/package.json

+1
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@
3838
"@types/string-width": "^4.0.1",
3939
"fast-check": "^3.14.0",
4040
"jest": "^29.7.0",
41+
"aws-sdk": "2.1516.0",
4142
"ts-jest": "^29.1.1"
4243
},
4344
"repository": {

0 commit comments

Comments
 (0)