Skip to content

Commit 9c3f3f5

Browse files
bergjaakbergjaak
authored
fix(diff): properties from ChangeSet diff were ignored (#30093)
### Issue # (if applicable) Closes #29731 ### Reason for this change * Diffs that are only present in the change set are ignored ### Description of changes * Include changes to properties that are only present in the changeset ### Description of how you validated changes * Here is an image of what the change looks like, with the fix on the right and the old behavior on the left. I did this with a CDK app that is the same as given in the example from the customer who opened the issue (29731), but the app also includes a new queue (which is in the left and right). <img width="851" alt="29731Fix" src="https://github.com/aws/aws-cdk/assets/108292982/2c6e9464-5c36-4697-88fd-2929cdbcf8cc"> * manual, unit, and integration tested. Ran all integration tests that mention `cdk diff`: ``` [INTEG TEST::cdk diff] Starting (pid 34550)... [INTEG TEST::cdk diff] Done (13725 ms). [INTEG TEST::cdk diff --fail on multiple stacks exits with error if any of the stacks contains a diff] Starting (pid 34550)... [INTEG TEST::cdk diff --fail on multiple stacks exits with error if any of the stacks contains a diff] Done (80833 ms). [INTEG TEST::cdk diff --fail with multiple stack exits with if any of the stacks contains a diff] Starting (pid 34550)... [INTEG TEST::cdk diff --fail with multiple stack exits with if any of the stacks contains a diff] Done (81796 ms). [INTEG TEST::cdk diff --security-only successfully outputs sso-permission-set-without-managed-policy information] Starting (pid 34550)... [INTEG TEST::cdk diff --security-only successfully outputs sso-permission-set-without-managed-policy information] Done (7394 ms). [INTEG TEST::cdk diff --security-only successfully outputs sso-permission-set-with-managed-policy information] Starting (pid 34550)... [INTEG TEST::cdk diff --security-only successfully outputs sso-permission-set-with-managed-policy information] Done (7043 ms). [INTEG TEST::cdk diff --security-only successfully outputs sso-assignment information] Starting (pid 34550)... [INTEG TEST::cdk diff --security-only successfully outputs sso-assignment information] Done (6930 ms). [INTEG TEST::cdk diff --security-only successfully outputs sso-access-control information] Starting (pid 34550)... [INTEG TEST::cdk diff --security-only successfully outputs sso-access-control information] Done (6958 ms). [INTEG TEST::cdk diff --security-only --fail exits when security diff for sso access control config] Starting (pid 34550)... [INTEG TEST::cdk diff --security-only --fail exits when security diff for sso access control config] Done (6945 ms). [INTEG TEST::cdk diff --security-only --fail exits when security diff for sso-perm-set-without-managed-policy] Starting (pid 34550)... [INTEG TEST::cdk diff --security-only --fail exits when security diff for sso-perm-set-without-managed-policy] Done (7042 ms). [INTEG TEST::cdk diff --security-only --fail exits when security diff for sso-perm-set-with-managed-policy] Starting (pid 34550)... [INTEG TEST::cdk diff --security-only --fail exits when security diff for sso-perm-set-with-managed-policy] Done (7131 ms). [INTEG TEST::cdk diff --security-only --fail exits when security diff for sso-assignment] Starting (pid 34550)... [INTEG TEST::cdk diff --security-only --fail exits when security diff for sso-assignment] Done (7225 ms). [INTEG TEST::cdk diff --security-only --fail exits when security changes are present] Starting (pid 34550)... [INTEG TEST::cdk diff --security-only --fail exits when security changes are present] Done (7124 ms). [INTEG TEST::cdk diff --quiet does not print 'There were no differences' message for stacks which have no differences] Starting (pid 34550)... [INTEG TEST::cdk diff --quiet does not print 'There were no differences' message for stacks which have no differences] Done (75387 ms). [INTEG TEST::cdk diff picks up changes that are only present in changeset] Starting (pid 34550)... [INTEG TEST::cdk diff picks up changes that are only present in changeset] Done (98624 ms). PASS tests/cli-integ-tests/cli.integtest.js (414.667 s) ● Console console.log Using regions: us-east-1 at log (../../lib/with-aws.ts:36:11) Test Suites: 2 skipped, 1 passed, 1 of 3 total Tests: 90 skipped, 14 passed, 104 total Snapshots: 0 total Time: 414.86 s Ran all test suites with tests matching "cdk diff". ``` ### 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 495204f commit 9c3f3f5

File tree

8 files changed

+528
-71
lines changed

8 files changed

+528
-71
lines changed

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

+2
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ 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>;
2324
public readonly sns: AwsCaller<AWS.SNS>;
2425
public readonly iam: AwsCaller<AWS.IAM>;
2526
public readonly lambda: AwsCaller<AWS.Lambda>;
@@ -39,6 +40,7 @@ export class AwsClients {
3940
this.ecs = makeAwsCaller(AWS.ECS, this.config);
4041
this.sso = makeAwsCaller(AWS.SSO, this.config);
4142
this.sns = makeAwsCaller(AWS.SNS, this.config);
43+
this.ssm = makeAwsCaller(AWS.SSM, this.config);
4244
this.iam = makeAwsCaller(AWS.IAM, this.config);
4345
this.lambda = makeAwsCaller(AWS.Lambda, this.config);
4446
this.sts = makeAwsCaller(AWS.STS, this.config);

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

+3-3
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-
"@types/semver": "^7.5.8",
34-
"@types/yargs": "^15.0.19",
33+
"@aws-cdk/pkglint": "0.0.0",
3534
"@types/fs-extra": "^9.0.13",
3635
"@types/glob": "^7.2.0",
3736
"@types/npm": "^7.19.3",
38-
"@aws-cdk/pkglint": "0.0.0"
37+
"@types/semver": "^7.5.8",
38+
"@types/yargs": "^15.0.19"
3939
},
4040
"dependencies": {
4141
"@octokit/rest": "^18.12.0",

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

+18
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,22 @@ 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+
129145
class ListMultipleDependentStack extends Stack {
130146
constructor(scope, id) {
131147
super(scope, id);
@@ -658,6 +674,8 @@ switch (stackSet) {
658674

659675
const failed = new FailedStack(app, `${stackPrefix}-failed`)
660676

677+
new DiffFromChangeSetStack(app, `${stackPrefix}-queue-name-defined-by-ssm-param`)
678+
661679
// A stack that depends on the failed stack -- used to test that '-e' does not deploy the failing stack
662680
const dependsOnFailed = new OutputsStack(app, `${stackPrefix}-depends-on-failed`);
663681
dependsOnFailed.addDependency(failed);

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

+45-1
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { promises as fs, existsSync } from 'fs';
22
import * as os from 'os';
33
import * as path from 'path';
4-
import { integTest, cloneDirectory, shell, withDefaultFixture, retry, sleep, randomInteger, withSamIntegrationFixture, RESOURCES_DIR, withCDKMigrateFixture, withExtendedTimeoutFixture } from '../../lib';
4+
import { integTest, cloneDirectory, shell, withDefaultFixture, retry, sleep, randomInteger, withSamIntegrationFixture, RESOURCES_DIR, withCDKMigrateFixture, withExtendedTimeoutFixture, randomString } from '../../lib';
55

66
jest.setTimeout(2 * 60 * 60_000); // Includes the time to acquire locks, worst-case single-threaded runtime
77

@@ -944,6 +944,50 @@ 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+
947991
integTest('deploy stack with docker asset', withDefaultFixture(async (fixture) => {
948992
await fixture.cdkDeploy('docker');
949993
}));

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

+91-67
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-
filterFalsePositives(theDiff, changeSet);
58+
refineDiffWithChangeSet(theDiff, changeSet, newTemplate.Resources);
5959
addImportInformation(theDiff, changeSet);
6060
} else if (isImport) {
6161
makeAllResourceChangesImports(theDiff);
@@ -143,13 +143,6 @@ 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-
153146
/**
154147
* Replace all references to the given logicalID on the given template, in-place
155148
*
@@ -229,45 +222,103 @@ function makeAllResourceChangesImports(diff: types.TemplateDiff) {
229222
});
230223
}
231224

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;
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+
};
238254
}
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;
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;
245271
}
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:
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]) {
257292
(value as types.PropertyDifference<any>).changeImpact = types.ResourceImpact.NO_CHANGE;
258293
(value as types.PropertyDifference<any>).isDifferent = false;
259-
break;
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;
260310
// 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+
}
261318
}
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-
}
319+
});
269320
});
270-
});
321+
}
271322
}
272323

273324
function findResourceImports(changeSet: DescribeChangeSetOutput): string[] {
@@ -281,33 +332,6 @@ function findResourceImports(changeSet: DescribeChangeSetOutput): string[] {
281332
return importedResourceLogicalIds;
282333
}
283334

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-
311335
function normalize(template: any) {
312336
if (typeof template === 'object') {
313337
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,6 +349,10 @@ 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+
352356
public get changes(): { [logicalId: string]: T } {
353357
return onlyChanges(this.diffs);
354358
}

0 commit comments

Comments
 (0)