diff --git a/packages/@aws-cdk/toolkit-lib/lib/actions/diagnose/index.ts b/packages/@aws-cdk/toolkit-lib/lib/actions/diagnose/index.ts index dfad646e0..92b845d6d 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/actions/diagnose/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/actions/diagnose/index.ts @@ -29,6 +29,7 @@ export type StackDiagnosis = export type StackProblemSource = | { type: 'deployment'; stackStatus: string; statusReason: string } | { type: 'change-set'; changeSetName: string; changeSetStatus: string; statusReason: string } + | { type: 'change-set-not-ready'; changeSetName: string; changeSetStatus: string; statusReason: string } | { type: 'early-validation'; changeSetName: string }; export interface DiagnosedStack { diff --git a/packages/@aws-cdk/toolkit-lib/lib/actions/diff/private/helpers.ts b/packages/@aws-cdk/toolkit-lib/lib/actions/diff/private/helpers.ts index 2085df46a..a07aecc90 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/actions/diff/private/helpers.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/actions/diff/private/helpers.ts @@ -5,6 +5,7 @@ import * as fs from 'fs-extra'; import type { ChangeSetDiffOptions, DiffOptions, LocalFileDiffOptions } from '..'; import { DiffMethod } from '..'; import type { SdkProvider } from '../../../api/aws-auth/private'; +import { ChangeSetDescriber } from '../../../api/change-sets'; import type { StackCollection } from '../../../api/cloud-assembly/stack-collection'; import type { NestedStackTemplates } from '../../../api/cloudformation'; import type { Deployments } from '../../../api/deployments'; @@ -89,7 +90,7 @@ async function cfnDiff( removeNonImportResources(stack); } - const changeSet = includeChangeSet ? await cfnApi.createDiffChangeSet(ioHelper, { + const changeSet = includeChangeSet ? (await cfnApi.createDiffChangeSet(ioHelper, { deployments, stack, sdkProvider, @@ -98,13 +99,12 @@ async function cfnDiff( failOnError: !(methodOptions.fallbackToTemplate ?? true), importExistingResources: methodOptions.importExistingResources, uuid: randomUUID(), - willExecute: false, - }) : undefined; + }))?.changeSet : undefined; // If the changeset includes nested stacks, describe each nested changeset // and attach it to the corresponding entry in nestedStacks. if (changeSet) { - await attachNestedChangeSetData(deployments, stack, changeSet, nestedStacks); + await attachNestedChangeSetData(ioHelper, deployments, stack, changeSet, nestedStacks); } const mappings = allMappings.find(m => @@ -130,6 +130,7 @@ async function cfnDiff( * attach it to the matching entry in the nestedStacks map. */ async function attachNestedChangeSetData( + ioHelper: IoHelper, deployments: Deployments, stack: cxapi.CloudFormationStackArtifact, rootChangeSet: DescribeChangeSetCommandOutput, @@ -149,10 +150,12 @@ async function attachNestedChangeSetData( continue; } - const nestedChangeSet = await cfn.describeChangeSet({ - ChangeSetName: rc.ChangeSetId, - StackName: rc.PhysicalResourceId ?? rc.LogicalResourceId, - }); + const nestedChangeSet = await new ChangeSetDescriber({ + cfn, + ioHelper, + stackNameOrArn: rc.PhysicalResourceId ?? rc.LogicalResourceId, + changeSetNameOrArn: rc.ChangeSetId, + }).waitForSettled(); // Replace the entry with one that includes the changeset (nestedStacks as any)[rc.LogicalResourceId] = { @@ -162,7 +165,7 @@ async function attachNestedChangeSetData( // Recurse into deeper nesting levels if (nestedChangeSet && Object.keys(nested.nestedStackTemplates).length > 0) { - await attachNestedChangeSetData(deployments, stack, nestedChangeSet, nested.nestedStackTemplates); + await attachNestedChangeSetData(ioHelper, deployments, stack, nestedChangeSet, nested.nestedStackTemplates); } } } diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/change-sets/change-set-describer.ts b/packages/@aws-cdk/toolkit-lib/lib/api/change-sets/change-set-describer.ts new file mode 100644 index 000000000..1bdd5a613 --- /dev/null +++ b/packages/@aws-cdk/toolkit-lib/lib/api/change-sets/change-set-describer.ts @@ -0,0 +1,303 @@ +import { format } from 'node:util'; +import type { Change, DescribeChangeSetCommandOutput, ResourceChangeDetail } from '@aws-sdk/client-cloudformation'; +import { ChangeSetStatus } from '@aws-sdk/client-cloudformation'; +import { DeploymentError } from '../../toolkit/toolkit-error'; +import { changeSetNameFromArn, stackNameFromArn } from '../../util/cloudformation'; +import { waitFor } from '../../util/promises'; +import type { ICloudFormationClient } from '../aws-auth/private'; +import type { Diagnosis } from '../diagnosing/diagnosis'; +import type { CloudFormationStackDiagnoser } from '../diagnosing/stack-diagnoser'; +import type { IoHelper } from '../io/private'; + +export interface ChangeSetReport { + readonly changeSet: DescribeChangeSetCommandOutput; + readonly diagnosis: Diagnosis; +} + +export interface ChangeSetDescriberProps { + readonly cfn: ICloudFormationClient; + readonly ioHelper: IoHelper; + /** + * The name or ARN of the stack the change set belongs to, prefer ARN. + */ + readonly stackNameOrArn: string; + /** + * The name or ARN of the change set, prefer ARN. + */ + readonly changeSetNameOrArn: string; +} + +export interface WaitForChangeSetOptions { + readonly diagnoser: CloudFormationStackDiagnoser; +} + +/** + * Describes a single CloudFormation change set. + * + * Change sets are described with `IncludePropertyValues`, which yields the before/after property + * values and resource contexts that let `diff` surface changes invisible to a pure template diff. + * CloudFormation may silently drop resource changes from such a response, in which case it says so + * in `StatusReason`; we then describe the change set a second time without property values and put + * back what was dropped. All pages of `Changes` are always fetched, so the change list is never + * truncated. See https://github.com/aws/aws-cdk-cli/issues/1780 + */ +export class ChangeSetDescriber { + /** + * Whether two change set change details describe the same change. + * + * This is the identity function for `mergeDescriptions`: a plain detail is only restored if no + * detailed one matches it. Compares identifying fields only; value-carrying fields differ + * between a description with and without `IncludePropertyValues`. `Target.Path` is deliberately + * ignored: it only exists on detailed details, and a detailed response can carry several + * path-level details for the same property name. A plain detail (which has no path) matching + * any of them means the property is already covered; restoring it would duplicate the change. + */ + private static describesSameChange(a: ResourceChangeDetail, b: ResourceChangeDetail): boolean { + return a.Target?.Attribute === b.Target?.Attribute + && a.Target?.Name === b.Target?.Name + && a.ChangeSource === b.ChangeSource + && a.CausingEntity === b.CausingEntity; + } + + private readonly cfn: ICloudFormationClient; + private readonly ioHelper: IoHelper; + private readonly stackNameOrArn: string; + private readonly changeSetNameOrArn: string; + private readonly stackDisplayName: string; + private readonly changeSetDisplayName: string; + + constructor(props: ChangeSetDescriberProps) { + this.cfn = props.cfn; + this.ioHelper = props.ioHelper; + this.stackNameOrArn = props.stackNameOrArn; + this.changeSetNameOrArn = props.changeSetNameOrArn; + this.stackDisplayName = stackNameFromArn(props.stackNameOrArn); + this.changeSetDisplayName = changeSetNameFromArn(props.changeSetNameOrArn); + } + + /** + * Waits for the change set to reach a terminal state (it stops being created), and describes it. + * + * Says nothing about whether the change set was created successfully; a failed change set is a + * perfectly good return value here. + */ + public async waitForSettled(): Promise { + await this.ioHelper.defaults.debug(format('Waiting for changeset %s on stack %s to finish creating...', this.changeSetDisplayName, this.stackDisplayName)); + const description = await waitFor(async () => { + const current = await this.describeCurrentState(); + + if (current.Status === 'CREATE_PENDING' || current.Status === 'CREATE_IN_PROGRESS') { + await this.ioHelper.defaults.debug(format('Changeset %s on stack %s is still creating', this.changeSetDisplayName, this.stackDisplayName)); + return undefined; + } + + return current; + }); + + if (!description) { + throw new DeploymentError('Change set took too long to be created; aborting', 'ChangeSetTimeout'); + } + + return description; + } + + /** + * Waits for the change set to settle and diagnoses the outcome, without throwing. + * + * Use this if you want to inspect or report the problems yourself. If you just want the + * operation to fail on a problem, use `waitAndThrowOnProblem`. + */ + public async waitForReport(options: WaitForChangeSetOptions): Promise { + const changeSet = await this.waitForSettled(); + const diagnosis = await options.diagnoser.diagnoseChangeSet(changeSet); + return { changeSet, diagnosis }; + } + + /** + * Waits for the change set to settle, and throws a `DeploymentError` if anything is wrong with it. + * + * Returns a change set that is either ready to be executed or that has no changes. + */ + public async waitAndThrowOnProblem(options: WaitForChangeSetOptions): Promise { + const report = await this.waitForReport(options); + report.diagnosis.throwOnError(); + return report; + } + + /** + * Describes the change set as it is right now, for the purpose of executing it. + * + * Like `describeCurrentState` this does not wait; unlike it, the returned report is guaranteed to + * describe a change set that can actually be executed. Use it for a change set that an earlier + * command already created (and that may not even be ours), where blocking on it indefinitely + * would be wrong: we report that it isn't ready instead. + */ + public async describeForExecution(options: WaitForChangeSetOptions): Promise { + const changeSet = await this.describeCurrentState(); + const diagnosis = await options.diagnoser.diagnoseChangeSet(changeSet, { requireExecutable: true }); + diagnosis.throwOnError(); + return { changeSet, diagnosis }; + } + + /** + * Waits for the change set to be gone (deleted, or never existed). + */ + public async waitForGone(): Promise { + await this.ioHelper.defaults.debug(format('Waiting for changeset %s on stack %s to finish deleting...', this.changeSetDisplayName, this.stackDisplayName)); + await waitFor(async () => { + try { + const description = await this.cfn.describeChangeSet({ + StackName: this.stackNameOrArn, + ChangeSetName: this.changeSetNameOrArn, + }); + + if (description.Status === ChangeSetStatus.DELETE_COMPLETE || description.Status === ChangeSetStatus.DELETE_FAILED) { + return true; + } + + return undefined; + } catch (e: any) { + if (e.name === 'ChangeSetNotFoundException') { + return true; + } + throw e; + } + }); + } + + /** + * Describe the change set as it is right now, without waiting for it to settle. + * + * Only use this for a change set that is already known to be in a terminal state (for example + * one that has already been waited for, or one that is being diagnosed after it failed). If the + * change set may still be creating, use `waitForSettled` or `waitAndThrowOnProblem` instead. + */ + public async describeCurrentState(): Promise { + const detailed = await this.describeOnce(true); + + // When the change set is pending, we never have changes so no need to try and recover missing ones. + if (detailed.Status === ChangeSetStatus.CREATE_PENDING || detailed.Status === ChangeSetStatus.CREATE_IN_PROGRESS) { + return detailed; + } + + // CloudFormation warned us that it left data out, so describe again without property values + // and put back whatever it dropped. + if (hasIncompletePropertyValuesWarning(detailed)) { + await this.ioHelper.defaults.debug(format( + 'Changeset %s on stack %s was described with incomplete data (%s); describing it again without property values to recover what was dropped', + this.changeSetDisplayName, this.stackDisplayName, detailed.StatusReason, + )); + return this.mergeDescriptions(detailed, await this.describeOnce(false)); + } + + return detailed; + } + + private async describeOnce(includePropertyValues: boolean): Promise { + const response = await this.cfn.describeChangeSet({ + StackName: this.stackNameOrArn, + ChangeSetName: this.changeSetNameOrArn, + IncludePropertyValues: includePropertyValues, + }); + + // Traverse all pages, so that `Changes` is always the complete set of changes. A truncated + // list would silently make callers believe a resource is unchanged. + while (response.NextToken != null) { + const nextPage = await this.cfn.describeChangeSet({ + StackName: response.StackId ?? this.stackNameOrArn, + ChangeSetName: response.ChangeSetId ?? this.changeSetNameOrArn, + NextToken: response.NextToken, + IncludePropertyValues: includePropertyValues, + }); + + // Consolidate the changes + if (nextPage.Changes != null) { + response.Changes = response.Changes != null ? response.Changes.concat(nextPage.Changes) : nextPage.Changes; + } + + // Forward the new NextToken + response.NextToken = nextPage.NextToken; + } + + return response; + } + + /** + * Merge two descriptions of the same change set: one described with `IncludePropertyValues` + * (detailed) and one described without (plain). + * + * CloudFormation can silently drop a `ResourceChange` from the detailed response when it + * fails to render property values (e.g. JSON-typed properties). The detailed description is + * used as the base; resource changes and details only present in the plain one are restored. + * + * See https://github.com/aws/aws-cdk-cli/issues/1780 + */ + private mergeDescriptions( + detailed: DescribeChangeSetCommandOutput, + plain: DescribeChangeSetCommandOutput, + ): DescribeChangeSetCommandOutput { + // Shallow-copy the changes so we can add details without mutating the input + const changes: Change[] = (detailed.Changes ?? []).map((change) => ({ + ...change, + ResourceChange: change.ResourceChange ? { ...change.ResourceChange } : undefined, + })); + + const changesByLogicalId = new Map(); + for (const change of changes) { + if (change.ResourceChange?.LogicalResourceId) { + changesByLogicalId.set(change.ResourceChange.LogicalResourceId, change); + } + } + + for (const change of plain.Changes ?? []) { + const logicalId = change.ResourceChange?.LogicalResourceId; + if (!logicalId) { + // Cannot correlate without a logical id; skip rather than risk duplicating. + continue; + } + + const existing = changesByLogicalId.get(logicalId); + if (!existing?.ResourceChange) { + // Dropped from the detailed response; restore it. + changes.push(change); + continue; + } + + // Present in both; restore any details dropped from the detailed response. + const existingDetails = existing.ResourceChange.Details ?? []; + const droppedDetails = (change.ResourceChange?.Details ?? []).filter( + (detail) => !existingDetails.some((existingDetail) => ChangeSetDescriber.describesSameChange(existingDetail, detail)), + ); + if (droppedDetails.length > 0) { + existing.ResourceChange.Details = [...existingDetails, ...droppedDetails]; + } + } + + // Take the status reason from the plain response: the detailed one is the incomplete-data + // warning, which would otherwise displace the real reason (for example the + // `AWS::EarlyValidation` marker that tells us why a change set failed to create). + return { ...detailed, StatusReason: plain.StatusReason ?? detailed.StatusReason, Changes: changes }; + } +} + +/** + * The marker CloudFormation puts in `StatusReason` when it left data out of a response that was + * requested with `IncludePropertyValues`. + * + * It only says this when it actually dropped something, and names the resources it gave up on. + * `Status` is `CREATE_COMPLETE` either way, so this is the only signal we get: + * + * ``` + * [WARN] --include-property-values option can return incomplete ChangeSet data because: Logical Id: MyResource, failed property validation + * ``` + */ +const INCOMPLETE_PROPERTY_VALUES_WARNING = '[WARN] --include-property-values option can return incomplete ChangeSet data'; + +/** + * Whether CloudFormation flagged this description as potentially missing data. + * + * See https://github.com/aws/aws-cdk-cli/issues/1780 + */ +function hasIncompletePropertyValuesWarning(description: DescribeChangeSetCommandOutput): boolean { + return description.StatusReason?.includes(INCOMPLETE_PROPERTY_VALUES_WARNING) ?? false; +} diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/change-sets/index.ts b/packages/@aws-cdk/toolkit-lib/lib/api/change-sets/index.ts new file mode 100644 index 000000000..4b0f7b0e9 --- /dev/null +++ b/packages/@aws-cdk/toolkit-lib/lib/api/change-sets/index.ts @@ -0,0 +1 @@ +export * from './change-set-describer'; diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/cfn-api.ts b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/cfn-api.ts index 1c1a74038..47e482e23 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/cfn-api.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/cfn-api.ts @@ -5,196 +5,29 @@ import { AssetManifest } from '@aws-cdk/cdk-assets-lib'; import * as cxapi from '@aws-cdk/cloud-assembly-api'; import { SSMPARAM_NO_INVALIDATE } from '@aws-cdk/cloud-assembly-api'; import { - ChangeSetStatus, - type DescribeChangeSetCommandOutput, type Parameter, type ResourceToImport, type Tag, } from '@aws-sdk/client-cloudformation'; import { AssetManifestBuilder } from './asset-manifest-builder'; import type { Deployments } from './deployments'; -import type { StackDiagnosis } from '../../actions/diagnose'; import { DeploymentError, ToolkitError } from '../../toolkit/toolkit-error'; import { changeSetNameFromArn, stackNameFromArn } from '../../util/cloudformation'; +import { waitFor } from '../../util/promises'; import type { ICloudFormationClient, SdkProvider } from '../aws-auth/private'; +import type { ChangeSetReport } from '../change-sets'; +import { ChangeSetDescriber } from '../change-sets'; import type { Template, TemplateBodyParameter, TemplateParameter } from '../cloudformation'; import { CloudFormationStack, makeBodyParameter } from '../cloudformation'; -import { throwDeploymentErrorFromDiagnosis } from '../diagnosing/diagnosis-formatting'; import { CloudFormationStackDiagnoser } from '../diagnosing/stack-diagnoser'; import type { IoHelper } from '../io/private'; import type { ResourcesToImport } from '../resource-import'; import { StackArtifactSourceTracer } from '../source-tracing/private/stack-source-tracing'; -/** - * Describe a changeset in CloudFormation, regardless of its current state. - * - * @param cfn - a CloudFormation client - * @param stackNameOrArn - the name or ARN of the Stack the ChangeSet belongs to, prefer ARN - * @param changeSetNameOrArn - the name or ARN of the ChangeSet, prefer ARN - * @param fetchAll - if true, fetches all pages of the change set description. - * - * @returns CloudFormation information about the ChangeSet - */ -async function describeChangeSet( - cfn: ICloudFormationClient, - stackNameOrArn: string, - changeSetNameOrArn: string, - { fetchAll, includePropertyValues }: { fetchAll: boolean; includePropertyValues?: boolean }, -): Promise { - const response = await cfn.describeChangeSet({ - StackName: stackNameOrArn, - ChangeSetName: changeSetNameOrArn, - IncludePropertyValues: includePropertyValues, - }); - - // If fetchAll is true, traverse all pages from the change set description. - while (fetchAll && response.NextToken != null) { - const nextPage = await cfn.describeChangeSet({ - StackName: response.StackId ?? stackNameOrArn, - ChangeSetName: response.ChangeSetId ?? changeSetNameOrArn, - NextToken: response.NextToken, - IncludePropertyValues: includePropertyValues, - }); - - // Consolidate the changes - if (nextPage.Changes != null) { - response.Changes = response.Changes != null ? response.Changes.concat(nextPage.Changes) : nextPage.Changes; - } - - // Forward the new NextToken - response.NextToken = nextPage.NextToken; - } - - return response; -} - -/** - * Waits for a function to return non-+undefined+ before returning. - * - * @param valueProvider - a function that will return a value that is not +undefined+ once the wait should be over - * @param timeout - the time to wait between two calls to +valueProvider+ - * - * @returns the value that was returned by +valueProvider+ - */ -async function waitFor( - valueProvider: () => Promise, - timeout: number = 5000, -): Promise { - while (true) { - const result = await valueProvider(); - if (result === null) { - return undefined; - } else if (result !== undefined) { - return result; - } - await new Promise((cb) => setTimeout(cb, timeout)); - } -} - -export interface ChangeSetReport { - readonly description: DescribeChangeSetCommandOutput; - readonly diagnosis: StackDiagnosis; -} - -/** - * Waits for a ChangeSet to reach a terminal state and returns the diagnosis without throwing. - */ -export async function waitForChangeSetReport( - cfn: ICloudFormationClient, - ioHelper: IoHelper, - stackNameOrArn: string, - changeSetNameOrArn: string, - { fetchAll, diagnoser, includePropertyValues }: { fetchAll: boolean; diagnoser: CloudFormationStackDiagnoser; includePropertyValues?: boolean }, -): Promise { - const stackDisplayName = stackNameFromArn(stackNameOrArn); - const changeSetDisplayName = changeSetNameFromArn(changeSetNameOrArn); - await ioHelper.defaults.debug(format('Waiting for changeset %s on stack %s to finish creating...', changeSetDisplayName, stackDisplayName)); - const ret = await waitFor(async () => { - const description = await describeChangeSet(cfn, stackNameOrArn, changeSetNameOrArn, { - fetchAll, - includePropertyValues, - }); - - if (description.Status === 'CREATE_PENDING' || description.Status === 'CREATE_IN_PROGRESS') { - await ioHelper.defaults.debug(format('Changeset %s on stack %s is still creating', changeSetDisplayName, stackDisplayName)); - return undefined; - } - - const diagnosis = await diagnoser.diagnoseChangeSet(description); - return { description, diagnosis }; - }); - - if (!ret) { - throw new DeploymentError('Change set took too long to be created; aborting', 'ChangeSetTimeout'); - } - - return ret; -} - -/** - * Waits for a ChangeSet to be available for triggering a StackUpdate. - * - * Will return a changeset that is either ready to be executed or has no changes. - * Will throw in other cases. - * - * @param cfn - a CloudFormation client - * @param stackNameOrArn - the name or ARN of the Stack the ChangeSet belongs to, prefer ARN - * @param changeSetNameOrArn - the name or ARN of the ChangeSet, prefer ARN - * @param fetchAll - if true, fetches all pages of the ChangeSet before returning. - * @returns the CloudFormation description of the ChangeSet - */ -export async function waitForChangeSet( - cfn: ICloudFormationClient, - ioHelper: IoHelper, - stackNameOrArn: string, - changeSetNameOrArn: string, - { fetchAll, diagnoser, includePropertyValues }: { fetchAll: boolean; diagnoser: CloudFormationStackDiagnoser; includePropertyValues?: boolean }, -): Promise { - const { description, diagnosis } = await waitForChangeSetReport( - cfn, ioHelper, stackNameOrArn, changeSetNameOrArn, - { fetchAll, diagnoser, includePropertyValues }, - ); - if (diagnosis.type !== 'no-problem') { - throwDeploymentErrorFromDiagnosis(diagnosis); - } - return description; -} - -export async function waitForChangeSetGone( - cfn: ICloudFormationClient, - ioHelper: IoHelper, - stackNameOrArn: string, - changeSetNameOrArn: string, -): Promise { - const stackDisplayName = stackNameFromArn(stackNameOrArn); - const changeSetDisplayName = changeSetNameFromArn(changeSetNameOrArn); - await ioHelper.defaults.debug(format('Waiting for changeset %s on stack %s to finish deleting...', changeSetDisplayName, stackDisplayName)); - await waitFor(async () => { - try { - const description = await cfn.describeChangeSet({ - StackName: stackNameOrArn, - ChangeSetName: changeSetNameOrArn, - }); - - if (description.Status === ChangeSetStatus.DELETE_COMPLETE || description.Status === ChangeSetStatus.DELETE_FAILED) { - return true; - } - - return undefined; - } catch (e: any) { - if (e.name === 'ChangeSetNotFoundException') { - return true; - } - throw e; - } - }); -} - export type PrepareChangeSetOptions = { stack: cxapi.CloudFormationStackArtifact; deployments: Deployments; uuid: string; - willExecute: boolean; sdkProvider: SdkProvider; parameters: { [name: string]: string | undefined }; resourcesToImport?: ResourcesToImport; @@ -212,7 +45,6 @@ export type PrepareChangeSetOptions = { export interface CreateChangeSetOptions { cfn: ICloudFormationClient; changeSetName: string; - willExecute: boolean; exists: boolean; uuid: string; stack: cxapi.CloudFormationStackArtifact; @@ -231,11 +63,27 @@ export interface CreateChangeSetOptions { export async function createDiffChangeSet( ioHelper: IoHelper, options: Omit, -): Promise { +): Promise { try { - return await uploadBodyParameterAndCreateChangeSet(ioHelper, { - ...options, + const { cfn, bodyParameter, exists, executionRoleArn, diagnoser } = await prepareChangeSetEnv(ioHelper, options); + + await ioHelper.defaults.info( + 'Hold on while we create a read-only change set to get a diff with accurate replacement information (use --method=template to use a less accurate but faster template-only diff)\n', + ); + + return await createChangeSetAndCleanup(ioHelper, { + cfn, + changeSetName: 'cdk-diff-change-set', + stack: options.stack, + exists, + uuid: options.uuid, + bodyParameter, + parameters: options.parameters, + resourcesToImport: options.resourcesToImport, + importExistingResources: options.importExistingResources, includeNestedStacks: true, + role: executionRoleArn, + diagnoser, }); } catch (e: any) { // This function is currently only used by diff so these messages are diff-specific @@ -317,33 +165,6 @@ async function prepareChangeSetEnv( return { cfn, bodyParameter, exists, stackExistedBefore: stack.exists, executionRoleArn, diagnoser }; } -async function uploadBodyParameterAndCreateChangeSet( - ioHelper: IoHelper, - options: PrepareChangeSetOptions, -): Promise { - const { cfn, bodyParameter, exists, executionRoleArn, diagnoser } = await prepareChangeSetEnv(ioHelper, options); - - await ioHelper.defaults.info( - 'Hold on while we create a read-only change set to get a diff with accurate replacement information (use --method=template to use a less accurate but faster template-only diff)\n', - ); - - return createChangeSetAndCleanup(ioHelper, { - cfn, - changeSetName: 'cdk-diff-change-set', - stack: options.stack, - exists, - uuid: options.uuid, - willExecute: options.willExecute, - bodyParameter, - parameters: options.parameters, - resourcesToImport: options.resourcesToImport, - importExistingResources: options.importExistingResources, - includeNestedStacks: options.includeNestedStacks, - role: executionRoleArn, - diagnoser, - }); -} - /** * Uploads the assets that look like templates for this CloudFormation stack * @@ -374,7 +195,7 @@ export async function uploadStackTemplateAssets(stack: cxapi.CloudFormationStack async function createChangeSetAndCleanup( ioHelper: IoHelper, options: CreateChangeSetOptions, -): Promise { +): Promise { if (options.exists) { await cleanupOldChangeset(options.cfn, ioHelper, options.changeSetName, options.stack.stackName); } @@ -403,21 +224,14 @@ async function createChangeSetAndCleanup( await ioHelper.defaults.debug(format('Initiated creation of changeset: %s; waiting for it to finish creating...', changeSet.Id)); // Fetching all pages if we'll execute, so we can have the correct change count when monitoring. - const createdChangeSet = await waitForChangeSet( - options.cfn, + const createdChangeSet = await new ChangeSetDescriber({ + cfn: options.cfn, ioHelper, - changeSet.StackId ?? options.stack.stackName, - changeSet.Id ?? options.changeSetName, - { - fetchAll: options.willExecute, - diagnoser: options.diagnoser, - // Ask CloudFormation to include the before/after property values and resource contexts - // in the change set description. This is what lets `cdk diff` surface changes that are - // invisible to a pure template diff (e.g. an SSM parameter or dynamic reference whose - // resolved value changed even though the local template did not). - includePropertyValues: true, - }, - ); + stackNameOrArn: changeSet.StackId ?? options.stack.stackName, + changeSetNameOrArn: changeSet.Id ?? options.changeSetName, + }).waitAndThrowOnProblem({ + diagnoser: options.diagnoser, + }); await cleanupOldChangeset( options.cfn, @@ -442,7 +256,7 @@ async function createChangeSetAndCleanup( /** * Create a change set for online validation (never executes, returns diagnosis instead of throwing). * - * Uses the same env preparation as diff, but calls `waitForChangeSetReport` to return + * Uses the same env preparation as diff, but calls `waitForReport` to return * the diagnosis rather than throwing on failure. Always cleans up the change set afterwards. */ export async function createValidationChangeSet( @@ -471,12 +285,12 @@ export async function createValidationChangeSet( }); try { - const report = await waitForChangeSetReport( - cfn, ioHelper, - changeSet.StackId ?? options.stack.stackName, - changeSet.Id ?? changeSetName, - { fetchAll: false, diagnoser }, - ); + const report = await new ChangeSetDescriber({ + cfn, + ioHelper, + stackNameOrArn: changeSet.StackId ?? options.stack.stackName, + changeSetNameOrArn: changeSet.Id ?? changeSetName, + }).waitForReport({ diagnoser }); return report; } finally { 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 04101cc74..31cce4d9c 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 @@ -4,7 +4,6 @@ import type * as cxapi from '@aws-cdk/cloud-assembly-api'; import type { CreateChangeSetCommandInput, CreateStackCommandInput, - DescribeChangeSetCommandOutput, ExecuteChangeSetCommandInput, UpdateStackCommandInput, Tag, @@ -20,10 +19,8 @@ import { } from './cfn-api'; import { TemplateParameters, - waitForChangeSet, waitForStackDeploy, waitForStackDelete, - waitForChangeSetGone, } from './cfn-api'; import { determineAllowCrossAccountAssetPublishing } from './checks'; import type { DeployStackResult, SuccessfulDeployStackResult } from './deployment-result'; @@ -34,9 +31,10 @@ import type { StabilizingResource } from '../../toolkit/types'; import { formatErrorMessage } from '../../util'; import { changeSetNameFromArn } from '../../util/cloudformation'; import type { SDK, SdkProvider, ICloudFormationClient } from '../aws-auth/private'; +import type { ChangeSetReport } from '../change-sets'; +import { ChangeSetDescriber } from '../change-sets'; import type { TemplateBodyParameter } from '../cloudformation'; import { makeBodyParameter, CfnEvaluationException, CloudFormationStack } from '../cloudformation'; -import { throwDeploymentErrorFromDiagnosis } from '../diagnosing/diagnosis-formatting'; import type { CloudFormationStackDiagnoser } from '../diagnosing/stack-diagnoser'; import { changeSetHasNoChanges } from '../diagnosing/stack-diagnoser'; import type { EnvironmentResources, StringWithoutPlaceholders } from '../environment'; @@ -450,7 +448,8 @@ class FullCloudFormationDeployment { const execute = deploymentMethod.execute ?? true; const importExistingResources = deploymentMethod.importExistingResources ?? false; const revertDrift = deploymentMethod.revertDrift ?? false; - const changeSetDescription = await this.createChangeSet(changeSetName, execute, importExistingResources, revertDrift); + const changeSetReport = await this.createChangeSet(changeSetName, importExistingResources, revertDrift); + const changeSetDescription = changeSetReport.changeSet; await this.updateTerminationProtection(); if (changeSetHasNoChanges(changeSetDescription)) { @@ -502,20 +501,23 @@ class FullCloudFormationDeployment { } // If there are replacements in the changeset, check the rollback flag and stack status - return this.checkAndExecuteChangeSet(changeSetDescription); + return this.checkAndExecuteChangeSet(changeSetReport); } 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, - }); + // The change set was created by an earlier command (possibly not even by us). Require it to + // have completed rather than waiting on it: blocking on someone else's change set + // indefinitely would be worse than reporting that it isn't ready. + const changeSetReport = await new ChangeSetDescriber({ + cfn: this.cfn, + ioHelper: this.ioHelper, + stackNameOrArn: this.stackName, + changeSetNameOrArn: deploymentMethod.changeSetName, + }).describeForExecution({ diagnoser: this.diagnoser }); - return this.checkAndExecuteChangeSet(changeSetDescription); + return this.checkAndExecuteChangeSet(changeSetReport); } private deployConfig(): DeploymentConfig { @@ -528,39 +530,48 @@ class FullCloudFormationDeployment { /** * 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); + private async checkAndExecuteChangeSet(changeSetReport: ChangeSetReport): Promise { + const replacement = hasReplacement(changeSetReport); const isPausedFailState = this.cloudFormationStack.stackStatus.isRollbackable; const rollback = this.options.rollback ?? true; - // Route express mode deployments directly to executeChangeset, express mode stacks cannot use rollback API - if (this.options.express) { - return this.executeChangeSet(changeSetDescription); + // For express mode deployments, don't check paused and failed, since express mode stacks cannot use rollback API + if (!this.options.express) { + if (isPausedFailState && replacement) { + return { type: 'failpaused-need-rollback-first', reason: 'replacement', status: this.cloudFormationStack.stackStatus.name }; + } + if (isPausedFailState && rollback) { + return { type: 'failpaused-need-rollback-first', reason: 'not-norollback', status: this.cloudFormationStack.stackStatus.name }; + } + if (!rollback && replacement) { + return { type: 'replacement-requires-rollback' }; + } } - if (isPausedFailState && replacement) { - return { type: 'failpaused-need-rollback-first', reason: 'replacement', status: this.cloudFormationStack.stackStatus.name }; - } - if (isPausedFailState && rollback) { - return { type: 'failpaused-need-rollback-first', reason: 'not-norollback', status: this.cloudFormationStack.stackStatus.name }; - } - if (!rollback && replacement) { - return { type: 'replacement-requires-rollback' }; - } + const changeSet = changeSetReport.changeSet; + await this.ioHelper.defaults.debug(format('Initiating execution of changeset %s on stack %s', changeSet.ChangeSetId, this.stackName)); + + await this.cfn.executeChangeSet({ + StackName: changeSet.StackId ?? this.stackName, + ChangeSetName: changeSet.ChangeSetId!, + ClientRequestToken: `exec${this.uuid}`, + ...this.commonExecuteOptions(), + }); + + await this.ioHelper.defaults.debug( + format( + 'Execution of changeset %s on stack %s has started; waiting for the update to complete...', + changeSet.ChangeSetId, + this.stackName, + ), + ); - return this.executeChangeSet(changeSetDescription); + // +1 for the extra event emitted from updates. + const changeSetLength: number = (changeSet.Changes ?? []).length + (this.update ? 1 : 0); + return this.monitorDeployment(changeSet.CreationTime!, changeSet.StackId!, changeSetLength); } - private async createChangeSet(changeSetName: string, willExecute: boolean, importExistingResources: boolean, revertDrift: boolean) { + private async createChangeSet(changeSetName: string, importExistingResources: boolean, revertDrift: boolean): Promise { await this.cleanupOldChangeset(changeSetName); await this.ioHelper.defaults.debug(`Attempting to create ChangeSet with name ${changeSetName} to ${this.verb} stack ${this.stackName}`); @@ -580,35 +591,16 @@ class FullCloudFormationDeployment { }); await this.ioHelper.defaults.debug(format('Initiated creation of changeset: %s; waiting for it to finish creating...', changeSet.Id)); - return waitForChangeSet(this.cfn, this.ioHelper, changeSet.StackId ?? this.stackName, changeSet.Id ?? changeSetName, { - fetchAll: willExecute, + return new ChangeSetDescriber({ + cfn: this.cfn, + ioHelper: this.ioHelper, + stackNameOrArn: changeSet.StackId ?? this.stackName, + changeSetNameOrArn: changeSet.Id ?? changeSetName, + }).waitAndThrowOnProblem({ diagnoser: this.diagnoser, }); } - private async executeChangeSet(changeSet: DescribeChangeSetCommandOutput): Promise { - await this.ioHelper.defaults.debug(format('Initiating execution of changeset %s on stack %s', changeSet.ChangeSetId, this.stackName)); - - await this.cfn.executeChangeSet({ - StackName: changeSet.StackId ?? this.stackName, - ChangeSetName: changeSet.ChangeSetId!, - ClientRequestToken: `exec${this.uuid}`, - ...this.commonExecuteOptions(), - }); - - await this.ioHelper.defaults.debug( - format( - 'Execution of changeset %s on stack %s has started; waiting for the update to complete...', - changeSet.ChangeSetId, - this.stackName, - ), - ); - - // +1 for the extra event emitted from updates. - const changeSetLength: number = (changeSet.Changes ?? []).length + (this.update ? 1 : 0); - return this.monitorDeployment(changeSet.CreationTime!, changeSet.StackId!, changeSetLength); - } - private async cleanupOldChangeset(changeSetNameOrArn: string) { if (this.cloudFormationStack.exists) { const changeSetDisplayName = changeSetNameFromArn(changeSetNameOrArn); @@ -621,7 +613,12 @@ class FullCloudFormationDeployment { }); // Deleting may take a bit, especially if it involves nested stack change sets. Wait until it is gone. - await waitForChangeSetGone(this.cfn, this.ioHelper, this.stackName, changeSetNameOrArn); + await new ChangeSetDescriber({ + cfn: this.cfn, + ioHelper: this.ioHelper, + stackNameOrArn: this.stackName, + changeSetNameOrArn: changeSetNameOrArn, + }).waitForGone(); } } @@ -722,9 +719,7 @@ class FullCloudFormationDeployment { const diagnosis = await this.diagnoser.diagnoseFromErrorCollection(monitor.errors, finalState.wrapped, true, { rollbackEnabled: this.options.rollback !== false, }); - if (diagnosis.type !== 'no-problem') { - throwDeploymentErrorFromDiagnosis(diagnosis); - } + diagnosis.throwOnError(); } // Otherwise rethrow the current error and hope it has enough information. @@ -964,8 +959,8 @@ function arrayEquals(a: any[], b: any[]): boolean { return a.every((item) => b.includes(item)) && b.every((item) => a.includes(item)); } -function hasReplacement(cs: DescribeChangeSetCommandOutput) { - return (cs.Changes ?? []).some(c => { +function hasReplacement(report: ChangeSetReport) { + return (report.changeSet.Changes ?? []).some(c => { const a = c.ResourceChange?.PolicyAction; return a === 'ReplaceAndDelete' || a === 'ReplaceAndRetain' || a === 'ReplaceAndSnapshot'; }); 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 b83a05eb8..0363920a9 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deployments.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/deployments/deployments.ts @@ -1,7 +1,6 @@ import { randomUUID } from 'node:crypto'; import * as cdk_assets from '@aws-cdk/cdk-assets-lib'; import type * as cxapi from '@aws-cdk/cloud-assembly-api'; -import type { DescribeChangeSetCommandOutput } from '@aws-sdk/client-cloudformation'; import chalk from 'chalk'; import { AssetManifestBuilder } from './asset-manifest-builder'; import { @@ -11,7 +10,6 @@ import { import { stabilizeStack, uploadStackTemplateAssets, - waitForChangeSet, waitForStackDelete, } from './cfn-api'; import { determineAllowCrossAccountAssetPublishing } from './checks'; @@ -23,6 +21,8 @@ import { DEFAULT_DEPLOY_CHANGE_SET_NAME } from '../../actions/deploy/private/dep import { DeploymentError, ToolkitError } from '../../toolkit/toolkit-error'; import { formatErrorMessage } from '../../util'; import type { SdkProvider } from '../aws-auth/private'; +import type { ChangeSetReport } from '../change-sets'; +import { ChangeSetDescriber } from '../change-sets'; import type { Template, RootTemplateWithNestedStacks, @@ -508,11 +508,15 @@ export class Deployments { stack: cxapi.CloudFormationStackArtifact, changeSetName: string, stackArn?: string, - ): Promise { + ): Promise { const env = await this.envs.accessStackForMutableStackOperations(stack); const cfn = env.sdk.cloudFormation(); - return waitForChangeSet(cfn, this.ioHelper, stackArn ?? stack.stackName, changeSetName, { - fetchAll: true, + return new ChangeSetDescriber({ + cfn, + ioHelper: this.ioHelper, + stackNameOrArn: stackArn ?? stack.stackName, + changeSetNameOrArn: changeSetName, + }).waitAndThrowOnProblem({ diagnoser: new CloudFormationStackDiagnoser({ sdk: env.sdk, envResources: env.resources, diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/diagnosing/diagnosis-formatting.ts b/packages/@aws-cdk/toolkit-lib/lib/api/diagnosing/diagnosis-formatting.ts index 63feb860a..1b0e8df24 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/diagnosing/diagnosis-formatting.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/diagnosing/diagnosis-formatting.ts @@ -1,6 +1,6 @@ +import { ChangeSetStatus } from '@aws-sdk/client-cloudformation'; import { sideBySide, wrapText } from './format-utils'; import type { DiagnosedStack, StackDiagnosis, StackProblemSource, TracedResourceError } from '../../actions/diagnose'; -import { DeploymentError, ToolkitError } from '../../toolkit/toolkit-error'; import { sortByKey } from '../../util'; import type { ActionLessMessage } from '../io/private'; import { IO } from '../io/private'; @@ -26,42 +26,16 @@ export function hostMessageFromDiagnosis(stack: DiagnosedStack): ActionLessMessa } /** - * Turn the given diagnosis into a DeploymentError + * Render the problem diagnosis as a human readable message */ -export function throwDeploymentErrorFromDiagnosis(diag: StackDiagnosis): never { - switch (diag.type) { - case 'no-problem': - throw new ToolkitError('DeploymentErrorNotError', 'Diagnosis should represent an error, but does not'); - - case 'error-diagnosing': - throw new DeploymentError(diag.message, 'ErrorDiagnosisFailed'); - } - // Guaranteed 'type=problem' here - - const errorCode = diag.problems[0]?.errorCode; - let defaultErrorCode; - switch (diag.detectedBy.type) { - case 'change-set': - defaultErrorCode = 'ChangeSetCreationFailed'; - break; - - case 'early-validation': - defaultErrorCode = 'EarlyValidationFailure'; - break; - - case 'deployment': - defaultErrorCode = 'StackDeployFailed'; - break; - } - - throw new DeploymentError(formatProblemDiagnosis(diag), errorCode ?? defaultErrorCode); -} - -function formatProblemDiagnosis(diag: Extract): string { +export function formatProblemDiagnosis(diag: Extract): string { switch (diag.detectedBy.type) { case 'change-set': return formatChangeSetProblems(diag.problems, diag.detectedBy); + case 'change-set-not-ready': + return formatChangeSetNotReady(diag.detectedBy); + case 'early-validation': return formatEarlyValidationProblems(diag.problems, diag.detectedBy); @@ -80,6 +54,20 @@ function formatChangeSetProblems(problems: TracedResourceError[], detectedBy: Ex } } +function formatChangeSetNotReady(detectedBy: Extract): string { + const pending: string[] = [ChangeSetStatus.CREATE_PENDING, ChangeSetStatus.CREATE_IN_PROGRESS]; + if (pending.includes(detectedBy.changeSetStatus)) { + return `Change set ${detectedBy.changeSetName} is still being created (${detectedBy.changeSetStatus}). Try again when it's finished.`; + } + + const caption = `Change set ${detectedBy.changeSetName} cannot be executed (${detectedBy.changeSetStatus})`; + if (detectedBy.statusReason) { + return `${caption}: ${detectedBy.statusReason}`; + } else { + return caption; + } +} + function formatEarlyValidationProblems(problems: TracedResourceError[], detectedBy: Extract): string { const caption = `Early validation failed for change set ${detectedBy.changeSetName}`; if (problems.length > 0) { diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/diagnosing/diagnosis.ts b/packages/@aws-cdk/toolkit-lib/lib/api/diagnosing/diagnosis.ts new file mode 100644 index 000000000..1a8ed58b5 --- /dev/null +++ b/packages/@aws-cdk/toolkit-lib/lib/api/diagnosing/diagnosis.ts @@ -0,0 +1,85 @@ +import { formatProblemDiagnosis } from './diagnosis-formatting'; +import type { StackDiagnosis, StackProblemSource, TracedResourceError } from '../../actions/diagnose'; +import { DeploymentError } from '../../toolkit/toolkit-error'; + +/** + * The outcome of diagnosing a stack, and what to do about it. + * + * This is the internal counterpart of the public `StackDiagnosis` type: it carries the same + * information (available as `result`, which is what we hand out at the API boundary) plus the + * behavior that belongs with it. + */ +export class Diagnosis { + /** + * No problem was found with the stack. + */ + public static noProblem(): Diagnosis { + return new Diagnosis({ type: 'no-problem' }); + } + + /** + * A problem was found with the stack. + */ + public static problem(detectedBy: StackProblemSource, problems: TracedResourceError[]): Diagnosis { + return new Diagnosis({ type: 'problem', detectedBy, problems }); + } + + /** + * Diagnosing the stack itself failed, so we cannot say whether there is a problem. + */ + public static errorDiagnosing(message: string): Diagnosis { + return new Diagnosis({ type: 'error-diagnosing', message }); + } + + private constructor(public readonly result: StackDiagnosis) { + } + + public get type(): StackDiagnosis['type'] { + return this.result.type; + } + + /** + * Whether this diagnosis represents a healthy stack. + */ + public get isNoProblem(): boolean { + return this.result.type === 'no-problem'; + } + + /** + * Throw a `DeploymentError` describing the diagnosis, unless there is no problem. + */ + public throwOnError(): void { + switch (this.result.type) { + case 'no-problem': + return; + + case 'error-diagnosing': + throw new DeploymentError(this.result.message, 'ErrorDiagnosisFailed'); + + case 'problem': + break; + } + + const errorCode = this.result.problems[0]?.errorCode; + let defaultErrorCode; + switch (this.result.detectedBy.type) { + case 'change-set': + defaultErrorCode = 'ChangeSetCreationFailed'; + break; + + case 'change-set-not-ready': + defaultErrorCode = 'ChangeSetNotReady'; + break; + + case 'early-validation': + defaultErrorCode = 'EarlyValidationFailure'; + break; + + case 'deployment': + defaultErrorCode = 'StackDeployFailed'; + break; + } + + throw new DeploymentError(formatProblemDiagnosis(this.result), errorCode ?? defaultErrorCode); + } +} diff --git a/packages/@aws-cdk/toolkit-lib/lib/api/diagnosing/stack-diagnoser.ts b/packages/@aws-cdk/toolkit-lib/lib/api/diagnosing/stack-diagnoser.ts index ae7e4d96f..e66cd37e7 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/api/diagnosing/stack-diagnoser.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/api/diagnosing/stack-diagnoser.ts @@ -3,9 +3,11 @@ import { ChangeSetStatus, ChangeType } from '@aws-sdk/client-cloudformation'; import type { ChangeSetResourceError } from './changeset-error-fetcher'; import { ChangeSetResourceErrorFetcher } from './changeset-error-fetcher'; import { attributedCloudTrailContexts, investigateStackViaCloudTrail } from './cloudtrail-investigation'; +import { Diagnosis } from './diagnosis'; import { investigateResource } from './resource-investigation'; -import type { AdditionalDiagnosticContext, StackDiagnosis, StackProblemSource, TracedResourceError } from '../../actions/diagnose'; +import type { AdditionalDiagnosticContext, StackProblemSource, TracedResourceError } from '../../actions/diagnose'; import type { ICloudFormationClient, SDK } from '../aws-auth/sdk'; +import { ChangeSetDescriber } from '../change-sets'; import type { EnvironmentResources } from '../environment'; import type { IoHelper } from '../io/private/io-helper'; import type { ISourceTracer } from '../source-tracing/private/source-tracing'; @@ -67,6 +69,20 @@ export interface DiagnoseOptions { readonly rollbackEnabled?: boolean; } +export interface DiagnoseChangeSetOptions { + /** + * Report a change set that cannot be executed as a problem. + * + * By default only a change set that failed to create is a problem: one that contains no changes, + * or that has since been deleted, is reported as healthy because for `deploy` those are normal + * outcomes. Set this when you are about to execute the change set, so that anything other than + * `CREATE_COMPLETE` is reported as a problem instead. + * + * @default false + */ + readonly requireExecutable?: boolean; +} + export class CloudFormationStackDiagnoser { private readonly cfn: ICloudFormationClient; private parentStackLogicalIds: string[]; @@ -81,23 +97,17 @@ export class CloudFormationStackDiagnoser { /** * Diagnose a stack's root cause given no pre-existing state */ - public async diagnoseFromFresh(stackName: string): Promise { + public async diagnoseFromFresh(stackName: string): Promise { try { const response = await this.cfn.describeStacks({ StackName: stackName }); const stack = response.Stacks?.[0]; if (!stack) { - return { - type: 'error-diagnosing', - message: `Stack with name ${stackName} not found`, - }; + return Diagnosis.errorDiagnosing(`Stack with name ${stackName} not found`); } const status = StackStatus.fromStackDescription(stack); if (status.isInProgress) { - return { - type: 'error-diagnosing', - message: `Stack with name ${stackName} is currently being updated (${status.name}). Try again when it's finished.`, - }; + return Diagnosis.errorDiagnosing(`Stack with name ${stackName} is currently being updated (${status.name}). Try again when it's finished.`); } if (status.isFailure || status.isRollbackSuccess) { @@ -106,18 +116,18 @@ export class CloudFormationStackDiagnoser { return await this._diagnoseChangeSetFailureFromStackName(stack); } catch (e: any) { - return { type: 'error-diagnosing', message: e.message }; + return Diagnosis.errorDiagnosing(e.message); } } /** * Diagnose potential problems with the change set */ - public async diagnoseChangeSet(changeSet: ChangeSetSummary): Promise { + public async diagnoseChangeSet(changeSet: ChangeSetSummary, options: DiagnoseChangeSetOptions = {}): Promise { try { - return await this._diagnoseChangeSetFailure(changeSet); + return await this._diagnoseChangeSetFailure(changeSet, options); } catch (e: any) { - return { type: 'error-diagnosing', message: e.message }; + return Diagnosis.errorDiagnosing(e.message); } } @@ -129,7 +139,7 @@ export class CloudFormationStackDiagnoser { stack: Stack, allowFallback = true, options: DiagnoseOptions = {}, - ): Promise { + ): Promise { this.rollbackEnabled = options.rollbackEnabled ?? true; if (errors.isEmpty()) { if (allowFallback) { @@ -141,18 +151,17 @@ export class CloudFormationStackDiagnoser { await this.props.ioHelper.defaults.debug(`Fallback diagnosis failed: ${e.message}`); } } - return { type: 'no-problem' }; + return Diagnosis.noProblem(); } - return { - type: 'problem', - detectedBy: { + return Diagnosis.problem( + { type: 'deployment', stackStatus: stack.StackStatus ?? '', statusReason: stack.StackStatusReason ?? '', }, - problems: await this.addCloudTrailContext(await this.enhanceErrors(errors.all), errors.all), - }; + await this.addCloudTrailContext(await this.enhanceErrors(errors.all), errors.all), + ); } /** @@ -220,7 +229,7 @@ export class CloudFormationStackDiagnoser { * * This is the same logic that the deployment monitor uses. */ - private async _diagnoseViaStackEvents(stack: Stack): Promise { + private async _diagnoseViaStackEvents(stack: Stack): Promise { const poller = new StackEventPoller(this.cfn, { stackArn: stack.StackId!, initialPollRange: PollRange.mostRecentDeploymentAttempt(), @@ -233,22 +242,19 @@ export class CloudFormationStackDiagnoser { return this.diagnoseFromErrorCollection(poller.errors, stack, false, { rollbackEnabled: this.rollbackEnabled }); } - private async _diagnoseChangeSetFailureFromStackName(stack: Stack): Promise { + private async _diagnoseChangeSetFailureFromStackName(stack: Stack): Promise { const cs = (await this.cfn.listChangeSets({ StackName: stack.StackId!, })).Summaries ?? []; const pending = cs.filter(x => x.Status === ChangeSetStatus.CREATE_IN_PROGRESS || x.Status === ChangeSetStatus.CREATE_PENDING); if (pending.length > 0) { - return { - type: 'error-diagnosing', - message: `Stack with name ${stack.StackName} has change sets currently being created (${pending[0].ChangeSetName}). Try again when it's finished.`, - }; + return Diagnosis.errorDiagnosing(`Stack with name ${stack.StackName} has change sets currently being created (${pending[0].ChangeSetName}). Try again when it's finished.`); } const failed = cs.filter(x => x.Status === ChangeSetStatus.FAILED); if (failed.length === 0) { - return { type: 'no-problem' }; + return Diagnosis.noProblem(); } return this._diagnoseChangeSetFailure(failed[0]); @@ -262,14 +268,32 @@ export class CloudFormationStackDiagnoser { * Usually this starts from trying to detect an error message pattern in the change set status reason, * and then potentially going to fetch additional information using additional API calls. */ - private async _diagnoseChangeSetFailure(changeSet: ChangeSetSummary): Promise { + private async _diagnoseChangeSetFailure(changeSet: ChangeSetSummary, options: DiagnoseChangeSetOptions = {}): Promise { + const diagnosis = await this._diagnoseChangeSetProblems(changeSet); + + // Nothing is wrong with the change set, but it still cannot be executed: it is either still + // being created, contains no changes, or has been deleted. (A change set that *did* fail keeps + // its own diagnosis, which explains the failure in much more detail than we could here.) + if (options.requireExecutable && diagnosis.isNoProblem && changeSet.Status !== ChangeSetStatus.CREATE_COMPLETE) { + return Diagnosis.problem({ + type: 'change-set-not-ready', + changeSetName: changeSet.ChangeSetName ?? '', + changeSetStatus: changeSet.Status ?? 'UNKNOWN', + statusReason: changeSet.StatusReason ?? '', + }, []); + } + + return diagnosis; + } + + private async _diagnoseChangeSetProblems(changeSet: ChangeSetSummary): Promise { if (changeSet.Status !== ChangeSetStatus.FAILED) { - return { type: 'no-problem' }; + return Diagnosis.noProblem(); } if (changeSetHasNoChanges(changeSet)) { // This will lead to a change set that is FAILED but it's not actually a problem - return { type: 'no-problem' }; + return Diagnosis.noProblem(); } // We report early validation errors differently than generic changeset errors. Mostly @@ -291,16 +315,15 @@ export class CloudFormationStackDiagnoser { // status reason, and nowhere else. We parse it specifically. const failedAutoErrors = this._tryDetectFailedAutoImport(changeSet); if (failedAutoErrors) { - return { - type: 'problem', - detectedBy: { + return Diagnosis.problem( + { type: 'change-set', changeSetStatus: changeSet.Status ?? '', changeSetName: changeSet.ChangeSetName ?? '', statusReason: changeSet.StatusReason ?? '', }, - problems: await this.enhanceErrors(failedAutoErrors), - }; + await this.enhanceErrors(failedAutoErrors), + ); } // Otherwise, a generic change set creation error where `DescribeEvents` @@ -320,17 +343,16 @@ export class CloudFormationStackDiagnoser { * the change set was failed by CloudFormation Hooks and report those. Otherwise, return a * generic error. */ - private async _reportChangeSetFailureFromEvents(changeSet: ChangeSetSummary, detectedBy: StackProblemSource): Promise { + private async _reportChangeSetFailureFromEvents(changeSet: ChangeSetSummary, detectedBy: StackProblemSource): Promise { const ev = await new ChangeSetResourceErrorFetcher(this.props.sdk, this.props.envResources) .fetchDetailsStructured(changeSet.ChangeSetName!, changeSet.StackName!); // If we have errors to return, return them if (ev.type === 'resource-errors' && ev.errors.length > 0) { - return { - type: 'problem', + return Diagnosis.problem( detectedBy, - problems: await this.enhanceErrors(ev.errors.map((e) => resourceErrorFromEarlyValidationError(changeSet.StackId ?? '', this.parentStackLogicalIds, e))), - }; + await this.enhanceErrors(ev.errors.map((e) => resourceErrorFromEarlyValidationError(changeSet.StackId ?? '', this.parentStackLogicalIds, e))), + ); } // Otherwise, we will return a generic changeset error message. If there was a problem @@ -344,11 +366,7 @@ export class CloudFormationStackDiagnoser { // details are only available through the hook results APIs. const hookErrors = await this._changeSetHookErrors(changeSet); if (hookErrors.length > 0) { - return { - type: 'problem', - detectedBy, - problems: await this.enhanceErrors(hookErrors), - }; + return Diagnosis.problem(detectedBy, await this.enhanceErrors(hookErrors)); } return this._nonSpecificChangeSetError(changeSet, detectedBy); @@ -458,28 +476,24 @@ export class CloudFormationStackDiagnoser { * * We can't point to a specific resource. */ - private async _nonSpecificChangeSetError(changeSet: ChangeSetSummary, detectedBy: StackProblemSource): Promise { - return { - type: 'problem', - detectedBy, - problems: [ - await this.enhanceError({ - // It's about a stack - logicalId: undefined, - message: changeSet.StatusReason ?? '', - parentStackLogicalIds: this.parentStackLogicalIds, - stackArn: changeSet.StackId ?? '', - physicalId: changeSet.StackId, - resourceType: 'AWS::CloudFormation::Stack', - }), - ], - }; + private async _nonSpecificChangeSetError(changeSet: ChangeSetSummary, detectedBy: StackProblemSource): Promise { + return Diagnosis.problem(detectedBy, [ + await this.enhanceError({ + // It's about a stack + logicalId: undefined, + message: changeSet.StatusReason ?? '', + parentStackLogicalIds: this.parentStackLogicalIds, + stackArn: changeSet.StackId ?? '', + physicalId: changeSet.StackId, + resourceType: 'AWS::CloudFormation::Stack', + }), + ]); } /** * Look for nested change sets that have failed, and diagnose those. */ - private async _diagnoseNestedChangeSetFailure(changeSet: ChangeSetSummary): Promise { + private async _diagnoseNestedChangeSetFailure(changeSet: ChangeSetSummary): Promise { const nested = await this._findFailedNestedStack(changeSet); if (!nested) { // That's weird. Let's return the change set's status reason as a non-specific error @@ -491,10 +505,12 @@ export class CloudFormationStackDiagnoser { }); } - const nestedCs = await this.cfn.describeChangeSet({ - ChangeSetName: nested.changeSetArn, - StackName: nested.stackArn, - }); + const nestedCs = await new ChangeSetDescriber({ + cfn: this.cfn, + ioHelper: this.props.ioHelper, + stackNameOrArn: nested.stackArn, + changeSetNameOrArn: nested.changeSetArn, + }).describeCurrentState(); const nestedDiag = new CloudFormationStackDiagnoser(this.props); nestedDiag.parentStackLogicalIds = [...this.parentStackLogicalIds, nested.logicalId]; @@ -509,27 +525,22 @@ export class CloudFormationStackDiagnoser { // The status reason only includes the change set ID, but we also need the stack name. The way to get this is // describe the current change set, then from the Changes find the stack whose ChangeSetId is mentioned in the // status reason, then look up that change set and recurse into a regular change set diagnosis. - let nextToken = undefined; - do { - // Changes in this response might be paginated - const resp = await this.cfn.describeChangeSet({ - StackName: changeSet.StackId, - ChangeSetName: changeSet.ChangeSetId, - ...nextToken ? { NextToken: nextToken } : {}, - }); - - for (const change of resp.Changes ?? []) { - if (change.Type === ChangeType.Resource && change.ResourceChange?.ResourceType === 'AWS::CloudFormation::Stack' && change.ResourceChange?.ChangeSetId && changeSet.StatusReason?.includes(change.ResourceChange?.ChangeSetId)) { - return { - changeSetArn: change.ResourceChange.ChangeSetId, - stackArn: change.ResourceChange.PhysicalResourceId ?? '', - logicalId: change.ResourceChange.LogicalResourceId ?? '', - }; - } + const resp = await new ChangeSetDescriber({ + cfn: this.cfn, + ioHelper: this.props.ioHelper, + stackNameOrArn: changeSet.StackId!, + changeSetNameOrArn: changeSet.ChangeSetId!, + }).describeCurrentState(); + + for (const change of resp.Changes ?? []) { + if (change.Type === ChangeType.Resource && change.ResourceChange?.ResourceType === 'AWS::CloudFormation::Stack' && change.ResourceChange?.ChangeSetId && changeSet.StatusReason?.includes(change.ResourceChange?.ChangeSetId)) { + return { + changeSetArn: change.ResourceChange.ChangeSetId, + stackArn: change.ResourceChange.PhysicalResourceId ?? '', + logicalId: change.ResourceChange.LogicalResourceId ?? '', + }; } - - nextToken = resp.NextToken; - } while (nextToken); + } return undefined; } diff --git a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts index 7f9f02132..48726e89e 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/toolkit/toolkit.ts @@ -654,7 +654,7 @@ export class Toolkit extends CloudAssemblySourceBuilder { const ret: DiagnosedStack = { stackName: stack.stackName, hierarchicalId: stack.hierarchicalId, - result: diagnosis, + result: diagnosis.result, }; await this.ioHost.notify({ @@ -733,12 +733,12 @@ export class Toolkit extends CloudAssemblySourceBuilder { stack, parameters: {}, uuid: randomUUID(), - willExecute: false, failOnError: true, }); - if (report.diagnosis.type === 'problem') { - for (const problem of report.diagnosis.problems) { + const diagnosis = report.diagnosis.result; + if (diagnosis.type === 'problem') { + for (const problem of diagnosis.problems) { violations.push({ ruleName: problem.errorCode ?? 'CloudFormationValidation', description: problem.message.replace(/\s*\(at\s+\/Resources\/[^)]+\)\s*$/, ''), @@ -920,7 +920,7 @@ export class Toolkit extends CloudAssemblySourceBuilder { if (!prepareResult?.noOp) { // For execute-change-set, describe the existing change set so we can show an accurate diff const diffChangeSet = isExecuteChangeSetDeployment(options.deploymentMethod) - ? await deployments.describeChangeSet(stack, options.deploymentMethod.changeSetName, prepareResult?.stackArn) + ? (await deployments.describeChangeSet(stack, options.deploymentMethod.changeSetName, prepareResult?.stackArn)).changeSet : prepareResult?.changeSet; const formatter = new DiffFormatter({ diff --git a/packages/@aws-cdk/toolkit-lib/lib/util/promises.ts b/packages/@aws-cdk/toolkit-lib/lib/util/promises.ts index 58a62713a..50ceb5cfa 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/util/promises.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/util/promises.ts @@ -17,3 +17,26 @@ interface PromiseAndResolvers { resolve: (value: A) => void; reject: (reason: any) => void; } + +/** + * Waits for a function to return non-+undefined+ before returning. + * + * @param valueProvider - a function that will return a value that is not +undefined+ once the wait should be over + * @param timeout - the time to wait between two calls to +valueProvider+ + * + * @returns the value that was returned by +valueProvider+ + */ +export async function waitFor( + valueProvider: () => Promise, + timeout: number = 5000, +): Promise { + while (true) { + const result = await valueProvider(); + if (result === null) { + return undefined; + } else if (result !== undefined) { + return result; + } + await new Promise((cb) => setTimeout(cb, timeout)); + } +} diff --git a/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts b/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts index d0d17fce9..421f14dfd 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts @@ -6,6 +6,7 @@ import { DiffMethod } from '../../lib/actions/diff'; import { StackSelectionStrategy } from '../../lib/api/cloud-assembly'; import * as deployments from '../../lib/api/deployments'; import * as cfnApi from '../../lib/api/deployments/cfn-api'; +import { Diagnosis } from '../../lib/api/diagnosing/diagnosis'; import { Toolkit } from '../../lib/toolkit'; import { cdkOutFixture, disposableCloudAssemblySource, TestIoHost } from '../_helpers'; import { mockCloudFormationClient, mockSSMClient, mockSdkProvider, restoreSdkMocksToDefault, setDefaultSTSMocks } from '../_helpers/mock-sdk'; @@ -405,15 +406,18 @@ describe('diff', () => { // Setup mock BEFORE calling the function const createDiffChangeSetMock = jest.spyOn(cfnApi, 'createDiffChangeSet').mockImplementationOnce(async () => { return { - $metadata: {}, - Changes: [ - { - ResourceChange: { - Action: 'Import', - LogicalResourceId: 'MyBucketF68F3FF0', + changeSet: { + $metadata: {}, + Changes: [ + { + ResourceChange: { + Action: 'Import', + LogicalResourceId: 'MyBucketF68F3FF0', + }, }, - }, - ], + ], + }, + diagnosis: Diagnosis.noProblem(), }; }); @@ -518,6 +522,81 @@ describe('diff', () => { expect(describeCalls.some(call => (call.args[0].input as any).IncludePropertyValues === true)).toBe(true); }); + test('ChangeSet diff keeps changes that CloudFormation drops from the IncludePropertyValues description (issue #1780)', async () => { + // GIVEN - an existing stack whose deployed template differs from the new template + jest.spyOn(deployments.Deployments.prototype, 'stackExists').mockResolvedValue(true); + jest.spyOn(deployments.Deployments.prototype, 'readCurrentTemplateWithNestedStacks').mockResolvedValue({ + deployedRootTemplate: { + Resources: { + MyBucketF68F3FF0: { + Type: 'AWS::S3::Bucket', + Properties: { BucketName: 'old-name' }, + UpdateReplacePolicy: 'Retain', + DeletionPolicy: 'Retain', + Metadata: { 'aws:cdk:path': 'Stack1/MyBucket/Resource' }, + }, + }, + }, + nestedStacks: [] as any, + }); + mockCloudFormationClient.on(DescribeStacksCommand).resolves({ + Stacks: [{ StackName: 'Stack1', StackStatus: 'CREATE_COMPLETE', CreationTime: new Date() }], + }); + mockSSMClient.on(GetParameterCommand).resolves({ Parameter: { Value: '99' } }); + mockCloudFormationClient.on(CreateChangeSetCommand).resolves({ Id: 'arn:aws:cloudformation:us-east-1:123456789012:changeSet/cdk-diff' }); + + // CloudFormation drops the ResourceChange from the IncludePropertyValues response... + mockCloudFormationClient.on(DescribeChangeSetCommand, { IncludePropertyValues: true }).resolves({ + Status: 'CREATE_COMPLETE', + StatusReason: '[WARN] --include-property-values option can return incomplete ChangeSet data because: Logical Id: MyBucketF68F3FF0, failed property validation', + Changes: [], + }); + // ...while the plain response still reports the change + mockCloudFormationClient.on(DescribeChangeSetCommand, { IncludePropertyValues: false }).resolves({ + Status: 'CREATE_COMPLETE', + Changes: [ + { + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: 'MyBucketF68F3FF0', + ResourceType: 'AWS::S3::Bucket', + Replacement: 'True', + Scope: ['Properties'], + Details: [{ + Target: { Attribute: 'Properties', Name: 'BucketName', RequiresRecreation: 'Always' }, + Evaluation: 'Static', + ChangeSource: 'DirectModification', + }], + }, + }, + ], + }); + + // WHEN + const cx = await cdkOutFixture(toolkit, 'stack-with-bucket'); + const result = await toolkit.diff(cx, { + stacks: { strategy: StackSelectionStrategy.ALL_STACKS }, + method: DiffMethod.ChangeSet({ fallbackToTemplate: false }), + }); + + // THEN - the change set was described both with and without property values + const describeCalls = mockCloudFormationClient.commandCalls(DescribeChangeSetCommand); + expect(describeCalls.some(call => (call.args[0].input as any).IncludePropertyValues === true)).toBe(true); + expect(describeCalls.some(call => (call.args[0].input as any).IncludePropertyValues === false)).toBe(true); + + // THEN - the property change is not erased from the diff + const bucketChange = result.Stack1.resources.get('MyBucketF68F3FF0'); + expect(bucketChange.isDifferent).toBe(true); + expect(bucketChange.propertyUpdates.BucketName.isDifferent).toBe(true); + expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({ + action: 'diff', + level: 'result', + code: 'CDK_TOOLKIT_I4001', + message: expect.stringContaining('✨ Number of stacks with differences: 1'), + })); + }); + test('ChangeSet diff treats DELETE_IN_PROGRESS stack as non-existent', async () => { // GIVEN - stack is in DELETE_IN_PROGRESS (from a previous diff cleanup) jest.spyOn(deployments.Deployments.prototype, 'stackExists').mockResolvedValue(true); diff --git a/packages/@aws-cdk/toolkit-lib/test/actions/validate.test.ts b/packages/@aws-cdk/toolkit-lib/test/actions/validate.test.ts index 9f9a291e8..cc67355bd 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/validate.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/validate.test.ts @@ -1,6 +1,7 @@ import * as awsauth from '../../lib/api/aws-auth/private'; import { StackSelectionStrategy } from '../../lib/api/cloud-assembly'; import * as cfnApi from '../../lib/api/deployments/cfn-api'; +import { Diagnosis } from '../../lib/api/diagnosing/diagnosis'; import { Toolkit } from '../../lib/toolkit'; import { cdkOutFixture, TestIoHost } from '../_helpers'; import { MockSdk, restoreSdkMocksToDefault, setDefaultSTSMocks } from '../_helpers/mock-sdk'; @@ -131,11 +132,10 @@ describe('validate --online', () => { test('reports CloudFormation validation errors as a plugin report', async () => { jest.spyOn(cfnApi, 'createValidationChangeSet').mockResolvedValue({ - description: { $metadata: {} } as any, - diagnosis: { - type: 'problem', - detectedBy: { type: 'early-validation', changeSetName: 'cdk-validate-change-set' }, - problems: [ + changeSet: { $metadata: {} } as any, + diagnosis: Diagnosis.problem( + { type: 'early-validation', changeSetName: 'cdk-validate-change-set' }, + [ { stackArn: 'arn:aws:cloudformation:us-east-1:123456789012:stack/Stack1', topLevelStackHierarchicalId: 'Stack1', @@ -147,7 +147,7 @@ describe('validate --online', () => { sourceTrace: undefined, }, ], - }, + ), }); const cx = await cdkOutFixture(toolkit, 'stack-with-bucket'); @@ -165,8 +165,8 @@ describe('validate --online', () => { test('passes when CloudFormation finds no problems', async () => { jest.spyOn(cfnApi, 'createValidationChangeSet').mockResolvedValue({ - description: { $metadata: {} } as any, - diagnosis: { type: 'no-problem' }, + changeSet: { $metadata: {} } as any, + diagnosis: Diagnosis.noProblem(), }); const cx = await cdkOutFixture(toolkit, 'stack-with-bucket'); @@ -178,11 +178,10 @@ describe('validate --online', () => { test('merges offline and online results', async () => { jest.spyOn(cfnApi, 'createValidationChangeSet').mockResolvedValue({ - description: { $metadata: {} } as any, - diagnosis: { - type: 'problem', - detectedBy: { type: 'early-validation', changeSetName: 'cdk-validate-change-set' }, - problems: [ + changeSet: { $metadata: {} } as any, + diagnosis: Diagnosis.problem( + { type: 'early-validation', changeSetName: 'cdk-validate-change-set' }, + [ { stackArn: 'arn:aws:cloudformation:us-east-1:123456789012:stack/Stack1', topLevelStackHierarchicalId: 'Stack1', @@ -193,7 +192,7 @@ describe('validate --online', () => { sourceTrace: undefined, }, ], - }, + ), }); const cx = await cdkOutFixture(toolkit, 'stack-with-validation-report'); @@ -209,8 +208,8 @@ describe('validate --online', () => { test('runs online validation by default when no options provided', async () => { const spy = jest.spyOn(cfnApi, 'createValidationChangeSet').mockResolvedValue({ - description: { $metadata: {} } as any, - diagnosis: { type: 'no-problem' }, + changeSet: { $metadata: {} } as any, + diagnosis: Diagnosis.noProblem(), }); const cx = await cdkOutFixture(toolkit, 'stack-with-bucket'); diff --git a/packages/@aws-cdk/toolkit-lib/test/api/change-sets/change-set-describer.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/change-sets/change-set-describer.test.ts new file mode 100644 index 000000000..f9cf193a0 --- /dev/null +++ b/packages/@aws-cdk/toolkit-lib/test/api/change-sets/change-set-describer.test.ts @@ -0,0 +1,351 @@ +import type { DescribeChangeSetCommandOutput } from '@aws-sdk/client-cloudformation'; +import { DescribeChangeSetCommand } from '@aws-sdk/client-cloudformation'; +import type { ICloudFormationClient } from '../../../lib/api/aws-auth/private'; +import { ChangeSetDescriber } from '../../../lib/api/change-sets'; +import type { CloudFormationStackDiagnoser } from '../../../lib/api/diagnosing/stack-diagnoser'; +import { MockSdk, mockCloudFormationClient, restoreSdkMocksToDefault } from '../../_helpers/mock-sdk'; +import { TestIoHost } from '../../_helpers/test-io-host'; + +const ioHost = new TestIoHost(); +const ioHelper = ioHost.asHelper('diff'); +const diagnoser = { diagnoseChangeSet: async () => ({ type: 'no-problem' }) } as unknown as CloudFormationStackDiagnoser; + +/** + * The warning CloudFormation adds to `StatusReason` when it dropped data from a response that was + * requested with `IncludePropertyValues`. See https://github.com/aws/aws-cdk-cli/issues/1780 + */ +const INCOMPLETE_DATA_WARNING = '[WARN] --include-property-values option can return incomplete ChangeSet data because: Logical Id: DataSource, failed property validation'; + +let cfn: ICloudFormationClient; + +beforeEach(() => { + restoreSdkMocksToDefault(); + cfn = new MockSdk().cloudFormation(); +}); + +function describer() { + return new ChangeSetDescriber({ + cfn, + ioHelper, + stackNameOrArn: 'my-stack', + changeSetNameOrArn: 'my-change-set', + }); +} + +/** + * Make DescribeChangeSet return `detailed` when property values are requested, and `plain` otherwise. + * + * The detailed response carries the incomplete-data warning, so that the describer goes looking for + * what CloudFormation dropped. + */ +function mockFlaggedResponses(detailed: Partial, plain: Partial) { + mockCloudFormationClient.on(DescribeChangeSetCommand, { IncludePropertyValues: true }).resolves({ + Status: 'CREATE_COMPLETE', + StatusReason: INCOMPLETE_DATA_WARNING, + ...detailed, + }); + mockCloudFormationClient.on(DescribeChangeSetCommand, { IncludePropertyValues: false }).resolves({ Status: 'CREATE_COMPLETE', ...plain }); +} + +function plainDescribeCalls() { + return mockCloudFormationClient.commandCalls(DescribeChangeSetCommand) + .filter((call) => call.args[0].input.IncludePropertyValues === false); +} + +describe('ChangeSetDescriber', () => { + test('restores a resource change that CloudFormation dropped from the detailed response', async () => { + // GIVEN - https://github.com/aws/aws-cdk-cli/issues/1780: with IncludePropertyValues, + // CFN drops the ResourceChange, leaving only a WARN in StatusReason + mockFlaggedResponses( + { + Changes: [], + }, + { + Changes: [{ + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: 'DataSource', + ResourceType: 'AWS::QBusiness::DataSource', + Replacement: 'False', + Scope: ['Properties'], + Details: [{ + Target: { Attribute: 'Properties', Name: 'Configuration', RequiresRecreation: 'Never' }, + Evaluation: 'Static', + ChangeSource: 'DirectModification', + }], + }, + }], + }, + ); + + // WHEN + const { changeSet: description } = await describer().waitForReport({ diagnoser }); + + // THEN + expect(description.Changes).toHaveLength(1); + expect(description.Changes?.[0].ResourceChange?.LogicalResourceId).toEqual('DataSource'); + expect(description.Changes?.[0].ResourceChange?.Details?.[0].Target?.Name).toEqual('Configuration'); + }); + + test('keeps the real status reason rather than the incomplete-data warning', async () => { + // GIVEN - a change set that failed to create, described as incomplete. The real reason is what + // tells the diagnoser this was an early validation failure, so the warning must not displace it. + mockFlaggedResponses( + { Status: 'FAILED', Changes: [] }, + { Status: 'FAILED', StatusReason: '(AWS::EarlyValidation::SomeError). Blah blah blah.', Changes: [] }, + ); + + // WHEN + const description = await describer().describeCurrentState(); + + // THEN + expect(description.StatusReason).toEqual('(AWS::EarlyValidation::SomeError). Blah blah blah.'); + }); + + test('keeps the detailed change (with property values) when a resource is present in both responses', async () => { + // GIVEN + mockFlaggedResponses( + { + Changes: [{ + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: 'Bucket', + ResourceType: 'AWS::S3::Bucket', + BeforeContext: '{"Properties":{"BucketName":"old"}}', + AfterContext: '{"Properties":{"BucketName":"new"}}', + Details: [{ + Target: { Attribute: 'Properties', Name: 'BucketName', RequiresRecreation: 'Always', BeforeValue: 'old', AfterValue: 'new' }, + ChangeSource: 'DirectModification', + }], + }, + }], + }, + { + Changes: [{ + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: 'Bucket', + ResourceType: 'AWS::S3::Bucket', + Details: [{ + Target: { Attribute: 'Properties', Name: 'BucketName', RequiresRecreation: 'Always' }, + ChangeSource: 'DirectModification', + }], + }, + }], + }, + ); + + // WHEN + const { changeSet: description } = await describer().waitForReport({ diagnoser }); + + // THEN - no duplicates, and the value-carrying detailed change wins + expect(description.Changes).toHaveLength(1); + expect(description.Changes?.[0].ResourceChange?.BeforeContext).toEqual('{"Properties":{"BucketName":"old"}}'); + expect(description.Changes?.[0].ResourceChange?.Details).toHaveLength(1); + expect(description.Changes?.[0].ResourceChange?.Details?.[0].Target?.BeforeValue).toEqual('old'); + }); + + test('restores per-property change details dropped from the detailed response', async () => { + // GIVEN - the resource is present in both, but the detailed response is missing a detail + mockFlaggedResponses( + { + Changes: [{ + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: 'DataSource', + Details: [{ + Target: { Attribute: 'Properties', Name: 'DisplayName', RequiresRecreation: 'Never', BeforeValue: 'a', AfterValue: 'b' }, + ChangeSource: 'DirectModification', + }], + }, + }], + }, + { + Changes: [{ + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: 'DataSource', + Details: [ + { + Target: { Attribute: 'Properties', Name: 'DisplayName', RequiresRecreation: 'Never' }, + ChangeSource: 'DirectModification', + }, + { + Target: { Attribute: 'Properties', Name: 'Configuration', RequiresRecreation: 'Never' }, + ChangeSource: 'DirectModification', + }, + ], + }, + }], + }, + ); + + // WHEN + const { changeSet: description } = await describer().waitForReport({ diagnoser }); + + // THEN + const details = description.Changes?.[0].ResourceChange?.Details; + expect(details).toHaveLength(2); + // the detail present in both keeps its detailed (value-carrying) version + expect(details?.find((d) => d.Target?.Name === 'DisplayName')?.Target?.BeforeValue).toEqual('a'); + // the dropped detail is restored + expect(details?.find((d) => d.Target?.Name === 'Configuration')).toBeDefined(); + }); + + test('does not treat details with a different change source as the same change', async () => { + // GIVEN + mockFlaggedResponses( + { + Changes: [{ + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: 'Bucket', + Details: [{ + Target: { Attribute: 'Properties', Name: 'BucketName', RequiresRecreation: 'Always' }, + Evaluation: 'Static', + ChangeSource: 'DirectModification', + }], + }, + }], + }, + { + Changes: [{ + Type: 'Resource', + ResourceChange: { + Action: 'Modify', + LogicalResourceId: 'Bucket', + Details: [{ + Target: { Attribute: 'Properties', Name: 'BucketName', RequiresRecreation: 'Always' }, + Evaluation: 'Dynamic', + ChangeSource: 'ParameterReference', + CausingEntity: 'MyParam', + }], + }, + }], + }, + ); + + // WHEN + const { changeSet: description } = await describer().waitForReport({ diagnoser }); + + // THEN + expect(description.Changes?.[0].ResourceChange?.Details).toHaveLength(2); + }); + + test('skips plain changes that cannot be correlated by logical id', async () => { + // GIVEN + mockFlaggedResponses( + { Changes: [] }, + { Changes: [{ Type: 'Resource', ResourceChange: { Action: 'Modify' } }] }, + ); + + // WHEN + const { changeSet: description } = await describer().waitForReport({ diagnoser }); + + // THEN + expect(description.Changes).toHaveLength(0); + }); + + test('logs that it is describing the change set again', async () => { + // GIVEN + ioHost.level = 'debug'; + mockFlaggedResponses( + { Changes: [] }, + { Changes: [{ Type: 'Resource', ResourceChange: { Action: 'Modify', LogicalResourceId: 'DataSource' } }] }, + ); + + // WHEN + await describer().describeCurrentState(); + + // THEN + expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({ + level: 'debug', + message: expect.stringContaining('was described with incomplete data'), + })); + }); + + test('describes the change set a second time when CloudFormation flags the response as incomplete', async () => { + // GIVEN + mockFlaggedResponses({ Changes: [] }, { Changes: [] }); + + // WHEN + await describer().waitForReport({ diagnoser }); + + // THEN + expect(plainDescribeCalls()).toHaveLength(1); + }); + + test('does not describe the change set a second time when the response is not flagged', async () => { + // GIVEN - a normal response, without the incomplete-data warning + mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ Status: 'CREATE_COMPLETE', Changes: [] }); + + // WHEN + await describer().waitForReport({ diagnoser }); + + // THEN - a single describe, with property values + const calls = mockCloudFormationClient.commandCalls(DescribeChangeSetCommand); + expect(calls).toHaveLength(1); + expect(calls[0].args[0].input.IncludePropertyValues).toBe(true); + }); + + test('does not describe a second time for an ordinary status reason', async () => { + // GIVEN - an empty change set: FAILED with a status reason, but no warning. This is the common + // case for a no-op diff or deploy, so it must not cost a second call. + mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ + Status: 'FAILED', + StatusReason: "The submitted information didn't contain changes.", + Changes: [], + }); + + // WHEN + await describer().describeCurrentState(); + + // THEN + expect(mockCloudFormationClient.commandCalls(DescribeChangeSetCommand)).toHaveLength(1); + }); + + test('does not describe a second time while the change set is still creating', async () => { + // GIVEN - still creating on the first poll, flagged as incomplete even then (which we don't + // expect CloudFormation to do, but we should not pay for it if it does) + mockCloudFormationClient.on(DescribeChangeSetCommand, { IncludePropertyValues: true }) + .resolvesOnce({ Status: 'CREATE_IN_PROGRESS', StatusReason: INCOMPLETE_DATA_WARNING }) + .resolves({ Status: 'CREATE_COMPLETE', StatusReason: INCOMPLETE_DATA_WARNING, Changes: [] }); + mockCloudFormationClient.on(DescribeChangeSetCommand, { IncludePropertyValues: false }).resolves({ Status: 'CREATE_COMPLETE', Changes: [] }); + + // WHEN + await describer().waitForReport({ diagnoser }); + + // THEN - the plain describe only happened for the terminal state, not for the in-progress poll + expect(plainDescribeCalls()).toHaveLength(1); + }); + + test('always fetches all pages of changes', async () => { + // GIVEN - the first page carries a NextToken + mockCloudFormationClient.on(DescribeChangeSetCommand) + .callsFake((input) => { + if (input.NextToken) { + return { + Status: 'CREATE_COMPLETE', + Changes: [{ Type: 'Resource', ResourceChange: { Action: 'Modify', LogicalResourceId: 'Second' } }], + }; + } + return { + Status: 'CREATE_COMPLETE', + NextToken: 'page-2', + Changes: [{ Type: 'Resource', ResourceChange: { Action: 'Modify', LogicalResourceId: 'First' } }], + }; + }); + + // WHEN + const description = await describer().describeCurrentState(); + + // THEN - changes from both pages are present + expect(description.Changes?.map((c) => c.ResourceChange?.LogicalResourceId).sort()).toEqual(['First', 'Second']); + }); +}); 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 8d9d2ac5f..78e608d57 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 @@ -346,6 +346,25 @@ test('execute-change-set describes and executes an existing change set', async ( }); test('execute-change-set throws if change set is not ready', async () => { + // GIVEN + mockCloudFormationClient.on(DescribeStacksCommand).resolves({ + Stacks: [{ ...baseResponse }], + }); + mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ + Status: 'CREATE_IN_PROGRESS', + StatusReason: 'doing stuff', + }); + + // WHEN/THEN + await expect( + testDeployStack({ + ...standardDeployStackArguments(), + deploymentMethod: { method: 'execute-change-set', changeSetName: 'MyChangeSet' }, + }), + ).rejects.toThrow('still being created'); +}); + +test('execute-change-set throws if change set failed', async () => { // GIVEN mockCloudFormationClient.on(DescribeStacksCommand).resolves({ Stacks: [{ ...baseResponse }], @@ -361,7 +380,7 @@ test('execute-change-set throws if change set is not ready', async () => { ...standardDeployStackArguments(), deploymentMethod: { method: 'execute-change-set', changeSetName: 'MyChangeSet' }, }), - ).rejects.toThrow('not ready for execution'); + ).rejects.toThrow('Failed to create change set'); }); test('call UpdateStack when method=direct and the stack exists already', async () => { @@ -825,7 +844,7 @@ test('deployStack reports no change if describeChangeSet returns an error that i }); test('deployStack throws error in case of early validation failures', async () => { - mockCloudFormationClient.on(DescribeChangeSetCommand).resolvesOnce({ + mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ ChangeSetName: 'cdk-deploy-change-set', Status: ChangeSetStatus.FAILED, StatusReason: '(AWS::EarlyValidation::SomeError). Blah blah blah.', @@ -849,7 +868,7 @@ test('deployStack throws error in case of early validation failures', async () = }); test('deployStack warns when it cannot get the events in case of early validation errors', async () => { - mockCloudFormationClient.on(DescribeChangeSetCommand).resolvesOnce({ + mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ ChangeSetName: 'cdk-deploy-change-set', Status: ChangeSetStatus.FAILED, StatusReason: '(AWS::EarlyValidation::SomeError). Blah blah blah.', @@ -867,7 +886,7 @@ test('deployStack warns when it cannot get the events in case of early validatio }); test('even if ChangeSet error does not match pattern, DescribeEvents is called', async () => { - mockCloudFormationClient.on(DescribeChangeSetCommand).resolvesOnce({ + mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ ChangeSetName: 'cdk-deploy-change-set', Status: ChangeSetStatus.FAILED, StatusReason: 'Something somewhere went wrong, cannot be more specific than that.', @@ -1597,7 +1616,7 @@ describe('execute-change-set deployment method', () => { expect(mockCloudFormationClient).toHaveReceivedCommand(ExecuteChangeSetCommand); }); - test('throws when change set is not in CREATE_COMPLETE status', async () => { + test('reports the diagnosis when change set failed to create', async () => { // GIVEN givenStackExists(); givenChangeSetExists({ @@ -1612,7 +1631,7 @@ describe('execute-change-set deployment method', () => { ...standardDeployStackArguments(), deploymentMethod: { method: 'execute-change-set', changeSetName: 'my-change-set' }, }), - ).rejects.toThrow(/not ready for execution.*FAILED.*Something went wrong/); + ).rejects.toThrow(/Failed to create change set my-change-set[\s\S]*Something went wrong/); }); test('throws when change set is in CREATE_PENDING status', async () => { @@ -1623,13 +1642,13 @@ describe('execute-change-set deployment method', () => { ChangeSetName: 'my-change-set', }); - // THEN + // THEN - we report that it hasn't settled rather than waiting for it await expect( testDeployStack({ ...standardDeployStackArguments(), deploymentMethod: { method: 'execute-change-set', changeSetName: 'my-change-set' }, }), - ).rejects.toThrow(/not ready for execution.*CREATE_PENDING/); + ).rejects.toThrow(/still being created.*CREATE_PENDING/); }); test('throws without reason when status reason is absent', async () => { @@ -1646,7 +1665,7 @@ describe('execute-change-set deployment method', () => { ...standardDeployStackArguments(), deploymentMethod: { method: 'execute-change-set', changeSetName: 'my-change-set' }, }), - ).rejects.toThrow(/not ready for execution.*DELETE_COMPLETE(?!.*:)/); + ).rejects.toThrow(/cannot be executed \(DELETE_COMPLETE\)$/); }); test('returns replacement-requires-rollback when change set has replacement and rollback is disabled', async () => { diff --git a/packages/@aws-cdk/toolkit-lib/test/api/diagnosing/diagnosis-formatting.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/diagnosing/diagnosis-formatting.test.ts index c07d0b533..f1e1e4ebd 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/diagnosing/diagnosis-formatting.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/diagnosing/diagnosis-formatting.test.ts @@ -1,7 +1,19 @@ import type { DiagnosedStack, StackDiagnosis, TracedResourceError } from '../../../lib/actions/diagnose'; -import { hostMessageFromDiagnosis, throwDeploymentErrorFromDiagnosis } from '../../../lib/api/diagnosing/diagnosis-formatting'; +import { Diagnosis } from '../../../lib/api/diagnosing/diagnosis'; +import { hostMessageFromDiagnosis } from '../../../lib/api/diagnosing/diagnosis-formatting'; import type { ActionLessMessage } from '../../../lib/api/io/private'; -import { DeploymentError, ToolkitError } from '../../../lib/toolkit/toolkit-error'; +import { DeploymentError } from '../../../lib/toolkit/toolkit-error'; + +function toDiagnosis(result: StackDiagnosis): Diagnosis { + switch (result.type) { + case 'no-problem': + return Diagnosis.noProblem(); + case 'problem': + return Diagnosis.problem(result.detectedBy, result.problems); + case 'error-diagnosing': + return Diagnosis.errorDiagnosing(result.message); + } +} function diagnosedStack(stackName: string, result: StackDiagnosis): DiagnosedStack { return { stackName, hierarchicalId: stackName, result }; @@ -202,44 +214,44 @@ describe('hostMessageFromDiagnosis', () => { }); }); -describe('throwDeploymentErrorFromDiagnosis', () => { - test('throws ToolkitError for no-problem', () => { - expect(() => throwDeploymentErrorFromDiagnosis({ type: 'no-problem' })).toThrow(ToolkitError); +describe('StackDiagnosis.throwOnError', () => { + test('does not throw for no-problem', () => { + expect(() => toDiagnosis({ type: 'no-problem' }).throwOnError()).not.toThrow(); }); test('throws DeploymentError for error-diagnosing', () => { - expect(() => throwDeploymentErrorFromDiagnosis({ + expect(() => toDiagnosis({ type: 'error-diagnosing', message: 'Could not diagnose', - })).toThrow(DeploymentError); + }).throwOnError()).toThrow(DeploymentError); }); test('throws DeploymentError with correct error code for deployment failure', () => { - expect(() => throwDeploymentErrorFromDiagnosis({ + expect(() => toDiagnosis({ type: 'problem', detectedBy: { type: 'deployment', stackStatus: 'UPDATE_FAILED', statusReason: 'failed' }, problems: [tracedError({ errorCode: 'S3:AccessDenied' })], - })).toThrow(expect.objectContaining({ + }).throwOnError()).toThrow(expect.objectContaining({ deploymentErrorCode: 'S3:AccessDenied', })); }); test('throws DeploymentError with default error code for change set failure', () => { - expect(() => throwDeploymentErrorFromDiagnosis({ + expect(() => toDiagnosis({ type: 'problem', detectedBy: { type: 'change-set', changeSetName: 'cs', changeSetStatus: 'FAILED', statusReason: 'err' }, problems: [tracedError()], - })).toThrow(expect.objectContaining({ + }).throwOnError()).toThrow(expect.objectContaining({ deploymentErrorCode: 'ChangeSetCreationFailed', })); }); test('throws DeploymentError with default error code for early validation failure', () => { - expect(() => throwDeploymentErrorFromDiagnosis({ + expect(() => toDiagnosis({ type: 'problem', detectedBy: { type: 'early-validation', changeSetName: 'cs' }, problems: [tracedError()], - })).toThrow(expect.objectContaining({ + }).throwOnError()).toThrow(expect.objectContaining({ deploymentErrorCode: 'EarlyValidationFailure', })); }); diff --git a/packages/@aws-cdk/toolkit-lib/test/api/diagnosing/stack-diagnoser.test.ts b/packages/@aws-cdk/toolkit-lib/test/api/diagnosing/stack-diagnoser.test.ts index 6622160a1..988675249 100644 --- a/packages/@aws-cdk/toolkit-lib/test/api/diagnosing/stack-diagnoser.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/api/diagnosing/stack-diagnoser.test.ts @@ -7,6 +7,7 @@ import { } from '@aws-sdk/client-cloudformation'; import { LookupEventsCommand } from '@aws-sdk/client-cloudtrail'; import type { StackDiagnosis } from '../../../lib/actions/diagnose'; +import type { Diagnosis } from '../../../lib/api/diagnosing/diagnosis'; import { CloudFormationStackDiagnoser } from '../../../lib/api/diagnosing/stack-diagnoser'; import type { ISourceTracer } from '../../../lib/api/source-tracing/private/source-tracing'; import type { SourceTrace } from '../../../lib/api/source-tracing/types'; @@ -58,10 +59,14 @@ class FakeSourceTracer implements ISourceTracer { } /** - * Type guard to narrow a StackDiagnosis to the 'problem' variant + * Assert the diagnosis is the 'problem' variant, and return it narrowed to that variant */ -function assertProblem(result: StackDiagnosis): asserts result is Extract { - expect(result.type).toBe('problem'); +function assertProblem(diagnosis: Diagnosis): Extract { + expect(diagnosis.type).toBe('problem'); + if (diagnosis.result.type !== 'problem') { + throw new Error(`Expected a 'problem' diagnosis, got '${diagnosis.type}'`); + } + return diagnosis.result; } describe('CloudFormationStackDiagnoser', () => { @@ -71,13 +76,13 @@ describe('CloudFormationStackDiagnoser', () => { const result = await makeDiagnoser().diagnoseFromFresh('MyStack'); - expect(result).toMatchObject({ type: 'no-problem' }); + expect(result.result).toMatchObject({ type: 'no-problem' }); }); test('returns error-diagnosing when stack does not exist', async () => { const result = await makeDiagnoser().diagnoseFromFresh('NonExistent'); - expect(result).toMatchObject({ + expect(result.result).toMatchObject({ type: 'error-diagnosing', message: expect.stringContaining('NonExistent'), }); @@ -88,7 +93,7 @@ describe('CloudFormationStackDiagnoser', () => { const result = await makeDiagnoser().diagnoseFromFresh('MyStack'); - expect(result).toMatchObject({ + expect(result.result).toMatchObject({ type: 'error-diagnosing', message: expect.stringContaining('currently being updated'), }); @@ -112,7 +117,7 @@ describe('CloudFormationStackDiagnoser', () => { const result = await makeDiagnoser().diagnoseFromFresh('MyStack'); - expect(result).toMatchObject({ + expect(result.result).toMatchObject({ type: 'problem', detectedBy: { type: 'deployment' }, problems: [expect.objectContaining({ @@ -173,7 +178,7 @@ describe('CloudFormationStackDiagnoser', () => { const result = await makeDiagnoser().diagnoseFromFresh('MyStack'); - expect(result).toMatchObject({ + expect(result.result).toMatchObject({ type: 'problem', detectedBy: { type: 'deployment' }, problems: [expect.objectContaining({ @@ -194,7 +199,7 @@ describe('CloudFormationStackDiagnoser', () => { const result = await makeDiagnoser().diagnoseFromFresh('MyStack'); - expect(result).toMatchObject({ + expect(result.result).toMatchObject({ type: 'problem', detectedBy: { type: 'change-set', @@ -215,7 +220,7 @@ describe('CloudFormationStackDiagnoser', () => { const result = await makeDiagnoser().diagnoseFromFresh('MyStack'); - expect(result).toMatchObject({ type: 'no-problem' }); + expect(result.result).toMatchObject({ type: 'no-problem' }); }); test('diagnoses auto-import failure', async () => { @@ -228,16 +233,16 @@ describe('CloudFormationStackDiagnoser', () => { }); const result = await makeDiagnoser().diagnoseFromFresh('MyStack'); - assertProblem(result); + const problem = assertProblem(result); - expect(result).toMatchObject({ + expect(result.result).toMatchObject({ detectedBy: { type: 'change-set' }, problems: [expect.objectContaining({ logicalId: 'MyBucket', message: expect.stringContaining('DeletionPolicy'), })], }); - expect(result.problems[0].message).toContain('RemovalPolicy.RETAIN'); + expect(problem.problems[0].message).toContain('RemovalPolicy.RETAIN'); }); test('diagnoses nested change set failure', async () => { @@ -271,7 +276,7 @@ describe('CloudFormationStackDiagnoser', () => { const result = await makeDiagnoser().diagnoseFromFresh('ParentStack'); - expect(result).toMatchObject({ + expect(result.result).toMatchObject({ type: 'problem', detectedBy: { type: 'change-set' }, problems: [expect.anything()], @@ -301,7 +306,7 @@ describe('CloudFormationStackDiagnoser', () => { const result = await makeDiagnoser().diagnoseFromFresh('MyStack'); - expect(result).toMatchObject({ + expect(result.result).toMatchObject({ type: 'problem', detectedBy: { type: 'early-validation' }, problems: [expect.objectContaining({ @@ -329,12 +334,12 @@ describe('CloudFormationStackDiagnoser', () => { fakeTracer.traceToReturn = { constructPath: 'MyStack/MyBucket/Resource' }; const result = await makeDiagnoser().diagnoseFromFresh('MyStack'); - assertProblem(result); + const problem = assertProblem(result); expect(fakeTracer.resourceCalls).toEqual([ expect.objectContaining({ logicalId: 'MyBucket', nestedStackLogicalIds: [] }), ]); - expect(result.problems[0].sourceTrace).toEqual({ constructPath: 'MyStack/MyBucket/Resource' }); + expect(problem.problems[0].sourceTrace).toEqual({ constructPath: 'MyStack/MyBucket/Resource' }); }); test('calls source tracer for non-specific change set errors (stack-level)', async () => { @@ -368,7 +373,7 @@ describe('CloudFormationStackDiagnoser', () => { const result = await makeDiagnoser('MyApp/MyStack').diagnoseFromFresh('MyStack'); - expect(result).toMatchObject({ + expect(result.result).toMatchObject({ type: 'problem', problems: [expect.objectContaining({ topLevelStackHierarchicalId: 'MyApp/MyStack' })], }); @@ -383,7 +388,20 @@ describe('CloudFormationStackDiagnoser', () => { Status: ChangeSetStatus.CREATE_COMPLETE, }); - expect(result).toMatchObject({ type: 'no-problem' }); + expect(result.result).toMatchObject({ type: 'no-problem' }); + }); + + test('reports a change set that has not settled yet as not executable', async () => { + const result = await makeDiagnoser().diagnoseChangeSet({ + ChangeSetName: 'my-cs', + StackName: 'MyStack', + Status: ChangeSetStatus.CREATE_IN_PROGRESS, + }, { requireExecutable: true }); + + expect(result.result).toMatchObject({ + type: 'problem', + detectedBy: { type: 'change-set-not-ready', changeSetStatus: 'CREATE_IN_PROGRESS' }, + }); }); test('diagnoses a failed change set', async () => { @@ -396,7 +414,7 @@ describe('CloudFormationStackDiagnoser', () => { StatusReason: 'Template error: something went wrong', }); - expect(result).toMatchObject({ + expect(result.result).toMatchObject({ type: 'problem', detectedBy: { type: 'change-set' }, }); @@ -454,7 +472,7 @@ describe('CloudFormationStackDiagnoser', () => { StatusReason: 'AWS::EarlyValidation failed', }); - expect(result).toMatchObject({ + expect(result.result).toMatchObject({ type: 'problem', detectedBy: { type: 'early-validation', @@ -545,8 +563,8 @@ describe('CloudFormationStackDiagnoser', () => { TargetType: 'CHANGE_SET', TargetId: CS_ARN, }); - assertProblem(result); - expect(result.problems).toEqual([expect.objectContaining({ + const problem = assertProblem(result); + expect(problem.problems).toEqual([expect.objectContaining({ errorCode: 'HookFailed', message: `Hook 'Example::CFNHook::Full' failed: ${LAMBDA_HOOK_REASON.trim()}`, stackArn: STACK_ARN, @@ -570,11 +588,11 @@ describe('CloudFormationStackDiagnoser', () => { const result = await makeDiagnoser().diagnoseChangeSet(failedChangeSet()); - assertProblem(result); - expect(result.problems[0].message).toContain("Hook 'Private::Guard::TestHook' failed"); - expect(result.problems[0].message).toContain('NonCompliant Rules:'); - expect(result.problems[0].message).toContain('[AWS_S3_Bucket_AccessControl]'); - expect(result.problems[0].message).toContain('Remediation: AccessControl is deprecated'); + const problem = assertProblem(result); + expect(problem.problems[0].message).toContain("Hook 'Private::Guard::TestHook' failed"); + expect(problem.problems[0].message).toContain('NonCompliant Rules:'); + expect(problem.problems[0].message).toContain('[AWS_S3_Bucket_AccessControl]'); + expect(problem.problems[0].message).toContain('Remediation: AccessControl is deprecated'); }); test('falls back to the summary HookStatusReason when GetHookResult fails', async () => { @@ -585,8 +603,8 @@ describe('CloudFormationStackDiagnoser', () => { const result = await makeDiagnoser().diagnoseChangeSet(failedChangeSet()); - assertProblem(result); - expect(result.problems).toEqual([expect.objectContaining({ + const problem = assertProblem(result); + expect(problem.problems).toEqual([expect.objectContaining({ errorCode: 'HookFailed', message: `Hook 'Example::CFNHook::Full' failed: ${LAMBDA_HOOK_REASON.trim()}`, })]); @@ -600,11 +618,11 @@ describe('CloudFormationStackDiagnoser', () => { const result = await makeDiagnoser().diagnoseChangeSet(failedChangeSet()); // Falls through to the non-specific change set error - assertProblem(result); - expect(result.problems).toEqual([expect.objectContaining({ + const problem = assertProblem(result); + expect(problem.problems).toEqual([expect.objectContaining({ message: expect.stringContaining('The following hook(s) failed'), })]); - expect(result.problems[0].errorCode).not.toEqual('HookFailed'); + expect(problem.problems[0].errorCode).not.toEqual('HookFailed'); }); test('reports the non-specific change set error when ListHookResults fails', async () => { @@ -612,8 +630,8 @@ describe('CloudFormationStackDiagnoser', () => { const result = await makeDiagnoser().diagnoseChangeSet(failedChangeSet()); - assertProblem(result); - expect(result.problems).toEqual([expect.objectContaining({ + const problem = assertProblem(result); + expect(problem.problems).toEqual([expect.objectContaining({ message: expect.stringContaining('The following hook(s) failed'), })]); }); @@ -651,8 +669,8 @@ describe('CloudFormationStackDiagnoser', () => { StatusReason: 'AWS::EarlyValidation failed', }); - assertProblem(result); - expect(result.problems).toEqual([expect.objectContaining({ logicalId: 'BadPolicy' })]); + const problem = assertProblem(result); + expect(problem.problems).toEqual([expect.objectContaining({ logicalId: 'BadPolicy' })]); expect(mockCloudFormationClient).not.toHaveReceivedCommand(ListHookResultsCommand); }); }); @@ -667,7 +685,7 @@ describe('CloudFormationStackDiagnoser', () => { CreationTime: new Date(), }); - expect(result).toMatchObject({ type: 'no-problem' }); + expect(result.result).toMatchObject({ type: 'no-problem' }); }); test('returns problem with traced errors', async () => { @@ -696,7 +714,7 @@ describe('CloudFormationStackDiagnoser', () => { CreationTime: new Date(), }); - expect(result).toMatchObject({ + expect(result.result).toMatchObject({ type: 'problem', problems: [expect.objectContaining({ sourceTrace: { constructPath: 'MyStack/MyFunc/Resource' }, @@ -771,8 +789,8 @@ describe('CloudFormationStackDiagnoser', () => { CreationTime: new Date(), }); - assertProblem(result); - const context = result.problems[0].additionalContext?.find((c) => c.source === 'CloudTrail Errors'); + const problem = assertProblem(result); + const context = problem.problems[0].additionalContext?.find((c) => c.source === 'CloudTrail Errors'); expect(context).toBeDefined(); expect(context!.messages[0]).toMatch(/AccessDenied on s3\.amazonaws\.com:CreateBucket/); }); diff --git a/packages/aws-cdk/lib/api-private.ts b/packages/aws-cdk/lib/api-private.ts index 303dd530f..2a2d8106d 100644 --- a/packages/aws-cdk/lib/api-private.ts +++ b/packages/aws-cdk/lib/api-private.ts @@ -2,6 +2,9 @@ export { deployStack } from '../../@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack'; export type { DeployStackOptions as DeployStackApiOptions, DestroyStackResult } from '../../@aws-cdk/toolkit-lib/lib/api/deployments/deploy-stack'; export * as cfnApi from '../../@aws-cdk/toolkit-lib/lib/api/deployments/cfn-api'; +export { ChangeSetDescriber } from '../../@aws-cdk/toolkit-lib/lib/api/change-sets'; +export { Diagnosis } from '../../@aws-cdk/toolkit-lib/lib/api/diagnosing/diagnosis'; +export type { ChangeSetReport } from '../../@aws-cdk/toolkit-lib/lib/api/change-sets'; export { createIgnoreMatcher } from '../../@aws-cdk/toolkit-lib/lib/util/glob-matcher'; export { formatExpressStabilizationWarning } from '../../@aws-cdk/toolkit-lib/lib/util/cfn-express'; export * from '../../@aws-cdk/toolkit-lib/lib/api/io/private'; diff --git a/packages/aws-cdk/lib/cli/cdk-toolkit.ts b/packages/aws-cdk/lib/cli/cdk-toolkit.ts index dc803f6df..400a77987 100644 --- a/packages/aws-cdk/lib/cli/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cli/cdk-toolkit.ts @@ -356,7 +356,7 @@ export class CdkToolkit { } const changeSet = (options.method !== 'template') - ? await this.tryCreateDiffChangeSet(stack, options, parameterMap, resourcesToImport, quiet) + ? (await this.tryCreateDiffChangeSet(stack, options, parameterMap, resourcesToImport, quiet))?.changeSet : undefined; const mappings = allMappings.find(m => @@ -435,7 +435,6 @@ export class CdkToolkit { stack, uuid: randomUUID(), deployments: this.props.deployments, - willExecute: false, sdkProvider: this.props.sdkProvider, parameters: Object.assign({}, parameterMap['*'], parameterMap[stack.stackName]), resourcesToImport, diff --git a/packages/aws-cdk/test/commands/diff.test.ts b/packages/aws-cdk/test/commands/diff.test.ts index 7f82261e1..b937a5793 100644 --- a/packages/aws-cdk/test/commands/diff.test.ts +++ b/packages/aws-cdk/test/commands/diff.test.ts @@ -3,11 +3,10 @@ import * as os from 'os'; import * as path from 'path'; import type { CloudFormationStackArtifact } from '@aws-cdk/cloud-assembly-api'; import * as cxschema from '@aws-cdk/cloud-assembly-schema'; -import type { DescribeChangeSetCommandOutput } from '@aws-sdk/client-cloudformation'; import type { NestedStackTemplates } from '../../lib/api'; import { Deployments } from '../../lib/api'; -import type { IoHelper } from '../../lib/api-private'; -import { cfnApi } from '../../lib/api-private'; +import type { ChangeSetReport, IoHelper } from '../../lib/api-private'; +import { ChangeSetDescriber, cfnApi, Diagnosis } from '../../lib/api-private'; import { CdkToolkit } from '../../lib/cli/cdk-toolkit'; import { CliIoHost } from '../../lib/cli/io-host'; import { instanceMockFrom, MockCloudExecutable } from '../_helpers'; @@ -117,7 +116,7 @@ describe('fixed template', () => { describe('import existing resources', () => { let createDiffChangeSet: jest.SpyInstance< - Promise, + Promise, [ioHelper: IoHelper, options: cfnApi.PrepareChangeSetOptions], any >; @@ -173,15 +172,18 @@ describe('import existing resources', () => { test('import action in change set output', async () => { createDiffChangeSet = jest.spyOn(cfnApi, 'createDiffChangeSet').mockImplementationOnce(async () => { return { - $metadata: {}, - Changes: [ - { - ResourceChange: { - Action: 'Import', - LogicalResourceId: 'MyGlobalTable', + changeSet: { + $metadata: {}, + Changes: [ + { + ResourceChange: { + Action: 'Import', + LogicalResourceId: 'MyGlobalTable', + }, }, - }, - ], + ], + }, + diagnosis: Diagnosis.noProblem(), }; }); @@ -211,15 +213,18 @@ Resources test('import action in change set output when not using --import-exsting-resources', async () => { createDiffChangeSet = jest.spyOn(cfnApi, 'createDiffChangeSet').mockImplementationOnce(async () => { return { - $metadata: {}, - Changes: [ - { - ResourceChange: { - Action: 'Add', - LogicalResourceId: 'MyGlobalTable', + changeSet: { + $metadata: {}, + Changes: [ + { + ResourceChange: { + Action: 'Add', + LogicalResourceId: 'MyGlobalTable', + }, }, - }, - ], + ], + }, + diagnosis: Diagnosis.noProblem(), }; }); @@ -250,15 +255,18 @@ Resources // WHEN createDiffChangeSet = jest.spyOn(cfnApi, 'createDiffChangeSet').mockImplementationOnce(async () => { return { - $metadata: {}, - Changes: [ - { - ResourceChange: { - Action: 'Add', - LogicalResourceId: 'MyGlobalTable', + changeSet: { + $metadata: {}, + Changes: [ + { + ResourceChange: { + Action: 'Add', + LogicalResourceId: 'MyGlobalTable', + }, }, - }, - ], + ], + }, + diagnosis: Diagnosis.noProblem(), }; }); @@ -311,7 +319,7 @@ Resources describe('imports', () => { let createDiffChangeSet: jest.SpyInstance< - Promise, + Promise, [ioHelper: IoHelper, options: cfnApi.PrepareChangeSetOptions], any >; @@ -325,27 +333,30 @@ describe('imports', () => { fs.writeFileSync('migrate.json', JSON.stringify(outputToJson, null, 2)); createDiffChangeSet = jest.spyOn(cfnApi, 'createDiffChangeSet').mockImplementationOnce(async () => { return { - $metadata: {}, - Changes: [ - { - ResourceChange: { - Action: 'Import', - LogicalResourceId: 'Queue', + changeSet: { + $metadata: {}, + Changes: [ + { + ResourceChange: { + Action: 'Import', + LogicalResourceId: 'Queue', + }, }, - }, - { - ResourceChange: { - Action: 'Import', - LogicalResourceId: 'Bucket', + { + ResourceChange: { + Action: 'Import', + LogicalResourceId: 'Bucket', + }, }, - }, - { - ResourceChange: { - Action: 'Import', - LogicalResourceId: 'Queue2', + { + ResourceChange: { + Action: 'Import', + LogicalResourceId: 'Queue2', + }, }, - }, - ], + ], + }, + diagnosis: Diagnosis.noProblem(), }; }); cloudExecutable = await MockCloudExecutable.create({ @@ -1147,7 +1158,7 @@ There were no differences`); test('diff falls back to non-changeset diff for nested stacks', async () => { // GIVEN - const changeSetSpy = jest.spyOn(cfnApi, 'waitForChangeSet'); + const changeSetSpy = jest.spyOn(ChangeSetDescriber.prototype, 'waitAndThrowOnProblem'); // WHEN const exitCode = await toolkit.diff({