From 559d676e2989739b38491b1f767face839d39f69 Mon Sep 17 00:00:00 2001 From: Rico Hermans Date: Thu, 28 Nov 2024 18:27:41 +0100 Subject: [PATCH] fix(cli): warns about missing `--no-rollback` flag that is present (#32309) The logic on `rollback` and `!rollback` was inverted in a couple of places, causing the check to be the wrong way around and making reasoning about these options harder. Revert the logic and do a more comprehensive test suite around these options. Also remove a stray `debug()` statement that was left in from a previous PR, and show the stack status in the error message. Closes #32295. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- packages/aws-cdk/lib/api/aws-auth/sdk.ts | 1 - packages/aws-cdk/lib/api/deploy-stack.ts | 17 +++++---- packages/aws-cdk/lib/cdk-toolkit.ts | 6 +-- .../aws-cdk/test/api/deploy-stack.test.ts | 37 +++++++++++++------ packages/aws-cdk/test/cdk-toolkit.test.ts | 10 ++--- 5 files changed, 42 insertions(+), 29 deletions(-) diff --git a/packages/aws-cdk/lib/api/aws-auth/sdk.ts b/packages/aws-cdk/lib/api/aws-auth/sdk.ts index 945b5f4513ad2..4924a6a1e5fef 100644 --- a/packages/aws-cdk/lib/api/aws-auth/sdk.ts +++ b/packages/aws-cdk/lib/api/aws-auth/sdk.ts @@ -951,7 +951,6 @@ export class SDK { }); const command = new GetCallerIdentityCommand({}); const result = await client.send(command); - debug(result.Account!, result.Arn, result.UserId); const accountId = result.Account; const partition = result.Arn!.split(':')[1]; if (!accountId) { diff --git a/packages/aws-cdk/lib/api/deploy-stack.ts b/packages/aws-cdk/lib/api/deploy-stack.ts index 5cc6819e0def6..38654116cd646 100644 --- a/packages/aws-cdk/lib/api/deploy-stack.ts +++ b/packages/aws-cdk/lib/api/deploy-stack.ts @@ -37,7 +37,7 @@ import { StringWithoutPlaceholders } from './util/placeholders'; export type DeployStackResult = | SuccessfulDeployStackResult | NeedRollbackFirstDeployStackResult - | ReplacementRequiresNoRollbackStackResult + | ReplacementRequiresRollbackStackResult ; /** Successfully deployed a stack */ @@ -52,11 +52,12 @@ export interface SuccessfulDeployStackResult { export interface NeedRollbackFirstDeployStackResult { readonly type: 'failpaused-need-rollback-first'; readonly reason: 'not-norollback' | 'replacement'; + readonly status: string; } -/** The upcoming change has a replacement, which requires deploying without --no-rollback */ -export interface ReplacementRequiresNoRollbackStackResult { - readonly type: 'replacement-requires-norollback'; +/** The upcoming change has a replacement, which requires deploying with --rollback */ +export interface ReplacementRequiresRollbackStackResult { + readonly type: 'replacement-requires-rollback'; } export function assertIsSuccessfulDeployStackResult(x: DeployStackResult): asserts x is SuccessfulDeployStackResult { @@ -512,13 +513,13 @@ class FullCloudFormationDeployment { const isPausedFailState = this.cloudFormationStack.stackStatus.isRollbackable; const rollback = this.options.rollback ?? true; if (isPausedFailState && replacement) { - return { type: 'failpaused-need-rollback-first', reason: 'replacement' }; + return { type: 'failpaused-need-rollback-first', reason: 'replacement', status: this.cloudFormationStack.stackStatus.name }; } - if (isPausedFailState && !rollback) { - return { type: 'failpaused-need-rollback-first', reason: 'not-norollback' }; + if (isPausedFailState && rollback) { + return { type: 'failpaused-need-rollback-first', reason: 'not-norollback', status: this.cloudFormationStack.stackStatus.name }; } if (!rollback && replacement) { - return { type: 'replacement-requires-norollback' }; + return { type: 'replacement-requires-rollback' }; } return this.executeChangeSet(changeSetDescription); diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index dc8b6cf34aca1..15fab3fe5dfe3 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -436,8 +436,8 @@ export class CdkToolkit { case 'failpaused-need-rollback-first': { const motivation = r.reason === 'replacement' - ? 'Stack is in a paused fail state and change includes a replacement which cannot be deployed with "--no-rollback"' - : 'Stack is in a paused fail state and command line arguments do not include "--no-rollback"'; + ? `Stack is in a paused fail state (${r.status}) and change includes a replacement which cannot be deployed with "--no-rollback"` + : `Stack is in a paused fail state (${r.status}) and command line arguments do not include "--no-rollback"`; if (options.force) { warning(`${motivation}. Rolling back first (--force).`); @@ -461,7 +461,7 @@ export class CdkToolkit { break; } - case 'replacement-requires-norollback': { + case 'replacement-requires-rollback': { const motivation = 'Change includes a replacement which cannot be deployed with "--no-rollback"'; if (options.force) { diff --git a/packages/aws-cdk/test/api/deploy-stack.test.ts b/packages/aws-cdk/test/api/deploy-stack.test.ts index c96a2170c6afe..024b6689c5217 100644 --- a/packages/aws-cdk/test/api/deploy-stack.test.ts +++ b/packages/aws-cdk/test/api/deploy-stack.test.ts @@ -1127,22 +1127,33 @@ describe('disable rollback', () => { }); test.each([ - [StackStatus.UPDATE_FAILED, 'failpaused-need-rollback-first'], - [StackStatus.CREATE_COMPLETE, 'replacement-requires-norollback'], -])('no-rollback and replacement is disadvised: %p -> %p', async (stackStatus, expectedType) => { + // From a failed state, a --no-rollback is possible as long as there is not a replacement + [StackStatus.UPDATE_FAILED, 'no-rollback', 'no-replacement', 'did-deploy-stack'], + [StackStatus.UPDATE_FAILED, 'no-rollback', 'replacement', 'failpaused-need-rollback-first'], + // Any combination of UPDATE_FAILED & rollback always requires a rollback first + [StackStatus.UPDATE_FAILED, 'rollback', 'replacement', 'failpaused-need-rollback-first'], + [StackStatus.UPDATE_FAILED, 'rollback', 'no-replacement', 'failpaused-need-rollback-first'], + // From a stable state, any deployment containing a replacement requires a regular deployment (--rollback) + [StackStatus.UPDATE_COMPLETE, 'no-rollback', 'replacement', 'replacement-requires-rollback'], +] satisfies Array<[StackStatus, 'rollback' | 'no-rollback', 'replacement' | 'no-replacement', string]>) +('no-rollback and replacement is disadvised: %s %s %s -> %s', async (stackStatus, rollback, replacement, expectedType) => { // GIVEN givenTemplateIs(FAKE_STACK.template); givenStackExists({ - NotificationARNs: ['arn:aws:sns:bermuda-triangle-1337:123456789012:TestTopic'], + // First call StackStatus: stackStatus, + }, { + // Later calls + StackStatus: 'UPDATE_COMPLETE', }); - givenChangeSetContainsReplacement(); + givenChangeSetContainsReplacement(replacement === 'replacement'); // WHEN const result = await deployStack({ ...standardDeployStackArguments(), stack: FAKE_STACK, - rollback: false, + rollback: rollback === 'rollback', + force: true, // Bypass 'canSkipDeploy' }); // THEN @@ -1150,7 +1161,7 @@ test.each([ }); test('assertIsSuccessfulDeployStackResult does what it says', () => { - expect(() => assertIsSuccessfulDeployStackResult({ type: 'replacement-requires-norollback' })).toThrow(); + expect(() => assertIsSuccessfulDeployStackResult({ type: 'replacement-requires-rollback' })).toThrow(); }); /** * Set up the mocks so that it looks like the stack exists to start with @@ -1162,12 +1173,14 @@ function givenStackExists(...overrides: Array>) { overrides = [{}]; } + let handler = mockCloudFormationClient.on(DescribeStacksCommand); + for (const override of overrides.slice(0, overrides.length - 1)) { - mockCloudFormationClient.on(DescribeStacksCommand).resolvesOnce({ + handler = handler.resolvesOnce({ Stacks: [{ ...baseResponse, ...override }], }); } - mockCloudFormationClient.on(DescribeStacksCommand).resolves({ + handler.resolves({ Stacks: [{ ...baseResponse, ...overrides[overrides.length - 1] }], }); } @@ -1178,10 +1191,10 @@ function givenTemplateIs(template: any) { }); } -function givenChangeSetContainsReplacement() { +function givenChangeSetContainsReplacement(replacement: boolean) { mockCloudFormationClient.on(DescribeChangeSetCommand).resolves({ Status: 'CREATE_COMPLETE', - Changes: [ + Changes: replacement ? [ { Type: 'Resource', ResourceChange: { @@ -1205,6 +1218,6 @@ function givenChangeSetContainsReplacement() { ], }, }, - ], + ] : [], }); } diff --git a/packages/aws-cdk/test/cdk-toolkit.test.ts b/packages/aws-cdk/test/cdk-toolkit.test.ts index bf23245377dd7..d69da1e007562 100644 --- a/packages/aws-cdk/test/cdk-toolkit.test.ts +++ b/packages/aws-cdk/test/cdk-toolkit.test.ts @@ -1464,11 +1464,11 @@ describe('synth', () => { }); test.each([ - [{ type: 'failpaused-need-rollback-first', reason: 'replacement' }, false], - [{ type: 'failpaused-need-rollback-first', reason: 'replacement' }, true], - [{ type: 'failpaused-need-rollback-first', reason: 'not-norollback' }, false], - [{ type: 'replacement-requires-norollback' }, false], - [{ type: 'replacement-requires-norollback' }, true], + [{ type: 'failpaused-need-rollback-first', reason: 'replacement', status: 'OOPS' }, false], + [{ type: 'failpaused-need-rollback-first', reason: 'replacement', status: 'OOPS' }, true], + [{ type: 'failpaused-need-rollback-first', reason: 'not-norollback', status: 'OOPS' }, false], + [{ type: 'replacement-requires-rollback' }, false], + [{ type: 'replacement-requires-rollback' }, true], ] satisfies Array<[DeployStackResult, boolean]>)('no-rollback deployment that cant proceed will be called with rollback on retry: %p (using force: %p)', async (firstResult, useForce) => { cloudExecutable = new MockCloudExecutable({ stacks: [