Skip to content

Commit

Permalink
fix(cli): diff with changeset fails if deploy role cannot be assumed (#…
Browse files Browse the repository at this point in the history
…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*
  • Loading branch information
scanlonp committed Apr 5, 2024
1 parent 615ee2d commit 21dba21
Show file tree
Hide file tree
Showing 3 changed files with 158 additions and 13 deletions.
8 changes: 7 additions & 1 deletion packages/aws-cdk/lib/api/deployments.ts
Expand Up @@ -265,6 +265,7 @@ export interface DestroyStackOptions {
export interface StackExistsOptions {
stack: cxapi.CloudFormationStackArtifact;
deployName?: string;
tryLookupRole?: boolean;
}

export interface DeploymentsProps {
Expand Down Expand Up @@ -430,7 +431,12 @@ export class Deployments {
}

public async stackExists(options: StackExistsOptions): Promise<boolean> {
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;
}
Expand Down
40 changes: 29 additions & 11 deletions packages/aws-cdk/lib/cdk-toolkit.ts
Expand Up @@ -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,
Expand All @@ -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.`);
}
}

Expand Down Expand Up @@ -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,
Expand All @@ -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.`);
}
}

Expand Down
123 changes: 122 additions & 1 deletion packages/aws-cdk/test/diff.test.ts
Expand Up @@ -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();

Expand All @@ -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', () => {
Expand Down

0 comments on commit 21dba21

Please sign in to comment.