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

Add deletion_protection to google_storage_bucket #7868

Closed
hadim opened this issue Nov 22, 2020 · 15 comments
Closed

Add deletion_protection to google_storage_bucket #7868

hadim opened this issue Nov 22, 2020 · 15 comments

Comments

@hadim
Copy link

hadim commented Nov 22, 2020

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment. If the issue is assigned to the "modular-magician" user, it is either in the process of being autogenerated, or is planned to be autogenerated soon. If the issue is assigned to a user, that user is claiming responsibility for the issue. If the issue is assigned to "hashibot", a community member has claimed the issue already.

Description

When deletion_protection is set to true for a google_sql_database_instance resource, the DB resource is not destroyed with terraform destroy which is an excellent feature. Could we have the same deletion_protection flag for bucket google_storage_bucket?

Unless the same behaviour is already possible?

New or Affected Resource(s)

  • google_storage_bucket

Potential Terraform Configuration

$ tf version
Terraform v0.13.5

References

  • b/275623926
@ghost ghost added the enhancement label Nov 22, 2020
@upodroid
Copy link
Contributor

Something like deletion_protection already exists with the name force_destroy.

The Google API requires the buckets to be empty before they are deleted. Terraform doesn't empty the bucket if force_destroy is not set to true.

As long as you set force_destroy to false or leave it undefined (defaults to false) terraform won't destroy the bucket if it has objects in it.

@hadim
Copy link
Author

hadim commented Nov 23, 2020

Perfect I'll use that then (already used it but wasn't sure if it was similar to deletion_protection).

I also have a similar question to #7869. Would that be possible to return an exit code of 0 without displaying any error when a bucket is not deleted?

@upodroid
Copy link
Contributor

Unlike #7869, the API returns an error if you try to delete a bucket with objects in it. We can't make it exit with a code of 0.

https://cloud.google.com/storage/docs/deleting-buckets#json-api

@hadim
Copy link
Author

hadim commented Nov 23, 2020

So I guess adding the deletion_protection in addition to force_destroy would solve this.

@upodroid
Copy link
Contributor

upodroid commented Nov 23, 2020

I don't think it is needed and I don't understand why you want terraform to exit code 0 when an incorrect configuration is being applied.

I expect a pipeline to break if terraform is deleting a bucket that has force_destroy set to false and it is not empty.

I don't see an issue with deleting an empty bucket and deletion_protection isn't worth it for such situations.

@hadim
Copy link
Author

hadim commented Nov 23, 2020

My use-case is a bit different actually. I want to create a bucket with deletion_protection = true (in that case the value of force_destroy does not really matter). The idea is really the same as for an SQL db.

So when I apply tf destroy the bucket (as for the db) is not deleted and tf destroy will return an exit code of 0 SINCE everything has succeeded.

In short, replace a db instance with a bucket in #7869 and you'll understand my point.

@rileykarson rileykarson added this to the Goals milestone Nov 23, 2020
@rileykarson rileykarson modified the milestones: Goals, Backlog Nov 23, 2020
@rileykarson
Copy link
Collaborator

rileykarson commented Nov 23, 2020

We are a little hesitant the same deletion_protection field here, because we expect that it's more normal to destroy storage buckets than it is to destroy databases. Additionally, force_destroy applies largely the same protection as a deletion_protection field would.

However I'm going to leave this open to collect additional feedback, and in case folks feel strongly otherwise. Also worth reading is hashicorp/terraform#24658.

@hadim
Copy link
Author

hadim commented Nov 23, 2020

Thanks for the feedback.

I don't think my use-case is very special and feel like I am not the only person that needs that flag. I am deploying an application that requires one database to store archived objects and an associated bucket storing the log files for each archived object of the database. I want both to persist while other resources (mostly networks, sa, clusters and compute instances) can be safely deleted during a destroy procedure.

More generally both a database and a bucket are designed to store and persist data so I feel like both should be treated the same way in regard to that.

Also, the deletion_protection flag could be set to false by default so the current behaviour is kept.

Anyway, I think you got my point so I will stop here xD. Thanks for the assistance.

@hadim
Copy link
Author

hadim commented Nov 23, 2020

About hashicorp/terraform#24658, I totally understand that deletion_protection flags are cumbersome to handle on your side and that is something that should be handled by Terraform itself. Hope it will be at some point.

@rileykarson
Copy link
Collaborator

rileykarson commented Nov 23, 2020

deletion_protection defaulting to false ends up largely providing the same protection as lifecycle.prevent_destroy. Could that work for you? And if not, I'm very curious why not.

Additionally, I think that it's useful for all deletion_protection fields in the provider to default true- that avoids users needing to remember which resources default true or default false for resources that support the field.

@hadim
Copy link
Author

hadim commented Nov 23, 2020

Using prevent_destroy is an option too but then the destroy command returns an exit code of 1 (similar to #7869) which is wrong IMHO (at least for that particular case) since, in fact, the command has succeeded. I will report upstream too.

@rileykarson
Copy link
Collaborator

rileykarson commented Nov 23, 2020

Replied inline to #7869 (along with ndmckinley) that Terraform returning a 1 there feels appropriate.

@melinath
Copy link
Collaborator

We now have a more formal policy around deletion protection: https://googlecloudplatform.github.io/magic-modules/docs/best-practices/

We likely wouldn't want to add deletion protection unless we're planning to deprecate force_destroy, which would be a breaking change.

modular-magician added a commit to modular-magician/terraform-provider-google that referenced this issue May 8, 2023
Co-authored-by: Edward Sun <sunedward@google.com>
Signed-off-by: Modular Magician <magic-modules@google.com>
modular-magician added a commit that referenced this issue May 8, 2023
Signed-off-by: Modular Magician <magic-modules@google.com>
Co-authored-by: Edward Sun <sunedward@google.com>
@rileykarson
Copy link
Collaborator

GCS' policy to not allow deleting buckets with data alongside force_destroy acting as an opt-in mechanism to bypass that accomplish the same level of protection as deletion_protection would provide here.

@github-actions
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants