Skip to content

Commit

Permalink
revert: "fix(diff): properties from ChangeSet diff were ignored" (#30243
Browse files Browse the repository at this point in the history 
)

Reverts: [30093](#30093)
  • Loading branch information
@SankyRed
SankyRed committed May 16, 2024
1 parent 2f7030a commit 811e224
Show file tree
Hide file tree
Showing 8 changed files with 73 additions and 530 deletions.
2 changes: 0 additions & 2 deletions packages/@aws-cdk-testing/cli-integ/lib/aws.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ export class AwsClients {
public readonly ecr: AwsCaller<AWS.ECR>;
public readonly ecs: AwsCaller<AWS.ECS>;
public readonly sso: AwsCaller<AWS.SSO>;
public readonly ssm: AwsCaller<AWS.SSM>;
public readonly sns: AwsCaller<AWS.SNS>;
public readonly iam: AwsCaller<AWS.IAM>;
public readonly lambda: AwsCaller<AWS.Lambda>;
Expand All @@ -40,7 +39,6 @@ export class AwsClients {
this.ecs = makeAwsCaller(AWS.ECS, this.config);
this.sso = makeAwsCaller(AWS.SSO, this.config);
this.sns = makeAwsCaller(AWS.SNS, this.config);
this.ssm = makeAwsCaller(AWS.SSM, this.config);
this.iam = makeAwsCaller(AWS.IAM, this.config);
this.lambda = makeAwsCaller(AWS.Lambda, this.config);
this.sts = makeAwsCaller(AWS.STS, this.config);
Expand Down
8 changes: 4 additions & 4 deletions packages/@aws-cdk-testing/cli-integ/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,12 @@
"license": "Apache-2.0",
"devDependencies": {
"@aws-cdk/cdk-build-tools": "0.0.0",
"@aws-cdk/pkglint": "0.0.0",
"@types/semver": "^7.5.8",
"@types/yargs": "^15.0.19",
"@types/fs-extra": "^9.0.13",
"@types/glob": "^7.2.0",
"@types/npm": "^7.19.3",
"@types/semver": "^7.5.8",
"@types/yargs": "^15.0.19"
"@aws-cdk/pkglint": "0.0.0"
},
"dependencies": {
"@octokit/rest": "^18.12.0",
Expand Down Expand Up @@ -72,4 +72,4 @@
"publishConfig": {
"tag": "latest"
}
}
}
18 changes: 0 additions & 18 deletions packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,22 +126,6 @@ class SsoInstanceAccessControlConfig extends Stack {
}
}

class DiffFromChangeSetStack extends Stack {
constructor(scope, id) {
super(scope, id);

const queueNameFromParameter = ssm.StringParameter.valueForStringParameter(this, 'for-queue-name-defined-by-ssm-param');
new sqs.Queue(this, "DiffFromChangeSetQueue", {
queueName: queueNameFromParameter,
})

new ssm.StringParameter(this, 'DiffFromChangeSetSSMParam', {
parameterName: 'DiffFromChangeSetSSMParamName',
stringValue: queueNameFromParameter,
});
}
}

class ListMultipleDependentStack extends Stack {
constructor(scope, id) {
super(scope, id);
Expand Down Expand Up @@ -701,8 +685,6 @@ switch (stackSet) {

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

new DiffFromChangeSetStack(app, `${stackPrefix}-queue-name-defined-by-ssm-param`)

// A stack that depends on the failed stack -- used to test that '-e' does not deploy the failing stack
const dependsOnFailed = new OutputsStack(app, `${stackPrefix}-depends-on-failed`);
dependsOnFailed.addDependency(failed);
Expand Down
44 changes: 0 additions & 44 deletions packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -944,50 +944,6 @@ integTest('cdk diff --quiet does not print \'There were no differences\' message
expect(diff).not.toContain('There were no differences');
}));

integTest('cdk diff picks up changes that are only present in changeset', withDefaultFixture(async (fixture) => {
// GIVEN
await fixture.aws.ssm('putParameter', {
Name: 'for-queue-name-defined-by-ssm-param',
Value: randomString(),
Type: 'String',
Overwrite: true,
});

try {
await fixture.cdkDeploy('queue-name-defined-by-ssm-param');

// WHEN
// We want to change the ssm value. Then the CFN changeset will detect that the queue will be changed upon deploy.
await fixture.aws.ssm('putParameter', {
Name: 'for-queue-name-defined-by-ssm-param',
Value: randomString(),
Type: 'String',
Overwrite: true,
});

const diff = await fixture.cdk(['diff', fixture.fullStackName('queue-name-defined-by-ssm-param')]);

// THEN
const normalizedPlainTextOutput = diff.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '') // remove all color and formatting (bolding, italic, etc)
.replace(/ /g, '') // remove all spaces
.replace(/\n/g, '') // remove all new lines
.replace(/\d+/g, ''); // remove all digits

const normalizedExpectedOutput = `
Resources
[~] AWS::SQS::Queue DiffFromChangeSetQueue DiffFromChangeSetQueue06622C07 replace
└─ [~] QueueName (requires replacement)
[~] AWS::SSM::Parameter DiffFromChangeSetSSMParam DiffFromChangeSetSSMParam92A9A723
└─ [~] Value`
.replace(/ /g, '')
.replace(/\n/g, '')
.replace(/\d+/g, '');
expect(normalizedPlainTextOutput).toContain(normalizedExpectedOutput);
} finally {
await fixture.cdkDestroy('queue-name-defined-by-ssm-param');
}
}));

integTest('deploy stack with docker asset', withDefaultFixture(async (fixture) => {
await fixture.cdkDeploy('docker');
}));
Expand Down
158 changes: 67 additions & 91 deletions packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export function fullDiff(
normalize(newTemplate);
const theDiff = diffTemplate(currentTemplate, newTemplate);
if (changeSet) {
refineDiffWithChangeSet(theDiff, changeSet, newTemplate.Resources);
filterFalsePositives(theDiff, changeSet);
addImportInformation(theDiff, changeSet);
} else if (isImport) {
makeAllResourceChangesImports(theDiff);
Expand Down Expand Up @@ -143,6 +143,13 @@ function calculateTemplateDiff(currentTemplate: { [key: string]: any }, newTempl
return new types.TemplateDiff(differences);
}

/**
* Compare two CloudFormation resources and return semantic differences between them
*/
export function diffResource(oldValue: types.Resource, newValue: types.Resource): types.ResourceDifference {
return impl.diffResource(oldValue, newValue);
}

/**
* Replace all references to the given logicalID on the given template, in-place
*
Expand Down Expand Up @@ -222,103 +229,45 @@ function makeAllResourceChangesImports(diff: types.TemplateDiff) {
});
}

function refineDiffWithChangeSet(diff: types.TemplateDiff, changeSet: DescribeChangeSetOutput, newTemplateResources: {[logicalId: string]: any}) {
const replacements = _findResourceReplacements(changeSet);

_addChangeSetResourcesToDiff(replacements, newTemplateResources);
_enhanceChangeImpacts(replacements);
return;

function _findResourceReplacements(_changeSet: 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,
};
function filterFalsePositives(diff: types.TemplateDiff, changeSet: DescribeChangeSetOutput) {
const replacements = findResourceReplacements(changeSet);
diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => {
if (change.resourceType.includes('AWS::Serverless')) {
// CFN applies the SAM transform before creating the changeset, so the changeset contains no information about SAM resources
return;
}

return _replacements;
}

function _addChangeSetResourcesToDiff(_replacements: types.ResourceReplacements, _newTemplateResources: {[logicalId: string]: any}) {
const resourceDiffLogicalIds = diff.resources.logicalIds;
for (const logicalId of Object.keys(_replacements)) {
if (!(resourceDiffLogicalIds.includes(logicalId))) {
const noChangeResourceDiff = impl.diffResource(_newTemplateResources[logicalId], _newTemplateResources[logicalId]);
diff.resources.add(logicalId, noChangeResourceDiff);
}

for (const propertyName of Object.keys(_replacements[logicalId].propertiesReplaced)) {
if (propertyName in diff.resources.get(logicalId).propertyUpdates) {
// If the property is already marked to be updated, then we don't need to do anything.
continue;
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;
}

const newProp = new types.PropertyDifference(
// these fields will be decided below
{}, {}, { changeImpact: undefined },
);
newProp.isDifferent = true;
diff.resources.get(logicalId).setPropertyChange(propertyName, newProp);
}
};
}

function _enhanceChangeImpacts(_replacements: types.ResourceReplacements) {
diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => {
if (change.resourceType.includes('AWS::Serverless')) {
// CFN applies the SAM transform before creating the changeset, so the changeset contains no information about SAM resources
return;
}
change.forEachDifference((type: 'Property' | 'Other', name: string, value: types.Difference<any> | types.PropertyDifference<any>) => {
if (type === 'Property') {
if (!_replacements[logicalId]) {
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;
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;
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;
}
}
});
} else if (type === 'Other') {
switch (name) {
case 'Metadata':
change.setOtherChange('Metadata', new types.Difference<string>(value.newValue, value.newValue));
break;
}
}
});
}
});
}

function findResourceImports(changeSet: DescribeChangeSetOutput): string[] {
Expand All @@ -332,6 +281,33 @@ function findResourceImports(changeSet: DescribeChangeSetOutput): string[] {
return importedResourceLogicalIds;
}

function findResourceReplacements(changeSet: 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 ?? {}))) {
Expand Down
4 changes: 0 additions & 4 deletions packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -349,10 +349,6 @@ export class PropertyDifference<ValueType> extends Difference<ValueType> {
export class DifferenceCollection<V, T extends IDifference<V>> {
constructor(private readonly diffs: { [logicalId: string]: T }) {}

public add(logicalId: string, diff: T): void {
this.diffs[logicalId] = diff;
}

public get changes(): { [logicalId: string]: T } {
return onlyChanges(this.diffs);
}
Expand Down

0 comments on commit 811e224

Please sign in to comment.