From 21dba2194819ccb244fcdbc5007c055f3930b4e1 Mon Sep 17 00:00:00 2001 From: Parker Scanlon <69879391+scanlonp@users.noreply.github.com> Date: Fri, 5 Apr 2024 16:56:33 -0700 Subject: [PATCH] fix(cli): diff with changeset fails if deploy role cannot be assumed (#29718) Closes #29650 ### Description of changes This addresses the issue in two ways: 1. If the describeStacks call errors out, we now catch it and default to classic diff behavior. 2. The describeStacks call now tries to use the lookup role rather than the deploy role. ### Description of how you validated changes Manual testing with a user that could only assume lookup roles. ### 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* --- packages/aws-cdk/lib/api/deployments.ts | 8 +- packages/aws-cdk/lib/cdk-toolkit.ts | 40 +++++--- packages/aws-cdk/test/diff.test.ts | 123 +++++++++++++++++++++++- 3 files changed, 158 insertions(+), 13 deletions(-) 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; } diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index e2614a3710aad..e1c4571182999 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,20 @@ 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 { + 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 +218,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.`); } } 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', () => {