Add delete_protection to SQL Instances + Service Networking Pre Create Checks #3954

upodroid · 2020-09-06T22:15:40Z

Fixes: hashicorp/terraform-provider-google#7072
Fixes: hashicorp/terraform-provider-google#7154

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Generated Terraform, and ran make test and make lint to ensure it passes unit and linter tests.
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Ran relevant acceptance tests (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read the Release Notes Guide before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

Added `deletion_protection` to `google_sql_database_instance`, which defaults to true. SQL instances can no longer be destroyed without setting `deletion_protection = false`.

sql: added a check to `google_sql_database_instance` to catch failures early by seeing if Service Networking Connections already exists for the private network of the instance.

Fork Update

modular-magician · 2020-09-06T22:16:52Z

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@ndmckinley, please review this PR or find an appropriate assignee.

upodroid · 2020-09-06T22:20:56Z

So, tests are failing for the deletion_protection but the service networking check is passing.

 REDACTED  MCW0CDP3YY  ~  go  …  github.com  hashicorp  terraform-provider-google-beta   master  39⬇  5✎  USAGE  $    make testacc TEST=./google-beta TESTARGS='-run=TestAccSqlDatabaseInstance_settings_checkServiceNetworking'
==> Checking source code against gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 TF_SCHEMA_PANIC_ON_ERROR=1 go test ./google-beta -v -run=TestAccSqlDatabaseInstance_settings_checkServiceNetworking -timeout 240m -ldflags="-X=github.com/hashicorp/terraform-provider-google-beta/version.ProviderVersion=acc"
=== RUN   TestAccSqlDatabaseInstance_settings_checkServiceNetworking
=== PAUSE TestAccSqlDatabaseInstance_settings_checkServiceNetworking
=== CONT  TestAccSqlDatabaseInstance_settings_checkServiceNetworking
--- PASS: TestAccSqlDatabaseInstance_settings_checkServiceNetworking (121.22s)
PASS
ok      github.com/hashicorp/terraform-provider-google-beta/google-beta 123.765s

The other test is complaining of non empty plan but my approach is similar to #3450 so not sure what is going on.

make test is also returning a long stack trace that starts with this. Seen it in #3917 as well.

2020/09/06 23:06:11 [TRACE] Waiting 500ms before next try
2020/09/06 23:06:12 [WARN] Composer version didn't match regexp: composer-1.4-airflow-1.9.0
fatal error: concurrent map writes

goroutine 371 [running]:
runtime.throw(0x3d33d64, 0x15)
	/usr/local/Cellar/go/1.14.4/libexec/src/runtime/panic.go:1116 +0x72 fp=0xc000b3eb38 sp=0xc000b3eb08 pc=0x1036672
runtime.mapassign_faststr(0x383c480, 0xc0002fc450, 0x3d754c3, 0x26, 0x5a30740)
	/usr/local/Cellar/go/1.14.4/libexec/src/runtime/map_faststr.go:211 +0x3f7 fp=0xc000b3eba0 sp=0xc000b3eb38 pc=0x1015997

modular-magician · 2020-09-06T22:22:44Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 140 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 3 files changed, 140 insertions(+), 1 deletion(-))

third_party/terraform/resources/resource_sql_database_instance.go.erb

modular-magician · 2020-09-08T21:44:43Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 140 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 3 files changed, 140 insertions(+), 1 deletion(-))

nat-henderson · 2020-09-16T18:03:26Z

Were you able to figure out the make test stacktrace? I'm not getting that on my end.

nat-henderson · 2020-09-16T18:03:38Z

/gcbrun

modular-magician · 2020-09-16T18:11:01Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 140 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 3 files changed, 140 insertions(+), 1 deletion(-))

nat-henderson · 2020-09-16T18:23:41Z

Yeah, still looks good on my end. I'm running the affected integration tests on our CI now.

nat-henderson · 2020-09-16T18:30:52Z

Almost there! You didn't update the regex for your error checker in the integration test:

TestAccSqlDatabaseInstance_settings_checkServiceNetworking: testing.go:667: Step 0, expected error:
errors during apply: Error, failed to create instance because the network doesn't have at least 1 private services connection. Please see https://cloud.google.com/sql/docs/mysql/private-ip#network_requirements for how to create this connection.
To match:
Error, failed to create instance because the network doesn't have atleast 1 private services connections. Please review the docs on how to create this connection.
--- FAIL: TestAccSqlDatabaseInstance_settings_checkServiceNetworking (89.08s)

nat-henderson · 2020-09-16T18:45:35Z

Ah, this also breaks all the other SQL tests, because it introduces a permadiff - if deletion_protection is unset, it will always show a diff.

upodroid · 2020-09-16T18:49:19Z

:( I have updated the regex, any ideas on the permadiff?

modular-magician · 2020-09-16T18:57:09Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 140 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 3 files changed, 140 insertions(+), 1 deletion(-))

nat-henderson · 2020-09-16T19:02:08Z

Aha. I think it's probably an odd manifestation of the fact that you have the type set to TypeString when it should be a boolean.

modular-magician · 2020-09-16T19:16:26Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 140 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 3 files changed, 140 insertions(+), 1 deletion(-))

modular-magician · 2020-09-16T19:33:41Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 3 files changed, 140 insertions(+), 1 deletion(-))
Terraform Beta: Diff ( 3 files changed, 140 insertions(+), 1 deletion(-))

nat-henderson

Looks good, running the tests again now.

nat-henderson · 2020-09-16T23:47:25Z

Okay, all the integration tests still fail with

Test ended in panic.
------- Stdout: -------
=== RUN   TestAccSqlDatabaseInstance_settings_basic
=== PAUSE TestAccSqlDatabaseInstance_settings_basic
=== CONT  TestAccSqlDatabaseInstance_settings_basic
------- Stderr: -------
panic: settings.0.deletion_protection: can only set full list
goroutine 937 [running]:

danawillow

Apologies for the detailed drive-by- I happened to notice this PR and since I wrote the other deletion_protection one, I have some Opinions on what the right approach is :)

third_party/terraform/resources/resource_sql_database_instance.go.erb

danawillow · 2020-09-18T04:24:55Z

third_party/terraform/resources/resource_sql_database_instance.go.erb

@@ -121,6 +121,12 @@ func resourceSqlDatabaseInstance() *schema.Resource {
 							Computed:    true,
 							Description: `Used to make sure changes to the settings block are atomic.`,
 						},
+						"deletion_protection": {
+							Type:        schema.TypeBool,
+							Default:     false,


How useful is this field with a false default? The reason why we added it to bigtable was to prevent people from accidentally destroying their instances because they didn't realize they should do a lifecycle prevent_destroy. If the default is false, wouldn't that just be equivalent to the prevent_destroy behavior?

I didn't consider that. I assumed that the users would set this true if they wanted to avoid terraform deleting their data. I never understood how the OP in 7072 blindly ran a plan that marked the instance for destruction and asked for more guardrails.

I'll set it to true.

In fairness, Google doesn't stop the instance from being deleted if a database already exists on it which is strange considering Google requires dependant resource to be deleted before the parent resource is deleted. ie DNS Records in DNS Zones, VPC Networks, GCE Disk, etc

danawillow · 2020-09-18T04:27:01Z

third_party/terraform/resources/resource_sql_database_instance.go.erb

@@ -650,6 +656,26 @@ func resourceSqlDatabaseInstanceCreate(d *schema.ResourceData, meta interface{})

 	d.Set("name", name)

+	// Before we create the instance, check if at least 1 service connection exists for private SQL Instances.


This change is completely unrelated to the deletion protection one. In the future, changes like these two should be in separate PRs so they can be easily reviewed independently.

third_party/terraform/website/docs/r/sql_database_instance.html.markdown

nat-henderson · 2020-09-21T18:23:55Z

Rerunning the tests now.

nat-henderson · 2020-09-21T19:38:11Z

The following tests still fail:

TestAccSqlUser_mysql
TestAccSqlClientCert_postgres
TestAccSqlUser_postgres
TestAccSqlClientCert_mysql
TestAccSqlDatabase_update
TestAccSqlDatabase_basic

As well as two that are already failing:

TestAccSqlDatabaseInstance_basicInferredName
TestAccSqlDatabaseInstance_PointInTimeRecoveryEnabled

upodroid · 2020-09-21T19:56:38Z

I forgot about the other resources linked to it. I'll push a commit now

upodroid · 2020-09-21T20:54:25Z

I think i got them all

modular-magician · 2020-09-21T21:02:06Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 270 insertions(+), 77 deletions(-))
Terraform Beta: Diff ( 6 files changed, 275 insertions(+), 80 deletions(-))

nat-henderson · 2020-09-23T00:41:55Z

Okay, I will run the tests overnight.

nat-henderson

Tests all pass. I updated the release notes as well.

nat-henderson

Ah, one last thing:

==> Checking source code against linters...
google/resource_sql_database_instance.go:1062:7: Error return value of `d.Set` is not checked (errcheck)
	d.Set("deletion_protection", true)
	     ^
make: *** [lint] Error 1

upodroid · 2020-09-23T18:54:38Z

it is fixed now

modular-magician · 2020-09-23T19:01:50Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 272 insertions(+), 77 deletions(-))
Terraform Beta: Diff ( 6 files changed, 277 insertions(+), 80 deletions(-))

nat-henderson · 2020-10-01T20:08:10Z

@danawillow - are your requested changes made? GH wants all reviewers to approve before submitting - can you approve or dismiss your review if we're good to merge? :)

danawillow

FYI I updated the changelog message to use the correct resource and field names.

third_party/terraform/website/docs/r/sql_database_instance.html.markdown

upodroid · 2020-10-02T21:33:08Z

I think we are ready to go.

modular-magician · 2020-10-02T21:54:28Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 272 insertions(+), 77 deletions(-))
Terraform Beta: Diff ( 6 files changed, 277 insertions(+), 80 deletions(-))

nat-henderson · 2020-10-02T22:13:19Z

Not sure what changed, but make test now reports

# github.com/hashicorp/terraform-provider-google-beta/google-beta
google-beta/resource_sql_database_instance.go:666:76: not enough arguments in call to retrieveServiceNetworkingNetworkName
	have (*schema.ResourceData, *Config, string)
	want (*schema.ResourceData, *Config, string, string)
note: module requires Go 1.14
make: *** [build] Error 2
GNUmakefile:9: recipe for target 'build' failed

upodroid · 2020-10-02T22:26:48Z

something snuck in when i merged master in to the branch, i'll fix it now

… into sql-delete-prot

upodroid · 2020-10-02T22:46:09Z

i think we are good now. 😌

modular-magician · 2020-10-02T22:51:58Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform GA: Diff ( 6 files changed, 273 insertions(+), 77 deletions(-))
Terraform Beta: Diff ( 6 files changed, 278 insertions(+), 80 deletions(-))

upodroid added 4 commits August 3, 2020 12:29

Merge pull request #1 from GoogleCloudPlatform/master

71c3aeb

Fork Update

Merge branch 'master' of github.com:GoogleCloudPlatform/magic-modules

e1a9488

Merge branch 'master' of github.com:GoogleCloudPlatform/magic-modules

51eaff6

add deletion protection

e21f89b

google-cla bot added the cla: yes label Sep 6, 2020

modular-magician requested a review from nat-henderson September 6, 2020 22:16

nat-henderson suggested changes Sep 8, 2020

View reviewed changes

third_party/terraform/resources/resource_sql_database_instance.go.erb Outdated Show resolved Hide resolved

third_party/terraform/resources/resource_sql_database_instance.go.erb Outdated Show resolved Hide resolved

fix typo and add link to docs

b3e34ae

update regex check

72cf93a

set type to bool

f6aec67

nat-henderson approved these changes Sep 16, 2020

View reviewed changes

danawillow suggested changes Sep 18, 2020

View reviewed changes

upodroid added 2 commits September 20, 2020 21:55

rework the deletion protection

9d9bcf4

don't verify deletion_protection field in the state

ea7678f

upodroid added 2 commits September 21, 2020 21:24

Merge branch 'master' into sql-delete-prot

7b47338

fix other broken tests

22e4f5c

nat-henderson approved these changes Sep 23, 2020

View reviewed changes

nat-henderson suggested changes Sep 23, 2020

View reviewed changes

handle errors for imports

44e1cc5

nat-henderson approved these changes Oct 1, 2020

View reviewed changes

danawillow approved these changes Oct 2, 2020

View reviewed changes

third_party/terraform/website/docs/r/sql_database_instance.html.markdown Outdated Show resolved Hide resolved

fix typo in docs

bb1d9b7

upodroid added 2 commits October 2, 2020 23:31

Merge branch 'master' of github.com:GoogleCloudPlatform/magic-modules…

9b2c496

… into sql-delete-prot

add ua arg as part of module attribution refactor

efe7f21

nat-henderson merged commit 869c471 into GoogleCloudPlatform:master Oct 12, 2020

This was referenced Oct 12, 2020

Add delete_protection to SQL Instances + Service Networking Pre Create Checks hashicorp/terraform-provider-google-beta#2579

Merged

Add delete_protection to SQL Instances + Service Networking Pre Create Checks hashicorp/terraform-provider-google#7499

Merged

rileykarson mentioned this pull request Oct 21, 2020

CloudSQL behavior change v3.44.0 hashicorp/terraform-provider-google#7595

Closed

wata727 mentioned this pull request Jan 11, 2023

google_sql_database_instance: cannot import deletion_protection flag hashicorp/terraform-provider-google#13452

Closed

		@@ -650,6 +656,26 @@ func resourceSqlDatabaseInstanceCreate(d *schema.ResourceData, meta interface{})

		d.Set("name", name)

		// Before we create the instance, check if at least 1 service connection exists for private SQL Instances.

Add delete_protection to SQL Instances + Service Networking Pre Create Checks #3954

Add delete_protection to SQL Instances + Service Networking Pre Create Checks #3954

Conversation

upodroid commented Sep 6, 2020 • edited by danawillow Loading

modular-magician commented Sep 6, 2020

upodroid commented Sep 6, 2020

modular-magician commented Sep 6, 2020

Diff report:

modular-magician commented Sep 8, 2020

Diff report:

nat-henderson commented Sep 16, 2020

nat-henderson commented Sep 16, 2020

modular-magician commented Sep 16, 2020

Diff report:

nat-henderson commented Sep 16, 2020

nat-henderson commented Sep 16, 2020

nat-henderson commented Sep 16, 2020

upodroid commented Sep 16, 2020

modular-magician commented Sep 16, 2020

Diff report:

nat-henderson commented Sep 16, 2020

modular-magician commented Sep 16, 2020

Diff report:

modular-magician commented Sep 16, 2020

Diff report:

nat-henderson left a comment

Choose a reason for hiding this comment

nat-henderson commented Sep 16, 2020

danawillow left a comment

Choose a reason for hiding this comment

danawillow Sep 18, 2020

Choose a reason for hiding this comment

upodroid Sep 18, 2020

Choose a reason for hiding this comment

upodroid Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

danawillow Sep 18, 2020

Choose a reason for hiding this comment

nat-henderson commented Sep 21, 2020

nat-henderson commented Sep 21, 2020

upodroid commented Sep 21, 2020

upodroid commented Sep 21, 2020

modular-magician commented Sep 21, 2020

Diff report:

nat-henderson commented Sep 23, 2020

nat-henderson left a comment

Choose a reason for hiding this comment

nat-henderson left a comment

Choose a reason for hiding this comment

upodroid commented Sep 23, 2020

modular-magician commented Sep 23, 2020

Diff report:

nat-henderson commented Oct 1, 2020

danawillow left a comment

Choose a reason for hiding this comment

upodroid commented Oct 2, 2020

modular-magician commented Oct 2, 2020

Diff report:

nat-henderson commented Oct 2, 2020

upodroid commented Oct 2, 2020

upodroid commented Oct 2, 2020

modular-magician commented Oct 2, 2020

Diff report:

upodroid commented Sep 6, 2020 •

edited by danawillow

Loading

upodroid Sep 18, 2020 •

edited

Loading