From b8c225e1a4a9fd1f41283f57af067502fb6f7745 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Wed, 23 Sep 2020 11:38:30 -0700 Subject: [PATCH 01/11] [compute_instance] - Allow updating of network and subnetwork properties --- .../resource_compute_instance.go.erb | 130 ++++++++---- .../resource_compute_instance_test.go.erb | 192 ++++++++++++++++++ 2 files changed, 280 insertions(+), 42 deletions(-) diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index 559c59ef8cb9..744e21a5319a 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -20,6 +20,7 @@ import ( "github.com/mitchellh/hashstructure" computeBeta "google.golang.org/api/compute/v0.beta" "google.golang.org/api/compute/v1" + "google.golang.org/api/googleapi" ) var ( @@ -233,7 +234,6 @@ func resourceComputeInstance() *schema.Resource { Type: schema.TypeString, Optional: true, Computed: true, - ForceNew: true, DiffSuppressFunc: compareSelfLinkOrResourceName, Description: `The name or self_link of the network attached to this interface.`, }, @@ -242,7 +242,6 @@ func resourceComputeInstance() *schema.Resource { Type: schema.TypeString, Optional: true, Computed: true, - ForceNew: true, DiffSuppressFunc: compareSelfLinkOrResourceName, Description: `The name or self_link of the subnetwork attached to this interface.`, }, @@ -251,7 +250,6 @@ func resourceComputeInstance() *schema.Resource { Type: schema.TypeString, Optional: true, Computed: true, - ForceNew: true, Description: `The project in which the subnetwork belongs.`, }, @@ -1329,21 +1327,50 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err } } - networkInterfacesCount := d.Get("network_interface.#").(int) + networkInterfaces, err := expandNetworkInterfaces(d, config) + if err != nil { + return fmt.Errorf("Error getting network interface from config: %s", err) + } + // Sanity check - if networkInterfacesCount != len(instance.NetworkInterfaces) { + if len(networkInterfaces) != len(instance.NetworkInterfaces) { return fmt.Errorf("Instance had unexpected number of network interfaces: %d", len(instance.NetworkInterfaces)) } - for i := 0; i < networkInterfacesCount; i++ { + + var updatesToNIWhileStopped []func(...googleapi.CallOption) (*computeBeta.Operation, error) + for i := 0; i < len(networkInterfaces); i++ { prefix := fmt.Sprintf("network_interface.%d", i) + networkInterface := networkInterfaces[i] instNetworkInterface := instance.NetworkInterfaces[i] + networkName := d.Get(prefix + ".name").(string) + subnetwork := networkInterface.Subnetwork + updateDurringStop := d.HasChange(prefix+".subnetwork") || d.HasChange(prefix+".network") || d.HasChange(prefix+".subnetwork_project") // Sanity check if networkName != instNetworkInterface.Name { return fmt.Errorf("Instance networkInterface had unexpected name: %s", instNetworkInterface.Name) } + if d.HasChange(prefix + ".subnetwork") { + if !d.HasChange(prefix + ".network") { + subnetProjectField := prefix + ".subnetwork_project" + sf, err := ParseSubnetworkFieldValueWithProjectField(subnetwork, subnetProjectField, d, config) + if err != nil { + return fmt.Errorf("Error, cannot determine self_link for subnetwork %q: %s", subnetwork, err) + } + resp, err := config.clientCompute.Subnetworks.Get(sf.Project, sf.Region, sf.Name).Do() + if err != nil { + return errwrap.Wrapf("Error getting subnetwork value: {{err}}", err) + } + nf, err := ParseNetworkFieldValue(resp.Network, d, config) + if err != nil { + return fmt.Errorf("Error, cannot determine self_link for network %q: %s", resp.Network, err) + } + networkInterface.Network = nf.RelativeLink() + } + } + if d.HasChange(prefix + ".access_config") { // TODO: This code deletes then recreates accessConfigs. This is bad because it may @@ -1389,51 +1416,58 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err return opErr } } - } - - if d.HasChange(prefix + ".alias_ip_range") { - rereadFingerprint := false - // Alias IP ranges cannot be updated; they must be removed and then added. - if len(instNetworkInterface.AliasIpRanges) > 0 { - ni := &computeBeta.NetworkInterface{ - Fingerprint: instNetworkInterface.Fingerprint, - ForceSendFields: []string{"AliasIpRanges"}, - } - op, err := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, ni).Do() - if err != nil { - return errwrap.Wrapf("Error removing alias_ip_range: {{err}}", err) - } - opErr := computeOperationWaitTime(config, op, project, "updating alias ip ranges", d.Timeout(schema.TimeoutUpdate)) - if opErr != nil { - return opErr - } - rereadFingerprint = true + //re-read fingerprint + instance, err = config.clientComputeBeta.Instances.Get(project, zone, instance.Name).Do() + if err != nil { + return err } + instNetworkInterface = instance.NetworkInterfaces[i] + } - ranges := d.Get(prefix + ".alias_ip_range").([]interface{}) - if len(ranges) > 0 { - if rereadFingerprint { + if !updateDurringStop { + if d.HasChange(prefix + ".alias_ip_range") { + // Alias IP ranges cannot be updated; they must be removed and then added. + // unless you are changing subnetwork/network + if len(instNetworkInterface.AliasIpRanges) > 0 { + ni := &computeBeta.NetworkInterface{ + Fingerprint: instNetworkInterface.Fingerprint, + ForceSendFields: []string{"AliasIpRanges"}, + } + op, err := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, ni).Do() + if err != nil { + return errwrap.Wrapf("Error removing alias_ip_range: {{err}}", err) + } + opErr := computeOperationWaitTime(config, op, project, "updating alias ip ranges", d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr + } + //re-read fingerprint instance, err = config.clientComputeBeta.Instances.Get(project, zone, instance.Name).Do() if err != nil { return err } instNetworkInterface = instance.NetworkInterfaces[i] } - ni := &computeBeta.NetworkInterface{ - AliasIpRanges: expandAliasIpRanges(ranges), - Fingerprint: instNetworkInterface.Fingerprint, - } - op, err := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, ni).Do() - if err != nil { - return errwrap.Wrapf("Error adding alias_ip_range: {{err}}", err) - } - opErr := computeOperationWaitTime(config, op, project, "updating alias ip ranges", d.Timeout(schema.TimeoutUpdate)) - if opErr != nil { - return opErr - } } } + + networkInterface.Fingerprint = instNetworkInterface.Fingerprint + networkInterface.NetworkIP = "" + networkInterface.AccessConfigs = nil + updateCall := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterface).Do + if !updateDurringStop && d.HasChange(prefix+".alias_ip_range") { + op, err := updateCall() + if err != nil { + return errwrap.Wrapf("Error updating network interface: {{err}}", err) + } + opErr := computeOperationWaitTime(config, op, project, "network interface to update", d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr + } + } else { + updatesToNIWhileStopped = append(updatesToNIWhileStopped, updateCall) + } } if d.HasChange("attached_disk") { @@ -1558,7 +1592,7 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err } } - needToStopInstanceBeforeUpdating := scopesChange || d.HasChange("service_account.0.email") || d.HasChange("machine_type") || d.HasChange("min_cpu_platform") || d.HasChange("enable_display") || d.HasChange("shielded_instance_config") + needToStopInstanceBeforeUpdating := scopesChange || d.HasChange("service_account.0.email") || d.HasChange("machine_type") || d.HasChange("min_cpu_platform") || d.HasChange("enable_display") || d.HasChange("shielded_instance_config") || len(updatesToNIWhileStopped) > 0 if d.HasChange("desired_status") && !needToStopInstanceBeforeUpdating { desiredStatus := d.Get("desired_status").(string) @@ -1592,7 +1626,8 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err desiredStatus := d.Get("desired_status").(string) if statusBeforeUpdate == "RUNNING" && desiredStatus != "TERMINATED" && !d.Get("allow_stopping_for_update").(bool) { - return fmt.Errorf("Changing the machine_type, min_cpu_platform, service_account, enable_display, or shielded_instance_config on a started instance requires stopping it. " + + return fmt.Errorf("Changing the machine_type, min_cpu_platform, service_account, enable_display, shielded_instance_config, " + + "or network_interface.[#d].(network/subnetwork/subnetwork_project) on a started instance requires stopping it. " + "To acknowledge this, please set allow_stopping_for_update = true in your config. " + "You can also stop it by setting desired_status = \"TERMINATED\", but the instance will not be restarted after the update.") } @@ -1696,6 +1731,17 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err } } + for _, updateCall := range updatesToNIWhileStopped { + op, err := updateCall() + if err != nil { + return errwrap.Wrapf("Error updating network interface: {{err}}", err) + } + opErr := computeOperationWaitTime(config, op, project, "network interface to update", d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr + } + } + if (statusBeforeUpdate == "RUNNING" && desiredStatus != "TERMINATED") || (statusBeforeUpdate == "TERMINATED" && desiredStatus == "RUNNING") { op, err := startInstanceOperation(d, config) diff --git a/third_party/terraform/tests/resource_compute_instance_test.go.erb b/third_party/terraform/tests/resource_compute_instance_test.go.erb index 6a1864e85774..35cfa65361a0 100644 --- a/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -1876,6 +1876,53 @@ func TestAccComputeInstance_resourcePolicyCollocate(t *testing.T) { }) } +func TestAccComputeInstance_subnetworkChange(t *testing.T) { + t.Parallel() + + var instance compute.Instance + instanceName := fmt.Sprintf("tf-test-%s", randString(t, 10)) + suffix := fmt.Sprintf("%s", randString(t, 10)) + subnet := "" + + checkAndUpdateSubnet := func(instance *compute.Instance) resource.TestCheckFunc { + oldSubnetwork := subnet + return func(s *terraform.State) error { + subnet = instance.NetworkInterfaces[0].Subnetwork + if oldSubnetwork != subnet { + return nil + } + + return fmt.Errorf("subnetwork equivelent to previous... expected change; %s : %s", oldSubnetwork, subnet) + } + } + + vcrTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckComputeInstanceDestroyProducer(t), + Steps: []resource.TestStep{ + { + Config: testAccComputeInstance_subnetworkChange(suffix, instanceName), + Check: resource.ComposeTestCheckFunc( + testAccCheckComputeInstanceExists(t, "google_compute_instance.foobar", &instance), + testAccCheckComputeInstanceHasAliasIpRange(&instance, "inst-test-tertiary", "10.1.0.0/20"), + checkAndUpdateSubnet(&instance), + ), + }, + computeInstanceImportStep("us-east1-d", instanceName, []string{"allow_stopping_for_update"}), + { + Config: testAccComputeInstance_subnetworkChangeUpdate(suffix, instanceName), + Check: resource.ComposeTestCheckFunc( + testAccCheckComputeInstanceExists(t, "google_compute_instance.foobar", &instance), + testAccCheckComputeInstanceHasAliasIpRange(&instance, "inst-test-secondary2", "173.16.0.0/24"), + checkAndUpdateSubnet(&instance), + ), + }, + computeInstanceImportStep("us-east1-d", instanceName, []string{"allow_stopping_for_update"}), + }, + }) +} + func testAccCheckComputeInstanceUpdateMachineType(t *testing.T, n string) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[n] @@ -4750,3 +4797,148 @@ resource "google_compute_resource_policy" "foo" { `, instance, instance, suffix) } + +func testAccComputeInstance_subnetworkChange(suffix, instance string) string { + return fmt.Sprintf(` + data "google_compute_image" "my_image" { + family = "debian-9" + project = "debian-cloud" + } + + resource "google_compute_network" "inst-test-network" { + name = "tf-test-network-%s" + } + + resource "google_compute_network" "inst-test-network2" { + name = "tf-test-network2-%s" + } + + resource "google_compute_subnetwork" "inst-test-subnetwork" { + name = "tf-test-compute-subnet-%s" + ip_cidr_range = "10.0.0.0/16" + region = "us-east1" + network = google_compute_network.inst-test-network.self_link + secondary_ip_range { + range_name = "inst-test-secondary" + ip_cidr_range = "172.16.0.0/20" + } + secondary_ip_range { + range_name = "inst-test-tertiary" + ip_cidr_range = "10.1.0.0/16" + } + } + + resource "google_compute_subnetwork" "inst-test-subnetwork2" { + name = "tf-test-compute-subnet2-%s" + ip_cidr_range = "10.3.0.0/16" + region = "us-east1" + network = google_compute_network.inst-test-network2.self_link + secondary_ip_range { + range_name = "inst-test-secondary2" + ip_cidr_range = "173.16.0.0/20" + } + secondary_ip_range { + range_name = "inst-test-tertiary2" + ip_cidr_range = "10.4.0.0/16" + } + } + + resource "google_compute_instance" "foobar" { + name = "%s" + machine_type = "n1-standard-1" + zone = "us-east1-d" + allow_stopping_for_update = true + + boot_disk { + initialize_params { + image = data.google_compute_image.my_image.self_link + } + } + + network_interface { + subnetwork = google_compute_subnetwork.inst-test-subnetwork.self_link + access_config { + network_tier = "STANDARD" + } + alias_ip_range { + subnetwork_range_name = google_compute_subnetwork.inst-test-subnetwork.secondary_ip_range[0].range_name + ip_cidr_range = "172.16.0.0/24" + } + + alias_ip_range { + subnetwork_range_name = google_compute_subnetwork.inst-test-subnetwork.secondary_ip_range[1].range_name + ip_cidr_range = "10.1.0.0/20" + } + } + } +`, suffix, suffix, suffix, suffix, instance) +} + +func testAccComputeInstance_subnetworkChangeUpdate(suffix, instance string) string { + return fmt.Sprintf(` + data "google_compute_image" "my_image" { + family = "debian-9" + project = "debian-cloud" + } + + resource "google_compute_network" "inst-test-network" { + name = "tf-test-network-%s" + } + + resource "google_compute_network" "inst-test-network2" { + name = "tf-test-network2-%s" + } + + resource "google_compute_subnetwork" "inst-test-subnetwork" { + name = "tf-test-compute-subnet-%s" + ip_cidr_range = "10.0.0.0/16" + region = "us-east1" + network = google_compute_network.inst-test-network.self_link + secondary_ip_range { + range_name = "inst-test-secondary" + ip_cidr_range = "172.16.0.0/20" + } + secondary_ip_range { + range_name = "inst-test-tertiary" + ip_cidr_range = "10.1.0.0/16" + } + } + + resource "google_compute_subnetwork" "inst-test-subnetwork2" { + name = "tf-test-compute-subnet2-%s" + ip_cidr_range = "10.3.0.0/16" + region = "us-east1" + network = google_compute_network.inst-test-network2.self_link + secondary_ip_range { + range_name = "inst-test-secondary2" + ip_cidr_range = "173.16.0.0/20" + } + secondary_ip_range { + range_name = "inst-test-tertiary2" + ip_cidr_range = "10.4.0.0/16" + } + } + + resource "google_compute_instance" "foobar" { + name = "%s" + machine_type = "n1-standard-1" + zone = "us-east1-d" + allow_stopping_for_update = true + + boot_disk { + initialize_params { + image = data.google_compute_image.my_image.self_link + } + } + + network_interface { + subnetwork = google_compute_subnetwork.inst-test-subnetwork2.self_link + + alias_ip_range { + subnetwork_range_name = google_compute_subnetwork.inst-test-subnetwork2.secondary_ip_range[0].range_name + ip_cidr_range = "173.16.0.0/24" + } + } + } +`, suffix, suffix, suffix, suffix, instance) +} From aafc4f1b21d9330e493752986917f3c0c374b8d0 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Wed, 23 Sep 2020 12:21:32 -0700 Subject: [PATCH 02/11] fix spelling error --- .../terraform/tests/resource_compute_instance_test.go.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/third_party/terraform/tests/resource_compute_instance_test.go.erb b/third_party/terraform/tests/resource_compute_instance_test.go.erb index 35cfa65361a0..67ca70634259 100644 --- a/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -1892,7 +1892,7 @@ func TestAccComputeInstance_subnetworkChange(t *testing.T) { return nil } - return fmt.Errorf("subnetwork equivelent to previous... expected change; %s : %s", oldSubnetwork, subnet) + return fmt.Errorf("subnetwork equivalent to previous... expected change; %s : %s", oldSubnetwork, subnet) } } From 464c82efbf28231963b5a450cfdd49ce58e712d7 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Wed, 23 Sep 2020 17:13:58 -0700 Subject: [PATCH 03/11] resolve comments --- .../resource_compute_instance.go.erb | 22 ++++++++++----- .../resource_compute_instance_test.go.erb | 27 +++---------------- 2 files changed, 20 insertions(+), 29 deletions(-) diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index 744e21a5319a..21c77f63c1ba 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1345,19 +1345,26 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err networkName := d.Get(prefix + ".name").(string) subnetwork := networkInterface.Subnetwork - updateDurringStop := d.HasChange(prefix+".subnetwork") || d.HasChange(prefix+".network") || d.HasChange(prefix+".subnetwork_project") + updateDuringStop := d.HasChange(prefix+".subnetwork") || d.HasChange(prefix+".network") || d.HasChange(prefix+".subnetwork_project") // Sanity check if networkName != instNetworkInterface.Name { return fmt.Errorf("Instance networkInterface had unexpected name: %s", instNetworkInterface.Name) } + // On creation the network is inferred if only subnetwork is given. + // Unforunately for us there is no way to determine if the user is + // explicitly asigning network or we are reusing the one that was infered + // from state. So here we check if subnetwork changed and network did not. + // In the scenario we assume network was inferred and attempt to figure out + // the new corresponding network. + if d.HasChange(prefix + ".subnetwork") { if !d.HasChange(prefix + ".network") { subnetProjectField := prefix + ".subnetwork_project" sf, err := ParseSubnetworkFieldValueWithProjectField(subnetwork, subnetProjectField, d, config) if err != nil { - return fmt.Errorf("Error, cannot determine self_link for subnetwork %q: %s", subnetwork, err) + return fmt.Errorf("Cannot determine self_link for subnetwork %q: %s", subnetwork, err) } resp, err := config.clientCompute.Subnetworks.Get(sf.Project, sf.Region, sf.Name).Do() if err != nil { @@ -1365,7 +1372,7 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err } nf, err := ParseNetworkFieldValue(resp.Network, d, config) if err != nil { - return fmt.Errorf("Error, cannot determine self_link for network %q: %s", resp.Network, err) + return fmt.Errorf("Cannot determine self_link for network %q: %s", resp.Network, err) } networkInterface.Network = nf.RelativeLink() } @@ -1425,7 +1432,7 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err instNetworkInterface = instance.NetworkInterfaces[i] } - if !updateDurringStop { + if !updateDuringStop { if d.HasChange(prefix + ".alias_ip_range") { // Alias IP ranges cannot be updated; they must be removed and then added. // unless you are changing subnetwork/network @@ -1452,11 +1459,14 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err } } - networkInterface.Fingerprint = instNetworkInterface.Fingerprint + // Setting NetworkIP to empty and AccessConfigs to nil. + // This will opt them out from being modified in the patch call. + // They cannot be changed by UpdateNetworkInterface networkInterface.NetworkIP = "" networkInterface.AccessConfigs = nil + networkInterface.Fingerprint = instNetworkInterface.Fingerprint updateCall := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterface).Do - if !updateDurringStop && d.HasChange(prefix+".alias_ip_range") { + if !updateDuringStop && d.HasChange(prefix+".alias_ip_range") { op, err := updateCall() if err != nil { return errwrap.Wrapf("Error updating network interface: {{err}}", err) diff --git a/third_party/terraform/tests/resource_compute_instance_test.go.erb b/third_party/terraform/tests/resource_compute_instance_test.go.erb index 67ca70634259..f83802ea6eb1 100644 --- a/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -1882,19 +1882,6 @@ func TestAccComputeInstance_subnetworkChange(t *testing.T) { var instance compute.Instance instanceName := fmt.Sprintf("tf-test-%s", randString(t, 10)) suffix := fmt.Sprintf("%s", randString(t, 10)) - subnet := "" - - checkAndUpdateSubnet := func(instance *compute.Instance) resource.TestCheckFunc { - oldSubnetwork := subnet - return func(s *terraform.State) error { - subnet = instance.NetworkInterfaces[0].Subnetwork - if oldSubnetwork != subnet { - return nil - } - - return fmt.Errorf("subnetwork equivalent to previous... expected change; %s : %s", oldSubnetwork, subnet) - } - } vcrTest(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, @@ -1903,20 +1890,10 @@ func TestAccComputeInstance_subnetworkChange(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccComputeInstance_subnetworkChange(suffix, instanceName), - Check: resource.ComposeTestCheckFunc( - testAccCheckComputeInstanceExists(t, "google_compute_instance.foobar", &instance), - testAccCheckComputeInstanceHasAliasIpRange(&instance, "inst-test-tertiary", "10.1.0.0/20"), - checkAndUpdateSubnet(&instance), - ), }, computeInstanceImportStep("us-east1-d", instanceName, []string{"allow_stopping_for_update"}), { Config: testAccComputeInstance_subnetworkChangeUpdate(suffix, instanceName), - Check: resource.ComposeTestCheckFunc( - testAccCheckComputeInstanceExists(t, "google_compute_instance.foobar", &instance), - testAccCheckComputeInstanceHasAliasIpRange(&instance, "inst-test-secondary2", "173.16.0.0/24"), - checkAndUpdateSubnet(&instance), - ), }, computeInstanceImportStep("us-east1-d", instanceName, []string{"allow_stopping_for_update"}), }, @@ -4807,10 +4784,12 @@ func testAccComputeInstance_subnetworkChange(suffix, instance string) string { resource "google_compute_network" "inst-test-network" { name = "tf-test-network-%s" + auto_create_subnetworks = false } resource "google_compute_network" "inst-test-network2" { name = "tf-test-network2-%s" + auto_create_subnetworks = false } resource "google_compute_subnetwork" "inst-test-subnetwork" { @@ -4883,10 +4862,12 @@ func testAccComputeInstance_subnetworkChangeUpdate(suffix, instance string) stri resource "google_compute_network" "inst-test-network" { name = "tf-test-network-%s" + auto_create_subnetworks = false } resource "google_compute_network" "inst-test-network2" { name = "tf-test-network2-%s" + auto_create_subnetworks = false } resource "google_compute_subnetwork" "inst-test-subnetwork" { From f13e905452c47dc3fcf13e9ec8dda56b87c725cf Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Wed, 23 Sep 2020 17:19:02 -0700 Subject: [PATCH 04/11] switch to use id over self link --- .../tests/resource_compute_instance_test.go.erb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/third_party/terraform/tests/resource_compute_instance_test.go.erb b/third_party/terraform/tests/resource_compute_instance_test.go.erb index f83802ea6eb1..735c2f04bb6d 100644 --- a/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -4796,7 +4796,7 @@ func testAccComputeInstance_subnetworkChange(suffix, instance string) string { name = "tf-test-compute-subnet-%s" ip_cidr_range = "10.0.0.0/16" region = "us-east1" - network = google_compute_network.inst-test-network.self_link + network = google_compute_network.inst-test-network.id secondary_ip_range { range_name = "inst-test-secondary" ip_cidr_range = "172.16.0.0/20" @@ -4811,7 +4811,7 @@ func testAccComputeInstance_subnetworkChange(suffix, instance string) string { name = "tf-test-compute-subnet2-%s" ip_cidr_range = "10.3.0.0/16" region = "us-east1" - network = google_compute_network.inst-test-network2.self_link + network = google_compute_network.inst-test-network2.id secondary_ip_range { range_name = "inst-test-secondary2" ip_cidr_range = "173.16.0.0/20" @@ -4830,12 +4830,12 @@ func testAccComputeInstance_subnetworkChange(suffix, instance string) string { boot_disk { initialize_params { - image = data.google_compute_image.my_image.self_link + image = data.google_compute_image.my_image.id } } network_interface { - subnetwork = google_compute_subnetwork.inst-test-subnetwork.self_link + subnetwork = google_compute_subnetwork.inst-test-subnetwork.id access_config { network_tier = "STANDARD" } @@ -4874,7 +4874,7 @@ func testAccComputeInstance_subnetworkChangeUpdate(suffix, instance string) stri name = "tf-test-compute-subnet-%s" ip_cidr_range = "10.0.0.0/16" region = "us-east1" - network = google_compute_network.inst-test-network.self_link + network = google_compute_network.inst-test-network.id secondary_ip_range { range_name = "inst-test-secondary" ip_cidr_range = "172.16.0.0/20" @@ -4889,7 +4889,7 @@ func testAccComputeInstance_subnetworkChangeUpdate(suffix, instance string) stri name = "tf-test-compute-subnet2-%s" ip_cidr_range = "10.3.0.0/16" region = "us-east1" - network = google_compute_network.inst-test-network2.self_link + network = google_compute_network.inst-test-network2.id secondary_ip_range { range_name = "inst-test-secondary2" ip_cidr_range = "173.16.0.0/20" @@ -4908,12 +4908,12 @@ func testAccComputeInstance_subnetworkChangeUpdate(suffix, instance string) stri boot_disk { initialize_params { - image = data.google_compute_image.my_image.self_link + image = data.google_compute_image.my_image.id } } network_interface { - subnetwork = google_compute_subnetwork.inst-test-subnetwork2.self_link + subnetwork = google_compute_subnetwork.inst-test-subnetwork2.id alias_ip_range { subnetwork_range_name = google_compute_subnetwork.inst-test-subnetwork2.secondary_ip_range[0].range_name From 8099c39b43d0c62ba24641ff336b09ee66f273df Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Wed, 23 Sep 2020 17:47:45 -0700 Subject: [PATCH 05/11] remove unused var --- .../terraform/tests/resource_compute_instance_test.go.erb | 2 -- 1 file changed, 2 deletions(-) diff --git a/third_party/terraform/tests/resource_compute_instance_test.go.erb b/third_party/terraform/tests/resource_compute_instance_test.go.erb index 735c2f04bb6d..ac1baad65724 100644 --- a/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -1878,8 +1878,6 @@ func TestAccComputeInstance_resourcePolicyCollocate(t *testing.T) { func TestAccComputeInstance_subnetworkChange(t *testing.T) { t.Parallel() - - var instance compute.Instance instanceName := fmt.Sprintf("tf-test-%s", randString(t, 10)) suffix := fmt.Sprintf("%s", randString(t, 10)) From 4987c92a9891438707a8c921fd43a2b4d78e73a2 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Thu, 24 Sep 2020 11:42:24 -0700 Subject: [PATCH 06/11] resolve comments --- .../resource_compute_instance.go.erb | 59 +++++++++---------- 1 file changed, 27 insertions(+), 32 deletions(-) diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index 21c77f63c1ba..aa9d6f741692 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1352,11 +1352,11 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err return fmt.Errorf("Instance networkInterface had unexpected name: %s", instNetworkInterface.Name) } - // On creation the network is inferred if only subnetwork is given. + // On creation the network is infered if only subnetwork is given. // Unforunately for us there is no way to determine if the user is // explicitly asigning network or we are reusing the one that was infered // from state. So here we check if subnetwork changed and network did not. - // In the scenario we assume network was inferred and attempt to figure out + // In the scenario we assume network was infered and attempt to figure out // the new corresponding network. if d.HasChange(prefix + ".subnetwork") { @@ -1424,7 +1424,7 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err } } - //re-read fingerprint + // re-read fingerprint instance, err = config.clientComputeBeta.Instances.Get(project, zone, instance.Name).Do() if err != nil { return err @@ -1432,41 +1432,36 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err instNetworkInterface = instance.NetworkInterfaces[i] } - if !updateDuringStop { - if d.HasChange(prefix + ".alias_ip_range") { - // Alias IP ranges cannot be updated; they must be removed and then added. - // unless you are changing subnetwork/network - if len(instNetworkInterface.AliasIpRanges) > 0 { - ni := &computeBeta.NetworkInterface{ - Fingerprint: instNetworkInterface.Fingerprint, - ForceSendFields: []string{"AliasIpRanges"}, - } - op, err := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, ni).Do() - if err != nil { - return errwrap.Wrapf("Error removing alias_ip_range: {{err}}", err) - } - opErr := computeOperationWaitTime(config, op, project, "updating alias ip ranges", d.Timeout(schema.TimeoutUpdate)) - if opErr != nil { - return opErr - } - //re-read fingerprint - instance, err = config.clientComputeBeta.Instances.Get(project, zone, instance.Name).Do() - if err != nil { - return err - } - instNetworkInterface = instance.NetworkInterfaces[i] - } - } - } - // Setting NetworkIP to empty and AccessConfigs to nil. // This will opt them out from being modified in the patch call. - // They cannot be changed by UpdateNetworkInterface networkInterface.NetworkIP = "" networkInterface.AccessConfigs = nil networkInterface.Fingerprint = instNetworkInterface.Fingerprint updateCall := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterface).Do - if !updateDuringStop && d.HasChange(prefix+".alias_ip_range") { + + if !updateDuringStop && d.HasChange(prefix + ".alias_ip_range") { + // Alias IP ranges cannot be updated; they must be removed and then added. + // unless you are changing subnetwork/network + if len(instNetworkInterface.AliasIpRanges) > 0 { + ni := &computeBeta.NetworkInterface{ + Fingerprint: instNetworkInterface.Fingerprint, + ForceSendFields: []string{"AliasIpRanges"}, + } + op, err := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, ni).Do() + if err != nil { + return errwrap.Wrapf("Error removing alias_ip_range: {{err}}", err) + } + opErr := computeOperationWaitTime(config, op, project, "updating alias ip ranges", d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr + } + // re-read fingerprint + instance, err = config.clientComputeBeta.Instances.Get(project, zone, instance.Name).Do() + if err != nil { + return err + } + instNetworkInterface = instance.NetworkInterfaces[i] + } op, err := updateCall() if err != nil { return errwrap.Wrapf("Error updating network interface: {{err}}", err) From a1641a116593f3e25134dd374f63e55d34b0fed4 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Thu, 24 Sep 2020 11:51:08 -0700 Subject: [PATCH 07/11] take into account fingerprint refresh before update call. --- .../terraform/resources/resource_compute_instance.go.erb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index aa9d6f741692..8ed3b8ae8e60 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1436,9 +1436,6 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err // This will opt them out from being modified in the patch call. networkInterface.NetworkIP = "" networkInterface.AccessConfigs = nil - networkInterface.Fingerprint = instNetworkInterface.Fingerprint - updateCall := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterface).Do - if !updateDuringStop && d.HasChange(prefix + ".alias_ip_range") { // Alias IP ranges cannot be updated; they must be removed and then added. // unless you are changing subnetwork/network @@ -1462,6 +1459,9 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err } instNetworkInterface = instance.NetworkInterfaces[i] } + + networkInterface.Fingerprint = instNetworkInterface.Fingerprint + updateCall := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterface).Do op, err := updateCall() if err != nil { return errwrap.Wrapf("Error updating network interface: {{err}}", err) @@ -1471,6 +1471,9 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err return opErr } } else { + + networkInterface.Fingerprint = instNetworkInterface.Fingerprint + updateCall := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterface).Do updatesToNIWhileStopped = append(updatesToNIWhileStopped, updateCall) } } From 314005e788b499800df3d129d2b60c81e6f73b90 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Thu, 24 Sep 2020 13:15:33 -0700 Subject: [PATCH 08/11] spelling fix --- .../terraform/resources/resource_compute_instance.go.erb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index 8ed3b8ae8e60..bb6b2f552480 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1352,11 +1352,11 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err return fmt.Errorf("Instance networkInterface had unexpected name: %s", instNetworkInterface.Name) } - // On creation the network is infered if only subnetwork is given. + // On creation the network is inferred if only subnetwork is given. // Unforunately for us there is no way to determine if the user is - // explicitly asigning network or we are reusing the one that was infered + // explicitly asigning network or we are reusing the one that was inferred // from state. So here we check if subnetwork changed and network did not. - // In the scenario we assume network was infered and attempt to figure out + // In the scenario we assume network was inferred and attempt to figure out // the new corresponding network. if d.HasChange(prefix + ".subnetwork") { From d21f704e37844d46b0482b7e3476934cbee97486 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Thu, 24 Sep 2020 18:28:33 -0700 Subject: [PATCH 09/11] Update third_party/terraform/resources/resource_compute_instance.go.erb typo correction Co-authored-by: Cameron Thornton --- .../terraform/resources/resource_compute_instance.go.erb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index bb6b2f552480..4a30654b29a2 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1354,9 +1354,9 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err // On creation the network is inferred if only subnetwork is given. // Unforunately for us there is no way to determine if the user is - // explicitly asigning network or we are reusing the one that was inferred + // explicitly assigning network or we are reusing the one that was inferred // from state. So here we check if subnetwork changed and network did not. - // In the scenario we assume network was inferred and attempt to figure out + // In this scenario we assume network was inferred and attempt to figure out // the new corresponding network. if d.HasChange(prefix + ".subnetwork") { From ebf565dd9380ff7a695cc6d03a1598324268b21b Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Thu, 24 Sep 2020 18:29:09 -0700 Subject: [PATCH 10/11] Update third_party/terraform/resources/resource_compute_instance.go.erb typo correction Co-authored-by: Cameron Thornton --- .../terraform/resources/resource_compute_instance.go.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index 4a30654b29a2..880f78d6bc4e 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1437,7 +1437,7 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err networkInterface.NetworkIP = "" networkInterface.AccessConfigs = nil if !updateDuringStop && d.HasChange(prefix + ".alias_ip_range") { - // Alias IP ranges cannot be updated; they must be removed and then added. + // Alias IP ranges cannot be updated; they must be removed and then added // unless you are changing subnetwork/network if len(instNetworkInterface.AliasIpRanges) > 0 { ni := &computeBeta.NetworkInterface{ From a04588cbee37168df7d9e0a45829213e91ba0ad9 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Thu, 24 Sep 2020 18:50:04 -0700 Subject: [PATCH 11/11] Add comment explaining subnetwork/network relationship. Changed test name --- .../tests/resource_compute_instance_test.go.erb | 10 +++++----- .../website/docs/r/compute_instance.html.markdown | 3 ++- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/third_party/terraform/tests/resource_compute_instance_test.go.erb b/third_party/terraform/tests/resource_compute_instance_test.go.erb index ac1baad65724..f28700b891fb 100644 --- a/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -1876,7 +1876,7 @@ func TestAccComputeInstance_resourcePolicyCollocate(t *testing.T) { }) } -func TestAccComputeInstance_subnetworkChange(t *testing.T) { +func TestAccComputeInstance_subnetworkUpdate(t *testing.T) { t.Parallel() instanceName := fmt.Sprintf("tf-test-%s", randString(t, 10)) suffix := fmt.Sprintf("%s", randString(t, 10)) @@ -1887,11 +1887,11 @@ func TestAccComputeInstance_subnetworkChange(t *testing.T) { CheckDestroy: testAccCheckComputeInstanceDestroyProducer(t), Steps: []resource.TestStep{ { - Config: testAccComputeInstance_subnetworkChange(suffix, instanceName), + Config: testAccComputeInstance_subnetworkUpdate(suffix, instanceName), }, computeInstanceImportStep("us-east1-d", instanceName, []string{"allow_stopping_for_update"}), { - Config: testAccComputeInstance_subnetworkChangeUpdate(suffix, instanceName), + Config: testAccComputeInstance_subnetworkUpdateTwo(suffix, instanceName), }, computeInstanceImportStep("us-east1-d", instanceName, []string{"allow_stopping_for_update"}), }, @@ -4773,7 +4773,7 @@ resource "google_compute_resource_policy" "foo" { `, instance, instance, suffix) } -func testAccComputeInstance_subnetworkChange(suffix, instance string) string { +func testAccComputeInstance_subnetworkUpdate(suffix, instance string) string { return fmt.Sprintf(` data "google_compute_image" "my_image" { family = "debian-9" @@ -4851,7 +4851,7 @@ func testAccComputeInstance_subnetworkChange(suffix, instance string) string { `, suffix, suffix, suffix, suffix, instance) } -func testAccComputeInstance_subnetworkChangeUpdate(suffix, instance string) string { +func testAccComputeInstance_subnetworkUpdateTwo(suffix, instance string) string { return fmt.Sprintf(` data "google_compute_image" "my_image" { family = "debian-9" diff --git a/third_party/terraform/website/docs/r/compute_instance.html.markdown b/third_party/terraform/website/docs/r/compute_instance.html.markdown index bc50ded0df82..6115ecfdaf05 100644 --- a/third_party/terraform/website/docs/r/compute_instance.html.markdown +++ b/third_party/terraform/website/docs/r/compute_instance.html.markdown @@ -248,7 +248,8 @@ The `network_interface` block supports: * `subnetwork` - (Optional) The name or self_link of the subnetwork to attach this interface to. The subnetwork must exist in the same region this instance will be - created in. Either `network` or `subnetwork` must be provided. + created in. If network isn't provided it will be inferred from the subnetwork. + Either `network` or `subnetwork` must be provided. * `subnetwork_project` - (Optional) The project in which the subnetwork belongs. If the `subnetwork` is a self_link, this field is ignored in favor of the project