Skip to content

Commit

Permalink
chore(migrate): only allow migrate on healthy stacks (#28452)
Browse files Browse the repository at this point in the history
If the stack is not in a healthy state, we should not allow cdk migrate to be run on it. 

Closes #<issue number here>.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
TheRealAmazonKendra committed Dec 23, 2023
1 parent a4a4c6f commit 8071015
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 14 deletions.
5 changes: 5 additions & 0 deletions packages/aws-cdk/lib/api/util/cloudformation/stack-status.ts
Expand Up @@ -41,6 +41,11 @@ export class StackStatus {
return !this.isNotFound && (this.name === 'CREATE_COMPLETE' || this.name === 'UPDATE_COMPLETE' || this.name === 'IMPORT_COMPLETE');
}

get isRollbackSuccess(): boolean {
return this.name === 'ROLLBACK_COMPLETE'
|| this.name === 'UPDATE_ROLLBACK_COMPLETE';
}

public toString(): string {
return this.name + (this.reason ? ` (${this.reason})` : '');
}
Expand Down
11 changes: 8 additions & 3 deletions packages/aws-cdk/lib/commands/migrate.ts
Expand Up @@ -6,6 +6,7 @@ import { Environment, UNKNOWN_ACCOUNT, UNKNOWN_REGION } from '@aws-cdk/cx-api';
import * as cdk_from_cfn from 'cdk-from-cfn';
import { cliInit } from '../../lib/init';
import { Mode, SdkProvider } from '../api';
import { CloudFormationStack } from '../api/util/cloudformation';
import { zipDirectory } from '../util/archive';

const camelCase = require('camelcase');
Expand Down Expand Up @@ -112,9 +113,13 @@ export function readFromPath(inputPath?: string): string | undefined {
export async function readFromStack(stackName: string, sdkProvider: SdkProvider, environment: Environment): Promise<string | undefined> {
const cloudFormation = (await sdkProvider.forEnvironment(environment, Mode.ForReading)).sdk.cloudFormation();

return (await cloudFormation.getTemplate({
StackName: stackName,
}).promise()).TemplateBody;
const stack = await CloudFormationStack.lookup(cloudFormation, stackName);
if (stack.stackStatus.isDeploySuccess || stack.stackStatus.isRollbackSuccess) {
return JSON.stringify(await stack.template());
} else {
throw new Error(`Stack '${stackName}' in account ${environment.account} and region ${environment.region} has a status of '${stack.stackStatus.name}' due to '${stack.stackStatus.reason}'. The stack cannot be migrated until it is in a healthy state.`);
}
return;
}

/**
Expand Down
8 changes: 4 additions & 4 deletions packages/aws-cdk/test/cdk-toolkit.test.ts
Expand Up @@ -869,7 +869,7 @@ describe('synth', () => {

test('migrate fails when neither --from-path or --from-stack are provided', async () => {
const toolkit = defaultToolkitSetup();
await expect(() => toolkit.migrate({ stackName: 'no-source' })).rejects.toThrowError('Either `--from-path` or `--from-stack` must be used to provide the source of the CloudFormation template.');
await expect(() => toolkit.migrate({ stackName: 'no-source' })).rejects.toThrow('Either `--from-path` or `--from-stack` must be used to provide the source of the CloudFormation template.');
expect(stderrMock.mock.calls[1][0]).toContain(' ❌ Migrate failed for `no-source`: Either `--from-path` or `--from-stack` must be used to provide the source of the CloudFormation template.');
});

Expand All @@ -879,7 +879,7 @@ describe('synth', () => {
stackName: 'no-source',
fromPath: './here/template.yml',
fromStack: true,
})).rejects.toThrowError('Only one of `--from-path` or `--from-stack` may be provided.');
})).rejects.toThrow('Only one of `--from-path` or `--from-stack` may be provided.');
expect(stderrMock.mock.calls[1][0]).toContain(' ❌ Migrate failed for `no-source`: Only one of `--from-path` or `--from-stack` may be provided.');
});

Expand All @@ -888,14 +888,14 @@ describe('synth', () => {
await expect(() => toolkit.migrate({
stackName: 'bad-local-source',
fromPath: './here/template.yml',
})).rejects.toThrowError('\'./here/template.yml\' is not a valid path.');
})).rejects.toThrow('\'./here/template.yml\' is not a valid path.');
expect(stderrMock.mock.calls[1][0]).toContain(' ❌ Migrate failed for `bad-local-source`: \'./here/template.yml\' is not a valid path.');
});

test('migrate fails when --from-stack is used and stack does not exist in account', async () => {
const mockSdkProvider = new MockSdkProvider();
mockSdkProvider.stubCloudFormation({
getTemplate(_request) {
describeStacks(_request) {
throw new Error('Stack does not exist in this environment');
},
});
Expand Down
39 changes: 32 additions & 7 deletions packages/aws-cdk/test/commands/migrate.test.ts
Expand Up @@ -11,6 +11,7 @@ const exec = promisify(_exec);
describe('Migrate Function Tests', () => {
let sdkProvider: MockSdkProvider;
let getTemplateMock: jest.Mock;
let describeStacksMock: jest.Mock;
let cfnMocks: MockedObject<SyncHandlerSubsetOf<AWS.CloudFormation>>;

const testResourcePath = [__dirname, 'test-resources'];
Expand All @@ -20,10 +21,11 @@ describe('Migrate Function Tests', () => {
const validTemplatePath = path.join(...templatePath, 's3-template.json');
const validTemplate = readFromPath(validTemplatePath)!;

beforeEach(() => {
beforeEach(async () => {
sdkProvider = new MockSdkProvider();
getTemplateMock = jest.fn();
cfnMocks = { getTemplate: getTemplateMock };
describeStacksMock = jest.fn();
cfnMocks = { getTemplate: getTemplateMock, describeStacks: describeStacksMock };
sdkProvider.stubCloudFormation(cfnMocks as any);
});

Expand Down Expand Up @@ -57,17 +59,40 @@ describe('Migrate Function Tests', () => {
});

test('readFromStack produces a string representation of the template retrieved from CloudFormation', async () => {
const template = fs.readFileSync(validTemplatePath);
getTemplateMock.mockImplementationOnce(() => ({
const template = fs.readFileSync(validTemplatePath, { encoding: 'utf-8' });
getTemplateMock.mockImplementation(() => ({
TemplateBody: template,
}));

expect(await readFromStack('this-one', sdkProvider, { account: 'num', region: 'here', name: 'hello-my-name-is-what...' })).toEqual(template);
describeStacksMock.mockImplementation(() => ({
Stacks: [
{
StackName: 'this-one',
StackStatus: 'CREATE_COMPLETE',
},
],
}));

expect(await readFromStack('this-one', sdkProvider, { account: 'num', region: 'here', name: 'hello-my-name-is-what...' })).toEqual(JSON.stringify(JSON.parse(template)));
});

test('readFromStack throws error when no stack exists with the stack name in the account and region', async () => {
getTemplateMock.mockImplementationOnce(() => { throw new Error('No stack. This did not go well.'); });
await expect(() => readFromStack('that-one', sdkProvider, { account: 'num', region: 'here', name: 'hello-my-name-is-who...' })).rejects.toThrowError('No stack. This did not go well.');
describeStacksMock.mockImplementation(() => { throw new Error('No stack. This did not go well.'); });
await expect(() => readFromStack('that-one', sdkProvider, { account: 'num', region: 'here', name: 'hello-my-name-is-who...' })).rejects.toThrow('No stack. This did not go well.');
});

test('readFromStack throws error when stack exists but the status is not healthy', async () => {
describeStacksMock.mockImplementation(() => ({
Stacks: [
{
StackName: 'this-one',
StackStatus: 'CREATE_FAILED',
StackStatusReason: 'Something went wrong',
},
],
}));

await expect(() => readFromStack('that-one', sdkProvider, { account: 'num', region: 'here', name: 'hello-my-name-is-chicka-chicka...' })).rejects.toThrow('Stack \'that-one\' in account num and region here has a status of \'CREATE_FAILED\' due to \'Something went wrong\'. The stack cannot be migrated until it is in a healthy state.');
});

test('setEnvironment sets account and region when provided', () => {
Expand Down

0 comments on commit 8071015

Please sign in to comment.