diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/json-intrinsic-app/app.js b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/json-intrinsic-app/app.js index 26df22527..78483b774 100644 --- a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/json-intrinsic-app/app.js +++ b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/json-intrinsic-app/app.js @@ -1,4 +1,5 @@ const cdk = require('aws-cdk-lib/core'); +const ecs = require('aws-cdk-lib/aws-ecs'); const ssm = require('aws-cdk-lib/aws-ssm'); const stackPrefix = process.env.STACK_NAME_PREFIX; @@ -6,42 +7,99 @@ if (!stackPrefix) { throw new Error(`the STACK_NAME_PREFIX environment variable is required`); } -// A string parameter holding JSON that is built with an intrinsic, the way Stack.toJsonString renders -// it (the EKS KubernetesManifest and HelmChart constructs use it too). VARIANT changes the JSON in a -// way that CloudFormation leaves out of a change set described with IncludePropertyValues, without a -// warning (on its own, or together with the description), changes a value (which CloudFormation does -// list), or rewrites it so that it resolves to the same value. See aws/aws-cdk-cli#2036. +/** + * Holds JSON built with an intrinsic (an Fn::Join, from Stack.toJsonString) in three places: + * + * - `-json-intrinsic`: an SSM string parameter + * - `-json-intrinsic-nested`: an SSM string parameter in a nested stack + * - `-json-intrinsic-replace`: a container environment variable of an ECS task definition. + * Changing a parameter's value updates it in place, but any change to a task definition replaces it. + * This stack makes sure if a change is dropped, it won't affect the functionality of + * `--require-approval=destructive` and `--no-rollback`, which depend on replacement. + * + * Each test deploys without JSON_CHANGE (unset), then sets JSON_CHANGE to switch on these changes: + * + * ``` + * (unset) {"stack":"","a":"1"} + * change-value {"stack":"","a":"2"} + * add-key {"stack":"","a":"1","b":"2"} + * add-key-with-token {"stack":"","a":"1","region":""} + * reorder-keys {"a":"1","stack":""} + * same-value-with-fn-sub {"stack":"","a":"1"}, built with Fn::Sub instead of Fn::Join + * ``` + * + * Described with IncludePropertyValues, CloudFormation leaves `add-key`, `add-key-with-token` and + * `reorder-keys` out of the change set, without a warning, or with one when the change is the task + * definition replacement (https://github.com/aws/aws-cdk-cli/issues/2036). + * The tests that rely on this only exercise the recovery while CloudFormation keeps leaving these + * changes out; the toolkit-lib and cloudformation-diff unit tests are the lasting coverage. + * + * DESCRIPTION sets the description of the SSM parameter in `-json-intrinsic` (default "d1"). + * This allows a test to change a separate property of the Parameter, and ensure that change is kept. + * + * To exercise this app: + * + * ``` + * npx cdk deploy -json-intrinsic + * env JSON_CHANGE=add-key npx cdk diff --method=change-set -json-intrinsic + * env JSON_CHANGE=add-key DESCRIPTION=d2 npx cdk deploy --require-approval=any-change -json-intrinsic + * ``` + */ class JsonIntrinsicStack extends cdk.Stack { constructor(scope, id, props) { super(scope, id, props); new ssm.StringParameter(this, 'Param', { - stringValue: this.parameterValue(process.env.VARIANT), - description: process.env.VARIANT === 'add-key-and-description' ? 'd2' : 'd1', + stringValue: jsonValue(this, process.env.JSON_CHANGE), + description: process.env.DESCRIPTION ?? 'd1', }); } +} + +class JsonIntrinsicReplaceStack extends cdk.Stack { + constructor(scope, id, props) { + super(scope, id, props); + const taskDefinition = new ecs.TaskDefinition(this, 'Task', { + compatibility: ecs.Compatibility.FARGATE, + cpu: '256', + memoryMiB: '512', + }); + taskDefinition.addContainer('Container', { + image: ecs.ContainerImage.fromRegistry('public.ecr.aws/docker/library/busybox:latest'), + environment: { CONFIG: jsonValue(this, process.env.JSON_CHANGE) }, + }); + } +} + +class JsonIntrinsicNestedStack extends cdk.Stack { + constructor(scope, id, props) { + super(scope, id, props); + const nested = new cdk.NestedStack(this, 'Nested'); + new ssm.StringParameter(nested, 'Param', { stringValue: jsonValue(nested, process.env.JSON_CHANGE) }); + } +} - parameterValue(variant) { - switch (variant) { - case undefined: - return this.toJsonString({ stack: cdk.Aws.STACK_NAME, a: '1' }); - case 'change-value': - return this.toJsonString({ stack: cdk.Aws.STACK_NAME, a: '2' }); - case 'add-key': - case 'add-key-and-description': - return this.toJsonString({ stack: cdk.Aws.STACK_NAME, a: '1', b: '2' }); - case 'add-key-with-token': - return this.toJsonString({ stack: cdk.Aws.STACK_NAME, a: '1', region: cdk.Aws.REGION }); - case 'reorder-keys': - return this.toJsonString({ a: '1', stack: cdk.Aws.STACK_NAME }); - case 'same-value-with-fn-sub': - return cdk.Fn.sub('{"stack":"${AWS::StackName}","a":"1"}'); - default: - throw new Error(`unknown VARIANT: ${variant}`); - } +function jsonValue(stack, change) { + switch (change) { + case undefined: + return stack.toJsonString({ stack: cdk.Aws.STACK_NAME, a: '1' }); + case 'change-value': + return stack.toJsonString({ stack: cdk.Aws.STACK_NAME, a: '2' }); + case 'add-key': + return stack.toJsonString({ stack: cdk.Aws.STACK_NAME, a: '1', b: '2' }); + case 'add-key-with-token': + return stack.toJsonString({ stack: cdk.Aws.STACK_NAME, a: '1', region: cdk.Aws.REGION }); + case 'reorder-keys': + return stack.toJsonString({ a: '1', stack: cdk.Aws.STACK_NAME }); + case 'same-value-with-fn-sub': + return cdk.Fn.sub('{"stack":"${AWS::StackName}","a":"1"}'); + default: + throw new Error(`unknown JSON_CHANGE: ${change}`); } } const app = new cdk.App(); new JsonIntrinsicStack(app, `${stackPrefix}-json-intrinsic`); +new JsonIntrinsicReplaceStack(app, `${stackPrefix}-json-intrinsic-replace`); +new JsonIntrinsicNestedStack(app, `${stackPrefix}-json-intrinsic-nested`); app.synth(); diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy---no-execute-shows-a-key-added-to-json-built-with-an-intrinsic-then-executes-it.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy---no-execute-shows-a-key-added-to-json-built-with-an-intrinsic-then-executes-it.integtest.ts new file mode 100644 index 000000000..a332f7834 --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy---no-execute-shows-a-key-added-to-json-built-with-an-intrinsic-then-executes-it.integtest.ts @@ -0,0 +1,33 @@ +import { integTest, withSpecificFixture } from '../../../lib'; + +integTest( + 'cdk deploy --no-execute shows a key added to JSON built with an intrinsic, then executes it', + withSpecificFixture('json-intrinsic-app', async (fixture) => { + const stackName = fixture.fullStackName('json-intrinsic'); + const changeSetName = 'json-intrinsic-add-key'; + + // GIVEN - a deployed parameter holding {"stack":"","a":"1"} + await fixture.cdkDeploy('json-intrinsic'); + + // WHEN - a key is added, and a change set is created without executing it (approval auto-confirmed) + const prepare = await fixture.cdkDeploy('json-intrinsic', { + options: ['--no-execute', `--change-set-name=${changeSetName}`, '--require-approval=any-change', '--yes'], + neverRequireApproval: false, + modEnv: { JSON_CHANGE: 'add-key' }, + }); + + // THEN - the approval diff shows the parameter value as changed + expect(prepare).toMatch(/\[~\].*AWS::SSM::Parameter(?!::)/); + expect(prepare).toMatch(/\[~\].*Value/); + + // WHEN - the change set is executed + await fixture.cdkDeploy('json-intrinsic', { + options: ['--method=execute-change-set', `--change-set-name=${changeSetName}`], + modEnv: { JSON_CHANGE: 'add-key' }, + }); + + // THEN - the deployed template has the key: a template diff finds nothing left to change + const diff = await fixture.cdk(['diff', '--method=template', stackName], { modEnv: { JSON_CHANGE: 'add-key' } }); + expect(diff).toContain('There were no differences'); + }), +); diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy---no-rollback-stops-for-a-replacement-cloudformation-drops-with-a-warning.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy---no-rollback-stops-for-a-replacement-cloudformation-drops-with-a-warning.integtest.ts new file mode 100644 index 000000000..5cc153b3b --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy---no-rollback-stops-for-a-replacement-cloudformation-drops-with-a-warning.integtest.ts @@ -0,0 +1,25 @@ +import { integTest, withSpecificFixture } from '../../../lib'; + +/** + * CDK checks for replacements on the change set it describes just before executing, not the one the + * approval uses. CloudFormation leaves the task definition replacement out of it, with a warning + * (https://github.com/aws/aws-cdk-cli/issues/2036). + */ +integTest( + 'cdk deploy --no-rollback stops for a replacement CloudFormation drops with a warning', + withSpecificFixture('json-intrinsic-app', async (fixture) => { + // GIVEN - a deployed task definition whose environment holds {"stack":"","a":"1"} + await fixture.cdkDeploy('json-intrinsic-replace'); + + // WHEN - a key is added to the JSON and the deploy runs with rollback disabled. Without a terminal, + // CDK cannot ask to deploy with rollback enabled instead, so it fails and nothing is replaced. + const output = await fixture.cdkDeploy('json-intrinsic-replace', { + options: ['--no-rollback'], + modEnv: { JSON_CHANGE: 'add-key' }, + allowErrExit: true, + }); + + // THEN - CDK found the replacement and refused to deploy it with rollback disabled + expect(output).toContain('Change includes a replacement which cannot be deployed while rollback is disabled'); + }), +); diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy---require-approval-any-change-shows-a-key-added-to-json-built-with-an-intrinsic.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy---require-approval-any-change-shows-a-key-added-to-json-built-with-an-intrinsic.integtest.ts new file mode 100644 index 000000000..15d91c461 --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy---require-approval-any-change-shows-a-key-added-to-json-built-with-an-intrinsic.integtest.ts @@ -0,0 +1,21 @@ +import { integTest, withSpecificFixture } from '../../../lib'; + +integTest( + 'cdk deploy --require-approval any-change shows a key added to JSON built with an intrinsic', + withSpecificFixture('json-intrinsic-app', async (fixture) => { + // GIVEN - a deployed parameter holding {"stack":"","a":"1"} + await fixture.cdkDeploy('json-intrinsic'); + + // WHEN - a key is added to the JSON, and the deploy asks for approval (auto-confirmed) + const output = await fixture.cdkDeploy('json-intrinsic', { + options: ['--require-approval=any-change', '--yes'], + neverRequireApproval: false, + modEnv: { JSON_CHANGE: 'add-key' }, + }); + + // THEN - the approval diff shows the parameter value as changed + expect(output).toContain('Do you wish to deploy these changes'); + expect(output).toMatch(/\[~\].*AWS::SSM::Parameter(?!::)/); + expect(output).toMatch(/\[~\].*Value/); + }), +); diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy---require-approval-destructive-prompts-for-a-replacement-cloudformation-drops.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy---require-approval-destructive-prompts-for-a-replacement-cloudformation-drops.integtest.ts new file mode 100644 index 000000000..613ca0d3c --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy---require-approval-destructive-prompts-for-a-replacement-cloudformation-drops.integtest.ts @@ -0,0 +1,25 @@ +import { integTest, withSpecificFixture } from '../../../lib'; + +/** + * CloudFormation leaves the task definition replacement out of the change set described with + * IncludePropertyValues, but warns that it did (https://github.com/aws/aws-cdk-cli/issues/2036), so + * CDK describes it again and still finds the replacement. + */ +integTest( + 'cdk deploy --require-approval destructive prompts for a replacement CloudFormation drops', + withSpecificFixture('json-intrinsic-app', async (fixture) => { + // GIVEN - a deployed task definition whose environment holds {"stack":"","a":"1"} + await fixture.cdkDeploy('json-intrinsic-replace'); + + // WHEN - a key is added to the JSON, and the deploy asks for approval of destructive changes (auto-confirmed) + const output = await fixture.cdkDeploy('json-intrinsic-replace', { + options: ['--require-approval=destructive', '--yes'], + neverRequireApproval: false, + modEnv: { JSON_CHANGE: 'add-key' }, + }); + + // THEN - the replacement is listed as destructive + expect(output).toContain('Do you wish to deploy these changes'); + expect(output).toMatch(/AWS::ECS::TaskDefinition.*will be replaced/); + }), +); diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-does-not-show-json-rewritten-to-the-same-value.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-does-not-show-json-rewritten-to-the-same-value.integtest.ts index c067e60f9..a9a4e934d 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-does-not-show-json-rewritten-to-the-same-value.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-does-not-show-json-rewritten-to-the-same-value.integtest.ts @@ -2,10 +2,8 @@ import { integTest, withSpecificFixture } from '../../../lib'; import '../../../lib/assertions'; /** - * Regression test for https://github.com/aws/aws-cdk-cli/issues/2036 - * - * The JSON is built with Fn::Sub instead of Fn::Join and resolves to the same value. The template - * text differs, so the changeset-based diff must keep trusting CloudFormation that nothing changes. + * The template text changes but the deployed value does not, so CloudFormation leaves it out of the + * change set. */ integTest( 'cdk diff --method=change-set does not show JSON rewritten to the same value', @@ -17,7 +15,7 @@ integTest( // WHEN - the same JSON is built with Fn::Sub const diff = await fixture.cdk(['diff', '--method=change-set', stackName], { - modEnv: { VARIANT: 'same-value-with-fn-sub' }, + modEnv: { JSON_CHANGE: 'same-value-with-fn-sub' }, }); // THEN - nothing is shown diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-a-changed-value-in-json-built-with-an-intrinsic.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-a-changed-value-in-json-built-with-an-intrinsic.integtest.ts index b7c11a86c..2ad2e9d10 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-a-changed-value-in-json-built-with-an-intrinsic.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-a-changed-value-in-json-built-with-an-intrinsic.integtest.ts @@ -1,13 +1,6 @@ import { integTest, withSpecificFixture } from '../../../lib'; import '../../../lib/assertions'; -/** - * Companion to the regression tests for https://github.com/aws/aws-cdk-cli/issues/2036 - * - * Changing an existing value in JSON that is built with an intrinsic is a change CloudFormation lists - * even in a change set described with IncludePropertyValues. The changeset-based diff shows it with - * or without the recovery for changes CloudFormation leaves out. - */ integTest( 'cdk diff --method=change-set shows a changed value in JSON built with an intrinsic', withSpecificFixture('json-intrinsic-app', async (fixture) => { @@ -18,7 +11,7 @@ integTest( // WHEN - an existing value in the JSON changes const diff = await fixture.cdk(['diff', '--method=change-set', stackName], { - modEnv: { VARIANT: 'change-value' }, + modEnv: { JSON_CHANGE: 'change-value' }, }); // THEN - the parameter value is shown as changed diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-a-key-added-to-json-built-with-an-intrinsic-when-another-property-changes.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-a-key-added-to-json-built-with-an-intrinsic-when-another-property-changes.integtest.ts index 1b0059fdd..0971203c3 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-a-key-added-to-json-built-with-an-intrinsic-when-another-property-changes.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-a-key-added-to-json-built-with-an-intrinsic-when-another-property-changes.integtest.ts @@ -1,16 +1,6 @@ import { integTest, withSpecificFixture } from '../../../lib'; import '../../../lib/assertions'; -/** - * Regression test for https://github.com/aws/aws-cdk-cli/issues/2036 - * - * When another property of the parameter changes too, CloudFormation keeps the parameter in a change - * set described with IncludePropertyValues, but leaves out the value whose JSON gained a key, without - * a warning. The changeset-based diff must show both changes. - * - * This only exercises the recovery for as long as CloudFormation keeps leaving the change out. If it - * stops, the test still passes; the toolkit-lib unit tests are the lasting coverage. - */ integTest( 'cdk diff --method=change-set shows a key added to JSON built with an intrinsic when another property changes', withSpecificFixture('json-intrinsic-app', async (fixture) => { @@ -21,7 +11,7 @@ integTest( // WHEN - a key is added to the JSON, and the description changes const diff = await fixture.cdk(['diff', '--method=change-set', stackName], { - modEnv: { VARIANT: 'add-key-and-description' }, + modEnv: { JSON_CHANGE: 'add-key', DESCRIPTION: 'd2' }, }); // THEN - both property changes are shown diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-a-key-added-to-json-in-a-nested-stack.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-a-key-added-to-json-in-a-nested-stack.integtest.ts new file mode 100644 index 000000000..1fc76453b --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-a-key-added-to-json-in-a-nested-stack.integtest.ts @@ -0,0 +1,26 @@ +import { integTest, withSpecificFixture } from '../../../lib'; +import '../../../lib/assertions'; + +/** + * The CLI diffs nested stacks from their templates, not their change sets, so it shows this change + * even though CloudFormation leaves it out of the nested change set + * (https://github.com/aws/aws-cdk-cli/issues/2036). It must keep showing it if the CLI starts reading + * nested change sets. + */ +integTest( + 'cdk diff --method=change-set shows a key added to JSON in a nested stack', + withSpecificFixture('json-intrinsic-app', async (fixture) => { + const stackName = fixture.fullStackName('json-intrinsic-nested'); + + // GIVEN - a deployed nested parameter holding {"stack":"","a":"1"} + await fixture.cdkDeploy('json-intrinsic-nested'); + + // WHEN - a key is added to the JSON + const diff = await fixture.cdk(['diff', '--method=change-set', stackName], { + modEnv: { JSON_CHANGE: 'add-key' }, + }); + + // THEN - the nested parameter value is shown as changed + expect(diff).toHaveNextLineMatching(/\[~\].*AWS::SSM::Parameter(?!::)/, /\[~\].*Value/); + }), +); diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-a-key-with-a-token-value-added-to-json-built-with-an-intrinsic.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-a-key-with-a-token-value-added-to-json-built-with-an-intrinsic.integtest.ts index bf77eb0f4..8434bb770 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-a-key-with-a-token-value-added-to-json-built-with-an-intrinsic.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-a-key-with-a-token-value-added-to-json-built-with-an-intrinsic.integtest.ts @@ -1,12 +1,6 @@ import { integTest, withSpecificFixture } from '../../../lib'; import '../../../lib/assertions'; -/** - * Regression test for https://github.com/aws/aws-cdk-cli/issues/2036 - * - * Like adding a key with a fixed value, but the new value is itself resolved at deploy time - * (`Aws.REGION`), so the deploy-time parts of the value change too. - */ integTest( 'cdk diff --method=change-set shows a key with a token value added to JSON built with an intrinsic', withSpecificFixture('json-intrinsic-app', async (fixture) => { @@ -17,7 +11,7 @@ integTest( // WHEN - a key whose value is the region is added to the JSON const diff = await fixture.cdk(['diff', '--method=change-set', stackName], { - modEnv: { VARIANT: 'add-key-with-token' }, + modEnv: { JSON_CHANGE: 'add-key-with-token' }, }); // THEN - the parameter value is shown as changed diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-changes-to-json-built-with-an-intrinsic.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-changes-to-json-built-with-an-intrinsic.integtest.ts index 6d2bf226e..5203d6948 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-changes-to-json-built-with-an-intrinsic.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-changes-to-json-built-with-an-intrinsic.integtest.ts @@ -1,16 +1,6 @@ import { integTest, withSpecificFixture } from '../../../lib'; import '../../../lib/assertions'; -/** - * Regression test for https://github.com/aws/aws-cdk-cli/issues/2036 - * - * Adding a key to JSON that is built with an intrinsic and stored in a string property is a change - * CloudFormation leaves out of a change set described with IncludePropertyValues, without a warning. - * The changeset-based diff must show it anyway. - * - * This only exercises the recovery for as long as CloudFormation keeps leaving the change out. If it - * stops, the test still passes; the toolkit-lib unit tests are the lasting coverage. - */ integTest( 'cdk diff --method=change-set shows changes to JSON built with an intrinsic', withSpecificFixture('json-intrinsic-app', async (fixture) => { @@ -21,7 +11,7 @@ integTest( // WHEN - a key is added to the JSON const diff = await fixture.cdk(['diff', '--method=change-set', stackName], { - modEnv: { VARIANT: 'add-key' }, + modEnv: { JSON_CHANGE: 'add-key' }, }); // THEN - the parameter value is shown as changed diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-reordered-keys-in-json-built-with-an-intrinsic.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-reordered-keys-in-json-built-with-an-intrinsic.integtest.ts index 7a0233481..4fa3270d6 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-reordered-keys-in-json-built-with-an-intrinsic.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-diff---method-change-set-shows-reordered-keys-in-json-built-with-an-intrinsic.integtest.ts @@ -1,12 +1,6 @@ import { integTest, withSpecificFixture } from '../../../lib'; import '../../../lib/assertions'; -/** - * Regression test for https://github.com/aws/aws-cdk-cli/issues/2036 - * - * Reordering the keys changes the stored string, but CloudFormation leaves the change out of a change - * set described with IncludePropertyValues, without a warning. - */ integTest( 'cdk diff --method=change-set shows reordered keys in JSON built with an intrinsic', withSpecificFixture('json-intrinsic-app', async (fixture) => { @@ -17,7 +11,7 @@ integTest( // WHEN - the keys of the JSON are reordered const diff = await fixture.cdk(['diff', '--method=change-set', stackName], { - modEnv: { VARIANT: 'reorder-keys' }, + modEnv: { JSON_CHANGE: 'reorder-keys' }, }); // THEN - the parameter value is shown as changed diff --git a/packages/@aws-cdk-testing/cli-integ/tests/toolkit-lib-integ-tests/toolkit-deploy-approval-shows-a-key-added-to-json-built-with-an-intrinsic.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/toolkit-lib-integ-tests/toolkit-deploy-approval-shows-a-key-added-to-json-built-with-an-intrinsic.integtest.ts new file mode 100644 index 000000000..81174b914 --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/tests/toolkit-lib-integ-tests/toolkit-deploy-approval-shows-a-key-added-to-json-built-with-an-intrinsic.integtest.ts @@ -0,0 +1,32 @@ +/* eslint-disable import/no-extraneous-dependencies */ +import * as toolkit from '@aws-cdk/toolkit-lib'; +import { approvalRecordingIoHost, assemblyFromCdkAppDir, hasParameterValueChange, toolkitFromFixture } from './toolkit-helpers'; +import { integTest, withSpecificFixture } from '../../lib'; + +integTest( + 'toolkit deploy approval shows a key added to JSON built with an intrinsic', + withSpecificFixture('json-intrinsic-app', async (fixture) => { + const stacks: toolkit.StackSelector = { + strategy: toolkit.StackSelectionStrategy.PATTERN_MUST_MATCH_SINGLE, + patterns: [fixture.fullStackName('json-intrinsic')], + }; + + const approving = toolkitFromFixture(fixture, { ioHost: approvalRecordingIoHost(true).ioHost }); + + // GIVEN - a deployed parameter holding {"stack":"","a":"1"} + await approving.deploy(await assemblyFromCdkAppDir(approving, fixture), { stacks }); + + // WHEN - a key is added, and the deploy approval is declined + const { ioHost, approvals } = approvalRecordingIoHost(false); + const tk = toolkitFromFixture(fixture, { ioHost }); + await expect(tk.deploy(await assemblyFromCdkAppDir(tk, fixture, { JSON_CHANGE: 'add-key' }), { + stacks, + deploymentMethod: { method: 'change-set' }, + })).rejects.toThrow(); + + // THEN - the diff sent with the approval request shows the parameter value as changed + expect(approvals).toHaveLength(1); + const diffs = Object.values(approvals[0].data.templateDiffs); + expect(diffs.some(hasParameterValueChange)).toBe(true); + }), +); diff --git a/packages/@aws-cdk-testing/cli-integ/tests/toolkit-lib-integ-tests/toolkit-diff-change-set-shows-a-key-added-to-json-in-a-nested-stack.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/toolkit-lib-integ-tests/toolkit-diff-change-set-shows-a-key-added-to-json-in-a-nested-stack.integtest.ts new file mode 100644 index 000000000..08fc998cc --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/tests/toolkit-lib-integ-tests/toolkit-diff-change-set-shows-a-key-added-to-json-in-a-nested-stack.integtest.ts @@ -0,0 +1,27 @@ +/* eslint-disable import/no-extraneous-dependencies */ +import * as toolkit from '@aws-cdk/toolkit-lib'; +import { approvalRecordingIoHost, assemblyFromCdkAppDir, hasParameterValueChange, toolkitFromFixture } from './toolkit-helpers'; +import { integTest, withSpecificFixture } from '../../lib'; + +integTest( + 'toolkit diff --method=change-set shows a key added to JSON in a nested stack', + withSpecificFixture('json-intrinsic-app', async (fixture) => { + const stacks: toolkit.StackSelector = { + strategy: toolkit.StackSelectionStrategy.PATTERN_MUST_MATCH_SINGLE, + patterns: [fixture.fullStackName('json-intrinsic-nested')], + }; + const tk = toolkitFromFixture(fixture, { ioHost: approvalRecordingIoHost(true).ioHost }); + + // GIVEN - a deployed nested parameter holding {"stack":"","a":"1"} + await tk.deploy(await assemblyFromCdkAppDir(tk, fixture), { stacks }); + + // WHEN - a key is added to the JSON + const result = await tk.diff(await assemblyFromCdkAppDir(tk, fixture, { JSON_CHANGE: 'add-key' }), { + stacks, + method: toolkit.DiffMethod.ChangeSet({ fallbackToTemplate: false }), + }); + + // THEN - the nested parameter value is shown as changed + expect(Object.values(result).some(hasParameterValueChange)).toBe(true); + }), +); diff --git a/packages/@aws-cdk-testing/cli-integ/tests/toolkit-lib-integ-tests/toolkit-execute-change-set-approval-shows-a-key-added-to-json-built-with-an-intrinsic.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/toolkit-lib-integ-tests/toolkit-execute-change-set-approval-shows-a-key-added-to-json-built-with-an-intrinsic.integtest.ts new file mode 100644 index 000000000..d1d47f17e --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/tests/toolkit-lib-integ-tests/toolkit-execute-change-set-approval-shows-a-key-added-to-json-built-with-an-intrinsic.integtest.ts @@ -0,0 +1,36 @@ +/* eslint-disable import/no-extraneous-dependencies */ +import * as toolkit from '@aws-cdk/toolkit-lib'; +import { approvalRecordingIoHost, assemblyFromCdkAppDir, hasParameterValueChange, toolkitFromFixture } from './toolkit-helpers'; +import { integTest, withSpecificFixture } from '../../lib'; + +integTest( + 'toolkit execute-change-set approval shows a key added to JSON built with an intrinsic', + withSpecificFixture('json-intrinsic-app', async (fixture) => { + const stacks: toolkit.StackSelector = { + strategy: toolkit.StackSelectionStrategy.PATTERN_MUST_MATCH_SINGLE, + patterns: [fixture.fullStackName('json-intrinsic')], + }; + const changeSetName = 'json-intrinsic-add-key'; + const approving = toolkitFromFixture(fixture, { ioHost: approvalRecordingIoHost(true).ioHost }); + + // GIVEN - a deployed parameter holding {"stack":"","a":"1"}, and a change set that adds a key + await approving.deploy(await assemblyFromCdkAppDir(approving, fixture), { stacks }); + await approving.deploy(await assemblyFromCdkAppDir(approving, fixture, { JSON_CHANGE: 'add-key' }), { + stacks, + deploymentMethod: { method: 'change-set', execute: false, changeSetName }, + }); + + // WHEN - the change set is executed, and the approval is declined + const { ioHost, approvals } = approvalRecordingIoHost(false); + const tk = toolkitFromFixture(fixture, { ioHost }); + await expect(tk.deploy(await assemblyFromCdkAppDir(tk, fixture, { JSON_CHANGE: 'add-key' }), { + stacks, + deploymentMethod: { method: 'execute-change-set', changeSetName }, + })).rejects.toThrow(); + + // THEN - the diff sent with the approval request shows the parameter value as changed + expect(approvals).toHaveLength(1); + const diffs = Object.values(approvals[0].data.templateDiffs); + expect(diffs.some(hasParameterValueChange)).toBe(true); + }), +); diff --git a/packages/@aws-cdk-testing/cli-integ/tests/toolkit-lib-integ-tests/toolkit-helpers.ts b/packages/@aws-cdk-testing/cli-integ/tests/toolkit-lib-integ-tests/toolkit-helpers.ts index ea28cfefc..fd22bffd9 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/toolkit-lib-integ-tests/toolkit-helpers.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/toolkit-lib-integ-tests/toolkit-helpers.ts @@ -29,7 +29,7 @@ export function toolkitFromFixture(fixture: AwsContext, options?: Omit = {}) { const cdkAppDir = fixture.integTestDir; const cdkJson = await JSON.parse(await fs.readFile(path.join(cdkAppDir, 'cdk.json'), 'utf-8')); const app = cdkJson.app; @@ -39,6 +39,40 @@ export async function assemblyFromCdkAppDir(tk: toolkit.Toolkit, fixture: TestFi env: { STACK_NAME_PREFIX: fixture.stackNamePrefix, PACKAGE_LAYOUT_VERSION: '2', + ...env, }, }); } + +/** + * An IoHost that answers deploy approval requests with `approve`, gives every other request its + * default response, and records the approval requests it was asked + */ +export function approvalRecordingIoHost(approve: boolean) { + const approvals: toolkit.IoRequest[] = []; + const ioHost: toolkit.IIoHost = { + notify: async () => { + }, + requestResponse: async (msg: toolkit.IoRequest): Promise => { + if (msg.code !== 'CDK_TOOLKIT_I5060') { + return msg.defaultResponse; + } + approvals.push(msg); + return approve as unknown as T; + }, + }; + return { ioHost, approvals }; +} + +/** + * Whether a template diff contains a change to the `Value` of an SSM parameter + */ +export function hasParameterValueChange(diff: any): boolean { + let found = false; + diff.resources.forEachDifference((_logicalId: string, change: any) => { + if (change.resourceType === 'AWS::SSM::Parameter' && change.propertyUpdates.Value) { + found = true; + } + }); + return found; +} diff --git a/packages/@aws-cdk/toolkit-lib/lib/actions/diff/private/helpers.ts b/packages/@aws-cdk/toolkit-lib/lib/actions/diff/private/helpers.ts index a2df50fd2..94a073e24 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/actions/diff/private/helpers.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/actions/diff/private/helpers.ts @@ -159,6 +159,7 @@ async function attachNestedChangeSetData( ioHelper, stackNameOrArn: rc.PhysicalResourceId ?? rc.LogicalResourceId, changeSetNameOrArn: rc.ChangeSetId, + templates: { deployed: nested.deployedTemplate, generated: nested.generatedTemplate }, }).waitForSettled(); // Replace the entry with one that includes the changeset diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/change-sets/change-set-describer.ts b/packages/@aws-cdk/toolkit-lib/lib/api/change-sets/change-set-describer.ts index a4e733a06..93bc267b7 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/change-sets/change-set-describer.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/change-sets/change-set-describer.ts @@ -30,15 +30,11 @@ export interface ChangeSetDescriberProps { /** * The templates the change set goes from and to. * - * When given, a created change set whose description would hide one of these template differences - * from the diff is described a second time without property values, and whatever the first - * description left out is put back. - * - * Optional because only callers that show the change set as a diff need the check. - * - * @default - no templates, so only the incomplete-data warning causes a second description + * A created change set whose description would hide one of these template differences is + * described a second time without property values, and whatever the first description left + * out is put back. */ - readonly templates?: ChangeSetTemplates; + readonly templates: ChangeSetTemplates; } /** @@ -98,7 +94,7 @@ export class ChangeSetDescriber { private readonly changeSetNameOrArn: string; private readonly stackDisplayName: string; private readonly changeSetDisplayName: string; - private readonly templates?: ChangeSetTemplates; + private readonly templates: ChangeSetTemplates; constructor(props: ChangeSetDescriberProps) { this.cfn = props.cfn; @@ -240,7 +236,7 @@ export class ChangeSetDescriber { * Only a created change set can hide anything. */ private async hidesTemplateDifferences(description: DescribeChangeSetCommandOutput): Promise { - if (!this.templates || description.Status !== ChangeSetStatus.CREATE_COMPLETE) { + if (description.Status !== ChangeSetStatus.CREATE_COMPLETE) { return false; } @@ -256,32 +252,7 @@ export class ChangeSetDescriber { } private async describeOnce(includePropertyValues: boolean): Promise { - const response = await this.cfn.describeChangeSet({ - StackName: this.stackNameOrArn, - ChangeSetName: this.changeSetNameOrArn, - IncludePropertyValues: includePropertyValues, - }); - - // Traverse all pages, so that `Changes` is always the complete set of changes. A truncated - // list would silently make callers believe a resource is unchanged. - while (response.NextToken != null) { - const nextPage = await this.cfn.describeChangeSet({ - StackName: response.StackId ?? this.stackNameOrArn, - ChangeSetName: response.ChangeSetId ?? this.changeSetNameOrArn, - NextToken: response.NextToken, - IncludePropertyValues: includePropertyValues, - }); - - // Consolidate the changes - if (nextPage.Changes != null) { - response.Changes = response.Changes != null ? response.Changes.concat(nextPage.Changes) : nextPage.Changes; - } - - // Forward the new NextToken - response.NextToken = nextPage.NextToken; - } - - return response; + return describeAllChangeSetPages(this.cfn, this.stackNameOrArn, this.changeSetNameOrArn, { includePropertyValues }); } /** @@ -364,3 +335,44 @@ const INCOMPLETE_PROPERTY_VALUES_WARNING = '[WARN] --include-property-values opt function hasIncompletePropertyValuesWarning(description: DescribeChangeSetCommandOutput): boolean { return description.StatusReason?.includes(INCOMPLETE_PROPERTY_VALUES_WARNING) ?? false; } + +/** + * Describe a change set once, with all pages of `Changes`. + * + * Use `ChangeSetDescriber` to show a change set: described with property values, CloudFormation can + * leave changes out, and only the describer puts them back. Described without property values, it + * lists every change, so this is enough for callers that only need which resources change. + */ +export async function describeAllChangeSetPages( + cfn: ICloudFormationClient, + stackNameOrArn: string, + changeSetNameOrArn: string, + options: { includePropertyValues: boolean }, +): Promise { + const response = await cfn.describeChangeSet({ + StackName: stackNameOrArn, + ChangeSetName: changeSetNameOrArn, + IncludePropertyValues: options.includePropertyValues, + }); + + // Traverse all pages, so that `Changes` is always the complete set of changes. A truncated + // list would silently make callers believe a resource is unchanged. + while (response.NextToken != null) { + const nextPage = await cfn.describeChangeSet({ + StackName: response.StackId ?? stackNameOrArn, + ChangeSetName: response.ChangeSetId ?? changeSetNameOrArn, + NextToken: response.NextToken, + IncludePropertyValues: options.includePropertyValues, + }); + + // Consolidate the changes + if (nextPage.Changes != null) { + response.Changes = response.Changes != null ? response.Changes.concat(nextPage.Changes) : nextPage.Changes; + } + + // Forward the new NextToken + response.NextToken = nextPage.NextToken; + } + + return response; +} diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/cfn-api.ts b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/cfn-api.ts index 16be61da3..c2ca8a629 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/cfn-api.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/cfn-api.ts @@ -166,6 +166,7 @@ interface PreparedChangeSetEnv { stackExistedBefore: boolean; executionRoleArn: string | undefined; diagnoser: CloudFormationStackDiagnoser; + deployedStack: CloudFormationStack; } async function prepareChangeSetEnv( @@ -195,7 +196,7 @@ async function prepareChangeSetEnv( topLevelStackHierarchicalId: options.stack.hierarchicalId, }); - return { cfn, bodyParameter, exists, stackExistedBefore: stack.exists, executionRoleArn, diagnoser }; + return { cfn, bodyParameter, exists, stackExistedBefore: stack.exists, executionRoleArn, diagnoser, deployedStack: stack }; } /** @@ -318,7 +319,7 @@ export async function createValidationChangeSet( ioHelper: IoHelper, options: Omit, ): Promise { - const { cfn, bodyParameter, exists, stackExistedBefore, executionRoleArn, diagnoser } = await prepareChangeSetEnv(ioHelper, options); + const { cfn, bodyParameter, exists, stackExistedBefore, executionRoleArn, diagnoser, deployedStack } = await prepareChangeSetEnv(ioHelper, options); const changeSetName = `cdk-validate-${options.uuid}`; const templateParams = TemplateParameters.fromTemplate(options.stack.template); @@ -345,6 +346,7 @@ export async function createValidationChangeSet( ioHelper, stackNameOrArn: changeSet.StackId ?? options.stack.stackName, changeSetNameOrArn: changeSet.Id ?? changeSetName, + templates: { deployed: exists ? await deployedStack.template() : {}, generated: options.stack.template }, }).waitForReport({ diagnoser }); return report; diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts index d5abea441..3598ad577 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack.ts @@ -33,7 +33,7 @@ import type { StabilizingResource } from '../../toolkit/types'; import { formatErrorMessage } from '../../util'; import { changeSetNameFromArn } from '../../util/cloudformation'; import type { SDK, SdkProvider, ICloudFormationClient } from '../aws-auth/private'; -import type { ChangeSetReport } from '../change-sets'; +import type { ChangeSetReport, ChangeSetTemplates } from '../change-sets'; import { ChangeSetDescriber } from '../change-sets'; import type { TemplateBodyParameter } from '../cloudformation'; import { makeBodyParameter, CfnEvaluationException, CloudFormationStack } from '../cloudformation'; @@ -546,6 +546,7 @@ class FullCloudFormationDeployment { ioHelper: this.ioHelper, stackNameOrArn: this.stackName, changeSetNameOrArn: deploymentMethod.changeSetName, + templates: await this.changeSetTemplates(), }).describeForExecution({ diagnoser: this.diagnoser }); return this.checkAndExecuteChangeSet(changeSetReport); @@ -647,6 +648,7 @@ class FullCloudFormationDeployment { ioHelper: this.ioHelper, stackNameOrArn: changeSet.StackId ?? this.stackName, changeSetNameOrArn: changeSet.Id ?? changeSetName, + templates: await this.changeSetTemplates(), }).waitAndThrowOnProblem({ diagnoser: this.diagnoser, }); @@ -669,10 +671,18 @@ class FullCloudFormationDeployment { ioHelper: this.ioHelper, stackNameOrArn: this.stackName, changeSetNameOrArn: changeSetNameOrArn, + templates: await this.changeSetTemplates(), }).waitForGone(); } } + /** + * The templates a change set on this stack goes from and to. + */ + private async changeSetTemplates(): Promise { + return { deployed: await this.cloudFormationStack.template(), generated: this.stackArtifact.template }; + } + private async updateTerminationProtection() { // Update termination protection only if it has changed. const terminationProtection = this.stackArtifact.terminationProtection ?? false; diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deployments.ts b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deployments.ts index 5c73ba961..93c904d98 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deployments.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deployments.ts @@ -523,12 +523,15 @@ export class Deployments { * * @param stack - The Stack artifact * @param changeSetName - The name of the change set + * @param deployedTemplate - The deployed template the diff compares against, so that a + * description that would hide a template difference is completed (see `ChangeSetDescriber`) * @param stackArn - Optional the exact arn of the stack. Prefer this if you have it available. * @returns the DescribeChangeSet response */ public async describeChangeSet( stack: cxapi.CloudFormationStackArtifact, changeSetName: string, + deployedTemplate: Template, stackArn?: string, ): Promise { const env = await this.envs.accessStackForMutableStackOperations(stack); @@ -538,6 +541,7 @@ export class Deployments { ioHelper: this.ioHelper, stackNameOrArn: stackArn ?? stack.stackName, changeSetNameOrArn: changeSetName, + templates: { deployed: deployedTemplate, generated: stack.template }, }).waitAndThrowOnProblem({ diagnoser: new CloudFormationStackDiagnoser({ sdk: env.sdk, diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/diagnosing/stack-diagnoser.ts b/packages/@aws-cdk/toolkit-lib/lib/api/diagnosing/stack-diagnoser.ts index 2a9165d0a..e8a4ae3d1 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/diagnosing/stack-diagnoser.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/diagnosing/stack-diagnoser.ts @@ -7,7 +7,7 @@ import { Diagnosis } from './diagnosis'; import { investigateResource } from './resource-investigation'; import type { AdditionalDiagnosticContext, StackProblemSource, TracedResourceError } from '../../actions/diagnose'; import type { ICloudFormationClient, SDK } from '../aws-auth/sdk'; -import { ChangeSetDescriber } from '../change-sets'; +import { describeAllChangeSetPages } from '../change-sets'; import type { EnvironmentResources } from '../environment'; import type { IoHelper } from '../io/private/io-helper'; import type { ISourceTracer } from '../source-tracing/private/source-tracing'; @@ -562,12 +562,8 @@ export class CloudFormationStackDiagnoser { }); } - const nestedCs = await new ChangeSetDescriber({ - cfn: this.cfn, - ioHelper: this.props.ioHelper, - stackNameOrArn: nested.stackArn, - changeSetNameOrArn: nested.changeSetArn, - }).describeCurrentState(); + // Diagnosing only needs the status and which resources change, not property values. + const nestedCs = await describeAllChangeSetPages(this.cfn, nested.stackArn, nested.changeSetArn, { includePropertyValues: false }); const nestedDiag = new CloudFormationStackDiagnoser(this.props); nestedDiag.parentStackLogicalIds = [...this.parentStackLogicalIds, nested.logicalId]; @@ -582,12 +578,7 @@ export class CloudFormationStackDiagnoser { // The status reason only includes the change set ID, but we also need the stack name. The way to get this is // describe the current change set, then from the Changes find the stack whose ChangeSetId is mentioned in the // status reason, then look up that change set and recurse into a regular change set diagnosis. - const resp = await new ChangeSetDescriber({ - cfn: this.cfn, - ioHelper: this.props.ioHelper, - stackNameOrArn: changeSet.StackId!, - changeSetNameOrArn: changeSet.ChangeSetId!, - }).describeCurrentState(); + const resp = await describeAllChangeSetPages(this.cfn, changeSet.StackId!, changeSet.ChangeSetId!, { includePropertyValues: false }); for (const change of resp.Changes ?? []) { if (change.Type === ChangeType.Resource && change.ResourceChange?.ResourceType === 'AWS::CloudFormation::Stack' && change.ResourceChange?.ChangeSetId && changeSet.StatusReason?.includes(change.ResourceChange?.ChangeSetId)) { diff --git a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts index 18d24aec8..2102bbe77 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts @@ -936,13 +936,15 @@ export class Toolkit extends CloudAssemblySourceBuilder { // changes — there is nothing for the user to approve. Outputs, stack ARN, // and timings are still emitted via the normal no-op deploy path below. if (!prepareResult?.noOp) { + // Nested stacks are included, so that changes to their resources are part of the diff and destructive changes + const { deployedRootTemplate, nestedStacks } = await deployments.readCurrentTemplateWithNestedStacks(stack); + // For execute-change-set, describe the existing change set so we can show an accurate diff const diffChangeSet = isExecuteChangeSetDeployment(options.deploymentMethod) - ? (await deployments.describeChangeSet(stack, options.deploymentMethod.changeSetName, prepareResult?.stackArn)).changeSet + ? (await deployments.describeChangeSet( + stack, options.deploymentMethod.changeSetName, deployedRootTemplate, prepareResult?.stackArn, + )).changeSet : prepareResult?.changeSet; - - // Nested stacks are included, so that changes to their resources are part of the diff and destructive changes - const { deployedRootTemplate, nestedStacks } = await deployments.readCurrentTemplateWithNestedStacks(stack); const formatter = new DiffFormatter({ templateInfo: { oldTemplate: deployedRootTemplate, diff --git a/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts b/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts index 0d8539f5d..3111bd3ae 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts @@ -262,6 +262,24 @@ IAM Statement Changes ); }); + test('execute-change-set describes the change set with the deployed template, so the approval diff can be completed (#2036)', async () => { + // GIVEN + const deployedRootTemplate = { Resources: { Param: { Type: 'AWS::SSM::Parameter', Properties: { Type: 'String', Value: 'deployed' } } } }; + jest.spyOn(deployments.Deployments.prototype, 'readCurrentTemplateWithNestedStacks').mockResolvedValue({ deployedRootTemplate, nestedStacks: {} }); + const describeChangeSet = jest.spyOn(deployments.Deployments.prototype, 'describeChangeSet').mockResolvedValue({ + changeSet: { Status: 'CREATE_COMPLETE', Changes: [], $metadata: {} }, + } as any); + + // WHEN + const cx = await cdkOutFixture(toolkit, 'stack-with-bucket'); + await toolkit.deploy(cx, { + deploymentMethod: { method: 'execute-change-set', changeSetName: 'my-change-set' }, + }); + + // THEN + expect(describeChangeSet).toHaveBeenCalledWith(expect.anything(), 'my-change-set', deployedRootTemplate, undefined); + }); + test('uses template-only diff with method=direct', async () => { // WHEN const cx = await cdkOutFixture(toolkit, 'stack-with-bucket'); diff --git a/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts b/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts index 3fcc0633c..f3063cdad 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts @@ -809,6 +809,64 @@ describe('diff', () => { })); }); + test('ChangeSet diff shows an added key in JSON in a nested stack that CloudFormation silently drops (issue #2036)', async () => { + // GIVEN - a nested stack whose parameter gains key "b" + const rootChangeSetId = 'arn:aws:cloudformation:us-east-1:123456789012:changeSet/cdk-diff'; + const nestedChangeSetId = 'arn:aws:cloudformation:us-east-1:123456789012:changeSet/cdk-diff-nested/nested'; + const nestedParameter = (value: unknown) => ({ + Resources: { Param: { Type: 'AWS::SSM::Parameter', Properties: { Type: 'String', Value: value } } }, + }); + jest.spyOn(deployments.Deployments.prototype, 'readCurrentTemplateWithNestedStacks').mockImplementation(async (stack) => ({ + deployedRootTemplate: stack.template, + nestedStacks: { + NestedStack: { + deployedTemplate: nestedParameter(jsonWithStackName({ a: '1' })), + generatedTemplate: nestedParameter(jsonWithStackName({ a: '1', b: '2' })), + physicalName: 'Stack1-NestedStack-ABC', + nestedStackTemplates: {}, + }, + }, + })); + givenChangeSetCanBeCreated('Stack1'); + + // The root change set reports the nested stack update... + mockCloudFormationClient.on(DescribeChangeSetCommand, { ChangeSetName: rootChangeSetId }).resolves({ + Status: 'CREATE_COMPLETE', + ExecutionStatus: 'AVAILABLE', + Changes: [{ + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: 'NestedStack', + PhysicalResourceId: 'arn:aws:cloudformation:us-east-1:123456789012:stack/Stack1-NestedStack-ABC/abc', + ResourceType: 'AWS::CloudFormation::Stack', + ChangeSetId: nestedChangeSetId, + }, + }], + }); + // ...and the nested change set leaves the parameter change out of its IncludePropertyValues description + mockCloudFormationClient.on(DescribeChangeSetCommand, { ChangeSetName: nestedChangeSetId, IncludePropertyValues: true }).resolves({ + Status: 'CREATE_COMPLETE', + Changes: [], + }); + mockCloudFormationClient.on(DescribeChangeSetCommand, { ChangeSetName: nestedChangeSetId, IncludePropertyValues: false }).resolves({ + Status: 'CREATE_COMPLETE', + Changes: [parameterValueChange('Param')], + }); + + // WHEN + const cx = await cdkOutFixture(toolkit, 'stack-with-bucket'); + const result = await toolkit.diff(cx, { + stacks: { strategy: StackSelectionStrategy.ALL_STACKS }, + method: DiffMethod.ChangeSet({ fallbackToTemplate: false }), + }); + + // THEN - the nested diff shows the change + const valueChange = result['Stack1-NestedStack-ABC'].resources.get('Param').propertyUpdates.Value; + expect(valueChange.isDifferent).toBe(true); + expect(valueChange.changeImpact).toEqual(ResourceImpact.WILL_UPDATE); + }); + test('ChangeSet diff still hides a rewrite that resolves to the same value, Fn::Sub to Fn::Join', async () => { // GIVEN - the deployed parameter builds the same JSON with Fn::Sub instead of Fn::Join givenDeployedProperty('Param165332EC', 'Value', { 'Fn::Sub': '{"stack":"${AWS::StackName}","a":"1","b":"2"}' }); diff --git a/packages/@aws-cdk/toolkit-lib/test/api/change-sets/change-set-describer.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/change-sets/change-set-describer.test.ts index 11531e564..dcdd4486e 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/change-sets/change-set-describer.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/change-sets/change-set-describer.test.ts @@ -31,6 +31,8 @@ function describer(props: Partial = {}) { ioHelper, stackNameOrArn: 'my-stack', changeSetNameOrArn: 'my-change-set', + // Identical templates, so the template check never asks for a second description. + templates: { deployed: {}, generated: {} }, ...props, }); } diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts index 1a5f59e34..9ceb9bb78 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/deploy-stack.test.ts @@ -349,9 +349,7 @@ test('execute-change-set describes and executes an existing change set', async ( test('execute-change-set throws if change set is not ready', async () => { // GIVEN - mockCloudFormationClient.on(DescribeStacksCommand).resolves({ - Stacks: [{ ...baseResponse }], - }); + givenStackExists(); mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ Status: 'CREATE_IN_PROGRESS', StatusReason: 'doing stuff', @@ -368,9 +366,7 @@ test('execute-change-set throws if change set is not ready', async () => { test('execute-change-set throws if change set failed', async () => { // GIVEN - mockCloudFormationClient.on(DescribeStacksCommand).resolves({ - Stacks: [{ ...baseResponse }], - }); + givenStackExists(); mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ Status: 'FAILED', StatusReason: 'some reason', @@ -1003,6 +999,45 @@ test('deployStack reports no change if describeChangeSet returns an error that i expect(deployResult.type === 'did-deploy-stack' && deployResult.noOp).toEqual(true); }); +test('describes the change set again without property values when it would hide a template difference (#2036)', async () => { + // GIVEN - a JSON string built with `Stack.toJsonString()` gains a key. CloudFormation reports the + // change when described without property values, and leaves it out, without a warning, when + // described with them. + const json = (lastFragment: string) => ({ 'Fn::Join': ['', ['{"stack":"', { Ref: 'AWS::StackName' }, lastFragment]] }); + const parameterTemplate = (value: any) => ({ Resources: { Param: { Type: 'AWS::SSM::Parameter', Properties: { Type: 'String', Value: value } } } }); + const parameterValueChange: Change = { + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: 'Param', + ResourceType: 'AWS::SSM::Parameter', + Replacement: 'False', + Details: [{ Target: { Attribute: 'Properties', Name: 'Value', RequiresRecreation: 'Never' }, Evaluation: 'Static', ChangeSource: 'DirectModification' }], + }, + }; + givenStackExists(); + givenTemplateIs(parameterTemplate(json('","a":"1"}'))); + fakeCfn.overrideChangeSetChanges = [parameterValueChange]; + mockCloudFormationClient.on(DescribeChangeSetCommand, { IncludePropertyValues: true }).callsFake(async (input) => ({ + ...await fakeCfn.describeChangeSet(input), + Changes: [], + })); + + // WHEN - deploy prepares the change set before showing the approval diff + const result = await testDeployStack({ + ...standardDeployStackArguments(testStack({ stackName: 'withouterrors', template: parameterTemplate(json('","a":"1","b":"2"}')) })), + deploymentMethod: { method: 'change-set', execute: false }, + willExecuteChangeSet: true, + }); + + // THEN - the change set is described again, and the returned description, which the approval + // diff uses, includes the change + const withoutPropertyValues = mockCloudFormationClient.commandCalls(DescribeChangeSetCommand) + .filter((call) => call.args[0].input.IncludePropertyValues === false); + expect(withoutPropertyValues).toHaveLength(1); + expect(result.type === 'did-deploy-stack' ? result.changeSet?.Changes : undefined).toEqual([parameterValueChange]); +}); + test('deployStack throws error in case of early validation failures', async () => { mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ ChangeSetName: 'cdk-deploy-change-set', @@ -1949,6 +1984,42 @@ describe('execute-change-set deployment method', () => { expect(mockCloudFormationClient).not.toHaveReceivedCommand(ExecuteChangeSetCommand); }); + test('returns replacement-requires-rollback when the description silently leaves the replacement out (#2036)', async () => { + // GIVEN - renaming the queue, with a name resolved at deploy time, replaces it. The change set's + // IncludePropertyValues description leaves that out without a warning. + const queueTemplate = (suffix: string) => ({ + Resources: { + Queue4A7E3555: { Type: 'AWS::SQS::Queue', Properties: { QueueName: { 'Fn::Join': ['', [{ Ref: 'AWS::StackName' }, suffix]] } } }, + }, + }); + const queueReplacement = replacementChange(); + queueReplacement.ResourceChange!.Details![0].Target!.Name = 'QueueName'; + givenStackExists(); + givenTemplateIs(queueTemplate('-a')); + givenChangeSetExists({ + Status: 'CREATE_COMPLETE', + ChangeSetName: 'my-change-set', + Changes: [queueReplacement], + }); + mockCloudFormationClient.on(DescribeChangeSetCommand, { IncludePropertyValues: true }).callsFake(async (input) => ({ + ...await fakeCfn.describeChangeSet(input), + Changes: [], + })); + + // WHEN + const result = await advanceTime( + testDeployStack({ + ...standardDeployStackArguments(testStack({ stackName: 'withouterrors', template: queueTemplate('-b') })), + deploymentMethod: { method: 'execute-change-set', changeSetName: 'my-change-set' }, + rollback: false, + }), + ); + + // THEN + expect(result.type).toEqual('replacement-requires-rollback'); + expect(mockCloudFormationClient).not.toHaveReceivedCommand(ExecuteChangeSetCommand); + }); + test('is never skipped by canSkipDeploy', async () => { // GIVEN - stack exists with identical template (would normally skip) givenStackExists(); diff --git a/packages/@aws-cdk/toolkit-lib/test/api/diagnosing/stack-diagnoser.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/diagnosing/stack-diagnoser.test.ts index a2703eaf4..b3bd1b98a 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/diagnosing/stack-diagnoser.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/diagnosing/stack-diagnoser.test.ts @@ -1,5 +1,7 @@ +import type { Change } from '@aws-sdk/client-cloudformation'; import { ChangeSetStatus, + DescribeChangeSetCommand, DescribeStackResourcesCommand, GetHookResultCommand, ListHookResultsCommand, @@ -294,6 +296,60 @@ describe('CloudFormationStackDiagnoser', () => { expect(fakeTracer.stackCalls.length + fakeTracer.resourceCalls.length).toBeGreaterThan(0); }); + test('finds the failed nested change set on a later page, describing without property values', async () => { + // GIVEN - the parent change set lists the failed nested stack on its second page + fakeCfn.createStackSync({ StackName: 'ParentStack', StackStatus: 'CREATE_COMPLETE' }); + fakeCfn.createStackSync({ StackName: 'NestedStack', StackStatus: 'CREATE_COMPLETE' }); + fakeCfn.createChangeSetSync({ + StackName: 'NestedStack', + ChangeSetName: 'nested-cs', + Status: 'FAILED', + StatusReason: 'Some nested error', + }); + const nestedCsId = fakeCfn.accessStack('NestedStack').changeSets[0].id; + const nestedStackChange: Change = { + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: 'NestedStackResource', + ResourceType: 'AWS::CloudFormation::Stack', + PhysicalResourceId: 'NestedStack', + ChangeSetId: nestedCsId, + }, + }; + fakeCfn.createChangeSetSync({ + StackName: 'ParentStack', + ChangeSetName: 'parent-cs', + Status: 'FAILED', + StatusReason: `Nested change set ${nestedCsId} failed`, + Changes: [nestedStackChange], + }); + const parentCsId = fakeCfn.accessStack('ParentStack').changeSets[0].id; + mockCloudFormationClient.on(DescribeChangeSetCommand).callsFake(async (input) => { + const response = await fakeCfn.describeChangeSet(input); + if (input.ChangeSetName !== parentCsId) { + return response; + } + return input.NextToken + ? { ...response, Changes: [nestedStackChange] } + : { ...response, Changes: [], NextToken: 'page-2' }; + }); + + // WHEN + const result = await makeDiagnoser().diagnoseFromFresh('ParentStack'); + + // THEN + expect(result.result).toMatchObject({ + type: 'problem', + detectedBy: { type: 'change-set' }, + problems: [expect.anything()], + }); + const nestedDescribes = mockCloudFormationClient.commandCalls(DescribeChangeSetCommand) + .filter((call) => call.args[0].input.ChangeSetName === nestedCsId); + expect(nestedDescribes.length).toBeGreaterThan(0); + expect(nestedDescribes.every((call) => call.args[0].input.IncludePropertyValues === false)).toBe(true); + }); + test('diagnoses early validation failure', async () => { fakeCfn.createStackSync({ StackName: 'MyStack', StackStatus: 'CREATE_COMPLETE' }); fakeCfn.createChangeSetSync({