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.

Sign up for GitHub

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 Firestore deletion protection #8906

Merged
merged 6 commits into from
Sep 15, 2023
Merged

Add Firestore deletion protection #8906

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1c95044
Add Firestore deletion protection
IchordeDionysos Sep 10, 2023
cfe45af
Merge branch 'main' into add-firestore-deletion-protection
IchordeDionysos Sep 11, 2023
ca6b7ef
Reformat
IchordeDionysos Sep 11, 2023
429573f
Add test for updating delete_protection_state
IchordeDionysos Sep 12, 2023
bdd9d9b
Make deleteProtectionState skip auto-generated tests
IchordeDionysos Sep 14, 2023
5aac2e0
Add a dedicated example for delete protection
IchordeDionysos Sep 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions mmv1/products/firestore/Database.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,15 @@ properties:
that is returned from the Cloud Datastore APIs in Google App Engine first generation runtimes.
This value may be empty in which case the appid to use for URL-encoded keys is the project_id (eg: foo instead of v~foo).
output: true
- !ruby/object:Api::Type::Enum
name: deleteProtectionState
description: |
State of delete protection for the database.
values:
- :DELETE_PROTECTION_STATE_UNSPECIFIED
- :DELETE_PROTECTION_ENABLED
- :DELETE_PROTECTION_DISABLED
default_from_api: true
- !ruby/object:Api::Type::Fingerprint
name: etag
description: |
Expand Down
1 change: 1 addition & 0 deletions mmv1/templates/terraform/examples/firestore_database.tf.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ resource "google_firestore_database" "<%= ctx[:primary_resource_id] %>" {
concurrency_mode = "OPTIMISTIC"
app_engine_integration_mode = "DISABLED"
point_in_time_recovery_enablement = "POINT_IN_TIME_RECOVERY_ENABLED"
delete_protection_state = "DELETE_PROTECTION_ENABLED"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this cause the database to not be deleted when the test completes?

Copy link
Contributor Author

@IchordeDionysos IchordeDionysos Sep 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will prevent the database from being deletable!

Meaning with delete_protection_state set to DELETE_PROTECTION_ENABLED this request would fail:

DELETE https://firestore.googleapis.com/v1/projects/not-a-project/databases/(default)

While this request would succeed when the delete_protection_state is set to DELETE_PROTECTION_DISABLED.

--

https://cloud.google.com/firestore/docs/manage-databases#delete_protection

It behaves similarly to the Cloud SQL instance deletion protection: https://cloud.google.com/sql/docs/mysql/deletion-protection#rest-v1
Or the Google Cloud Project liens deletion protection: https://cloud.google.com/resource-manager/docs/project-liens

--

Setting, unsettling, or changing this field has no direct impact on the deletion of the database.

So, when you create a database with delete_projection_state set to DELETE_PROTECTION_ENABLED, you can't remove that resource in a single change request/PR.
You first have to set it to DELETE_PROTECTION_DISABLED and then you can remove the resource.

This is useful to prevent accidental deletions of Firestore databases.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I think we'll want to use skip_test to prevent this test from being run and leaving behind databases in our test environment. We should rely on a handwritten test that removes deletion protection before completing so that the test databases can be cleaned up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trodge that makes sense!

I'm wondering. I've already added a custom test that checks if updating works for that field!
https://github.com/GoogleCloudPlatform/magic-modules/pull/8906/files#diff-0f0af92ffe258bbd03d14d44b699ac64106de5d46fed4db170cb2c37ab5a8546R92-R120

And, as I understand it, that test

  • creates a database with that field set
  • verifies if the database was created properly with the delete protection enabled
  • updates the value to disable the delete protection enabled
  • verifies if the delete protection is disabled

So that test should cover those cases already, no?

Is there anything else I should add to the tests?
Besides adding the skip_test flag to the delete_protection_state field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trodge I've added the skip_test flag to the field.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skip_test is an attribute of the example, not the field. We'll want to skip the example test because the update test already covers everything it covers and we don't want to leave orphaned resources.

Copy link
Contributor Author

@IchordeDionysos IchordeDionysos Sep 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trodge sorry, misunderstood you there!

I haven't found documentation about this skip_test here:
https://googlecloudplatform.github.io/magic-modules/develop/resource/
https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/api/type.rb
https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/api/resource.rb

I did not want to skip the tests for firestore_database as a lot of other fields are being tested there.
So I've removed the delete_protection_state field from this standard test and added a custom example (which is being skipped) for the delete protection.

I took the opportunity to describe how to remove a database with delete protection enabled!


depends_on = [google_project_service.firestore]
}