fix(cli): re-bootstrapping loses previous configuration (#10120)

Various properties of the bootstrapping stack you configured with
switches on the command line would be lost again if you performed
bootstrapping again. This includes:

- Template parameters such as the `qualifier`,
  `public-access-block-configuration`, `trust`,
  `cloudformation-execution-roles`.
- Stack attributes such as `termination-protection`, `tags`.

ALSO IN THIS PR

- In the CLI integ tests, only show the CLI output if the test fails.
  This reduces noise when trying to inspect the causes of a failed test.
- Some light refactoring (but not too much) of the division of
  responsibilities between `BootstrapStack`, `ToolkitInfo` and
  `CloudFormationStack`.

Fixes #10091.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

packages/aws-cdk/bin/cdk.ts

@@ -71,13 +71,13 @@ async function parseCommandLineArguments() {
       .option('bootstrap-bucket-name', { type: 'string', alias: ['b', 'toolkit-bucket-name'], desc: 'The name of the CDK toolkit bucket; bucket will be created and must not exist', default: undefined })
       .option('bootstrap-kms-key-id', { type: 'string', desc: 'AWS KMS master key ID used for the SSE-KMS encryption', default: undefined })
       .option('qualifier', { type: 'string', desc: 'Unique string to distinguish multiple bootstrap stacks', default: undefined })
-      .option('public-access-block-configuration', { type: 'boolean', desc: 'Block public access configuration on CDK toolkit bucket (enabled by default) ', default: true })
+      .option('public-access-block-configuration', { type: 'boolean', desc: 'Block public access configuration on CDK toolkit bucket (enabled by default) ', default: undefined })
       .option('tags', { type: 'array', alias: 't', desc: 'Tags to add for the stack (KEY=VALUE)', nargs: 1, requiresArg: true, default: [] })
       .option('execute', { type: 'boolean', desc: 'Whether to execute ChangeSet (--no-execute will NOT execute the ChangeSet)', default: true })
       .option('trust', { type: 'array', desc: 'The AWS account IDs that should be trusted to perform deployments into this environment (may be repeated)', default: [], nargs: 1, requiresArg: true, hidden: true })
       .option('cloudformation-execution-policies', { type: 'array', desc: 'The Managed Policy ARNs that should be attached to the role performing deployments into this environment. Required if --trust was passed (may be repeated)', default: [], nargs: 1, requiresArg: true, hidden: true })
       .option('force', { alias: 'f', type: 'boolean', desc: 'Always bootstrap even if it would downgrade template version', default: false })
-      .option('termination-protection', { type: 'boolean', default: false, desc: 'Toggle CloudFormation termination protection on the bootstrap stacks' })
+      .option('termination-protection', { type: 'boolean', default: undefined, desc: 'Toggle CloudFormation termination protection on the bootstrap stacks' })
       .option('show-template', { type: 'boolean', desc: 'Instead of actual bootstrapping, print the current CLI\'s bootstrapping template to stdout for customization.', default: false })
       .option('template', { type: 'string', requiresArg: true, desc: 'Use the template from the given file instead of the built-in one (use --show-template to obtain an example).' }),
     )
@@ -87,7 +87,7 @@ async function parseCommandLineArguments() {
       .option('require-approval', { type: 'string', choices: [RequireApproval.Never, RequireApproval.AnyChange, RequireApproval.Broadening], desc: 'What security-sensitive changes need manual approval' })
       .option('ci', { type: 'boolean', desc: 'Force CI detection', default: process.env.CI !== undefined })
       .option('notification-arns', { type: 'array', desc: 'ARNs of SNS topics that CloudFormation will notify with stack related events', nargs: 1, requiresArg: true })
-      .option('tags', { type: 'array', alias: 't', desc: 'Tags to add to the stack (KEY=VALUE)', nargs: 1, requiresArg: true })
+      .option('tags', { type: 'array', alias: 't', desc: 'Tags to add to the stack (KEY=VALUE), overrides tags from Cloud Assembly', nargs: 1, requiresArg: true })
       .option('execute', { type: 'boolean', desc: 'Whether to execute ChangeSet (--no-execute will NOT execute the ChangeSet)', default: true })
       .option('force', { alias: 'f', type: 'boolean', desc: 'Always deploy stack even if templates are identical', default: false })
       .option('parameters', { type: 'array', desc: 'Additional parameters passed to CloudFormation at deploy time (STACK:KEY=VALUE)', nargs: 1, requiresArg: true, default: {} })
@@ -270,8 +270,8 @@ async function initCommandLine() {
             kmsKeyId: configuration.settings.get(['toolkitBucket', 'kmsKeyId']),
             qualifier: args.qualifier,
             publicAccessBlockConfiguration: args.publicAccessBlockConfiguration,
-            trustedAccounts: args.trust,
-            cloudFormationExecutionPolicies: args.cloudformationExecutionPolicies,
+            trustedAccounts: arrayFromYargs(args.trust),
+            cloudFormationExecutionPolicies: arrayFromYargs(args.cloudformationExecutionPolicies),
           },
         });
 
@@ -336,6 +336,20 @@ async function initCommandLine() {
   }
 }
 
+/**
+ * Translate a Yargs input array to something that makes more sense in a programming language
+ * model (telling the difference between absence and an empty array)
+ *
+ * - An empty array is the default case, meaning the user didn't pass any arguments. We return
+ *   undefined.
+ * - If the user passed a single empty string, they did something like `--array=`, which we'll
+ *   take to mean they passed an empty array.
+ */
+function arrayFromYargs(xs: string[]): string[] | undefined {
+  if (xs.length === 0) { return undefined; }
+  return xs.filter(x => x !== '');
+}
+
 initCommandLine()
   .then(value => {
     if (value == null) { return; }

packages/aws-cdk/lib/api/bootstrap/bootstrap-environment.ts

@@ -1,10 +1,11 @@
+import { info } from 'console';
 import * as path from 'path';
 import * as cxapi from '@aws-cdk/cx-api';
 import { loadStructuredFile, toYAML } from '../../serialize';
 import { SdkProvider } from '../aws-auth';
 import { DeployStackResult } from '../deploy-stack';
 import { BootstrapEnvironmentOptions, BootstrappingParameters } from './bootstrap-props';
-import { deployBootstrapStack, bootstrapVersionFromTemplate } from './deploy-bootstrap';
+import { BootstrapStack, bootstrapVersionFromTemplate } from './deploy-bootstrap';
 import { legacyBootstrapTemplate } from './legacy-template';
 
 /* eslint-disable max-len */
@@ -53,12 +54,11 @@ export class Bootstrapper {
       throw new Error('--qualifier can only be passed for the new bootstrap experience.');
     }
 
-    return deployBootstrapStack(
-      await this.loadTemplate(params),
-      {},
-      environment,
-      sdkProvider,
-      options);
+    const current = await BootstrapStack.lookup(sdkProvider, environment, options.toolkitStackName);
+    return current.update(await this.loadTemplate(params), {}, {
+      ...options,
+      terminationProtection: options.terminationProtection ?? current.terminationProtection,
+    });
   }
 
   /**
@@ -73,25 +73,42 @@ export class Bootstrapper {
 
     const params = options.parameters ?? {};
 
-    if (params.trustedAccounts?.length && !params.cloudFormationExecutionPolicies?.length) {
-      throw new Error('--cloudformation-execution-policies are required if --trust has been passed!');
-    }
-
     const bootstrapTemplate = await this.loadTemplate();
 
-    return deployBootstrapStack(
+    const current = await BootstrapStack.lookup(sdkProvider, environment, options.toolkitStackName);
+
+    // If people re-bootstrap, existing parameter values are reused so that people don't accidentally change the configuration
+    // on their bootstrap stack (this happens automatically in deployStack). However, to do proper validation on the
+    // combined arguments (such that if --trust has been given, --cloudformation-execution-policies is necessary as well)
+    // we need to take this parameter reuse into account.
+    //
+    // Ideally we'd do this inside the template, but the `Rules` section of CFN
+    // templates doesn't seem to be able to express the conditions that we need
+    // (can't use Fn::Join or reference Conditions) so we do it here instead.
+    const trustedAccounts = params.trustedAccounts ?? current.parameters.TrustedAccounts?.split(',') ?? [];
+    const cloudFormationExecutionPolicies = params.cloudFormationExecutionPolicies ?? current.parameters.CloudFormationExecutionPolicies?.split(',') ?? [];
+
+    if (trustedAccounts.length > 0 && cloudFormationExecutionPolicies.length === 0) {
+      throw new Error(`You need to pass '--cloudformation-execution-policies' when trusting other accounts using '--trust' (${trustedAccounts}). Try a managed policy of the form 'arn:aws:iam::aws:policy/<PolicyName>'.`);
+    }
+    // Remind people what the current settings are
+    info(`Trusted accounts:   ${trustedAccounts.length > 0 ? trustedAccounts.join(', ') : '(none)'}`);
+    info(`Execution policies: ${cloudFormationExecutionPolicies.join(', ')}`);
+
+    return current.update(
       bootstrapTemplate,
       {
         FileAssetsBucketName: params.bucketName,
         FileAssetsBucketKmsKeyId: params.kmsKeyId,
-        TrustedAccounts: params.trustedAccounts?.join(','),
-        CloudFormationExecutionPolicies: params.cloudFormationExecutionPolicies?.join(','),
+        // Empty array becomes empty string
+        TrustedAccounts: trustedAccounts.join(','),
+        CloudFormationExecutionPolicies: cloudFormationExecutionPolicies.join(','),
         Qualifier: params.qualifier,
         PublicAccessBlockConfiguration: params.publicAccessBlockConfiguration || params.publicAccessBlockConfiguration === undefined ? 'true' : 'false',
-      },
-      environment,
-      sdkProvider,
-      options);
+      }, {
+        ...options,
+        terminationProtection: options.terminationProtection ?? current.terminationProtection,
+      });
   }
 
   private async customBootstrap(
@@ -119,5 +136,4 @@ export class Bootstrapper {
         return legacyBootstrapTemplate(params);
     }
   }
-
-}
+}

packages/aws-cdk/lib/api/bootstrap/deploy-bootstrap.ts

@@ -3,59 +3,104 @@ import * as path from 'path';
 import * as cxschema from '@aws-cdk/cloud-assembly-schema';
 import * as cxapi from '@aws-cdk/cx-api';
 import * as fs from 'fs-extra';
-import { Mode, SdkProvider } from '../aws-auth';
+import { Mode, SdkProvider, ISDK } from '../aws-auth';
 import { deployStack, DeployStackResult } from '../deploy-stack';
 import { DEFAULT_TOOLKIT_STACK_NAME, ToolkitInfo } from '../toolkit-info';
 import { BOOTSTRAP_VERSION_OUTPUT, BootstrapEnvironmentOptions, BOOTSTRAP_VERSION_RESOURCE } from './bootstrap-props';
 
 /**
- * Perform the actual deployment of a bootstrap stack, given a template and some parameters
+ * A class to hold state around stack bootstrapping
+ *
+ * This class exists so we can break bootstrapping into 2 phases:
+ *
+ * ```ts
+ * const current = BootstrapStack.lookup(...);
+ * // ...
+ * current.update(newTemplate, ...);
+ * ```
+ *
+ * And do something in between the two phases (such as look at the
+ * current bootstrap stack and doing something intelligent).
+ *
+ * This class is different from `ToolkitInfo` in that `ToolkitInfo`
+ * is purely read-only, and `ToolkitInfo.lookup()` returns `undefined`
+ * if the stack does not exist. But honestly, these classes could and
+ * should probably be merged at some point.
  */
-export async function deployBootstrapStack(
-  template: any,
-  parameters: Record<string, string | undefined>,
-  environment: cxapi.Environment,
-  sdkProvider: SdkProvider,
-  options: Omit<BootstrapEnvironmentOptions, 'parameters'>): Promise<DeployStackResult> {
+export class BootstrapStack {
+  public static async lookup(sdkProvider: SdkProvider, environment: cxapi.Environment, toolkitStackName?: string) {
+    toolkitStackName = toolkitStackName ?? DEFAULT_TOOLKIT_STACK_NAME;
 
-  const toolkitStackName = options.toolkitStackName ?? DEFAULT_TOOLKIT_STACK_NAME;
+    const resolvedEnvironment = await sdkProvider.resolveEnvironment(environment);
+    const sdk = await sdkProvider.forEnvironment(resolvedEnvironment, Mode.ForWriting);
+    const currentToolkitInfo = await ToolkitInfo.lookup(resolvedEnvironment, sdk, toolkitStackName);
 
-  const resolvedEnvironment = await sdkProvider.resolveEnvironment(environment);
-  const sdk = await sdkProvider.forEnvironment(resolvedEnvironment, Mode.ForWriting);
+    return new BootstrapStack(sdkProvider, sdk, resolvedEnvironment, toolkitStackName, currentToolkitInfo);
+  }
+
+  protected constructor(
+    private readonly sdkProvider: SdkProvider,
+    private readonly sdk: ISDK,
+    private readonly resolvedEnvironment: cxapi.Environment,
+    private readonly toolkitStackName: string,
+    private readonly currentToolkitInfo?: ToolkitInfo) {
+  }
 
-  const newVersion = bootstrapVersionFromTemplate(template);
-  const currentBootstrapStack = await ToolkitInfo.lookup(resolvedEnvironment, sdk, toolkitStackName);
-  if (currentBootstrapStack && newVersion < currentBootstrapStack.version && !options.force) {
-    throw new Error(`Not downgrading existing bootstrap stack from version '${currentBootstrapStack.version}' to version '${newVersion}'. Use --force to force.`);
+  public get parameters(): Record<string, string> {
+    return this.currentToolkitInfo?.parameters ?? {};
   }
 
-  const outdir = await fs.mkdtemp(path.join(os.tmpdir(), 'cdk-bootstrap'));
-  const builder = new cxapi.CloudAssemblyBuilder(outdir);
-  const templateFile = `${toolkitStackName}.template.json`;
-  await fs.writeJson(path.join(builder.outdir, templateFile), template, { spaces: 2 });
+  public get terminationProtection() {
+    return this.currentToolkitInfo?.stack?.terminationProtection;
+  }
 
-  builder.addArtifact(toolkitStackName, {
-    type: cxschema.ArtifactType.AWS_CLOUDFORMATION_STACK,
-    environment: cxapi.EnvironmentUtils.format(environment.account, environment.region),
-    properties: {
-      templateFile,
-      terminationProtection: options.terminationProtection ?? false,
-    },
-  });
+  public async partition(): Promise<string> {
+    return (await this.sdk.currentAccount()).partition;
+  }
 
-  const assembly = builder.buildAssembly();
+  /**
+   * Perform the actual deployment of a bootstrap stack, given a template and some parameters
+   */
+  public async update(
+    template: any,
+    parameters: Record<string, string | undefined>,
+    options: Omit<BootstrapEnvironmentOptions, 'parameters'>,
+  ): Promise<DeployStackResult> {
 
-  return await deployStack({
-    stack: assembly.getStackByName(toolkitStackName),
-    resolvedEnvironment,
-    sdk: await sdkProvider.forEnvironment(resolvedEnvironment, Mode.ForWriting),
-    sdkProvider,
-    force: options.force,
-    roleArn: options.roleArn,
-    tags: options.tags,
-    execute: options.execute,
-    parameters,
-  });
+    const newVersion = bootstrapVersionFromTemplate(template);
+    if (this.currentToolkitInfo && newVersion < this.currentToolkitInfo.version && !options.force) {
+      throw new Error(`Not downgrading existing bootstrap stack from version '${this.currentToolkitInfo.version}' to version '${newVersion}'. Use --force to force.`);
+    }
+
+    const outdir = await fs.mkdtemp(path.join(os.tmpdir(), 'cdk-bootstrap'));
+    const builder = new cxapi.CloudAssemblyBuilder(outdir);
+    const templateFile = `${this.toolkitStackName}.template.json`;
+    await fs.writeJson(path.join(builder.outdir, templateFile), template, { spaces: 2 });
+
+    builder.addArtifact(this.toolkitStackName, {
+      type: cxschema.ArtifactType.AWS_CLOUDFORMATION_STACK,
+      environment: cxapi.EnvironmentUtils.format(this.resolvedEnvironment.account, this.resolvedEnvironment.region),
+      properties: {
+        templateFile,
+        terminationProtection: options.terminationProtection ?? false,
+      },
+    });
+
+    const assembly = builder.buildAssembly();
+
+    return deployStack({
+      stack: assembly.getStackByName(this.toolkitStackName),
+      resolvedEnvironment: this.resolvedEnvironment,
+      sdk: this.sdk,
+      sdkProvider: this.sdkProvider,
+      force: options.force,
+      roleArn: options.roleArn,
+      tags: options.tags,
+      execute: options.execute,
+      parameters,
+      usePreviousParameters: true,
+    });
+  }
 }
 
 export function bootstrapVersionFromTemplate(template: any): number {

packages/aws-cdk/lib/api/deploy-stack.ts

@@ -149,7 +149,7 @@ export interface DeployStackOptions {
    *
    * If not set, all parameters must be specified for every deployment.
    *
-   * @default true
+   * @default false
    */
   usePreviousParameters?: boolean;