rfc: `cdk-bootstrap` command by skinny85 · Pull Request #4461 · aws/aws-cdk

skinny85 · 2019-10-10T22:06:00Z

This is an RFC for an enhanced cdk-bootstrap command, as needed by our "Continuous Delivery for CDK apps" story.

Related to #3437

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

mergify · 2019-10-10T22:06:38Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-10-10T22:40:58Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

skinny85 · 2019-10-23T20:56:54Z

@eladb ping

eladb

Please add

design/cdk-bootstrap.md

eladb · 2019-10-24T03:30:32Z

design/cdk-bootstrap.md

+
+* `--kms-key-id`, `-k`: optional identifier of the KMS key used for encrypting the file assets S3 bucket.
+
+These are the new options:


No need to separate old and new

I kind of like it. Makes it easy at a glance to see what is there because of backwards compatibility, and what we're adding.

You can add a section in the end that describes the delta from the current experience.

eladb · 2019-10-24T03:30:51Z

design/cdk-bootstrap.md

+* `--qualifier`, `-q`: an optional qualifier added to the physical names of the bootstrap resources
+  in order to protect against name collisions, especially for things like S3 buckets
+  (which are globally unique in a given AWS partition).
+


--template

Can we not have it in V1?

As we talked, we are not planning to implement a tool in v1, just a template and also not provide any degrees of freedom for names or keys or anything. This is the only customization hook and therefore it’s required for v1

design/cdk-bootstrap.md

eladb · 2019-10-24T03:34:03Z

design/cdk-bootstrap.md

+  in order to protect against name collisions, especially for things like S3 buckets
+  (which are globally unique in a given AWS partition).
+
+## Updating bootstrap resources


This is a really important topic, but we need to figure out a solution that can seamlessly work without our CI/CD story. Needs more thinking.

skinny85 · 2019-10-25T01:12:34Z

Please add

Did you not finish this sentence perhaps...?

eladb

The more I am thinking about this the more I realise that basically cdk-bootstrap is just a CloudFormation template. All options should be expressed as CFN parameters with default values (besides the deployment managed policy name, which should be required). Then, users should be able to deploy these templates in any way they want - either through cdk-bootstrap or through other means like StackSets.

design/cdk-bootstrap.md

skinny85 · 2019-10-26T01:44:46Z

Incorporated the comments, and submitted a new revision.

aws-cdk-automation · 2019-10-26T02:27:22Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

design/cdk-bootstrap.md

skinny85 · 2019-11-02T01:29:09Z

New revision, incorporating the comments.

BTW, I checked, and specifying multiple principals in the assume role policy document works as expected (IAM treats it as an 'OR', not an 'AND').

aws-cdk-automation · 2019-11-02T01:54:33Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb

Please add a backwards compat section (derived from the main CI/CD backwards compat section). Since we should support both OLD and NEW bootstrap stacks, and we are going to remove a bunch of options, I am wondering if we should just create another CLI command cdk bootstrap-v2 or cdk boot for the new bootstrap model. This way we don't have to maintain backwards compat on the API and this allows users to also decide if they want to use the old bootstrap stack or the new based on their use case.

design/cdk-bootstrap.md

skinny85 · 2019-11-05T01:42:17Z

Since we should support both OLD and NEW bootstrap stacks, and we are going to remove a bunch of options, I am wondering if we should just create another CLI command cdk bootstrap-v2 or cdk boot for the new bootstrap model. This way we don't have to maintain backwards compat on the API and this allows users to also decide if they want to use the old bootstrap stack or the new based on their use case.

I really don't like that. We are surfacing to the customers something that is an implementation detail of bootstrapping, and that they don't care about (I don't think anybody wants to learn the differences between the 2 bootstrap commands).

In my opinion, we should actually strive to keep the existing command-line options, and make the transition as transparent to the customer as possible (for example, I would keep the same logical ID of the file assets bucket as the current bootstrap template, so that CFN recognizes them as the same resource).

skinny85 · 2019-11-05T01:44:41Z

Please add a backwards compat section (derived from the main CI/CD backwards compat section).

Done in the newest revision. I definitely need your help in filling out the behavior for some of the rows, so if you could review that in detail, that would be great. Thanks!

aws-cdk-automation · 2019-11-05T02:09:23Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb · 2019-11-05T07:02:25Z

I really don't like that. We are surfacing to the customers something that is an implementation detail of bootstrapping, and that they don't care about (I don't think anybody wants to learn the differences between the 2 bootstrap commands).

I don't see how we can avoid supporting both old and new bootstrap stacks given our compatbility requirements. Do you have a better suggestion?

skinny85 · 2019-11-05T23:09:20Z

I really don't like that. We are surfacing to the customers something that is an implementation detail of bootstrapping, and that they don't care about (I don't think anybody wants to learn the differences between the 2 bootstrap commands).

I don't see how we can avoid supporting both old and new bootstrap stacks given our compatibility requirements. Do you have a better suggestion?

In my opinion, we need to handle a very similar case anyway, so we might as well allow this.

Imagine somebody deployed a custom CloudFormation template for bootstrapping instead of ours, and they gave the file asset bucket a specific name. However, they forgot to override the default settings when creating their Stack classes. Obviously, this is an error, and we should have code in commands like cdk deploy and cdk publish that checks whether the bucket/ECR repository that the assets are supposed land in exist, and print a useful error message if not (for example: we can check whether the name is our "standard" name, and then differentiate between the cases when bootstrap was not run at all, or when bootstrap was customized).

If we have to handle that case anyway, then handling cdk bootstrap --trust 123456789012 --bootstrap-bucket-name my-bucket seems doable.

skinny85 · 2019-11-05T23:18:07Z

Newest revision expands the "Backwards compatibility" section, and makes some changes to the template.

aws-cdk-automation · 2019-11-05T23:48:42Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb

some initial comments

design/cdk-bootstrap.md

eladb · 2019-11-06T15:33:33Z

design/cdk-bootstrap.md

+
+* If the export value is larger than the `BOOTSTRAP_VERSION` constant in the current CLI,
+  that means the bootstrap stack is actually from a later version than the used CLI version.
+  In this case, I think it's correct to proceed with carrying out the operation;


I guess that means that all new versions must be backwards compatible, correct? Do we want semver instead of just a number then?

Sure, that's a good idea.

Actually, I don't know how should that work... what if the bootstrapped environment has version 1.2.0, and the current CLI needs version 2.1.3? What should happen?

Can you outline the cases, and what should be behavior be, in all the cases, like it is now for the monotonically increasing numbers?

skinny85 · 2019-11-22T23:48:28Z

Published a new revision with the latest comments included, @eladb please re-review.

Also, if you could address this comment that I made, that would be great.

aws-cdk-automation · 2019-11-23T00:11:48Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

eladb

I think the RFC is in a good state. There are a few things that we will probably tweak as we dive deeper into the implementation (I am still not 100% sure about how version checks will work in practice)

eladb · 2019-11-24T16:45:56Z

design/cdk-bootstrap.md

+I don't think invoking the full `cdk-bootstrap` tool on every deploy is a good idea, though;
+I worry that calculating a full diff of actual versus desired resource state might impact the performance of commands like
+`deploy` too negatively.


This is also something we cannot do in the CI/CD scenario

mergify · 2019-11-27T13:43:19Z

Thank you for contributing! Your pull request is now being automatically merged.

mergify · 2019-11-27T13:43:19Z

Thank you for contributing! Your pull request is now being automatically merged.

aws-cdk-automation · 2019-11-27T14:07:50Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

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

In the original RFC for the modified cdk bootstrap command done in aws#4461, we removed two options from the cdk bootstrap command-line utility: `--toolkit-bucket-name` and `--bootstrap-kms-key-id`. In this small revision to that RFC, I propose we not remove them to preserve backwards compatibility for customers who already use those options, and I show what changes that would require to our bootstrap CloudFormation template.

…5757) In the original RFC for the modified cdk bootstrap command done in #4461, we removed two options from the cdk bootstrap command-line utility: `--toolkit-bucket-name` and `--bootstrap-kms-key-id`. In this small revision to that RFC, I propose we not remove them to preserve backwards compatibility for customers who already use those options, and I show what changes that would require to our bootstrap CloudFormation template.

skinny85 requested a review from eladb October 10, 2019 22:06

SomayaB assigned shivlaks Oct 11, 2019

SomayaB added the contribution/core This is a PR that came from AWS. label Oct 11, 2019

skinny85 assigned skinny85 and unassigned shivlaks Oct 12, 2019

eladb reviewed Oct 24, 2019

View reviewed changes

eladb suggested changes Oct 25, 2019

View reviewed changes

design/cdk-bootstrap.md Outdated Show resolved Hide resolved

design/cdk-bootstrap.md Show resolved Hide resolved

design/cdk-bootstrap.md Outdated Show resolved Hide resolved

design/cdk-bootstrap.md Show resolved Hide resolved

skinny85 force-pushed the rfc/cdk-bootstrap branch from 5b872da to df50369 Compare October 26, 2019 01:40

eladb reviewed Oct 26, 2019

View reviewed changes

design/cdk-bootstrap.md Show resolved Hide resolved

skinny85 force-pushed the rfc/cdk-bootstrap branch from df50369 to e71588b Compare November 2, 2019 01:28

eladb suggested changes Nov 4, 2019

View reviewed changes

skinny85 force-pushed the rfc/cdk-bootstrap branch from e71588b to ce9ba2d Compare November 5, 2019 01:44

skinny85 force-pushed the rfc/cdk-bootstrap branch from ce9ba2d to fac3160 Compare November 5, 2019 23:17

eladb reviewed Nov 13, 2019

View reviewed changes

eladb added the management/rfc request for comments label Nov 13, 2019

eladb changed the title ~~feat: cdk-bootstrap command design document~~ rfc: cdk-bootstrap command design document Nov 13, 2019

eladb changed the title ~~rfc: cdk-bootstrap command design document~~ rfc: cdk-bootstrap command Nov 13, 2019

skinny85 requested a review from a team November 15, 2019 19:25

skinny85 added 6 commits November 22, 2019 14:25

feat: cdk-bootstrap command design document

fb679f2

Second iteration.

8c20275

Third version.

e3d7e2d

Fourth revision.

a637164

Fifth revision.

57bef19

Sixth revision.

c1cfaa3

skinny85 force-pushed the rfc/cdk-bootstrap branch from fac3160 to c1cfaa3 Compare November 22, 2019 23:47

eladb approved these changes Nov 27, 2019

View reviewed changes

Merge branch 'master' into rfc/cdk-bootstrap

6e7b5e1

mergify bot merged commit e9f282b into aws:master Nov 27, 2019

skinny85 deleted the rfc/cdk-bootstrap branch November 27, 2019 18:09

skinny85 mentioned this pull request Jan 10, 2020

rfc: bring back removed cdk bootstrap CLI options #5757

Merged

eladb mentioned this pull request Jan 15, 2020

new cdk-bootstrap: tag all resources #5809

Closed

2 tasks

skinny85 mentioned this pull request Jan 18, 2020

feat(cli): implementation of the new bootstrap logic #5856

Merged


		* `--kms-key-id`, `-k`: optional identifier of the KMS key used for encrypting the file assets S3 bucket.

		These are the new options:

Conversation

skinny85 commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify bot commented Oct 10, 2019

Uh oh!

aws-cdk-automation commented Oct 10, 2019

AWS CodeBuild CI Report

Uh oh!

skinny85 commented Oct 23, 2019

Uh oh!

eladb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skinny85 commented Oct 25, 2019

Uh oh!

eladb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skinny85 commented Oct 26, 2019

Uh oh!

aws-cdk-automation commented Oct 26, 2019

AWS CodeBuild CI Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skinny85 commented Nov 2, 2019

Uh oh!

aws-cdk-automation commented Nov 2, 2019

AWS CodeBuild CI Report

Uh oh!

eladb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

skinny85 commented Nov 5, 2019

Uh oh!

skinny85 commented Nov 5, 2019

Uh oh!

aws-cdk-automation commented Nov 5, 2019

skinny85 commented Oct 10, 2019 •

edited

Loading