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

feat(bootstrap): customizable bootstrap template #9886

Merged
merged 6 commits into from Aug 31, 2020

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Aug 21, 2020

There are many requests for customization of the built-in bootstrapping
template. Rather than implementing each and every request, it's more
productive to allow users to help themselves.

This change introduces two new flags to cdk bootstrap:

  • cdk bootstrap --show-template: prints the current template to
    stdout, which people can pipe to a file.
  • cdk bootstrap --template FILE: reads the template from a file
    instead of using the built-in template.

This can be used to arbitrarily customize the bootstrapping template
for use in any organization.

I know that the documentation changes in this PR are pretty light,
but really a Developer Guide topic should be written on bootstrapping,
which is next on my TODO list.

Resolves #9256, resolves #8724, resolves #3684, resolves #1528, necessary for #9681.


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

There are many requests for customization of the built-in bootstrapping
template. Rather than implementing each and every request, it's more
productive to allow users to help themselves.

This change introduces two new flags to `cdk bootstrap`:

* `cdk bootstrap --show-template`: prints the current template to
  stdout, which people can pipe to a file.
* `cdk bootstrap --template FILE`: reads the template from a file
  instead of using the built-in template.

This can be used to arbitrarily customize the bootstrapping template
for use in any organization.

I know that the documentation changes in this PR are pretty light,
but really a Developer Guide topic should be written on bootstrapping,
which is next on my TODO list.
@rix0rrr rix0rrr requested a review from a team August 21, 2020 09:54
@rix0rrr rix0rrr self-assigned this Aug 21, 2020
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Aug 21, 2020
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

I've not looked through the code but simply through the commit description and the README.
There are a couple of things that don't feel right about this change.

  1. There are some minimum requirements for a bootstrap stack to be 'valid'.

While I agree that supporting all of the customizations on the CLI is ugly, allowing any yaml as a bootstrap stack feels wrong as well. It can give the perception that the account/region is bootstrapped while it actually isn't.

Can we find some middle ground where customers can customize their stack but still meet some minimum CDK requirements?

  1. Allowing customers to provide an arbitrary template that is not checked in or codified anywhere seems anti to the behaviour we have been encouraging.

This leaves questions open around where the customized bootstrap template will be stored and how it can be guaranteed to be consistent across all of their accounts/regions. One can argue that this is not our responsibility but I think it is. It should be our responsibility to encourage customers do the right thing in terms of how they interact with the CDK and with their AWS accounts via the CDK.

It feels like we should explore better options here. Maybe let customers define the bootstrap stack as a CDK app/stack? If that's not feasible, other options?

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Aug 25, 2020

I have a feeling you're letting perfect be the enemy of good here.

Let's establish first that there is a need for this. We have a slew of requests for juuuuust this one customization of the bootstrap stack, so that my organization is happy. And just one more for this other person. Oh and we have our resources slightly differently. Oh and you're going to maintain and test all these combinations going forward, right?

Furthermore, we always had the plan of providing the template to ops engineers at $BIG_CO, so that they can bootstrap a large number of accounts at the same time using--what's the service again, Control Tower? They will want to take the template in a form that's [currently] familiar to them, inspect it and then roll it out in their organization.

Can we find some middle ground where customers can customize their stack but still meet some minimum CDK requirements?

Sure -- we can add validations on the template. Is that more important than adding the base capability first?

And honestly, many things of the template are customizable on purpose (you just have to tell the Stack Synthesizer about the changes you made). Wouldn't it be great to have a UI or a tool that warned you exactly about the non-standard changes you were making and gave you a piece of code to copy/paste into your code to configure the StackSynthesizer exactly how you wanted it? Sure! Is that really a prerequisite for adding the capability first?

This leaves questions open around where the customized bootstrap template will be stored and how it can be guaranteed to be consistent across all of their accounts/regions.

I'm not really sure what you're implying here. Do we need to force people to only refer to templates stored in git repositories?

Do you want to tie the bootstrap stack to a particular CDK app (and force it to be stored in its repository?). The bootstrap stack can be reused between applications so that wouldn't be a correct modeling.

Maybe let customers define the bootstrap stack as a CDK app/stack?

Yeah, that'd be a good idea. Doesn't REALLY solve the customization issue though. Let's say the bootstrap stack was available as a construct in @aws-cdk/bootstrap-stack. That's good for us, it would be an alternative way to maintain it other than the YAML template. It doesn't really help anyone customize it. Yes, we could take a bunch of parameters, but in effect we'd be in the same situation as we are today, except modeled differently. The problem is not that I don't want to write CloudFormation, the problem is that I don't want to maintain a combinatorial explosion of bootstrapping parameters.

I guess we could think about vending a "Bootstrapping Stack Kit", we could have constructs in there to Build Your Own Bootstrap Stack. When comfortable with the CDK, it would be easier to customize then.

It would also:

  • Not guarantee that people store the result in a git repository, or deploy the same thing everywhere.
  • Be harder to explain how to customize, and to distribute, than just a simple YAML file

I'm not opposed against it, it would be nicely meta, and the features we would build for this would be pretty sweet (insta-deploy a CDK app from NPM into your account would be a cool feature to have!), but I'm not sure it's the most important thing to work on right now.

@rix0rrr rix0rrr dismissed nija-at’s stale review August 25, 2020 08:36

I think you are expecting too much all at once

@rix0rrr rix0rrr requested a review from a team August 25, 2020 08:36
@nija-at
Copy link
Contributor

nija-at commented Aug 26, 2020

Looks like you've made up your mind that this change needs to go through, and anything I say is going to dead end at "Do you have a better solution that can be shipped to customers straight away?".
I'm ok with shipping this sort of solution for now, but something about this still doesn't sit well with me. #disagreeandcommit

However, I still think this is going to introduce more ways for customers to shoot themselves in the foot by bootstrapping 'invalid' templates. Sure, they can adjust the KMS key permissions or bring their own, but should they be able to delete the staging bucket entirely; or ECR repo?

If this does occur, IIUC, the error is going to occur at cdk deploy and I would hate to see issues in my queue because a customer is unable to deploy a CDK app with a lambda function on it because the staging bucket is missing; and it's impossible to debug.

suggestion: a 'validate bootstrap' command on the CLI that ensures that the bootstrap stack meets a minimum bar?

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Aug 26, 2020

We can totally do that. Unfortunately, be aware this is an infeasible problem to solve in generality. We cannot statically check that the user running cdk deploy will have the required permission to all the things it needs to perform the deployment. So we're most likely going to check "some" things because they're easy to check, and that'll be it.

That may catch some things, but it will not catch everything. I am okay with that, with the understanding that the custom bootstrap template is a power user feature and so I am more likely to trust the people that reach for it.

Who knows, I might come to regret this position :).

@njlynch
Copy link
Contributor

njlynch commented Aug 26, 2020

If customers do shoot themselves in the foot here, how might it manifest, and are there any changes in messaging/etc. that we could invest in to make detection or troubleshooting easier?

Copy link
Contributor

@njlynch njlynch left a comment

Choose a reason for hiding this comment

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

Mostly nitpicks in the code itself; not approving otherwise to ensure the "meta" conversation is resolved first.

packages/aws-cdk/README.md Outdated Show resolved Hide resolved
packages/aws-cdk/bin/cdk.ts Outdated Show resolved Hide resolved
packages/aws-cdk/bin/cdk.ts Show resolved Hide resolved
Co-authored-by: Nick Lynch <nlynch@amazon.com>
@njlynch njlynch added the pr/do-not-merge This PR should not be merged at this time. label Aug 27, 2020
Copy link
Contributor

@njlynch njlynch left a comment

Choose a reason for hiding this comment

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

Approving, but marking do-not-merge just in case @nija-at or anyone else wants to make any final comments on the approach itself.

@tomas-mazak
Copy link
Contributor

Hi @rix0rrr , thanks for this one, much appreciated. I actually had to bootstrap personal account to get the template to customize, as in our corporate accounts, the default bootstrap template fails to instantiate due to SCP restrictions. The --show-template option is going to make this so much easier.

However, I was thinking if, in it's current implementation, the cdk bootstrap --template is going to add any value over simply applying that template using CloudFormation directly. Would it make sense to keep the --show-template and drop --template option for now?

More general thought: I would be a bit worried about the "stability" of the interface. What if a new version of CDK will need extra resources in the bootstrap stack... I will need to maintain my template consistency manually. For this reason, I would slightly prefer some way of providing a "patch" to be applied to the template, before instantiating. Random idea: http://jsonpatch.com/

@rix0rrr rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Aug 31, 2020
@mergify
Copy link
Contributor

mergify bot commented Aug 31, 2020

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).

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Aug 31, 2020

I'm sensitive to all of your responses, but I'm going to push this through anyway. Motivation as follows:

  • Though it is true that people could take the template and deploy it themselves, I'm worried that a large portion of our userbase will take this as a non-feature. Today, they could have taken the template from our repository and CFN-deployed that themselves, and yet they don't and choose to come to our bug tracker to ask for additional feature flags. I'm not sure that a "show template" feature only is any better.

  • We could try and do validation on the template, but I honestly am not sure exactly what to validate without being too restrictive. I think I'd rather push this out first and see what kind of traps people are likely to fall into accidentally, and then validate on those.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Aug 31, 2020

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).

@mergify mergify bot merged commit 2596ef7 into master Aug 31, 2020
@mergify mergify bot deleted the huijbers/cli-custom-bootstrap branch August 31, 2020 09:50
@tysoncung
Copy link

cdk bootstrap --show-template > bootstrap-template.yaml
cdk bootstrap --template bootstrap-template.yaml

failed bootstrapping: Error [ValidationError]: Template format error: unsupported structure.
............
  code: 'ValidationError',
  time: 2021-05-24T08:29:11.920Z,
  requestId: '-----------------------------',
  statusCode: 400,
  retryable: false,
  retryDelay: 118.77435400360392
}

Template format error: unsupported structure.

@praveenrengarajan
Copy link

@tysoncung any luck on the validationError of bootstrap template

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
7 participants