diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/failing-changeset-app/app.js b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/failing-changeset-app/app.js new file mode 100644 index 0000000000000..daf7173eafdfe --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/failing-changeset-app/app.js @@ -0,0 +1,38 @@ +const cdk = require('aws-cdk-lib'); +const sns = require('aws-cdk-lib/aws-sns'); +/** + * This stack will be deployed in multiple phases, to achieve a very specific effect + * + * - Deploy Phase: a stack is deployed with an output set to a static string + * - Diff Phase: a reference to a non-existing macro is used for the output value + * + * To exercise this app: + * + * ``` + * npx cdk deploy + * env DIFF_PHASE=on npx cdk diff --no-fallback + * # This will surface an error to the user about the missing macro + * ``` + */ +class FailingChangesetStack extends cdk.Stack { + constructor(scope, id, props) { + super(scope, id, props); + + // Have at least one resource so that we can deploy this + new sns.Topic(this, 'topic', { + removalPolicy: cdk.RemovalPolicy.DESTROY, + }); + + const outputValue = process.env.DIFF_PHASE ? cdk.Fn.transform('NonExisting', { Param: 'Value' }) : 'static-string' + + new cdk.CfnOutput(this, 'MyOutput', { + value: outputValue + }) + } +} +const stackPrefix = process.env.STACK_NAME_PREFIX; +const app = new cdk.App(); + +new FailingChangesetStack(app, `${stackPrefix}-test-failing-changeset`); + +app.synth(); diff --git a/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/failing-changeset-app/cdk.json b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/failing-changeset-app/cdk.json new file mode 100644 index 0000000000000..f0075b1c9e33b --- /dev/null +++ b/packages/@aws-cdk-testing/cli-integ/resources/cdk-apps/failing-changeset-app/cdk.json @@ -0,0 +1,4 @@ +{ + "app": "node app.js", + "versionReporting": false +} 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 f4736584a065f..9a3fd6843f129 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=template-only', + 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=template-only', 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'; @@ -2931,3 +2951,40 @@ function actions(requests: CompletedRequest[]): string[] { .map(x => x.Action as string) .filter(action => action != null))]; } + +integTest( + 'cdk diff returns change-set creation errors when using --mode=change-set', + withSpecificFixture('failing-changeset-app', async (fixture) => { + // Should succeed + 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', '--mode=change-set', fixture.fullStackName('test-failing-changeset')], { + modEnv: { DIFF_PHASE: 'on' }, + allowErrExit: true, + captureStderr: true, + }); + expect(diffOutput).toContain('No transform named'); + + // Should throw + 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-failing-changeset')], { + modEnv: { DIFF_PHASE: 'on' }, + captureStderr: true, + allowErrExit: true, + }); + + expect(diffOutputWithFallback).toContain('will base the diff on template differences'); + + } finally { + await fixture.cdkDestroy('test-failing-changeset', { verbose: false }); + } + }), +); diff --git a/packages/aws-cdk/README.md b/packages/aws-cdk/README.md index a824c37697ef0..2fff6da4894bf 100644 --- a/packages/aws-cdk/README.md +++ b/packages/aws-cdk/README.md @@ -169,9 +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: + +- 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 `template-only` 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 `template-only` mode. Also note that this mode will always return an error when used against stacks that contain nested stacks. +- When set to `template-only`: 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 aa963292626f4..a9c89cd5bd6b2 100644 --- a/packages/aws-cdk/lib/api/util/cloudformation.ts +++ b/packages/aws-cdk/lib/api/util/cloudformation.ts @@ -353,9 +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') { - 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'); } } @@ -391,44 +389,35 @@ function templatesFromAssetManifestArtifact( async function uploadBodyParameterAndCreateChangeSet( options: PrepareChangeSetOptions, ): Promise { - try { - await uploadStackTemplateAssets(options.stack, options.deployments); - const env = await options.deployments.envs.accessStackForMutableStackOperations(options.stack); - - const bodyParameter = await makeBodyParameter( - options.stack, - env.resolvedEnvironment, - new AssetManifestBuilder(), - env.resources, - ); - const cfn = env.sdk.cloudFormation(); - const exists = (await CloudFormationStack.lookup(cfn, options.stack.stackName, false)).exists; - - const executionRoleArn = await env.replacePlaceholders(options.stack.cloudFormationExecutionRoleArn); - options.stream.write( - 'Hold on while we create a read-only change set to get a diff with accurate replacement information (use --no-change-set to use a less accurate but faster template-only diff)\n', - ); + await uploadStackTemplateAssets(options.stack, options.deployments); + const env = await options.deployments.envs.accessStackForMutableStackOperations(options.stack); + + const bodyParameter = await makeBodyParameter( + options.stack, + env.resolvedEnvironment, + new AssetManifestBuilder(), + env.resources, + ); + const cfn = env.sdk.cloudFormation(); + const exists = (await CloudFormationStack.lookup(cfn, options.stack.stackName, false)).exists; - return await createChangeSet({ - cfn, - changeSetName: 'cdk-diff-change-set', - stack: options.stack, - exists, - uuid: options.uuid, - willExecute: options.willExecute, - bodyParameter, - parameters: options.parameters, - resourcesToImport: options.resourcesToImport, - role: executionRoleArn, - }); - } catch (e: any) { - debug(e); - options.stream.write( - 'Could not create a change set, will base the diff on template differences (run again with -v to see the reason)\n', - ); + const executionRoleArn = await env.replacePlaceholders(options.stack.cloudFormationExecutionRoleArn); + options.stream.write( + 'Hold on while we create a read-only change set to get a diff with accurate replacement information (use --no-change-set to use a less accurate but faster template-only diff)\n', + ); - return undefined; - } + return createChangeSet({ + cfn, + changeSetName: 'cdk-diff-change-set', + stack: options.stack, + exists, + uuid: options.uuid, + willExecute: options.willExecute, + bodyParameter, + parameters: options.parameters, + resourcesToImport: options.resourcesToImport, + role: executionRoleArn, + }); } /** diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 8b0bd4b9a5026..0381c77b64971 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -199,7 +199,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({ @@ -218,16 +218,30 @@ export class CdkToolkit { } if (stackExists) { - changeSet = await createDiffChangeSet({ - stack, - uuid: uuid.v4(), - deployments: this.props.deployments, - willExecute: false, - sdkProvider: this.props.sdkProvider, - parameters: Object.assign({}, parameterMap['*'], parameterMap[stack.stackName]), - resourcesToImport, - stream, - }); + try { + changeSet = await createDiffChangeSet({ + stack, + uuid: uuid.v4(), + deployments: this.props.deployments, + willExecute: false, + sdkProvider: this.props.sdkProvider, + parameters: Object.assign({}, parameterMap['*'], parameterMap[stack.stackName]), + resourcesToImport, + stream, + }); + } catch (e: any) { + 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', + ); + } else { + stream.write( + `Could not create change set. Reason: ${e.message}\n`, + ); + return 1; + } + } } else { debug( `the stack '${stack.stackName}' has not been deployed to CloudFormation or describeStacks call failed, skipping changeset creation.`, @@ -1354,9 +1368,19 @@ export interface DiffOptions { /** * Whether or not to create, analyze, and subsequently delete a changeset * + * @deprecated - use `mode` option instead * @default true */ changeSet?: boolean; + + /** + * Auto mode will first attempt to use change-set mode, and if any error should occur it will fallback to template-only 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. + * Template-only mode compares the current local template with template applied on the stack + * + * @default 'auto' + */ + mode?: 'auto' | 'change-set' | 'template-only'; } interface CfnDeployOptions { diff --git a/packages/aws-cdk/lib/cli.ts b/packages/aws-cdk/lib/cli.ts index b1a40738fc819..7ba9f5a3211c5 100644 --- a/packages/aws-cdk/lib/cli.ts +++ b/packages/aws-cdk/lib/cli.ts @@ -242,6 +242,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 }, + '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=template-only instead', default: true }, + 'mode': { + type: 'string', + choices: ['auto', 'change-set', 'template-only'], + 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 template-only 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 ' + + 'Template-only 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 4b400aa844424..a6c14f3d131d0 100644 --- a/packages/aws-cdk/lib/convert-to-user-input.ts +++ b/packages/aws-cdk/lib/convert-to-user-input.ts @@ -184,6 +184,7 @@ export function convertYargsToUserInput(args: any): UserInput { processed: args.processed, quiet: args.quiet, changeSet: args.changeSet, + mode: args.mode, STACKS: args.STACKS, }; break; @@ -396,6 +397,7 @@ export function convertConfigToUserInput(config: any): UserInput { processed: config.diff?.processed, quiet: config.diff?.quiet, changeSet: config.diff?.changeSet, + 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 20b09694290fa..c30c7a6c49c3e 100644 --- a/packages/aws-cdk/lib/parse-command-line-arguments.ts +++ b/packages/aws-cdk/lib/parse-command-line-arguments.ts @@ -709,7 +709,17 @@ export function parseCommandLineArguments(args: Array): any { default: true, 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=template-only instead', + }) + .option('mode', { + default: 'auto', + type: 'string', + choices: ['auto', 'change-set', 'template-only'], + 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 template-only 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 Template-only 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 369a7fe325515..e9f29a62e416a 100644 --- a/packages/aws-cdk/lib/user-input.ts +++ b/packages/aws-cdk/lib/user-input.ts @@ -1090,14 +1090,22 @@ 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 * + * @deprecated Use mode=auto or mode=template-only instead * @default - true */ readonly changeSet?: boolean; + /** + * 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 template-only 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 Template-only mode compares the current local template with template applied on the stack + * + * @default - "auto" + */ + 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 e248de2f2386e..0391a2735d241 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 @@ -471,6 +470,30 @@ describe('non-nested stacks', () => { expect(buffer.data.trim()).toContain('✨ Number of stacks with differences: 1'); expect(exitCode).toBe(0); }); + + 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'); + createDiffChangeSet.mockImplementation(async () => { + throw new Error('Something went wrong'); + }); + + // GIVEN + const buffer = new StringWritable(); + + // WHEN + const exitCode = await toolkit.diff({ + stackNames: ['A'], + stream: buffer, + mode: 'change-set', + }); + + // THEN + expect(cloudFormation.stackExists).toHaveBeenCalled(); + expect(buffer.data.trim()).toContain('Something went wrong'); + expect(exitCode).toBe(1); + }); }); describe('stack exists checks', () => { @@ -561,6 +584,24 @@ describe('stack exists checks', () => { expect(cloudFormation.stackExists).not.toHaveBeenCalled(); }); + test('diff does not check for stack existence when --mode=template-only is passed', async () => { + // GIVEN + const buffer = new StringWritable(); + + // WHEN + const exitCode = await toolkit.diff({ + stackNames: ['A', 'A'], + stream: buffer, + fail: false, + quiet: true, + mode: 'template-only', + }); + + // 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(); @@ -573,7 +614,7 @@ describe('stack exists checks', () => { stream: buffer, fail: false, quiet: true, - changeSet: true, + mode: 'auto', }); // THEN @@ -598,7 +639,7 @@ describe('stack exists checks', () => { stream: buffer, fail: false, quiet: true, - changeSet: true, + mode: 'auto', }); // THEN @@ -879,7 +920,7 @@ describe('nested stacks', () => { const exitCode = await toolkit.diff({ stackNames: ['Parent'], stream: buffer, - changeSet: false, + mode: 'template-only', }); // THEN @@ -936,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 @@ -997,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();