Skip to content

Commit 811e224

Browse files
authored
revert: "fix(diff): properties from ChangeSet diff were ignored" (#30243)
Reverts: [30093](#30093)
1 parent 2f7030a commit 811e224

File tree

8 files changed

+73
-530
lines changed

8 files changed

+73
-530
lines changed

packages/@aws-cdk-testing/cli-integ/lib/aws.ts

-2
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ export class AwsClients {
2020
public readonly ecr: AwsCaller<AWS.ECR>;
2121
public readonly ecs: AwsCaller<AWS.ECS>;
2222
public readonly sso: AwsCaller<AWS.SSO>;
23-
public readonly ssm: AwsCaller<AWS.SSM>;
2423
public readonly sns: AwsCaller<AWS.SNS>;
2524
public readonly iam: AwsCaller<AWS.IAM>;
2625
public readonly lambda: AwsCaller<AWS.Lambda>;
@@ -40,7 +39,6 @@ export class AwsClients {
4039
this.ecs = makeAwsCaller(AWS.ECS, this.config);
4140
this.sso = makeAwsCaller(AWS.SSO, this.config);
4241
this.sns = makeAwsCaller(AWS.SNS, this.config);
43-
this.ssm = makeAwsCaller(AWS.SSM, this.config);
4442
this.iam = makeAwsCaller(AWS.IAM, this.config);
4543
this.lambda = makeAwsCaller(AWS.Lambda, this.config);
4644
this.sts = makeAwsCaller(AWS.STS, this.config);

packages/@aws-cdk-testing/cli-integ/package.json

+4-4
Original file line numberDiff line numberDiff line change
@@ -30,12 +30,12 @@
3030
"license": "Apache-2.0",
3131
"devDependencies": {
3232
"@aws-cdk/cdk-build-tools": "0.0.0",
33-
"@aws-cdk/pkglint": "0.0.0",
33+
"@types/semver": "^7.5.8",
34+
"@types/yargs": "^15.0.19",
3435
"@types/fs-extra": "^9.0.13",
3536
"@types/glob": "^7.2.0",
3637
"@types/npm": "^7.19.3",
37-
"@types/semver": "^7.5.8",
38-
"@types/yargs": "^15.0.19"
38+
"@aws-cdk/pkglint": "0.0.0"
3939
},
4040
"dependencies": {
4141
"@octokit/rest": "^18.12.0",
@@ -72,4 +72,4 @@
7272
"publishConfig": {
7373
"tag": "latest"
7474
}
75-
}
75+
}

packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js

-18
Original file line numberDiff line numberDiff line change
@@ -126,22 +126,6 @@ class SsoInstanceAccessControlConfig extends Stack {
126126
}
127127
}
128128

129-
class DiffFromChangeSetStack extends Stack {
130-
constructor(scope, id) {
131-
super(scope, id);
132-
133-
const queueNameFromParameter = ssm.StringParameter.valueForStringParameter(this, 'for-queue-name-defined-by-ssm-param');
134-
new sqs.Queue(this, "DiffFromChangeSetQueue", {
135-
queueName: queueNameFromParameter,
136-
})
137-
138-
new ssm.StringParameter(this, 'DiffFromChangeSetSSMParam', {
139-
parameterName: 'DiffFromChangeSetSSMParamName',
140-
stringValue: queueNameFromParameter,
141-
});
142-
}
143-
}
144-
145129
class ListMultipleDependentStack extends Stack {
146130
constructor(scope, id) {
147131
super(scope, id);
@@ -701,8 +685,6 @@ switch (stackSet) {
701685

702686
const failed = new FailedStack(app, `${stackPrefix}-failed`)
703687

704-
new DiffFromChangeSetStack(app, `${stackPrefix}-queue-name-defined-by-ssm-param`)
705-
706688
// A stack that depends on the failed stack -- used to test that '-e' does not deploy the failing stack
707689
const dependsOnFailed = new OutputsStack(app, `${stackPrefix}-depends-on-failed`);
708690
dependsOnFailed.addDependency(failed);

packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts

-44
Original file line numberDiff line numberDiff line change
@@ -944,50 +944,6 @@ integTest('cdk diff --quiet does not print \'There were no differences\' message
944944
expect(diff).not.toContain('There were no differences');
945945
}));
946946

947-
integTest('cdk diff picks up changes that are only present in changeset', withDefaultFixture(async (fixture) => {
948-
// GIVEN
949-
await fixture.aws.ssm('putParameter', {
950-
Name: 'for-queue-name-defined-by-ssm-param',
951-
Value: randomString(),
952-
Type: 'String',
953-
Overwrite: true,
954-
});
955-
956-
try {
957-
await fixture.cdkDeploy('queue-name-defined-by-ssm-param');
958-
959-
// WHEN
960-
// We want to change the ssm value. Then the CFN changeset will detect that the queue will be changed upon deploy.
961-
await fixture.aws.ssm('putParameter', {
962-
Name: 'for-queue-name-defined-by-ssm-param',
963-
Value: randomString(),
964-
Type: 'String',
965-
Overwrite: true,
966-
});
967-
968-
const diff = await fixture.cdk(['diff', fixture.fullStackName('queue-name-defined-by-ssm-param')]);
969-
970-
// THEN
971-
const normalizedPlainTextOutput = diff.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '') // remove all color and formatting (bolding, italic, etc)
972-
.replace(/ /g, '') // remove all spaces
973-
.replace(/\n/g, '') // remove all new lines
974-
.replace(/\d+/g, ''); // remove all digits
975-
976-
const normalizedExpectedOutput = `
977-
Resources
978-
[~] AWS::SQS::Queue DiffFromChangeSetQueue DiffFromChangeSetQueue06622C07 replace
979-
└─ [~] QueueName (requires replacement)
980-
[~] AWS::SSM::Parameter DiffFromChangeSetSSMParam DiffFromChangeSetSSMParam92A9A723
981-
└─ [~] Value`
982-
.replace(/ /g, '')
983-
.replace(/\n/g, '')
984-
.replace(/\d+/g, '');
985-
expect(normalizedPlainTextOutput).toContain(normalizedExpectedOutput);
986-
} finally {
987-
await fixture.cdkDestroy('queue-name-defined-by-ssm-param');
988-
}
989-
}));
990-
991947
integTest('deploy stack with docker asset', withDefaultFixture(async (fixture) => {
992948
await fixture.cdkDeploy('docker');
993949
}));

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

+67-91
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ export function fullDiff(
5555
normalize(newTemplate);
5656
const theDiff = diffTemplate(currentTemplate, newTemplate);
5757
if (changeSet) {
58-
refineDiffWithChangeSet(theDiff, changeSet, newTemplate.Resources);
58+
filterFalsePositives(theDiff, changeSet);
5959
addImportInformation(theDiff, changeSet);
6060
} else if (isImport) {
6161
makeAllResourceChangesImports(theDiff);
@@ -143,6 +143,13 @@ function calculateTemplateDiff(currentTemplate: { [key: string]: any }, newTempl
143143
return new types.TemplateDiff(differences);
144144
}
145145

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+
146153
/**
147154
* Replace all references to the given logicalID on the given template, in-place
148155
*
@@ -222,103 +229,45 @@ function makeAllResourceChangesImports(diff: types.TemplateDiff) {
222229
});
223230
}
224231

225-
function refineDiffWithChangeSet(diff: types.TemplateDiff, changeSet: DescribeChangeSetOutput, newTemplateResources: {[logicalId: string]: any}) {
226-
const replacements = _findResourceReplacements(changeSet);
227-
228-
_addChangeSetResourcesToDiff(replacements, newTemplateResources);
229-
_enhanceChangeImpacts(replacements);
230-
return;
231-
232-
function _findResourceReplacements(_changeSet: DescribeChangeSetOutput): types.ResourceReplacements {
233-
const _replacements: types.ResourceReplacements = {};
234-
for (const resourceChange of _changeSet.Changes ?? []) {
235-
const propertiesReplaced: { [propName: string]: types.ChangeSetReplacement } = {};
236-
for (const propertyChange of resourceChange.ResourceChange?.Details ?? []) {
237-
if (propertyChange.Target?.Attribute === 'Properties') {
238-
const requiresReplacement = propertyChange.Target.RequiresRecreation === 'Always';
239-
if (requiresReplacement && propertyChange.Evaluation === 'Static') {
240-
propertiesReplaced[propertyChange.Target.Name!] = 'Always';
241-
} else if (requiresReplacement && propertyChange.Evaluation === 'Dynamic') {
242-
// If Evaluation is 'Dynamic', then this may cause replacement, or it may not.
243-
// see 'Replacement': https://docs.aws.amazon.com/AWSCloudFormation/latest/APIReference/API_ResourceChange.html
244-
propertiesReplaced[propertyChange.Target.Name!] = 'Conditionally';
245-
} else {
246-
propertiesReplaced[propertyChange.Target.Name!] = propertyChange.Target.RequiresRecreation as types.ChangeSetReplacement;
247-
}
248-
}
249-
}
250-
_replacements[resourceChange.ResourceChange?.LogicalResourceId!] = {
251-
resourceReplaced: resourceChange.ResourceChange?.Replacement === 'True',
252-
propertiesReplaced,
253-
};
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;
254238
}
255-
256-
return _replacements;
257-
}
258-
259-
function _addChangeSetResourcesToDiff(_replacements: types.ResourceReplacements, _newTemplateResources: {[logicalId: string]: any}) {
260-
const resourceDiffLogicalIds = diff.resources.logicalIds;
261-
for (const logicalId of Object.keys(_replacements)) {
262-
if (!(resourceDiffLogicalIds.includes(logicalId))) {
263-
const noChangeResourceDiff = impl.diffResource(_newTemplateResources[logicalId], _newTemplateResources[logicalId]);
264-
diff.resources.add(logicalId, noChangeResourceDiff);
265-
}
266-
267-
for (const propertyName of Object.keys(_replacements[logicalId].propertiesReplaced)) {
268-
if (propertyName in diff.resources.get(logicalId).propertyUpdates) {
269-
// If the property is already marked to be updated, then we don't need to do anything.
270-
continue;
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;
271245
}
272-
273-
const newProp = new types.PropertyDifference(
274-
// these fields will be decided below
275-
{}, {}, { changeImpact: undefined },
276-
);
277-
newProp.isDifferent = true;
278-
diff.resources.get(logicalId).setPropertyChange(propertyName, newProp);
279-
}
280-
};
281-
}
282-
283-
function _enhanceChangeImpacts(_replacements: types.ResourceReplacements) {
284-
diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => {
285-
if (change.resourceType.includes('AWS::Serverless')) {
286-
// CFN applies the SAM transform before creating the changeset, so the changeset contains no information about SAM resources
287-
return;
288-
}
289-
change.forEachDifference((type: 'Property' | 'Other', name: string, value: types.Difference<any> | types.PropertyDifference<any>) => {
290-
if (type === 'Property') {
291-
if (!_replacements[logicalId]) {
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:
292257
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE;
293258
(value as types.PropertyDifference<any>).isDifferent = false;
294-
return;
295-
}
296-
switch (_replacements[logicalId].propertiesReplaced[name]) {
297-
case 'Always':
298-
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.WILL_REPLACE;
299-
break;
300-
case 'Never':
301-
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.WILL_UPDATE;
302-
break;
303-
case 'Conditionally':
304-
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.MAY_REPLACE;
305-
break;
306-
case undefined:
307-
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE;
308-
(value as types.PropertyDifference<any>).isDifferent = false;
309-
break;
259+
break;
310260
// otherwise, defer to the changeImpact from `diffTemplate`
311-
}
312-
} else if (type === 'Other') {
313-
switch (name) {
314-
case 'Metadata':
315-
change.setOtherChange('Metadata', new types.Difference<string>(value.newValue, value.newValue));
316-
break;
317-
}
318261
}
319-
});
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+
}
320269
});
321-
}
270+
});
322271
}
323272

324273
function findResourceImports(changeSet: DescribeChangeSetOutput): string[] {
@@ -332,6 +281,33 @@ function findResourceImports(changeSet: DescribeChangeSetOutput): string[] {
332281
return importedResourceLogicalIds;
333282
}
334283

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+
335311
function normalize(template: any) {
336312
if (typeof template === 'object') {
337313
for (const key of (Object.keys(template ?? {}))) {

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

-4
Original file line numberDiff line numberDiff line change
@@ -349,10 +349,6 @@ export class PropertyDifference<ValueType> extends Difference<ValueType> {
349349
export class DifferenceCollection<V, T extends IDifference<V>> {
350350
constructor(private readonly diffs: { [logicalId: string]: T }) {}
351351

352-
public add(logicalId: string, diff: T): void {
353-
this.diffs[logicalId] = diff;
354-
}
355-
356352
public get changes(): { [logicalId: string]: T } {
357353
return onlyChanges(this.diffs);
358354
}

0 commit comments

Comments
 (0)