From f8834bc45fcf94b05c0e6f7de2dcf8003e6d7745 Mon Sep 17 00:00:00 2001 From: Sarah French <15078782+SarahFrench@users.noreply.github.com> Date: Wed, 10 Jul 2024 18:41:53 +0100 Subject: [PATCH] Fix permadiff on google_vpc_access_connector resource with breaking changes (min_instances, max_instances, min_throughput, max_throughput fields) (#10313) --- mmv1/products/vpcaccess/Connector.yaml | 15 +- .../examples/vpc_access_connector.tf.erb | 2 + .../vpc_access_connector_shared_vpc.tf.erb | 2 + ...ce_app_engine_standard_app_version_test.go | 2 + ...source_cloudfunctions_function_test.go.erb | 2 + .../data_source_vpc_access_connector_test.go | 8 +- .../resource_vpc_access_connector_test.go | 159 ++++++++++++++++++ .../docs/d/vpc_access_connector.html.markdown | 2 + .../guides/version_6_upgrade.html.markdown | 28 ++- .../data/example_vpc_access_connector.tf | 2 + 10 files changed, 213 insertions(+), 9 deletions(-) diff --git a/mmv1/products/vpcaccess/Connector.yaml b/mmv1/products/vpcaccess/Connector.yaml index b7a68917399c..b5e2fa4b797b 100644 --- a/mmv1/products/vpcaccess/Connector.yaml +++ b/mmv1/products/vpcaccess/Connector.yaml @@ -115,14 +115,18 @@ properties: Minimum throughput of the connector in Mbps. Default and min is 200. Refers to the expected throughput when using an e2-micro machine type. Value must be a multiple of 100 from 200 through 900. Must be lower than the value specified by max_throughput. If both min_throughput and min_instances are provided, min_instances takes precedence over min_throughput. The use of min_throughput is discouraged in favor of min_instances. - default_value: 200 validation: !ruby/object:Provider::Terraform::Validation function: 'validation.IntBetween(200, 1000)' + default_from_api: true + conflicts: + - min_instances - !ruby/object:Api::Type::Integer name: minInstances description: | Minimum value of instances in autoscaling group underlying the connector. Value must be between 2 and 9, inclusive. Must be lower than the value specified by max_instances. + conflicts: + - min_throughput default_from_api: true - !ruby/object:Api::Type::Integer name: maxInstances @@ -130,19 +134,20 @@ properties: Maximum value of instances in autoscaling group underlying the connector. Value must be between 3 and 10, inclusive. Must be higher than the value specified by min_instances. default_from_api: true + conflicts: + - max_throughput - !ruby/object:Api::Type::Integer name: maxThroughput - # The API documentation says this will default to 200, but when I tried that I got an error that the minimum - # throughput must be lower than the maximum. The console defaults to 1000, so I changed it to that. - # API returns 300 if it is not sent description: | Maximum throughput of the connector in Mbps, must be greater than `min_throughput`. Default is 300. Refers to the expected throughput when using an e2-micro machine type. Value must be a multiple of 100 from 300 through 1000. Must be higher than the value specified by min_throughput. If both max_throughput and max_instances are provided, max_instances takes precedence over max_throughput. The use of max_throughput is discouraged in favor of max_instances. - default_value: 300 validation: !ruby/object:Provider::Terraform::Validation function: 'validation.IntBetween(200, 1000)' + default_from_api: true + conflicts: + - max_instances - !ruby/object:Api::Type::String name: 'selfLink' description: | diff --git a/mmv1/templates/terraform/examples/vpc_access_connector.tf.erb b/mmv1/templates/terraform/examples/vpc_access_connector.tf.erb index 46ab6b13120d..57fa413a5a36 100644 --- a/mmv1/templates/terraform/examples/vpc_access_connector.tf.erb +++ b/mmv1/templates/terraform/examples/vpc_access_connector.tf.erb @@ -2,4 +2,6 @@ resource "google_vpc_access_connector" "connector" { name = "<%= ctx[:vars]['name'] %>" ip_cidr_range = "10.8.0.0/28" network = "<%= ctx[:vars]['network_name'] %>" + min_instances = 2 + max_instances = 3 } diff --git a/mmv1/templates/terraform/examples/vpc_access_connector_shared_vpc.tf.erb b/mmv1/templates/terraform/examples/vpc_access_connector_shared_vpc.tf.erb index e2ce01691191..822e5f9eb02d 100644 --- a/mmv1/templates/terraform/examples/vpc_access_connector_shared_vpc.tf.erb +++ b/mmv1/templates/terraform/examples/vpc_access_connector_shared_vpc.tf.erb @@ -4,6 +4,8 @@ resource "google_vpc_access_connector" "connector" { name = google_compute_subnetwork.custom_test.name } machine_type = "e2-standard-4" + min_instances = 2 + max_instances = 3 } resource "google_compute_subnetwork" "custom_test" { diff --git a/mmv1/third_party/terraform/services/appengine/resource_app_engine_standard_app_version_test.go b/mmv1/third_party/terraform/services/appengine/resource_app_engine_standard_app_version_test.go index 024b3d6a5931..fc0d37f19218 100644 --- a/mmv1/third_party/terraform/services/appengine/resource_app_engine_standard_app_version_test.go +++ b/mmv1/third_party/terraform/services/appengine/resource_app_engine_standard_app_version_test.go @@ -189,6 +189,8 @@ resource "google_vpc_access_connector" "bar" { region = "us-central1" ip_cidr_range = "10.8.0.16/28" network = "default" + min_throughput = 200 + max_throughput = 300 } resource "google_app_engine_standard_app_version" "foo" { diff --git a/mmv1/third_party/terraform/services/cloudfunctions/resource_cloudfunctions_function_test.go.erb b/mmv1/third_party/terraform/services/cloudfunctions/resource_cloudfunctions_function_test.go.erb index b63b04ab3b57..d9f068458f10 100644 --- a/mmv1/third_party/terraform/services/cloudfunctions/resource_cloudfunctions_function_test.go.erb +++ b/mmv1/third_party/terraform/services/cloudfunctions/resource_cloudfunctions_function_test.go.erb @@ -985,6 +985,8 @@ resource "google_vpc_access_connector" "%s" { region = "us-central1" ip_cidr_range = "%s" network = google_compute_network.vpc.name + min_throughput = 200 + max_throughput = 300 } resource "google_storage_bucket" "bucket" { diff --git a/mmv1/third_party/terraform/services/vpcaccess/data_source_vpc_access_connector_test.go b/mmv1/third_party/terraform/services/vpcaccess/data_source_vpc_access_connector_test.go index b64da1c8fd1f..9ede70f34d36 100644 --- a/mmv1/third_party/terraform/services/vpcaccess/data_source_vpc_access_connector_test.go +++ b/mmv1/third_party/terraform/services/vpcaccess/data_source_vpc_access_connector_test.go @@ -37,9 +37,11 @@ func testAccVPCAccessConnectorDatasourceConfig(suffix string) string { return fmt.Sprintf(` resource "google_vpc_access_connector" "connector" { name = "tf-test-%s" - ip_cidr_range = "10.8.0.32/28" - network = "default" - region = "us-central1" + ip_cidr_range = "10.8.0.32/28" + network = "default" + region = "us-central1" + min_throughput = 200 + max_throughput = 300 } data "google_vpc_access_connector" "connector" { diff --git a/mmv1/third_party/terraform/services/vpcaccess/resource_vpc_access_connector_test.go b/mmv1/third_party/terraform/services/vpcaccess/resource_vpc_access_connector_test.go index c427072f786a..aea4124ccbf7 100644 --- a/mmv1/third_party/terraform/services/vpcaccess/resource_vpc_access_connector_test.go +++ b/mmv1/third_party/terraform/services/vpcaccess/resource_vpc_access_connector_test.go @@ -1,6 +1,7 @@ package vpcaccess_test import ( + "regexp" "testing" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource" @@ -31,6 +32,81 @@ func TestAccVPCAccessConnector_vpcAccessConnectorThroughput(t *testing.T) { }) } +func TestAccVPCAccessConnector_vpcAccessConnectorThroughput_combiningThroughputAndInstancesFields_conflict(t *testing.T) { + // Need to skip this test as the expected failure happens before the provider interacts with APIs + // In VCR mode this test fails due to lack of cassettes + acctest.SkipIfVcr(t) + t.Parallel() + + context := map[string]interface{}{ + "random_suffix": acctest.RandString(t, 10), + } + + acctest.VcrTest(t, resource.TestCase{ + PreCheck: func() { acctest.AccTestPreCheck(t) }, + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t), + CheckDestroy: testAccCheckVPCAccessConnectorDestroyProducer(t), + Steps: []resource.TestStep{ + { + Config: testAccVPCAccessConnector_vpcAccessConnectorThroughput_bothThroughputAndInstances(context), + // When all 4 of min_instance/max_instance and min_throughput/max_throughput fields are sent to the API + // the API ignores the throughput field values. Instead the API returns values for min and max throughput + // based on the value of min and max instances. The mismatch with the config causes a permadiff. + // Due to this we make the fields conflict with each other. + ExpectError: regexp.MustCompile("conflicts with"), + }, + }, + }) +} + +func TestAccVPCAccessConnector_vpcAccessConnectorThroughput_usingThroughputOrInstancesLimits(t *testing.T) { + t.Parallel() + + context := map[string]interface{}{ + "random_suffix": acctest.RandString(t, 10), + } + + acctest.VcrTest(t, resource.TestCase{ + PreCheck: func() { acctest.AccTestPreCheck(t) }, + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t), + CheckDestroy: testAccCheckVPCAccessConnectorDestroyProducer(t), + Steps: []resource.TestStep{ + { + Config: testAccVPCAccessConnector_vpcAccessConnectorThroughput_justThroughputFields(context), + Check: resource.ComposeTestCheckFunc( + // These fields are set by the config in this test step + resource.TestCheckResourceAttr( + "google_vpc_access_connector.connector", "min_throughput", "400"), + resource.TestCheckResourceAttr( + "google_vpc_access_connector.connector", "max_throughput", "800"), + // These fields aren't set in the config; the API sets and returns values + // based on the thoughput values provided + resource.TestCheckResourceAttr( + "google_vpc_access_connector.connector", "min_instances", "4"), + resource.TestCheckResourceAttr( + "google_vpc_access_connector.connector", "max_instances", "8"), + ), + }, + { + Config: testAccVPCAccessConnector_vpcAccessConnectorThroughput_justInstanceFields(context), + Check: resource.ComposeTestCheckFunc( + // These fields are set by the config in this test step + resource.TestCheckResourceAttr( + "google_vpc_access_connector.connector", "min_instances", "5"), + resource.TestCheckResourceAttr( + "google_vpc_access_connector.connector", "max_instances", "7"), + // These fields aren't set in the config; the API sets and returns values + // based on the instance limit values provided + resource.TestCheckResourceAttr( + "google_vpc_access_connector.connector", "min_throughput", "500"), + resource.TestCheckResourceAttr( + "google_vpc_access_connector.connector", "max_throughput", "700"), + ), + }, + }, + }) +} + func testAccVPCAccessConnector_vpcAccessConnectorThroughput(context map[string]interface{}) string { return acctest.Nprintf(` resource "google_vpc_access_connector" "connector" { @@ -57,3 +133,86 @@ resource "google_compute_network" "custom_test" { } `, context) } + +func testAccVPCAccessConnector_vpcAccessConnectorThroughput_bothThroughputAndInstances(context map[string]interface{}) string { + return acctest.Nprintf(` +resource "google_vpc_access_connector" "connector" { + name = "tf-test-vpc-con%{random_suffix}" + subnet { + name = google_compute_subnetwork.custom_test.name + } + machine_type = "e2-standard-4" + min_instances = 2 + max_instances = 3 + min_throughput = 400 + max_throughput = 1000 + region = "us-central1" +} + +resource "google_compute_subnetwork" "custom_test" { + name = "tf-test-vpc-con%{random_suffix}" + ip_cidr_range = "10.2.0.0/28" + region = "us-central1" + network = google_compute_network.custom_test.id +} + +resource "google_compute_network" "custom_test" { + name = "tf-test-vpc-con%{random_suffix}" + auto_create_subnetworks = false +} +`, context) +} + +func testAccVPCAccessConnector_vpcAccessConnectorThroughput_justInstanceFields(context map[string]interface{}) string { + return acctest.Nprintf(` +resource "google_vpc_access_connector" "connector" { + name = "tf-test-vpc-con%{random_suffix}" + subnet { + name = google_compute_subnetwork.custom_test.name + } + machine_type = "e2-standard-4" + min_instances = 5 + max_instances = 7 + region = "us-central1" +} + +resource "google_compute_subnetwork" "custom_test" { + name = "tf-test-vpc-con%{random_suffix}" + ip_cidr_range = "10.2.0.0/28" + region = "us-central1" + network = google_compute_network.custom_test.id +} + +resource "google_compute_network" "custom_test" { + name = "tf-test-vpc-con%{random_suffix}" + auto_create_subnetworks = false +} +`, context) +} + +func testAccVPCAccessConnector_vpcAccessConnectorThroughput_justThroughputFields(context map[string]interface{}) string { + return acctest.Nprintf(` +resource "google_vpc_access_connector" "connector" { + name = "tf-test-vpc-con%{random_suffix}" + subnet { + name = google_compute_subnetwork.custom_test.name + } + machine_type = "e2-standard-4" + min_throughput = 400 + max_throughput = 800 + region = "us-central1" +} + +resource "google_compute_subnetwork" "custom_test" { + name = "tf-test-vpc-con%{random_suffix}" + ip_cidr_range = "10.2.0.0/28" + region = "us-central1" + network = google_compute_network.custom_test.id +} + +resource "google_compute_network" "custom_test" { + name = "tf-test-vpc-con%{random_suffix}" + auto_create_subnetworks = false +} +`, context) +} diff --git a/mmv1/third_party/terraform/website/docs/d/vpc_access_connector.html.markdown b/mmv1/third_party/terraform/website/docs/d/vpc_access_connector.html.markdown index b107af9a6a55..a44818a7a137 100644 --- a/mmv1/third_party/terraform/website/docs/d/vpc_access_connector.html.markdown +++ b/mmv1/third_party/terraform/website/docs/d/vpc_access_connector.html.markdown @@ -26,6 +26,8 @@ resource "google_vpc_access_connector" "connector" { ip_cidr_range = "10.8.0.0/28" network = "default" region = "us-central1" + min_instances = 2 + max_instances = 3 } ``` diff --git a/mmv1/third_party/terraform/website/docs/guides/version_6_upgrade.html.markdown b/mmv1/third_party/terraform/website/docs/guides/version_6_upgrade.html.markdown index 641c2e5338ea..5b97efe08867 100644 --- a/mmv1/third_party/terraform/website/docs/guides/version_6_upgrade.html.markdown +++ b/mmv1/third_party/terraform/website/docs/guides/version_6_upgrade.html.markdown @@ -118,4 +118,30 @@ Removed in favor of field `settings.ip_configuration.ssl_mode`. ### `schema_settings` no longer has a default value -An empty value means the setting should be cleared. \ No newline at end of file +An empty value means the setting should be cleared. + +## Resource: `google_vpc_access_connector` + +### Fields `min_throughput` and `max_throughput` no longer have default values + +The fields `min_throughput` and `max_throughput` no longer have default values +set by the provider. This was necessary to add conflicting field validation, also +described in this guide. + +No configuration changes are needed for existing resources as these fields' values +will default to values present in data returned from the API. + +### Conflicting field validation added for `min_throughput` and `min_instances`, and `max_throughput` and `max_instances` + +The provider will now enforce that `google_vpc_access_connector` resources can only +include one of `min_throughput` and `min_instances` and one of `max_throughput`and +`max_instances`. Previously if a user included all four fields in a resource block +they would experience a permadiff. This is a result of how `min_instances` and +`max_instances` fields' values take precedence in the API, and how the API calculates +values for `min_throughput` and `max_throughput` that match the number of instances. + +Users will need to check their configuration for any `google_vpc_access_connector` +resource blocks that contain both fields in a conflicting pair, and remove one of those fields. +The fields that are removed from the configuration will still have Computed values, +that are derived from the API. + diff --git a/mmv1/third_party/tgc/tests/data/example_vpc_access_connector.tf b/mmv1/third_party/tgc/tests/data/example_vpc_access_connector.tf index 9281200db1f0..308803bb3db5 100644 --- a/mmv1/third_party/tgc/tests/data/example_vpc_access_connector.tf +++ b/mmv1/third_party/tgc/tests/data/example_vpc_access_connector.tf @@ -31,4 +31,6 @@ resource "google_vpc_access_connector" "connector" { ip_cidr_range = "10.8.0.0/28" network = "default" region = "us-central1" + min_instances = 2 + max_instances = 3 }