Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(core): modern deployments fail if bootstrap stack is renamed #12594

Merged
merged 6 commits into from
Jan 21, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,21 @@ export interface AwsCloudFormationStackProperties {
* @default - No bootstrap stack required
*/
readonly requiresBootstrapStackVersion?: number;

/**
* SSM parameter where the bootstrap stack version number can be found
*
* Only used if `requiresBootstrapStackVersion` is set.
*
* - If this value is not set, the bootstrap stack name must be known at
* deployment time so the stack version can be looked up from the stack
* outputs.
* - If this value is set, the bootstrap stack can have any name because
* we won't need to look it up.
*
* @default - Bootstrap stack version number looked up
*/
readonly bootstrapStackVersionSsmParameter?: string;
}

/**
Expand All @@ -79,6 +94,19 @@ export interface AssetManifestProperties {
* @default - Version 1 (basic modern bootstrap stack)
*/
readonly requiresBootstrapStackVersion?: number;

/**
* SSM parameter where the bootstrap stack version number can be found
*
* - If this value is not set, the bootstrap stack name must be known at
* deployment time so the stack version can be looked up from the stack
* outputs.
* - If this value is set, the bootstrap stack can have any name because
* we won't need to look it up.
*
* @default - Bootstrap stack version number looked up
*/
readonly bootstrapStackVersionSsmParameter?: string;
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,10 @@
"requiresBootstrapStackVersion": {
"description": "Version of bootstrap stack required to deploy this stack (Default - No bootstrap stack required)",
"type": "number"
},
"bootstrapStackVersionSsmParameter": {
"description": "SSM parameter where the bootstrap stack version number can be found\n\nOnly used if `requiresBootstrapStackVersion` is set.\n\n- If this value is not set, the bootstrap stack name must be known at\n deployment time so the stack version can be looked up from the stack\n outputs.\n- If this value is set, the bootstrap stack can have any name because\n we won't need to look it up. (Default - Bootstrap stack version number looked up)",
"type": "string"
}
},
"required": [
Expand All @@ -323,6 +327,10 @@
"requiresBootstrapStackVersion": {
"description": "Version of bootstrap stack required to deploy this stack (Default - Version 1 (basic modern bootstrap stack))",
"type": "number"
},
"bootstrapStackVersionSsmParameter": {
"description": "SSM parameter where the bootstrap stack version number can be found\n\n- If this value is not set, the bootstrap stack name must be known at\n deployment time so the stack version can be looked up from the stack\n outputs.\n- If this value is set, the bootstrap stack can have any name because\n we won't need to look it up. (Default - Bootstrap stack version number looked up)",
"type": "string"
}
},
"required": [
Expand Down
Original file line number Diff line number Diff line change
@@ -1 +1 @@
{"version":"8.0.0"}
{"version":"9.0.0"}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe not a big deal, but shouldn't it be "8.1.0"? Old CDK CLI refuses to deploy future major versions of cloud assembly, but technically this change doesn't prevent them from doing so

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, but it will be very confusing for people otherwise if they have a framework version >1.86.0 (say) and SUPPOSED to have this feature work, but it won't work because they're using a CLI <1.86.0 (and nobody's telling them).

Better to force both upgrades to happen synchronously.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but it won't work because they're using a CLI

If new bootstrap continues to create CF stack output old CLI is checking, then everything continues to work, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not the issue. The issue is people depending on the "new feature" (advertised in the CHANGELOG) that you can now deploy even with a nonstandard bootstrap stack name--except it doesn't work!

Why? Because they upgraded their framework but not their CLI.

Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,7 @@ export class DefaultStackSynthesizer extends StackSynthesizer {
cloudFormationExecutionRoleArn: this._cloudFormationExecutionRoleArn,
stackTemplateAssetObjectUrl: templateManifestUrl,
requiresBootstrapStackVersion: MIN_BOOTSTRAP_STACK_VERSION,
bootstrapStackVersionSsmParameter: `/cdk-bootstrap/${this.qualifier}/version`,
additionalDependencies: [artifactId],
});
}
Expand Down Expand Up @@ -472,6 +473,7 @@ export class DefaultStackSynthesizer extends StackSynthesizer {
properties: {
file: manifestFile,
requiresBootstrapStackVersion: MIN_BOOTSTRAP_STACK_VERSION,
bootstrapStackVersionSsmParameter: `/cdk-bootstrap/${this.qualifier}/version`,
},
});

Expand Down
15 changes: 15 additions & 0 deletions packages/@aws-cdk/core/lib/stack-synthesizers/stack-synthesizer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,4 +106,19 @@ export interface SynthesizeStackArtifactOptions {
* @default - No bootstrap stack required
*/
readonly requiresBootstrapStackVersion?: number;

/**
* SSM parameter where the bootstrap stack version number can be found
*
* Only used if `requiresBootstrapStackVersion` is set.
*
* - If this value is not set, the bootstrap stack name must be known at
* deployment time so the stack version can be looked up from the stack
* outputs.
* - If this value is set, the bootstrap stack can have any name because
* we won't need to look it up.
*
* @default - Bootstrap stack version number looked up
*/
readonly bootstrapStackVersionSsmParameter?: string;
}
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,15 @@ nodeunitShim({
const asm = app.synth();

// THEN - we have an asset manifest with both assets and the stack template in there
const manifest = readAssetManifest(asm);
const manifestArtifact = getAssetManifest(asm);
const manifest = readAssetManifest(manifestArtifact);

test.equals(Object.keys(manifest.files || {}).length, 2);
test.equals(Object.keys(manifest.dockerImages || {}).length, 1);

// THEN - the asset manifest has an SSM parameter entry
expect(manifestArtifact.bootstrapStackVersionSsmParameter).toEqual('/cdk-bootstrap/hnb659fds/version');

// THEN - every artifact has an assumeRoleArn
for (const file of Object.values(manifest.files ?? {})) {
for (const destination of Object.values(file.destinations)) {
Expand Down Expand Up @@ -200,7 +204,7 @@ nodeunitShim({

// THEN
const asm = myapp.synth();
const manifest = readAssetManifest(asm);
const manifest = readAssetManifest(getAssetManifest(asm));

test.deepEqual(manifest.files?.['file-asset-hash']?.destinations?.['current_account-current_region'], {
bucketName: 'file-asset-bucket',
Expand Down Expand Up @@ -247,7 +251,7 @@ nodeunitShim({
const stackArtifact = asm.getStackArtifact('mystack-bucketPrefix');

// THEN - we have an asset manifest with both assets and the stack template in there
const manifest = readAssetManifest(asm);
const manifest = readAssetManifest(getAssetManifest(asm));

// THEN
test.deepEqual(manifest.files?.['file-asset-hash-with-prefix']?.destinations?.['current_account-current_region'], {
Expand Down Expand Up @@ -299,10 +303,13 @@ function isAssetManifest(x: cxapi.CloudArtifact): x is cxapi.AssetManifestArtifa
return x instanceof cxapi.AssetManifestArtifact;
}

function readAssetManifest(asm: cxapi.CloudAssembly): cxschema.AssetManifest {
function getAssetManifest(asm: cxapi.CloudAssembly): cxapi.AssetManifestArtifact {
const manifestArtifact = asm.artifacts.filter(isAssetManifest)[0];
if (!manifestArtifact) { throw new Error('no asset manifest in assembly'); }
return manifestArtifact;
}

function readAssetManifest(manifestArtifact: cxapi.AssetManifestArtifact): cxschema.AssetManifest {
return JSON.parse(fs.readFileSync(manifestArtifact.file, { encoding: 'utf-8' }));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@ export class AssetManifestArtifact extends CloudArtifact {
*/
public readonly requiresBootstrapStackVersion: number;

/**
* Name of SSM parameter with bootstrap stack version
*
* @default - Discover SSM parameter by reading stack
*/
public readonly bootstrapStackVersionSsmParameter?: string;

constructor(assembly: CloudAssembly, name: string, artifact: cxschema.ArtifactManifest) {
super(assembly, name, artifact);

Expand All @@ -26,5 +33,6 @@ export class AssetManifestArtifact extends CloudArtifact {
}
this.file = path.resolve(this.assembly.directory, properties.file);
this.requiresBootstrapStackVersion = properties.requiresBootstrapStackVersion ?? 1;
this.bootstrapStackVersionSsmParameter = properties.bootstrapStackVersionSsmParameter;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,13 @@ export class CloudFormationStackArtifact extends CloudArtifact {
*/
public readonly requiresBootstrapStackVersion?: number;

/**
* Name of SSM parameter with bootstrap stack version
*
* @default - Discover SSM parameter by reading stack
*/
public readonly bootstrapStackVersionSsmParameter?: string;

/**
* Whether termination protection is enabled for this stack.
*/
Expand Down Expand Up @@ -110,6 +117,7 @@ export class CloudFormationStackArtifact extends CloudArtifact {
this.cloudFormationExecutionRoleArn = properties.cloudFormationExecutionRoleArn;
this.stackTemplateAssetObjectUrl = properties.stackTemplateAssetObjectUrl;
this.requiresBootstrapStackVersion = properties.requiresBootstrapStackVersion;
this.bootstrapStackVersionSsmParameter = properties.bootstrapStackVersionSsmParameter;
this.terminationProtection = properties.terminationProtection;

this.stackName = properties.stackName || artifactId;
Expand Down
6 changes: 4 additions & 2 deletions packages/aws-cdk/bin/cdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,10 @@ async function initCommandLine() {
debug('Command line arguments:', argv);

const configuration = new Configuration({
...argv,
_: argv._ as [Command, ...string[]], // TypeScript at its best
commandLineArguments: {
...argv,
_: argv._ as [Command, ...string[]], // TypeScript at its best
},
});
await configuration.load();

Expand Down
4 changes: 2 additions & 2 deletions packages/aws-cdk/lib/api/aws-auth/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -218,9 +218,9 @@ export class SDK implements ISDK {
// do additional things to errors.
return Object.assign(Object.create(response), {
promise() {
return response.promise().catch((e: Error) => {
return response.promise().catch((e: Error & { code?: string }) => {
e = self.makeDetailedException(e);
debug(`Call failed: ${prop}(${JSON.stringify(args[0])}) => ${e.message}`);
debug(`Call failed: ${prop}(${JSON.stringify(args[0])}) => ${e.message} (code=${e.code})`);
return Promise.reject(e); // Re-'throw' the new error
});
},
Expand Down
8 changes: 7 additions & 1 deletion packages/aws-cdk/lib/api/bootstrap/bootstrap-template.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,12 @@ Resources:
- sts:GetCallerIdentity
Resource: "*"
Effect: Allow
- Sid: ReadVersion
Effect: Allow
Action:
- ssm:GetParameter
Resource:
- Fn::Sub: "arn:aws:ssm:${AWS::Region}:${AWS::AccountId}:parameter${CdkBootstrapVersion}"
Version: '2012-10-17'
PolicyName: default
RoleName:
Expand Down Expand Up @@ -387,7 +393,7 @@ Resources:
Type: String
Name:
Fn::Sub: '/cdk-bootstrap/${Qualifier}/version'
Value: '4'
Value: '5'
Outputs:
BucketName:
Description: The name of the S3 bucket owned by the CDK toolkit stack
Expand Down
16 changes: 7 additions & 9 deletions packages/aws-cdk/lib/api/bootstrap/deploy-bootstrap.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,14 @@ import { BOOTSTRAP_VERSION_OUTPUT, BootstrapEnvironmentOptions, BOOTSTRAP_VERSIO
*
* And do something in between the two phases (such as look at the
* current bootstrap stack and doing something intelligent).
*
* This class is different from `ToolkitInfo` in that `ToolkitInfo`
* is purely read-only, and `ToolkitInfo.lookup()` returns `undefined`
* if the stack does not exist. But honestly, these classes could and
* should probably be merged at some point.
*/
export class BootstrapStack {
public static async lookup(sdkProvider: SdkProvider, environment: cxapi.Environment, toolkitStackName?: string) {
toolkitStackName = toolkitStackName ?? DEFAULT_TOOLKIT_STACK_NAME;

const resolvedEnvironment = await sdkProvider.resolveEnvironment(environment);
const sdk = await sdkProvider.forEnvironment(resolvedEnvironment, Mode.ForWriting);

const currentToolkitInfo = await ToolkitInfo.lookup(resolvedEnvironment, sdk, toolkitStackName);

return new BootstrapStack(sdkProvider, sdk, resolvedEnvironment, toolkitStackName, currentToolkitInfo);
Expand All @@ -43,15 +39,15 @@ export class BootstrapStack {
private readonly sdk: ISDK,
private readonly resolvedEnvironment: cxapi.Environment,
private readonly toolkitStackName: string,
private readonly currentToolkitInfo?: ToolkitInfo) {
private readonly currentToolkitInfo: ToolkitInfo) {
}

public get parameters(): Record<string, string> {
return this.currentToolkitInfo?.parameters ?? {};
return this.currentToolkitInfo.found ? this.currentToolkitInfo.bootstrapStack.parameters : {};
}

public get terminationProtection() {
return this.currentToolkitInfo?.stack?.terminationProtection;
return this.currentToolkitInfo.found ? this.currentToolkitInfo.bootstrapStack.terminationProtection : undefined;
}

public async partition(): Promise<string> {
Expand All @@ -68,7 +64,7 @@ export class BootstrapStack {
): Promise<DeployStackResult> {

const newVersion = bootstrapVersionFromTemplate(template);
if (this.currentToolkitInfo && newVersion < this.currentToolkitInfo.version && !options.force) {
if (this.currentToolkitInfo.found && newVersion < this.currentToolkitInfo.version && !options.force) {
throw new Error(`Not downgrading existing bootstrap stack from version '${this.currentToolkitInfo.version}' to version '${newVersion}'. Use --force to force.`);
}

Expand Down Expand Up @@ -99,6 +95,8 @@ export class BootstrapStack {
execute: options.execute,
parameters,
usePreviousParameters: true,
// Obviously we can't need a bootstrap stack to deploy a bootstrap stack
toolkitInfo: ToolkitInfo.bootstraplessDeploymentsOnly(this.sdk),
});
}
}
Expand Down
27 changes: 19 additions & 8 deletions packages/aws-cdk/lib/api/cloudformation-deployments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,11 @@ export class CloudFormationDeployments {
await this.publishStackAssets(options.stack, toolkitInfo);

// Do a verification of the bootstrap stack version
this.validateBootstrapStackVersion(options.stack.stackName, options.stack.requiresBootstrapStackVersion, toolkitInfo);
await this.validateBootstrapStackVersion(
options.stack.stackName,
options.stack.requiresBootstrapStackVersion,
options.stack.bootstrapStackVersionSsmParameter,
toolkitInfo);

return deployStack({
stack: options.stack,
Expand Down Expand Up @@ -251,12 +255,16 @@ export class CloudFormationDeployments {
/**
* Publish all asset manifests that are referenced by the given stack
*/
private async publishStackAssets(stack: cxapi.CloudFormationStackArtifact, bootstrapStack: ToolkitInfo | undefined) {
private async publishStackAssets(stack: cxapi.CloudFormationStackArtifact, toolkitInfo: ToolkitInfo) {
const stackEnv = await this.sdkProvider.resolveEnvironment(stack.environment);
const assetArtifacts = stack.dependencies.filter(isAssetManifestArtifact);

for (const assetArtifact of assetArtifacts) {
this.validateBootstrapStackVersion(stack.stackName, assetArtifact.requiresBootstrapStackVersion, bootstrapStack);
await this.validateBootstrapStackVersion(
stack.stackName,
assetArtifact.requiresBootstrapStackVersion,
assetArtifact.bootstrapStackVersionSsmParameter,
toolkitInfo);

const manifest = AssetManifest.fromFile(assetArtifact.file);
await publishAssets(manifest, this.sdkProvider, stackEnv);
Expand All @@ -266,19 +274,22 @@ export class CloudFormationDeployments {
/**
* Validate that the bootstrap stack has the right version for this stack
*/
private validateBootstrapStackVersion(
private async validateBootstrapStackVersion(
stackName: string,
requiresBootstrapStackVersion: number | undefined,
bootstrapStack: ToolkitInfo | undefined) {
bootstrapStackVersionSsmParameter: string | undefined,
toolkitInfo: ToolkitInfo) {

if (requiresBootstrapStackVersion === undefined) { return; }

if (!bootstrapStack) {
if (!toolkitInfo.found) {
throw new Error(`${stackName}: publishing assets requires bootstrap stack version '${requiresBootstrapStackVersion}', no bootstrap stack found. Please run 'cdk bootstrap'.`);
}

if (requiresBootstrapStackVersion > bootstrapStack.version) {
throw new Error(`${stackName}: publishing assets requires bootstrap stack version '${requiresBootstrapStackVersion}', found '${bootstrapStack.version}'. Please run 'cdk bootstrap' with a newer CLI version.`);
try {
await toolkitInfo.validateVersion(requiresBootstrapStackVersion, bootstrapStackVersionSsmParameter);
} catch (e) {
throw new Error(`${stackName}: ${e.message}`);
}
}
}
Expand Down
8 changes: 3 additions & 5 deletions packages/aws-cdk/lib/api/deploy-stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,8 @@ export interface DeployStackOptions {

/**
* Information about the bootstrap stack found in the target environment
*
* @default - Assume there is no bootstrap stack
*/
toolkitInfo?: ToolkitInfo;
toolkitInfo: ToolkitInfo;

/**
* Role to pass to CloudFormation to execute the change set
Expand Down Expand Up @@ -313,7 +311,7 @@ async function makeBodyParameter(
stack: cxapi.CloudFormationStackArtifact,
resolvedEnvironment: cxapi.Environment,
assetManifest: AssetManifestBuilder,
toolkitInfo?: ToolkitInfo): Promise<TemplateBodyParameter> {
toolkitInfo: ToolkitInfo): Promise<TemplateBodyParameter> {

// If the template has already been uploaded to S3, just use it from there.
if (stack.stackTemplateAssetObjectUrl) {
Expand All @@ -327,7 +325,7 @@ async function makeBodyParameter(
return { TemplateBody: templateJson };
}

if (!toolkitInfo) {
if (!toolkitInfo.found) {
error(
`The template for stack "${stack.displayName}" is ${Math.round(templateJson.length / 1024)}KiB. ` +
`Templates larger than ${LARGE_TEMPLATE_SIZE_KB}KiB must be uploaded to S3.\n` +
Expand Down
Loading