From 8778309a24112809a3b840bac8831849d5caeeba Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Mon, 22 Jun 2020 11:46:42 +0200 Subject: [PATCH 1/2] fix(cli): cannot change policies or trust after initial bootstrap Our nested stack deployment optimizer only used to look at template differences in order to be able to skip deployments. This has been enhanced over time, but stack parameters were still not included in the evaluation. The new bootstrapping experience relies heavily on parameters to configure important aspects such as policies and trust. However, due to the stack deployment skipping, you would not be able to reconfigure trust or policies after the initial deployment. Now takes parameters into account as well. Relates to #8571, relates to #6582, fixes #6581. --- packages/aws-cdk/lib/api/deploy-stack.ts | 44 ++++++++---- .../aws-cdk/lib/api/util/cloudformation.ts | 72 ++++++++++++++++--- .../aws-cdk/test/api/deploy-stack.test.ts | 58 +++++++++++++++ .../aws-cdk/test/util/cloudformation.test.ts | 58 +++++++++++---- 4 files changed, 193 insertions(+), 39 deletions(-) diff --git a/packages/aws-cdk/lib/api/deploy-stack.ts b/packages/aws-cdk/lib/api/deploy-stack.ts index e7c7999b1a542..93ffd09022c9e 100644 --- a/packages/aws-cdk/lib/api/deploy-stack.ts +++ b/packages/aws-cdk/lib/api/deploy-stack.ts @@ -10,7 +10,7 @@ import { publishAssets } from '../util/asset-publishing'; import { contentHash } from '../util/content-hash'; import { ISDK, SdkProvider } from './aws-auth'; import { ToolkitInfo } from './toolkit-info'; -import { changeSetHasNoChanges, CloudFormationStack, TemplateParameters, waitForChangeSet, waitForStack } from './util/cloudformation'; +import { changeSetHasNoChanges, CloudFormationStack, TemplateParameters, waitForChangeSet, waitForStack, StackParameters } from './util/cloudformation'; import { StackActivityMonitor } from './util/cloudformation/stack-activity-monitor'; // We need to map regions to domain suffixes, and the SDK already has a function to do this. @@ -186,7 +186,20 @@ export async function deployStack(options: DeployStackOptions): Promise { +async function canSkipDeploy(deployStackOptions: DeployStackOptions, cloudFormationStack: CloudFormationStack, params: StackParameters): Promise { const deployName = deployStackOptions.deployName || deployStackOptions.stack.stackName; debug(`${deployName}: checking if we can skip deploy`); @@ -406,6 +414,12 @@ async function canSkipDeploy(deployStackOptions: DeployStackOptions, cloudFormat return false; } + // Parameters have changed + if (params.changed) { + debug(`${deployName}: parameters have changed`); + return false; + } + // We can skip deploy return true; } diff --git a/packages/aws-cdk/lib/api/util/cloudformation.ts b/packages/aws-cdk/lib/api/util/cloudformation.ts index c0ec78e05decd..854e0365e083e 100644 --- a/packages/aws-cdk/lib/api/util/cloudformation.ts +++ b/packages/aws-cdk/lib/api/util/cloudformation.ts @@ -9,6 +9,7 @@ export type Template = { }; interface TemplateParameter { + Type: string; Default?: any; [key: string]: any; } @@ -122,7 +123,21 @@ export class CloudFormationStack { * Empty list if the stack does not exist. */ public get parameterNames(): string[] { - return this.exists ? (this.stack!.Parameters || []).map(p => p.ParameterKey!) : []; + return Object.keys(this.parameters); + } + + /** + * Return the names and values of all current parameters to the stack + * + * Empty object if the stack does not exist. + */ + public get parameters(): Record { + if (!this.exists) { return {}; } + const ret: Record = {}; + for (const param of this.stack!.Parameters ?? []) { + ret[param.ParameterKey!] = param.ParameterValue!; + } + return ret; } /** @@ -286,23 +301,54 @@ export class TemplateParameters { } /** - * Return the set of CloudFormation parameters to pass to the CreateStack or UpdateStack API + * Calculate stack parameters to pass from the given desired parameter values + * + * Will throw if parameters without a Default value or a Previous value are not + * supplied. + */ + public toStackParameters(updates: Record): StackParameters { + return new StackParameters(this.params, updates); + } + + /** + * From the template, the given desired values and the current values, calculate the changes to the stack parameters * * Will take into account parameters already set on the template (will emit * 'UsePreviousValue: true' for those unless the value is changed), and will * throw if parameters without a Default value or a Previous value are not * supplied. */ - public makeApiParameters(updates: Record, prevParams: string[]): CloudFormation.Parameter[] { + public diff(updates: Record, previousValues: Record): StackParameters { + return new StackParameters(this.params, updates, previousValues); + } +} + +export class StackParameters { + /** + * The CloudFormation parameters to pass to the CreateStack or UpdateStack API + */ + public readonly apiParameters: CloudFormation.Parameter[] = []; + + private _changes = false; + + constructor(private readonly params: Record, updates: Record, previousValues: Record = {}) { const missingRequired = new Array(); - const ret: CloudFormation.Parameter[] = []; for (const [key, param] of Object.entries(this.params)) { + // If any of the parameters are SSM parameters, they will always lead to a change + if (param.Type.startsWith('AWS::SSM::Parameter::')) { + this._changes = true; + } if (key in updates && updates[key]) { - ret.push({ ParameterKey: key, ParameterValue: updates[key] }); - } else if (prevParams.includes(key)) { - ret.push({ ParameterKey: key, UsePreviousValue: true }); + this.apiParameters.push({ ParameterKey: key, ParameterValue: updates[key] }); + + // If the updated value is different than the current value, this will lead to a change + if (!(key in previousValues) || updates[key] !== previousValues[key]) { + this._changes = true; + } + } else if (key in previousValues) { + this.apiParameters.push({ ParameterKey: key, UsePreviousValue: true }); } else if (param.Default === undefined) { missingRequired.push(key); } @@ -318,10 +364,14 @@ export class TemplateParameters { const unknownParam = ([key, _]: [string, any]) => this.params[key] === undefined; const hasValue = ([_, value]: [string, any]) => !!value; for (const [key, value] of Object.entries(updates).filter(unknownParam).filter(hasValue)) { - ret.push({ ParameterKey: key, ParameterValue: value }); + this.apiParameters.push({ ParameterKey: key, ParameterValue: value }); } + } - return ret; - + /** + * Whether this set of parameter updates will change the actual stack values + */ + public get changed() { + return this._changes; } -} +} \ No newline at end of file diff --git a/packages/aws-cdk/test/api/deploy-stack.test.ts b/packages/aws-cdk/test/api/deploy-stack.test.ts index 3f9f33b20342d..585665b3e23d9 100644 --- a/packages/aws-cdk/test/api/deploy-stack.test.ts +++ b/packages/aws-cdk/test/api/deploy-stack.test.ts @@ -191,6 +191,64 @@ test('deploy is skipped if template did not change', async () => { expect(cfnMocks.executeChangeSet).not.toBeCalled(); }); +test('deploy is skipped if parameters are the same', async () => { + // GIVEN + givenTemplateIs(FAKE_STACK_WITH_PARAMETERS.template); + givenStackExists({ + Parameters: [ + { ParameterKey: 'HasValue', ParameterValue: 'HasValue' }, + { ParameterKey: 'HasDefault', ParameterValue: 'HasDefault' }, + { ParameterKey: 'OtherParameter', ParameterValue: 'OtherParameter' }, + ], + }); + + // WHEN + await deployStack({ + stack: FAKE_STACK_WITH_PARAMETERS, + sdk, + sdkProvider, + resolvedEnvironment: mockResolvedEnvironment(), + parameters: {}, + usePreviousParameters: true, + }); + + // THEN + expect(cfnMocks.createChangeSet).not.toHaveBeenCalled(); +}); + +test('deploy is not skipped if parameters are different', async () => { + // GIVEN + givenTemplateIs(FAKE_STACK_WITH_PARAMETERS.template); + givenStackExists({ + Parameters: [ + { ParameterKey: 'HasValue', ParameterValue: 'HasValue' }, + { ParameterKey: 'HasDefault', ParameterValue: 'HasDefault' }, + { ParameterKey: 'OtherParameter', ParameterValue: 'OtherParameter' }, + ], + }); + + // WHEN + await deployStack({ + stack: FAKE_STACK_WITH_PARAMETERS, + sdk, + sdkProvider, + resolvedEnvironment: mockResolvedEnvironment(), + parameters: { + HasValue: 'NewValue', + }, + usePreviousParameters: true, + }); + + // THEN + expect(cfnMocks.createChangeSet).toHaveBeenCalledWith(expect.objectContaining({ + Parameters: [ + { ParameterKey: 'HasValue', ParameterValue: 'NewValue' }, + { ParameterKey: 'HasDefault', UsePreviousValue: true }, + { ParameterKey: 'OtherParameter', UsePreviousValue: true }, + ], + })); +}); + test('if existing stack failed to create, it is deleted and recreated', async () => { // GIVEN givenStackExists( diff --git a/packages/aws-cdk/test/util/cloudformation.test.ts b/packages/aws-cdk/test/util/cloudformation.test.ts index 7f813c768d6e8..5662aea49cb2c 100644 --- a/packages/aws-cdk/test/util/cloudformation.test.ts +++ b/packages/aws-cdk/test/util/cloudformation.test.ts @@ -31,12 +31,15 @@ test('A non-existent stack pretends to have an empty template', async () => { expect(await stack.template()).toEqual({}); }); -test('given override, always use the override', () => { - for (const haveDefault of [false, true]) { - for (const havePrevious of [false, true]) { - expect(makeParams(haveDefault, havePrevious, true)).toEqual([USE_OVERRIDE]); - } - } +test.each([ + [false, false], + [false, true], + [true, false], + [true, true]])('given override, always use the override (parameter has a default: %p, parameter previously supplied: %p)', (haveDefault, havePrevious) => { + expect(makeParams(haveDefault, havePrevious, true)).toEqual({ + apiParameters: [USE_OVERRIDE], + changed: true, + }); }); test('no default, no prev, no override => error', () => { @@ -44,15 +47,41 @@ test('no default, no prev, no override => error', () => { }); test('no default, yes prev, no override => use previous', () => { - expect(makeParams(false, true, false)).toEqual([USE_PREVIOUS]); + expect(makeParams(false, true, false)).toEqual({ + apiParameters: [USE_PREVIOUS], + changed: false, + }); }); test('default, no prev, no override => empty param set', () => { - expect(makeParams(true, false, false)).toEqual([]); + expect(makeParams(true, false, false)).toEqual({ + apiParameters: [], + changed: false, + }); }); test('default, prev, no override => use previous', () => { - expect(makeParams(true, true, false)).toEqual([USE_PREVIOUS]); + expect(makeParams(true, true, false)).toEqual({ + apiParameters: [USE_PREVIOUS], + changed: false, + }); +}); + +test('if a parameter is retrieved from SSM, the parameters always count as changed', () => { + const params = TemplateParameters.fromTemplate({ + Parameters: { + Foo: { + Type: 'AWS::SSM::Parameter::Name', + Default: '/Some/Key', + }, + }, + }); + + // If we don't pass a new value + expect(params.diff({}, {'Foo': '/Some/Key'}).changed).toEqual(true); + + // If we do pass a new value but it's the same as the old one + expect(params.diff({'Foo': '/Some/Key'}, {'Foo': '/Some/Key'}).changed).toEqual(true); }); test('unknown parameter in overrides, pass it anyway', () => { @@ -61,11 +90,11 @@ test('unknown parameter in overrides, pass it anyway', () => { // just error out. But maybe we want to be warned of typos... const params = TemplateParameters.fromTemplate({ Parameters: { - Foo: { Default: 'Foo' }, + Foo: { Type: 'String', Default: 'Foo' }, }, }); - expect(params.makeApiParameters({ Bar: 'Bar' }, [])).toEqual([ + expect(params.diff({ Bar: 'Bar' }, {}).apiParameters).toEqual([ { ParameterKey: 'Bar', ParameterValue: 'Bar' }, ]); }); @@ -74,10 +103,13 @@ function makeParams(defaultValue: boolean, hasPrevValue: boolean, override: bool const params = TemplateParameters.fromTemplate({ Parameters: { [PARAM]: { + Type: 'String', Default: defaultValue ? DEFAULT : undefined, }, }, }); - const prevParams = hasPrevValue ? [PARAM] : []; - return params.makeApiParameters({ [PARAM]: override ? OVERRIDE : undefined }, prevParams); + const prevParams: Record = hasPrevValue ? {[PARAM]: 'Foo'} : {}; + const stackParams = params.diff({ [PARAM]: override ? OVERRIDE : undefined }, prevParams); + + return { apiParameters: stackParams.apiParameters, changed: stackParams.changed }; } \ No newline at end of file From 26bf019a2a6146df7d01c60b69433f670cea5a30 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 23 Jun 2020 11:17:04 +0200 Subject: [PATCH 2/2] Happify eslint --- packages/aws-cdk/lib/api/deploy-stack.ts | 8 ++++++-- packages/aws-cdk/lib/api/util/cloudformation.ts | 6 +++++- packages/aws-cdk/test/util/cloudformation.test.ts | 15 ++++++++------- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/packages/aws-cdk/lib/api/deploy-stack.ts b/packages/aws-cdk/lib/api/deploy-stack.ts index 93ffd09022c9e..bf9267986bdea 100644 --- a/packages/aws-cdk/lib/api/deploy-stack.ts +++ b/packages/aws-cdk/lib/api/deploy-stack.ts @@ -10,7 +10,7 @@ import { publishAssets } from '../util/asset-publishing'; import { contentHash } from '../util/content-hash'; import { ISDK, SdkProvider } from './aws-auth'; import { ToolkitInfo } from './toolkit-info'; -import { changeSetHasNoChanges, CloudFormationStack, TemplateParameters, waitForChangeSet, waitForStack, StackParameters } from './util/cloudformation'; +import { changeSetHasNoChanges, CloudFormationStack, StackParameters, TemplateParameters, waitForChangeSet, waitForStack } from './util/cloudformation'; import { StackActivityMonitor } from './util/cloudformation/stack-activity-monitor'; // We need to map regions to domain suffixes, and the SDK already has a function to do this. @@ -380,7 +380,11 @@ export async function destroyStack(options: DestroyStackOptions) { * updated, and the deployment will take a long time to in effect not * do anything. */ -async function canSkipDeploy(deployStackOptions: DeployStackOptions, cloudFormationStack: CloudFormationStack, params: StackParameters): Promise { +async function canSkipDeploy( + deployStackOptions: DeployStackOptions, + cloudFormationStack: CloudFormationStack, + params: StackParameters): Promise { + const deployName = deployStackOptions.deployName || deployStackOptions.stack.stackName; debug(`${deployName}: checking if we can skip deploy`); diff --git a/packages/aws-cdk/lib/api/util/cloudformation.ts b/packages/aws-cdk/lib/api/util/cloudformation.ts index 854e0365e083e..1f88754b1c405 100644 --- a/packages/aws-cdk/lib/api/util/cloudformation.ts +++ b/packages/aws-cdk/lib/api/util/cloudformation.ts @@ -331,7 +331,11 @@ export class StackParameters { private _changes = false; - constructor(private readonly params: Record, updates: Record, previousValues: Record = {}) { + constructor( + private readonly params: Record, + updates: Record, + previousValues: Record = {}) { + const missingRequired = new Array(); for (const [key, param] of Object.entries(this.params)) { diff --git a/packages/aws-cdk/test/util/cloudformation.test.ts b/packages/aws-cdk/test/util/cloudformation.test.ts index 5662aea49cb2c..ab5bdbcb745c6 100644 --- a/packages/aws-cdk/test/util/cloudformation.test.ts +++ b/packages/aws-cdk/test/util/cloudformation.test.ts @@ -35,12 +35,13 @@ test.each([ [false, false], [false, true], [true, false], - [true, true]])('given override, always use the override (parameter has a default: %p, parameter previously supplied: %p)', (haveDefault, havePrevious) => { - expect(makeParams(haveDefault, havePrevious, true)).toEqual({ - apiParameters: [USE_OVERRIDE], - changed: true, + [true, true]])('given override, always use the override (parameter has a default: %p, parameter previously supplied: %p)', + (haveDefault, havePrevious) => { + expect(makeParams(haveDefault, havePrevious, true)).toEqual({ + apiParameters: [USE_OVERRIDE], + changed: true, + }); }); -}); test('no default, no prev, no override => error', () => { expect(() => makeParams(false, false, false)).toThrow(/missing a value: TheParameter/); @@ -78,10 +79,10 @@ test('if a parameter is retrieved from SSM, the parameters always count as chang }); // If we don't pass a new value - expect(params.diff({}, {'Foo': '/Some/Key'}).changed).toEqual(true); + expect(params.diff({}, {Foo: '/Some/Key'}).changed).toEqual(true); // If we do pass a new value but it's the same as the old one - expect(params.diff({'Foo': '/Some/Key'}, {'Foo': '/Some/Key'}).changed).toEqual(true); + expect(params.diff({Foo: '/Some/Key'}, {Foo: '/Some/Key'}).changed).toEqual(true); }); test('unknown parameter in overrides, pass it anyway', () => {