From 7a8dc32e204d9d2f634452f766570c6d309c059d Mon Sep 17 00:00:00 2001 From: Matteo Sessa Date: Tue, 14 Jan 2025 12:37:40 +1100 Subject: [PATCH] feat: replace --no-fallback option with --mode switch --- .../app.js | 4 +- .../cdk.json | 0 .../tests/cli-integ-tests/cli.integtest.ts | 34 +++++++--- packages/aws-cdk/README.md | 8 +-- .../aws-cdk/lib/api/util/cloudformation.ts | 5 +- packages/aws-cdk/lib/cdk-toolkit.ts | 13 ++-- packages/aws-cdk/lib/cli.ts | 2 +- packages/aws-cdk/lib/config.ts | 14 ++++- packages/aws-cdk/lib/convert-to-user-input.ts | 4 +- .../lib/parse-command-line-arguments.ts | 17 +++-- packages/aws-cdk/lib/user-input.ts | 11 ++-- packages/aws-cdk/test/diff.test.ts | 62 +++++++++++++++---- 12 files changed, 125 insertions(+), 49 deletions(-) rename packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/{diff-no-fallback-app => failing-changeset-app}/app.js (89%) rename packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/{diff-no-fallback-app => failing-changeset-app}/cdk.json (100%) diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/diff-no-fallback-app/app.js b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/failing-changeset-app/app.js similarity index 89% rename from packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/diff-no-fallback-app/app.js rename to packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/failing-changeset-app/app.js index 599be28a88b20..daf7173eafdfe 100644 --- a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/diff-no-fallback-app/app.js +++ b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/failing-changeset-app/app.js @@ -14,7 +14,7 @@ const sns = require('aws-cdk-lib/aws-sns'); * # This will surface an error to the user about the missing macro * ``` */ -class DiffNoFallbackTestStack extends cdk.Stack { +class FailingChangesetStack extends cdk.Stack { constructor(scope, id, props) { super(scope, id, props); @@ -33,6 +33,6 @@ class DiffNoFallbackTestStack extends cdk.Stack { const stackPrefix = process.env.STACK_NAME_PREFIX; const app = new cdk.App(); -new DiffNoFallbackTestStack(app, `${stackPrefix}-test-diff-no-fallback`); +new FailingChangesetStack(app, `${stackPrefix}-test-failing-changeset`); app.synth(); diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/diff-no-fallback-app/cdk.json b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/failing-changeset-app/cdk.json similarity index 100% rename from packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/diff-no-fallback-app/cdk.json rename to packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/failing-changeset-app/cdk.json diff --git a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts index 8e99f59080269..657711b4f48ac 100644 --- a/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts +++ b/packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/cli.integtest.ts @@ -1300,6 +1300,26 @@ integTest( }), ); +integTest( + 'cdk diff shows resource metadata changes with --mode=local', + withDefaultFixture(async (fixture) => { + + // GIVEN - small initial stack with default resource metadata + await fixture.cdkDeploy('metadata'); + + // WHEN - changing resource metadata value + const diff = await fixture.cdk(['diff --mode=local', fixture.fullStackName('metadata')], { + verbose: true, + modEnv: { + INTEG_METADATA_VALUE: 'custom', + }, + }); + + // Assert there are changes + expect(diff).not.toContain('There were no differences'); + }), +); + integTest('cdk diff with large changeset and custom toolkit stack name and qualifier does not fail', withoutBootstrap(async (fixture) => { // Bootstrapping with custom toolkit stack name and qualifier const qualifier = 'abc1111'; @@ -2933,15 +2953,15 @@ function actions(requests: CompletedRequest[]): string[] { } integTest( - 'cdk diff noFallback', - withSpecificFixture('diff-no-fallback-app', async (fixture) => { + 'cdk diff returns change-set creation errors when using --mode=change-set', + withSpecificFixture('failing-changeset-app', async (fixture) => { // Should succeed - await fixture.cdkDeploy('test-diff-no-fallback', { + await fixture.cdkDeploy('test-failing-changeset', { verbose: false, }); try { // Should surface the missing transform error from cloudformation in the output - const diffOutput = await fixture.cdk(['diff', '--no-fallback', fixture.fullStackName('test-diff-no-fallback')], { + const diffOutput = await fixture.cdk(['diff', '--mode=change-set', fixture.fullStackName('test-failing-changeset')], { modEnv: { DIFF_PHASE: 'on' }, allowErrExit: true, captureStderr: true, @@ -2949,13 +2969,13 @@ integTest( expect(diffOutput).toContain('No transform named'); // Should throw - await expect(fixture.cdk(['diff', '--no-fallback', fixture.fullStackName('test-diff-no-fallback')], { + await expect(fixture.cdk(['diff', '--mode=change-set', fixture.fullStackName('test-failing-changeset')], { modEnv: { DIFF_PHASE: 'on' }, captureStderr: true, })).rejects.toThrow(); // Should fallback to template-only diff as normal - const diffOutputWithFallback = await fixture.cdk(['diff', fixture.fullStackName('test-diff-no-fallback')], { + const diffOutputWithFallback = await fixture.cdk(['diff', fixture.fullStackName('test-failing-changeset')], { modEnv: { DIFF_PHASE: 'on' }, captureStderr: true, allowErrExit: true, @@ -2964,7 +2984,7 @@ integTest( expect(diffOutputWithFallback).toContain('will base the diff on template differences'); } finally { - await fixture.cdkDestroy('test-diff-no-fallback', { verbose: false }); + await fixture.cdkDestroy('test-failing-changeset', { verbose: false }); } }), ); diff --git a/packages/aws-cdk/README.md b/packages/aws-cdk/README.md index 86c0adb6f9e0c..804c89fcd1ffd 100644 --- a/packages/aws-cdk/README.md +++ b/packages/aws-cdk/README.md @@ -169,11 +169,11 @@ $ cdk diff --quiet --app='node bin/main.js' MyStackName Note that the CDK::Metadata resource and the `CheckBootstrapVersion` Rule are excluded from `cdk diff` by default. You can force `cdk diff` to display them by passing the `--strict` flag. -The `change-set` flag will make `diff` create a change set and extract resource replacement data from it. This is a bit slower, but will provide no false positives for resource replacement. -The `--no-change-set` mode will consider any change to a property that requires replacement to be a resource replacement, -even if the change is purely cosmetic (like replacing a resource reference with a hardcoded arn). +The `mode` option selects the approach that will be used to analyze the differences: -If the `change-set` flag is enabled and an error is encountered during changeset creation, CDK will automatically fall back to template-only diff. This behaviour can be disabled by passing the `--no-fallback` flag. +- When set to `auto`: CDK will first attempt to create a ChangeSet, should that not be possible due to the stack not yet being created, or any error encountered during the creation of the StackSet, it will automatically fallback to `local` mode. +- When set to `change-set`: CDK will create a change set and extract resource replacement data from it. This is a bit slower, but will provide no false positives for resource replacement. Errors in creating a change-set will result in a non-zero exit code. Note that when using this mode against a stack that has not been created will still result in a fallback to `local` mode. Also note that this mode will always return an error when used against stacks that contain nested stacks. +- When set to `local`: CDK will compare the local template with the template currently applied to the stack. Note that this mode will consider any change to a property that requires replacement to be a resource replacement, even if the change is purely cosmetic (like replacing a resource reference with a hardcoded arn) ### `cdk deploy` diff --git a/packages/aws-cdk/lib/api/util/cloudformation.ts b/packages/aws-cdk/lib/api/util/cloudformation.ts index d190dbd59eae6..a9c89cd5bd6b2 100644 --- a/packages/aws-cdk/lib/api/util/cloudformation.ts +++ b/packages/aws-cdk/lib/api/util/cloudformation.ts @@ -353,10 +353,7 @@ export async function createDiffChangeSet( // This causes CreateChangeSet to fail with `Template Error: Fn::Equals cannot be partially collapsed`. for (const resource of Object.values(options.stack.template.Resources ?? {})) { if ((resource as any).Type === 'AWS::CloudFormation::Stack') { - // eslint-disable-next-line no-console - debug('This stack contains one or more nested stacks, falling back to template-only diff...'); - - return undefined; + throw new Error('Cannot create change-set diff when using nested stacks'); } } diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 73024d83b99e2..8c9a2a88cc4a5 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -193,7 +193,7 @@ export class CdkToolkit { let changeSet = undefined; - if (options.changeSet) { + if (options.changeSet || options.mode === 'auto' || options.mode === 'change-set') { let stackExists = false; try { stackExists = await this.props.deployments.stackExists({ @@ -224,7 +224,7 @@ export class CdkToolkit { stream, }); } catch (e: any) { - if (options.fallback ?? true) { + if (options.mode === 'auto') { debug(e); stream.write( 'Could not create a change set, will base the diff on template differences (run again with -v to see the reason)\n', @@ -1427,16 +1427,19 @@ export interface DiffOptions { /** * Whether or not to create, analyze, and subsequently delete a changeset * + * @deprecated - use `mode` option instead * @default true */ changeSet?: boolean; /** - * If the changeset option is turned on, whether to enable falling back to template-only diff in case creating the changeset results in an error + * Auto mode will first attempt to use change-set mode, and if any error should occur it will fallback to local mode. + * Change-set mode will use a change-set to analyze resource replacements. In this mode, diff will use the deploy role instead of the lookup role. + * Local mode compares the current local template with template applied on the stack * - * @default true + * @default 'auto' */ - fallback?: boolean; + mode?: 'auto' | 'change-set' | 'local'; } interface CfnDeployOptions { diff --git a/packages/aws-cdk/lib/cli.ts b/packages/aws-cdk/lib/cli.ts index b4c83cbf07ffd..eb1a77cc9e1fe 100644 --- a/packages/aws-cdk/lib/cli.ts +++ b/packages/aws-cdk/lib/cli.ts @@ -239,7 +239,7 @@ export async function exec(args: string[], synthesizer?: Synthesizer): Promise { 'fail': { type: 'boolean', desc: 'Fail with exit code 1 in case of diff' }, 'processed': { type: 'boolean', desc: 'Whether to compare against the template with Transforms already processed', default: false }, 'quiet': { type: 'boolean', alias: 'q', desc: 'Do not print stack name and default message when there is no diff to stdout', default: false }, - 'change-set': { type: 'boolean', alias: 'changeset', desc: 'Whether to create a changeset to analyze resource replacements. In this mode, diff will use the deploy role instead of the lookup role.', default: true }, - 'fallback': { type: 'boolean', desc: 'If the changeset option is turned on, whether to enable falling back to template-only diff in case creating the changeset results in an error.', default: true }, + 'change-set': { type: 'boolean', alias: 'changeset', desc: 'Whether to create a changeset to analyze resource replacements. In this mode, diff will use the deploy role instead of the lookup role', conflicts: 'mode', deprecated: 'Use mode=auto or mode=local instead' }, + 'mode': { + type: 'string', + choices: ['auto', 'change-set', 'local'], + default: 'auto', + conflicts: 'change-set', + requiresArg: true, + desc: 'How to perform the the diff operation. ' + + 'Auto mode will first attempt to use change-set mode, and if any error should occur it will fallback to local mode. ' + + 'Change-set mode will use a change-set to analyze resource replacements. In this mode, diff will use the deploy role instead of the lookup role. Unhandled errors in change-set creation will return a non-zero exit code ' + + 'Local mode compares the current local template with template applied on the stack', + }, }, }, metadata: { diff --git a/packages/aws-cdk/lib/convert-to-user-input.ts b/packages/aws-cdk/lib/convert-to-user-input.ts index eb28b48d78425..a6c14f3d131d0 100644 --- a/packages/aws-cdk/lib/convert-to-user-input.ts +++ b/packages/aws-cdk/lib/convert-to-user-input.ts @@ -184,7 +184,7 @@ export function convertYargsToUserInput(args: any): UserInput { processed: args.processed, quiet: args.quiet, changeSet: args.changeSet, - fallback: args.fallback, + mode: args.mode, STACKS: args.STACKS, }; break; @@ -397,7 +397,7 @@ export function convertConfigToUserInput(config: any): UserInput { processed: config.diff?.processed, quiet: config.diff?.quiet, changeSet: config.diff?.changeSet, - fallback: config.diff?.fallback, + mode: config.diff?.mode, }; const metadataOptions = {}; const acknowledgeOptions = {}; diff --git a/packages/aws-cdk/lib/parse-command-line-arguments.ts b/packages/aws-cdk/lib/parse-command-line-arguments.ts index 32961d9707cb7..fdc08ebfe6f00 100644 --- a/packages/aws-cdk/lib/parse-command-line-arguments.ts +++ b/packages/aws-cdk/lib/parse-command-line-arguments.ts @@ -706,15 +706,20 @@ export function parseCommandLineArguments(args: Array): any { desc: 'Do not print stack name and default message when there is no diff to stdout', }) .option('change-set', { - default: true, + default: undefined, type: 'boolean', alias: 'changeset', - desc: 'Whether to create a changeset to analyze resource replacements. In this mode, diff will use the deploy role instead of the lookup role.', + desc: 'Whether to create a changeset to analyze resource replacements. In this mode, diff will use the deploy role instead of the lookup role', + conflicts: 'mode', + deprecated: 'Use mode=auto or mode=local instead', }) - .option('fallback', { - default: true, - type: 'boolean', - desc: 'If the changeset option is turned on, whether to enable falling back to template-only diff in case creating the changeset results in an error.', + .option('mode', { + default: 'auto', + type: 'string', + choices: ['auto', 'change-set', 'local'], + conflicts: 'change-set', + requiresArg: true, + desc: 'How to perform the the diff operation. Auto mode will first attempt to use change-set mode, and if any error should occur it will fallback to local mode. Change-set mode will use a change-set to analyze resource replacements. In this mode, diff will use the deploy role instead of the lookup role. Unhandled errors in change-set creation will return a non-zero exit code Local mode compares the current local template with template applied on the stack', }), ) .command('metadata [STACK]', 'Returns all metadata associated with this stack') diff --git a/packages/aws-cdk/lib/user-input.ts b/packages/aws-cdk/lib/user-input.ts index 0b62b23a9517f..dbd693d4c0735 100644 --- a/packages/aws-cdk/lib/user-input.ts +++ b/packages/aws-cdk/lib/user-input.ts @@ -1090,20 +1090,21 @@ export interface DiffOptions { readonly quiet?: boolean; /** - * Whether to create a changeset to analyze resource replacements. In this mode, diff will use the deploy role instead of the lookup role. + * Whether to create a changeset to analyze resource replacements. In this mode, diff will use the deploy role instead of the lookup role * * aliases: changeset * - * @default - true + * @deprecated Use mode=auto or mode=local instead + * @default - undefined */ readonly changeSet?: boolean; /** - * If the changeset option is turned on, whether to enable falling back to template-only diff in case creating the changeset results in an error. + * How to perform the the diff operation. Auto mode will first attempt to use change-set mode, and if any error should occur it will fallback to local mode. Change-set mode will use a change-set to analyze resource replacements. In this mode, diff will use the deploy role instead of the lookup role. Unhandled errors in change-set creation will return a non-zero exit code Local mode compares the current local template with template applied on the stack * - * @default - true + * @default - "auto" */ - readonly fallback?: boolean; + readonly mode?: string; /** * Positional argument for diff diff --git a/packages/aws-cdk/test/diff.test.ts b/packages/aws-cdk/test/diff.test.ts index 0727741df94ea..45293b40e382d 100644 --- a/packages/aws-cdk/test/diff.test.ts +++ b/packages/aws-cdk/test/diff.test.ts @@ -88,7 +88,6 @@ describe('fixed template', () => { const exitCode = await toolkit.diff({ stackNames: ['A'], stream: buffer, - changeSet: undefined, templatePath, }); @@ -204,7 +203,7 @@ describe('imports', () => { const exitCode = await toolkit.diff({ stackNames: ['A'], stream: buffer, - changeSet: true, + mode: 'auto', }); // THEN @@ -231,7 +230,7 @@ Resources const exitCode = await toolkit.diff({ stackNames: ['A'], stream: buffer, - changeSet: true, + mode: 'auto', }); // THEN @@ -472,7 +471,7 @@ describe('non-nested stacks', () => { expect(exitCode).toBe(0); }); - test('when changeset creation fails but fallback to template only diff is disabled, the error is surfaced', async () => { + test('when changeset creation fails and mode=change-set, the error is surfaced', async () => { cloudFormation.stackExists = jest.fn().mockReturnValue(Promise.resolve(true)); const createDiffChangeSet = jest.spyOn(cfn, 'createDiffChangeSet'); @@ -487,8 +486,7 @@ describe('non-nested stacks', () => { const exitCode = await toolkit.diff({ stackNames: ['A'], stream: buffer, - changeSet: true, - fallback: false, + mode: 'change-set', }); // THEN @@ -586,6 +584,24 @@ describe('stack exists checks', () => { expect(cloudFormation.stackExists).not.toHaveBeenCalled(); }); + test('diff does not check for stack existence when --mode=local is passed', async () => { + // GIVEN + const buffer = new StringWritable(); + + // WHEN + const exitCode = await toolkit.diff({ + stackNames: ['A', 'A'], + stream: buffer, + fail: false, + quiet: true, + mode: 'local', + }); + + // THEN + expect(exitCode).toBe(0); + expect(cloudFormation.stackExists).not.toHaveBeenCalled(); + }); + test('diff falls back to classic diff when stack does not exist', async () => { // GIVEN const buffer = new StringWritable(); @@ -598,7 +614,7 @@ describe('stack exists checks', () => { stream: buffer, fail: false, quiet: true, - changeSet: true, + mode: 'auto', }); // THEN @@ -623,7 +639,7 @@ describe('stack exists checks', () => { stream: buffer, fail: false, quiet: true, - changeSet: true, + mode: 'auto', }); // THEN @@ -904,7 +920,7 @@ describe('nested stacks', () => { const exitCode = await toolkit.diff({ stackNames: ['Parent'], stream: buffer, - changeSet: false, + mode: 'local', }); // THEN @@ -961,18 +977,20 @@ Stack UnChangedChild test('diff falls back to non-changeset diff for nested stacks', async () => { // GIVEN const changeSetSpy = jest.spyOn(cfn, 'waitForChangeSet'); + jest.spyOn(cloudFormation, 'stackExists').mockResolvedValue(true); const buffer = new StringWritable(); // WHEN const exitCode = await toolkit.diff({ stackNames: ['Parent'], stream: buffer, - changeSet: true, + mode: 'auto', }); // THEN const plainTextOutput = buffer.data.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '').replace(/[ \t]+$/gm, ''); - expect(plainTextOutput.trim()).toEqual(`Stack Parent + expect(plainTextOutput.trim()).toEqual(`Could not create a change set, will base the diff on template differences (run again with -v to see the reason) +Stack Parent Resources [~] AWS::CloudFormation::Stack AdditionChild └─ [~] TemplateURL @@ -1022,6 +1040,28 @@ Stack UnChangedChild expect(changeSetSpy).not.toHaveBeenCalled(); }); + test('diff falls back to non-changeset diff for nested stacks and mode=change-set', async () => { + // GIVEN + const changeSetSpy = jest.spyOn(cfn, 'waitForChangeSet'); + jest.spyOn(cloudFormation, 'stackExists').mockResolvedValue(true); + + const buffer = new StringWritable(); + + // WHEN + const exitCode = await toolkit.diff({ + stackNames: ['Parent'], + stream: buffer, + mode: 'change-set', + }); + + // THEN + const plainTextOutput = buffer.data.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '').replace(/[ \t]+$/gm, ''); + expect(plainTextOutput.trim()).toContain('Cannot create change-set diff when using nested stacks'); + + expect(exitCode).toBe(1); + expect(changeSetSpy).not.toHaveBeenCalled(); + }); + test('when quiet mode is enabled, nested stacks with no diffs should not print stack name & no differences to stdout', async () => { // GIVEN const buffer = new StringWritable();