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(diff): properties from ChangeSet diff were ignored #30268

Closed
wants to merge 42 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
b6d2cda
in progress
bergjaak May 18, 2024
73e97bb
in progress
bergjaak May 18, 2024
cd751b3
Merge branch 'main' of github.com:aws/aws-cdk into bergjaak/ResourceD…
bergjaak May 18, 2024
c15b448
in progress
bergjaak May 18, 2024
48c694c
I am going to consider what happens if I just always replace resource…
bergjaak May 18, 2024
62338a6
get resource details from changeset
bergjaak May 19, 2024
7a74592
get resource details from changeset
bergjaak May 19, 2024
0eba0cc
get resource details from changeset
bergjaak May 19, 2024
2e259b2
update unit tests
bergjaak May 19, 2024
464b699
separate file for changeset tests
bergjaak May 19, 2024
f8593d4
integ test added and updated
bergjaak May 19, 2024
05956d6
more tests
bergjaak May 19, 2024
52129b0
clean
bergjaak May 19, 2024
2683ed7
tests are passing
bergjaak May 19, 2024
56e20d8
fully tested
bergjaak May 19, 2024
54a920e
fully tested
bergjaak May 19, 2024
416f4b6
make _maybeParse better
bergjaak May 19, 2024
c15f9ef
make _maybeParse better
bergjaak May 19, 2024
bd9eccf
rename describe
bergjaak May 20, 2024
3b986cb
clean
bergjaak May 20, 2024
fff90cc
fix build
bergjaak May 20, 2024
82c7a57
add test for formatting
bergjaak May 20, 2024
207c416
fix describe changeset
bergjaak May 20, 2024
0b29d2b
Merge branch 'main' of github.com:aws/aws-cdk into bergjaak/ResourceD…
bergjaak May 20, 2024
f12b22b
better
bergjaak May 20, 2024
3e38f56
add private and public
bergjaak May 20, 2024
9fcaadd
use enum and better UNKNOWN
bergjaak May 20, 2024
94cbbda
include property values
bergjaak May 20, 2024
f23ce3d
now use beforeContext and afterContext to see property changes
bergjaak May 21, 2024
61bbcce
integ test for security changes
bergjaak May 21, 2024
a06fc7b
fixing unit tests
bergjaak May 22, 2024
c87aaa9
Merge branch 'main' of github.com:aws/aws-cdk into bergjaak/ResourceD…
bergjaak May 22, 2024
6fcfafd
stuff
bergjaak May 22, 2024
84ce20b
handle regions where describeChangeSet.IncludePropertyValues is not a…
bergjaak May 23, 2024
e65abf7
clean up comments
bergjaak May 23, 2024
f221971
removed context backups
bergjaak May 23, 2024
213df93
ready
bergjaak May 24, 2024
357ff5e
ready
bergjaak May 24, 2024
ed49a27
Merge branch 'main' into bergjaak/ResourceDiff2
bergjaak May 24, 2024
3694e71
ready
bergjaak May 24, 2024
e1ff643
Merge branch 'main' of github.com:aws/aws-cdk into bergjaak/ResourceD…
bergjaak May 24, 2024
8af1b4e
Merge branch 'bergjaak/ResourceDiff2' of github.com:aws/aws-cdk into …
bergjaak May 24, 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
2 changes: 2 additions & 0 deletions packages/@aws-cdk-testing/cli-integ/lib/aws.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ 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 @@ -38,6 +39,7 @@ export class AwsClients {
this.ecr = makeAwsCaller(AWS.ECR, this.config);
this.ecs = makeAwsCaller(AWS.ECS, this.config);
this.sso = makeAwsCaller(AWS.SSO, this.config);
this.ssm = makeAwsCaller(AWS.SSM, this.config);
this.sns = makeAwsCaller(AWS.SNS, this.config);
this.iam = makeAwsCaller(AWS.IAM, this.config);
this.lambda = makeAwsCaller(AWS.Lambda, this.config);
Expand Down
10 changes: 10 additions & 0 deletions packages/@aws-cdk-testing/cli-integ/lib/integ-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,3 +79,13 @@ export function randomString() {
// Crazy
return Math.random().toString(36).replace(/[^a-z0-9]+/g, '');
}

export function normalizeDiffOutput(s: string, removeFormatting: boolean = false): string {
if (removeFormatting) {
// remove all color and formatting (bolding, italic, etc)
s = s.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '');
}

return s.replace(/ /g, '') // remove all spaces
.replace(/\n/g, ''); // remove all new lines
}
42 changes: 42 additions & 0 deletions packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -530,6 +530,45 @@ class DockerStackWithCustomFile extends cdk.Stack {
});
}
}
class SecurityDiffFromChangeSetStack extends Stack {
constructor(scope, id) {
super(scope, id);

const iamResourceName = ssm.StringParameter.valueForStringParameter(this, 'for-iam-role-defined-by-ssm-param');

new iam.Role(this, 'changeSetDiffIamRole', {
assumedBy: new iam.ServicePrincipal('ec2.amazonaws.com'),
inlinePolicies: {
fun: new iam.PolicyDocument({
statements: [
new iam.PolicyStatement({
effect: iam.Effect.DENY,
actions: ['sqs:*'],
resources: [`arn:aws:sqs:us-east-1:444455556666:${iamResourceName}`],
}),
],
}),
},
});

}
}

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,
});
}
}

/**
* A stack that will never succeed deploying (done in a way that CDK cannot detect but CFN will complain about)
Expand Down Expand Up @@ -685,6 +724,9 @@ switch (stackSet) {

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

new DiffFromChangeSetStack(app, `${stackPrefix}-queue-name-defined-by-ssm-param`)
new SecurityDiffFromChangeSetStack(app, `${stackPrefix}-iam-role-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
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { promises as fs, existsSync } from 'fs';
import * as os from 'os';
import * as path from 'path';
import { integTest, cloneDirectory, shell, withDefaultFixture, retry, sleep, randomInteger, withSamIntegrationFixture, RESOURCES_DIR, withCDKMigrateFixture, withExtendedTimeoutFixture, randomString } from '../../lib';
import { integTest, cloneDirectory, shell, withDefaultFixture, retry, sleep, randomInteger, withSamIntegrationFixture, RESOURCES_DIR, withCDKMigrateFixture, withExtendedTimeoutFixture, randomString, normalizeDiffOutput } from '../../lib';

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

Expand Down Expand Up @@ -944,6 +944,107 @@ 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 security changes that are only in changeset', withDefaultFixture(async (fixture) => {
// GIVEN
const originalRoleName = randomString();
await fixture.aws.ssm('putParameter', {
Name: 'for-iam-role-defined-by-ssm-param',
Value: originalRoleName,
Type: 'String',
Overwrite: true,
});

try {
await fixture.cdkDeploy('iam-role-defined-by-ssm-param');

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

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

// THEN
const normalizedPlainTextOutput = normalizeDiffOutput(diff, true);

const title = normalizeDiffOutput('IAM Statement Changes');
const header = normalizeDiffOutput('│ │ Resource │ Effect │ Action │ Principal │ Condition │ ');
const remove = normalizeDiffOutput(`│ - │ arn:aws:sqs:us-east-1:444455556666:${originalRoleName} │ Deny │ sqs:* │ AWS:\${changeSetDiffIamRole} │ │ `);
const add = normalizeDiffOutput(` │ + │ arn:aws:sqs:us-east-1:444455556666:${newRoleName} │ Deny │ sqs:* │ AWS:\${changeSetDiffIamRole} │ │ `);
const expectedDiff = normalizeDiffOutput(`
Resources
[~] AWS::IAM::Role changeSetDiffIamRole changeSetDiffIamRole9C803BA2
└─ [~] Policies
└─ @@ -6,7 +6,7 @@
[ ] "Statement": [
[ ] {
[ ] "Action": "sqs:*",
[-] "Resource": "arn:aws:sqs:us-east-1:444455556666:${originalRoleName}",
[+] "Resource": "arn:aws:sqs:us-east-1:444455556666:${newRoleName}",
[ ] "Effect": "Deny"
[ ] }
[ ] ]`);

expect(normalizedPlainTextOutput).toContain(expectedDiff);
expect(normalizedPlainTextOutput).toContain(title);
expect(normalizedPlainTextOutput).toContain(header);
expect(normalizedPlainTextOutput).toContain(remove);
expect(normalizedPlainTextOutput).toContain(add);
} finally {
await fixture.cdkDestroy('iam-role-defined-by-ssm-param');
}
}));

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

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

// THEN
const normalizedPlainTextOutput = normalizeDiffOutput(diff, true);

const normalizedExpectedOutput = normalizeDiffOutput(`
Resources
[~] AWS::SQS::Queue DiffFromChangeSetQueue DiffFromChangeSetQueue06622C07 replace
└─ [~] QueueName (requires replacement)
├─ [-] ${originalQueueName}
└─ [+] ${newQueueName}
[~] AWS::SSM::Parameter DiffFromChangeSetSSMParam DiffFromChangeSetSSMParam92A9A723
└─ [~] Value
├─ [-] ${originalQueueName}
└─ [+] ${newQueueName}`);

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
109 changes: 11 additions & 98 deletions packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The SDK should not make network calls here
import type { DescribeChangeSetOutput as DescribeChangeSet } from '@aws-sdk/client-cloudformation';
import * as impl from './diff';
import { TemplateAndChangeSetDiffMerger } from './diff/template-and-changeset-diff-merger';
import * as types from './diff/types';
import { deepEqual, diffKeyedEntities, unionOf } from './diff/util';

Expand Down Expand Up @@ -53,10 +54,16 @@ export function fullDiff(

normalize(currentTemplate);
normalize(newTemplate);
const theDiff = diffTemplate(currentTemplate, newTemplate);
let theDiff = diffTemplate(currentTemplate, newTemplate);
if (changeSet) {
filterFalsePositives(theDiff, changeSet);
addImportInformation(theDiff, changeSet);
// These methods mutate the state of theDiff, using the changeSet.
const changeSetDiff = new TemplateAndChangeSetDiffMerger({ changeSet });
changeSetDiff.overrideDiffResourcesWithChangeSetResources(theDiff.resources, currentTemplate.Resources);
theDiff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) =>
changeSetDiff.overrideDiffResourceChangeImpactWithChangeSetChangeImpact(logicalId, change),
);
changeSetDiff.addImportInformationFromChangeset(theDiff.resources);
theDiff = new types.TemplateDiff(theDiff); // do this to propagate security changes.
} else if (isImport) {
makeAllResourceChangesImports(theDiff);
}
Expand Down Expand Up @@ -98,6 +105,7 @@ export function diffTemplate(
.filter(r => isReplacement(r!.changeImpact))
.forEachDifference((logicalId, downstreamReplacement) => {
const resource = theDiff.resources.get(logicalId);
if (!resource) { throw new Error(`No object with logical ID '${logicalId}'`); }

if (resource.changeImpact !== downstreamReplacement.changeImpact) {
propagatePropertyReplacement(downstreamReplacement, resource);
Expand Down Expand Up @@ -143,13 +151,6 @@ 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 @@ -214,100 +215,12 @@ function deepCopy(x: any): any {
return x;
}

function addImportInformation(diff: types.TemplateDiff, changeSet: DescribeChangeSetOutput) {
const imports = findResourceImports(changeSet);
diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => {
if (imports.includes(logicalId)) {
change.isImport = true;
}
});
}

function makeAllResourceChangesImports(diff: types.TemplateDiff) {
diff.resources.forEachDifference((_logicalId: string, change: types.ResourceDifference) => {
change.isImport = true;
});
}

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;
}
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 findResourceImports(changeSet: DescribeChangeSetOutput): string[] {
const importedResourceLogicalIds = [];
for (const resourceChange of changeSet.Changes ?? []) {
if (resourceChange.ResourceChange?.Action === 'Import') {
importedResourceLogicalIds.push(resourceChange.ResourceChange.LogicalResourceId!);
}
}

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;
}
Comment on lines -284 to -309
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was refactored into multiple functions in the new class


function normalize(template: any) {
if (typeof template === 'object') {
for (const key of (Object.keys(template ?? {}))) {
Expand Down
Loading
Loading