From d1190f89fb1283c40d3cc5b3d6f527609131ee36 Mon Sep 17 00:00:00 2001 From: Parker Scanlon <69879391+scanlonp@users.noreply.github.com> Date: Wed, 3 Apr 2024 20:06:03 -0700 Subject: [PATCH 1/5] catch errors with descibeStacks call and revert to classic diff --- packages/aws-cdk/lib/cdk-toolkit.ts | 42 +++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index e2614a3710aad..83b16d6bd3bf5 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -139,10 +139,19 @@ export class CdkToolkit { let changeSet = undefined; if (options.changeSet) { - const stackExists = await this.props.deployments.stackExists({ - stack: stacks.firstStack, - deployName: stacks.firstStack.stackName, - }); + let stackExists = false; + try { + stackExists = await this.props.deployments.stackExists({ + stack: stacks.firstStack, + deployName: stacks.firstStack.stackName, + tryLookupRole: true, + }); + } catch (e: any) { + debug(e.message); + stream.write('Checking if the stack exists before creating the changeset has failed, will base the diff on template differences (run again with -v to see the reason)\n'); + stackExists = false; + } + if (stackExists) { changeSet = await createDiffChangeSet({ stack: stacks.firstStack, @@ -154,7 +163,7 @@ export class CdkToolkit { stream, }); } else { - debug(`the stack '${stacks.firstStack.stackName}' has not been deployed to CloudFormation, skipping changeset creation.`); + debug(`the stack '${stacks.firstStack.stackName}' has not been deployed to CloudFormation or describeStacks call failed, skipping changeset creation.`); } } @@ -183,11 +192,22 @@ export class CdkToolkit { let changeSet = undefined; if (options.changeSet) { - // only perform this check if we're going to make a changeset. This check requires permissions that --no-changeset users might not have. - const stackExists = await this.props.deployments.stackExists({ - stack: stack, - deployName: stack.stackName, - }); + + let stackExists = false; + try { + // only perform this check if we're going to make a changeset. This check requires permissions that --no-changeset users might not have. + // the call should now use the lookup role, but keep behind the flag since we only need it if the changeset is being made + stackExists = await this.props.deployments.stackExists({ + stack, + deployName: stack.stackName, + tryLookupRole: true, + }); + } catch (e: any) { + debug(e.message); + stream.write('Checking if the stack exists before creating the changeset has failed, will base the diff on template differences (run again with -v to see the reason)\n'); + stackExists = false; + } + if (stackExists) { changeSet = await createDiffChangeSet({ stack, @@ -200,7 +220,7 @@ export class CdkToolkit { stream, }); } else { - debug(`the stack '${stack.stackName}' has not been deployed to CloudFormation, skipping changeset creation.`); + debug(`the stack '${stack.stackName}' has not been deployed to CloudFormation or describeStacks call failed, skipping changeset creation.`); } } From 037af3f8c3b8ec9d29e1037da7b8b7e1e1993aaa Mon Sep 17 00:00:00 2001 From: Parker Scanlon <69879391+scanlonp@users.noreply.github.com> Date: Wed, 3 Apr 2024 20:07:27 -0700 Subject: [PATCH 2/5] add option to make stackExists function use lookup role --- packages/aws-cdk/lib/api/deployments.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/aws-cdk/lib/api/deployments.ts b/packages/aws-cdk/lib/api/deployments.ts index b7bea90c56c28..f407ffd774776 100644 --- a/packages/aws-cdk/lib/api/deployments.ts +++ b/packages/aws-cdk/lib/api/deployments.ts @@ -265,6 +265,7 @@ export interface DestroyStackOptions { export interface StackExistsOptions { stack: cxapi.CloudFormationStackArtifact; deployName?: string; + tryLookupRole?: boolean; } export interface DeploymentsProps { @@ -430,7 +431,12 @@ export class Deployments { } public async stackExists(options: StackExistsOptions): Promise { - const { stackSdk } = await this.prepareSdkFor(options.stack, undefined, Mode.ForReading); + let stackSdk; + if (options.tryLookupRole) { + stackSdk = (await this.prepareSdkWithLookupOrDeployRole(options.stack)).stackSdk; + } else { + stackSdk = (await this.prepareSdkFor(options.stack, undefined, Mode.ForReading)).stackSdk; + } const stack = await CloudFormationStack.lookup(stackSdk.cloudFormation(), options.deployName ?? options.stack.stackName); return stack.exists; } From 31c3c58d12075802269428f08d687fa0ce23ec07 Mon Sep 17 00:00:00 2001 From: Parker Scanlon <69879391+scanlonp@users.noreply.github.com> Date: Thu, 4 Apr 2024 15:56:15 -0700 Subject: [PATCH 3/5] unit tests --- packages/aws-cdk/test/diff.test.ts | 123 ++++++++++++++++++++++++++++- 1 file changed, 122 insertions(+), 1 deletion(-) diff --git a/packages/aws-cdk/test/diff.test.ts b/packages/aws-cdk/test/diff.test.ts index e120103390087..0678513c9736c 100644 --- a/packages/aws-cdk/test/diff.test.ts +++ b/packages/aws-cdk/test/diff.test.ts @@ -335,8 +335,86 @@ describe('non-nested stacks', () => { expect(buffer.data.trim()).not.toContain('There were no differences'); expect(exitCode).toBe(0); }); +}); + +describe('stack exists checks', () => { + beforeEach(() => { + + jest.resetAllMocks(); + + cloudExecutable = new MockCloudExecutable({ + stacks: [{ + stackName: 'A', + template: { resource: 'A' }, + }, + { + stackName: 'B', + depends: ['A'], + template: { resource: 'B' }, + }, + { + stackName: 'C', + depends: ['A'], + template: { resource: 'C' }, + metadata: { + '/resource': [ + { + type: cxschema.ArtifactMetadataEntryType.ERROR, + data: 'this is an error', + }, + ], + }, + }, + { + stackName: 'D', + template: { resource: 'D' }, + }], + }); + + cloudFormation = instanceMockFrom(Deployments); + + toolkit = new CdkToolkit({ + cloudExecutable, + deployments: cloudFormation, + configuration: cloudExecutable.configuration, + sdkProvider: cloudExecutable.sdkProvider, + }); - test('diff does not check for stack existence when --no-changeset is passed', async () => { + // Default implementations + cloudFormation.readCurrentTemplateWithNestedStacks.mockImplementation((stackArtifact: CloudFormationStackArtifact) => { + if (stackArtifact.stackName === 'D') { + return Promise.resolve({ + deployedRootTemplate: { resource: 'D' }, + nestedStacks: {}, + }); + } + return Promise.resolve({ + deployedRootTemplate: {}, + nestedStacks: {}, + }); + }); + cloudFormation.deployStack.mockImplementation((options) => Promise.resolve({ + noOp: true, + outputs: {}, + stackArn: '', + stackArtifact: options.stack, + })); + + jest.spyOn(cfn, 'createDiffChangeSet').mockImplementationOnce(async () => { + return { + Changes: [ + { + ResourceChange: { + Action: 'Dummy', + LogicalResourceId: 'Object', + }, + }, + ], + }; + }); + }); + + test('diff does not check for stack existence when --no-change-set is passed', async () => { // GIVEN const buffer = new StringWritable(); @@ -353,6 +431,49 @@ describe('non-nested stacks', () => { 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(); + cloudFormation.stackExists = jest.fn().mockReturnValue(Promise.resolve(false)); + + // WHEN + const exitCode = await toolkit.diff({ + stackNames: ['A', 'A'], + stream: buffer, + fail: false, + quiet: true, + changeSet: true, + }); + + // THEN + expect(exitCode).toBe(0); + expect(cloudFormation.stackExists).toHaveBeenCalled(); + expect(cfn.createDiffChangeSet).not.toHaveBeenCalled(); + }); + + test('diff falls back to classic diff when stackExists call fails', async () => { + // GIVEN + const buffer = new StringWritable(); + + cloudFormation.stackExists.mockImplementation(() => { + throw new Error('Fail fail fail'); + }); + + // WHEN + const exitCode = await toolkit.diff({ + stackNames: ['A', 'A'], + stream: buffer, + fail: false, + quiet: true, + changeSet: true, + }); + + // THEN + expect(exitCode).toBe(0); + expect(cloudFormation.stackExists).toHaveBeenCalled(); + expect(cfn.createDiffChangeSet).not.toHaveBeenCalled(); + }); }); describe('nested stacks', () => { From c2da3944bfeb3163546386d0bd246a9c23b0b9e5 Mon Sep 17 00:00:00 2001 From: Parker Scanlon <69879391+scanlonp@users.noreply.github.com> Date: Fri, 5 Apr 2024 13:48:19 -0700 Subject: [PATCH 4/5] Update packages/aws-cdk/lib/cdk-toolkit.ts --- packages/aws-cdk/lib/cdk-toolkit.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 83b16d6bd3bf5..1049443016dbe 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -196,7 +196,6 @@ export class CdkToolkit { let stackExists = false; try { // only perform this check if we're going to make a changeset. This check requires permissions that --no-changeset users might not have. - // the call should now use the lookup role, but keep behind the flag since we only need it if the changeset is being made stackExists = await this.props.deployments.stackExists({ stack, deployName: stack.stackName, From c0ef95183292063bdd1b3d1fa723d396d4c6d87a Mon Sep 17 00:00:00 2001 From: Parker Scanlon <69879391+scanlonp@users.noreply.github.com> Date: Fri, 5 Apr 2024 13:49:07 -0700 Subject: [PATCH 5/5] Update packages/aws-cdk/lib/cdk-toolkit.ts --- packages/aws-cdk/lib/cdk-toolkit.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 1049443016dbe..e1c4571182999 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -195,7 +195,6 @@ export class CdkToolkit { let stackExists = false; try { - // only perform this check if we're going to make a changeset. This check requires permissions that --no-changeset users might not have. stackExists = await this.props.deployments.stackExists({ stack, deployName: stack.stackName,