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

[efs] The default policy retains a resource #9270

Closed
fogfish opened this issue Jul 27, 2020 · 4 comments
Closed

[efs] The default policy retains a resource #9270

fogfish opened this issue Jul 27, 2020 · 4 comments
Assignees
Labels
@aws-cdk/aws-efs Related to Amazon Elastic File System feature-request A feature should be added or improved. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.

Comments

@fogfish
Copy link
Contributor

fogfish commented Jul 27, 2020

EFS uses inconsistent approach (with other modules) on the default removal policy. It retain the resource while other storage related modules removes them (e.g. RDS, S3, ...).

Reproduction Steps

new efs.FileSystem(this, 'Efs', {
      vpc,
      securityGroup: sg,
      vpcSubnets: {subnetType: ec2.SubnetType.PRIVATE}
    })

Environment

  • CLI Version : 1.50
  • Framework Version: 1.50
  • Node.js Version: 12.12
  • Language (Version): TypeScript (3.8.3)
@fogfish fogfish added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 27, 2020
@github-actions github-actions bot added the @aws-cdk/aws-efs Related to Amazon Elastic File System label Jul 27, 2020
@ericzbeard ericzbeard removed the needs-triage This issue or PR still needs to be triaged. label Jul 27, 2020
@ericzbeard ericzbeard removed their assignment Jul 28, 2020
rix0rrr added a commit that referenced this issue Aug 4, 2020
Bug reports typically result from a distinction between a user's mental model and actual behavior.

To bring this to the front and center, I like the phrasing:

* Tell me what you expected to happen
* Tell me what actually happened

More than the current:

* Paste code
* See error

The new phrasing subsumes the old one (what actually happened? I got an error) while also allowing to catch more bug-like scenarios.

Hopefully it will prevent incomplete reports like this: #9270 where the user pasted in the code, didn't get an error so didn't fill out the "error" section, and didn't really state what they expected or saw happen.
@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 4, 2020

Do you mean that you see "RemovalPolicy": "RETAIN" in the final template?

Fairly sure that the default retention period for S3 is also RETAIN, and I think for RDS as well. I think we do this for most stateful resources.

Do you mean there is no way to opt out of the retain behavior?

@rix0rrr rix0rrr added feature-request A feature should be added or improved. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed bug This issue is a bug. labels Aug 4, 2020
@fogfish
Copy link
Contributor Author

fogfish commented Aug 4, 2020

Fortunately, you can opt out the default retain behaviour. What I notice (was under strong impression), that CF removes RDS, S3, etc when resource is created with default retain policy. Only, EFS requires manual clean-up or explicit definition of retain policy.

@rix0rrr
Copy link
Contributor

rix0rrr commented Aug 4, 2020

CF removes RDS, S3,

It does, but CDK changes the default for these resources, for two reasons:

  • Since these are stateful resources, you'd be sad if you accidentally lost them. We want to keep your data safe.
  • S3 buckets can't even be deleted if they are not empty, so deleting a stack with an S3 bucket in it will most likely fail.

As for EFS resources, do you have qualms with the CDK default or the CFN default? Although for consistency's sake, we are most likely to keep the defaults the same as other stateful resources.

@fogfish
Copy link
Contributor Author

fogfish commented Aug 4, 2020

When I said "CF removes" I mean if they are created via CDK.

  • RDS is removed only snapshots remains
  • S3 I strongly believe, the removal attempt is made but it fails if it is not empty like you said.

but now, I see you point. yes, it is CfnFileSystem default one.

I do not think we should do anything about it.

@rix0rrr rix0rrr closed this as completed Aug 4, 2020
mergify bot pushed a commit that referenced this issue Aug 4, 2020
Bug reports typically result from a distinction between a user's mental model and actual behavior.

To bring this to the front and center, I like the phrasing:

* Tell me what you expected to happen
* Tell me what actually happened

More than the current:

* Paste code
* See error

The new phrasing subsumes the old one (what actually happened? I got an error) while also allowing to catch more bug-like scenarios.

Hopefully it will prevent incomplete reports like this: #9270 where the user pasted in the code, didn't get an error so didn't fill out the "error" section, and didn't really state what they expected or saw happen.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
eladb pushed a commit that referenced this issue Aug 10, 2020
Bug reports typically result from a distinction between a user's mental model and actual behavior.

To bring this to the front and center, I like the phrasing:

* Tell me what you expected to happen
* Tell me what actually happened

More than the current:

* Paste code
* See error

The new phrasing subsumes the old one (what actually happened? I got an error) while also allowing to catch more bug-like scenarios.

Hopefully it will prevent incomplete reports like this: #9270 where the user pasted in the code, didn't get an error so didn't fill out the "error" section, and didn't really state what they expected or saw happen.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
curtiseppel pushed a commit to curtiseppel/aws-cdk that referenced this issue Aug 11, 2020
Bug reports typically result from a distinction between a user's mental model and actual behavior.

To bring this to the front and center, I like the phrasing:

* Tell me what you expected to happen
* Tell me what actually happened

More than the current:

* Paste code
* See error

The new phrasing subsumes the old one (what actually happened? I got an error) while also allowing to catch more bug-like scenarios.

Hopefully it will prevent incomplete reports like this: aws#9270 where the user pasted in the code, didn't get an error so didn't fill out the "error" section, and didn't really state what they expected or saw happen.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-efs Related to Amazon Elastic File System feature-request A feature should be added or improved. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days.
Projects
None yet
Development

No branches or pull requests

3 participants