fix: UserPool, Volume, ElasticSearch, FSx are now RETAIN by default #12920

rix0rrr · 2021-02-08T13:29:58Z

(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

… huijbers/removal-policy

gitpod-io · 2021-02-08T13:30:01Z

…licy

PatMyron · 2021-02-08T18:34:50Z

but we weren't providing that option at the L2 level yet, and the L1 API is cumbersome

Is it possible to tackle this specifically? I agree none of the options here are clearly better without downsides, but making it more obvious and less cumbersome to set removalPolicy when CDK hasn't set defaults seems strictly better

packages/@aws-cdk/aws-cognito/lib/user-pool.ts

Co-authored-by: Pat Myron <PatMyron@users.noreply.github.com>

…licy

eladb · 2021-02-09T13:43:50Z

@rix0rrr do we have some kind of enforcement/linting that future stateful resources also have a removal policy?

eladb · 2021-02-09T13:44:17Z

@rix0rrr also - see aws/aws-cdk-rfcs#23 - perhaps would be useful to reference.

rix0rrr · 2021-02-09T14:12:45Z

do we have some kind of enforcement/linting that future stateful resources also have a removal policy?

Not at the moment, because it's not clear where that information should live. I suppose in the doc comments of each L2 is as good a place as any... though I'm not quite sure how linting and static analysis is going to help us verify (for example) default behavior.

What we CAN do is say "if a resource has @stateful, then it MUST accept a removalPolicy?: RemovalPolicy as property."

That's good as far as it goes, but just adding:

class Resource {
  /** get/set */
  public removalPolicy: RemovalPolicy;

  /* or this */
  public applyRemovalPolicy(policy) {
    (this.node.defaultChild as CfnResource).applyRemovalPolicy(policy);
  }
}

Is going to unlock the API for ALL resources, stateful or otherwise. Why wouldn't we do that instead? And if we have this, then the annotation and the forced removalPolicy prop is less urgent.

Also, what is the point of the stateful: boolean property you described in that RFC? Not sure how it would be used

eladb · 2021-02-09T14:24:39Z

Not at the moment, because it's not clear where that information should live.

Where can we obtain a list of stateful resources? Does cfn-lint have that info somewhere?

Also, what is the point of the stateful: boolean property you described in that RFC? Not sure how it would be used

The RFC issue is just a bunch of preliminary ideas. I wouldn't read too much into it. If I recall, the stateful: boolean is intended to allow runtime introspection. I.e. "I want to traverse the tree and apply some policy to all stateful resources."

rix0rrr · 2021-02-09T14:28:52Z

Where can we obtain a list of stateful resources? Does cfn-lint have that info somewhere?

It does. We aren't really set up for this yet, but we could potentially start extending our L1 model with information from external sources such as cfnlint.

The behavior I think would work is that we would mimic the same check cfn-lint would perform during validation: we can emit code for stateful L1s (as marked as such by cfn-lint) to require that they have a removal policy set.

That doesn't force anyhthing about HOW the removal policy is set, but you will encounter this error while developing an L2 (while writing the unit tests for it) and the path of least resistance will be to just add that removalPolicy prop that all the other resources have.

eladb · 2021-02-09T14:39:02Z

@rix0rrr sounds like a step in the right direction!

skinny85

I've only reviewed the RDS part. LGTM.

iliapolo

ElasticSearch
EKS

nija-at · 2021-02-18T11:22:07Z

packages/@aws-cdk/aws-sqs/lib/queue.ts

+ * @default RemovalPolicy.DESTROY
+ */
+ readonly removalPolicy?: RemovalPolicy;


You're sure this is the CF default, yeah?

It is not. I've chosen to make this the default.

nija-at · 2021-02-18T11:26:02Z

packages/@aws-cdk/cfnspec/build-tools/build.ts

+ // eslint-disable-next-line no-console
+ console.log(outFile);


stale from local dev?

nija-at · 2021-02-18T11:26:48Z

packages/@aws-cdk/cfnspec/build-tools/build.ts

+ // eslint-disable-next-line no-console
+ console.log(outFile);


nija-at · 2021-02-18T11:32:40Z

packages/@aws-cdk/cfnspec/spec-source/cfn-lint/StatefulResources/000.json

@@ -0,0 +1,30 @@
+{
+ "ResourceTypes": {


ow, this is a start of a new pattern.

Do we have guarantees from cfnlint about this file spec staying backwards compat? I'd hate for another round of dealing with breaking changes.

Not really, but I don't see any way around that.

nija-at · 2021-02-18T15:34:36Z

packages/@aws-cdk/cfnspec/build-tools/build.ts

+ // Apply the loaded file as a patch onto the current structure
+ patch(structure, data);


minor clarification - this the same patching mechanism as the cfn spec, yeah?

nija-at · 2021-02-18T15:36:03Z

packages/@aws-cdk/cfnspec/build-tools/update-cfnlint.sh

+echo >&2 "Downloading from ${url}..."
+curl -sSfL "${url}" -o ${intermediate}
+
+for file in StatefulResources; do


minor: not familiar with this bash-ism: what is StatefulResources?

A literal string :D

nija-at · 2021-02-18T15:43:45Z

tools/cfn2ts/lib/codegen.ts

+ * ```
+ */
+ private emitConstructValidator(resourceType: genspec.CodeName) {
+ this.code.openBlock('protected validate(): string[]');


Assume you're trying to use the validate API on construct compat layer. This is going away with v2 - https://github.com/aws/aws-cdk-rfcs/blob/master/text/0192-remove-constructs-compat.md#08-validation-the-validate-hook-is-now-nodeaddvalidation

Use addValidation() instead, so we don't have to come back to fix this in v2.

Did that, the types don't like it :(

@aws-cdk/core: �[96mlib/cloudformation.generated.ts�[0m:�[93m883�[0m:�[93m23�[0m - �[91merror�[0m�[90m TS2339: �[0mProperty 'addValidation' does not exist on type 'ConstructNode'.

oh wait. Is this because this doesn't exist on constructs pre-10.x?

It does exist: https://github.com/aws/constructs/blob/117d3f1fee039eeac2d2aefa1b3e2d81530a63ff/src/construct.ts#L415

Use:

Node.of(x).addValidation()

nija-at

Please fix this - #12920 (comment)

Would also like to hear about the cfn-lint guarantee.

Otherwise looks good.

…licy

mergify · 2021-02-22T15:31:14Z

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

aws-cdk-automation · 2021-02-22T16:06:27Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: 4a72864
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2021-02-22T16:07:27Z

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 added 8 commits February 1, 2021 15:49

Cognito User Pool

0b3e755

EC2 Volume

aec4c1b

ElasticSearch Domain

e09ea5e

FSx FileSystem

2fd98f7

WIP SQS

51b4c71

SQS Queue

6810bcf

NestedStack

2753d11

Merge branch 'huijbers/removal-policy' of github.com:aws/aws-cdk into…

e6acca6

… huijbers/removal-policy

rix0rrr requested a review from a team February 8, 2021 13:29

rix0rrr self-assigned this Feb 8, 2021

mergify bot added the contribution/core This is a PR that came from AWS. label Feb 8, 2021

rix0rrr added 3 commits February 8, 2021 14:31

Merge remote-tracking branch 'origin/master' into huijbers/removal-po…

d6d0141

…licy

Update expectation

6a6fbad

Update expectations

8c2e64d

PatMyron reviewed Feb 8, 2021

View reviewed changes

packages/@aws-cdk/aws-cognito/lib/user-pool.ts Outdated Show resolved Hide resolved

rix0rrr and others added 5 commits February 9, 2021 10:20

Update packages/@aws-cdk/aws-cognito/lib/user-pool.ts

45ccb58

Co-authored-by: Pat Myron <PatMyron@users.noreply.github.com>

Merge branch 'master' into huijbers/removal-policy

28c1d68

Update expectations

48b49f6

Merge remote-tracking branch 'origin/master' into huijbers/removal-po…

35d2abb

…licy

Update expectation

f25897b

Update expectations

994d8c3

Update expectations

19a7333

skinny85 approved these changes Feb 17, 2021

View reviewed changes

rix0rrr changed the title ~~fix: add removal policies to stateful resources~~ fix: User Pools, Volumes, ElasticSeach, FSx are now RETAIN by default Feb 18, 2021

rix0rrr changed the title ~~fix: User Pools, Volumes, ElasticSeach, FSx are now RETAIN by default~~ fix: UserPools, Volumes, ElasticSearch, FSx are now RETAIN by default Feb 18, 2021

github-actions bot assigned iliapolo Feb 18, 2021

github-actions bot added @aws-cdk/aws-elasticsearch Related to Amazon Elasticsearch Service @aws-cdk/aws-fsx Related to Amazon FSx labels Feb 18, 2021

rix0rrr changed the title ~~fix: UserPools, Volumes, ElasticSearch, FSx are now RETAIN by default~~ fix: UserPool, Volume, ElasticSearch, FSx are now RETAIN by default Feb 18, 2021

iliapolo approved these changes Feb 18, 2021

View reviewed changes

nija-at reviewed Feb 18, 2021

View reviewed changes

nija-at approved these changes Feb 18, 2021

View reviewed changes

skinny85 mentioned this pull request Feb 18, 2021

RFC 287: CLI deprecated warnings aws/aws-cdk-rfcs#290

Merged

rix0rrr added 4 commits February 19, 2021 13:47

Switch to this.node.addValidation()

c152625

Add construct-compat for addValidation()

b053d49

Make the construct-based validation work

2f09cc3

Thanks eslint

1b0e894

iliapolo removed their assignment Feb 21, 2021

rix0rrr added 3 commits February 22, 2021 11:12

Test tells me compatibility is already ensured! ON to the new way!

d18bf9d

Merge remote-tracking branch 'origin/master' into huijbers/removal-po…

d6e8813

…licy

Update expectations

3edf12a

rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Feb 22, 2021

Merge branch 'master' into huijbers/removal-policy

4a72864

mergify bot merged commit 5a54741 into master Feb 22, 2021

mergify bot deleted the huijbers/removal-policy branch February 22, 2021 16:07

PatMyron mentioned this pull request Feb 22, 2021

refactor: move list of stateful resource types to data file aws-cloudformation/cfn-lint#1897

Merged

rix0rrr mentioned this pull request Jan 6, 2022

1Pager: stateful resources aws/aws-cdk-rfcs#398

Closed

laurelmay mentioned this pull request Sep 8, 2022

(cfnspec): Unable to run bump-cfnspec.sh due to AWS::DynamoDB::Table changes #21957

Closed

colifran mentioned this pull request Jun 17, 2024

aws_kinesis: Stream default removal policy changed in v2.140.0 #30562

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: UserPool, Volume, ElasticSearch, FSx are now RETAIN by default #12920

fix: UserPool, Volume, ElasticSearch, FSx are now RETAIN by default #12920

rix0rrr commented Feb 8, 2021 •

edited

Loading

gitpod-io bot commented Feb 8, 2021 •

edited

Loading

PatMyron commented Feb 8, 2021

eladb commented Feb 9, 2021

eladb commented Feb 9, 2021

rix0rrr commented Feb 9, 2021 •

edited

Loading

eladb commented Feb 9, 2021

rix0rrr commented Feb 9, 2021 •

edited

Loading

eladb commented Feb 9, 2021

skinny85 left a comment

iliapolo left a comment

nija-at Feb 18, 2021

rix0rrr Feb 19, 2021

nija-at Feb 18, 2021

nija-at Feb 18, 2021

nija-at Feb 18, 2021

rix0rrr Feb 19, 2021

nija-at Feb 18, 2021

rix0rrr Feb 19, 2021

nija-at Feb 18, 2021

rix0rrr Feb 19, 2021

nija-at Feb 18, 2021 •

edited

Loading

rix0rrr Feb 19, 2021 •

edited

Loading

nija-at Feb 19, 2021

nija-at Feb 19, 2021

eladb Feb 21, 2021 •

edited

Loading

nija-at left a comment

mergify bot commented Feb 22, 2021

aws-cdk-automation commented Feb 22, 2021

mergify bot commented Feb 22, 2021

		// Apply the loaded file as a patch onto the current structure
		patch(structure, data);

fix: UserPool, Volume, ElasticSearch, FSx are now RETAIN by default #12920

fix: UserPool, Volume, ElasticSearch, FSx are now RETAIN by default #12920

Conversation

rix0rrr commented Feb 8, 2021 • edited Loading

Is RETAIN the correct default?

Can we afford changing the default here?

gitpod-io bot commented Feb 8, 2021 • edited Loading

PatMyron commented Feb 8, 2021

eladb commented Feb 9, 2021

eladb commented Feb 9, 2021

rix0rrr commented Feb 9, 2021 • edited Loading

eladb commented Feb 9, 2021

rix0rrr commented Feb 9, 2021 • edited Loading

eladb commented Feb 9, 2021

skinny85 left a comment

Choose a reason for hiding this comment

iliapolo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nija-at Feb 18, 2021 • edited Loading

Choose a reason for hiding this comment

rix0rrr Feb 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eladb Feb 21, 2021 • edited Loading

Choose a reason for hiding this comment

nija-at left a comment

Choose a reason for hiding this comment

mergify bot commented Feb 22, 2021

aws-cdk-automation commented Feb 22, 2021

AWS CodeBuild CI Report

mergify bot commented Feb 22, 2021

rix0rrr commented Feb 8, 2021 •

edited

Loading

gitpod-io bot commented Feb 8, 2021 •

edited

Loading

rix0rrr commented Feb 9, 2021 •

edited

Loading

rix0rrr commented Feb 9, 2021 •

edited

Loading

nija-at Feb 18, 2021 •

edited

Loading

rix0rrr Feb 19, 2021 •

edited

Loading

eladb Feb 21, 2021 •

edited

Loading