From 9cc1a01d4777cfc0efdf31e82a58427cf96e6f36 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Mon, 12 Aug 2024 15:32:51 -0700 Subject: [PATCH 1/7] template obscurement --- packages/aws-cdk/lib/cdk-toolkit.ts | 37 ++++++++++++++++++++++++----- 1 file changed, 31 insertions(+), 6 deletions(-) diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 854b7ec6419c2..c72644d902cd3 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 > 2) { + delete template.Rules.CheckBootstrapVersion; + } else { + delete template.Rules; + } + } + } + + return template; +} From ac059d6bf5d1ba421b6966439887e055c60c13c6 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Mon, 12 Aug 2024 15:49:42 -0700 Subject: [PATCH 2/7] obscure --- packages/aws-cdk/lib/cdk-toolkit.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index c72644d902cd3..af64056e2fc29 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -1541,7 +1541,7 @@ 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 > 2) { + if (Object.keys(template.Rules).length > 1) { delete template.Rules.CheckBootstrapVersion; } else { delete template.Rules; From a5cc246db6f576e7a3ad0955ca1cf3b6f293915c Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Mon, 12 Aug 2024 16:15:24 -0700 Subject: [PATCH 3/7] obscure diff --- packages/aws-cdk/lib/diff.ts | 39 ++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/packages/aws-cdk/lib/diff.ts b/packages/aws-cdk/lib/diff.ts index 2fc043fcd38a3..36780a2596275 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; + }); + } +} From a68d7cbb28f4eebc04e5b461913b9c1db6e6d7f5 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Tue, 13 Aug 2024 14:17:19 -0700 Subject: [PATCH 4/7] cli-integ --- .../tests/cli-integ-tests/cli.integtest.ts | 14 +++ packages/aws-cdk/lib/diff.ts | 4 +- packages/aws-cdk/test/diff.test.ts | 89 +++++++++++++++++++ 3 files changed, 105 insertions(+), 2 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..9018ec1bd5345 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,20 @@ integTest('cdk synth', withDefaultFixture(async (fixture) => { }, })); + expect(fixture.template('test-1')).toEqual(expect.not.objectContaining({ + Resources: { + CDKMetadata: { + Type: 'AWS::CDK::Metadata', + }, + }, + })); + + expect(fixture.template('test-1')).toEqual(expect.not.objectContaining({ + Rules: { + CheckBootstrapVersion: expect.anything(), + }, + })); + 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/lib/diff.ts b/packages/aws-cdk/lib/diff.ts index 36780a2596275..0e9f1c15543dc 100644 --- a/packages/aws-cdk/lib/diff.ts +++ b/packages/aws-cdk/lib/diff.ts @@ -173,8 +173,8 @@ function obscureDiff(diff: TemplateDiff) { // 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; } + if (change.newValue?.CheckBootstrapVersion) { return false; } + if (change.oldValue?.CheckBootstrapVersion) { 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; From e66e5c45d3b16fab38dda8ddd13cc81f95a3ec31 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Wed, 14 Aug 2024 14:24:59 -0700 Subject: [PATCH 5/7] readme --- packages/aws-cdk/README.md | 2 ++ 1 file changed, 2 insertions(+) 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). From ab8189688774f7a65ea7668311c1490cbbf2d025 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Thu, 15 Aug 2024 12:24:43 -0700 Subject: [PATCH 6/7] integ test --- .../tests/cli-integ-tests/cli.integtest.ts | 18 +++++------------- 1 file changed, 5 insertions(+), 13 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 9018ec1bd5345..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,19 +102,11 @@ integTest('cdk synth', withDefaultFixture(async (fixture) => { }, })); - expect(fixture.template('test-1')).toEqual(expect.not.objectContaining({ - Resources: { - CDKMetadata: { - Type: 'AWS::CDK::Metadata', - }, - }, - })); - - expect(fixture.template('test-1')).toEqual(expect.not.objectContaining({ - Rules: { - CheckBootstrapVersion: expect.anything(), - }, - })); + 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({ From e959a42434480730285dba23c745f13777dd8067 Mon Sep 17 00:00:00 2001 From: Calvin Combs Date: Fri, 16 Aug 2024 09:30:07 -0700 Subject: [PATCH 7/7] CLI option text --- packages/aws-cdk/lib/cli.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 })