Skip to content

Commit

Permalink
fix(cli): cdk diff falsely reports resource replacements on trivial…
Browse files Browse the repository at this point in the history
… 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*
  • Loading branch information
comcalvi authored Jan 4, 2024
1 parent a74aacf commit 10ed194
Show file tree
Hide file tree
Showing 22 changed files with 1,137 additions and 335 deletions.
118 changes: 116 additions & 2 deletions packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
// The SDK is only used to reference `DescribeChangeSetOutput`, so the SDK is added as a devDependency.
// The SDK should not make network calls here
// eslint-disable-next-line import/no-extraneous-dependencies
import { CloudFormation } from 'aws-sdk';
import * as impl from './diff';
import * as types from './diff/types';
import { deepEqual, diffKeyedEntities, unionOf } from './diff/util';
Expand Down Expand Up @@ -33,12 +37,33 @@ const DIFF_HANDLERS: HandlerRegistry = {
*
* @param currentTemplate the current state of the stack.
* @param newTemplate the target state of the stack.
* @param changeSet the change set for this stack.
*
* @returns a +types.TemplateDiff+ object that represents the changes that will happen if
* a stack which current state is described by +currentTemplate+ is updated with
* the template +newTemplate+.
*/
export function diffTemplate(currentTemplate: { [key: string]: any }, newTemplate: { [key: string]: any }): types.TemplateDiff {
export function fullDiff(
currentTemplate: { [key: string]: any },
newTemplate: { [key: string]: any },
changeSet?: CloudFormation.DescribeChangeSetOutput,
): types.TemplateDiff {

normalize(currentTemplate);
normalize(newTemplate);
const theDiff = diffTemplate(currentTemplate, newTemplate);
if (changeSet) {
filterFalsePositivies(theDiff, changeSet);
}

return theDiff;
}

function diffTemplate(
currentTemplate: { [key: string]: any },
newTemplate: { [key: string]: any },
): types.TemplateDiff {

// Base diff
const theDiff = calculateTemplateDiff(currentTemplate, newTemplate);

Expand Down Expand Up @@ -105,7 +130,6 @@ function calculateTemplateDiff(currentTemplate: { [key: string]: any }, newTempl
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);
Expand Down Expand Up @@ -184,3 +208,93 @@ function deepCopy(x: any): any {

return x;
}

function filterFalsePositivies(diff: types.TemplateDiff, changeSet: CloudFormation.DescribeChangeSetOutput) {
const replacements = findResourceReplacements(changeSet);
diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => {
change.forEachDifference((type: 'Property' | 'Other', name: string, value: types.Difference<any> | types.PropertyDifference<any>) => {
if (type === 'Property') {
if (!replacements[logicalId]) {
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE;
(value as types.PropertyDifference<any>).isDifferent = false;
return;
}
switch (replacements[logicalId].propertiesReplaced[name]) {
case 'Always':
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.WILL_REPLACE;
break;
case 'Never':
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.WILL_UPDATE;
break;
case 'Conditionally':
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.MAY_REPLACE;
break;
case undefined:
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE;
(value as types.PropertyDifference<any>).isDifferent = false;
break;
// otherwise, defer to the changeImpact from `diffTemplate`
}
} else if (type === 'Other') {
switch (name) {
case 'Metadata':
change.setOtherChange('Metadata', new types.Difference<string>(value.newValue, value.newValue));
break;
}
}
});
});
}

function findResourceReplacements(changeSet: CloudFormation.DescribeChangeSetOutput): types.ResourceReplacements {
const replacements: types.ResourceReplacements = {};
for (const resourceChange of changeSet.Changes ?? []) {
const propertiesReplaced: { [propName: string]: types.ChangeSetReplacement } = {};
for (const propertyChange of resourceChange.ResourceChange?.Details ?? []) {
if (propertyChange.Target?.Attribute === 'Properties') {
const requiresReplacement = propertyChange.Target.RequiresRecreation === 'Always';
if (requiresReplacement && propertyChange.Evaluation === 'Static') {
propertiesReplaced[propertyChange.Target.Name!] = 'Always';
} else if (requiresReplacement && propertyChange.Evaluation === 'Dynamic') {
// If Evaluation is 'Dynamic', then this may cause replacement, or it may not.
// see 'Replacement': https://docs.aws.amazon.com/AWSCloudFormation/latest/APIReference/API_ResourceChange.html
propertiesReplaced[propertyChange.Target.Name!] = 'Conditionally';
} else {
propertiesReplaced[propertyChange.Target.Name!] = propertyChange.Target.RequiresRecreation as types.ChangeSetReplacement;
}
}
}
replacements[resourceChange.ResourceChange?.LogicalResourceId!] = {
resourceReplaced: resourceChange.ResourceChange?.Replacement === 'True',
propertiesReplaced,
};
}

return replacements;
}

function normalize(template: any) {
if (typeof template === 'object') {
for (const key of (Object.keys(template ?? {}))) {
if (key === 'Fn::GetAtt' && typeof template[key] === 'string') {
template[key] = template[key].split('.');
continue;
} else if (key === 'DependsOn') {
if (typeof template[key] === 'string') {
template[key] = [template[key]];
} else if (Array.isArray(template[key])) {
template[key] = template[key].sort();
}
continue;
}

if (Array.isArray(template[key])) {
for (const element of (template[key])) {
normalize(element);
}
} else {
normalize(template[key]);
}
}
}
}
29 changes: 27 additions & 2 deletions packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,15 @@ import { SecurityGroupChanges } from '../network/security-group-changes';

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

export type ResourceReplacements = { [logicalId: string]: ResourceReplacement };

export interface ResourceReplacement {
resourceReplaced: boolean,
propertiesReplaced: { [propertyName: string]: ChangeSetReplacement };
}

export type ChangeSetReplacement = 'Always' | 'Never' | 'Conditionally';

/** Semantic differences between two CloudFormation templates. */
export class TemplateDiff implements ITemplateDiff {
public awsTemplateFormatVersion?: Difference<string>;
Expand Down Expand Up @@ -296,7 +305,7 @@ export class Difference<ValueType> implements IDifference<ValueType> {
*
* isDifferent => (isUpdate | isRemoved | isUpdate)
*/
public readonly isDifferent: boolean;
public isDifferent: boolean;

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

export class PropertyDifference<ValueType> extends Difference<ValueType> {
public readonly changeImpact?: ResourceImpact;
public changeImpact?: ResourceImpact;

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

public remove(logicalId: string): void {
delete this.diffs[logicalId];
}

public get logicalIds(): string[] {
return Object.keys(this.changes);
}
Expand Down Expand Up @@ -621,6 +634,18 @@ export class ResourceDifference implements IDifference<Resource> {
this.propertyDiffs[propertyName] = change;
}

/**
* Replace a OtherChange in this object
*
* This affects the property diff as it is summarized to users, but it DOES
* NOT affect either the "oldValue" or "newValue" values; those still contain
* the actual template values as provided by the user (they might still be
* used for downstream processing).
*/
public setOtherChange(otherName: string, change: PropertyDifference<any>) {
this.otherDiffs[otherName] = change;
}

public get changeImpact(): ResourceImpact {
// Check the Type first
if (this.resourceTypes.oldType !== this.resourceTypes.newType) {
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/cloudformation-diff/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"@types/string-width": "^4.0.1",
"fast-check": "^3.14.0",
"jest": "^29.7.0",
"aws-sdk": "2.1516.0",
"ts-jest": "^29.1.1"
},
"repository": {
Expand Down
Loading

0 comments on commit 10ed194

Please sign in to comment.