Skip to content

Commit 8071015

Browse files
chore(migrate): only allow migrate on healthy stacks (#28452)
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*
1 parent a4a4c6f commit 8071015

File tree

4 files changed

+49
-14
lines changed

4 files changed

+49
-14
lines changed

packages/aws-cdk/lib/api/util/cloudformation/stack-status.ts

+5
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,11 @@ export class StackStatus {
4141
return !this.isNotFound && (this.name === 'CREATE_COMPLETE' || this.name === 'UPDATE_COMPLETE' || this.name === 'IMPORT_COMPLETE');
4242
}
4343

44+
get isRollbackSuccess(): boolean {
45+
return this.name === 'ROLLBACK_COMPLETE'
46+
|| this.name === 'UPDATE_ROLLBACK_COMPLETE';
47+
}
48+
4449
public toString(): string {
4550
return this.name + (this.reason ? ` (${this.reason})` : '');
4651
}

packages/aws-cdk/lib/commands/migrate.ts

+8-3
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { Environment, UNKNOWN_ACCOUNT, UNKNOWN_REGION } from '@aws-cdk/cx-api';
66
import * as cdk_from_cfn from 'cdk-from-cfn';
77
import { cliInit } from '../../lib/init';
88
import { Mode, SdkProvider } from '../api';
9+
import { CloudFormationStack } from '../api/util/cloudformation';
910
import { zipDirectory } from '../util/archive';
1011

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

115-
return (await cloudFormation.getTemplate({
116-
StackName: stackName,
117-
}).promise()).TemplateBody;
116+
const stack = await CloudFormationStack.lookup(cloudFormation, stackName);
117+
if (stack.stackStatus.isDeploySuccess || stack.stackStatus.isRollbackSuccess) {
118+
return JSON.stringify(await stack.template());
119+
} else {
120+
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.`);
121+
}
122+
return;
118123
}
119124

120125
/**

packages/aws-cdk/test/cdk-toolkit.test.ts

+4-4
Original file line numberDiff line numberDiff line change
@@ -869,7 +869,7 @@ describe('synth', () => {
869869

870870
test('migrate fails when neither --from-path or --from-stack are provided', async () => {
871871
const toolkit = defaultToolkitSetup();
872-
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.');
872+
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.');
873873
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.');
874874
});
875875

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

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

895895
test('migrate fails when --from-stack is used and stack does not exist in account', async () => {
896896
const mockSdkProvider = new MockSdkProvider();
897897
mockSdkProvider.stubCloudFormation({
898-
getTemplate(_request) {
898+
describeStacks(_request) {
899899
throw new Error('Stack does not exist in this environment');
900900
},
901901
});

packages/aws-cdk/test/commands/migrate.test.ts

+32-7
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ const exec = promisify(_exec);
1111
describe('Migrate Function Tests', () => {
1212
let sdkProvider: MockSdkProvider;
1313
let getTemplateMock: jest.Mock;
14+
let describeStacksMock: jest.Mock;
1415
let cfnMocks: MockedObject<SyncHandlerSubsetOf<AWS.CloudFormation>>;
1516

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

23-
beforeEach(() => {
24+
beforeEach(async () => {
2425
sdkProvider = new MockSdkProvider();
2526
getTemplateMock = jest.fn();
26-
cfnMocks = { getTemplate: getTemplateMock };
27+
describeStacksMock = jest.fn();
28+
cfnMocks = { getTemplate: getTemplateMock, describeStacks: describeStacksMock };
2729
sdkProvider.stubCloudFormation(cfnMocks as any);
2830
});
2931

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

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

65-
expect(await readFromStack('this-one', sdkProvider, { account: 'num', region: 'here', name: 'hello-my-name-is-what...' })).toEqual(template);
67+
describeStacksMock.mockImplementation(() => ({
68+
Stacks: [
69+
{
70+
StackName: 'this-one',
71+
StackStatus: 'CREATE_COMPLETE',
72+
},
73+
],
74+
}));
75+
76+
expect(await readFromStack('this-one', sdkProvider, { account: 'num', region: 'here', name: 'hello-my-name-is-what...' })).toEqual(JSON.stringify(JSON.parse(template)));
6677
});
6778

6879
test('readFromStack throws error when no stack exists with the stack name in the account and region', async () => {
69-
getTemplateMock.mockImplementationOnce(() => { throw new Error('No stack. This did not go well.'); });
70-
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.');
80+
describeStacksMock.mockImplementation(() => { throw new Error('No stack. This did not go well.'); });
81+
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.');
82+
});
83+
84+
test('readFromStack throws error when stack exists but the status is not healthy', async () => {
85+
describeStacksMock.mockImplementation(() => ({
86+
Stacks: [
87+
{
88+
StackName: 'this-one',
89+
StackStatus: 'CREATE_FAILED',
90+
StackStatusReason: 'Something went wrong',
91+
},
92+
],
93+
}));
94+
95+
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.');
7196
});
7297

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

0 commit comments

Comments
 (0)