From 37378b49cde311ef00a99e93a0e5deff44f14761 Mon Sep 17 00:00:00 2001 From: Momo Kornher Date: Thu, 30 Jul 2026 14:55:12 +0100 Subject: [PATCH 1/3] fix(toolkit-lib): change-set diff misses changes to JSON-typed properties CloudFormation can drop an entire ResourceChange from a DescribeChangeSet response that is requested with IncludePropertyValues, when it fails property validation while rendering the values. The only trace is a [WARN] in StatusReason; Status stays CREATE_COMPLETE. Because the diff merger treats a resource that is absent from the change set as unchanged, the template diff for that resource was erased and `cdk diff` reported no differences. Describe every change set twice, with and without property values, and merge the two responses so dropped resource changes and dropped per-property details are restored. All pages of Changes are now always fetched as well, so a truncated list can no longer make a caller believe a resource is unchanged. Collect the change set describing logic into a ChangeSetDescriber class in the new api/change-sets module, so that every caller goes through the same code path and the distinction between reading the current state, waiting for the change set to settle, and requiring an executable change set is explicit. --- .../@aws-cdk-testing/cli-integ/lib/regions.ts | 19 + .../resources/cdk-apps/qbusiness-app/app.js | 124 +++++++ .../resources/cdk-apps/qbusiness-app/cdk.json | 3 + ...nges-to-json-typed-properties.integtest.ts | 37 ++ .../toolkit-lib/lib/actions/diagnose/index.ts | 1 + .../lib/actions/diff/private/helpers.ts | 21 +- .../api/change-sets/change-set-describer.ts | 303 +++++++++++++++ .../toolkit-lib/lib/api/change-sets/index.ts | 1 + .../lib/api/deployments/cfn-api.ts | 260 ++----------- .../lib/api/deployments/deploy-stack.ts | 133 ++++--- .../lib/api/deployments/deployments.ts | 14 +- .../api/diagnosing/diagnosis-formatting.ts | 52 +-- .../lib/api/diagnosing/diagnosis.ts | 85 +++++ .../lib/api/diagnosing/stack-diagnoser.ts | 185 ++++----- .../toolkit-lib/lib/toolkit/toolkit.ts | 10 +- .../@aws-cdk/toolkit-lib/lib/util/promises.ts | 23 ++ .../toolkit-lib/test/actions/diff.test.ts | 95 ++++- .../toolkit-lib/test/actions/validate.test.ts | 31 +- .../change-sets/change-set-describer.test.ts | 351 ++++++++++++++++++ .../test/api/deployments/deploy-stack.test.ts | 37 +- .../diagnosing/diagnosis-formatting.test.ts | 38 +- .../api/diagnosing/stack-diagnoser.test.ts | 100 +++-- packages/aws-cdk/lib/api-private.ts | 3 + packages/aws-cdk/lib/cli/cdk-toolkit.ts | 3 +- packages/aws-cdk/test/commands/diff.test.ts | 107 +++--- 25 files changed, 1469 insertions(+), 567 deletions(-) create mode 100644 packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/qbusiness-app/app.js create mode 100644 packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/qbusiness-app/cdk.json create mode 100644 packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-cdk-diff---method-change-set-detects-changes-to-json-typed-properties.integtest.ts create mode 100644 packages/@aws-cdk/toolkit-lib/lib/api/change-sets/change-set-describer.ts create mode 100644 packages/@aws-cdk/toolkit-lib/lib/api/change-sets/index.ts create mode 100644 packages/@aws-cdk/toolkit-lib/lib/api/diagnosing/diagnosis.ts create mode 100644 packages/@aws-cdk/toolkit-lib/test/api/change-sets/change-set-describer.test.ts diff --git a/packages/@aws-cdk-testing/cli-integ/lib/regions.ts b/packages/@aws-cdk-testing/cli-integ/lib/regions.ts index a76880a2f..a5abd8d6d 100644 --- a/packages/@aws-cdk-testing/cli-integ/lib/regions.ts +++ b/packages/@aws-cdk-testing/cli-integ/lib/regions.ts @@ -42,6 +42,13 @@ export function allBut(exclude: string[]): string[] { return Array.from(new Set(ALL_REGIONS.filter((r) => !exclude.includes(r)))); } +/** + * Only the given regions, restricted to the ones atmosphere can hand out + */ +export function only(include: string[]): string[] { + return Array.from(new Set(ALL_REGIONS.filter((r) => include.includes(r)))); +} + /** * Regions that don't support CloudFormation Stack Refactoring */ @@ -66,3 +73,15 @@ export const BEDROCK_AGENT_REGIONS = allBut([ 'mx-central-1', 'us-west-1', ]); + +/** + * Regions that support AWS::QBusiness resources + * + * Listed rather than excluded, because Amazon Q Business exists in only a handful of regions. + */ +export const QBUSINESS_REGIONS = only([ + 'us-east-1', + 'us-west-2', + 'eu-west-1', + 'ap-southeast-2', +]); diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/qbusiness-app/app.js b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/qbusiness-app/app.js new file mode 100644 index 000000000..504cb311b --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/qbusiness-app/app.js @@ -0,0 +1,124 @@ +const cdk = require('aws-cdk-lib/core'); +const iam = require('aws-cdk-lib/aws-iam'); +const qbusiness = require('aws-cdk-lib/aws-qbusiness'); + +const stackPrefix = process.env.STACK_NAME_PREFIX; +if (!stackPrefix) { + throw new Error('the STACK_NAME_PREFIX environment variable is required'); +} + +// A QBusiness web crawler data source, whose `Configuration` is a JSON-typed (opaque blob) +// property. CloudFormation fails to render property values for it when a change set is described +// with IncludePropertyValues, and silently drops the resource change. Used to verify that +// `cdk diff --method=change-set` still detects changes inside the blob. +// See aws/aws-cdk-cli#1780. +class QBusinessDataSourceStack extends cdk.Stack { + constructor(scope, id, props) { + super(scope, id, props); + + // Anonymous identity type avoids the IAM Identity Center requirement. + const application = new qbusiness.CfnApplication(this, 'Application', { + displayName: `${id}-app`, + identityType: 'ANONYMOUS', + }); + + const index = new qbusiness.CfnIndex(this, 'Index', { + applicationId: application.attrApplicationId, + displayName: `${id}-index`, + type: 'STARTER', + capacityConfiguration: { units: 1 }, + }); + + const dataSourceRole = new iam.Role(this, 'DataSourceRole', { + assumedBy: new iam.ServicePrincipal('qbusiness.amazonaws.com', { + conditions: { + StringEquals: { 'aws:SourceAccount': this.account }, + ArnLike: { + 'aws:SourceArn': this.formatArn({ + service: 'qbusiness', + resource: 'application', + resourceName: '*', + }), + }, + }, + }), + inlinePolicies: { + DataSourceAccess: new iam.PolicyDocument({ + statements: [ + new iam.PolicyStatement({ + sid: 'AllowsAmazonQToIngestDocuments', + actions: ['qbusiness:BatchPutDocument', 'qbusiness:BatchDeleteDocument'], + resources: [ + index.attrIndexArn, + `${application.attrApplicationArn}/index/${index.attrIndexId}/data-source/*`, + ], + }), + new iam.PolicyStatement({ + sid: 'AllowsAmazonQToIngestPrincipalMapping', + actions: [ + 'qbusiness:PutGroup', + 'qbusiness:CreateUser', + 'qbusiness:DeleteGroup', + 'qbusiness:UpdateUser', + 'qbusiness:ListGroups', + ], + resources: [ + application.attrApplicationArn, + index.attrIndexArn, + `${application.attrApplicationArn}/index/${index.attrIndexId}/data-source/*`, + ], + }), + ], + }), + }, + }); + + // The test changes only these patterns, inside the JSON blob + const patterns = [process.env.WEB_CRAWLER_URL_PATTERN || 'https://docs\\.aws\\.amazon\\.com/cdk/.*\\.html$']; + + new qbusiness.CfnDataSource(this, 'WebCrawlerDataSource', { + applicationId: application.attrApplicationId, + indexId: index.attrIndexId, + displayName: `${id}-webcrawler`, + roleArn: dataSourceRole.roleArn, + configuration: { + type: 'WEBCRAWLERV2', + syncMode: 'FULL_CRAWL', + connectionConfiguration: { + repositoryEndpointMetadata: { + seedUrlConnections: [{ seedUrl: 'https://docs.aws.amazon.com/cdk/' }], + authentication: 'NoAuthentication', + }, + }, + repositoryConfigurations: { + webPage: { + fieldMappings: [ + { + indexFieldName: '_source_uri', + indexFieldType: 'STRING', + dataSourceFieldName: 'sourceUrl', + }, + ], + }, + }, + additionalProperties: { + rateLimit: '300', + maxFileSize: '50', + crawlDepth: '1', + maxLinksPerUrl: '10', + crawlSubDomain: false, + crawlAllDomain: false, + honorRobots: true, + crawlAttachments: false, + inclusionURLCrawlPatterns: patterns, + inclusionURLIndexPatterns: patterns, + }, + }, + }); + } +} + +const app = new cdk.App(); +new QBusinessDataSourceStack(app, `${stackPrefix}-qbusiness-datasource`); + +app.synth(); diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/qbusiness-app/cdk.json b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/qbusiness-app/cdk.json new file mode 100644 index 000000000..1fa96aa39 --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/qbusiness-app/cdk.json @@ -0,0 +1,3 @@ +{ + "app": "node app" +} diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-cdk-diff---method-change-set-detects-changes-to-json-typed-properties.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-cdk-diff---method-change-set-detects-changes-to-json-typed-properties.integtest.ts new file mode 100644 index 000000000..29b95a548 --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-cdk-diff---method-change-set-detects-changes-to-json-typed-properties.integtest.ts @@ -0,0 +1,37 @@ +import { integTest, withSpecificFixture } from '../../../lib'; +import { QBUSINESS_REGIONS } from '../../../lib/regions'; + +/** + * Regression test for https://github.com/aws/aws-cdk-cli/issues/1780 + * + * `AWS::QBusiness::DataSource.Configuration` is a JSON-typed (opaque blob) property. When a + * change set is described with `IncludePropertyValues`, CloudFormation fails property + * validation on it and silently drops the entire resource change from the response, which + * made `cdk diff --method=change-set` report "There were no differences". + * + * This test deploys the data source, changes a regex pattern inside the JSON blob, and + * asserts that the change-set diff detects the change. It lives in its own app because + * QBusiness only exists in a few regions, and synthesizing it elsewhere produces + * "resource type does not exist" validation warnings. + */ +integTest( + 'cdk diff --method=change-set detects changes to JSON-typed properties', + withSpecificFixture('qbusiness-app', async (fixture) => { + const stackName = fixture.fullStackName('qbusiness-datasource'); + + // GIVEN - a deployed data source whose JSON-typed Configuration contains the original patterns + await fixture.cdkDeploy('qbusiness-datasource', { + modEnv: { WEB_CRAWLER_URL_PATTERN: 'https://docs\\.aws\\.amazon\\.com/cdk/.*\\.html$' }, + }); + + // WHEN - only a value inside the JSON blob changes + const diff = await fixture.cdk(['diff', '--method=change-set', stackName], { + modEnv: { WEB_CRAWLER_URL_PATTERN: 'https://docs\\.aws\\.amazon\\.com/cdk/NEW-.*\\.html$' }, + }); + + // THEN - the change-set diff surfaces the change + expect(diff).not.toContain('There were no differences'); + expect(diff).toContain('AWS::QBusiness::DataSource'); + expect(diff).toContain('Configuration'); + }, { aws: { regions: QBUSINESS_REGIONS } }), +); 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({ From f7e657f887e2a6efe70b399e32d2365c87ccc23f Mon Sep 17 00:00:00 2001 From: Momo Kornher Date: Thu, 30 Jul 2026 17:56:30 +0100 Subject: [PATCH 2/3] remove integ test --- .../resources/cdk-apps/qbusiness-app/app.js | 124 ------------------ .../resources/cdk-apps/qbusiness-app/cdk.json | 3 - ...nges-to-json-typed-properties.integtest.ts | 37 ------ 3 files changed, 164 deletions(-) delete mode 100644 packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/qbusiness-app/app.js delete mode 100644 packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/qbusiness-app/cdk.json delete mode 100644 packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-cdk-diff---method-change-set-detects-changes-to-json-typed-properties.integtest.ts diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/qbusiness-app/app.js b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/qbusiness-app/app.js deleted file mode 100644 index 504cb311b..000000000 --- a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/qbusiness-app/app.js +++ /dev/null @@ -1,124 +0,0 @@ -const cdk = require('aws-cdk-lib/core'); -const iam = require('aws-cdk-lib/aws-iam'); -const qbusiness = require('aws-cdk-lib/aws-qbusiness'); - -const stackPrefix = process.env.STACK_NAME_PREFIX; -if (!stackPrefix) { - throw new Error('the STACK_NAME_PREFIX environment variable is required'); -} - -// A QBusiness web crawler data source, whose `Configuration` is a JSON-typed (opaque blob) -// property. CloudFormation fails to render property values for it when a change set is described -// with IncludePropertyValues, and silently drops the resource change. Used to verify that -// `cdk diff --method=change-set` still detects changes inside the blob. -// See aws/aws-cdk-cli#1780. -class QBusinessDataSourceStack extends cdk.Stack { - constructor(scope, id, props) { - super(scope, id, props); - - // Anonymous identity type avoids the IAM Identity Center requirement. - const application = new qbusiness.CfnApplication(this, 'Application', { - displayName: `${id}-app`, - identityType: 'ANONYMOUS', - }); - - const index = new qbusiness.CfnIndex(this, 'Index', { - applicationId: application.attrApplicationId, - displayName: `${id}-index`, - type: 'STARTER', - capacityConfiguration: { units: 1 }, - }); - - const dataSourceRole = new iam.Role(this, 'DataSourceRole', { - assumedBy: new iam.ServicePrincipal('qbusiness.amazonaws.com', { - conditions: { - StringEquals: { 'aws:SourceAccount': this.account }, - ArnLike: { - 'aws:SourceArn': this.formatArn({ - service: 'qbusiness', - resource: 'application', - resourceName: '*', - }), - }, - }, - }), - inlinePolicies: { - DataSourceAccess: new iam.PolicyDocument({ - statements: [ - new iam.PolicyStatement({ - sid: 'AllowsAmazonQToIngestDocuments', - actions: ['qbusiness:BatchPutDocument', 'qbusiness:BatchDeleteDocument'], - resources: [ - index.attrIndexArn, - `${application.attrApplicationArn}/index/${index.attrIndexId}/data-source/*`, - ], - }), - new iam.PolicyStatement({ - sid: 'AllowsAmazonQToIngestPrincipalMapping', - actions: [ - 'qbusiness:PutGroup', - 'qbusiness:CreateUser', - 'qbusiness:DeleteGroup', - 'qbusiness:UpdateUser', - 'qbusiness:ListGroups', - ], - resources: [ - application.attrApplicationArn, - index.attrIndexArn, - `${application.attrApplicationArn}/index/${index.attrIndexId}/data-source/*`, - ], - }), - ], - }), - }, - }); - - // The test changes only these patterns, inside the JSON blob - const patterns = [process.env.WEB_CRAWLER_URL_PATTERN || 'https://docs\\.aws\\.amazon\\.com/cdk/.*\\.html$']; - - new qbusiness.CfnDataSource(this, 'WebCrawlerDataSource', { - applicationId: application.attrApplicationId, - indexId: index.attrIndexId, - displayName: `${id}-webcrawler`, - roleArn: dataSourceRole.roleArn, - configuration: { - type: 'WEBCRAWLERV2', - syncMode: 'FULL_CRAWL', - connectionConfiguration: { - repositoryEndpointMetadata: { - seedUrlConnections: [{ seedUrl: 'https://docs.aws.amazon.com/cdk/' }], - authentication: 'NoAuthentication', - }, - }, - repositoryConfigurations: { - webPage: { - fieldMappings: [ - { - indexFieldName: '_source_uri', - indexFieldType: 'STRING', - dataSourceFieldName: 'sourceUrl', - }, - ], - }, - }, - additionalProperties: { - rateLimit: '300', - maxFileSize: '50', - crawlDepth: '1', - maxLinksPerUrl: '10', - crawlSubDomain: false, - crawlAllDomain: false, - honorRobots: true, - crawlAttachments: false, - inclusionURLCrawlPatterns: patterns, - inclusionURLIndexPatterns: patterns, - }, - }, - }); - } -} - -const app = new cdk.App(); -new QBusinessDataSourceStack(app, `${stackPrefix}-qbusiness-datasource`); - -app.synth(); diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/qbusiness-app/cdk.json b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/qbusiness-app/cdk.json deleted file mode 100644 index 1fa96aa39..000000000 --- a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/qbusiness-app/cdk.json +++ /dev/null @@ -1,3 +0,0 @@ -{ - "app": "node app" -} diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-cdk-diff---method-change-set-detects-changes-to-json-typed-properties.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-cdk-diff---method-change-set-detects-changes-to-json-typed-properties.integtest.ts deleted file mode 100644 index 29b95a548..000000000 --- a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/diff/cdk-cdk-diff---method-change-set-detects-changes-to-json-typed-properties.integtest.ts +++ /dev/null @@ -1,37 +0,0 @@ -import { integTest, withSpecificFixture } from '../../../lib'; -import { QBUSINESS_REGIONS } from '../../../lib/regions'; - -/** - * Regression test for https://github.com/aws/aws-cdk-cli/issues/1780 - * - * `AWS::QBusiness::DataSource.Configuration` is a JSON-typed (opaque blob) property. When a - * change set is described with `IncludePropertyValues`, CloudFormation fails property - * validation on it and silently drops the entire resource change from the response, which - * made `cdk diff --method=change-set` report "There were no differences". - * - * This test deploys the data source, changes a regex pattern inside the JSON blob, and - * asserts that the change-set diff detects the change. It lives in its own app because - * QBusiness only exists in a few regions, and synthesizing it elsewhere produces - * "resource type does not exist" validation warnings. - */ -integTest( - 'cdk diff --method=change-set detects changes to JSON-typed properties', - withSpecificFixture('qbusiness-app', async (fixture) => { - const stackName = fixture.fullStackName('qbusiness-datasource'); - - // GIVEN - a deployed data source whose JSON-typed Configuration contains the original patterns - await fixture.cdkDeploy('qbusiness-datasource', { - modEnv: { WEB_CRAWLER_URL_PATTERN: 'https://docs\\.aws\\.amazon\\.com/cdk/.*\\.html$' }, - }); - - // WHEN - only a value inside the JSON blob changes - const diff = await fixture.cdk(['diff', '--method=change-set', stackName], { - modEnv: { WEB_CRAWLER_URL_PATTERN: 'https://docs\\.aws\\.amazon\\.com/cdk/NEW-.*\\.html$' }, - }); - - // THEN - the change-set diff surfaces the change - expect(diff).not.toContain('There were no differences'); - expect(diff).toContain('AWS::QBusiness::DataSource'); - expect(diff).toContain('Configuration'); - }, { aws: { regions: QBUSINESS_REGIONS } }), -); From 08b0d250e297e0d23e758a1914b4e69d080aa911 Mon Sep 17 00:00:00 2001 From: Momo Kornher Date: Thu, 30 Jul 2026 20:49:39 +0100 Subject: [PATCH 3/3] remove test leftover --- .../@aws-cdk-testing/cli-integ/lib/regions.ts | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/packages/@aws-cdk-testing/cli-integ/lib/regions.ts b/packages/@aws-cdk-testing/cli-integ/lib/regions.ts index a5abd8d6d..a76880a2f 100644 --- a/packages/@aws-cdk-testing/cli-integ/lib/regions.ts +++ b/packages/@aws-cdk-testing/cli-integ/lib/regions.ts @@ -42,13 +42,6 @@ export function allBut(exclude: string[]): string[] { return Array.from(new Set(ALL_REGIONS.filter((r) => !exclude.includes(r)))); } -/** - * Only the given regions, restricted to the ones atmosphere can hand out - */ -export function only(include: string[]): string[] { - return Array.from(new Set(ALL_REGIONS.filter((r) => include.includes(r)))); -} - /** * Regions that don't support CloudFormation Stack Refactoring */ @@ -73,15 +66,3 @@ export const BEDROCK_AGENT_REGIONS = allBut([ 'mx-central-1', 'us-west-1', ]); - -/** - * Regions that support AWS::QBusiness resources - * - * Listed rather than excluded, because Amazon Q Business exists in only a handful of regions. - */ -export const QBUSINESS_REGIONS = only([ - 'us-east-1', - 'us-west-2', - 'eu-west-1', - 'ap-southeast-2', -]);