Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cli): cdk diff falsely reports resource replacements on trivial template changes #28336

Merged
merged 51 commits into from
Jan 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
b299958
can now create changeset during diff
comcalvi Dec 1, 2023
9f73241
working changeset diff for resource replacement, but not for property…
comcalvi Dec 4, 2023
b59e97f
property-level diff
comcalvi Dec 4, 2023
850fd0f
removed resource metadata from diff, we now consider evaluation: dyna…
comcalvi Dec 6, 2023
858abcc
Merge branch 'main' of github.com:aws/aws-cdk into comcalvi/changeset…
comcalvi Dec 6, 2023
edd8dc3
diff now evaluates fn::getAtt short form and long form equivalence
comcalvi Dec 6, 2023
7f5cec0
refactor
comcalvi Dec 6, 2023
f7f8d15
dependson is another problem...
comcalvi Dec 11, 2023
49e890c
tests, pt 1
comcalvi Dec 12, 2023
5129e95
cleanup
comcalvi Dec 12, 2023
4c66a1e
move to devDeps
comcalvi Dec 12, 2023
d373680
yarn.lock
comcalvi Dec 12, 2023
03a79f2
clean
comcalvi Dec 12, 2023
7c6287b
refactor
comcalvi Dec 13, 2023
da00e1a
CLI options
comcalvi Dec 13, 2023
212f603
remove metadata, but only when the flag is passed
comcalvi Dec 13, 2023
9d01051
not sure who uses this, but someone might...
comcalvi Dec 13, 2023
73c4685
console
comcalvi Dec 13, 2023
29d9094
conflicts
comcalvi Dec 13, 2023
3096aec
cleanup
comcalvi Dec 13, 2023
a106c7d
added the SDK comment
comcalvi Dec 13, 2023
e6e770d
style
comcalvi Dec 13, 2023
054e432
test comment cleanup
comcalvi Dec 13, 2023
561663d
tests
comcalvi Dec 13, 2023
5d44f9a
TS
comcalvi Dec 13, 2023
64e918b
bleh
comcalvi Dec 13, 2023
4686b37
cleanup
comcalvi Dec 14, 2023
d3edce4
lefotver
comcalvi Dec 14, 2023
76d9621
resource to import/...
comcalvi Dec 14, 2023
645feb2
more resources to import
comcalvi Dec 14, 2023
f90ba59
naming
comcalvi Dec 14, 2023
d186cc9
rework
comcalvi Dec 16, 2023
a81a5dd
testing
comcalvi Dec 16, 2023
52f86a1
error handling
comcalvi Dec 16, 2023
7d0d38f
dependsOn
comcalvi Dec 16, 2023
dec2261
rename
comcalvi Dec 16, 2023
d1b8533
nested stacks
comcalvi Dec 20, 2023
c9341ea
depends on improvements + comments
comcalvi Dec 20, 2023
ca152bb
1,1
comcalvi Dec 20, 2023
abfe661
done
comcalvi Dec 20, 2023
44041b1
unit test
comcalvi Dec 22, 2023
7b4d7e1
rename
comcalvi Jan 2, 2024
5c654da
cloudformation wowww
comcalvi Jan 3, 2024
3bd3b03
disable it, just for testing...
comcalvi Jan 3, 2024
0956232
Merge branch 'main' of github.com:aws/aws-cdk into comcalvi/changeset…
comcalvi Jan 3, 2024
b07e9ac
Merge branch 'main' of github.com:aws/aws-cdk into comcalvi/changeset…
comcalvi Jan 3, 2024
f0984a6
removing this fix, for now...
comcalvi Jan 3, 2024
c264294
comments
comcalvi Jan 3, 2024
7ebc5ce
nested stack test
comcalvi Jan 3, 2024
b35e538
idk what this is doing here...
comcalvi Jan 3, 2024
4f12072
Merge branch 'main' into comcalvi/changeset-diff
mergify[bot] Jan 4, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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);
comcalvi marked this conversation as resolved.
Show resolved Hide resolved
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',
rix0rrr marked this conversation as resolved.
Show resolved Hide resolved
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
Loading