Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cli): diff with changeset fails if deploy role cannot be assumed #29718

Merged
merged 9 commits into from
Apr 5, 2024
8 changes: 7 additions & 1 deletion packages/aws-cdk/lib/api/deployments.ts
Original file line number Diff line number Diff line change
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This option is confusing...if it's on, then try to assume the lookup role or the deploy role, but if it's off, we assume the lookup role only?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can follow this one up in another PR

stackSdk = (await this.prepareSdkWithLookupOrDeployRole(options.stack)).stackSdk;
} else {
stackSdk = (await this.prepareSdkFor(options.stack, undefined, Mode.ForReading)).stackSdk;
}
Comment on lines +435 to +439
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prepareSdkWithLookupOrDeployRole defaults to ForReading. If we can't assume the lookup role, it will fall back to the deploy role.

Don't use prepareSdkFor directly, use the wrappers like prepareSdkWithLookupRole or whichever one you want to use; those methods have names that make it clear which role is being assumed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should still try to fallback to the deploy role, in case somebody doesn't have permissions to the lookup role. Why don't we catch any errors we get when trying to assume the deploy role? If we can't assume it then we stop changeset creation.

Inside of prepareSdkFor, cachedSdkForEnvironment returns an object with didAssumeRole on it. didAssumeRole is not returned to the caller, but you could add it as a property to the returned object. Then you have a way of checking if the fallback was successful.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed off(on?)line, we can investigate this in a follow up next week

const stack = await CloudFormationStack.lookup(stackSdk.cloudFormation(), options.deployName ?? options.stack.stackName);
return stack.exists;
}
Expand Down
42 changes: 31 additions & 11 deletions packages/aws-cdk/lib/cdk-toolkit.ts
Original file line number Diff line number Diff line change
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,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.
scanlonp marked this conversation as resolved.
Show resolved Hide resolved
// the call should now use the lookup role, but keep behind the flag since we only need it if the changeset is being made
comcalvi marked this conversation as resolved.
Show resolved Hide resolved
scanlonp marked this conversation as resolved.
Show resolved Hide resolved
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 +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.`);
}
}

Expand Down
123 changes: 122 additions & 1 deletion packages/aws-cdk/test/diff.test.ts
Original file line number Diff line number Diff line change
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