-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix: UserPool, Volume, ElasticSearch, FSx are now RETAIN by default (#…
…12920) (Gave this change an eye-catching title so it would stand out in the change log) `cfn-lint` expects all stateful resources to have a removal policy, but we weren't providing that option at the L2 level yet, and the L1 API is cumbersome. Add the `removalPolicy` option to the following resources: * Cognito User Pools (default: RETAIN) * EC2 Volume (default: RETAIN) * ElasticSearch Domain (default: RETAIN) * FSx FileSystem (default: RETAIN) * SQS Queue (default: DESTROY) * Nested Stack (default: DESTROY) All L2 resources now have an `applyRemovalPolicy` method, so it can be set even for non-stateful resources. A mechanism has been added to the codegen so that new L2 authors cannot forget about the `removalPolicy` when they're writing an L2. I'm aware that the choice to make most of these RETAIN by default is going to be contentious. There are 2 questions here: * Is RETAIN the correct default in general? * Can we afford switching from implicit-DESTROY to implicit-RETAIN? ### Is RETAIN the correct default? I would argue "yes", by process of elimination: Defaulting `removalPolicy` for all resources to `DESTROY` if nothing is given (and writing `DESTROY` to the template) is just going to put us back to the situation in CloudFormation *before* `cfn-lint` existed (your stateful resources are going to be destroyed and nothing is going to warn you about it). Making `removalPolicy` explicitly required for all stateful resources is a backwards-breaking change, and the point about CDK is that it will have sane defaults. I can't come up with a better solution than picking RETAIN as default. ### Can we afford changing the default here? I'm sensitive to the arguments that this a breaking change which we can't afford. It feels like this is the value we would have given the resources had we thought about this earlier, and not being able to correct past mistakes by saying they are "locked in" is going to be a death knell for the project. We could go for a "future behavior" flag, but the risk here seems minimal: this is a change that REDUCES risk of data loss. It might increase risk of deployment errors (duplicate resource names), but that can be manually managed. I will fast-follow with a change that will introduce warnings for resources that have a physical name set and also a RETAIN policy, to clearly identify you're doing something dangerous in CloudFormation. I'm not sure it's worth the effort to go further than that. Although please: discussion welcome. Fixes #12563. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
- Loading branch information
Showing
123 changed files
with
1,120 additions
and
388 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.