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

rix0rrr · 2021-01-19T15:00:45Z

One of the goals of the "modern synthesis" was to couple the deployment
only to the naming of the resources in the bootstrap stack, not the
bootstrap stack itself. We mostly had that correct, but missed it in
one aspect: because the modern bootstrap stack template is still in
flux, the CLI does a version check against it; however, it currently
performs that version check against the Stack Output of the bootstrap
stack, and in order to do that has to look up the bootstrap stack and
inadvertently became tied to its name again. This was identified by
@redbaron in #11952.

In order to be able to do that version check without the CLI needing to
look up the bootstrap stack, the CLI looks up the SSM parameter with the
version number directly.

The following changes enable that:

Encode the SSM version parameter name into the Cloud Assembly (this
is the magic that decouples from the bootstrap stack name).
In order to be able to read the SSM parameter, the Deploy Role needs
ssm:GetParameter permissions to it. Addition of these permissions
requires a bootstrap stack version bump.
ToolkitInfo.lookup() now always returns an object (even if the
lookup failed), and that object serves as a cache for the SSM read,
so that we don't have to re-fetch the parameter for every asset.
Add an integration test that verifies we can deploy a
modern-synthesized application without knowing the bootstrap stack
name.
Various unit test changes to account for the new API.

There's one little edge case we need to deal with: bootstrap stack
template v5 includes the ssm:GetParameter permissions that we need to
check the bootstrap stack version in an out-of-band way... but if we're
still on bootstrap stack v4 we won't have the permissions yet to read
the version! If we detect that happens (AccessDeniedException), we'll
still formulate that into an "upgrade" message that's as accurate as
possible, using the bootstrap stack template's Output version if found,
or a more generic message if not.

This PR supersedes #11952. Fixes #11420, fixes #9053.

BREAKING CHANGE: users of modern synthesis (DefaultSynthesizer,
used by CDK Pipelines) must upgrade their bootstrap stacks. Run cdk bootstrap.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

In #12394, the ability was added to read context from the CDK config file in the user home directory. However, this interferes with the unit tests which expect a pristine config, so the unit tests fail on a machine with a `~/.cdk.json` file that has context. Fix that.

@redbaron

One of the goals of the "modern synthesis" was to couple the deployment only to the naming of the resources in the bootstrap stack, not the bootstrap stack itself. We *mostly* had that correct, but missed it in one aspect: because the modern bootstrap stack template is still in flux, the CLI does a version check against it; however, it currently performs that version check against the Stack Output of the bootstrap stack, and in order to do that has to look up the bootstrap stack and inadvertently became tied to its name again. This was identified by @redbaron in #11952. In order to be able to do that version check without the CLI needing to look up the bootstrap stack, the CLI looks up the SSM parameter with the version number directly. The following changes enable that: - Encode the SSM version parameter name into the Cloud Assembly (this is the magic that decouples from the bootstrap stack name). - In order to be able to read the SSM parameter, the Deploy Role needs `ssm:GetParameter` permissions to it. Addition of these permissions requires a bootstrap stack version bump. - `ToolkitInfo.lookup()` now always returns an object (even if the lookup failed), and that object serves as a cache for the SSM read, so that we don't have to re-fetch the parameter for every asset. - Add an integration test that verifies we can deploy a modern-synthesized application without knowing the bootstrap stack name. - Various unit test changes to account for the new API. There's one little edge case we need to deal with: bootstrap stack template v5 includes the `ssm:GetParameter` permissions that we need to check the bootstrap stack version in an out-of-band way... but if we're still on bootstrap stack v4 we won't have the permissions yet to read the version! If we detect that happens (`AccessDeniedException`), we'll still formulate that into an "upgrade" message that's as accurate as possible, using the bootstrap stack template's Output version if found, or a more generic message if not. BREAKING CHANGES: users of modern synthesis (`DefaultSynthesizer`, used by CDK Pipelines) must upgrade their bootstrap stacks. Run `cdk bootstrap`.

gitpod-io · 2021-01-19T15:00:52Z

packages/aws-cdk/lib/api/cloudformation-deployments.ts

njlynch · 2021-01-19T18:10:20Z

packages/aws-cdk/lib/api/toolkit-info.ts

-  constructor(public readonly stack: CloudFormationStack, private readonly sdk?: ISDK) {
+  public abstract readonly found: boolean;
+  public abstract readonly bucketUrl: string;
+  public abstract readonly bucketName: string;
+  public abstract readonly version: number;
+  public abstract readonly bootstrapStack: CloudFormationStack;
+
+  private readonly ssmCache = new Map<string, number>();
+
+  constructor(protected readonly sdk: ISDK) {


ToolkitInfo appears to be an exported/exposed class. I don't know what the use cases would be for programmatically integrating with it, but this seems like a breaking API change if anyone is.

It is, but the CLI is not designed to be consumed in that way and we have no contract to uphold here. I have no qualms about breaking this. The class is even marked @experimental to make that very clear.

njlynch · 2021-01-20T11:09:12Z

packages/aws-cdk/lib/api/toolkit-info.ts

+      this.ssmCache.set(parameterName, asNumber);
+      return asNumber;
+    } catch (e) {
+      if (e.code === 'ParameterNotFound') {


Are we sure this only happens when it's a legitimate 404/NotFound? I would bet in some circumstances (e.g., cross-account) at least, you get a ParameterNotFound when you don't have access. That would mean the user gets the less helpful version of the message.

After thinking about this a little: we'd always assume the cross-account role first, then do an in-account read of this parameter. So combined with the research you did, seems like we're safe.

njlynch · 2021-01-20T11:14:52Z

packages/aws-cdk/lib/settings.ts

@@ -47,6 +47,22 @@ export type Arguments = {
  readonly [name: string]: unknown;
 };

+export interface ConfigurationProps {


Are these changes related to the SSM/v5 changes at all? They seem unrelated.

They are the result of an other issue that I found while working on this. Separate PR here: #12579

These changes on this branch will vanish if that other PR is merged.

redbaron · 2021-01-20T11:59:03Z

packages/@aws-cdk/cloud-assembly-schema/schema/cloud-assembly.version.json

@@ -1 +1 @@
-{"version":"8.0.0"}
+{"version":"9.0.0"}


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

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.

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?

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.

Co-authored-by: Nick Lynch <nlynch@amazon.com>

mergify · 2021-01-21T14:29:42Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2021-01-21T15:06:44Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 0a8960f
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2021-01-21T15:07:02Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@redbaron

…#12594) One of the goals of the "modern synthesis" was to couple the deployment only to the naming of the resources in the bootstrap stack, not the bootstrap stack itself. We *mostly* had that correct, but missed it in one aspect: because the modern bootstrap stack template is still in flux, the CLI does a version check against it; however, it currently performs that version check against the Stack Output of the bootstrap stack, and in order to do that has to look up the bootstrap stack and inadvertently became tied to its name again. This was identified by @redbaron in aws#11952. In order to be able to do that version check without the CLI needing to look up the bootstrap stack, the CLI looks up the SSM parameter with the version number directly. The following changes enable that: - Encode the SSM version parameter name into the Cloud Assembly (this is the magic that decouples from the bootstrap stack name). - In order to be able to read the SSM parameter, the Deploy Role needs `ssm:GetParameter` permissions to it. Addition of these permissions requires a bootstrap stack version bump. - `ToolkitInfo.lookup()` now always returns an object (even if the lookup failed), and that object serves as a cache for the SSM read, so that we don't have to re-fetch the parameter for every asset. - Add an integration test that verifies we can deploy a modern-synthesized application without knowing the bootstrap stack name. - Various unit test changes to account for the new API. There's one little edge case we need to deal with: bootstrap stack template v5 includes the `ssm:GetParameter` permissions that we need to check the bootstrap stack version in an out-of-band way... but if we're still on bootstrap stack v4 we won't have the permissions yet to read the version! If we detect that happens (`AccessDeniedException`), we'll still formulate that into an "upgrade" message that's as accurate as possible, using the bootstrap stack template's Output version if found, or a more generic message if not. This PR supersedes aws#11952. Fixes aws#11420, fixes aws#9053. BREAKING CHANGE: users of modern synthesis (`DefaultSynthesizer`, used by CDK Pipelines) must upgrade their bootstrap stacks. Run `cdk bootstrap`. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Two mistakes in the previous attempt at fixing this (#12594): * There was a big fat `if (!bootstrapStack.found) { throw; }` line still in the middle of the code path. We had written an integ test to validate that the new situation would work, however the test was incorrect: it would create a non-default bootstrap stack, but if the account already happened to be default-bootstrapped before, the CLI would accidentally find that default bootstrap stack and use it, thereby never triggering the offending line. * The `BootsraplessSynthesizer` set `requiresBootstrapStackVersion`, which is pretty silly. This synthesizer was being used by CodePipeline's cross-region support stacks, so for cross-region deployments we would still require a bootstrap stack. Both of these are fixed and the test has been updated to force the CLI to look up a definitely nonexistent bootstrap stack. Fixes #12732.

…12771) Two mistakes in the previous attempt at fixing this (#12594): * There was a big fat `if (!bootstrapStack.found) { throw; }` line still in the middle of the code path. We had written an integ test to validate that the new situation would work, however the test was incorrect: it would create a non-default bootstrap stack, but if the account already happened to be default-bootstrapped before, the CLI would accidentally find that default bootstrap stack and use it, thereby never triggering the offending line. * The `BootsraplessSynthesizer` set `requiresBootstrapStackVersion`, which is pretty silly. This synthesizer was being used by CodePipeline's cross-region support stacks, so for cross-region deployments we would still require a bootstrap stack. Both of these are fixed and the test has been updated to force the CLI to look up a definitely nonexistent bootstrap stack. Fixes #12732. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…ws#12771) Two mistakes in the previous attempt at fixing this (aws#12594): * There was a big fat `if (!bootstrapStack.found) { throw; }` line still in the middle of the code path. We had written an integ test to validate that the new situation would work, however the test was incorrect: it would create a non-default bootstrap stack, but if the account already happened to be default-bootstrapped before, the CLI would accidentally find that default bootstrap stack and use it, thereby never triggering the offending line. * The `BootsraplessSynthesizer` set `requiresBootstrapStackVersion`, which is pretty silly. This synthesizer was being used by CodePipeline's cross-region support stacks, so for cross-region deployments we would still require a bootstrap stack. Both of these are fixed and the test has been updated to force the CLI to look up a definitely nonexistent bootstrap stack. Fixes aws#12732. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

rix0rrr added 2 commits January 19, 2021 15:26

rix0rrr requested a review from a team January 19, 2021 15:00

rix0rrr self-assigned this Jan 19, 2021

github-actions bot added the @aws-cdk/core Related to core CDK functionality label Jan 19, 2021

mergify bot added the contribution/core This is a PR that came from AWS. label Jan 19, 2021

rix0rrr mentioned this pull request Jan 19, 2021

feat(cli): decouple bootstrap stack resource discovery from CF stack name #11952

Closed

Merge branch 'master' into huijbers/fail-bootstrap-lookup

cfcf821

njlynch suggested changes Jan 20, 2021

View reviewed changes

redbaron reviewed Jan 20, 2021

View reviewed changes

Update packages/aws-cdk/lib/api/cloudformation-deployments.ts

5a177db

Co-authored-by: Nick Lynch <nlynch@amazon.com>

rix0rrr requested a review from njlynch January 20, 2021 13:02

Fix test

7556f7e

njlynch approved these changes Jan 21, 2021

View reviewed changes

Merge branch 'master' into huijbers/fail-bootstrap-lookup

0a8960f

mergify bot merged commit e5c616f into master Jan 21, 2021

mergify bot deleted the huijbers/fail-bootstrap-lookup branch January 21, 2021 15:07

hross-frae mentioned this pull request Jan 27, 2021

(pipelines): Stages deployed via pipelines do not support renamed bootstrap stacks #12732

Closed

rix0rrr mentioned this pull request Jan 29, 2021

fix(cli, codepipeline): renamed bootstrap stack still not supported #12771

Merged

rix0rrr mentioned this pull request Apr 6, 2021

(cli): the toolkit stack not being found is not an error, but the message printed by default looks threatening #13764

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rix0rrr commented Jan 19, 2021 •

edited

Loading

gitpod-io bot commented Jan 19, 2021 •

edited

Loading

njlynch Jan 19, 2021

rix0rrr Jan 20, 2021

njlynch Jan 20, 2021

rix0rrr Jan 20, 2021

njlynch Jan 20, 2021

rix0rrr Jan 20, 2021

redbaron Jan 20, 2021

rix0rrr Jan 20, 2021

redbaron Jan 20, 2021

rix0rrr Jan 21, 2021

mergify bot commented Jan 21, 2021

aws-cdk-automation commented Jan 21, 2021

mergify bot commented Jan 21, 2021

		@@ -1 +1 @@
		{"version":"8.0.0"}
		{"version":"9.0.0"}

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

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

Conversation

rix0rrr commented Jan 19, 2021 • edited Loading

gitpod-io bot commented Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Jan 21, 2021

aws-cdk-automation commented Jan 21, 2021

AWS CodeBuild CI Report

mergify bot commented Jan 21, 2021

rix0rrr commented Jan 19, 2021 •

edited

Loading

gitpod-io bot commented Jan 19, 2021 •

edited

Loading