From 279149011711faa50a4805f25470742bca2058b6 Mon Sep 17 00:00:00 2001 From: Kaizen Conroy Date: Thu, 10 Apr 2025 15:08:17 -0400 Subject: [PATCH 1/2] feat(toolkit-lib): fallBackToTemplate option for diffs --- .../src/api/deployments/cfn-api.ts | 22 +++++++-- .../toolkit-lib/lib/actions/diff/index.ts | 17 ++++--- .../lib/actions/diff/private/helpers.ts | 26 +++++++++-- .../toolkit-lib/test/actions/diff.test.ts | 46 +++++++++++++++++-- 4 files changed, 90 insertions(+), 21 deletions(-) diff --git a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/deployments/cfn-api.ts b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/deployments/cfn-api.ts index 16958af04..bcdf22cb5 100644 --- a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/deployments/cfn-api.ts +++ b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/deployments/cfn-api.ts @@ -142,6 +142,14 @@ export type PrepareChangeSetOptions = { sdkProvider: SdkProvider; parameters: { [name: string]: string | undefined }; resourcesToImport?: ResourcesToImport; + /** + * Enable falling back to template-based diff in case creating the changeset is not possible or results in an error. + * + * Should be used for stacks containing nested stacks or when change set permissions aren't available. + * + * @default true + */ + fallBackToTemplate?: boolean; } export type CreateChangeSetOptions = { @@ -240,12 +248,16 @@ async function uploadBodyParameterAndCreateChangeSet( role: executionRoleArn, }); } catch (e: any) { - await ioHelper.notify(IO.DEFAULT_TOOLKIT_DEBUG.msg(String(e))); - await ioHelper.notify(IO.DEFAULT_TOOLKIT_INFO.msg( - 'Could not create a change set, will base the diff on template differences (run again with -v to see the reason)\n', - )); + if (options.fallBackToTemplate ?? true) { + await ioHelper.notify(IO.DEFAULT_TOOLKIT_DEBUG.msg(String(e))); + await ioHelper.notify(IO.DEFAULT_TOOLKIT_INFO.msg( + 'Could not create a change set, will base the diff on template differences (run again with -v to see the reason)\n', + )); - return undefined; + return undefined; + } + + throw new ToolkitError('Could not create a change set, set fallBackToTemplate=true to base the diff on template differences\n', e); } } diff --git a/packages/@aws-cdk/toolkit-lib/lib/actions/diff/index.ts b/packages/@aws-cdk/toolkit-lib/lib/actions/diff/index.ts index 6255bfef1..9fda9f36d 100644 --- a/packages/@aws-cdk/toolkit-lib/lib/actions/diff/index.ts +++ b/packages/@aws-cdk/toolkit-lib/lib/actions/diff/index.ts @@ -11,15 +11,14 @@ export interface CloudFormationDiffOptions { } export interface ChangeSetDiffOptions extends CloudFormationDiffOptions { - // @TODO: add this as a feature - // /** - // * Enable falling back to template-based diff in case creating the changeset is not possible or results in an error. - // * - // * Should be used for stacks containing nested stacks or when change set permissions aren't available. - // * - // * @default true - // */ - // readonly fallbackToTemplate?: boolean; + /** + * Enable falling back to template-based diff in case creating the changeset is not possible or results in an error. + * + * Should be used for stacks containing nested stacks or when change set permissions aren't available. + * + * @default true + */ + readonly fallbackToTemplate?: boolean; /** * Additional parameters for CloudFormation when creating a diff change set 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 f2a752004..3b5d72b41 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 @@ -58,7 +58,7 @@ async function cfnDiff( deployments: Deployments, options: DiffOptions, sdkProvider: SdkProvider, - changeSet: boolean, + includeChangeSet: boolean, ): Promise { const templateInfos = []; const methodOptions = (options.method?.options ?? {}) as ChangeSetDiffOptions; @@ -78,13 +78,23 @@ async function cfnDiff( removeNonImportResources(stack); } + const changeSet = includeChangeSet ? await changeSetDiff( + ioHelper, + deployments, + stack, + sdkProvider, + resourcesToImport, + methodOptions.parameters, + methodOptions.fallbackToTemplate, + ) : undefined; + templateInfos.push({ oldTemplate: currentTemplate, newTemplate: stack, stackName: stack.stackName, isImport: !!resourcesToImport, nestedStacks, - changeSet: changeSet ? await changeSetDiff(ioHelper, deployments, stack, sdkProvider, resourcesToImport, methodOptions.parameters) : undefined, + changeSet, }); } @@ -98,6 +108,7 @@ async function changeSetDiff( sdkProvider: SdkProvider, resourcesToImport?: ResourcesToImport, parameters: { [name: string]: string | undefined } = {}, + fallBackToTemplate: boolean = true, ): Promise { let stackExists = false; try { @@ -107,6 +118,10 @@ async function changeSetDiff( tryLookupRole: true, }); } catch (e: any) { + if (!fallBackToTemplate) { + throw new ToolkitError(`describeStacks call failed with ${e} for ${stack.stackName}, set fallBackToTemplate to true or use DiffMethod.templateOnly to base the diff on template differences.`); + } + await ioHelper.notify(IO.DEFAULT_TOOLKIT_DEBUG.msg(`Checking if the stack ${stack.stackName} exists before creating the changeset has failed, will base the diff on template differences.\n`)); await ioHelper.notify(IO.DEFAULT_TOOLKIT_DEBUG.msg(formatErrorMessage(e))); stackExists = false; @@ -121,9 +136,14 @@ async function changeSetDiff( sdkProvider, parameters: parameters, resourcesToImport, + fallBackToTemplate, }); } else { - await ioHelper.notify(IO.DEFAULT_TOOLKIT_DEBUG.msg(`the stack '${stack.stackName}' has not been deployed to CloudFormation or describeStacks call failed, skipping changeset creation.`)); + if (!fallBackToTemplate) { + throw new ToolkitError(`the stack '${stack.stackName}' has not been deployed to CloudFormation, set fallBackToTemplate to true or use DiffMethod.templateOnly to base the diff on template differences.`); + } + + await ioHelper.notify(IO.DEFAULT_TOOLKIT_DEBUG.msg(`the stack '${stack.stackName}' has not been deployed to CloudFormation, skipping changeset creation.`)); return; } } 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 8cf7eb9f0..64d314a07 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts @@ -1,5 +1,4 @@ import * as path from 'path'; -import { format } from 'util'; import * as chalk from 'chalk'; import { DiffMethod } from '../../lib/actions/diff'; import * as apis from '../../lib/api/shared-private'; @@ -7,7 +6,6 @@ import { RequireApproval } from '../../lib/api/shared-private'; import { StackSelectionStrategy } from '../../lib/api/shared-public'; import { Toolkit } from '../../lib/toolkit'; import { builderFixture, TestIoHost } from '../_helpers'; -import { MockSdk } from '../_helpers/mock-sdk'; let ioHost: TestIoHost; let toolkit: Toolkit; @@ -18,7 +16,6 @@ beforeEach(() => { ioHost.requireDeployApproval = RequireApproval.NEVER; toolkit = new Toolkit({ ioHost }); - const sdk = new MockSdk(); // Some default implementations jest.spyOn(apis.Deployments.prototype, 'readCurrentTemplateWithNestedStacks').mockResolvedValue({ @@ -177,7 +174,48 @@ describe('diff', () => { })); }); - describe('templatePath', () => { + describe('DiffMethod.ChangeSet', () => { + test('ChangeSet diff method falls back to template only if changeset not found', async () => { + // WHEN + ioHost.level = 'debug'; + const cx = await builderFixture(toolkit, 'stack-with-bucket'); + await toolkit.diff(cx, { + stacks: { strategy: StackSelectionStrategy.ALL_STACKS }, + method: DiffMethod.ChangeSet(), + }); + + // THEN + expect(ioHost.notifySpy).toHaveBeenCalledWith(expect.objectContaining({ + action: 'diff', + level: 'info', + code: 'CDK_TOOLKIT_I0000', + message: expect.stringContaining('Could not create a change set, will base the diff on template differences'), + })); + }); + + test('ChangeSet diff method throws if changeSet fails and fallBackToTemplate = false', async () => { + // WHEN + const cx = await builderFixture(toolkit, 'stack-with-bucket'); + await expect(async () => toolkit.diff(cx, { + stacks: { strategy: StackSelectionStrategy.ALL_STACKS }, + method: DiffMethod.ChangeSet({ fallbackToTemplate: false }), + })).rejects.toThrow(/Could not create a change set, set fallBackToTemplate=true/); + }); + + test('ChangeSet diff method throws if stack not found and fallBackToTemplate = false', async () => { + // GIVEN + jest.spyOn(apis.Deployments.prototype, 'stackExists').mockResolvedValue(false); + + // WHEN + const cx = await builderFixture(toolkit, 'stack-with-bucket'); + await expect(async () => toolkit.diff(cx, { + stacks: { strategy: StackSelectionStrategy.ALL_STACKS }, + method: DiffMethod.ChangeSet({ fallbackToTemplate: false }), + })).rejects.toThrow(/the stack 'Stack1' has not been deployed to CloudFormation/); + }); + }); + + describe('DiffMethod.LocalFile', () => { test('fails with multiple stacks', async () => { // WHEN + THEN const cx = await builderFixture(toolkit, 'two-empty-stacks'); From 201c7a3a22873cdc51526ef73cad5197e485ecee Mon Sep 17 00:00:00 2001 From: Kaizen Conroy Date: Fri, 11 Apr 2025 10:49:39 -0400 Subject: [PATCH 2/2] rename to be more generic --- .../src/api/deployments/cfn-api.ts | 14 +++++++------- .../lib/actions/diff/private/helpers.ts | 2 +- .../@aws-cdk/toolkit-lib/test/actions/diff.test.ts | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/deployments/cfn-api.ts b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/deployments/cfn-api.ts index bcdf22cb5..82453f361 100644 --- a/packages/@aws-cdk/tmp-toolkit-helpers/src/api/deployments/cfn-api.ts +++ b/packages/@aws-cdk/tmp-toolkit-helpers/src/api/deployments/cfn-api.ts @@ -143,13 +143,12 @@ export type PrepareChangeSetOptions = { parameters: { [name: string]: string | undefined }; resourcesToImport?: ResourcesToImport; /** - * Enable falling back to template-based diff in case creating the changeset is not possible or results in an error. + * Default behavior is to log AWS CloudFormation errors and move on. Set this property to true to instead + * fail on errors received by AWS CloudFormation. * - * Should be used for stacks containing nested stacks or when change set permissions aren't available. - * - * @default true + * @default false */ - fallBackToTemplate?: boolean; + failOnError?: boolean; } export type CreateChangeSetOptions = { @@ -248,7 +247,8 @@ async function uploadBodyParameterAndCreateChangeSet( role: executionRoleArn, }); } catch (e: any) { - if (options.fallBackToTemplate ?? true) { + // This function is currently only used by diff so these messages are diff-specific + if (!options.failOnError) { await ioHelper.notify(IO.DEFAULT_TOOLKIT_DEBUG.msg(String(e))); await ioHelper.notify(IO.DEFAULT_TOOLKIT_INFO.msg( 'Could not create a change set, will base the diff on template differences (run again with -v to see the reason)\n', @@ -257,7 +257,7 @@ async function uploadBodyParameterAndCreateChangeSet( return undefined; } - throw new ToolkitError('Could not create a change set, set fallBackToTemplate=true to base the diff on template differences\n', e); + throw new ToolkitError('Could not create a change set and failOnError is set. (run again with failOnError off to base the diff on template differences)\n', e); } } 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 3b5d72b41..5ff327cad 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 @@ -136,7 +136,7 @@ async function changeSetDiff( sdkProvider, parameters: parameters, resourcesToImport, - fallBackToTemplate, + failOnError: !fallBackToTemplate, }); } else { if (!fallBackToTemplate) { 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 e16deff1a..7442a8ccd 100644 --- a/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts +++ b/packages/@aws-cdk/toolkit-lib/test/actions/diff.test.ts @@ -200,7 +200,7 @@ describe('diff', () => { await expect(async () => toolkit.diff(cx, { stacks: { strategy: StackSelectionStrategy.ALL_STACKS }, method: DiffMethod.ChangeSet({ fallbackToTemplate: false }), - })).rejects.toThrow(/Could not create a change set, set fallBackToTemplate=true/); + })).rejects.toThrow(/Could not create a change set and failOnError is set/); }); test('ChangeSet diff method throws if stack not found and fallBackToTemplate = false', async () => {