aws-cdk
aws-cdk copied to clipboard
aws
feat(docdb): support snapshot removal policy
Adds support for removalPolicy: RemovalPolicy.SNAPSHOT for DocumentDB clusters as specified in the documentation.
Closes #28773. Closes https://github.com/aws/aws-cdk/issues/28861
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license
Exemption Request. The feature is currently not documented. Let me know if we should add some information about it.
@lpizzinidev but is it a good idea to silently fallback to another removal policy here? Personally I find it better to know that something is not supported instead of learning sometime later in production that there is no snapshot although my infrastructure code says there should be.
@kornicameister
From my understanding, the snapshot removal policy is supported for AWS::DocDB::DBCluster.
The fallback to delete is used on AWS::DocDB::DBInstance and AWS::EC2::SecurityGroup resources since they don't support a snapshot removal policy.
The cluster's snapshot is created.
I think this is a valid approach for handling this feature, please let me know if you have suggestions for changing the current implementation.
@lpizzinidev suggestion remains. I would maintain creating security group with defaults which excludes setting up any retention policies inside of a document DB construct.
If user wants to retain w security group it may create one before creating a cluster or an instance. That way, at least, caller is aware of a setting instead of it being magically transformed between incompatible resources.
What it user wishes to remove security group but retain cluster. What is there is a need to remove cluster but retain a group. All in all there's more than one combination and I find it better to let caller decide.
Finally... isn't what you are proposing, potentially dangerous, precedence? I know CDK comes with reasonable defaults but, at least to my knowledge, dealing with security group is always exposed for caller to customize. It is like that here but there is this one extra bit that requires extra documentation.
Are there more places hacking their way through escape hatches like that? Or is it really a precedence?
@lpizzinidev your link leads to RDS.
Also I tried setting up snapshot policy for cluster but cdk synth failed for me with unsupported policy error.
Error: AWS::DocDB::DBCluster does not support snapshot removal policy
@kornicameister If you scroll down you can see the full list of resources supporting a snapshot removal policy. If you synth using the current CDK version it won't work as the list of supported resources is outdated. We can discuss what's the best fallback option for the resources not supporting a snapshot removal policy. My approach is based on the idea that the user probably doesn't want to retain resources when specifying a snapshot policy, so I simply fall back to delete when a snapshot is not possible.
@lpizzinidev I rest my case about it being unsupported...my bad. However that begs a question where is this list and how we can update it. That's a bit of a separate issue I am happy to report.
Nonetheless I still do not agree with this fallback. It still sounds a bit like hidden hack here.
Perhaps it's time to get other people here to express opinion because I am just one guy and it feels wrong just for me which is hardly an argument if general attitude in cdk is to let those hacks happen. In that case I have to live with it 😝.
Long story short such discussion is wonderful idea but I would argue that it should start with general RFC here and handle things globally in sort of unified manner instead of dealing with it here and leaving rest of CDK untouched. Given that is what is happening, because I am not sure of it. Once again a greater number of contributors must express what they think of it.
We can play ping pong all day long...but where does it take us?
@kornicameister Yep, it's up to the core team to discuss the intended behavior, mine is just a proposal. Anyway, I've updated the documentation as this might be confusing for users as well. Thanks for following up with your suggestions.
@GavinZZ
Correct me if wrong, but there's no way to specify RETAIN instance while using SNAPSHOT on cluster.
That's correct. However, I think that if we want to allow users to specify a different policy when the cluster is the to SNAPSHOT, then I think the clearer approach is just providing a separate prop for that. What if there are users who need to apply the DELETE policy in that case?
On there other hand. I haven't checked that but assuming one can restore from snapshot. Should we either handle this here or create separate issue?
@lpizzinidev @GavinZZ ; friendly ping here. I am quite interested in being able to do restoration from snapshot, so bumping up a question.
Based on CloudFormation: https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-docdb-dbcluster.html#cfn-docdb-dbcluster-restoretotime we do have quite a few options to consider.
@kornicameister I would think that as a separate issue and PR.
@lpizzinidev I do agree with you that adding a separate props for the removal policy is the best way to go here. Would you be available to work on it? If not I can help pushing commits to this PR.
@GavinZZ later in the evening I will submit another ticket for passing sourceSnapshot into cluster to make restoration possible.
@lpizzinidev @GavinZZ issue is already created by someone else: #27188.
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).
![mergify[bot] avatar](https://avatars.githubusercontent.com/in/10562?v=4 "Published by a pub.dev verified publisher"){kind=small}
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).
update
❌ Mergify doesn't have permission to update
For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/close-stale-prs.yml without workflows permission
AWS CodeBuild CI Report
- CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
- Commit ID: ec36779d914c826c7d128fa7285ea7813c338513
- Result: SUCCEEDED
- Build Logs (available for 30 days)
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).