From 9c3f3f5dbb9b4b9f86911d9cd7c056a9fc0432b3 Mon Sep 17 00:00:00 2001 From: bergjaak <108292982+bergjaak@users.noreply.github.com> Date: Fri, 10 May 2024 12:04:07 -0400 Subject: [PATCH] fix(diff): properties from ChangeSet diff were ignored (#30093) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ### 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). 29731Fix * 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* --- .../@aws-cdk-testing/cli-integ/lib/aws.ts | 2 + .../@aws-cdk-testing/cli-integ/package.json | 6 +- .../cli-integ/resources/cdk-apps/app/app.js | 18 + .../tests/cli-integ-tests/cli.integtest.ts | 46 ++- .../cloudformation-diff/lib/diff-template.ts | 158 +++++---- .../cloudformation-diff/lib/diff/types.ts | 4 + .../test/diff-template.test.ts | 333 ++++++++++++++++++ .../@aws-cdk/cloudformation-diff/test/util.ts | 32 ++ 8 files changed, 528 insertions(+), 71 deletions(-) diff --git a/packages/@aws-cdk-testing/cli-integ/lib/aws.ts b/packages/@aws-cdk-testing/cli-integ/lib/aws.ts index bcf19f4a47e3f..cd06cad3b5e94 100644 --- a/packages/@aws-cdk-testing/cli-integ/lib/aws.ts +++ b/packages/@aws-cdk-testing/cli-integ/lib/aws.ts @@ -20,6 +20,7 @@ export class AwsClients { public readonly ecr: AwsCaller; public readonly ecs: AwsCaller; public readonly sso: AwsCaller; + public readonly ssm: AwsCaller; public readonly sns: AwsCaller; public readonly iam: AwsCaller; public readonly lambda: AwsCaller; @@ -39,6 +40,7 @@ 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); diff --git a/packages/@aws-cdk-testing/cli-integ/package.json b/packages/@aws-cdk-testing/cli-integ/package.json index 44ae905358779..28a49bf5f0c0c 100644 --- a/packages/@aws-cdk-testing/cli-integ/package.json +++ b/packages/@aws-cdk-testing/cli-integ/package.json @@ -30,12 +30,12 @@ "license": "Apache-2.0", "devDependencies": { "@aws-cdk/cdk-build-tools": "0.0.0", - "@types/semver": "^7.5.8", - "@types/yargs": "^15.0.19", + "@aws-cdk/pkglint": "0.0.0", "@types/fs-extra": "^9.0.13", "@types/glob": "^7.2.0", "@types/npm": "^7.19.3", - "@aws-cdk/pkglint": "0.0.0" + "@types/semver": "^7.5.8", + "@types/yargs": "^15.0.19" }, "dependencies": { "@octokit/rest": "^18.12.0", diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js index 5683951c2a832..8dbd2b4fd40b3 100755 --- a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js +++ b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/app/app.js @@ -126,6 +126,22 @@ 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); @@ -658,6 +674,8 @@ 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); diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts index 749788183f26e..9fea9abc0d0b8 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts @@ -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 } from '../../lib'; +import { integTest, cloneDirectory, shell, withDefaultFixture, retry, sleep, randomInteger, withSamIntegrationFixture, RESOURCES_DIR, withCDKMigrateFixture, withExtendedTimeoutFixture, randomString } from '../../lib'; jest.setTimeout(2 * 60 * 60_000); // Includes the time to acquire locks, worst-case single-threaded runtime @@ -944,6 +944,50 @@ 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'); })); diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts index 118df03c6ebc5..025a707b62386 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts @@ -55,7 +55,7 @@ export function fullDiff( normalize(newTemplate); const theDiff = diffTemplate(currentTemplate, newTemplate); if (changeSet) { - filterFalsePositives(theDiff, changeSet); + refineDiffWithChangeSet(theDiff, changeSet, newTemplate.Resources); addImportInformation(theDiff, changeSet); } else if (isImport) { makeAllResourceChangesImports(theDiff); @@ -143,13 +143,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 * @@ -229,45 +222,103 @@ function makeAllResourceChangesImports(diff: types.TemplateDiff) { }); } -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; +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, + }; } - change.forEachDifference((type: 'Property' | 'Other', name: string, value: types.Difference | types.PropertyDifference) => { - if (type === 'Property') { - if (!replacements[logicalId]) { - (value as types.PropertyDifference).changeImpact = types.ResourceImpact.NO_CHANGE; - (value as types.PropertyDifference).isDifferent = false; - 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; } - switch (replacements[logicalId].propertiesReplaced[name]) { - case 'Always': - (value as types.PropertyDifference).changeImpact = types.ResourceImpact.WILL_REPLACE; - break; - case 'Never': - (value as types.PropertyDifference).changeImpact = types.ResourceImpact.WILL_UPDATE; - break; - case 'Conditionally': - (value as types.PropertyDifference).changeImpact = types.ResourceImpact.MAY_REPLACE; - break; - case undefined: + + 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 | types.PropertyDifference) => { + if (type === 'Property') { + if (!_replacements[logicalId]) { (value as types.PropertyDifference).changeImpact = types.ResourceImpact.NO_CHANGE; (value as types.PropertyDifference).isDifferent = false; - break; + return; + } + switch (_replacements[logicalId].propertiesReplaced[name]) { + case 'Always': + (value as types.PropertyDifference).changeImpact = types.ResourceImpact.WILL_REPLACE; + break; + case 'Never': + (value as types.PropertyDifference).changeImpact = types.ResourceImpact.WILL_UPDATE; + break; + case 'Conditionally': + (value as types.PropertyDifference).changeImpact = types.ResourceImpact.MAY_REPLACE; + break; + case undefined: + (value as types.PropertyDifference).changeImpact = types.ResourceImpact.NO_CHANGE; + (value as types.PropertyDifference).isDifferent = false; + break; // otherwise, defer to the changeImpact from `diffTemplate` + } + } else if (type === 'Other') { + switch (name) { + case 'Metadata': + change.setOtherChange('Metadata', new types.Difference(value.newValue, value.newValue)); + break; + } } - } else if (type === 'Other') { - switch (name) { - case 'Metadata': - change.setOtherChange('Metadata', new types.Difference(value.newValue, value.newValue)); - break; - } - } + }); }); - }); + } } function findResourceImports(changeSet: DescribeChangeSetOutput): string[] { @@ -281,33 +332,6 @@ 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 ?? {}))) { diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts index 4b65ef484e919..9d66476c62852 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff/types.ts @@ -349,6 +349,10 @@ export class PropertyDifference extends Difference { export class DifferenceCollection> { 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); } diff --git a/packages/@aws-cdk/cloudformation-diff/test/diff-template.test.ts b/packages/@aws-cdk/cloudformation-diff/test/diff-template.test.ts index 318f27b6bbf30..5490d9051c22a 100644 --- a/packages/@aws-cdk/cloudformation-diff/test/diff-template.test.ts +++ b/packages/@aws-cdk/cloudformation-diff/test/diff-template.test.ts @@ -1,5 +1,6 @@ import * as fc from 'fast-check'; import { arbitraryTemplate } from './test-arbitraries'; +import { sqsQueue, sqsQueueWithAargs, ssmParam } from './util'; import { fullDiff, ResourceImpact } from '../lib/diff-template'; const POLICY_DOCUMENT = { foo: 'Bar' }; // Obviously a fake one! @@ -1231,4 +1232,336 @@ describe('changeset', () => { expect(differences.resources.differenceCount).toBe(1); expect(differences.resources.get('BucketResource').changeImpact === ResourceImpact.WILL_IMPORT); }); + + test('properties that only show up in changeset diff are included in fullDiff', () => { + // GIVEN + const currentTemplate = { + Parameters: { + SsmParameterValuetestbugreportC9: { + Type: 'AWS::SSM::Parameter::Value', + Default: 'goodJob', + }, + }, + Resources: { + mySsmParameter: ssmParam, + }, + }; + + // WHEN + const diffWithoutChangeSet = fullDiff(currentTemplate, currentTemplate); + const diffWithChangeSet = fullDiff(currentTemplate, currentTemplate, + { + Changes: [ + { + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: 'mySsmParameter', + PhysicalResourceId: 'mySsmParameterFromStack', + ResourceType: 'AWS::SSM::Parameter', + Replacement: 'False', + Scope: ['Properties'], + Details: [{ + Target: { Attribute: 'Properties', Name: 'Value', RequiresRecreation: 'Never' }, + Evaluation: 'Static', + ChangeSource: 'DirectModification', + }], + }, + }, + ], + Parameters: [{ + ParameterKey: 'SsmParameterValuetestbugreportC9', + ParameterValue: 'goodJob', + ResolvedValue: 'changedVal', + }], + }, + ); + + // THEN + expect(diffWithoutChangeSet.differenceCount).toBe(0); + expect(diffWithoutChangeSet.resources.changes).toEqual({}); + + expect(diffWithChangeSet.differenceCount).toBe(1); + expect(diffWithChangeSet.resources.changes).toEqual( + { + mySsmParameter: { + oldValue: ssmParam, + newValue: ssmParam, + resourceTypes: { + oldType: 'AWS::SSM::Parameter', + newType: 'AWS::SSM::Parameter', + }, + propertyDiffs: { + Name: { + oldValue: 'mySsmParameterFromStack', + newValue: 'mySsmParameterFromStack', + isDifferent: false, + changeImpact: 'NO_CHANGE', + }, + Type: { + oldValue: 'String', + newValue: 'String', + isDifferent: false, + changeImpact: 'NO_CHANGE', + }, + Value: { + oldValue: {}, + newValue: {}, + isDifferent: true, + changeImpact: 'WILL_UPDATE', // this is what changed! + }, + }, + otherDiffs: { + Type: { + oldValue: 'AWS::SSM::Parameter', + newValue: 'AWS::SSM::Parameter', + isDifferent: false, + }, + }, + isAddition: false, + isRemoval: false, + isImport: undefined, + }, + }, + ); + }); + + test('resources that only show up in changeset diff are included in fullDiff', () => { + // GIVEN + const currentTemplate = { + Parameters: { + SsmParameterValuetestbugreportC9: { + Type: 'AWS::SSM::Parameter::Value', + Default: 'goodJob', + }, + }, + Resources: { + Queue: { + Type: 'AWS::SQS::Queue', + Properties: { + QueueName: { + Ref: 'SsmParameterValuetestbugreportC9', + }, + }, + }, + }, + }; + + // WHEN + const diffWithoutChangeSet = fullDiff(currentTemplate, currentTemplate); + const diffWithChangeSet = fullDiff(currentTemplate, currentTemplate, + { + Changes: [ + { + Type: 'Resource', + ResourceChange: { + PolicyAction: 'ReplaceAndDelete', + Action: 'Modify', + LogicalResourceId: 'Queue', + PhysicalResourceId: 'https://sqs.us-east-1.amazonaws.com/012345678901/hiii', + ResourceType: 'AWS::SQS::Queue', + Replacement: 'True', + Scope: ['Properties'], + Details: [{ + Target: { Attribute: 'Properties', Name: 'QueueName', RequiresRecreation: 'Always' }, + Evaluation: 'Static', + ChangeSource: 'DirectModification', + }], + }, + }, + ], + Parameters: [{ + ParameterKey: 'SsmParameterValuetestbugreportC9', + ParameterValue: 'goodJob', + ResolvedValue: 'changedVal', + }], + }, + ); + + // THEN + expect(diffWithoutChangeSet.differenceCount).toBe(0); + expect(diffWithoutChangeSet.resources.changes).toEqual({}); + + expect(diffWithChangeSet.differenceCount).toBe(1); + expect(diffWithChangeSet.resources.changes).toEqual( + { + Queue: { + oldValue: sqsQueue, + newValue: sqsQueue, + resourceTypes: { + oldType: 'AWS::SQS::Queue', + newType: 'AWS::SQS::Queue', + }, + propertyDiffs: { + QueueName: { + oldValue: {}, + newValue: {}, + isDifferent: true, + changeImpact: 'WILL_REPLACE', // this is what changed! + }, + }, + otherDiffs: { + Type: { + oldValue: 'AWS::SQS::Queue', + newValue: 'AWS::SQS::Queue', + isDifferent: false, + }, + }, + isAddition: false, + isRemoval: false, + isImport: undefined, + }, + }, + ); + }); + + test('a resource in the diff that is missing a property has the missing property added to the diff', () => { + // The idea is, we detect 1 change in the template diff -- and we detect another change in the changeset diff. + + // GIVEN + const currentTemplate = { + Parameters: { + SsmParameterValuetestbugreportC9: { + Type: 'AWS::SSM::Parameter::Value', + Default: 'goodJob', + }, + }, + Resources: { + Queue: sqsQueueWithAargs({ waitTime: 10 }), + }, + }; + + const newTemplate = { + Parameters: { + SsmParameterValuetestbugreportC9: { + Type: 'AWS::SSM::Parameter::Value', + Default: 'goodJob', + }, + }, + Resources: { + Queue: sqsQueueWithAargs({ waitTime: 20 }), + }, + }; + + // WHEN + const diffWithoutChangeSet = fullDiff(currentTemplate, newTemplate); + const diffWithChangeSet = fullDiff(currentTemplate, newTemplate, + { + Changes: [ + { + Type: 'Resource', + ResourceChange: { + PolicyAction: 'ReplaceAndDelete', + Action: 'Modify', + LogicalResourceId: 'Queue', + PhysicalResourceId: 'https://sqs.us-east-1.amazonaws.com/012345678901/newValueNEEEWWWEEERRRRR', + ResourceType: 'AWS::SQS::Queue', + Replacement: 'True', + Scope: [ + 'Properties', + ], + Details: [{ + Target: { Attribute: 'Properties', Name: 'QueueName', RequiresRecreation: 'Always' }, + Evaluation: 'Static', + ChangeSource: 'DirectModification', + }, + { + Target: { Attribute: 'Properties', Name: 'ReceiveMessageWaitTimeSeconds', RequiresRecreation: 'Never' }, + Evaluation: 'Static', + ChangeSource: 'DirectModification', + }], + }, + }, + ], + Parameters: [{ + ParameterKey: 'SsmParameterValuetestbugreportC9', + ParameterValue: 'goodJob', + ResolvedValue: 'changedddd', + }], + }, + ); + + // THEN + expect(diffWithoutChangeSet.differenceCount).toBe(1); + expect(diffWithoutChangeSet.resources.changes).toEqual( + { + Queue: { + oldValue: sqsQueueWithAargs({ waitTime: 10 }), + newValue: sqsQueueWithAargs({ waitTime: 20 }), + resourceTypes: { + oldType: 'AWS::SQS::Queue', + newType: 'AWS::SQS::Queue', + }, + propertyDiffs: { + QueueName: { + oldValue: { + Ref: 'SsmParameterValuetestbugreportC9', + }, + newValue: { + Ref: 'SsmParameterValuetestbugreportC9', + }, + isDifferent: false, + changeImpact: 'NO_CHANGE', + }, + ReceiveMessageWaitTimeSeconds: { + oldValue: 10, + newValue: 20, + isDifferent: true, + changeImpact: 'WILL_UPDATE', + }, + }, + otherDiffs: { + Type: { + oldValue: 'AWS::SQS::Queue', + newValue: 'AWS::SQS::Queue', + isDifferent: false, + }, + }, + isAddition: false, + isRemoval: false, + isImport: undefined, + }, + }, + ); + + expect(diffWithChangeSet.differenceCount).toBe(1); // this is the count of how many resources have changed + expect(diffWithChangeSet.resources.changes).toEqual( + { + Queue: { + oldValue: sqsQueueWithAargs({ waitTime: 10 }), + newValue: sqsQueueWithAargs({ waitTime: 20 }), + resourceTypes: { + oldType: 'AWS::SQS::Queue', + newType: 'AWS::SQS::Queue', + }, + propertyDiffs: { + QueueName: { + oldValue: { + }, + newValue: { + }, + isDifferent: true, + changeImpact: 'WILL_REPLACE', + }, + ReceiveMessageWaitTimeSeconds: { + oldValue: 10, + newValue: 20, + isDifferent: true, + changeImpact: 'WILL_UPDATE', + }, + }, + otherDiffs: { + Type: { + oldValue: 'AWS::SQS::Queue', + newValue: 'AWS::SQS::Queue', + isDifferent: false, + }, + }, + isAddition: false, + isRemoval: false, + isImport: undefined, + }, + }, + ); + }); }); diff --git a/packages/@aws-cdk/cloudformation-diff/test/util.ts b/packages/@aws-cdk/cloudformation-diff/test/util.ts index bce0b48b214eb..9c1bb4d4dfaed 100644 --- a/packages/@aws-cdk/cloudformation-diff/test/util.ts +++ b/packages/@aws-cdk/cloudformation-diff/test/util.ts @@ -73,3 +73,35 @@ export function largeSsoPermissionSet() { ), }); } + +export const ssmParam = { + Type: 'AWS::SSM::Parameter', + Properties: { + Name: 'mySsmParameterFromStack', + Type: 'String', + Value: { + Ref: 'SsmParameterValuetestbugreportC9', + }, + }, +}; + +export const sqsQueue = { + Type: 'AWS::SQS::Queue', + Properties: { + QueueName: { + Ref: 'SsmParameterValuetestbugreportC9', + }, + }, +}; + +export function sqsQueueWithAargs(args: { waitTime: number }) { + return { + Type: 'AWS::SQS::Queue', + Properties: { + QueueName: { + Ref: 'SsmParameterValuetestbugreportC9', + }, + ReceiveMessageWaitTimeSeconds: args.waitTime, + }, + }; +}