From 140fb77f965b39e94d7c1d446332d4f4b36c186a Mon Sep 17 00:00:00 2001 From: Momo Kornher Date: Fri, 27 Mar 2026 18:52:33 +0000 Subject: [PATCH] fix(deploy): use real CloudFormation change set for approval diff Implement two-phase deploy for `--method=change-set`: always create the change set upfront, use it for the approval diff, then execute the same change set. This avoids double change set creation and provides accurate resource replacement information in the approval prompt. Add `PrepareStackOptions` interface with `cleanupOnNoOp` flag to control empty change set cleanup. When the orchestration layer forces `execute: false` internally, empty change sets are cleaned up. When the user explicitly asks for `--no-execute`, they are left for inspection. Add `ExecuteChangeSetDeployment` internal deployment method to execute a pre-created change set without creating a new one. On retries after rollback, fall back to the original `change-set` method since the prepared change set is gone. Add `beforeInput` async callback to integ test `UserInteraction` interface for verifying external state while a process is paused at a prompt. --- .../@aws-cdk-testing/cli-integ/lib/shell.ts | 31 +- ...ny-change-approval-shows-diff.integtest.ts | 2 - ...with-change-set-approval-diff.integtest.ts | 53 +++ .../deploy/private/deployment-method.ts | 53 +++ .../lib/actions/deploy/private/index.ts | 1 + .../lib/api/deployments/deploy-stack.ts | 47 ++- .../lib/api/deployments/deployment-result.ts | 7 + .../lib/api/deployments/deployments.ts | 82 +++- .../toolkit-lib/lib/toolkit/toolkit.ts | 99 +++-- .../test/actions/deploy-hotswap.test.ts | 1 + .../toolkit-lib/test/actions/deploy.test.ts | 92 +++++ .../test/actions/deployment-method.test.ts | 68 ++++ .../cloudformation-deployments.test.ts | 46 +++ .../test/api/deployments/deploy-stack.test.ts | 153 ++++++++ packages/aws-cdk/lib/api/deploy-private.ts | 2 + packages/aws-cdk/lib/cli/cdk-toolkit.ts | 130 ++++--- packages/aws-cdk/test/cli/cdk-toolkit.test.ts | 361 ++++++++++++++++-- 17 files changed, 1102 insertions(+), 126 deletions(-) create mode 100644 packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy-with-change-set-approval-diff.integtest.ts create mode 100644 packages/@aws-cdk/toolkit-lib/lib/actions/deploy/private/deployment-method.ts create mode 100644 packages/@aws-cdk/toolkit-lib/test/actions/deployment-method.test.ts create mode 100644 packages/aws-cdk/lib/api/deploy-private.ts diff --git a/packages/@aws-cdk-testing/cli-integ/lib/shell.ts b/packages/@aws-cdk-testing/cli-integ/lib/shell.ts index 5ec61d5b8..436ee7633 100644 --- a/packages/@aws-cdk-testing/cli-integ/lib/shell.ts +++ b/packages/@aws-cdk-testing/cli-integ/lib/shell.ts @@ -68,9 +68,22 @@ export async function shell(command: string[], options: ShellOptions = {}): Prom // now write the input with a slight delay to ensure // the child process has already started reading. - setTimeout(() => { + const sendInput = () => { child.writeStdin(interaction.input + (interaction.end ?? os.EOL)); - }, 500); + }; + + if (interaction.beforeInput) { + void interaction.beforeInput() + .catch((err) => { + writeToOutputs(`\n[Prompt: ${interaction.prompt.toString()}] beforeInput hook failed!\n`); + writeToOutputs(`${err}\n\n`); + }) + .finally(() => { + setTimeout(sendInput, 500); + }); + } else { + setTimeout(sendInput, 500); + } } } }); @@ -100,7 +113,7 @@ export async function shell(command: string[], options: ShellOptions = {}): Prom const stdoutOutput = Buffer.concat(stdout).toString('utf-8'); const out = (options.onlyStderr ? stderrOutput : stdoutOutput + stderrOutput).trim(); - const logAndreject = (error: Error) => { + const logAndReject = (error: Error) => { if (show === 'error') { writeToOutputs(`${out}\n`); } @@ -109,15 +122,15 @@ export async function shell(command: string[], options: ShellOptions = {}): Prom if (remainingInteractions.length !== 0) { // regardless of the exit code, if we didn't consume all expected interactions we probably - // did somethiing wrong. - logAndreject(new Error(`Expected more user interactions but subprocess exited with ${code}`)); + // did something wrong. + logAndReject(new Error(`Expected more user interactions but subprocess exited with ${code}`)); return; } if (code === 0 || options.allowErrExit) { resolve(out); } else { - logAndreject(new Error(`'${command.join(' ')}' exited with error code ${code}.`)); + logAndReject(new Error(`'${command.join(' ')}' exited with error code ${code}.`)); } }); }); @@ -168,6 +181,12 @@ export interface UserInteraction { * @default os.EOL */ readonly end?: string; + + /** + * An async callback to run after the prompt is matched but before the input is sent. + * Useful for verifying external state while the process is paused at a prompt. + */ + readonly beforeInput?: () => Promise; } export interface ShellOptions extends child_process.SpawnOptions { diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy-with-any-change-approval-shows-diff.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy-with-any-change-approval-shows-diff.integtest.ts index 7a22f5ea0..ea6c1507b 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy-with-any-change-approval-shows-diff.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy-with-any-change-approval-shows-diff.integtest.ts @@ -1,7 +1,5 @@ import { integTest, withDefaultFixture } from '../../../lib'; -jest.setTimeout(2 * 60 * 60_000); // Includes the time to acquire locks, worst-case single-threaded runtime - integTest( 'deploy with any-change approval shows diff', withDefaultFixture(async (fixture) => { diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy-with-change-set-approval-diff.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy-with-change-set-approval-diff.integtest.ts new file mode 100644 index 000000000..db5b0a7e1 --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/deploy/cdk-deploy-with-change-set-approval-diff.integtest.ts @@ -0,0 +1,53 @@ +import { DescribeStacksCommand, ListChangeSetsCommand } from '@aws-sdk/client-cloudformation'; +import { integTest, withDefaultFixture } from '../../../lib'; + +integTest( + 'deploy with change-set method uses change set for approval diff', + withDefaultFixture(async (fixture) => { + let changeSetVerified = false; + const stackName = fixture.fullStackName('test-2'); + const changeSetName = `${fixture.stackNamePrefix}-approval-diff-test`; + + // Deploy with --require-approval=any-change without --yes. + // The CLI will create a change set for the approval diff, pause for confirmation, + // and then execute the same change set after the user confirms. + const output = await fixture.cdkDeploy('test-2', { + options: ['--require-approval=any-change', '--method=change-set', `--change-set-name=${changeSetName}`], + neverRequireApproval: false, + interact: [ + { + prompt: /Do you wish to deploy these changes/, + input: 'y', + beforeInput: async () => { + // While the CLI is paused at the approval prompt, verify that + // the named change set has been created and is ready for execution. + const response = await fixture.aws.cloudFormation.send( + new ListChangeSetsCommand({ StackName: stackName }), + ); + const changeSets = response.Summaries ?? []; + const namedChangeSet = changeSets.find(cs => cs.ChangeSetName === changeSetName); + expect(namedChangeSet).toBeDefined(); + expect(namedChangeSet?.Status).toEqual('CREATE_COMPLETE'); + changeSetVerified = true; + }, + }, + ], + modEnv: { + FORCE_COLOR: '0', + }, + }); + + // The approval diff should contain resource information from the change set + expect(output).toContain('AWS::SNS::Topic'); + expect(output).toContain('Do you wish to deploy these changes'); + + // Verify the beforeInput callback actually ran + expect(changeSetVerified).toBe(true); + + // Verify the stack was actually deployed + const response = await fixture.aws.cloudFormation.send( + new DescribeStacksCommand({ StackName: fixture.fullStackName('test-2') }), + ); + expect(response.Stacks?.[0].StackStatus).toEqual('CREATE_COMPLETE'); + }), +); diff --git a/packages/@aws-cdk/toolkit-lib/lib/actions/deploy/private/deployment-method.ts b/packages/@aws-cdk/toolkit-lib/lib/actions/deploy/private/deployment-method.ts new file mode 100644 index 000000000..a7141b796 --- /dev/null +++ b/packages/@aws-cdk/toolkit-lib/lib/actions/deploy/private/deployment-method.ts @@ -0,0 +1,53 @@ +import type { ChangeSetDeployment, DeploymentMethod } from '..'; + +export const DEFAULT_DEPLOY_CHANGE_SET_NAME = 'cdk-deploy-change-set'; + +/** + * Execute a previously created change set. + * This is an internal deployment method used by the two-phase deploy flow. + */ +export interface ExecuteChangeSetDeployment { + readonly method: 'execute-change-set'; + readonly changeSetName: string; +} + +/** + * A change set deployment that will execute. + */ +export type ExecutingChangeSetDeployment = ChangeSetDeployment & { execute: true }; + +/** + * A change set deployment that will not execute. + */ +export type NonExecutingChangeSetDeployment = ChangeSetDeployment & { execute: false }; + +/** + * Returns true if the deployment method is a change-set deployment. + */ +export function isChangeSetDeployment(method?: DeploymentMethod): method is ChangeSetDeployment { + return method?.method === 'change-set'; +} + +/** + * Returns true if the deployment method is a change-set deployment that will execute. + */ +export function isExecutingChangeSetDeployment(method?: DeploymentMethod): method is ExecutingChangeSetDeployment { + return isChangeSetDeployment(method) && (method.execute ?? true); +} + +/** + * Returns true if the deployment method is a change-set deployment that will not execute. + */ +export function isNonExecutingChangeSetDeployment(method?: DeploymentMethod): method is NonExecutingChangeSetDeployment { + return isChangeSetDeployment(method) && (method.execute === false); +} + +/** + * Create an ExecuteChangeSetDeployment from a ChangeSetDeployment. + */ +export function toExecuteChangeSetDeployment(method: ChangeSetDeployment): ExecuteChangeSetDeployment { + return { + method: 'execute-change-set', + changeSetName: method.changeSetName ?? DEFAULT_DEPLOY_CHANGE_SET_NAME, + }; +} diff --git a/packages/@aws-cdk/toolkit-lib/lib/actions/deploy/private/index.ts b/packages/@aws-cdk/toolkit-lib/lib/actions/deploy/private/index.ts index 688292d45..e99fb9bfd 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/actions/deploy/private/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/actions/deploy/private/index.ts @@ -1,3 +1,4 @@ export * from './deploy-options'; +export * from './deployment-method'; export * from './helpers'; 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 7e75b30c1..e74018d1f 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 @@ -27,6 +27,7 @@ import { import { determineAllowCrossAccountAssetPublishing } from './checks'; import type { DeployStackResult, SuccessfulDeployStackResult } from './deployment-result'; import type { ChangeSetDeployment, DeploymentMethod, DirectDeployment } from '../../actions/deploy'; +import { DEFAULT_DEPLOY_CHANGE_SET_NAME } from '../../actions/deploy/private/deployment-method'; import { DeploymentError, DeploymentErrorCodes, ToolkitError } from '../../toolkit/toolkit-error'; import { formatErrorMessage } from '../../util'; import type { SDK, SdkProvider, ICloudFormationClient } from '../aws-auth/private'; @@ -40,6 +41,7 @@ import type { IoHelper } from '../io/private'; import type { ResourcesToImport } from '../resource-import'; import { StackActivityMonitor } from '../stack-events'; import { EarlyValidationReporter } from './early-validation'; +import type { ExecuteChangeSetDeployment } from '../../actions/deploy/private/deployment-method'; export interface DeployStackOptions { /** @@ -125,7 +127,7 @@ export interface DeployStackOptions { * * @default - Change set with defaults */ - readonly deploymentMethod?: DeploymentMethod; + readonly deploymentMethod?: DeploymentMethod | ExecuteChangeSetDeployment; /** * The collection of extra parameters @@ -357,7 +359,7 @@ class FullCloudFormationDeployment { private readonly uuid: string; constructor( - private readonly deploymentMethod: DirectDeployment | ChangeSetDeployment, + private readonly deploymentMethod: DirectDeployment | ChangeSetDeployment | ExecuteChangeSetDeployment, private readonly options: DeployStackOptions, private readonly cloudFormationStack: CloudFormationStack, private readonly stackArtifact: cxapi.CloudFormationStackArtifact, @@ -384,13 +386,16 @@ class FullCloudFormationDeployment { case 'change-set': return this.changeSetDeployment(deploymentMethod); + case 'execute-change-set': + return this.executeExistingChangeSet(deploymentMethod); + case 'direct': return this.directDeployment(); } } private async changeSetDeployment(deploymentMethod: ChangeSetDeployment): Promise { - const changeSetName = deploymentMethod.changeSetName ?? 'cdk-deploy-change-set'; + const changeSetName = deploymentMethod.changeSetName ?? DEFAULT_DEPLOY_CHANGE_SET_NAME; const execute = deploymentMethod.execute ?? true; const importExistingResources = deploymentMethod.importExistingResources ?? false; const revertDrift = deploymentMethod.revertDrift ?? false; @@ -437,10 +442,40 @@ class FullCloudFormationDeployment { noOp: false, outputs: this.cloudFormationStack.outputs, stackArn: changeSetDescription.StackId!, + changeSet: changeSetDescription, }; } // If there are replacements in the changeset, check the rollback flag and stack status + return this.checkAndExecuteChangeSet(changeSetDescription); + } + + private async executeExistingChangeSet(deploymentMethod: ExecuteChangeSetDeployment): Promise { + await this.updateTerminationProtection(); + + // The change set was already created and validated during the prepare phase, + // just describe it to get the info needed for execution. + const changeSetDescription = await this.cfn.describeChangeSet({ + StackName: this.stackName, + ChangeSetName: deploymentMethod.changeSetName, + }); + + return this.checkAndExecuteChangeSet(changeSetDescription); + } + + /** + * Check rollback/replacement constraints and execute the change set if all checks pass. + */ + private async checkAndExecuteChangeSet(changeSetDescription: DescribeChangeSetCommandOutput): Promise { + if (changeSetDescription.Status !== 'CREATE_COMPLETE') { + const status = changeSetDescription.Status ?? 'UNKNOWN'; + const reason = changeSetDescription.StatusReason ? `: ${changeSetDescription.StatusReason}` : ''; + throw new ToolkitError( + 'ChangeSetNotReady', + `Change set '${changeSetDescription.ChangeSetName}' on stack '${this.stackName}' is not ready for execution (status: ${status}${reason})`, + ); + } + const replacement = hasReplacement(changeSetDescription); const isPausedFailState = this.cloudFormationStack.stackStatus.isRollbackable; const rollback = this.options.rollback ?? true; @@ -739,6 +774,12 @@ async function canSkipDeploy( return false; } + // Executing an existing change set, never skip + if (deployStackOptions.deploymentMethod?.method === 'execute-change-set') { + await ioHelper.defaults.debug(`${deployName}: executing existing change set, never skip`); + return false; + } + // Drift-aware if ( deployStackOptions.deploymentMethod?.method === 'change-set' && diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deployment-result.ts b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deployment-result.ts index db21414b8..5fd0fd42b 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deployment-result.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deployment-result.ts @@ -1,3 +1,4 @@ +import type { DescribeChangeSetCommandOutput } from '@aws-sdk/client-cloudformation'; import { ToolkitError } from '../../toolkit/toolkit-error'; export type DeployStackResult = @@ -12,6 +13,12 @@ export interface SuccessfulDeployStackResult { readonly noOp: boolean; readonly outputs: { [name: string]: string }; readonly stackArn: string; + + /** + * The change set that was created during deployment, if any. + * Populated when using `change-set` deployment method with `execute: false`. + */ + readonly changeSet?: DescribeChangeSetCommandOutput; } /** The stack is currently in a failpaused state, and needs to be rolled back before the deployment */ 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 5666f9599..3b7a7422e 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deployments.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deployments.ts @@ -10,12 +10,15 @@ import { import { stabilizeStack, uploadStackTemplateAssets, + waitForStackDelete, } from './cfn-api'; import { determineAllowCrossAccountAssetPublishing } from './checks'; import { deployStack, destroyStack } from './deploy-stack'; -import type { DeployStackResult } from './deployment-result'; -import type { DeploymentMethod } from '../../actions/deploy'; +import type { DeployStackResult, SuccessfulDeployStackResult } from './deployment-result'; +import type { ChangeSetDeployment, DeploymentMethod } from '../../actions/deploy'; +import { DEFAULT_DEPLOY_CHANGE_SET_NAME } from '../../actions/deploy/private/deployment-method'; +import type { ExecuteChangeSetDeployment } from '../../actions/deploy/private/deployment-method'; import { DeploymentError, ToolkitError } from '../../toolkit/toolkit-error'; import { formatErrorMessage } from '../../util'; import type { SdkProvider } from '../aws-auth/private'; @@ -89,7 +92,7 @@ export interface DeployStackOptions { * * @default - Change set with default options */ - readonly deploymentMethod?: DeploymentMethod; + readonly deploymentMethod?: DeploymentMethod | ExecuteChangeSetDeployment; /** * Force deployment, even if the deployed template is identical to the one we are about to deploy. @@ -146,6 +149,24 @@ export interface DeployStackOptions { readonly assetParallelism?: boolean; } +export interface PrepareStackOptions extends Omit { + /** + * The change-set deployment method to use. + */ + readonly deploymentMethod: ChangeSetDeployment; + + /** + * Whether to clean up the change set if it has no changes. + * + * Set to true when the caller forced execute: false internally + * (two-phase deploy). Set to false when the user explicitly + * asked for --no-execute (prepare-change-set). + * + * @default false + */ + readonly cleanupOnNoOp?: boolean; +} + export interface RollbackStackOptions { /** * Stack to roll back @@ -389,6 +410,61 @@ export class Deployments { }, this.ioHelper); } + /** + * Create a change set for a stack without executing it. + * + * Returns the result if the change set was successfully created, or undefined + * if the prepare returned a non-success result (e.g. rollback needed). + */ + public async prepareStack( + options: PrepareStackOptions, + ): Promise { + const result = await this.deployStack({ + ...options, + deploymentMethod: { ...options.deploymentMethod, execute: false }, + }); + + // With execute: false, the only possible result type is did-deploy-stack + // (either noOp for empty change sets, or with a changeSet description). + // Rollback/replacement checks are only reached when executing. + if (result.type !== 'did-deploy-stack') { + return undefined; + } + + // Clean up empty change sets if requested (i.e. when the caller forced + // execute: false internally, not when the user explicitly asked for --no-execute). + if (result.noOp && options.cleanupOnNoOp) { + const changeSetName = options.deploymentMethod.changeSetName ?? DEFAULT_DEPLOY_CHANGE_SET_NAME; + await this.cleanupChangeSet(options.stack, changeSetName); + } + + return result; + } + + /** + * Clean up a change set that was created by prepareStack but never executed. + * If the stack was created in REVIEW_IN_PROGRESS state (new stack), delete the stack too. + */ + public async cleanupChangeSet(stack: cxapi.CloudFormationStackArtifact, changeSetName: string): Promise { + const env = await this.envs.accessStackForMutableStackOperations(stack); + const cfn = env.sdk.cloudFormation(); + const deployName = stack.stackName; + + const cloudFormationStack = await CloudFormationStack.lookup(cfn, deployName); + if (!cloudFormationStack.exists) { + return; + } + + await cfn.deleteChangeSet({ StackName: deployName, ChangeSetName: changeSetName }); + + // If the stack was newly created for this change set, it will be in REVIEW_IN_PROGRESS. + // Delete it and wait for the deletion to complete so we don't leave an empty stack behind. + if (cloudFormationStack.stackStatus.name === 'REVIEW_IN_PROGRESS') { + await cfn.deleteStack({ StackName: deployName }); + await waitForStackDelete(cfn, this.ioHelper, deployName); + } + } + public async rollbackStack(options: RollbackStackOptions): Promise { let resourcesToSkip: string[] = options.orphanLogicalIds ?? []; if (options.orphanFailedResources && resourcesToSkip.length > 0) { diff --git a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts index 02e99087f..821cf6516 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts @@ -38,8 +38,12 @@ import { BootstrapSource } from '../actions/bootstrap'; import { AssetBuildTime, type DeployOptions } from '../actions/deploy'; import { buildParameterMap, + isChangeSetDeployment, + isExecutingChangeSetDeployment, + isNonExecutingChangeSetDeployment, type PrivateDeployOptions, removePublishedAssetsFromWorkGraph, + toExecuteChangeSetDeployment, } from '../actions/deploy/private'; import { type DestroyOptions } from '../actions/destroy'; import type { DiffOptions } from '../actions/diff'; @@ -667,10 +671,60 @@ export class Toolkit extends CloudAssemblySourceBuilder { const currentTemplate = await deployments.readCurrentTemplate(stack); + // Following are the same semantics we apply with respect to Notification ARNs (dictated by the SDK) + // + // - undefined => cdk ignores it, as if it wasn't supported (allows external management). + // - []: => cdk manages it, and the user wants to wipe it out. + // - ['arn-1'] => cdk manages it, and the user wants to set it to ['arn-1']. + const notificationArns = (!!options.notificationArns || !!stack.notificationArns) + ? (options.notificationArns ?? []).concat(stack.notificationArns ?? []) + : undefined; + + for (const notificationArn of notificationArns ?? []) { + if (!validateSnsTopicArn(notificationArn)) { + throw new ToolkitError('InvalidSnsTopicArn', `Notification arn ${notificationArn} is not a valid arn for an SNS topic`); + } + } + + // Deploy options that are shared between change set creation and execution + const sharedDeployOptions = { + stack, + deployName: stack.stackName, + roleArn: options.roleArn, + toolkitStackName: this.toolkitStackName, + reuseAssets: options.reuseAssets, + tags: options.tags?.length ? options.tags : tagsForStack(stack), + forceDeployment: options.forceDeployment, + parameters: Object.assign({}, parameterMap['*'], parameterMap[stack.stackName]), + usePreviousParameters: options.parameters?.keepExistingParameters, + rollback: options.rollback, + notificationArns, + extraUserAgent: options.extraUserAgent, + assetParallelism: options.assetParallelism, + }; + + // When using change-set method, always create the change set upfront. + // This gives us an accurate diff for approval and avoids creating it twice. + // For non-executing deployments (prepare-change-set), this is the final result. + const prepareResult = isChangeSetDeployment(options.deploymentMethod) + ? await deployments.prepareStack({ + ...sharedDeployOptions, + deploymentMethod: options.deploymentMethod, + cleanupOnNoOp: isExecutingChangeSetDeployment(options.deploymentMethod), + }) + : undefined; + + // Empty change set — no changes to deploy + if (prepareResult?.noOp === true) { + await ioHelper.notify(IO.CDK_TOOLKIT_I5900.msg(chalk.green(`\n ✅ ${stack.displayName} (no changes)`), prepareResult)); + return; + } + const formatter = new DiffFormatter({ templateInfo: { oldTemplate: currentTemplate, newTemplate: stack, + changeSet: prepareResult?.changeSet, }, }); @@ -693,22 +747,10 @@ export class Toolkit extends CloudAssemblySourceBuilder { templateDiffs: formatter.diffs, })); if (!deployConfirmed) { - throw new ToolkitError('DeployAborted', 'Aborted by user'); - } - - // Following are the same semantics we apply with respect to Notification ARNs (dictated by the SDK) - // - // - undefined => cdk ignores it, as if it wasn't supported (allows external management). - // - []: => cdk manages it, and the user wants to wipe it out. - // - ['arn-1'] => cdk manages it, and the user wants to set it to ['arn-1']. - const notificationArns = (!!options.notificationArns || !!stack.notificationArns) - ? (options.notificationArns ?? []).concat(stack.notificationArns ?? []) - : undefined; - - for (const notificationArn of notificationArns ?? []) { - if (!validateSnsTopicArn(notificationArn)) { - throw new ToolkitError('InvalidSnsTopicArn', `Notification arn ${notificationArn} is not a valid arn for an SNS topic`); + if (prepareResult?.changeSet?.ChangeSetName) { + await deployments.cleanupChangeSet(stack, prepareResult.changeSet.ChangeSetName); } + throw new ToolkitError('DeployAborted', 'Aborted by user'); } const stackIndex = stacks.indexOf(stack) + 1; @@ -720,14 +762,10 @@ export class Toolkit extends CloudAssemblySourceBuilder { }); deploySpan.incCounter('resources', resourceCount); - let tags = options.tags; - if (!tags || tags.length === 0) { - tags = tagsForStack(stack); - } - let deployDuration; try { - let deployResult: SuccessfulDeployStackResult | undefined; + const prepareIsFinal = isNonExecutingChangeSetDeployment(options.deploymentMethod); + let deployResult: SuccessfulDeployStackResult | undefined = prepareIsFinal ? prepareResult : undefined; let rollback = options.rollback; let iteration = 0; @@ -737,20 +775,13 @@ export class Toolkit extends CloudAssemblySourceBuilder { } const r = await deployments.deployStack({ - stack, - deployName: stack.stackName, - roleArn: options.roleArn, - toolkitStackName: this.toolkitStackName, - reuseAssets: options.reuseAssets, - notificationArns, - tags, - deploymentMethod: options.deploymentMethod, - forceDeployment: options.forceDeployment, - parameters: Object.assign({}, parameterMap['*'], parameterMap[stack.stackName]), - usePreviousParameters: options.parameters?.keepExistingParameters, + ...sharedDeployOptions, + // On the first iteration, execute the prepared change set. + // On retries (after rollback), create a new change set since the old one is gone. + deploymentMethod: iteration === 1 && isExecutingChangeSetDeployment(options.deploymentMethod) + ? toExecuteChangeSetDeployment(options.deploymentMethod) + : options.deploymentMethod, rollback, - extraUserAgent: options.extraUserAgent, - assetParallelism: options.assetParallelism, }); switch (r.type) { diff --git a/packages/@aws-cdk/toolkit-lib/test/actions/deploy-hotswap.test.ts b/packages/@aws-cdk/toolkit-lib/test/actions/deploy-hotswap.test.ts index 243b27a4e..03942be99 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/deploy-hotswap.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/deploy-hotswap.test.ts @@ -16,6 +16,7 @@ jest.mock('../../lib/api/deployments', () => { ...jest.requireActual('../../lib/api/deployments'), Deployments: jest.fn().mockImplementation(() => ({ deployStack: mockDeployStack, + prepareStack: jest.fn().mockResolvedValue(undefined), resolveEnvironment: jest.fn().mockResolvedValue({}), isSingleAssetPublished: jest.fn().mockResolvedValue(true), readCurrentTemplate: jest.fn().mockResolvedValue({ Resources: {} }), 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 537c11584..3cadde085 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/deploy.test.ts @@ -1,4 +1,5 @@ import { StackParameters } from '../../lib/actions/deploy'; +import { DEFAULT_DEPLOY_CHANGE_SET_NAME } from '../../lib/actions/deploy/private'; import type { DeployStackOptions, DeployStackResult } from '../../lib/api/deployments'; import * as deployments from '../../lib/api/deployments'; import { WorkGraphBuilder } from '../../lib/api/work-graph'; @@ -123,6 +124,97 @@ IAM Statement Changes })); }); + describe('two-phase deploy with change-set approval', () => { + test('calls deployStack with execute:false then execute-change-set', async () => { + // GIVEN + jest.spyOn(deployments.Deployments.prototype, 'prepareStack').mockResolvedValueOnce({ + type: 'did-deploy-stack', + noOp: false, + outputs: {}, + stackArn: 'arn:aws:cloudformation:region:account:stack/test-stack', + changeSet: { Status: 'CREATE_COMPLETE', Changes: [{ Type: 'Resource' }], ChangeSetName: DEFAULT_DEPLOY_CHANGE_SET_NAME, $metadata: {} } as any, + }); + mockDeployStack + .mockReset() + .mockResolvedValueOnce({ + type: 'did-deploy-stack', + noOp: false, + outputs: {}, + stackArn: 'arn:aws:cloudformation:region:account:stack/test-stack', + }); + + // WHEN + const cx = await builderFixture(toolkit, 'stack-with-bucket'); + await toolkit.deploy(cx, { + deploymentMethod: { method: 'change-set' }, + }); + + // THEN + expect(mockDeployStack).toHaveBeenCalledTimes(1); + expect(mockDeployStack).toHaveBeenCalledWith( + expect.objectContaining({ + deploymentMethod: expect.objectContaining({ method: 'execute-change-set' }), + }), + ); + }); + + test('uses template-only diff with method=direct', async () => { + // WHEN + const cx = await builderFixture(toolkit, 'stack-with-bucket'); + await toolkit.deploy(cx, { + deploymentMethod: { method: 'direct' }, + }); + + // THEN — only one deployStack call with direct method + expect(mockDeployStack).toHaveBeenCalledTimes(1); + expect(mockDeployStack).toHaveBeenCalledWith( + expect.objectContaining({ + deploymentMethod: { method: 'direct' }, + }), + ); + }); + + test('skips execute when prepare returns noOp', async () => { + // GIVEN + jest.spyOn(deployments.Deployments.prototype, 'prepareStack').mockResolvedValueOnce({ + type: 'did-deploy-stack', + noOp: true, + outputs: {}, + stackArn: 'arn:aws:cloudformation:region:account:stack/test-stack', + }); + jest.spyOn(deployments.Deployments.prototype, 'cleanupChangeSet').mockResolvedValue(); + + // WHEN + const cx = await builderFixture(toolkit, 'stack-with-bucket'); + await toolkit.deploy(cx, { + deploymentMethod: { method: 'change-set' }, + }); + + // THEN — no deployStack call, prepare was final + expect(mockDeployStack).toHaveBeenCalledTimes(0); + }); + + test('non-executing change-set skips deploy loop', async () => { + // GIVEN + jest.spyOn(deployments.Deployments.prototype, 'prepareStack').mockResolvedValueOnce({ + type: 'did-deploy-stack', + noOp: false, + outputs: {}, + stackArn: 'arn:aws:cloudformation:region:account:stack/test-stack', + changeSet: { Status: 'CREATE_COMPLETE', Changes: [{ Type: 'Resource' }], ChangeSetName: 'cdk-deploy-change-set', $metadata: {} } as any, + }); + + // WHEN + const cx = await builderFixture(toolkit, 'stack-with-bucket'); + await toolkit.deploy(cx, { + deploymentMethod: { method: 'change-set', execute: false }, + }); + + // THEN — no deployStack call, prepare was final + expect(mockDeployStack).toHaveBeenCalledTimes(0); + }); + }); + describe('deployment options', () => { test('parameters are passed in', async () => { // WHEN diff --git a/packages/@aws-cdk/toolkit-lib/test/actions/deployment-method.test.ts b/packages/@aws-cdk/toolkit-lib/test/actions/deployment-method.test.ts new file mode 100644 index 000000000..cb6e4676c --- /dev/null +++ b/packages/@aws-cdk/toolkit-lib/test/actions/deployment-method.test.ts @@ -0,0 +1,68 @@ +import { + isChangeSetDeployment, + isExecutingChangeSetDeployment, + isNonExecutingChangeSetDeployment, + toExecuteChangeSetDeployment, +} from '../../lib/actions/deploy/private/deployment-method'; + +describe('isChangeSetDeployment', () => { + test('true for change-set method', () => { + expect(isChangeSetDeployment({ method: 'change-set' })).toBe(true); + }); + + test('false for direct method', () => { + expect(isChangeSetDeployment({ method: 'direct' })).toBe(false); + }); + + test('false for undefined', () => { + expect(isChangeSetDeployment(undefined)).toBe(false); + }); +}); + +describe('isExecutingChangeSetDeployment', () => { + test('true when execute is undefined (defaults to true)', () => { + expect(isExecutingChangeSetDeployment({ method: 'change-set' })).toBe(true); + }); + + test('true when execute is true', () => { + expect(isExecutingChangeSetDeployment({ method: 'change-set', execute: true })).toBe(true); + }); + + test('false when execute is false', () => { + expect(isExecutingChangeSetDeployment({ method: 'change-set', execute: false })).toBe(false); + }); + + test('false for direct method', () => { + expect(isExecutingChangeSetDeployment({ method: 'direct' })).toBe(false); + }); +}); + +describe('isNonExecutingChangeSetDeployment', () => { + test('true when execute is false', () => { + expect(isNonExecutingChangeSetDeployment({ method: 'change-set', execute: false })).toBe(true); + }); + + test('false when execute is undefined', () => { + expect(isNonExecutingChangeSetDeployment({ method: 'change-set' })).toBe(false); + }); + + test('false when execute is true', () => { + expect(isNonExecutingChangeSetDeployment({ method: 'change-set', execute: true })).toBe(false); + }); + + test('false for direct method', () => { + expect(isNonExecutingChangeSetDeployment({ method: 'direct' })).toBe(false); + }); +}); + +describe('toExecuteChangeSetDeployment', () => { + test('uses provided changeSetName', () => { + const result = toExecuteChangeSetDeployment({ method: 'change-set', changeSetName: 'my-cs' }); + expect(result).toEqual({ method: 'execute-change-set', changeSetName: 'my-cs' }); + }); + + test('defaults changeSetName to cdk-deploy-change-set', () => { + const result = toExecuteChangeSetDeployment({ method: 'change-set' }); + expect(result).toEqual({ method: 'execute-change-set', changeSetName: 'cdk-deploy-change-set' }); + }); +}); diff --git a/packages/@aws-cdk/toolkit-lib/test/api/deployments/cloudformation-deployments.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/deployments/cloudformation-deployments.test.ts index 7b3de279e..12cb2814f 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/deployments/cloudformation-deployments.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/deployments/cloudformation-deployments.test.ts @@ -93,6 +93,52 @@ test('passes through deploymentMethod with hotswap to deployStack()', async () = ); }); +test('prepareStack calls deployStack with execute: false and returns successful result', async () => { + // GIVEN + (deployStack as jest.Mock).mockResolvedValue({ + type: 'did-deploy-stack', + noOp: false, + outputs: {}, + stackArn: 'arn:stack', + changeSet: { Status: 'CREATE_COMPLETE' }, + }); + + // WHEN + const result = await deployments.prepareStack({ + stack: testStack({ stackName: 'boop' }), + deploymentMethod: { method: 'change-set', changeSetName: 'my-cs' }, + }); + + // THEN + expect(deployStack).toHaveBeenCalledWith( + expect.objectContaining({ + deploymentMethod: { method: 'change-set', changeSetName: 'my-cs', execute: false }, + }), + expect.anything(), + ); + expect(result).toEqual(expect.objectContaining({ + type: 'did-deploy-stack', + noOp: false, + changeSet: { Status: 'CREATE_COMPLETE' }, + })); +}); + +test('prepareStack returns undefined for non-success results', async () => { + // GIVEN + (deployStack as jest.Mock).mockResolvedValue({ + type: 'replacement-requires-rollback', + }); + + // WHEN + const result = await deployments.prepareStack({ + stack: testStack({ stackName: 'boop' }), + deploymentMethod: { method: 'change-set' }, + }); + + // THEN + expect(result).toBeUndefined(); +}); + test('placeholders are substituted in CloudFormation execution role', async () => { await deployments.deployStack({ stack: testStack({ 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 4439381ca..41310a2b4 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 @@ -1258,6 +1258,159 @@ test.each([ test('assertIsSuccessfulDeployStackResult does what it says', () => { expect(() => assertIsSuccessfulDeployStackResult({ type: 'replacement-requires-rollback' })).toThrow(); }); + +describe('execute-change-set deployment method', () => { + test('executes an existing change set without creating a new one', async () => { + // GIVEN + givenStackExists(); + mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ + Status: 'CREATE_COMPLETE', + ChangeSetName: 'my-change-set', + Changes: [{ Type: 'Resource' as const }], + }); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + deploymentMethod: { method: 'execute-change-set', changeSetName: 'my-change-set' }, + }); + + // THEN + expect(result.type).toEqual('did-deploy-stack'); + expect(mockCloudFormationClient).not.toHaveReceivedCommand(CreateChangeSetCommand); + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); + }); + + test('throws when change set is not in CREATE_COMPLETE status', async () => { + // GIVEN + givenStackExists(); + mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ + Status: 'FAILED', + StatusReason: 'Something went wrong', + ChangeSetName: 'my-change-set', + }); + + // THEN + await expect(testDeployStack({ + ...standardDeployStackArguments(), + deploymentMethod: { method: 'execute-change-set', changeSetName: 'my-change-set' }, + })).rejects.toThrow(/not ready for execution.*FAILED.*Something went wrong/); + }); + + test('throws when change set is in CREATE_PENDING status', async () => { + // GIVEN + givenStackExists(); + mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ + Status: 'CREATE_PENDING', + ChangeSetName: 'my-change-set', + }); + + // THEN + await expect(testDeployStack({ + ...standardDeployStackArguments(), + deploymentMethod: { method: 'execute-change-set', changeSetName: 'my-change-set' }, + })).rejects.toThrow(/not ready for execution.*CREATE_PENDING/); + }); + + test('throws without reason when status reason is absent', async () => { + // GIVEN + givenStackExists(); + mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ + Status: 'DELETE_COMPLETE', + ChangeSetName: 'my-change-set', + }); + + // THEN + await expect(testDeployStack({ + ...standardDeployStackArguments(), + deploymentMethod: { method: 'execute-change-set', changeSetName: 'my-change-set' }, + })).rejects.toThrow(/not ready for execution.*DELETE_COMPLETE(?!.*:)/); + }); + + test('returns replacement-requires-rollback when change set has replacement and rollback is disabled', async () => { + // GIVEN + givenStackExists(); + givenChangeSetContainsReplacement(true); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + 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(); + givenTemplateIs(DEFAULT_FAKE_TEMPLATE); + mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ + Status: 'CREATE_COMPLETE', + ChangeSetName: 'my-change-set', + Changes: [{ Type: 'Resource' as const }], + }); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + deploymentMethod: { method: 'execute-change-set', changeSetName: 'my-change-set' }, + }); + + // THEN - still executes despite identical template + expect(result.type).toEqual('did-deploy-stack'); + expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); + }); +}); + +describe('change set returned with execute:false', () => { + test('returns changeSet description when execute is false', async () => { + // GIVEN + const changeSetResponse = { + Status: ChangeSetStatus.CREATE_COMPLETE, + ChangeSetName: 'cdk-deploy-change-set', + ChangeSetId: 'arn:aws:cloudformation:change-set/123', + StackId: 'arn:aws:cloudformation:stack/123', + Changes: [{ Type: 'Resource' as const }], + }; + mockCloudFormationClient.on(DescribeChangeSetCommand).resolves(changeSetResponse); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + deploymentMethod: { method: 'change-set', execute: false }, + }); + + // THEN + expect(result.type).toEqual('did-deploy-stack'); + assertIsSuccessfulDeployStackResult(result); + expect(result.noOp).toBe(false); + expect(result.changeSet).toBeDefined(); + expect(result.changeSet?.Status).toEqual('CREATE_COMPLETE'); + }); + + test('does not return changeSet when change set is empty', async () => { + // GIVEN + mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ + Status: 'FAILED', + StatusReason: "The submitted information didn't contain changes.", + }); + + // WHEN + const result = await testDeployStack({ + ...standardDeployStackArguments(), + deploymentMethod: { method: 'change-set', execute: false }, + }); + + // THEN + assertIsSuccessfulDeployStackResult(result); + expect(result.noOp).toBe(true); + expect(result.changeSet).toBeUndefined(); + }); +}); /** * Set up the mocks so that it looks like the stack exists to start with * diff --git a/packages/aws-cdk/lib/api/deploy-private.ts b/packages/aws-cdk/lib/api/deploy-private.ts new file mode 100644 index 000000000..8dc7f93db --- /dev/null +++ b/packages/aws-cdk/lib/api/deploy-private.ts @@ -0,0 +1,2 @@ +/* eslint-disable import/no-relative-packages */ +export * from '../../../@aws-cdk/toolkit-lib/lib/actions/deploy/private/deployment-method'; diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index c76e7c1ec..f5b0204ac 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -32,10 +32,12 @@ import type { SdkProvider } from '../api/aws-auth'; import type { BootstrapEnvironmentOptions } from '../api/bootstrap'; import { Bootstrapper } from '../api/bootstrap'; import { ExtendedStackSelection, StackCollection } from '../api/cloud-assembly'; +import { isChangeSetDeployment, isExecutingChangeSetDeployment, isNonExecutingChangeSetDeployment, toExecuteChangeSetDeployment } from '../api/deploy-private'; import type { Deployments, SuccessfulDeployStackResult } from '../api/deployments'; import { mappingsByEnvironment, parseMappingGroups } from '../api/refactor'; import { type Tag } from '../api/tags'; import { StackActivityProgress } from '../commands/deploy'; +import { FlagOperations } from '../commands/flags/operations'; import { listStacks } from '../commands/list-stacks'; import type { FromScan, GenerateTemplateOutput } from '../commands/migrate'; import { @@ -69,7 +71,6 @@ import { canCollectTelemetry } from './telemetry/collect-telemetry'; import { cdkCliErrorName } from './telemetry/error'; import { CLI_PRIVATE_SPAN } from './telemetry/messages'; import type { ErrorDetails } from './telemetry/schema'; -import { FlagOperations } from '../commands/flags/operations'; // Must use a require() otherwise esbuild complains about calling a namespace // eslint-disable-next-line @typescript-eslint/no-require-imports,@typescript-eslint/consistent-type-imports @@ -371,7 +372,9 @@ export class CdkToolkit { quiet: boolean, ) { try { - await this.props.deployments.stackExists({ + // we don't actually need to know if the stack exists here + // we just use this to flush our any permissions issues and drop the result + void await this.props.deployments.stackExists({ stack, deployName: stack.stackName, tryLookupRole: true, @@ -509,12 +512,62 @@ export class CdkToolkit { return; } + // Following are the same semantics we apply with respect to Notification ARNs (dictated by the SDK) + // + // - undefined => cdk ignores it, as if it wasn't supported (allows external management). + // - []: => cdk manages it, and the user wants to wipe it out. + // - ['arn-1'] => cdk manages it, and the user wants to set it to ['arn-1']. + const notificationArns = (!!options.notificationArns || !!stack.notificationArns) + ? (options.notificationArns ?? []).concat(stack.notificationArns ?? []) + : undefined; + + for (const notificationArn of notificationArns ?? []) { + if (!validateSnsTopicArn(notificationArn)) { + throw new ToolkitError('InvalidSnsTopicArn', `Notification arn ${notificationArn} is not a valid arn for an SNS topic`); + } + } + + // Deploy options that are shared between change set creation and execution + const sharedDeployOptions = { + stack, + deployName: stack.stackName, + roleArn: options.roleArn, + toolkitStackName: options.toolkitStackName, + reuseAssets: options.reuseAssets, + tags: (options.tags?.length ? options.tags : tagsForStack(stack)), + forceDeployment: options.force, + parameters: Object.assign({}, parameterMap['*'], parameterMap[stack.stackName]), + usePreviousParameters: options.usePreviousParameters, + rollback: options.rollback, + notificationArns, + extraUserAgent: options.extraUserAgent, + assetParallelism: options.assetParallelism, + }; + + // When using change-set method, always create the change set upfront. + // This gives us an accurate diff for approval and avoids creating it twice. + // For non-executing deployments (prepare-change-set), this is the final result. + const prepareResult = isChangeSetDeployment(options.deploymentMethod) + ? await this.props.deployments.prepareStack({ + ...sharedDeployOptions, + deploymentMethod: options.deploymentMethod, + cleanupOnNoOp: isExecutingChangeSetDeployment(options.deploymentMethod), + }) + : undefined; + + // Empty change set — no changes to deploy + if (prepareResult?.noOp === true) { + await this.ioHost.asIoHelper().defaults.info(' ✅ %s (no changes)', chalk.bold(stack.displayName)); + return; + } + if (requireApproval !== RequireApproval.NEVER) { const currentTemplate = await this.props.deployments.readCurrentTemplate(stack); const formatter = new DiffFormatter({ templateInfo: { oldTemplate: currentTemplate, newTemplate: stack, + changeSet: prepareResult?.changeSet, }, }); const securityDiff = formatter.formatSecurityDiff(); @@ -526,40 +579,28 @@ export class CdkToolkit { const diffOutput = hasSecurityChanges ? securityDiff.formattedDiff : formatter.formatStackDiff().formattedDiff; await this.ioHost.asIoHelper().defaults.info(diffOutput); - await askUserConfirmation( - this.ioHost, - IO.CDK_TOOLKIT_I5060.req(`${motivation}: 'Do you wish to deploy these changes'`, { - motivation, - concurrency, - permissionChangeType: securityDiff.permissionChangeType, - templateDiffs: formatter.diffs, - }), - ); - } - } - - // Following are the same semantics we apply with respect to Notification ARNs (dictated by the SDK) - // - // - undefined => cdk ignores it, as if it wasn't supported (allows external management). - // - []: => cdk manages it, and the user wants to wipe it out. - // - ['arn-1'] => cdk manages it, and the user wants to set it to ['arn-1']. - const notificationArns = (!!options.notificationArns || !!stack.notificationArns) - ? (options.notificationArns ?? []).concat(stack.notificationArns ?? []) - : undefined; - - for (const notificationArn of notificationArns ?? []) { - if (!validateSnsTopicArn(notificationArn)) { - throw new ToolkitError('InvalidSnsTopicArn', `Notification arn ${notificationArn} is not a valid arn for an SNS topic`); + try { + await askUserConfirmation( + this.ioHost, + IO.CDK_TOOLKIT_I5060.req(`${motivation}: 'Do you wish to deploy these changes'`, { + motivation, + concurrency, + permissionChangeType: securityDiff.permissionChangeType, + templateDiffs: formatter.diffs, + }), + ); + } catch (e) { + if (prepareResult?.changeSet?.ChangeSetName) { + await this.props.deployments.cleanupChangeSet(stack, prepareResult.changeSet.ChangeSetName); + } + throw e; + } } } const stackIndex = stacks.indexOf(stack) + 1; await this.ioHost.asIoHelper().defaults.info(`${chalk.bold(stack.displayName)}: deploying... [${stackIndex}/${stackCollection.stackCount}]`); const startDeployTime = new Date().getTime(); - let tags = options.tags; - if (!tags || tags.length === 0) { - tags = tagsForStack(stack); - } // There is already a startDeployTime constant, but that does not work with telemetry. // We should integrate the two in the future @@ -568,9 +609,17 @@ export class CdkToolkit { let error: ErrorDetails | undefined; let elapsedDeployTime = 0; try { - let deployResult: SuccessfulDeployStackResult | undefined; + // The prepare result is final if the change set was empty (noOp) or + // the deployment method is non-executing (prepare-change-set). + const prepareIsFinal = prepareResult && isNonExecutingChangeSetDeployment(options.deploymentMethod); + let deployResult: SuccessfulDeployStackResult | undefined = prepareIsFinal ? prepareResult : undefined; + // Start with user config for rollback, + // but it might change if we encounter a failed state. let rollback = options.rollback; + + // We limit the loop to 2 iterations max as defensive programming. + // Should not be possible to happen. let iteration = 0; while (!deployResult) { if (++iteration > 2) { @@ -578,20 +627,13 @@ export class CdkToolkit { } const r = await this.props.deployments.deployStack({ - stack, - deployName: stack.stackName, - roleArn: options.roleArn, - toolkitStackName: options.toolkitStackName, - reuseAssets: options.reuseAssets, - notificationArns, - tags, - deploymentMethod: options.deploymentMethod, - forceDeployment: options.force, - parameters: Object.assign({}, parameterMap['*'], parameterMap[stack.stackName]), - usePreviousParameters: options.usePreviousParameters, + ...sharedDeployOptions, + // On the first iteration, execute the prepared change set. + // On retries (after rollback), create a new change set since the old one is gone. + deploymentMethod: iteration === 1 && isExecutingChangeSetDeployment(options.deploymentMethod) + ? toExecuteChangeSetDeployment(options.deploymentMethod) + : options.deploymentMethod, rollback, - extraUserAgent: options.extraUserAgent, - assetParallelism: options.assetParallelism, }); switch (r.type) { diff --git a/packages/aws-cdk/test/cli/cdk-toolkit.test.ts b/packages/aws-cdk/test/cli/cdk-toolkit.test.ts index 9b06deeb8..50a2ea684 100644 --- a/packages/aws-cdk/test/cli/cdk-toolkit.test.ts +++ b/packages/aws-cdk/test/cli/cdk-toolkit.test.ts @@ -61,7 +61,7 @@ import * as cxschema from '@aws-cdk/cloud-assembly-schema'; import { Manifest, RequireApproval } from '@aws-cdk/cloud-assembly-schema'; import type { DeploymentMethod } from '@aws-cdk/toolkit-lib'; import type { DestroyStackResult } from '@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack'; -import { DescribeStacksCommand, GetTemplateCommand, StackStatus } from '@aws-sdk/client-cloudformation'; +import { CreateChangeSetCommand, DescribeChangeSetCommand, DescribeStacksCommand, GetTemplateCommand, StackStatus } from '@aws-sdk/client-cloudformation'; import { GetParameterCommand } from '@aws-sdk/client-ssm'; import * as fs from 'fs-extra'; import { type Template, type SdkProvider, WorkGraphBuilder } from '../../lib/api'; @@ -225,6 +225,286 @@ describe('deploy', () => { })); }); + describe('two-phase deploy with change-set approval', () => { + test('calls prepareStack then execute-change-set when approval is required', async () => { + // GIVEN + const mockCfnDeployments = instanceMockFrom(Deployments); + mockCfnDeployments.readCurrentTemplate.mockResolvedValue({}); + mockCfnDeployments.prepareStack.mockResolvedValue({ + type: 'did-deploy-stack', + noOp: false, + outputs: {}, + stackArn: 'stackArn', + changeSet: { Status: 'CREATE_COMPLETE', Changes: [{ Type: 'Resource' }], ChangeSetName: 'cdk-deploy-change-set', $metadata: {} }, + }); + mockCfnDeployments.deployStack.mockResolvedValue({ + type: 'did-deploy-stack', + noOp: false, + outputs: {}, + stackArn: 'stackArn', + }); + + const cdkToolkit = new CdkToolkit({ + ioHost, + cloudExecutable, + configuration: cloudExecutable.configuration, + sdkProvider: cloudExecutable.sdkProvider, + deployments: mockCfnDeployments, + }); + + // WHEN + await cdkToolkit.deploy({ + selector: { patterns: ['Test-Stack-A-Display-Name'] }, + requireApproval: RequireApproval.ANYCHANGE, + deploymentMethod: { method: 'change-set' }, + }); + + // THEN + expect(mockCfnDeployments.prepareStack).toHaveBeenCalledTimes(1); + expect(mockCfnDeployments.prepareStack).toHaveBeenCalledWith( + expect.objectContaining({ + deploymentMethod: expect.objectContaining({ method: 'change-set' }), + }), + ); + expect(mockCfnDeployments.deployStack).toHaveBeenCalledTimes(1); + expect(mockCfnDeployments.deployStack).toHaveBeenCalledWith( + expect.objectContaining({ + deploymentMethod: expect.objectContaining({ method: 'execute-change-set' }), + }), + ); + }); + + test('always creates change set upfront with method=change-set even when requireApproval is never', async () => { + // GIVEN + const mockCfnDeployments = instanceMockFrom(Deployments); + mockCfnDeployments.readCurrentTemplate.mockResolvedValue({}); + mockCfnDeployments.prepareStack.mockResolvedValue({ + type: 'did-deploy-stack', + noOp: false, + outputs: {}, + stackArn: 'stackArn', + changeSet: { Status: 'CREATE_COMPLETE', Changes: [], ChangeSetName: 'cdk-deploy-change-set', $metadata: {} }, + }); + mockCfnDeployments.deployStack.mockResolvedValue({ + type: 'did-deploy-stack', + noOp: false, + outputs: {}, + stackArn: 'stackArn', + }); + + const cdkToolkit = new CdkToolkit({ + ioHost, + cloudExecutable, + configuration: cloudExecutable.configuration, + sdkProvider: cloudExecutable.sdkProvider, + deployments: mockCfnDeployments, + }); + + // WHEN + await cdkToolkit.deploy({ + selector: { patterns: ['Test-Stack-A-Display-Name'] }, + requireApproval: RequireApproval.NEVER, + deploymentMethod: { method: 'change-set' }, + }); + + // THEN — prepare + execute + expect(mockCfnDeployments.prepareStack).toHaveBeenCalledTimes(1); + expect(mockCfnDeployments.prepareStack).toHaveBeenCalledWith( + expect.objectContaining({ + deploymentMethod: expect.objectContaining({ method: 'change-set' }), + }), + ); + expect(mockCfnDeployments.deployStack).toHaveBeenCalledTimes(1); + expect(mockCfnDeployments.deployStack).toHaveBeenCalledWith( + expect.objectContaining({ + deploymentMethod: expect.objectContaining({ method: 'execute-change-set' }), + }), + ); + }); + + test('does not create change set for approval with method=direct', async () => { + // GIVEN + const mockCfnDeployments = instanceMockFrom(Deployments); + mockCfnDeployments.readCurrentTemplate.mockResolvedValue({}); + mockCfnDeployments.deployStack.mockResolvedValue({ + type: 'did-deploy-stack', + noOp: false, + outputs: {}, + stackArn: 'stackArn', + }); + + const cdkToolkit = new CdkToolkit({ + ioHost, + cloudExecutable, + configuration: cloudExecutable.configuration, + sdkProvider: cloudExecutable.sdkProvider, + deployments: mockCfnDeployments, + }); + + // WHEN + await cdkToolkit.deploy({ + selector: { patterns: ['Test-Stack-A-Display-Name'] }, + requireApproval: RequireApproval.ANYCHANGE, + deploymentMethod: { method: 'direct' }, + }); + + // THEN — only one call with direct method + expect(mockCfnDeployments.deployStack).toHaveBeenCalledTimes(1); + expect(mockCfnDeployments.deployStack).toHaveBeenCalledWith( + expect.objectContaining({ + deploymentMethod: { method: 'direct' }, + }), + ); + }); + + test('skips deploy when prepare returns noOp', async () => { + // GIVEN + const mockCfnDeployments = instanceMockFrom(Deployments); + mockCfnDeployments.readCurrentTemplate.mockResolvedValue({}); + mockCfnDeployments.deployStack.mockResolvedValueOnce({ + type: 'did-deploy-stack', + noOp: true, + outputs: {}, + stackArn: 'stackArn', + }); + + const cdkToolkit = new CdkToolkit({ + ioHost, + cloudExecutable, + configuration: cloudExecutable.configuration, + sdkProvider: cloudExecutable.sdkProvider, + deployments: mockCfnDeployments, + }); + + // WHEN + await cdkToolkit.deploy({ + selector: { patterns: ['Test-Stack-A-Display-Name'] }, + requireApproval: RequireApproval.ANYCHANGE, + deploymentMethod: { method: 'change-set' }, + }); + + // THEN — only the prepare call, no execute + expect(mockCfnDeployments.deployStack).toHaveBeenCalledTimes(1); + }); + + test('cleans up change set when approval is rejected', async () => { + // GIVEN + const mockCfnDeployments = instanceMockFrom(Deployments); + mockCfnDeployments.readCurrentTemplate.mockResolvedValue({}); + mockCfnDeployments.prepareStack.mockResolvedValue({ + type: 'did-deploy-stack', + noOp: false, + outputs: {}, + stackArn: 'stackArn', + changeSet: { Status: 'CREATE_COMPLETE', Changes: [{ Type: 'Resource' }], ChangeSetName: 'my-change-set', $metadata: {} }, + }); + + // Reject approval + requestSpy.mockRejectedValue(new Error('Aborted by user')); + + const cdkToolkit = new CdkToolkit({ + ioHost, + cloudExecutable, + configuration: cloudExecutable.configuration, + sdkProvider: cloudExecutable.sdkProvider, + deployments: mockCfnDeployments, + }); + + // WHEN + await expect(cdkToolkit.deploy({ + selector: { patterns: ['Test-Stack-A-Display-Name'] }, + requireApproval: RequireApproval.ANYCHANGE, + deploymentMethod: { method: 'change-set' }, + })).rejects.toThrow(/Aborted/); + + // THEN + expect(mockCfnDeployments.cleanupChangeSet).toHaveBeenCalledWith( + expect.anything(), + 'my-change-set', + ); + }); + + test('prepare-change-set skips deploy loop and returns prepare result', async () => { + // GIVEN + const mockCfnDeployments = instanceMockFrom(Deployments); + mockCfnDeployments.prepareStack.mockResolvedValue({ + type: 'did-deploy-stack', + noOp: false, + outputs: {}, + stackArn: 'stackArn', + changeSet: { Status: 'CREATE_COMPLETE', Changes: [{ Type: 'Resource' }], ChangeSetName: 'cdk-deploy-change-set', $metadata: {} }, + }); + + const cdkToolkit = new CdkToolkit({ + ioHost, + cloudExecutable, + configuration: cloudExecutable.configuration, + sdkProvider: cloudExecutable.sdkProvider, + deployments: mockCfnDeployments, + }); + + // WHEN + await cdkToolkit.deploy({ + selector: { patterns: ['Test-Stack-A-Display-Name'] }, + requireApproval: RequireApproval.NEVER, + deploymentMethod: { method: 'change-set', execute: false }, + }); + + // THEN — prepare only, no deployStack call + expect(mockCfnDeployments.prepareStack).toHaveBeenCalledTimes(1); + expect(mockCfnDeployments.deployStack).not.toHaveBeenCalled(); + }); + + test('falls back to original method on retry after rollback', async () => { + // GIVEN + const mockCfnDeployments = instanceMockFrom(Deployments); + mockCfnDeployments.readCurrentTemplate.mockResolvedValue({}); + mockCfnDeployments.prepareStack.mockResolvedValue({ + type: 'did-deploy-stack', + noOp: false, + outputs: {}, + stackArn: 'stackArn', + changeSet: { Status: 'CREATE_COMPLETE', Changes: [{ Type: 'Resource' }], ChangeSetName: 'cdk-deploy-change-set', $metadata: {} }, + }); + // First deploy: needs rollback. Second deploy: succeeds. + mockCfnDeployments.deployStack + .mockResolvedValueOnce({ type: 'failpaused-need-rollback-first', status: 'UPDATE_ROLLBACK_FAILED', reason: 'not-norollback' }) + .mockResolvedValueOnce({ type: 'did-deploy-stack', noOp: false, outputs: {}, stackArn: 'stackArn' }); + mockCfnDeployments.rollbackStack.mockResolvedValue({ success: true, stackArn: 'stackArn' }); + + // Auto-confirm rollback prompt + requestSpy.mockResolvedValue(true); + + const cdkToolkit = new CdkToolkit({ + ioHost, + cloudExecutable, + configuration: cloudExecutable.configuration, + sdkProvider: cloudExecutable.sdkProvider, + deployments: mockCfnDeployments, + }); + + // WHEN + await cdkToolkit.deploy({ + selector: { patterns: ['Test-Stack-A-Display-Name'] }, + requireApproval: RequireApproval.NEVER, + deploymentMethod: { method: 'change-set' }, + }); + + // THEN — first call uses execute-change-set, second uses original change-set method + expect(mockCfnDeployments.deployStack).toHaveBeenCalledTimes(2); + expect(mockCfnDeployments.deployStack).toHaveBeenNthCalledWith(1, + expect.objectContaining({ + deploymentMethod: expect.objectContaining({ method: 'execute-change-set' }), + }), + ); + expect(mockCfnDeployments.deployStack).toHaveBeenNthCalledWith(2, + expect.objectContaining({ + deploymentMethod: expect.objectContaining({ method: 'change-set' }), + }), + ); + }); + }); + test('fails when no valid stack names are given', async () => { // GIVEN const toolkit = defaultToolkitSetup(); @@ -876,6 +1156,14 @@ describe('deploy', () => { CreationTime: new Date(), }, ], + }) + .on(CreateChangeSetCommand) + .resolves({ Id: 'changeset-id', StackId: 'stack-id' }) + .on(DescribeChangeSetCommand) + .resolves({ + Status: 'CREATE_COMPLETE', + ChangeSetName: 'cdk-deploy-change-set', + Changes: [{ Type: 'Resource' }], }); }); @@ -906,7 +1194,7 @@ describe('deploy', () => { }); expect(mockForEnvironment).toHaveBeenCalledTimes(2); expect(mockForEnvironment).toHaveBeenNthCalledWith( - 1, + 2, { account: '123456789012', name: 'aws://123456789012/here', @@ -954,7 +1242,7 @@ describe('deploy', () => { }); expect(mockForEnvironment).toHaveBeenCalledTimes(3); expect(mockForEnvironment).toHaveBeenNthCalledWith( - 1, + 2, { account: '123456789012', name: 'aws://123456789012/here', @@ -967,7 +1255,7 @@ describe('deploy', () => { }, ); expect(mockForEnvironment).toHaveBeenNthCalledWith( - 2, + 3, { account: '123456789012', name: 'aws://123456789012/here', @@ -1012,7 +1300,7 @@ describe('deploy', () => { ); expect(mockForEnvironment).toHaveBeenCalledTimes(3); expect(mockForEnvironment).toHaveBeenNthCalledWith( - 1, + 2, { account: '123456789012', name: 'aws://123456789012/here', @@ -1025,7 +1313,7 @@ describe('deploy', () => { }, ); expect(mockForEnvironment).toHaveBeenNthCalledWith( - 2, + 3, { account: '123456789012', name: 'aws://123456789012/here', @@ -1041,10 +1329,12 @@ describe('deploy', () => { test('fallback to deploy role if forEnvironment throws', async () => { // GIVEN - // throw error first for the 'prepareSdkWithLookupRoleFor' call and succeed for the rest - mockForEnvironment = jest.spyOn(sdkProvider, 'forEnvironment').mockImplementationOnce(() => { - throw new Error('TheErrorThatGetsThrown'); - }); + // throw error for the 'prepareSdkWithLookupRoleFor' call (second call, after the prepare phase) and succeed for the rest + mockForEnvironment = jest.spyOn(sdkProvider, 'forEnvironment') + .mockResolvedValueOnce({ sdk: mockSdk, didAssumeRole: true }) + .mockImplementationOnce(() => { + throw new Error('TheErrorThatGetsThrown'); + }); const cdkToolkit = new CdkToolkit({ ioHost, @@ -1070,7 +1360,7 @@ describe('deploy', () => { ); expect(mockForEnvironment).toHaveBeenCalledTimes(3); expect(mockForEnvironment).toHaveBeenNthCalledWith( - 1, + 2, { account: '123456789012', name: 'aws://123456789012/here', @@ -1083,7 +1373,7 @@ describe('deploy', () => { }, ); expect(mockForEnvironment).toHaveBeenNthCalledWith( - 2, + 3, { account: '123456789012', name: 'aws://123456789012/here', @@ -1134,11 +1424,10 @@ describe('deploy', () => { name: 'aws://123456789012/here', region: 'here', }, - Mode.ForReading, - { - assumeRoleArn: 'bloop-lookup:here:123456789012', - assumeRoleExternalId: undefined, - }, + Mode.ForWriting, + expect.objectContaining({ + assumeRoleArn: 'bloop:here:123456789012', + }), ); expect(mockForEnvironment).toHaveBeenNthCalledWith( 2, @@ -1147,9 +1436,9 @@ describe('deploy', () => { name: 'aws://123456789012/here', region: 'here', }, - Mode.ForWriting, + Mode.ForReading, { - assumeRoleArn: 'bloop:here:123456789012', + assumeRoleArn: 'bloop-lookup:here:123456789012', assumeRoleExternalId: undefined, }, ); @@ -1190,11 +1479,10 @@ describe('deploy', () => { name: 'aws://123456789012/here', region: 'here', }, - 0, - { - assumeRoleArn: undefined, - assumeRoleExternalId: undefined, - }, + Mode.ForWriting, + expect.objectContaining({ + assumeRoleArn: 'bloop:here:123456789012', + }), ); expect(mockForEnvironment).toHaveBeenNthCalledWith( 2, @@ -1203,9 +1491,9 @@ describe('deploy', () => { name: 'aws://123456789012/here', region: 'here', }, - 1, + Mode.ForReading, { - assumeRoleArn: 'bloop:here:123456789012', + assumeRoleArn: undefined, assumeRoleExternalId: undefined, }, ); @@ -1938,7 +2226,17 @@ describe('rollback', () => { const mockedDeployStack = jest .spyOn(deployments, 'deployStack') + // First call: prepare phase (execute: false) + .mockResolvedValueOnce({ + type: 'did-deploy-stack', + noOp: false, + outputs: {}, + stackArn: 'stack:arn', + changeSet: { Status: 'CREATE_COMPLETE', Changes: [], ChangeSetName: 'cdk-deploy-change-set', $metadata: {} }, + }) + // Second call: execute-change-set returns the test's expected result .mockResolvedValueOnce(firstResult) + // Third call: retry after rollback .mockResolvedValueOnce({ type: 'did-deploy-stack', noOp: false, @@ -1978,8 +2276,9 @@ describe('rollback', () => { } } - expect(mockedDeployStack).toHaveBeenNthCalledWith(1, expect.objectContaining({ rollback: false })); - expect(mockedDeployStack).toHaveBeenNthCalledWith(2, expect.objectContaining({ rollback: true })); + expect(mockedDeployStack).toHaveBeenNthCalledWith(1, expect.objectContaining({ deploymentMethod: expect.objectContaining({ execute: false }) })); + expect(mockedDeployStack).toHaveBeenNthCalledWith(2, expect.objectContaining({ rollback: false, deploymentMethod: expect.objectContaining({ method: 'execute-change-set' }) })); + expect(mockedDeployStack).toHaveBeenNthCalledWith(3, expect.objectContaining({ rollback: true })); }); }); @@ -2113,12 +2412,6 @@ class FakeCloudFormation extends Deployments { expect(options.tags).toEqual(this.expectedTags[options.stack.stackName]); } - // In these tests, we don't make a distinction here between `undefined` and `[]`. - // - // In tests `deployStack` itself we do treat `undefined` and `[]` differently, - // and in `aws-cdk-lib` we emit them under different conditions. But this test - // without normalization depends on a version of `aws-cdk-lib` that hasn't been - // released yet. expect(options.notificationArns ?? []).toEqual(this.expectedNotificationArns ?? []); return Promise.resolve({ type: 'did-deploy-stack',