Skip to content

Commit

Permalink
fix(cli): cannot change policies or trust after initial bootstrap (#8677
Browse files Browse the repository at this point in the history 
)

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.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
@rix0rrr
rix0rrr authored Jun 24, 2020
1 parent 9568bf7 commit 6e6b23e
Show file tree
Hide file tree
Showing 4 changed files with 203 additions and 40 deletions.
48 changes: 33 additions & 15 deletions packages/aws-cdk/lib/api/deploy-stack.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, 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.
Expand Down Expand Up @@ -186,7 +186,20 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt
cloudFormationStack = CloudFormationStack.doesNotExist(cfn, deployName);
}

if (await canSkipDeploy(options, cloudFormationStack)) {
// Detect "legacy" assets (which remain in the metadata) and publish them via
// an ad-hoc asset manifest, while passing their locations via template
// parameters.
const legacyAssets = new AssetManifestBuilder();
const assetParams = await addMetadataAssetsToManifest(stackArtifact, legacyAssets, options.toolkitInfo, options.reuseAssets);

const finalParameterValues = { ...options.parameters, ...assetParams };

const templateParams = TemplateParameters.fromTemplate(stackArtifact.template);
const stackParams = options.usePreviousParameters
? templateParams.diff(finalParameterValues, cloudFormationStack.parameters)
: templateParams.toStackParameters(finalParameterValues);

if (await canSkipDeploy(options, cloudFormationStack, stackParams)) {
debug(`${deployName}: skipping deployment (use --force to override)`);
return {
noOp: true,
Expand All @@ -198,17 +211,6 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt
debug(`${deployName}: deploying...`);
}

// Detect "legacy" assets (which remain in the metadata) and publish them via
// an ad-hoc asset manifest, while passing their locations via template
// parameters.
const legacyAssets = new AssetManifestBuilder();
const assetParams = await addMetadataAssetsToManifest(stackArtifact, legacyAssets, options.toolkitInfo, options.reuseAssets);

const apiParameters = TemplateParameters.fromTemplate(stackArtifact.template).makeApiParameters({
...options.parameters,
...assetParams,
}, options.usePreviousParameters ? cloudFormationStack.parameterNames : []);

const executionId = uuid.v4();
const bodyParameter = await makeBodyParameter(stackArtifact, options.resolvedEnvironment, legacyAssets, options.toolkitInfo);

Expand All @@ -226,7 +228,7 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt
Description: `CDK Changeset for execution ${executionId}`,
TemplateBody: bodyParameter.TemplateBody,
TemplateURL: bodyParameter.TemplateURL,
Parameters: apiParameters,
Parameters: stackParams.apiParameters,
RoleARN: options.roleArn,
NotificationARNs: options.notificationArns,
Capabilities: [ 'CAPABILITY_IAM', 'CAPABILITY_NAMED_IAM', 'CAPABILITY_AUTO_EXPAND' ],
Expand Down Expand Up @@ -371,8 +373,18 @@ export async function destroyStack(options: DestroyStackOptions) {

/**
* Checks whether we can skip deployment
*
* We do this in a complicated way by preprocessing (instead of just
* looking at the changeset), because if there are nested stacks involved
* the changeset will always show the nested stacks as needing to be
* updated, and the deployment will take a long time to in effect not
* do anything.
*/
async function canSkipDeploy(deployStackOptions: DeployStackOptions, cloudFormationStack: CloudFormationStack): Promise<boolean> {
async function canSkipDeploy(
deployStackOptions: DeployStackOptions,
cloudFormationStack: CloudFormationStack,
params: StackParameters): Promise<boolean> {

const deployName = deployStackOptions.deployName || deployStackOptions.stack.stackName;
debug(`${deployName}: checking if we can skip deploy`);

Expand Down Expand Up @@ -406,6 +418,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;
}
Expand Down
76 changes: 65 additions & 11 deletions packages/aws-cdk/lib/api/util/cloudformation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ export type Template = {
};

interface TemplateParameter {
Type: string;
Default?: any;
[key: string]: any;
}
Expand Down Expand Up @@ -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<string, string> {
if (!this.exists) { return {}; }
const ret: Record<string, string> = {};
for (const param of this.stack!.Parameters ?? []) {
ret[param.ParameterKey!] = param.ParameterValue!;
}
return ret;
}

/**
Expand Down Expand Up @@ -286,23 +301,58 @@ 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<string, string | undefined>): 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<string, string | undefined>, prevParams: string[]): CloudFormation.Parameter[] {
public diff(updates: Record<string, string | undefined>, previousValues: Record<string, string>): 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<string, TemplateParameter>,
updates: Record<string, string | undefined>,
previousValues: Record<string, string> = {}) {

const missingRequired = new Array<string>();

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);
}
Expand All @@ -318,10 +368,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;
}
}
}
58 changes: 58 additions & 0 deletions packages/aws-cdk/test/api/deploy-stack.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
61 changes: 47 additions & 14 deletions packages/aws-cdk/test/util/cloudformation.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,28 +31,58 @@ 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', () => {
expect(() => makeParams(false, false, false)).toThrow(/missing a value: TheParameter/);
});

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', () => {
Expand All @@ -61,11 +91,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' },
]);
});
Expand All @@ -74,10 +104,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<string, string> = hasPrevValue ? {[PARAM]: 'Foo'} : {};
const stackParams = params.diff({ [PARAM]: override ? OVERRIDE : undefined }, prevParams);

return { apiParameters: stackParams.apiParameters, changed: stackParams.changed };
}

0 comments on commit 6e6b23e

Please sign in to comment.