Skip to content

Commit

Permalink
fix(CLI): cdk diff stack deletion causes a race condition (#29492)
Browse files Browse the repository at this point in the history 
*Co-authored by*: @scanlonp

### Issue # (if applicable)

Closes #29265.

### Reason for this change

Creating a changeset for a stack that has not been deployed yet causes CFN to create a stack in state `REVIEW_IN_PROGRESS`. Previously we deleted this empty stack, but did not wait for the stack status to be `DELETE_COMPLETE`. This allowed `cdk diff` to exit while the stack status was still `DELETE_IN_PROGRESS`, which can cause subsequent CDK commands to fail, because a stack deletion operation is still in progress. 

### Description of changes

No longer create the changeset if the stack doesn't exist. Only perform the existence check if the changeset parameter is specified, to avoid a permission error when looking up a stack. 

### 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
@comcalvi
comcalvi committed Mar 19, 2024
1 parent 7fe8ad3 commit 067539a
Show file tree
Hide file tree
Showing 5 changed files with 113 additions and 37 deletions.
9 changes: 9 additions & 0 deletions packages/@aws-cdk/cloudformation-diff/lib/diff-template.ts
View file
Expand Up @@ -46,6 +46,7 @@ export function fullDiff(
currentTemplate: { [key: string]: any },
newTemplate: { [key: string]: any },
changeSet?: CloudFormation.DescribeChangeSetOutput,
isImport?: boolean,
): types.TemplateDiff {

normalize(currentTemplate);
Expand All @@ -54,6 +55,8 @@ export function fullDiff(
if (changeSet) {
filterFalsePositives(theDiff, changeSet);
addImportInformation(theDiff, changeSet);
} else if (isImport) {
makeAllResourceChangesImports(theDiff);
}

return theDiff;
Expand Down Expand Up @@ -218,6 +221,12 @@ function addImportInformation(diff: types.TemplateDiff, changeSet: CloudFormatio
});
}

function makeAllResourceChangesImports(diff: types.TemplateDiff) {
diff.resources.forEachDifference((_logicalId: string, change: types.ResourceDifference) => {
change.isImport = true;
});
}

function filterFalsePositives(diff: types.TemplateDiff, changeSet: CloudFormation.DescribeChangeSetOutput) {
const replacements = findResourceReplacements(changeSet);
diff.resources.forEachDifference((logicalId: string, change: types.ResourceDifference) => {
Expand Down
21 changes: 7 additions & 14 deletions packages/aws-cdk/lib/api/util/cloudformation.ts
View file
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
67 changes: 46 additions & 21 deletions packages/aws-cdk/lib/cdk-toolkit.ts
View file
Expand Up @@ -139,20 +139,32 @@ 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}' has not been deployed to CloudFormation, skipping changeset creation.`);
}
}

const template = deserializeStructure(await fs.readFile(options.templatePath, { encoding: 'UTF-8' }));
diffs = options.securityOnly
? numberFromBool(printSecurityDiff(template, stacks.firstStack, RequireApproval.Broadening, changeSet))
: printStackDiff(template, stacks.firstStack.template, strict, contextLines, quiet, changeSet, stream);
: printStackDiff(template, stacks.firstStack.template, strict, contextLines, quiet, changeSet, false, stream);
} else {
// Compare N stacks against deployed templates
for (const stack of stacks.stackArtifacts) {
Expand All @@ -171,16 +183,29 @@ 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) {
// 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,
});
if (stackExists) {
changeSet = await createDiffChangeSet({
stack,
uuid: uuid.v4(),
deployments: this.props.deployments,
willExecute: false,
sdkProvider: this.props.sdkProvider,
parameters: Object.assign({}, parameterMap['*'], parameterMap[stack.stackName]),
resourcesToImport,
stream,
});
} else {
debug(`the stack '${stack.stackName}' has not been deployed to CloudFormation, skipping changeset creation.`);
}
}

if (resourcesToImport) {
stream.write('Parameters and rules created during migration do not affect resource configuration.\n');
Expand All @@ -189,7 +214,7 @@ export class CdkToolkit {
const stackCount =
options.securityOnly
? (numberFromBool(printSecurityDiff(currentTemplate, stack, RequireApproval.Broadening, changeSet)))
: (printStackDiff(currentTemplate, stack, strict, contextLines, quiet, changeSet, stream, nestedStacks));
: (printStackDiff(currentTemplate, stack, strict, contextLines, quiet, changeSet, !!resourcesToImport, stream, nestedStacks));

diffs += stackCount;
}
Expand Down
4 changes: 3 additions & 1 deletion packages/aws-cdk/lib/diff.ts
View file
Expand Up @@ -25,10 +25,11 @@ export function printStackDiff(
context: number,
quiet: boolean,
changeSet?: CloudFormation.DescribeChangeSetOutput,
isImport?: boolean,
stream: cfnDiff.FormatStream = process.stderr,
nestedStackTemplates?: { [nestedStackLogicalId: string]: NestedStackTemplates }): number {

let diff = cfnDiff.fullDiff(oldTemplate, newTemplate.template, changeSet);
let diff = cfnDiff.fullDiff(oldTemplate, newTemplate.template, changeSet, isImport);

// detect and filter out mangled characters from the diff
let filteredChangesCount = 0;
Expand Down Expand Up @@ -82,6 +83,7 @@ export function printStackDiff(
context,
quiet,
undefined,
isImport,
stream,
nestedStack.nestedStackTemplates,
);
Expand Down
49 changes: 48 additions & 1 deletion packages/aws-cdk/test/diff.test.ts
View file
Expand Up @@ -94,9 +94,10 @@ describe('imports', () => {
fs.rmSync('migrate.json');
});

test('imports', async () => {
test('imports render correctly for a nonexistant stack without creating a changeset', async () => {
// GIVEN
const buffer = new StringWritable();
cloudFormation.stackExists = jest.fn().mockReturnValue(Promise.resolve(false));

// WHEN
const exitCode = await toolkit.diff({
Expand All @@ -107,6 +108,34 @@ describe('imports', () => {

// THEN
const plainTextOutput = buffer.data.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '');
expect(cfn.createDiffChangeSet).not.toHaveBeenCalled();
expect(plainTextOutput).toContain(`Stack A
Parameters and rules created during migration do not affect resource configuration.
Resources
[←] AWS::SQS::Queue Queue import
[←] AWS::SQS::Queue Queue2 import
[←] AWS::S3::Bucket Bucket import
`);

expect(buffer.data.trim()).toContain('✨ Number of stacks with differences: 1');
expect(exitCode).toBe(0);
});

test('imports render correctly for an existing stack and diff creates a changeset', async () => {
// GIVEN
const buffer = new StringWritable();
cloudFormation.stackExists = jest.fn().mockReturnValue(Promise.resolve(true));

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

// THEN
const plainTextOutput = buffer.data.replace(/\x1B\[[0-?]*[ -/]*[@-~]/g, '');
expect(cfn.createDiffChangeSet).toHaveBeenCalled();
expect(plainTextOutput).toContain(`Stack A
Parameters and rules created during migration do not affect resource configuration.
Resources
Expand Down Expand Up @@ -306,6 +335,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 existence 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

0 comments on commit 067539a

Please sign in to comment.