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

redbaron · 2020-12-09T03:23:15Z

Previously cdk deploy was locating CDKToolkit stack by name. Sometimes name is not known upfront, for instance when bootstraping is done via StackSets.

All cdk deploy needs to know is asset bucket and bootstrap version, these can be communicated via SSM parameters with a known names.

This PR is made of individual patches gradually removing dependency on a CF stack name
from cdk deploy stage. cdk bootstrap is not affected and continues to use CF stack name.

Main user visible changes are:

toolkit-stack-name CLI flag is accepted in bootstrap command, but not in deploy anymore
new optional bootstrap-qualifier param for cdk deploy command. It defaults to the value used by bootstrap CF template.
bootstrap stack version bumped to 5.

fixes: #11420

cc: @wulfmann

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

ToolkitInfo served 2 functions: 1. locate CDKToolkit stack and it's parameters. This is used when bootstrapping. 2. locate bootstrapped resources, such as S3 asset bucket and ECR registry. This is used when deploying. This change extracts CDKToolkit stack related function to own ToolkitStackInfo class. This prepares code for a future change to the way deployment locates bootstrapped resources.

…ts and ECR `cdk deploy` needs to know where to put assets and previously was using CDKToolkit stack's outputs to find it. This ties resource discovery to a CloudFormation stack name, which has following disadvantages: - Bootstrap stack can't be depoyed via StackSet, because stacks created in target accounts have random names - Bootstrapping must be done with CloudFormation only This changes asset resource discovery to be done via qualified SSM Parameters.

Previously `cdk deploy` was locating CDKToolkit stack by name. Sometimes name is not known upfront, for instance when bootstraping is done via StackSets. Preivous commits in this patch series changed bootstrap stack to create SSM parameters with same value as a stack output. This patch changes lookup mehanism to use SSM and qualifier to locate assets buckets and ECR registry. With known qualifier we know by convention exact SSM parameters to lookup. This decouples discovery from CF stack names. It even allows bootstrap to be done not by CF if user desires so.

gitpod-io · 2020-12-09T03:23:18Z

redbaron · 2020-12-10T14:54:39Z

@rix0rrr , would you mind to take a look if you have time?

cdk deploy reads SSM parameters created by bootstrap stack

rix0rrr · 2020-12-14T10:33:57Z

Did not look at the code yet. Thanks for writing a comprehensive PR description, that really helps a lot. I immediately have some things to say about this:

All cdk deploy needs to know is asset bucket and bootstrap version

Well no. For DefaultSynthesized stacks, the asset bucket will be known by the CDK app and written to the asset manifest. cdk deploy does not need to know it.

For LegacySynthesized stacks... I guess we don't support bootstrapping via stacksets. We could try and support this by scanning the stacks in the account and finding the bootstrap stack between all available ones, but I don't necessarily thing that's the best way of going about it. Switching to DefaultSynthesized stacks would be better.

toolkit-stack-name CLI flag is accepted in bootstrap command, but not in deploy anymore

We can't do that.

First of all: for stacks using the LegacySynthesizer, this will still be required, so we can't outright remove this.
Second of all: we can't just remove command line flags because people might be using them in CI scripts and making the flag unrecognized will throw an error and break the CI pipeline. Best we can do is deprecate.

new optional bootstrap-qualifier param for cdk deploy command. It defaults to the value used by bootstrap CF template.

Not sure I'm into this. We should strive to keep the CLI simple. The current mechanism for this is to have the app specify the bootstrap qualifier, and this can be done either in code or using context (either, again, in code, or in cdk.json). No CLI flag necessary.

rix0rrr

Thanks for submitting, but I think I disagree with the premise of the changes made here.

redbaron · 2020-12-14T11:52:09Z

@rix0rrr , thanks for looking into this.

This change focuses on a newly synthesized stacks, leaving legacy stacks out of scope. I am happy to make changes, leaving legacy behaviour as is. My follow up comments are in the context of using it with new synth only.

Well no. For DefaultSynthesized stacks, the asset bucket will be known by the CDK app and written to the asset manifest. cdk deploy does not need to know it.

In theory yes, but for some reason current code uses CDKToolkit output, not Cloud Assembly asset url to upload:

aws-cdk/packages/aws-cdk/lib/assets.ts

Line 82 in 9417b02

const s3url = `s3://${toolkitInfo.bucketName}/${key}`;

I didn't want to change that behaviour.

Another place where S3 bucket information is used is during template body upload if it is not set in cloud assembly:

aws-cdk/packages/aws-cdk/lib/api/deploy-stack.ts

Line 350 in 9417b02

const templateURL = `${toolkitInfo.bucketUrl}/${key}`;

If you think, that cloud assembly should be authoritative for asset location, then it is likely that there is no need to make use of CDKToolkit CF template at all and all the code to look it up can be removed.

Only thing left would be version check, which can be switched to existing SSM parameter.

toolkit-stack-name CLI flag is accepted in bootstrap command, but not in deploy anymore

We can't do that.

I agree. Once we find a good way to move forward, I'll make sure that new behaviour kicks in only if flag is not specified.

The current mechanism for this is to have the app specify the bootstrap qualifier, and this can be done either in code or using context (either, again, in code, or in cdk.json). No CLI flag necessary.

My understanding is that qualifier can be set during synth time (via context or in code), but is never persisted in cloud assembly as a first class value. Therefore when we come to cdk deploy, it should be able to deploy any cloud assembly artefact and for that it needs to know qualifier to at least check bootstrap stack version. Currently it has no other means to get qualifier other than asking users to provide it, because cdk deploy could be called with just an cloud assembly artefact provided, but outside of a directory where that artefact was built.

redbaron · 2020-12-23T10:56:23Z

@rix0rrr , I'd appreciate your input on this.

rix0rrr · 2021-01-06T14:38:26Z

In theory yes, but for some reason current code uses CDKToolkit output, not Cloud Assembly asset url to upload:

Another place where S3 bucket information is used is during template body upload if it is not set in cloud assembly:

It's true. Both of these should not happen in case of a DefaultSynthesized stack, but the code won't know beforehand whether that's true or not.

I wonder if it would be helpful to return a dummy instance of ToolkitInfo which we'll return if we can't find the bootstrap stack.

If any of its members are called (such as toolkitInfo.bucketName) it can then throw a descriptive error message saying it needs to know the name of the bootstrap stack; basically, throw the same error as would be thrown today.

In case of a DefaultSynthesized stack, none of those members should ever need to be called (as all the information will be in the cloud assembly already) and so it's fine that we returned a dummy object.

Feels that is the best way to marry legacy deployments and modern deployments.

What do you think?

it should be able to deploy any cloud assembly artefact and for that it needs to know qualifier to at least check bootstrap stack version

Hah! You are correct, I apparently didn't reason that all the way through 🤣 . I suppose adding the name of the SSM parameter to the Cloud Assembly manifest is the thing to do here. I'd prefer that over just the qualifier itself as it's functionally equivalent, but leaves more decisions open to the CDK App & the related synthesizer (which is where we prefer them).

There are like 2 or 3 locations where we have this version number in the manifest.

rix0rrr · 2021-01-07T14:57:30Z

This PR also probably closes #9053

redbaron · 2021-01-07T15:21:04Z

I suppose adding the name of the SSM parameter to the Cloud Assembly manifest is the thing to do here.

This is a best course of action IMHO, but I wasn't confident enough in making change to cloud assembly schema and bumping version. If you think it's relatively easy I can try.

redbaron · 2021-01-07T16:49:34Z

Maybe remove version check all together? Remove toolkitInfo.bucketName and toolkitInfo.bucketUrl calls from cdk deploy and if anything fails, give a hint that we are expecting certain version of a bootstrap stack?

rix0rrr · 2021-01-11T10:02:34Z

Maybe remove version check all together?

I strongly believe in giving accurate and actionable error messages as much as we can. Having to replace it with a wishy-washy "something is wrong and it might be this" seems suboptimal.

Let me try and get you started...

Pull request has been modified.

rix0rrr · 2021-01-11T12:49:28Z

I added a commit to switch over to an SSM parameter name in the Cloud Assembly.

I think the next step would be to have ToolkitInfo.lookup always return an object and never fail there, but it may return an object that represents a "failed to find bootstrap stack" state, and which will throw if any of the members that require the bootstrap stack to have been found are called, with an appropriately descriptive error message (please don't throw errors of the form "bucket name not available" or something to that effect -- they will only be helpful to maintainers, not to users!)

I think that should do it. Feels like most of the other changes introduced in this PR could be backed out again. Let me know if I missed something!

aws-cdk-automation · 2021-01-11T13:21:51Z

AWS CodeBuild CI Report

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

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

@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`.

rix0rrr · 2021-01-19T15:01:16Z

Superseded by #12594

@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. 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*

@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*

redbaron added 3 commits December 9, 2020 02:57

github-actions bot added the package/tools Related to AWS CDK Tools or CLI label Dec 9, 2020

github-actions bot assigned rix0rrr Dec 9, 2020

Update readme

abe12df

Guard against missing SSM parameter

468dbb6

redbaron force-pushed the toolkit-lookup-by-qualifier branch from e7a1f22 to 468dbb6 Compare December 10, 2020 20:05

Add ssm:GetParameter to bootstrap deploy-role

c12825f

cdk deploy reads SSM parameters created by bootstrap stack

rix0rrr previously requested changes Dec 14, 2020

View reviewed changes

redbaron requested a review from rix0rrr December 16, 2020 09:24

Merge branch 'master' into toolkit-lookup-by-qualifier

e05a5b6

rix0rrr added 2 commits January 11, 2021 13:44

Encode SSM parameter name into Cloud Assembly

fb5081f

Merge remote-tracking branch 'origin/master' into pr/redbaron/11952

2a5e83a

rix0rrr mentioned this pull request Jan 18, 2021

(pipelines): Unable to deploy specific stack in stage when target stack has dependency stack in another account #12510

Closed

rix0rrr mentioned this pull request Jan 19, 2021

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

Merged

rix0rrr closed this Jan 19, 2021

redbaron deleted the toolkit-lookup-by-qualifier branch February 15, 2021 07:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

redbaron commented Dec 9, 2020

gitpod-io bot commented Dec 9, 2020 •

edited

Loading

redbaron commented Dec 10, 2020

rix0rrr commented Dec 14, 2020 •

edited

Loading

rix0rrr left a comment

redbaron commented Dec 14, 2020 •

edited

Loading

redbaron commented Dec 23, 2020

rix0rrr commented Jan 6, 2021

rix0rrr commented Jan 7, 2021

redbaron commented Jan 7, 2021

redbaron commented Jan 7, 2021

rix0rrr commented Jan 11, 2021

rix0rrr commented Jan 11, 2021

aws-cdk-automation commented Jan 11, 2021

rix0rrr commented Jan 19, 2021

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

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

Conversation

redbaron commented Dec 9, 2020

gitpod-io bot commented Dec 9, 2020 • edited Loading

redbaron commented Dec 10, 2020

rix0rrr commented Dec 14, 2020 • edited Loading

rix0rrr left a comment

Choose a reason for hiding this comment

redbaron commented Dec 14, 2020 • edited Loading

redbaron commented Dec 23, 2020

rix0rrr commented Jan 6, 2021

rix0rrr commented Jan 7, 2021

redbaron commented Jan 7, 2021

redbaron commented Jan 7, 2021

rix0rrr commented Jan 11, 2021

rix0rrr commented Jan 11, 2021

aws-cdk-automation commented Jan 11, 2021

AWS CodeBuild CI Report

rix0rrr commented Jan 19, 2021

gitpod-io bot commented Dec 9, 2020 •

edited

Loading

rix0rrr commented Dec 14, 2020 •

edited

Loading

redbaron commented Dec 14, 2020 •

edited

Loading