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

Conversation

IchordeDionysos
Copy link
Contributor

Adds Firestore deletion protection support to prevent accidental deletion of Firestore databases.

Fixes hashicorp/terraform-provider-google#15789

Release Note Template for Downstream PRs (will be copied)

firestore: added `delete_protection_state` field to `google_firestore_database` resource.

@modular-magician
Copy link
Collaborator

Hello! I am a robot. It looks like you are a: Community Contributor Googler Core Contributor. Tests will require approval to run.

@trodge, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look.

You can help make sure that review is quick by doing a self-review and by running impacted tests locally.

@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Sep 10, 2023
@modular-magician modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Sep 11, 2023
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 41 insertions(+))
Terraform Beta: Diff ( 3 files changed, 41 insertions(+))
TF Conversion: Diff ( 1 file changed, 10 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3026
Passed tests 2726
Skipped tests: 297
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccFirestoreDatabase_firestoreDatabaseExample|TestAccFirestoreDatabase_updateConcurrencyMode|TestAccFirestoreDatabase_updatePitrEnablement

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccFirestoreDatabase_firestoreDatabaseExample[Debug log]
TestAccFirestoreDatabase_updateConcurrencyMode[Debug log]
TestAccFirestoreDatabase_updatePitrEnablement[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

@IchordeDionysos
Copy link
Contributor Author

IchordeDionysos commented Sep 11, 2023

@trodge given that the tests have passed does that mean the PR is good to be merged once the API design and the implementation has been approved?

@@ -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

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

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!

Copy link
Contributor

@trodge trodge left a comment

Choose a reason for hiding this comment

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

Could you also add a test that updates the field?

Update tests are located in mmv1/third_party/terraform/services/firestore/resource_firestore_database_update_test.go.erb

@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Sep 12, 2023
@IchordeDionysos
Copy link
Contributor Author

@trodge I've added a test for updating the field

@modular-magician modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Sep 12, 2023
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 4 files changed, 93 insertions(+))
Terraform Beta: Diff ( 4 files changed, 93 insertions(+))
TF Conversion: Diff ( 1 file changed, 10 insertions(+))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3038
Passed tests 2735
Skipped tests: 297
Affected tests: 6

Action taken

Found 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccFirestoreDatabase_updateDeleteProtectionState|TestAccGKEHub2Scope_gkehubScopeBasicExample|TestAccGKEHub2ScopeIamMemberGenerated|TestAccGKEHub2ScopeIamBindingGenerated|TestAccGKEHub2ScopeIamPolicyGenerated|TestAccDataSourceGoogleServiceAccountAccessToken_basic

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccFirestoreDatabase_updateDeleteProtectionState[Debug log]
TestAccGKEHub2Scope_gkehubScopeBasicExample[Debug log]
TestAccGKEHub2ScopeIamMemberGenerated[Debug log]
TestAccGKEHub2ScopeIamBindingGenerated[Debug log]
TestAccGKEHub2ScopeIamPolicyGenerated[Debug log]
TestAccDataSourceGoogleServiceAccountAccessToken_basic[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

@modular-magician modular-magician added the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Sep 14, 2023
@modular-magician modular-magician removed the awaiting-approval Pull requests that needs reviewer's approval to run presubmit tests label Sep 15, 2023
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 109 insertions(+))
Terraform Beta: Diff ( 3 files changed, 109 insertions(+))
TF Conversion: Diff ( 1 file changed, 10 insertions(+))

Missing test report

Your PR includes resource fields which are not covered by any test.

Resource: google_firestore_database (4 total tests)
Please add an acceptance test which includes these fields. The test should include the following:

resource "google_firestore_database" "primary" {
  delete_protection_state = # value needed
}

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 3060
Passed tests 2760
Skipped tests: 298
Affected tests: 2

Action taken

Found 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
TestAccDataprocClusterIamPolicy|TestAccFirestoreDatabase_firestoreDatabaseExample

Get to know how VCR tests work

@modular-magician
Copy link
Collaborator

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDataprocClusterIamPolicy[Debug log]
TestAccFirestoreDatabase_firestoreDatabaseExample[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$


$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

@trodge trodge merged commit 56aed36 into GoogleCloudPlatform:main Sep 15, 2023
7 checks passed
@IchordeDionysos IchordeDionysos deleted the add-firestore-deletion-protection branch September 16, 2023 14:48
joelkattapuram pushed a commit to joelkattapuram/magic-modules that referenced this pull request Sep 20, 2023
* Add Firestore deletion protection

* Reformat

* Add test for updating delete_protection_state

* Make deleteProtectionState skip auto-generated tests

* Add a dedicated example for delete protection
nevzheng pushed a commit to nevzheng/magic-modules that referenced this pull request Sep 22, 2023
* Add Firestore deletion protection

* Reformat

* Add test for updating delete_protection_state

* Make deleteProtectionState skip auto-generated tests

* Add a dedicated example for delete protection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Firestore: Add ability to configure deletion protection
3 participants