From 8338c2a4a193cb36d40122bdf0378cf7d7eb7f35 Mon Sep 17 00:00:00 2001 From: Parker Scanlon <69879391+scanlonp@users.noreply.github.com> Date: Wed, 6 Mar 2024 18:12:38 -0800 Subject: [PATCH] prevent changeset diff for non-deployed stacks --- .../cloudformation-diff/lib/diff-template.ts | 29 ++++-- packages/aws-cdk/lib/cdk-toolkit.ts | 26 ++++- packages/aws-cdk/lib/diff.ts | 5 +- packages/aws-cdk/test/diff.test.ts | 94 ------------------- 4 files changed, 48 insertions(+), 106 deletions(-) diff --git a/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts b/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts index 734753b1d7fc9..1902d757f486d 100644 --- a/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts +++ b/packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts @@ -37,6 +37,7 @@ const DIFF_HANDLERS: HandlerRegistry = { * @param currentTemplate the current state of the stack. * @param newTemplate the target state of the stack. * @param changeSet the change set for this stack. + * @param isImport if the stack is importing resources (a migrate stack). * * @returns a +types.TemplateDiff+ object that represents the changes that will happen if * a stack which current state is described by +currentTemplate+ is updated with @@ -46,6 +47,7 @@ export function fullDiff( currentTemplate: { [key: string]: any }, newTemplate: { [key: string]: any }, changeSet?: CloudFormation.DescribeChangeSetOutput, + isImport?: boolean, ): types.TemplateDiff { normalize(currentTemplate); @@ -55,6 +57,9 @@ export function fullDiff( filterFalsePositivies(theDiff, changeSet); addImportInformation(theDiff, changeSet); } + if (isImport) { + addImportInformation(theDiff); + } return theDiff; } @@ -209,13 +214,25 @@ function deepCopy(x: any): any { return x; } -function addImportInformation(diff: types.TemplateDiff, changeSet: CloudFormation.DescribeChangeSetOutput) { - const imports = findResourceImports(changeSet); - diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => { - if (imports.includes(logicalId)) { +/** + * Sets import flag to true for resource imports. + * When the changeset parameter is not set, the stack is a new migrate stack, + * so all resource changes are imports. + */ +function addImportInformation(diff: types.TemplateDiff, changeSet?: CloudFormation.DescribeChangeSetOutput) { + if (changeSet) { + const imports = findResourceImports(changeSet); + diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => { + if (imports.includes(logicalId)) { + change.isImport = true; + } + }); + } else { + diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => { + logicalId; // dont know how to get past warning that this variable is not used. change.isImport = true; - } - }); + }); + } } function filterFalsePositivies(diff: types.TemplateDiff, changeSet: CloudFormation.DescribeChangeSetOutput) { diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index ea3931fc6ddc3..8119b0f2d4d42 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -135,7 +135,14 @@ export class CdkToolkit { throw new Error(`There is no file at ${options.templatePath}`); } - const changeSet = options.changeSet ? await createDiffChangeSet({ + const stackExistsOptions = { + stack: stacks.firstStack, + deployName: stacks.firstStack.stackName, + }; + + const stackExists = await this.props.deployments.stackExists(stackExistsOptions); + + const changeSet = (stackExists && options.changeSet) ? await createDiffChangeSet({ stack: stacks.firstStack, uuid: uuid.v4(), willExecute: false, @@ -167,13 +174,23 @@ export class CdkToolkit { removeNonImportResources(stack); } - const changeSet = options.changeSet ? await createDiffChangeSet({ + const stackExistsOptions = { + stack, + deployName: stack.stackName, + }; + + const stackExists = await this.props.deployments.stackExists(stackExistsOptions); + + // if the stack does not already exist, do not do a changeset + // this prevents race conditions between deleting the dummy changeset stack and deploying the real changeset stack + // migrate stacks that import resources will not previously exist and default to old diff logic + const changeSet = (stackExists && options.changeSet) ? await createDiffChangeSet({ stack, uuid: uuid.v4(), deployments: this.props.deployments, willExecute: false, sdkProvider: this.props.sdkProvider, - parameters: Object.assign({}, parameterMap['*'], parameterMap[stacks.firstStack.stackName]), + parameters: Object.assign({}, parameterMap['*'], parameterMap[stacks.firstStack.stackName]), // should this be stack? resourcesToImport, stream, }) : undefined; @@ -182,10 +199,11 @@ export class CdkToolkit { stream.write('Parameters and rules created during migration do not affect resource configuration.\n'); } + // pass a boolean to print if the stack is a migrate stack in order to set all resource diffs to import const stackCount = options.securityOnly ? (numberFromBool(printSecurityDiff(currentTemplate, stack, RequireApproval.Broadening, changeSet)) > 0 ? 1 : 0) - : (printStackDiff(currentTemplate, stack, strict, contextLines, quiet, changeSet, stream) > 0 ? 1 : 0); + : (printStackDiff(currentTemplate, stack, strict, contextLines, quiet, changeSet, stream, !!resourcesToImport) > 0 ? 1 : 0); diffs += stackCount + nestedStackCount; } diff --git a/packages/aws-cdk/lib/diff.ts b/packages/aws-cdk/lib/diff.ts index 399f08abeac6a..fa897430ad37b 100644 --- a/packages/aws-cdk/lib/diff.ts +++ b/packages/aws-cdk/lib/diff.ts @@ -23,9 +23,10 @@ export function printStackDiff( context: number, quiet: boolean, changeSet?: CloudFormation.DescribeChangeSetOutput, - stream?: cfnDiff.FormatStream): number { + stream?: cfnDiff.FormatStream, + isImport?: boolean): number { - let diff = cfnDiff.fullDiff(oldTemplate, newTemplate.template, changeSet); + let diff = cfnDiff.fullDiff(oldTemplate, newTemplate.template, changeSet, isImport); // detect and filter out mangled characters from the diff let filteredChangesCount = 0; diff --git a/packages/aws-cdk/test/diff.test.ts b/packages/aws-cdk/test/diff.test.ts index a92c6d53551c3..a7b5905e12f87 100644 --- a/packages/aws-cdk/test/diff.test.ts +++ b/packages/aws-cdk/test/diff.test.ts @@ -12,100 +12,6 @@ let cloudExecutable: MockCloudExecutable; let cloudFormation: jest.Mocked; let toolkit: CdkToolkit; -describe('imports', () => { - beforeEach(() => { - jest.spyOn(cfn, 'createDiffChangeSet').mockImplementation(async () => { - return { - Changes: [ - { - ResourceChange: { - Action: 'Import', - LogicalResourceId: 'Queue', - }, - }, - { - ResourceChange: { - Action: 'Import', - LogicalResourceId: 'Bucket', - }, - }, - { - ResourceChange: { - Action: 'Import', - LogicalResourceId: 'Queue2', - }, - }, - ], - }; - }); - cloudExecutable = new MockCloudExecutable({ - stacks: [{ - stackName: 'A', - template: { - Resources: { - Queue: { - Type: 'AWS::SQS::Queue', - }, - Queue2: { - Type: 'AWS::SQS::Queue', - }, - Bucket: { - Type: 'AWS::S3::Bucket', - }, - }, - }, - }], - }); - - cloudFormation = instanceMockFrom(Deployments); - - toolkit = new CdkToolkit({ - cloudExecutable, - deployments: cloudFormation, - configuration: cloudExecutable.configuration, - sdkProvider: cloudExecutable.sdkProvider, - }); - - // Default implementations - cloudFormation.readCurrentTemplateWithNestedStacks.mockImplementation((_stackArtifact: CloudFormationStackArtifact) => { - return Promise.resolve({ - deployedTemplate: {}, - nestedStackCount: 0, - }); - }); - cloudFormation.deployStack.mockImplementation((options) => Promise.resolve({ - noOp: true, - outputs: {}, - stackArn: '', - stackArtifact: options.stack, - })); - }); - - test('imports', async () => { - // GIVEN - const buffer = new StringWritable(); - - // WHEN - const exitCode = await toolkit.diff({ - stackNames: ['A'], - stream: buffer, - changeSet: true, - }); - - // THEN - const plainTextOutput = buffer.data.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, ''); - expect(plainTextOutput).toContain(`Stack A -Resources -[←] AWS::SQS::Queue Queue import -[←] AWS::SQS::Queue Queue2 import -[←] AWS::S3::Bucket Bucket import -`); - - expect(buffer.data.trim()).toContain('✨ Number of stacks with differences: 1'); - expect(exitCode).toBe(0); - }); -}); - describe('non-nested stacks', () => { beforeEach(() => { cloudExecutable = new MockCloudExecutable({