Skip to content

Commit 526d4ad

Browse files
authored
refactor(diff): make dedicated file and class for incorporating changeset to templateDiff (#30332)
### Reason for this change I am making this change as part of #30268, but implementing the bug fix in a satisfactory way is becoming much, much, much more difficult than I thought it would. As it's now possible to view the changed values before and after a changeset is applied by using the DescribeChangeSets api with IncludePropertyValues, but the API is difficult to use because of not being supported in all regions, not including StatusReason, and being unable to paginate. So, I want to make that fix in a separate PR, once this refactor change is done. ### Description of changes * A ton of unit tests and moved changeset diff logic into a dedicated class and file. ### Description of how you validated changes * Many unit tests, integration tests, and manual tests ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent f54a945 commit 526d4ad

File tree

8 files changed

+1563
-715
lines changed

8 files changed

+1563
-715
lines changed

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

+7-97
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// The SDK should not make network calls here
33
import type { DescribeChangeSetOutput as DescribeChangeSet } from '@aws-sdk/client-cloudformation';
44
import * as impl from './diff';
5+
import { TemplateAndChangeSetDiffMerger } from './diff/template-and-changeset-diff-merger';
56
import * as types from './diff/types';
67
import { deepEqual, diffKeyedEntities, unionOf } from './diff/util';
78

@@ -55,8 +56,12 @@ export function fullDiff(
5556
normalize(newTemplate);
5657
const theDiff = diffTemplate(currentTemplate, newTemplate);
5758
if (changeSet) {
58-
filterFalsePositives(theDiff, changeSet);
59-
addImportInformation(theDiff, changeSet);
59+
// These methods mutate the state of theDiff, using the changeSet.
60+
const changeSetDiff = new TemplateAndChangeSetDiffMerger({ changeSet });
61+
theDiff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) =>
62+
changeSetDiff.overrideDiffResourceChangeImpactWithChangeSetChangeImpact(logicalId, change),
63+
);
64+
changeSetDiff.addImportInformationFromChangeset(theDiff.resources);
6065
} else if (isImport) {
6166
makeAllResourceChangesImports(theDiff);
6267
}
@@ -143,13 +148,6 @@ function calculateTemplateDiff(currentTemplate: { [key: string]: any }, newTempl
143148
return new types.TemplateDiff(differences);
144149
}
145150

146-
/**
147-
* Compare two CloudFormation resources and return semantic differences between them
148-
*/
149-
export function diffResource(oldValue: types.Resource, newValue: types.Resource): types.ResourceDifference {
150-
return impl.diffResource(oldValue, newValue);
151-
}
152-
153151
/**
154152
* Replace all references to the given logicalID on the given template, in-place
155153
*
@@ -214,100 +212,12 @@ function deepCopy(x: any): any {
214212
return x;
215213
}
216214

217-
function addImportInformation(diff: types.TemplateDiff, changeSet: DescribeChangeSetOutput) {
218-
const imports = findResourceImports(changeSet);
219-
diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => {
220-
if (imports.includes(logicalId)) {
221-
change.isImport = true;
222-
}
223-
});
224-
}
225-
226215
function makeAllResourceChangesImports(diff: types.TemplateDiff) {
227216
diff.resources.forEachDifference((_logicalId: string, change: types.ResourceDifference) => {
228217
change.isImport = true;
229218
});
230219
}
231220

232-
function filterFalsePositives(diff: types.TemplateDiff, changeSet: DescribeChangeSetOutput) {
233-
const replacements = findResourceReplacements(changeSet);
234-
diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => {
235-
if (change.resourceType.includes('AWS::Serverless')) {
236-
// CFN applies the SAM transform before creating the changeset, so the changeset contains no information about SAM resources
237-
return;
238-
}
239-
change.forEachDifference((type: 'Property' | 'Other', name: string, value: types.Difference<any> | types.PropertyDifference<any>) => {
240-
if (type === 'Property') {
241-
if (!replacements[logicalId]) {
242-
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE;
243-
(value as types.PropertyDifference<any>).isDifferent = false;
244-
return;
245-
}
246-
switch (replacements[logicalId].propertiesReplaced[name]) {
247-
case 'Always':
248-
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.WILL_REPLACE;
249-
break;
250-
case 'Never':
251-
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.WILL_UPDATE;
252-
break;
253-
case 'Conditionally':
254-
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.MAY_REPLACE;
255-
break;
256-
case undefined:
257-
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE;
258-
(value as types.PropertyDifference<any>).isDifferent = false;
259-
break;
260-
// otherwise, defer to the changeImpact from `diffTemplate`
261-
}
262-
} else if (type === 'Other') {
263-
switch (name) {
264-
case 'Metadata':
265-
change.setOtherChange('Metadata', new types.Difference<string>(value.newValue, value.newValue));
266-
break;
267-
}
268-
}
269-
});
270-
});
271-
}
272-
273-
function findResourceImports(changeSet: DescribeChangeSetOutput): string[] {
274-
const importedResourceLogicalIds = [];
275-
for (const resourceChange of changeSet.Changes ?? []) {
276-
if (resourceChange.ResourceChange?.Action === 'Import') {
277-
importedResourceLogicalIds.push(resourceChange.ResourceChange.LogicalResourceId!);
278-
}
279-
}
280-
281-
return importedResourceLogicalIds;
282-
}
283-
284-
function findResourceReplacements(changeSet: DescribeChangeSetOutput): types.ResourceReplacements {
285-
const replacements: types.ResourceReplacements = {};
286-
for (const resourceChange of changeSet.Changes ?? []) {
287-
const propertiesReplaced: { [propName: string]: types.ChangeSetReplacement } = {};
288-
for (const propertyChange of resourceChange.ResourceChange?.Details ?? []) {
289-
if (propertyChange.Target?.Attribute === 'Properties') {
290-
const requiresReplacement = propertyChange.Target.RequiresRecreation === 'Always';
291-
if (requiresReplacement && propertyChange.Evaluation === 'Static') {
292-
propertiesReplaced[propertyChange.Target.Name!] = 'Always';
293-
} else if (requiresReplacement && propertyChange.Evaluation === 'Dynamic') {
294-
// If Evaluation is 'Dynamic', then this may cause replacement, or it may not.
295-
// see 'Replacement': https://docs.aws.amazon.com/AWSCloudFormation/latest/APIReference/API_ResourceChange.html
296-
propertiesReplaced[propertyChange.Target.Name!] = 'Conditionally';
297-
} else {
298-
propertiesReplaced[propertyChange.Target.Name!] = propertyChange.Target.RequiresRecreation as types.ChangeSetReplacement;
299-
}
300-
}
301-
}
302-
replacements[resourceChange.ResourceChange?.LogicalResourceId!] = {
303-
resourceReplaced: resourceChange.ResourceChange?.Replacement === 'True',
304-
propertiesReplaced,
305-
};
306-
}
307-
308-
return replacements;
309-
}
310-
311221
function normalize(template: any) {
312222
if (typeof template === 'object') {
313223
for (const key of (Object.keys(template ?? {}))) {
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
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+
import type { DescribeChangeSetOutput as DescribeChangeSet, ResourceChangeDetail as RCD } from '@aws-sdk/client-cloudformation';
4+
import * as types from '../diff/types';
5+
6+
export type DescribeChangeSetOutput = DescribeChangeSet;
7+
type ChangeSetResourceChangeDetail = RCD;
8+
9+
interface TemplateAndChangeSetDiffMergerOptions {
10+
/*
11+
* Only specifiable for testing. Otherwise, this is the datastructure that the changeSet is converted into so
12+
* that we only pay attention to the subset of changeSet properties that are relevant for computing the diff.
13+
*
14+
* @default - the changeSet is converted into this datastructure.
15+
*/
16+
readonly changeSetResources?: types.ChangeSetResources;
17+
}
18+
19+
export interface TemplateAndChangeSetDiffMergerProps extends TemplateAndChangeSetDiffMergerOptions {
20+
/*
21+
* The changeset that will be read and merged into the template diff.
22+
*/
23+
readonly changeSet: DescribeChangeSetOutput;
24+
}
25+
26+
/**
27+
* The purpose of this class is to include differences from the ChangeSet to differences in the TemplateDiff.
28+
*/
29+
export class TemplateAndChangeSetDiffMerger {
30+
31+
public static determineChangeSetReplacementMode(propertyChange: ChangeSetResourceChangeDetail): types.ReplacementModes {
32+
if (propertyChange.Target?.RequiresRecreation === undefined) {
33+
// We can't determine if the resource will be replaced or not. That's what conditionally means.
34+
return 'Conditionally';
35+
}
36+
37+
if (propertyChange.Target.RequiresRecreation === 'Always') {
38+
switch (propertyChange.Evaluation) {
39+
case 'Static':
40+
return 'Always';
41+
case 'Dynamic':
42+
// If Evaluation is 'Dynamic', then this may cause replacement, or it may not.
43+
// see 'Replacement': https://docs.aws.amazon.com/AWSCloudFormation/latest/APIReference/API_ResourceChange.html
44+
return 'Conditionally';
45+
}
46+
}
47+
48+
return propertyChange.Target.RequiresRecreation as types.ReplacementModes;
49+
}
50+
51+
// If we somehow cannot find the resourceType, then we'll mark it as UNKNOWN, so that can be seen in the diff.
52+
private static UNKNOWN_RESOURCE_TYPE = 'UNKNOWN_RESOURCE_TYPE';
53+
54+
public changeSet: DescribeChangeSetOutput | undefined;
55+
public changeSetResources: types.ChangeSetResources;
56+
57+
constructor(props: TemplateAndChangeSetDiffMergerProps) {
58+
this.changeSet = props.changeSet;
59+
this.changeSetResources = props.changeSetResources ?? this.convertDescribeChangeSetOutputToChangeSetResources(this.changeSet);
60+
}
61+
62+
/**
63+
* Read resources from the changeSet, extracting information into ChangeSetResources.
64+
*/
65+
private convertDescribeChangeSetOutputToChangeSetResources(changeSet: DescribeChangeSetOutput): types.ChangeSetResources {
66+
const changeSetResources: types.ChangeSetResources = {};
67+
for (const resourceChange of changeSet.Changes ?? []) {
68+
if (resourceChange.ResourceChange?.LogicalResourceId === undefined) {
69+
continue; // Being defensive, here.
70+
}
71+
72+
const propertyReplacementModes: types.PropertyReplacementModeMap = {};
73+
for (const propertyChange of resourceChange.ResourceChange.Details ?? []) { // Details is only included if resourceChange.Action === 'Modify'
74+
if (propertyChange.Target?.Attribute === 'Properties' && propertyChange.Target.Name) {
75+
propertyReplacementModes[propertyChange.Target.Name] = {
76+
replacementMode: TemplateAndChangeSetDiffMerger.determineChangeSetReplacementMode(propertyChange),
77+
};
78+
}
79+
}
80+
81+
changeSetResources[resourceChange.ResourceChange.LogicalResourceId] = {
82+
resourceWasReplaced: resourceChange.ResourceChange.Replacement === 'True',
83+
resourceType: resourceChange.ResourceChange.ResourceType ?? TemplateAndChangeSetDiffMerger.UNKNOWN_RESOURCE_TYPE, // DescribeChangeSet doesn't promise to have the ResourceType...
84+
propertyReplacementModes: propertyReplacementModes,
85+
};
86+
}
87+
88+
return changeSetResources;
89+
}
90+
91+
/**
92+
* This is writing over the "ChangeImpact" that was computed from the template difference, and instead using the ChangeImpact that is included from the ChangeSet.
93+
* Using the ChangeSet ChangeImpact is more accurate. The ChangeImpact tells us what the consequence is of changing the field. If changing the field causes resource
94+
* replacement (e.g., changing the name of an IAM role requires deleting and replacing the role), then ChangeImpact is "Always".
95+
*/
96+
public overrideDiffResourceChangeImpactWithChangeSetChangeImpact(logicalId: string, change: types.ResourceDifference) {
97+
// resourceType getter throws an error if resourceTypeChanged
98+
if ((change.resourceTypeChanged === true) || change.resourceType?.includes('AWS::Serverless')) {
99+
// CFN applies the SAM transform before creating the changeset, so the changeset contains no information about SAM resources
100+
return;
101+
}
102+
change.forEachDifference((type: 'Property' | 'Other', name: string, value: types.Difference<any> | types.PropertyDifference<any>) => {
103+
if (type === 'Property') {
104+
if (!this.changeSetResources[logicalId]) {
105+
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE;
106+
(value as types.PropertyDifference<any>).isDifferent = false;
107+
return;
108+
}
109+
110+
const changingPropertyCausesResourceReplacement = (this.changeSetResources[logicalId].propertyReplacementModes ?? {})[name]?.replacementMode;
111+
switch (changingPropertyCausesResourceReplacement) {
112+
case 'Always':
113+
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.WILL_REPLACE;
114+
break;
115+
case 'Never':
116+
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.WILL_UPDATE;
117+
break;
118+
case 'Conditionally':
119+
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.MAY_REPLACE;
120+
break;
121+
case undefined:
122+
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE;
123+
(value as types.PropertyDifference<any>).isDifferent = false;
124+
break;
125+
// otherwise, defer to the changeImpact from the template diff
126+
}
127+
} else if (type === 'Other') {
128+
switch (name) {
129+
case 'Metadata':
130+
// we want to ignore metadata changes in the diff, so compare newValue against newValue.
131+
change.setOtherChange('Metadata', new types.Difference<string>(value.newValue, value.newValue));
132+
break;
133+
}
134+
}
135+
});
136+
}
137+
138+
public addImportInformationFromChangeset(resourceDiffs: types.DifferenceCollection<types.Resource, types.ResourceDifference>) {
139+
const imports = this.findResourceImports();
140+
resourceDiffs.forEachDifference((logicalId: string, change: types.ResourceDifference) => {
141+
if (imports.includes(logicalId)) {
142+
change.isImport = true;
143+
}
144+
});
145+
}
146+
147+
public findResourceImports(): (string | undefined)[] {
148+
const importedResourceLogicalIds = [];
149+
for (const resourceChange of this.changeSet?.Changes ?? []) {
150+
if (resourceChange.ResourceChange?.Action === 'Import') {
151+
importedResourceLogicalIds.push(resourceChange.ResourceChange.LogicalResourceId);
152+
}
153+
}
154+
155+
return importedResourceLogicalIds;
156+
}
157+
}

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

+30-7
Original file line numberDiff line numberDiff line change
@@ -6,14 +6,33 @@ import { SecurityGroupChanges } from '../network/security-group-changes';
66

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

9-
export type ResourceReplacements = { [logicalId: string]: ResourceReplacement };
9+
export type ChangeSetResources = { [logicalId: string]: ChangeSetResource };
1010

11-
export interface ResourceReplacement {
12-
resourceReplaced: boolean;
13-
propertiesReplaced: { [propertyName: string]: ChangeSetReplacement };
11+
/**
12+
* @param beforeContext is the BeforeContext field from the ChangeSet.ResourceChange.BeforeContext. This is the part of the CloudFormation template
13+
* that defines what the resource is before the change is applied; that is, BeforeContext is CloudFormationTemplate.Resources[LogicalId] before the ChangeSet is executed.
14+
*
15+
* @param afterContext same as beforeContext but for after the change is made; that is, AfterContext is CloudFormationTemplate.Resources[LogicalId] after the ChangeSet is executed.
16+
*
17+
* * Here is an example of what a beforeContext/afterContext looks like:
18+
* '{"Properties":{"Value":"sdflkja","Type":"String","Name":"mySsmParameterFromStack"},"Metadata":{"aws:cdk:path":"cdk/mySsmParameter/Resource"}}'
19+
*/
20+
export interface ChangeSetResource {
21+
resourceWasReplaced: boolean;
22+
resourceType: string | undefined;
23+
propertyReplacementModes: PropertyReplacementModeMap | undefined;
1424
}
1525

16-
export type ChangeSetReplacement = 'Always' | 'Never' | 'Conditionally';
26+
export type PropertyReplacementModeMap = {
27+
[propertyName: string]: {
28+
replacementMode: ReplacementModes | undefined;
29+
};
30+
}
31+
32+
/**
33+
* 'Always' means that changing the corresponding property will always cause a resource replacement. Never means never. Conditionally means maybe.
34+
*/
35+
export type ReplacementModes = 'Always' | 'Never' | 'Conditionally';
1736

1837
/** Semantic differences between two CloudFormation templates. */
1938
export class TemplateDiff implements ITemplateDiff {
@@ -198,6 +217,10 @@ export class TemplateDiff implements ITemplateDiff {
198217
}
199218
}
200219
} else {
220+
if (!resourceChange.resourceType) {
221+
continue;
222+
}
223+
201224
const resourceModel = loadResourceModel(resourceChange.resourceType);
202225
if (resourceModel && this.resourceIsScrutinizable(resourceModel, scrutinyTypes)) {
203226
ret.push({
@@ -626,11 +649,11 @@ export class ResourceDifference implements IDifference<Resource> {
626649
*
627650
* If the resource type was changed, it's an error to call this.
628651
*/
629-
public get resourceType(): string {
652+
public get resourceType(): string | undefined {
630653
if (this.resourceTypeChanged) {
631654
throw new Error('Cannot get .resourceType, because the type was changed');
632655
}
633-
return this.resourceTypes.oldType || this.resourceTypes.newType!;
656+
return this.resourceTypes.oldType || this.resourceTypes.newType;
634657
}
635658

636659
/**

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ const UPDATE = chalk.yellow('[~]');
8181
const REMOVAL = chalk.red('[-]');
8282
const IMPORT = chalk.blue('[←]');
8383

84-
class Formatter {
84+
export class Formatter {
8585
constructor(
8686
private readonly stream: FormatStream,
8787
private readonly logicalToPathMap: { [logicalId: string]: string },

0 commit comments

Comments
 (0)