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): cdk diff stack deletion causes a race condition #29492

Merged
merged 16 commits into from
Mar 19, 2024
21 changes: 7 additions & 14 deletions packages/aws-cdk/lib/api/util/cloudformation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,7 @@ async function uploadBodyParameterAndCreateChangeSet(options: PrepareChangeSetOp
}

async function createChangeSet(options: CreateChangeSetOptions): Promise<CloudFormation.DescribeChangeSetOutput> {
await cleanupOldChangeset(options.exists, options.changeSetName, options.stack.stackName, options.cfn);
await cleanupOldChangeset(options.changeSetName, options.stack.stackName, options.cfn);

debug(`Attempting to create ChangeSet with name ${options.changeSetName} for stack ${options.stack.stackName}`);

Expand All @@ -388,23 +388,16 @@ async function createChangeSet(options: CreateChangeSetOptions): Promise<CloudFo
debug('Initiated creation of changeset: %s; waiting for it to finish creating...', changeSet.Id);
// Fetching all pages if we'll execute, so we can have the correct change count when monitoring.
const createdChangeSet = await waitForChangeSet(options.cfn, options.stack.stackName, options.changeSetName, { fetchAll: options.willExecute });
await cleanupOldChangeset(options.exists, options.changeSetName, options.stack.stackName, options.cfn);
await cleanupOldChangeset(options.changeSetName, options.stack.stackName, options.cfn);

return createdChangeSet;
}

export async function cleanupOldChangeset(stackExists: boolean, changeSetName: string, stackName: string, cfn: CloudFormation) {
if (stackExists) {
// Delete any existing change sets generated by CDK since change set names must be unique.
// The delete request is successful as long as the stack exists (even if the change set does not exist).
debug(`Removing existing change set with name ${changeSetName} if it exists`);
await cfn.deleteChangeSet({ StackName: stackName, ChangeSetName: changeSetName }).promise();
} else {
// delete the stack since creating a changeset for a stack that doesn't exist leaves that stack in a REVIEW_IN_PROGRESS state
// that prevents other changesets from being created, even after the changeset has been deleted.
debug(`Removing stack with name ${stackName}`);
await cfn.deleteStack({ StackName: stackName }).promise();
}
export async function cleanupOldChangeset(changeSetName: string, stackName: string, cfn: CloudFormation) {
// Delete any existing change sets generated by CDK since change set names must be unique.
// The delete request is successful as long as the stack exists (even if the change set does not exist).
debug(`Removing existing change set with name ${changeSetName} if it exists`);
await cfn.deleteChangeSet({ StackName: stackName, ChangeSetName: changeSetName }).promise();
}

/**
Expand Down
62 changes: 43 additions & 19 deletions packages/aws-cdk/lib/cdk-toolkit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,15 +136,27 @@ export class CdkToolkit {
throw new Error(`There is no file at ${options.templatePath}`);
}

const changeSet = options.changeSet ? await createDiffChangeSet({
stack: stacks.firstStack,
uuid: uuid.v4(),
willExecute: false,
deployments: this.props.deployments,
sdkProvider: this.props.sdkProvider,
parameters: Object.assign({}, parameterMap['*'], parameterMap[stacks.firstStack.stackName]),
stream,
}) : undefined;
let changeSet = undefined;

if (options.changeSet) {
const stackExists = await this.props.deployments.stackExists({
stack: stacks.firstStack,
deployName: stacks.firstStack.stackName,
});
if (stackExists) {
changeSet = await createDiffChangeSet({
stack: stacks.firstStack,
uuid: uuid.v4(),
deployments: this.props.deployments,
willExecute: false,
sdkProvider: this.props.sdkProvider,
parameters: Object.assign({}, parameterMap['*'], parameterMap[stacks.firstStack.stackName]),
stream,
});
} else {
debug(`the stack '${stacks.firstStack.stackName}' does not exist, skipping changeset creation.`);
scanlonp marked this conversation as resolved.
Show resolved Hide resolved
}
}

const template = deserializeStructure(await fs.readFile(options.templatePath, { encoding: 'UTF-8' }));
diffs = options.securityOnly
Expand All @@ -168,16 +180,28 @@ export class CdkToolkit {
removeNonImportResources(stack);
}

const changeSet = 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]),
resourcesToImport,
stream,
}) : undefined;
let changeSet = undefined;

if (options.changeSet) {
const stackExists = await this.props.deployments.stackExists({
stack: stack,
deployName: stack.stackName,
});
if (stackExists) {
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]),
resourcesToImport,
stream,
});
} else {
debug(`the stack '${stack.stackName}' does not exist, skipping changeset creation.`);
comcalvi marked this conversation as resolved.
Show resolved Hide resolved
}
}

if (resourcesToImport) {
stream.write('Parameters and rules created during migration do not affect resource configuration.\n');
Expand Down
19 changes: 19 additions & 0 deletions packages/aws-cdk/test/diff.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ describe('imports', () => {
});

cloudFormation = instanceMockFrom(Deployments);
cloudFormation.stackExists = jest.fn().mockReturnValue(Promise.resolve(true));

toolkit = new CdkToolkit({
cloudExecutable,
Expand Down Expand Up @@ -306,6 +307,24 @@ describe('non-nested stacks', () => {
expect(buffer.data.trim()).not.toContain('There were no differences');
expect(exitCode).toBe(0);
});

test('diff does not check for stack existance when --no-changeset is passed', async () => {
// GIVEN
const buffer = new StringWritable();

// WHEN
const exitCode = await toolkit.diff({
stackNames: ['A', 'A'],
stream: buffer,
fail: false,
quiet: true,
changeSet: false,
});

// THEN
expect(exitCode).toBe(0);
expect(cloudFormation.stackExists).not.toHaveBeenCalled();
});
});

describe('nested stacks', () => {
Expand Down