Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RFC 66: Self Managed Stack Set Support #357
RFC 66: Self Managed Stack Set Support #357
Changes from all commits
8ad7686
813262b
40cba6a
b5caa51
e320fe0
9e9e3c1
995c36b
7042c9a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please replace "stack sets" (or any variation thereof) with "StackSets" (one word) across this doc. I believe this is the how it is used by the service documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this better than what
CfnStackSet
does?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add some information about what StackSets are (from the official CFN docs) and a reference to the official docs entry point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These roles sounds very much like the ones we use for CDK pipelines. Can't we reuse them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. I'm thinking the same.
If all we need is some slight adjustment to the existing role, then we don't need to change the
cdk bootstrap
command.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so after discussing with Adam, we decided to go the route of introducing new roles:
Stack Set Execution Role
names should be identical in all accounts, which is a requirement for how stack sets operate. The current roles in the bootstrap template are suffixed with specific account idsStack Set Execution Role
also must trust theStack Set Administration Role
, to assume and provision the stack set instance.DeploymentActionRole
, where it only has permissions to to create stack/changeset, and the actual role with large scope of permissions isCloudFormationExecutionRole
which is not assumable to by the trusted accountThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main value of a stack set is to deploy a template to different accounts (and regions).
The usage should talk about this primary use case - how to specify the list of target accounts and regions for this stack set.
And also cover, how to configure parameters that will have different values in the target accounts/regions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there any limitations on StackSets comparing to normal stacks? If there are, this is where they should be listed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our convention is not to use namespaced imports for core types:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to include a bit more involved example (i.e. add a bucket or something)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to 2 space indentation please
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To maintain a good flow for the document, I would put all the information about custom bootstrapping, roles and qualifiers to a separate section at the end "Customizing StackSet Bootstrapping".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this assuming that the execution role will be the same in all accounts? Should it be more flexible than that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this is mentioned just after, but when I read this first it wasn't clear to me that when I first deploy a stack set, it won't deploy any instances. Maybe merge the two sections about "deploy stackSets" and "creating StackSet instances" into a single section about deployment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How exactly? Why not via the cdk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CDK's cloud assembly is a public API. If we're making changes there (to the schema), the working backwards section should include the API changes there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not answering the question. What is the benefit of this change? "Why should I use it?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understand, 'remove instance from stack set' is a very important use case for stack sets.
Without this, we will corner users into a situation where a production stack is bound to a stack set and they lose flexibility.
Will users be able to do this outside of the cdk? If so, we should have a section in the README to talk about this and the impact of an 'out of band' action.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be the experience of running "cdk destroy"? Maybe we can support
cdk destroy
for StackSets that do not have instances?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm mostly repeating myself here, but this doesn't cover why this approach is better than using
CfnStackSet
.We should be talking about this in terms of what we are enabling for users here that didn't previously exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about bootstrap stack changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like assets are not going to be supported anyway
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to say that this does not sound like a good enough reason not to implement this. Migrating from existing infrastructure (either CFN-based or not) is hard in most cases...
What are other reasons for us to support this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Repeat from above: can we use/expand the CDK pipelines roles we already have in the bootstrap stack? Intuitively these should suffice since CDK Pipelines is basically doing the same thing as stack sets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, it would be nice if we didn't need any of this