From 751a922de30e88a1faa7a20fb7b29cb43856d522 Mon Sep 17 00:00:00 2001 From: Calvin Combs <66279577+comcalvi@users.noreply.github.com> Date: Fri, 16 Aug 2024 17:13:34 -0700 Subject: [PATCH] feat(CLI): synth displays "AssertDescription: CDK bootstrap stack version 6 required" (#31092) ### Issue # (if applicable) Closes #17942. ### Reason for this change The CDK CLI shows the stack template, which includes the CFN Rule `CheckBootstrapVersion`. This rule will fail a deployment if the bootstrap is not right. Customers think this rule is an error message. ### Description of changes Obscure this `CheckBootstrapVersion` Rule from the template when we print it, if it exists. If it is the only Rule, remove the `Rules` section entirely. ### Description of how you validated changes Manual testing, unit tests, and CLI integration tests. ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../tests/cli-integ-tests/cli.integtest.ts | 6 ++ packages/aws-cdk/README.md | 2 + packages/aws-cdk/lib/cdk-toolkit.ts | 37 ++++++-- packages/aws-cdk/lib/cli.ts | 2 +- packages/aws-cdk/lib/diff.ts | 39 ++++++-- packages/aws-cdk/test/diff.test.ts | 89 +++++++++++++++++++ 6 files changed, 160 insertions(+), 15 deletions(-) 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 f323110eecfa4..609cf50d297c6 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 @@ -102,6 +102,12 @@ integTest('cdk synth', withDefaultFixture(async (fixture) => { }, })); + expect(await fixture.cdkSynth({ + options: [fixture.fullStackName('test-1')], + })).not.toEqual(expect.stringContaining(` +Rules: + CheckBootstrapVersion:`)); + await fixture.cdk(['synth', fixture.fullStackName('test-2')], { verbose: false }); expect(fixture.template('test-2')).toEqual(expect.objectContaining({ Resources: { diff --git a/packages/aws-cdk/README.md b/packages/aws-cdk/README.md index 2b06a12c6b2d4..25a058a2fcbd1 100644 --- a/packages/aws-cdk/README.md +++ b/packages/aws-cdk/README.md @@ -165,6 +165,8 @@ $ # Diff against the currently deployed stack with quiet parameter enabled $ 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). diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 854b7ec6419c2..af64056e2fc29 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -105,7 +105,7 @@ export class CdkToolkit { public async metadata(stackName: string, json: boolean) { const stacks = await this.selectSingleStackByName(stackName); - data(serializeStructure(stacks.firstStack.manifest.metadata ?? {}, json)); + printSerializedObject(stacks.firstStack.manifest.metadata ?? {}, json); } public async acknowledge(noticeId: string) { @@ -632,7 +632,7 @@ export class CdkToolkit { }); if (options.long && options.showDeps) { - data(serializeStructure(stacks, options.json ?? false)); + printSerializedObject(stacks, options.json ?? false); return 0; } @@ -646,7 +646,7 @@ export class CdkToolkit { }); } - data(serializeStructure(stackDeps, options.json ?? false)); + printSerializedObject(stackDeps, options.json ?? false); return 0; } @@ -660,7 +660,7 @@ export class CdkToolkit { environment: stack.environment, }); } - data(serializeStructure(long, options.json ?? false)); + printSerializedObject(long, options.json ?? false); return 0; } @@ -687,7 +687,7 @@ export class CdkToolkit { // if we have a single stack, print it to STDOUT if (stacks.stackCount === 1) { if (!quiet) { - data(serializeStructure(stacks.firstStack.template, json ?? false)); + printSerializedObject(obscureTemplate(stacks.firstStack.template), json ?? false); } return undefined; } @@ -701,7 +701,7 @@ export class CdkToolkit { // behind an environment variable. const isIntegMode = process.env.CDK_INTEG_MODE === '1'; if (isIntegMode) { - data(serializeStructure(stacks.stackArtifacts.map(s => s.template), json ?? false)); + printSerializedObject(stacks.stackArtifacts.map(s => obscureTemplate(s.template)), json ?? false); } // not outputting template to stdout, let's explain things to the user a little bit... @@ -1045,6 +1045,13 @@ export class CdkToolkit { } } +/** + * Print a serialized object (YAML or JSON) to stdout. + */ +function printSerializedObject(obj: any, json: boolean) { + data(serializeStructure(obj, json)); +} + export interface DiffOptions { /** * Stack names to diff @@ -1526,3 +1533,21 @@ function buildParameterMap(parameters: { return parameterMap; } + +/** + * Remove any template elements that we don't want to show users. + */ +function obscureTemplate(template: any = {}) { + if (template.Rules) { + // see https://github.com/aws/aws-cdk/issues/17942 + if (template.Rules.CheckBootstrapVersion) { + if (Object.keys(template.Rules).length > 1) { + delete template.Rules.CheckBootstrapVersion; + } else { + delete template.Rules; + } + } + } + + return template; +} diff --git a/packages/aws-cdk/lib/cli.ts b/packages/aws-cdk/lib/cli.ts index 2c15bb7b9949f..a05a0cbb4625c 100644 --- a/packages/aws-cdk/lib/cli.ts +++ b/packages/aws-cdk/lib/cli.ts @@ -260,7 +260,7 @@ async function parseCommandLineArguments(args: string[]) { .option('exclusively', { type: 'boolean', alias: 'e', desc: 'Only diff requested stacks, don\'t include dependencies' }) .option('context-lines', { type: 'number', desc: 'Number of context lines to include in arbitrary JSON diff rendering', default: 3, requiresArg: true }) .option('template', { type: 'string', desc: 'The path to the CloudFormation template to compare with', requiresArg: true }) - .option('strict', { type: 'boolean', desc: 'Do not filter out AWS::CDK::Metadata resources or mangled non-ASCII characters', default: false }) + .option('strict', { type: 'boolean', desc: 'Do not filter out AWS::CDK::Metadata resources, mangled non-ASCII characters, or the CheckBootstrapVersionRule', default: false }) .option('security-only', { type: 'boolean', desc: 'Only diff for broadened security changes', default: false }) .option('fail', { type: 'boolean', desc: 'Fail with exit code 1 in case of diff' }) .option('processed', { type: 'boolean', desc: 'Whether to compare against the template with Transforms already processed', default: false }) diff --git a/packages/aws-cdk/lib/diff.ts b/packages/aws-cdk/lib/diff.ts index 2fc043fcd38a3..0e9f1c15543dc 100644 --- a/packages/aws-cdk/lib/diff.ts +++ b/packages/aws-cdk/lib/diff.ts @@ -19,7 +19,7 @@ import { print, warning } from './logging'; * * @param oldTemplate the old/current state of the stack. * @param newTemplate the new/target state of the stack. - * @param strict do not filter out AWS::CDK::Metadata + * @param strict do not filter out AWS::CDK::Metadata or Rules * @param context lines of context to use in arbitrary JSON diff * @param quiet silences \'There were no differences\' messages * @@ -50,13 +50,9 @@ export function printStackDiff( } // filter out 'AWS::CDK::Metadata' resources from the template - if (diff.resources && !strict) { - diff.resources = diff.resources.filter(change => { - if (!change) { return true; } - if (change.newResourceType === 'AWS::CDK::Metadata') { return false; } - if (change.oldResourceType === 'AWS::CDK::Metadata') { return false; } - return true; - }); + // filter out 'CheckBootstrapVersion' rules from the template + if (!strict) { + obscureDiff(diff); } let stackDiffCount = 0; @@ -165,3 +161,30 @@ function logicalIdMapFromTemplate(template: any) { } return ret; } + +/** + * Remove any template elements that we don't want to show users. + * This is currently: + * - AWS::CDK::Metadata resource + * - CheckBootstrapVersion Rule + */ +function obscureDiff(diff: TemplateDiff) { + if (diff.unknown) { + // see https://github.com/aws/aws-cdk/issues/17942 + diff.unknown = diff.unknown.filter(change => { + if (!change) { return true; } + if (change.newValue?.CheckBootstrapVersion) { return false; } + if (change.oldValue?.CheckBootstrapVersion) { return false; } + return true; + }); + } + + if (diff.resources) { + diff.resources = diff.resources.filter(change => { + if (!change) { return true; } + if (change.newResourceType === 'AWS::CDK::Metadata') { return false; } + if (change.oldResourceType === 'AWS::CDK::Metadata') { return false; } + return true; + }); + } +} diff --git a/packages/aws-cdk/test/diff.test.ts b/packages/aws-cdk/test/diff.test.ts index 0155f74dd192d..ffaa157e5fc20 100644 --- a/packages/aws-cdk/test/diff.test.ts +++ b/packages/aws-cdk/test/diff.test.ts @@ -855,6 +855,95 @@ Resources }); }); +describe('--strict', () => { + const templatePath = 'oldTemplate.json'; + beforeEach(() => { + const oldTemplate = {}; + + cloudExecutable = new MockCloudExecutable({ + stacks: [{ + stackName: 'A', + template: { + Resources: { + MetadataResource: { + Type: 'AWS::CDK::Metadata', + Properties: { + newMeta: 'newData', + }, + }, + SomeOtherResource: { + Type: 'AWS::Something::Amazing', + }, + }, + Rules: { + CheckBootstrapVersion: { + newCheck: 'newBootstrapVersion', + }, + }, + }, + }], + }); + + toolkit = new CdkToolkit({ + cloudExecutable, + deployments: cloudFormation, + configuration: cloudExecutable.configuration, + sdkProvider: cloudExecutable.sdkProvider, + }); + + fs.writeFileSync(templatePath, JSON.stringify(oldTemplate)); + }); + + afterEach(() => fs.rmSync(templatePath)); + + test('--strict does not obscure CDK::Metadata or CheckBootstrapVersion', async () => { + // GIVEN + const buffer = new StringWritable(); + + // WHEN + const exitCode = await toolkit.diff({ + stackNames: ['A'], + stream: buffer, + strict: true, + }); + + // THEN + const plainTextOutput = buffer.data.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, ''); + expect(plainTextOutput.trim()).toEqual(`Stack A +Resources +[+] AWS::CDK::Metadata MetadataResource +[+] AWS::Something::Amazing SomeOtherResource + +Other Changes +[+] Unknown Rules: {\"CheckBootstrapVersion\":{\"newCheck\":\"newBootstrapVersion\"}} + + +✨ Number of stacks with differences: 1`); + expect(exitCode).toBe(0); + }); + + test('--no-strict obscures CDK::Metadata and CheckBootstrapVersion', async () => { + // GIVEN + const buffer = new StringWritable(); + + // WHEN + const exitCode = await toolkit.diff({ + stackNames: ['A'], + stream: buffer, + }); + + // THEN + const plainTextOutput = buffer.data.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, ''); + expect(plainTextOutput.trim()).toEqual(`Stack A +Resources +[+] AWS::Something::Amazing SomeOtherResource + + +✨ Number of stacks with differences: 1`); + expect(exitCode).toBe(0); + }); +}); + class StringWritable extends Writable { public data: string; private readonly _decoder: StringDecoder;