From 62655488947f243dc38eaf4281167705824272ba Mon Sep 17 00:00:00 2001 From: Modular Magician Date: Fri, 25 Sep 2020 17:09:32 +0000 Subject: [PATCH] [compute_instance] - Allow updating of network and subnetwork properties (#4011) * [compute_instance] - Allow updating of network and subnetwork properties * fix spelling error * resolve comments * switch to use id over self link * remove unused var * resolve comments * take into account fingerprint refresh before update call. * spelling fix * Update third_party/terraform/resources/resource_compute_instance.go.erb typo correction Co-authored-by: Cameron Thornton * Update third_party/terraform/resources/resource_compute_instance.go.erb typo correction Co-authored-by: Cameron Thornton * Add comment explaining subnetwork/network relationship. Changed test name Co-authored-by: Cameron Thornton Signed-off-by: Modular Magician --- .changelog/4011.txt | 3 + google/resource_compute_instance.go | 122 +++++++++---- google/resource_compute_instance_test.go | 171 ++++++++++++++++++ website/docs/r/compute_instance.html.markdown | 3 +- 4 files changed, 264 insertions(+), 35 deletions(-) create mode 100644 .changelog/4011.txt diff --git a/.changelog/4011.txt b/.changelog/4011.txt new file mode 100644 index 00000000000..ccf2d8cf00c --- /dev/null +++ b/.changelog/4011.txt @@ -0,0 +1,3 @@ +```release-note:enhancement +compute: added support for updating `network_interface.[d].network` and `network_interface.[d].subnetwork` properties on `google_compute_instance`. +``` diff --git a/google/resource_compute_instance.go b/google/resource_compute_instance.go index f4a95734b42..b901cb95004 100644 --- a/google/resource_compute_instance.go +++ b/google/resource_compute_instance.go @@ -19,6 +19,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 ( @@ -229,7 +230,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.`, }, @@ -238,7 +238,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.`, }, @@ -247,7 +246,6 @@ func resourceComputeInstance() *schema.Resource { Type: schema.TypeString, Optional: true, Computed: true, - ForceNew: true, Description: `The project in which the subnetwork belongs.`, }, @@ -1291,21 +1289,57 @@ 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 + 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 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 this 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("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("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 @@ -1351,12 +1385,22 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err return opErr } } - } - if d.HasChange(prefix + ".alias_ip_range") { - rereadFingerprint := false + // re-read fingerprint + instance, err = config.clientComputeBeta.Instances.Get(project, zone, instance.Name).Do() + if err != nil { + return err + } + instNetworkInterface = instance.NetworkInterfaces[i] + } - // Alias IP ranges cannot be updated; they must be removed and then added. + // Setting NetworkIP to empty and AccessConfigs to nil. + // This will opt them out from being modified in the patch call. + 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 + // unless you are changing subnetwork/network if len(instNetworkInterface.AliasIpRanges) > 0 { ni := &computeBeta.NetworkInterface{ Fingerprint: instNetworkInterface.Fingerprint, @@ -1370,31 +1414,29 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err if opErr != nil { return opErr } - rereadFingerprint = true - } - - ranges := d.Get(prefix + ".alias_ip_range").([]interface{}) - if len(ranges) > 0 { - if rereadFingerprint { - 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() + // re-read fingerprint + instance, err = config.clientComputeBeta.Instances.Get(project, zone, instance.Name).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 + return 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) } + opErr := computeOperationWaitTime(config, op, project, "network interface to update", d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr + } + } else { + + networkInterface.Fingerprint = instNetworkInterface.Fingerprint + updateCall := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterface).Do + updatesToNIWhileStopped = append(updatesToNIWhileStopped, updateCall) } } @@ -1520,7 +1562,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) @@ -1554,7 +1596,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.") } @@ -1658,6 +1701,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/google/resource_compute_instance_test.go b/google/resource_compute_instance_test.go index 0e8216351f0..a23916d13a3 100644 --- a/google/resource_compute_instance_test.go +++ b/google/resource_compute_instance_test.go @@ -1849,6 +1849,28 @@ func TestAccComputeInstance_resourcePolicyCollocate(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)) + + vcrTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckComputeInstanceDestroyProducer(t), + Steps: []resource.TestStep{ + { + Config: testAccComputeInstance_subnetworkUpdate(suffix, instanceName), + }, + computeInstanceImportStep("us-east1-d", instanceName, []string{"allow_stopping_for_update"}), + { + Config: testAccComputeInstance_subnetworkUpdateTwo(suffix, instanceName), + }, + 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] @@ -4660,3 +4682,152 @@ resource "google_compute_resource_policy" "foo" { `, instance, instance, suffix) } + +func testAccComputeInstance_subnetworkUpdate(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" + 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" { + name = "tf-test-compute-subnet-%s" + ip_cidr_range = "10.0.0.0/16" + region = "us-east1" + network = google_compute_network.inst-test-network.id + 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.id + 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.id + } + } + + network_interface { + subnetwork = google_compute_subnetwork.inst-test-subnetwork.id + 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_subnetworkUpdateTwo(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" + 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" { + name = "tf-test-compute-subnet-%s" + ip_cidr_range = "10.0.0.0/16" + region = "us-east1" + network = google_compute_network.inst-test-network.id + 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.id + 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.id + } + } + + network_interface { + 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 + ip_cidr_range = "173.16.0.0/24" + } + } + } +`, suffix, suffix, suffix, suffix, instance) +} diff --git a/website/docs/r/compute_instance.html.markdown b/website/docs/r/compute_instance.html.markdown index bc50ded0df8..6115ecfdaf0 100644 --- a/website/docs/r/compute_instance.html.markdown +++ b/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