fix(toolkit-lib): deploy approval diff hides an added key in a JSON string with an intrinsic

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

@@ -1,47 +1,105 @@
 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;
 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:
+ *
+ * - `<prefix>-json-intrinsic`: an SSM string parameter
+ * - `<prefix>-json-intrinsic-nested`: an SSM string parameter in a nested stack
+ * - `<prefix>-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":"<stack name>","a":"1"}
+ * change-value              {"stack":"<stack name>","a":"2"}
+ * add-key                   {"stack":"<stack name>","a":"1","b":"2"}
+ * add-key-with-token        {"stack":"<stack name>","a":"1","region":"<region>"}
+ * reorder-keys              {"a":"1","stack":"<stack name>"}
+ * same-value-with-fn-sub    {"stack":"<stack name>","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 `<prefix>-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 <prefix>-json-intrinsic
+ * env JSON_CHANGE=add-key npx cdk diff --method=change-set <prefix>-json-intrinsic
+ * env JSON_CHANGE=add-key DESCRIPTION=d2 npx cdk deploy --require-approval=any-change <prefix>-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();

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":"<stack name>","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');
+  }),
+);

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":"<stack name>","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');
+  }),
+);

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":"<stack name>","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/);
+  }),
+);

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":"<stack name>","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/);
+  }),
+);

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

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

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

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":"<nested stack name>","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/);
+  }),
+);

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

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