From 8fa2b400ca058480f578c30da61dffc4a8bee856 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Mon, 28 Sep 2020 17:54:08 -0700 Subject: [PATCH 01/25] Compute - Add update support for Network IP when changing network/subnetwork --- .../resource_compute_instance.go.erb | 184 +++++++++++++----- .../resource_compute_instance_test.go.erb | 9 + 2 files changed, 148 insertions(+), 45 deletions(-) diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index f72ea8c61814..1aba1e789243 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -58,6 +58,31 @@ var ( } ) +// network_interface.[d].network_ip can only changge when subnet/network +// is also changing. Validate that if network_ip is changing this scenario +// holds up to par. +func forceNewIfNetworkIPChangedAndNotAble(ctx context.Context, d *schema.ResourceDiff, meta interface{}) error { + oldcount, newcount := d.GetChange("network_interface.#") + if oldcount.(int) != newcount.(int) { + return nil + } + + for i := 0; i < newcount.(int); i++ { + prefix := fmt.Sprintf("network_interface.%d", i) + networkKey := prefix + ".network" + subnetworkKey := prefix + ".subnetwork" + subnetworkProjectKey := prefix + ".subnetwork_project" + networkIPKey := prefix + ".network_ip" + if d.HasChange(networkIPKey) { + if !d.HasChange(networkKey) && !d.HasChange(subnetworkKey) && !d.HasChange(subnetworkProjectKey) { + d.ForceNew(networkIPKey) + } + } + } + + return nil +} + func resourceComputeInstance() *schema.Resource { return &schema.Resource{ Create: resourceComputeInstanceCreate, @@ -256,7 +281,6 @@ func resourceComputeInstance() *schema.Resource { "network_ip": { Type: schema.TypeString, Optional: true, - ForceNew: true, Computed: true, Description: `The private IP address assigned to the instance.`, }, @@ -717,6 +741,7 @@ func resourceComputeInstance() *schema.Resource { suppressEmptyGuestAcceleratorDiff, ), desiredStatusDiff, + forceNewIfNetworkIPChangedAndNotAble, ), } } @@ -1337,7 +1362,7 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err return fmt.Errorf("Instance had unexpected number of network interfaces: %d", len(instance.NetworkInterfaces)) } - var updatesToNIWhileStopped []func(...googleapi.CallOption) (*computeBeta.Operation, error) + var updatesToNIWhileStopped []func(*computeBeta.Instance) error for i := 0; i < len(networkInterfaces); i++ { prefix := fmt.Sprintf("network_interface.%d", i) networkInterface := networkInterfaces[i] @@ -1378,14 +1403,17 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err } } - if d.HasChange(prefix + ".access_config") { - - // TODO: This code deletes then recreates accessConfigs. This is bad because it may - // leave the machine inaccessible from either ip if the creation part fails (network - // timeout etc). However right now there is a GCE limit of 1 accessConfig so it is - // the only way to do it. In future this should be revised to only change what is - // necessary, and also add before removing. + readFingerPrint := func() error { + // re-read fingerprint + instance, err = config.clientComputeBeta.Instances.Get(project, zone, instance.Name).Do() + if err != nil { + return err + } + instNetworkInterface = instance.NetworkInterfaces[i] + return nil + } + deleteAccessConfigs := func() error { // Delete any accessConfig that currently exists in instNetworkInterface for _, ac := range instNetworkInterface.AccessConfigs { op, err := config.clientCompute.Instances.DeleteAccessConfig( @@ -1398,7 +1426,10 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err return opErr } } + return nil + } + createAccessConfigs := func() error { // Create new ones accessConfigsCount := d.Get(prefix + ".access_config.#").(int) for j := 0; j < accessConfigsCount; j++ { @@ -1423,22 +1454,10 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err 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] + return nil } - // 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 + deleteAliasIPRanges := func() error { if len(instNetworkInterface.AliasIpRanges) > 0 { ni := &computeBeta.NetworkInterface{ Fingerprint: instNetworkInterface.Fingerprint, @@ -1452,28 +1471,99 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err if opErr != nil { return opErr } - // re-read fingerprint - instance, err = config.clientComputeBeta.Instances.Get(project, zone, instance.Name).Do() + } + return nil + } + + if !updateDuringStop { + if d.HasChange(prefix + ".access_config") { + // Access configs cannot be updated, they must first be removed + if d.HasChange(prefix + ".access_config") { + err := deleteAccessConfigs() + if err != nil { + return err + } + err = createAccessConfigs() + if err != nil { + return err + } + err = readFingerPrint() + if err != nil { + return err + } + } + } + if d.HasChange(prefix + ".alias_ip_range") { + // Lets be explicit about what we are changing in the patch call + networkInterfacePatchObj := &computeBeta.NetworkInterface{} + + // Alias IP ranges cannot be updated; they must be removed and then added + // unless you are changing subnetwork/network + if d.HasChange(prefix + ".alias_ip_range") { + err := deleteAliasIPRanges() + if err != nil { + return err + } + networkInterfacePatchObj.AliasIpRanges = networkInterface.AliasIpRanges + networkInterfacePatchObj.Fingerprint = instNetworkInterface.Fingerprint + } + + patchCall := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterfacePatchObj).Do + op, err := patchCall() + 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 { + // Lets be explicit about what we are changing in the patch call + networkInterfacePatchObj := &computeBeta.NetworkInterface{ + Network: networkInterface.Network, + Subnetwork: networkInterface.Subnetwork, + AliasIpRanges: networkInterface.AliasIpRanges, + } + + // network_ip can be inferred if not declared. Let's only patch if it's being changed by user + // otherwise this could fail if the network ip is not compatible with the new Subnetwork/Network. + if d.HasChange(prefix + ".network_ip") { + networkInterfacePatchObj.NetworkIP = networkInterface.NetworkIP + } + + // Access config can run into some issues since we can't derive users original intent due to + // terraform limitation. Lets only update it if we need to. + if d.HasChange(prefix + ".access_config") { + err := deleteAccessConfigs() if err != nil { 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 + // Access configs' ip changes when the instance stops invalidating our fingerprint + // expect caller to re-validate instance before calling patch + patchIndex := i + patch := func(instance *computeBeta.Instance) error { + networkInterfacePatchObj.Fingerprint = instance.NetworkInterfaces[patchIndex].Fingerprint + op, err := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterfacePatchObj).Do() + 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 d.HasChange(prefix + ".access_config") { + err := createAccessConfigs() + if err != nil { + return err + } + } + return nil } - } else if updateDuringStop { - networkInterface.Fingerprint = instNetworkInterface.Fingerprint - updateCall := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterface).Do - updatesToNIWhileStopped = append(updatesToNIWhileStopped, updateCall) + + updatesToNIWhileStopped = append(updatesToNIWhileStopped, patch) } } @@ -1738,14 +1828,18 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err } } - for _, updateCall := range updatesToNIWhileStopped { - op, err := updateCall() + // If the instance stops it can invalidate the fingerprint for network interface. + // refresh the instance to get a new fingerprint + if len(updatesToNIWhileStopped) > 0 { + instance, err = config.clientComputeBeta.Instances.Get(project, zone, instance.Name).Do() if err != nil { - return errwrap.Wrapf("Error updating network interface: {{err}}", err) + return err } - opErr := computeOperationWaitTime(config, op, project, "network interface to update", d.Timeout(schema.TimeoutUpdate)) - if opErr != nil { - return opErr + } + for _, patch := range updatesToNIWhileStopped { + err := patch(instance) + if err != nil { + return 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 44967acb3dfd..c542a8951d72 100644 --- a/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -1887,6 +1887,10 @@ func TestAccComputeInstance_subnetworkUpdate(t *testing.T) { Config: testAccComputeInstance_subnetworkUpdateTwo(suffix, instanceName), }, computeInstanceImportStep("us-east1-d", instanceName, []string{"allow_stopping_for_update"}), + { + Config: testAccComputeInstance_subnetworkUpdate(suffix, instanceName), + }, + computeInstanceImportStep("us-east1-d", instanceName, []string{"allow_stopping_for_update"}), }, }) } @@ -4896,6 +4900,11 @@ func testAccComputeInstance_subnetworkUpdateTwo(suffix, instance string) string machine_type = "n1-standard-1" zone = "us-east1-d" allow_stopping_for_update = true + network_ip = "10.3.0.3" + + access_config { + network_tier = "STANDARD" + } boot_disk { initialize_params { From c758f19cf747a0e695ad4d5eba64208104d1fe4e Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Mon, 28 Sep 2020 18:18:12 -0700 Subject: [PATCH 02/25] add field required by beta provider --- .../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 da68fe3be099..fbe9ecc47e15 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1522,7 +1522,7 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err 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)) + opErr := computeOperationWaitTime(config, op, project, "network interface to update", userAgent, d.Timeout(schema.TimeoutUpdate)) if opErr != nil { return opErr } @@ -1559,7 +1559,7 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err 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)) + opErr := computeOperationWaitTime(config, op, project, "network interface to update", userAgent, d.Timeout(schema.TimeoutUpdate)) if opErr != nil { return opErr } From be26563f5b4af5780ea0204770d4180a49b57a5f Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Mon, 28 Sep 2020 18:30:04 -0700 Subject: [PATCH 03/25] error check for d.ForceNew --- .../terraform/resources/resource_compute_instance.go.erb | 4 +++- 1 file changed, 3 insertions(+), 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 fbe9ecc47e15..aa469784c9fb 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -75,7 +75,9 @@ func forceNewIfNetworkIPChangedAndNotAble(ctx context.Context, d *schema.Resourc networkIPKey := prefix + ".network_ip" if d.HasChange(networkIPKey) { if !d.HasChange(networkKey) && !d.HasChange(subnetworkKey) && !d.HasChange(subnetworkProjectKey) { - d.ForceNew(networkIPKey) + if err := d.ForceNew(networkIPKey); err != nil { + return err + } } } } From d43335b6372b2e7cd3cad7b7ef63ca0c91dca3dc Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Tue, 29 Sep 2020 16:28:41 -0700 Subject: [PATCH 04/25] refactor functions into own file. introduce network-interface-helper --- .../resource_compute_instance.go.erb | 199 +++--------------- .../resource_compute_instance_test.go.erb | 10 +- ...pute_instance_network_interface_helpers.go | 184 ++++++++++++++++ 3 files changed, 215 insertions(+), 178 deletions(-) create mode 100644 third_party/terraform/utils/compute_instance_network_interface_helpers.go diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index aa469784c9fb..a5a51aadb394 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -58,16 +58,16 @@ var ( } ) -// network_interface.[d].network_ip can only changge when subnet/network +// network_interface.[d].network_ip can only change when subnet/network // is also changing. Validate that if network_ip is changing this scenario // holds up to par. -func forceNewIfNetworkIPChangedAndNotAble(ctx context.Context, d *schema.ResourceDiff, meta interface{}) error { - oldcount, newcount := d.GetChange("network_interface.#") - if oldcount.(int) != newcount.(int) { +func forceNewIfNetworkIPNotUpdatable(ctx context.Context, d *schema.ResourceDiff, meta interface{}) error { + oldCount, newCount := d.GetChange("network_interface.#") + if oldCount.(int) != newCount.(int) { return nil } - for i := 0; i < newcount.(int); i++ { + for i := 0; i < newCount.(int); i++ { prefix := fmt.Sprintf("network_interface.%d", i) networkKey := prefix + ".network" subnetworkKey := prefix + ".subnetwork" @@ -743,7 +743,7 @@ func resourceComputeInstance() *schema.Resource { suppressEmptyGuestAcceleratorDiff, ), desiredStatusDiff, - forceNewIfNetworkIPChangedAndNotAble, + forceNewIfNetworkIPNotUpdatable, ), } } @@ -1376,16 +1376,10 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err var updatesToNIWhileStopped []func(*computeBeta.Instance) 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) + niH, err := networkInterfaceHelperFactory(d, config, instance, networkInterfaces, i, project, zone, userAgent) + if err != nil { + return err } // On creation the network is inferred if only subnetwork is given. @@ -1397,183 +1391,44 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err 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() + niH.InferNetworkFromSubnetwork() } } - readFingerPrint := func() error { - // re-read fingerprint - instance, err = config.clientComputeBeta.Instances.Get(project, zone, instance.Name).Do() - if err != nil { - return err - } - instNetworkInterface = instance.NetworkInterfaces[i] - return nil - } - - deleteAccessConfigs := func() error { - // Delete any accessConfig that currently exists in instNetworkInterface - for _, ac := range instNetworkInterface.AccessConfigs { - op, err := config.clientCompute.Instances.DeleteAccessConfig( - project, zone, instance.Name, ac.Name, networkName).Do() + if !updateDuringStop { + if d.HasChange(prefix + ".access_config") { + // Access configs cannot be updated, they must first be removed + err := niH.DeleteAccessConfigs() if err != nil { - return fmt.Errorf("Error deleting old access_config: %s", err) - } - opErr := computeOperationWaitTime(config, op, project, "old access_config to delete", userAgent, d.Timeout(schema.TimeoutUpdate)) - if opErr != nil { - return opErr - } - } - return nil - } - - createAccessConfigs := func() error { - // Create new ones - accessConfigsCount := d.Get(prefix + ".access_config.#").(int) - for j := 0; j < accessConfigsCount; j++ { - acPrefix := fmt.Sprintf("%s.access_config.%d", prefix, j) - ac := &computeBeta.AccessConfig{ - Type: "ONE_TO_ONE_NAT", - NatIP: d.Get(acPrefix + ".nat_ip").(string), - NetworkTier: d.Get(acPrefix + ".network_tier").(string), - } - if ptr, ok := d.GetOk(acPrefix + ".public_ptr_domain_name"); ok && ptr != "" { - ac.SetPublicPtr = true - ac.PublicPtrDomainName = ptr.(string) + return err } - - op, err := config.clientComputeBeta.Instances.AddAccessConfig( - project, zone, instance.Name, networkName, ac).Do() + err = niH.CreateAccessConfigs() if err != nil { - return fmt.Errorf("Error adding new access_config: %s", err) - } - opErr := computeOperationWaitTime(config, op, project, "new access_config to add", userAgent, d.Timeout(schema.TimeoutUpdate)) - if opErr != nil { - return opErr - } - } - return nil - } - - deleteAliasIPRanges := func() error { - if len(instNetworkInterface.AliasIpRanges) > 0 { - ni := &computeBeta.NetworkInterface{ - Fingerprint: instNetworkInterface.Fingerprint, - ForceSendFields: []string{"AliasIpRanges"}, + return err } - op, err := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, ni).Do() + // refresh the instance to get an up to date fingerpint for subsequent changes + err = niH.RefreshInstance() if err != nil { - return errwrap.Wrapf("Error removing alias_ip_range: {{err}}", err) - } - opErr := computeOperationWaitTime(config, op, project, "updating alias ip ranges", userAgent, d.Timeout(schema.TimeoutUpdate)) - if opErr != nil { - return opErr - } - } - return nil - } - - if !updateDuringStop { - if d.HasChange(prefix + ".access_config") { - // Access configs cannot be updated, they must first be removed - if d.HasChange(prefix + ".access_config") { - err := deleteAccessConfigs() - if err != nil { - return err - } - err = createAccessConfigs() - if err != nil { - return err - } - err = readFingerPrint() - if err != nil { - return err - } + return err } } if d.HasChange(prefix + ".alias_ip_range") { - // Lets be explicit about what we are changing in the patch call - networkInterfacePatchObj := &computeBeta.NetworkInterface{} - // Alias IP ranges cannot be updated; they must be removed and then added // unless you are changing subnetwork/network - if d.HasChange(prefix + ".alias_ip_range") { - err := deleteAliasIPRanges() - if err != nil { - return err - } - networkInterfacePatchObj.AliasIpRanges = networkInterface.AliasIpRanges - networkInterfacePatchObj.Fingerprint = instNetworkInterface.Fingerprint - } - - patchCall := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterfacePatchObj).Do - op, err := patchCall() + err := niH.DeleteAliasIPRanges() if err != nil { - return errwrap.Wrapf("Error updating network interface: {{err}}", err) - } - opErr := computeOperationWaitTime(config, op, project, "network interface to update", userAgent, d.Timeout(schema.TimeoutUpdate)) - if opErr != nil { - return opErr + return err } - } - } else { - // Lets be explicit about what we are changing in the patch call - networkInterfacePatchObj := &computeBeta.NetworkInterface{ - Network: networkInterface.Network, - Subnetwork: networkInterface.Subnetwork, - AliasIpRanges: networkInterface.AliasIpRanges, - } - - // network_ip can be inferred if not declared. Let's only patch if it's being changed by user - // otherwise this could fail if the network ip is not compatible with the new Subnetwork/Network. - if d.HasChange(prefix + ".network_ip") { - networkInterfacePatchObj.NetworkIP = networkInterface.NetworkIP - } - - // Access config can run into some issues since we can't derive users original intent due to - // terraform limitation. Lets only update it if we need to. - if d.HasChange(prefix + ".access_config") { - err := deleteAccessConfigs() + err = niH.CreateAliasIPRanges() if err != nil { return err } } - - // Access configs' ip changes when the instance stops invalidating our fingerprint - // expect caller to re-validate instance before calling patch - patchIndex := i - patch := func(instance *computeBeta.Instance) error { - networkInterfacePatchObj.Fingerprint = instance.NetworkInterfaces[patchIndex].Fingerprint - op, err := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterfacePatchObj).Do() - if err != nil { - return errwrap.Wrapf("Error updating network interface: {{err}}", err) - } - opErr := computeOperationWaitTime(config, op, project, "network interface to update", userAgent, d.Timeout(schema.TimeoutUpdate)) - if opErr != nil { - return opErr - } - if d.HasChange(prefix + ".access_config") { - err := createAccessConfigs() - if err != nil { - return err - } - } - return nil + } else { + patch, err := niH.CreateUpdateWhileStoppedCall() + if err != nil { + return err } - updatesToNIWhileStopped = append(updatesToNIWhileStopped, patch) } } 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 d53aae8aa939..af2e4316377b 100644 --- a/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -4900,11 +4900,6 @@ func testAccComputeInstance_subnetworkUpdateTwo(suffix, instance string) string machine_type = "n1-standard-1" zone = "us-east1-d" allow_stopping_for_update = true - network_ip = "10.3.0.3" - - access_config { - network_tier = "STANDARD" - } boot_disk { initialize_params { @@ -4914,7 +4909,10 @@ func testAccComputeInstance_subnetworkUpdateTwo(suffix, instance string) string network_interface { subnetwork = google_compute_subnetwork.inst-test-subnetwork2.id - + network_ip = "10.3.0.3" + access_config { + network_tier = "STANDARD" + } 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" diff --git a/third_party/terraform/utils/compute_instance_network_interface_helpers.go b/third_party/terraform/utils/compute_instance_network_interface_helpers.go new file mode 100644 index 000000000000..89ed93872b86 --- /dev/null +++ b/third_party/terraform/utils/compute_instance_network_interface_helpers.go @@ -0,0 +1,184 @@ +package google + +import ( + "fmt" + + "github.com/hashicorp/errwrap" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + computeBeta "google.golang.org/api/compute/v0.beta" +) + +type networkInterfaceHelper struct { + InferNetworkFromSubnetwork func() error + RefreshInstance func() error + DeleteAccessConfigs func() error + CreateAccessConfigs func() error + DeleteAliasIPRanges func() error + CreateAliasIPRanges func() error + CreateUpdateWhileStoppedCall func() (func(inst *computeBeta.Instance) error, error) +} + +func networkInterfaceHelperFactory(d *schema.ResourceData, config *Config, instance *computeBeta.Instance, networkInterfaces []*computeBeta.NetworkInterface, index int, project, zone, userAgent string) (networkInterfaceHelper, error) { + prefix := fmt.Sprintf("network_interface.%d", index) + networkInterface := networkInterfaces[index] + instNetworkInterface := instance.NetworkInterfaces[index] + + networkName := d.Get(prefix + ".name").(string) + subnetwork := networkInterface.Subnetwork + + if networkName != instNetworkInterface.Name { + return networkInterfaceHelper{}, fmt.Errorf("Instance networkInterface had unexpected name: %s", instNetworkInterface.Name) + } + + inferNetworkFromSubnetwork := func() error { + 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() + return nil + } + + refreshInstance := func() error { + // re-read fingerprint + inst, err := config.clientComputeBeta.Instances.Get(project, zone, instance.Name).Do() + if err != nil { + return nil + } + + instance = inst + instNetworkInterface = instance.NetworkInterfaces[index] + return nil + } + + deleteAccessConfigs := func() error { + // Delete any accessConfig that currently exists in instNetworkInterface + for _, ac := range instNetworkInterface.AccessConfigs { + op, err := config.clientCompute.Instances.DeleteAccessConfig( + project, zone, instance.Name, ac.Name, networkName).Do() + if err != nil { + return fmt.Errorf("Error deleting old access_config: %s", err) + } + opErr := computeOperationWaitTime(config, op, project, "old access_config to delete", userAgent, d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr + } + } + return nil + } + + createAccessConfigs := func() error { + // Create new ones + for _, ac := range networkInterface.AccessConfigs { + op, err := config.clientComputeBeta.Instances.AddAccessConfig( + project, zone, instance.Name, networkName, ac).Do() + if err != nil { + return fmt.Errorf("Error adding new access_config: %s", err) + } + opErr := computeOperationWaitTime(config, op, project, "new access_config to add", userAgent, d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr + } + } + return nil + } + + deleteAliasIPRanges := func() error { + 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", userAgent, d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr + } + } + return nil + } + + createAliasIPRanges := func() error { + // Lets be explicit about what we are changing in the patch call + networkInterfacePatchObj := &computeBeta.NetworkInterface{} + networkInterfacePatchObj.AliasIpRanges = networkInterface.AliasIpRanges + networkInterfacePatchObj.Fingerprint = instNetworkInterface.Fingerprint + op, err := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterfacePatchObj).Do() + if err != nil { + return errwrap.Wrapf("Error updating network interface: {{err}}", err) + } + opErr := computeOperationWaitTime(config, op, project, "network interface to update", userAgent, d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr + } + return nil + } + + createUpdateWhileStoppedCall := func() (func(inst *computeBeta.Instance) error, error) { + // Lets be explicit about what we are changing in the patch call + networkInterfacePatchObj := &computeBeta.NetworkInterface{ + Network: networkInterface.Network, + Subnetwork: networkInterface.Subnetwork, + AliasIpRanges: networkInterface.AliasIpRanges, + } + + // network_ip can be inferred if not declared. Let's only patch if it's being changed by user + // otherwise this could fail if the network ip is not compatible with the new Subnetwork/Network. + if d.HasChange(prefix + ".network_ip") { + networkInterfacePatchObj.NetworkIP = networkInterface.NetworkIP + } + + // Access config can run into some issues since we can't derive users original intent due to + // terraform limitation. Lets only update it if we need to. + if d.HasChange(prefix + ".access_config") { + err := deleteAccessConfigs() + if err != nil { + return nil, err + } + } + + // Access configs' ip changes when the instance stops invalidating our fingerprint + // expect caller to re-validate instance before calling patch + updateCall := func(instance *computeBeta.Instance) error { + networkInterfacePatchObj.Fingerprint = instance.NetworkInterfaces[index].Fingerprint + op, err := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterfacePatchObj).Do() + if err != nil { + return errwrap.Wrapf("Error updating network interface: {{err}}", err) + } + opErr := computeOperationWaitTime(config, op, project, "network interface to update", userAgent, d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr + } + if d.HasChange(prefix + ".access_config") { + err := createAccessConfigs() + if err != nil { + return err + } + } + return nil + } + return updateCall, nil + } + + return networkInterfaceHelper{ + InferNetworkFromSubnetwork: inferNetworkFromSubnetwork, + RefreshInstance: refreshInstance, + DeleteAccessConfigs: deleteAccessConfigs, + CreateAccessConfigs: createAccessConfigs, + DeleteAliasIPRanges: deleteAliasIPRanges, + CreateAliasIPRanges: createAliasIPRanges, + CreateUpdateWhileStoppedCall: createUpdateWhileStoppedCall, + }, nil +} From 8de60c0272e0db747713fe038d534dc62be90a6d Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Tue, 29 Sep 2020 17:21:25 -0700 Subject: [PATCH 05/25] refresh instance after deleting alias config --- .../terraform/resources/resource_compute_instance.go.erb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index a5a51aadb394..e9f026fb1b08 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1419,6 +1419,11 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err if err != nil { return err } + // refresh the instance to get an up to date fingerpint for subsequent changes + err = niH.RefreshInstance() + if err != nil { + return err + } err = niH.CreateAliasIPRanges() if err != nil { return err From d6a1176d34807d1a119aa8f2267e5288b5e772e7 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Tue, 29 Sep 2020 19:51:41 -0700 Subject: [PATCH 06/25] correct bad merge --- .../utils/compute_instance_network_interface_helpers.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/third_party/terraform/utils/compute_instance_network_interface_helpers.go b/third_party/terraform/utils/compute_instance_network_interface_helpers.go index 9ca5a1c24eed..699b504d567d 100644 --- a/third_party/terraform/utils/compute_instance_network_interface_helpers.go +++ b/third_party/terraform/utils/compute_instance_network_interface_helpers.go @@ -36,7 +36,7 @@ func networkInterfaceHelperFactory(d *schema.ResourceData, config *Config, insta if err != nil { return fmt.Errorf("Cannot determine self_link for subnetwork %q: %s", subnetwork, err) } - resp, err := config.NewComputeClient.Subnetworks.Get(sf.Project, sf.Region, sf.Name).Do() + resp, err := config.NewComputeClient(userAgent).Subnetworks.Get(sf.Project, sf.Region, sf.Name).Do() if err != nil { return errwrap.Wrapf("Error getting subnetwork value: {{err}}", err) } @@ -63,7 +63,7 @@ func networkInterfaceHelperFactory(d *schema.ResourceData, config *Config, insta deleteAccessConfigs := func() error { // Delete any accessConfig that currently exists in instNetworkInterface for _, ac := range instNetworkInterface.AccessConfigs { - op, err := config.NewComputeClient.Instances.DeleteAccessConfig( + op, err := config.NewComputeClient(userAgent).Instances.DeleteAccessConfig( project, zone, instance.Name, ac.Name, networkName).Do() if err != nil { return fmt.Errorf("Error deleting old access_config: %s", err) From 465b8d50209e61403d0744821a676c266000a3b8 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Tue, 29 Sep 2020 20:43:49 -0700 Subject: [PATCH 07/25] spelling fix --- .../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 267c748b0925..4446fd67abb7 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1409,7 +1409,7 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err if err != nil { return err } - // refresh the instance to get an up to date fingerpint for subsequent changes + // refresh the instance to get an up to date fingerprint for subsequent changes err = niH.RefreshInstance() if err != nil { return err @@ -1422,7 +1422,7 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err if err != nil { return err } - // refresh the instance to get an up to date fingerpint for subsequent changes + // refresh the instance to get an up to date fingerprint for subsequent changes err = niH.RefreshInstance() if err != nil { return err From 566c7ba6c11867a300ac4aec36070b9712d45ee3 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Tue, 29 Sep 2020 21:15:07 -0700 Subject: [PATCH 08/25] check error --- .../terraform/resources/resource_compute_instance.go.erb | 5 ++++- 1 file changed, 4 insertions(+), 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 4446fd67abb7..d62502c8878e 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1394,7 +1394,10 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err if d.HasChange(prefix + ".subnetwork") { if !d.HasChange(prefix + ".network") { - niH.InferNetworkFromSubnetwork() + err := niH.InferNetworkFromSubnetwork() + if err != nil { + return err + } } } From 849d0c67606301fe9cb038f99f97824da85b0eb5 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Wed, 30 Sep 2020 12:15:24 -0700 Subject: [PATCH 09/25] Add forcenew unit test --- .../resource_compute_instance.go.erb | 5 + .../resource_compute_instance_test.go.erb | 126 ++++++++++++++++++ third_party/terraform/utils/test_utils.go | 5 + third_party/terraform/utils/utils.go.erb | 1 + 4 files changed, 137 insertions(+) diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index d62502c8878e..15df50ad3eb8 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -62,6 +62,11 @@ var ( // is also changing. Validate that if network_ip is changing this scenario // holds up to par. func forceNewIfNetworkIPNotUpdatable(ctx context.Context, d *schema.ResourceDiff, meta interface{}) error { + // separate func to allow unit testing + return forceNewIfNetworkIPNotUpdatableFunc(d) +} + +func forceNewIfNetworkIPNotUpdatableFunc(d TerraformResourceDiff) error { oldCount, newCount := d.GetChange("network_interface.#") if oldCount.(int) != newCount.(int) { return nil 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 b9a5e1fb80c7..c5289ae028b5 100644 --- a/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -1954,6 +1954,132 @@ func testAccCheckComputeInstanceDestroyProducer(t *testing.T) func(s *terraform. } } +func TestComputeInstance_networkIPCustomizedDiff(t *testing.T) { + t.Parallel() + + d := &ResourceDiffMock{ + Before: map[string]interface{}{ + "network_interface.#": 0, + }, + After: map[string]interface{}{ + "network_interface.#": 1, + }, + } + + forceNewIfNetworkIPNotUpdatableFunc(d) + if d.IsForceNew { + t.Errorf("Expected not force new if network_interface array size static") + } + + cases := map[string]struct { + NetworkBefore string + SubnetworkBefore string + SubnetworkProjectBefore string + NetworkIPBefore string + NetworkAfter string + SubnetworkAfter string + SubnetworkProjectAfter string + NetworkIPAfter string + ExpectedForceNew bool + }{ + "NetworkIP only change": { + ExpectedForceNew: true, + NetworkBefore: "a", + SubnetworkBefore: "a", + SubnetworkProjectBefore: "a", + NetworkIPBefore: "a", + + NetworkAfter: "a", + SubnetworkAfter: "a", + SubnetworkProjectAfter: "a", + NetworkIPAfter: "b", + }, + "NetworkIP and Network change": { + ExpectedForceNew: false, + NetworkBefore: "a", + SubnetworkBefore: "a", + SubnetworkProjectBefore: "a", + NetworkIPBefore: "a", + + NetworkAfter: "b", + SubnetworkAfter: "a", + SubnetworkProjectAfter: "a", + NetworkIPAfter: "b", + }, + "NetworkIP and Subtwork change": { + ExpectedForceNew: false, + NetworkBefore: "a", + SubnetworkBefore: "a", + SubnetworkProjectBefore: "a", + NetworkIPBefore: "a", + + NetworkAfter: "a", + SubnetworkAfter: "b", + SubnetworkProjectAfter: "a", + NetworkIPAfter: "b", + }, + "NetworkIP and SubtworkProject change": { + ExpectedForceNew: false, + NetworkBefore: "a", + SubnetworkBefore: "a", + SubnetworkProjectBefore: "a", + NetworkIPBefore: "a", + + NetworkAfter: "a", + SubnetworkAfter: "a", + SubnetworkProjectAfter: "b", + NetworkIPAfter: "b", + }, + "All change ": { + ExpectedForceNew: false, + NetworkBefore: "a", + SubnetworkBefore: "a", + SubnetworkProjectBefore: "a", + NetworkIPBefore: "a", + + NetworkAfter: "b", + SubnetworkAfter: "b", + SubnetworkProjectAfter: "b", + NetworkIPAfter: "b", + }, + "No change": { + ExpectedForceNew: false, + NetworkBefore: "a", + SubnetworkBefore: "a", + SubnetworkProjectBefore: "a", + NetworkIPBefore: "a", + + NetworkAfter: "a", + SubnetworkAfter: "a", + SubnetworkProjectAfter: "a", + NetworkIPAfter: "a", + }, + } + + for tn, tc := range cases { + d := &ResourceDiffMock{ + Before: map[string]interface{}{ + "network_interface.#": 1, + "network_interface.0.network": tc.NetworkBefore, + "network_interface.0.subnetwork": tc.SubnetworkBefore, + "network_interface.0.subnetwork_project": tc.SubnetworkProjectBefore, + "network_interface.0.network_ip": tc.NetworkIPBefore, + }, + After: map[string]interface{}{ + "network_interface.#": 1, + "network_interface.0.network": tc.NetworkAfter, + "network_interface.0.subnetwork": tc.SubnetworkAfter, + "network_interface.0.subnetwork_project": tc.SubnetworkProjectAfter, + "network_interface.0.network_ip": tc.NetworkIPAfter, + }, + } + forceNewIfNetworkIPNotUpdatableFunc(d) + if tc.ExpectedForceNew != d.IsForceNew { + t.Errorf("%v: Scenario got unexpected result", tn) + } + } +} + func testAccCheckComputeInstanceExists(t *testing.T, n string, instance interface{}) resource.TestCheckFunc { if instance == nil { panic("Attempted to check existence of Instance that was nil.") diff --git a/third_party/terraform/utils/test_utils.go b/third_party/terraform/utils/test_utils.go index 8eb6f2de7088..2f4fa5d79de5 100644 --- a/third_party/terraform/utils/test_utils.go +++ b/third_party/terraform/utils/test_utils.go @@ -73,6 +73,11 @@ func (d *ResourceDiffMock) GetChange(key string) (interface{}, interface{}) { return d.Before[key], d.After[key] } +func (d *ResourceDiffMock) HasChange(key string) bool { + old, new := d.GetChange(key) + return old != new +} + func (d *ResourceDiffMock) Get(key string) interface{} { return d.After[key] } diff --git a/third_party/terraform/utils/utils.go.erb b/third_party/terraform/utils/utils.go.erb index c5bd2f6c5260..5dbcce5bce92 100644 --- a/third_party/terraform/utils/utils.go.erb +++ b/third_party/terraform/utils/utils.go.erb @@ -30,6 +30,7 @@ type TerraformResourceData interface { } type TerraformResourceDiff interface { + HasChange(string) bool GetChange(string) (interface{}, interface{}) Get(string) interface{} Clear(string) error From cf00d31de714143bf71747a4b71fb60cad1c376b Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Wed, 30 Sep 2020 12:40:47 -0700 Subject: [PATCH 10/25] resolve build issue --- .../tests/resource_compute_instance_test.go.erb | 12 ++++++++++-- .../compute_instance_network_interface_helpers.go | 3 +-- 2 files changed, 11 insertions(+), 4 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 c5289ae028b5..30af43cdf50c 100644 --- a/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -1966,7 +1966,12 @@ func TestComputeInstance_networkIPCustomizedDiff(t *testing.T) { }, } - forceNewIfNetworkIPNotUpdatableFunc(d) + err := forceNewIfNetworkIPNotUpdatableFunc(d); + + if err != nil { + t.Errorf(err) + } + if d.IsForceNew { t.Errorf("Expected not force new if network_interface array size static") } @@ -2073,7 +2078,10 @@ func TestComputeInstance_networkIPCustomizedDiff(t *testing.T) { "network_interface.0.network_ip": tc.NetworkIPAfter, }, } - forceNewIfNetworkIPNotUpdatableFunc(d) + err := forceNewIfNetworkIPNotUpdatableFunc(d); + if err != nil { + t.Errorf(err) + } if tc.ExpectedForceNew != d.IsForceNew { t.Errorf("%v: Scenario got unexpected result", tn) } diff --git a/third_party/terraform/utils/compute_instance_network_interface_helpers.go b/third_party/terraform/utils/compute_instance_network_interface_helpers.go index 699b504d567d..0b82dd4a8455 100644 --- a/third_party/terraform/utils/compute_instance_network_interface_helpers.go +++ b/third_party/terraform/utils/compute_instance_network_interface_helpers.go @@ -22,15 +22,14 @@ func networkInterfaceHelperFactory(d *schema.ResourceData, config *Config, insta prefix := fmt.Sprintf("network_interface.%d", index) networkInterface := networkInterfaces[index] instNetworkInterface := instance.NetworkInterfaces[index] - networkName := d.Get(prefix + ".name").(string) - subnetwork := networkInterface.Subnetwork if networkName != instNetworkInterface.Name { return networkInterfaceHelper{}, fmt.Errorf("Instance networkInterface had unexpected name: %s", instNetworkInterface.Name) } inferNetworkFromSubnetwork := func() error { + subnetwork := networkInterface.Subnetwork subnetProjectField := prefix + ".subnetwork_project" sf, err := ParseSubnetworkFieldValueWithProjectField(subnetwork, subnetProjectField, d, config) if err != nil { From 0880d41bc1231bd6d47eefd95952d47817304616 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Wed, 30 Sep 2020 12:49:04 -0700 Subject: [PATCH 11/25] Update third_party/terraform/tests/resource_compute_instance_test.go.erb Co-authored-by: Cameron Thornton --- .../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 30af43cdf50c..8e231e8701a1 100644 --- a/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -2011,7 +2011,7 @@ func TestComputeInstance_networkIPCustomizedDiff(t *testing.T) { SubnetworkProjectAfter: "a", NetworkIPAfter: "b", }, - "NetworkIP and Subtwork change": { + "NetworkIP and Subnetwork change": { ExpectedForceNew: false, NetworkBefore: "a", SubnetworkBefore: "a", From f6160eab447125cad042f9db88ed81a502da0bee Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Wed, 30 Sep 2020 12:49:11 -0700 Subject: [PATCH 12/25] Update third_party/terraform/tests/resource_compute_instance_test.go.erb Co-authored-by: Cameron Thornton --- .../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 8e231e8701a1..44ef56722f74 100644 --- a/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -2023,7 +2023,7 @@ func TestComputeInstance_networkIPCustomizedDiff(t *testing.T) { SubnetworkProjectAfter: "a", NetworkIPAfter: "b", }, - "NetworkIP and SubtworkProject change": { + "NetworkIP and SubnetworkProject change": { ExpectedForceNew: false, NetworkBefore: "a", SubnetworkBefore: "a", From 09c87d9334c30befc9e037ba119628aaa5757d92 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Wed, 30 Sep 2020 14:15:36 -0700 Subject: [PATCH 13/25] error couldn't convert to string --- .../terraform/tests/resource_compute_instance_test.go.erb | 4 ++-- 1 file changed, 2 insertions(+), 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 30af43cdf50c..25813c961503 100644 --- a/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -1969,7 +1969,7 @@ func TestComputeInstance_networkIPCustomizedDiff(t *testing.T) { err := forceNewIfNetworkIPNotUpdatableFunc(d); if err != nil { - t.Errorf(err) + t.Errorf("%v", err) } if d.IsForceNew { @@ -2080,7 +2080,7 @@ func TestComputeInstance_networkIPCustomizedDiff(t *testing.T) { } err := forceNewIfNetworkIPNotUpdatableFunc(d); if err != nil { - t.Errorf(err) + t.Errorf("%v", err) } if tc.ExpectedForceNew != d.IsForceNew { t.Errorf("%v: Scenario got unexpected result", tn) From f6136848085408f5a002cb9640542692965b16ac Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Wed, 30 Sep 2020 16:05:45 -0700 Subject: [PATCH 14/25] to type string --- .../terraform/tests/resource_compute_instance_test.go.erb | 4 ++-- 1 file changed, 2 insertions(+), 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 d9f14b3c6da1..e8bb6211e1d3 100644 --- a/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -1969,7 +1969,7 @@ func TestComputeInstance_networkIPCustomizedDiff(t *testing.T) { err := forceNewIfNetworkIPNotUpdatableFunc(d); if err != nil { - t.Errorf("%v", err) + t.Errorf(err.Error()) } if d.IsForceNew { @@ -2080,7 +2080,7 @@ func TestComputeInstance_networkIPCustomizedDiff(t *testing.T) { } err := forceNewIfNetworkIPNotUpdatableFunc(d); if err != nil { - t.Errorf("%v", err) + t.Errorf(err.Error()) } if tc.ExpectedForceNew != d.IsForceNew { t.Errorf("%v: Scenario got unexpected result", tn) From 7eb0af65b89350e1dd272a57b6c9e9b0a1e888f0 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Thu, 1 Oct 2020 14:48:39 -0700 Subject: [PATCH 15/25] Refactored some test code for cleaner style and condensed some if statements --- .../resource_compute_instance.go.erb | 10 +- .../resource_compute_instance_test.go.erb | 168 +++++++++--------- 2 files changed, 85 insertions(+), 93 deletions(-) diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index 15df50ad3eb8..5642251af43b 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1397,12 +1397,10 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err // 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") { - err := niH.InferNetworkFromSubnetwork() - if err != nil { - return err - } + if d.HasChange(prefix + ".subnetwork") && !d.HasChange(prefix + ".network") { + err := niH.InferNetworkFromSubnetwork() + if err != nil { + return 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 e8bb6211e1d3..8b0a55b16ff9 100644 --- a/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -1966,98 +1966,92 @@ func TestComputeInstance_networkIPCustomizedDiff(t *testing.T) { }, } - err := forceNewIfNetworkIPNotUpdatableFunc(d); - + err := forceNewIfNetworkIPNotUpdatableFunc(d) if err != nil { - t.Errorf(err.Error()) + t.Error(err) } if d.IsForceNew { - t.Errorf("Expected not force new if network_interface array size static") + t.Errorf("Expected not force new if network_interface array size changes") + } + + type NetworkInterface struct { + Network string + Subnetwork string + SubnetworkProject string + NetworkIP string + } + NIBefore := NetworkInterface{ + Network: "a", + Subnetwork: "a", + SubnetworkProject: "a", + NetworkIP: "a", } cases := map[string]struct { - NetworkBefore string - SubnetworkBefore string - SubnetworkProjectBefore string - NetworkIPBefore string - NetworkAfter string - SubnetworkAfter string - SubnetworkProjectAfter string - NetworkIPAfter string - ExpectedForceNew bool + ExpectedForceNew bool + Before NetworkInterface + After NetworkInterface }{ "NetworkIP only change": { - ExpectedForceNew: true, - NetworkBefore: "a", - SubnetworkBefore: "a", - SubnetworkProjectBefore: "a", - NetworkIPBefore: "a", - - NetworkAfter: "a", - SubnetworkAfter: "a", - SubnetworkProjectAfter: "a", - NetworkIPAfter: "b", + ExpectedForceNew: true, + Before: NIBefore, + After: NetworkInterface{ + Network: "a", + Subnetwork: "a", + SubnetworkProject: "a", + NetworkIP: "b", + }, }, "NetworkIP and Network change": { - ExpectedForceNew: false, - NetworkBefore: "a", - SubnetworkBefore: "a", - SubnetworkProjectBefore: "a", - NetworkIPBefore: "a", - - NetworkAfter: "b", - SubnetworkAfter: "a", - SubnetworkProjectAfter: "a", - NetworkIPAfter: "b", + ExpectedForceNew: false, + Before: NIBefore, + After: NetworkInterface{ + Network: "b", + Subnetwork: "a", + SubnetworkProject: "a", + NetworkIP: "b", + }, }, - "NetworkIP and Subnetwork change": { - ExpectedForceNew: false, - NetworkBefore: "a", - SubnetworkBefore: "a", - SubnetworkProjectBefore: "a", - NetworkIPBefore: "a", - - NetworkAfter: "a", - SubnetworkAfter: "b", - SubnetworkProjectAfter: "a", - NetworkIPAfter: "b", + "NetworkIP and Subtwork change": { + ExpectedForceNew: false, + Before: NIBefore, + After: NetworkInterface{ + Network: "a", + Subnetwork: "b", + SubnetworkProject: "a", + NetworkIP: "b", + }, }, - "NetworkIP and SubnetworkProject change": { - ExpectedForceNew: false, - NetworkBefore: "a", - SubnetworkBefore: "a", - SubnetworkProjectBefore: "a", - NetworkIPBefore: "a", - - NetworkAfter: "a", - SubnetworkAfter: "a", - SubnetworkProjectAfter: "b", - NetworkIPAfter: "b", + "NetworkIP and SubtworkProject change": { + ExpectedForceNew: false, + Before: NIBefore, + After: NetworkInterface{ + Network: "a", + Subnetwork: "a", + SubnetworkProject: "b", + NetworkIP: "b", + }, }, "All change ": { - ExpectedForceNew: false, - NetworkBefore: "a", - SubnetworkBefore: "a", - SubnetworkProjectBefore: "a", - NetworkIPBefore: "a", - - NetworkAfter: "b", - SubnetworkAfter: "b", - SubnetworkProjectAfter: "b", - NetworkIPAfter: "b", + ExpectedForceNew: false, + Before: NIBefore, + After: NetworkInterface{ + Network: "b", + Subnetwork: "b", + SubnetworkProject: "b", + NetworkIP: "b", + }, }, "No change": { - ExpectedForceNew: false, - NetworkBefore: "a", - SubnetworkBefore: "a", - SubnetworkProjectBefore: "a", - NetworkIPBefore: "a", - - NetworkAfter: "a", - SubnetworkAfter: "a", - SubnetworkProjectAfter: "a", - NetworkIPAfter: "a", + ExpectedForceNew: false, + Before: NIBefore, + After: NetworkInterface{ + Network: "a", + Subnetwork: "a", + SubnetworkProject: "a", + NetworkIP: "a", + }, }, } @@ -2065,25 +2059,25 @@ func TestComputeInstance_networkIPCustomizedDiff(t *testing.T) { d := &ResourceDiffMock{ Before: map[string]interface{}{ "network_interface.#": 1, - "network_interface.0.network": tc.NetworkBefore, - "network_interface.0.subnetwork": tc.SubnetworkBefore, - "network_interface.0.subnetwork_project": tc.SubnetworkProjectBefore, - "network_interface.0.network_ip": tc.NetworkIPBefore, + "network_interface.0.network": tc.Before.Network, + "network_interface.0.subnetwork": tc.Before.Subnetwork, + "network_interface.0.subnetwork_project": tc.Before.SubnetworkProject, + "network_interface.0.network_ip": tc.Before.NetworkIP, }, After: map[string]interface{}{ "network_interface.#": 1, - "network_interface.0.network": tc.NetworkAfter, - "network_interface.0.subnetwork": tc.SubnetworkAfter, - "network_interface.0.subnetwork_project": tc.SubnetworkProjectAfter, - "network_interface.0.network_ip": tc.NetworkIPAfter, + "network_interface.0.network": tc.After.Network, + "network_interface.0.subnetwork": tc.After.Subnetwork, + "network_interface.0.subnetwork_project": tc.After.SubnetworkProject, + "network_interface.0.network_ip": tc.After.NetworkIP, }, } - err := forceNewIfNetworkIPNotUpdatableFunc(d); - if err != nil { - t.Errorf(err.Error()) + err := forceNewIfNetworkIPNotUpdatableFunc(d) + if err != nil { + t.Error(err) } if tc.ExpectedForceNew != d.IsForceNew { - t.Errorf("%v: Scenario got unexpected result", tn) + t.Errorf("%v: expected d.IsForceNew to be %v, but was %v", tn, tc.ExpectedForceNew, d.IsForceNew) } } } From f3402548fa0fcdd428d7d40b7c34a5542ccf8927 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Thu, 1 Oct 2020 15:22:17 -0700 Subject: [PATCH 16/25] resolve merge issue with megan's change --- .../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 67c1b620aa69..86b9c58d6104 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1709,7 +1709,7 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err // If the instance stops it can invalidate the fingerprint for network interface. // refresh the instance to get a new fingerprint if len(updatesToNIWhileStopped) > 0 { - instance, err = config.clientComputeBeta.Instances.Get(project, zone, instance.Name).Do() + instance, err = config.NewComputeBetaClient(userAgent).Instances.Get(project, zone, instance.Name).Do() if err != nil { return err } From 6c0e819214294048965f7e20c838c75845c21248 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Fri, 2 Oct 2020 10:59:02 -0700 Subject: [PATCH 17/25] update network interface helper to promote better go readability --- .../resource_compute_instance.go.erb | 2 +- ...pute_instance_network_interface_helpers.go | 267 +++++++++--------- 2 files changed, 137 insertions(+), 132 deletions(-) diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index 86b9c58d6104..18576ff56ab0 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1383,7 +1383,7 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err for i := 0; i < len(networkInterfaces); i++ { prefix := fmt.Sprintf("network_interface.%d", i) updateDuringStop := d.HasChange(prefix+".subnetwork") || d.HasChange(prefix+".network") || d.HasChange(prefix+".subnetwork_project") - niH, err := networkInterfaceHelperFactory(d, config, instance, networkInterfaces, i, project, zone, userAgent) + niH, err := createNetworkInterfaceHelper(d, config, instance, networkInterfaces, i, project, zone, userAgent) if err != nil { return err } diff --git a/third_party/terraform/utils/compute_instance_network_interface_helpers.go b/third_party/terraform/utils/compute_instance_network_interface_helpers.go index 6fe69a3d8e55..360592f6fcd5 100644 --- a/third_party/terraform/utils/compute_instance_network_interface_helpers.go +++ b/third_party/terraform/utils/compute_instance_network_interface_helpers.go @@ -9,16 +9,19 @@ import ( ) type networkInterfaceHelper struct { - InferNetworkFromSubnetwork func() error - RefreshInstance func() error - DeleteAccessConfigs func() error - CreateAccessConfigs func() error - DeleteAliasIPRanges func() error - CreateAliasIPRanges func() error - CreateUpdateWhileStoppedCall func() (func(inst *computeBeta.Instance) error, error) + d *schema.ResourceData + config *Config + instNetworkInterface *computeBeta.NetworkInterface + networkInterface *computeBeta.NetworkInterface + index int + instanceName string + prefix string + project string + zone string + userAgent string } -func networkInterfaceHelperFactory(d *schema.ResourceData, config *Config, instance *computeBeta.Instance, networkInterfaces []*computeBeta.NetworkInterface, index int, project, zone, userAgent string) (networkInterfaceHelper, error) { +func createNetworkInterfaceHelper(d *schema.ResourceData, config *Config, instance *computeBeta.Instance, networkInterfaces []*computeBeta.NetworkInterface, index int, project, zone, userAgent string) (networkInterfaceHelper, error) { prefix := fmt.Sprintf("network_interface.%d", index) networkInterface := networkInterfaces[index] instNetworkInterface := instance.NetworkInterfaces[index] @@ -27,157 +30,159 @@ func networkInterfaceHelperFactory(d *schema.ResourceData, config *Config, insta if networkName != instNetworkInterface.Name { return networkInterfaceHelper{}, fmt.Errorf("Instance networkInterface had unexpected name: %s", instNetworkInterface.Name) } + return networkInterfaceHelper{ + d: d, + config: config, + instNetworkInterface: instNetworkInterface, + networkInterface: networkInterface, + index: index, + instanceName: instance.Name, + prefix: prefix, + project: project, + zone: zone, + userAgent: userAgent, + }, nil +} - inferNetworkFromSubnetwork := func() error { - subnetwork := networkInterface.Subnetwork - subnetProjectField := prefix + ".subnetwork_project" - sf, err := ParseSubnetworkFieldValueWithProjectField(subnetwork, subnetProjectField, d, config) +func (niH *networkInterfaceHelper) InferNetworkFromSubnetwork() error { + subnetwork := niH.networkInterface.Subnetwork + subnetProjectField := niH.prefix + ".subnetwork_project" + sf, err := ParseSubnetworkFieldValueWithProjectField(subnetwork, subnetProjectField, niH.d, niH.config) + if err != nil { + return fmt.Errorf("Cannot determine self_link for subnetwork %q: %s", subnetwork, err) + } + resp, err := niH.config.NewComputeClient(niH.userAgent).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, niH.d, niH.config) + if err != nil { + return fmt.Errorf("Cannot determine self_link for network %q: %s", resp.Network, err) + } + niH.networkInterface.Network = nf.RelativeLink() + return nil +} + +func (niH *networkInterfaceHelper) RefreshInstance() error { + // re-read fingerprint + inst, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.Get(niH.project, niH.zone, niH.networkInterface.Name).Do() + if err != nil { + return nil + } + + instance := inst + niH.instNetworkInterface = instance.NetworkInterfaces[niH.index] + return nil +} + +func (niH *networkInterfaceHelper) DeleteAccessConfigs() error { + // Delete any accessConfig that currently exists in instNetworkInterface + for _, ac := range niH.instNetworkInterface.AccessConfigs { + op, err := niH.config.NewComputeClient(niH.userAgent).Instances.DeleteAccessConfig( + niH.project, niH.zone, niH.instanceName, ac.Name, niH.networkInterface.Name).Do() if err != nil { - return fmt.Errorf("Cannot determine self_link for subnetwork %q: %s", subnetwork, err) + return fmt.Errorf("Error deleting old access_config: %s", err) } - resp, err := config.NewComputeClient(userAgent).Subnetworks.Get(sf.Project, sf.Region, sf.Name).Do() - if err != nil { - return errwrap.Wrapf("Error getting subnetwork value: {{err}}", err) + opErr := computeOperationWaitTime(niH.config, op, niH.project, "old access_config to delete", niH.userAgent, niH.d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr } - nf, err := ParseNetworkFieldValue(resp.Network, d, config) + } + return nil +} + +func (niH *networkInterfaceHelper) CreateAccessConfigs() error { + // Create new ones + for _, ac := range niH.networkInterface.AccessConfigs { + op, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.AddAccessConfig( + niH.project, niH.zone, niH.instanceName, niH.networkInterface.Name, ac).Do() if err != nil { - return fmt.Errorf("Cannot determine self_link for network %q: %s", resp.Network, err) + return fmt.Errorf("Error adding new access_config: %s", err) + } + opErr := computeOperationWaitTime(niH.config, op, niH.project, "new access_config to add", niH.userAgent, niH.d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr } - networkInterface.Network = nf.RelativeLink() - return nil } + return nil +} - refreshInstance := func() error { - // re-read fingerprint - inst, err := config.NewComputeBetaClient(userAgent).Instances.Get(project, zone, instance.Name).Do() +func (niH *networkInterfaceHelper) DeleteAliasIPRanges() error { + if len(niH.instNetworkInterface.AliasIpRanges) > 0 { + ni := &computeBeta.NetworkInterface{ + Fingerprint: niH.instNetworkInterface.Fingerprint, + ForceSendFields: []string{"AliasIpRanges"}, + } + op, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, niH.networkInterface.Name, ni).Do() if err != nil { - return nil + return errwrap.Wrapf("Error removing alias_ip_range: {{err}}", err) } + opErr := computeOperationWaitTime(niH.config, op, niH.project, "updating alias ip ranges", niH.userAgent, niH.d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr + } + } + return nil +} - instance = inst - instNetworkInterface = instance.NetworkInterfaces[index] - return nil +func (niH *networkInterfaceHelper) CreateAliasIPRanges() error { + // Lets be explicit about what we are changing in the patch call + networkInterfacePatchObj := &computeBeta.NetworkInterface{} + networkInterfacePatchObj.AliasIpRanges = niH.networkInterface.AliasIpRanges + networkInterfacePatchObj.Fingerprint = niH.instNetworkInterface.Fingerprint + op, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, niH.networkInterface.Name, networkInterfacePatchObj).Do() + if err != nil { + return errwrap.Wrapf("Error updating network interface: {{err}}", err) } + opErr := computeOperationWaitTime(niH.config, op, niH.project, "network interface to update", niH.userAgent, niH.d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr + } + return nil +} - deleteAccessConfigs := func() error { - // Delete any accessConfig that currently exists in instNetworkInterface - for _, ac := range instNetworkInterface.AccessConfigs { - op, err := config.NewComputeClient(userAgent).Instances.DeleteAccessConfig( - project, zone, instance.Name, ac.Name, networkName).Do() - if err != nil { - return fmt.Errorf("Error deleting old access_config: %s", err) - } - opErr := computeOperationWaitTime(config, op, project, "old access_config to delete", userAgent, d.Timeout(schema.TimeoutUpdate)) - if opErr != nil { - return opErr - } - } - return nil +func (niH *networkInterfaceHelper) CreateUpdateWhileStoppedCall() (func(inst *computeBeta.Instance) error, error) { + // Lets be explicit about what we are changing in the patch call + networkInterfacePatchObj := &computeBeta.NetworkInterface{ + Network: niH.networkInterface.Network, + Subnetwork: niH.networkInterface.Subnetwork, + AliasIpRanges: niH.networkInterface.AliasIpRanges, } - createAccessConfigs := func() error { - // Create new ones - for _, ac := range networkInterface.AccessConfigs { - op, err := config.NewComputeBetaClient(userAgent).Instances.AddAccessConfig( - project, zone, instance.Name, networkName, ac).Do() - if err != nil { - return fmt.Errorf("Error adding new access_config: %s", err) - } - opErr := computeOperationWaitTime(config, op, project, "new access_config to add", userAgent, d.Timeout(schema.TimeoutUpdate)) - if opErr != nil { - return opErr - } - } - return nil + // network_ip can be inferred if not declared. Let's only patch if it's being changed by user + // otherwise this could fail if the network ip is not compatible with the new Subnetwork/Network. + if niH.d.HasChange(niH.prefix + ".network_ip") { + networkInterfacePatchObj.NetworkIP = niH.networkInterface.NetworkIP } - deleteAliasIPRanges := func() error { - if len(instNetworkInterface.AliasIpRanges) > 0 { - ni := &computeBeta.NetworkInterface{ - Fingerprint: instNetworkInterface.Fingerprint, - ForceSendFields: []string{"AliasIpRanges"}, - } - op, err := config.NewComputeBetaClient(userAgent).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", userAgent, d.Timeout(schema.TimeoutUpdate)) - if opErr != nil { - return opErr - } + // Access config can run into some issues since we can't derive users original intent due to + // terraform limitation. Lets only update it if we need to. + if niH.d.HasChange(niH.prefix + ".access_config") { + err := niH.DeleteAccessConfigs() + if err != nil { + return nil, err } - return nil } - createAliasIPRanges := func() error { - // Lets be explicit about what we are changing in the patch call - networkInterfacePatchObj := &computeBeta.NetworkInterface{} - networkInterfacePatchObj.AliasIpRanges = networkInterface.AliasIpRanges - networkInterfacePatchObj.Fingerprint = instNetworkInterface.Fingerprint - op, err := config.NewComputeBetaClient(userAgent).Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterfacePatchObj).Do() + // Access configs' ip changes when the instance stops invalidating our fingerprint + // expect caller to re-validate instance before calling patch + updateCall := func(instance *computeBeta.Instance) error { + networkInterfacePatchObj.Fingerprint = instance.NetworkInterfaces[niH.index].Fingerprint + op, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, niH.networkInterface.Name, networkInterfacePatchObj).Do() if err != nil { return errwrap.Wrapf("Error updating network interface: {{err}}", err) } - opErr := computeOperationWaitTime(config, op, project, "network interface to update", userAgent, d.Timeout(schema.TimeoutUpdate)) + opErr := computeOperationWaitTime(niH.config, op, niH.project, "network interface to update", niH.userAgent, niH.d.Timeout(schema.TimeoutUpdate)) if opErr != nil { return opErr } - return nil - } - - createUpdateWhileStoppedCall := func() (func(inst *computeBeta.Instance) error, error) { - // Lets be explicit about what we are changing in the patch call - networkInterfacePatchObj := &computeBeta.NetworkInterface{ - Network: networkInterface.Network, - Subnetwork: networkInterface.Subnetwork, - AliasIpRanges: networkInterface.AliasIpRanges, - } - - // network_ip can be inferred if not declared. Let's only patch if it's being changed by user - // otherwise this could fail if the network ip is not compatible with the new Subnetwork/Network. - if d.HasChange(prefix + ".network_ip") { - networkInterfacePatchObj.NetworkIP = networkInterface.NetworkIP - } - - // Access config can run into some issues since we can't derive users original intent due to - // terraform limitation. Lets only update it if we need to. - if d.HasChange(prefix + ".access_config") { - err := deleteAccessConfigs() - if err != nil { - return nil, err - } - } - - // Access configs' ip changes when the instance stops invalidating our fingerprint - // expect caller to re-validate instance before calling patch - updateCall := func(instance *computeBeta.Instance) error { - networkInterfacePatchObj.Fingerprint = instance.NetworkInterfaces[index].Fingerprint - op, err := config.NewComputeBetaClient(userAgent).Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterfacePatchObj).Do() + if niH.d.HasChange(niH.prefix + ".access_config") { + err := niH.CreateAccessConfigs() if err != nil { - return errwrap.Wrapf("Error updating network interface: {{err}}", err) - } - opErr := computeOperationWaitTime(config, op, project, "network interface to update", userAgent, d.Timeout(schema.TimeoutUpdate)) - if opErr != nil { - return opErr - } - if d.HasChange(prefix + ".access_config") { - err := createAccessConfigs() - if err != nil { - return err - } + return err } - return nil } - return updateCall, nil + return nil } - - return networkInterfaceHelper{ - InferNetworkFromSubnetwork: inferNetworkFromSubnetwork, - RefreshInstance: refreshInstance, - DeleteAccessConfigs: deleteAccessConfigs, - CreateAccessConfigs: createAccessConfigs, - DeleteAliasIPRanges: deleteAliasIPRanges, - CreateAliasIPRanges: createAliasIPRanges, - CreateUpdateWhileStoppedCall: createUpdateWhileStoppedCall, - }, nil + return updateCall, nil } From 1f1a50692f26e2b91f30370982bfe84d144f2fd5 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Fri, 2 Oct 2020 11:20:27 -0700 Subject: [PATCH 18/25] fixed instances where incorrect name was referenced --- .../compute_instance_network_interface_helpers.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/third_party/terraform/utils/compute_instance_network_interface_helpers.go b/third_party/terraform/utils/compute_instance_network_interface_helpers.go index 360592f6fcd5..936a47caee4c 100644 --- a/third_party/terraform/utils/compute_instance_network_interface_helpers.go +++ b/third_party/terraform/utils/compute_instance_network_interface_helpers.go @@ -65,7 +65,7 @@ func (niH *networkInterfaceHelper) InferNetworkFromSubnetwork() error { func (niH *networkInterfaceHelper) RefreshInstance() error { // re-read fingerprint - inst, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.Get(niH.project, niH.zone, niH.networkInterface.Name).Do() + inst, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.Get(niH.project, niH.zone, niH.instanceName).Do() if err != nil { return nil } @@ -79,7 +79,7 @@ func (niH *networkInterfaceHelper) DeleteAccessConfigs() error { // Delete any accessConfig that currently exists in instNetworkInterface for _, ac := range niH.instNetworkInterface.AccessConfigs { op, err := niH.config.NewComputeClient(niH.userAgent).Instances.DeleteAccessConfig( - niH.project, niH.zone, niH.instanceName, ac.Name, niH.networkInterface.Name).Do() + niH.project, niH.zone, niH.instanceName, ac.Name, niH.instNetworkInterface.Name).Do() if err != nil { return fmt.Errorf("Error deleting old access_config: %s", err) } @@ -95,7 +95,7 @@ func (niH *networkInterfaceHelper) CreateAccessConfigs() error { // Create new ones for _, ac := range niH.networkInterface.AccessConfigs { op, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.AddAccessConfig( - niH.project, niH.zone, niH.instanceName, niH.networkInterface.Name, ac).Do() + niH.project, niH.zone, niH.instanceName, niH.instNetworkInterface.Name, ac).Do() if err != nil { return fmt.Errorf("Error adding new access_config: %s", err) } @@ -113,7 +113,7 @@ func (niH *networkInterfaceHelper) DeleteAliasIPRanges() error { Fingerprint: niH.instNetworkInterface.Fingerprint, ForceSendFields: []string{"AliasIpRanges"}, } - op, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, niH.networkInterface.Name, ni).Do() + op, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, niH.instNetworkInterface.Name, ni).Do() if err != nil { return errwrap.Wrapf("Error removing alias_ip_range: {{err}}", err) } @@ -130,7 +130,7 @@ func (niH *networkInterfaceHelper) CreateAliasIPRanges() error { networkInterfacePatchObj := &computeBeta.NetworkInterface{} networkInterfacePatchObj.AliasIpRanges = niH.networkInterface.AliasIpRanges networkInterfacePatchObj.Fingerprint = niH.instNetworkInterface.Fingerprint - op, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, niH.networkInterface.Name, networkInterfacePatchObj).Do() + op, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, niH.instNetworkInterface.Name, networkInterfacePatchObj).Do() if err != nil { return errwrap.Wrapf("Error updating network interface: {{err}}", err) } @@ -168,7 +168,7 @@ func (niH *networkInterfaceHelper) CreateUpdateWhileStoppedCall() (func(inst *co // expect caller to re-validate instance before calling patch updateCall := func(instance *computeBeta.Instance) error { networkInterfacePatchObj.Fingerprint = instance.NetworkInterfaces[niH.index].Fingerprint - op, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, niH.networkInterface.Name, networkInterfacePatchObj).Do() + op, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, niH.instNetworkInterface.Name, networkInterfacePatchObj).Do() if err != nil { return errwrap.Wrapf("Error updating network interface: {{err}}", err) } From 17a4d832e3f79b2d9a2dd0a3e17563254c5780c7 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Tue, 6 Oct 2020 20:16:40 -0700 Subject: [PATCH 19/25] removed extraneous dependencies and extrapolated self mutating operations --- .../resource_compute_instance.go.erb | 48 ++++-- ...pute_instance_network_interface_helpers.go | 144 +++++++----------- 2 files changed, 93 insertions(+), 99 deletions(-) diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index 18576ff56ab0..ecc849b70e6a 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1383,9 +1383,29 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err for i := 0; i < len(networkInterfaces); i++ { prefix := fmt.Sprintf("network_interface.%d", i) updateDuringStop := d.HasChange(prefix+".subnetwork") || d.HasChange(prefix+".network") || d.HasChange(prefix+".subnetwork_project") - niH, err := createNetworkInterfaceHelper(d, config, instance, networkInterfaces, i, project, zone, userAgent) - if err != nil { - return err + networkName := d.Get(prefix + ".name").(string) + instNetworkInterface := instance.NetworkInterfaces[i] + networkInterface := networkInterfaces[i] + + if networkName != instNetworkInterface.Name { + return fmt.Errorf("Instance networkInterface had unexpected name: %s", instNetworkInterface.Name) + } + + waitForOperation := func(res interface{}, activity string) error { + return computeOperationWaitTime(config, res, project, activity, userAgent, d.Timeout(schema.TimeoutUpdate)) + } + + var niH networkInterfaceHelperInterface = &networkInterfaceHelper{ + WaitForOperation: waitForOperation, + GetSubnetworks: config.NewComputeClient(userAgent).Subnetworks.Get, + DeleteAccessConfig: config.NewComputeClient(userAgent).Instances.DeleteAccessConfig, + AddAccessConfig: config.NewComputeBetaClient(userAgent).Instances.AddAccessConfig, + UpdateNetworkInterface: config.NewComputeBetaClient(userAgent).Instances.UpdateNetworkInterface, + HasChange: d.HasChange, + instanceName: instance.Name, + prefix: prefix, + project: project, + zone: zone, } // On creation the network is inferred if only subnetwork is given. @@ -1394,50 +1414,52 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err // 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") && !d.HasChange(prefix + ".network") { - err := niH.InferNetworkFromSubnetwork() + if d.HasChange(prefix+".subnetwork") && !d.HasChange(prefix+".network") { + network, err := niH.InferNetworkFromSubnetwork(d, config, instNetworkInterface, networkInterface) if err != nil { return err } + networkInterface.Network = network } if !updateDuringStop { if d.HasChange(prefix + ".access_config") { // Access configs cannot be updated, they must first be removed - err := niH.DeleteAccessConfigs() + err := niH.DeleteAccessConfigs(instNetworkInterface) if err != nil { return err } - err = niH.CreateAccessConfigs() + err = niH.CreateAccessConfigs(instNetworkInterface, networkInterface) if err != nil { return err } // refresh the instance to get an up to date fingerprint for subsequent changes - err = niH.RefreshInstance() + instance, err = config.NewComputeBetaClient(userAgent).Instances.Get(project, zone, instance.Name).Do() if err != nil { return err } + instNetworkInterface = instance.NetworkInterfaces[i] } 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 - err := niH.DeleteAliasIPRanges() + err := niH.DeleteAliasIPRanges(instNetworkInterface) if err != nil { return err } // refresh the instance to get an up to date fingerprint for subsequent changes - err = niH.RefreshInstance() + instance, err = config.NewComputeBetaClient(userAgent).Instances.Get(project, zone, instance.Name).Do() if err != nil { return err } - err = niH.CreateAliasIPRanges() + instNetworkInterface = instance.NetworkInterfaces[i] + err = niH.CreateAliasIPRanges(instNetworkInterface, networkInterface) if err != nil { return err } } } else { - patch, err := niH.CreateUpdateWhileStoppedCall() + patch, err := niH.CreateUpdateWhileStoppedCall(instNetworkInterface, networkInterface, i) if err != nil { return err } diff --git a/third_party/terraform/utils/compute_instance_network_interface_helpers.go b/third_party/terraform/utils/compute_instance_network_interface_helpers.go index 936a47caee4c..a58ca7c0a44c 100644 --- a/third_party/terraform/utils/compute_instance_network_interface_helpers.go +++ b/third_party/terraform/utils/compute_instance_network_interface_helpers.go @@ -6,84 +6,57 @@ import ( "github.com/hashicorp/errwrap" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" computeBeta "google.golang.org/api/compute/v0.beta" + "google.golang.org/api/compute/v1" ) type networkInterfaceHelper struct { - d *schema.ResourceData - config *Config - instNetworkInterface *computeBeta.NetworkInterface - networkInterface *computeBeta.NetworkInterface - index int - instanceName string - prefix string - project string - zone string - userAgent string + WaitForOperation func(interface{}, string) error + GetSubnetworks func(string, string, string) *compute.SubnetworksGetCall + DeleteAccessConfig func(string, string, string, string, string) *compute.InstancesDeleteAccessConfigCall + AddAccessConfig func(string, string, string, string, *computeBeta.AccessConfig) *computeBeta.InstancesAddAccessConfigCall + UpdateNetworkInterface func(string, string, string, string, *computeBeta.NetworkInterface) *computeBeta.InstancesUpdateNetworkInterfaceCall + HasChange func(string) bool + instanceName string + prefix string + project string + zone string } -func createNetworkInterfaceHelper(d *schema.ResourceData, config *Config, instance *computeBeta.Instance, networkInterfaces []*computeBeta.NetworkInterface, index int, project, zone, userAgent string) (networkInterfaceHelper, error) { - prefix := fmt.Sprintf("network_interface.%d", index) - networkInterface := networkInterfaces[index] - instNetworkInterface := instance.NetworkInterfaces[index] - networkName := d.Get(prefix + ".name").(string) - - if networkName != instNetworkInterface.Name { - return networkInterfaceHelper{}, fmt.Errorf("Instance networkInterface had unexpected name: %s", instNetworkInterface.Name) - } - return networkInterfaceHelper{ - d: d, - config: config, - instNetworkInterface: instNetworkInterface, - networkInterface: networkInterface, - index: index, - instanceName: instance.Name, - prefix: prefix, - project: project, - zone: zone, - userAgent: userAgent, - }, nil +type networkInterfaceHelperInterface interface { + InferNetworkFromSubnetwork(d *schema.ResourceData, config *Config, instNetworkInterface, networkInterface *computeBeta.NetworkInterface) (string, error) + DeleteAccessConfigs(instNetworkInterface *computeBeta.NetworkInterface) error + CreateAccessConfigs(instNetworkInterface, networkInterface *computeBeta.NetworkInterface) error + DeleteAliasIPRanges(instNetworkInterface *computeBeta.NetworkInterface) error + CreateAliasIPRanges(instNetworkInterface, networkInterface *computeBeta.NetworkInterface) error + CreateUpdateWhileStoppedCall(instNetworkInterface, networkInterface *computeBeta.NetworkInterface, index int) (func(inst *computeBeta.Instance) error, error) } -func (niH *networkInterfaceHelper) InferNetworkFromSubnetwork() error { - subnetwork := niH.networkInterface.Subnetwork +func (niH *networkInterfaceHelper) InferNetworkFromSubnetwork(d *schema.ResourceData, config *Config, instNetworkInterface, networkInterface *computeBeta.NetworkInterface) (string, error) { + subnetwork := networkInterface.Subnetwork subnetProjectField := niH.prefix + ".subnetwork_project" - sf, err := ParseSubnetworkFieldValueWithProjectField(subnetwork, subnetProjectField, niH.d, niH.config) + sf, err := ParseSubnetworkFieldValueWithProjectField(subnetwork, subnetProjectField, d, config) if err != nil { - return fmt.Errorf("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 := niH.config.NewComputeClient(niH.userAgent).Subnetworks.Get(sf.Project, sf.Region, sf.Name).Do() + resp, err := niH.GetSubnetworks(sf.Project, sf.Region, sf.Name).Do() if err != nil { - return errwrap.Wrapf("Error getting subnetwork value: {{err}}", err) + return "", errwrap.Wrapf("Error getting subnetwork value: {{err}}", err) } - nf, err := ParseNetworkFieldValue(resp.Network, niH.d, niH.config) - if err != nil { - return fmt.Errorf("Cannot determine self_link for network %q: %s", resp.Network, err) - } - niH.networkInterface.Network = nf.RelativeLink() - return nil -} - -func (niH *networkInterfaceHelper) RefreshInstance() error { - // re-read fingerprint - inst, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.Get(niH.project, niH.zone, niH.instanceName).Do() + nf, err := ParseNetworkFieldValue(resp.Network, d, config) if err != nil { - return nil + return "", fmt.Errorf("Cannot determine self_link for network %q: %s", resp.Network, err) } - - instance := inst - niH.instNetworkInterface = instance.NetworkInterfaces[niH.index] - return nil + return nf.RelativeLink(), nil } -func (niH *networkInterfaceHelper) DeleteAccessConfigs() error { +func (niH *networkInterfaceHelper) DeleteAccessConfigs(instNetworkInterface *computeBeta.NetworkInterface) error { // Delete any accessConfig that currently exists in instNetworkInterface - for _, ac := range niH.instNetworkInterface.AccessConfigs { - op, err := niH.config.NewComputeClient(niH.userAgent).Instances.DeleteAccessConfig( - niH.project, niH.zone, niH.instanceName, ac.Name, niH.instNetworkInterface.Name).Do() + for _, ac := range instNetworkInterface.AccessConfigs { + op, err := niH.DeleteAccessConfig(niH.project, niH.zone, niH.instanceName, ac.Name, instNetworkInterface.Name).Do() if err != nil { return fmt.Errorf("Error deleting old access_config: %s", err) } - opErr := computeOperationWaitTime(niH.config, op, niH.project, "old access_config to delete", niH.userAgent, niH.d.Timeout(schema.TimeoutUpdate)) + opErr := niH.WaitForOperation(op, "old access_config to delete") if opErr != nil { return opErr } @@ -91,15 +64,14 @@ func (niH *networkInterfaceHelper) DeleteAccessConfigs() error { return nil } -func (niH *networkInterfaceHelper) CreateAccessConfigs() error { +func (niH *networkInterfaceHelper) CreateAccessConfigs(instNetworkInterface, networkInterface *computeBeta.NetworkInterface) error { // Create new ones - for _, ac := range niH.networkInterface.AccessConfigs { - op, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.AddAccessConfig( - niH.project, niH.zone, niH.instanceName, niH.instNetworkInterface.Name, ac).Do() + for _, ac := range networkInterface.AccessConfigs { + op, err := niH.AddAccessConfig(niH.project, niH.zone, niH.instanceName, instNetworkInterface.Name, ac).Do() if err != nil { return fmt.Errorf("Error adding new access_config: %s", err) } - opErr := computeOperationWaitTime(niH.config, op, niH.project, "new access_config to add", niH.userAgent, niH.d.Timeout(schema.TimeoutUpdate)) + opErr := niH.WaitForOperation(op, "new access_config to add") if opErr != nil { return opErr } @@ -107,17 +79,17 @@ func (niH *networkInterfaceHelper) CreateAccessConfigs() error { return nil } -func (niH *networkInterfaceHelper) DeleteAliasIPRanges() error { - if len(niH.instNetworkInterface.AliasIpRanges) > 0 { +func (niH *networkInterfaceHelper) DeleteAliasIPRanges(instNetworkInterface *computeBeta.NetworkInterface) error { + if len(instNetworkInterface.AliasIpRanges) > 0 { ni := &computeBeta.NetworkInterface{ - Fingerprint: niH.instNetworkInterface.Fingerprint, + Fingerprint: instNetworkInterface.Fingerprint, ForceSendFields: []string{"AliasIpRanges"}, } - op, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, niH.instNetworkInterface.Name, ni).Do() + op, err := niH.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, instNetworkInterface.Name, ni).Do() if err != nil { return errwrap.Wrapf("Error removing alias_ip_range: {{err}}", err) } - opErr := computeOperationWaitTime(niH.config, op, niH.project, "updating alias ip ranges", niH.userAgent, niH.d.Timeout(schema.TimeoutUpdate)) + opErr := niH.WaitForOperation(op, "updating alias ip ranges") if opErr != nil { return opErr } @@ -125,40 +97,40 @@ func (niH *networkInterfaceHelper) DeleteAliasIPRanges() error { return nil } -func (niH *networkInterfaceHelper) CreateAliasIPRanges() error { +func (niH *networkInterfaceHelper) CreateAliasIPRanges(instNetworkInterface, networkInterface *computeBeta.NetworkInterface) error { // Lets be explicit about what we are changing in the patch call networkInterfacePatchObj := &computeBeta.NetworkInterface{} - networkInterfacePatchObj.AliasIpRanges = niH.networkInterface.AliasIpRanges - networkInterfacePatchObj.Fingerprint = niH.instNetworkInterface.Fingerprint - op, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, niH.instNetworkInterface.Name, networkInterfacePatchObj).Do() + networkInterfacePatchObj.AliasIpRanges = networkInterface.AliasIpRanges + networkInterfacePatchObj.Fingerprint = instNetworkInterface.Fingerprint + op, err := niH.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, instNetworkInterface.Name, networkInterfacePatchObj).Do() if err != nil { return errwrap.Wrapf("Error updating network interface: {{err}}", err) } - opErr := computeOperationWaitTime(niH.config, op, niH.project, "network interface to update", niH.userAgent, niH.d.Timeout(schema.TimeoutUpdate)) + opErr := niH.WaitForOperation(op, "network interface to update") if opErr != nil { return opErr } return nil } -func (niH *networkInterfaceHelper) CreateUpdateWhileStoppedCall() (func(inst *computeBeta.Instance) error, error) { +func (niH *networkInterfaceHelper) CreateUpdateWhileStoppedCall(instNetworkInterface, networkInterface *computeBeta.NetworkInterface, index int) (func(inst *computeBeta.Instance) error, error) { // Lets be explicit about what we are changing in the patch call networkInterfacePatchObj := &computeBeta.NetworkInterface{ - Network: niH.networkInterface.Network, - Subnetwork: niH.networkInterface.Subnetwork, - AliasIpRanges: niH.networkInterface.AliasIpRanges, + Network: networkInterface.Network, + Subnetwork: networkInterface.Subnetwork, + AliasIpRanges: networkInterface.AliasIpRanges, } // network_ip can be inferred if not declared. Let's only patch if it's being changed by user // otherwise this could fail if the network ip is not compatible with the new Subnetwork/Network. - if niH.d.HasChange(niH.prefix + ".network_ip") { - networkInterfacePatchObj.NetworkIP = niH.networkInterface.NetworkIP + if niH.HasChange(niH.prefix + ".network_ip") { + networkInterfacePatchObj.NetworkIP = networkInterface.NetworkIP } // Access config can run into some issues since we can't derive users original intent due to // terraform limitation. Lets only update it if we need to. - if niH.d.HasChange(niH.prefix + ".access_config") { - err := niH.DeleteAccessConfigs() + if niH.HasChange(niH.prefix + ".access_config") { + err := niH.DeleteAccessConfigs(instNetworkInterface) if err != nil { return nil, err } @@ -167,17 +139,17 @@ func (niH *networkInterfaceHelper) CreateUpdateWhileStoppedCall() (func(inst *co // Access configs' ip changes when the instance stops invalidating our fingerprint // expect caller to re-validate instance before calling patch updateCall := func(instance *computeBeta.Instance) error { - networkInterfacePatchObj.Fingerprint = instance.NetworkInterfaces[niH.index].Fingerprint - op, err := niH.config.NewComputeBetaClient(niH.userAgent).Instances.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, niH.instNetworkInterface.Name, networkInterfacePatchObj).Do() + networkInterfacePatchObj.Fingerprint = instance.NetworkInterfaces[index].Fingerprint + op, err := niH.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, instNetworkInterface.Name, networkInterfacePatchObj).Do() if err != nil { return errwrap.Wrapf("Error updating network interface: {{err}}", err) } - opErr := computeOperationWaitTime(niH.config, op, niH.project, "network interface to update", niH.userAgent, niH.d.Timeout(schema.TimeoutUpdate)) + opErr := niH.WaitForOperation(op, "network interface to update") if opErr != nil { return opErr } - if niH.d.HasChange(niH.prefix + ".access_config") { - err := niH.CreateAccessConfigs() + if niH.HasChange(niH.prefix + ".access_config") { + err := niH.CreateAccessConfigs(instance.NetworkInterfaces[index], networkInterface) if err != nil { return err } From 8cf7348a19d8ee5cf7f83a56c150f653669f1c9c Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Fri, 9 Oct 2020 02:18:22 -0700 Subject: [PATCH 20/25] scrap refactor. only extrapolate functions when needed. --- .../resource_compute_instance.go.erb | 138 ++++++++++------- .../resource_compute_instance_test.go.erb | 118 +++++++-------- ...pute_instance_network_interface_helpers.go | 140 ++++-------------- 3 files changed, 176 insertions(+), 220 deletions(-) diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index ecc849b70e6a..d16f572d157e 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1379,91 +1379,123 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err return fmt.Errorf("Instance had unexpected number of network interfaces: %d", len(instance.NetworkInterfaces)) } - var updatesToNIWhileStopped []func(*computeBeta.Instance) error + + var updatesToNIWhileStopped []func(inst *computeBeta.Instance) error for i := 0; i < len(networkInterfaces); i++ { prefix := fmt.Sprintf("network_interface.%d", i) - updateDuringStop := d.HasChange(prefix+".subnetwork") || d.HasChange(prefix+".network") || d.HasChange(prefix+".subnetwork_project") - networkName := d.Get(prefix + ".name").(string) - instNetworkInterface := instance.NetworkInterfaces[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) } - waitForOperation := func(res interface{}, activity string) error { - return computeOperationWaitTime(config, res, project, activity, userAgent, d.Timeout(schema.TimeoutUpdate)) - } - - var niH networkInterfaceHelperInterface = &networkInterfaceHelper{ - WaitForOperation: waitForOperation, - GetSubnetworks: config.NewComputeClient(userAgent).Subnetworks.Get, - DeleteAccessConfig: config.NewComputeClient(userAgent).Instances.DeleteAccessConfig, - AddAccessConfig: config.NewComputeBetaClient(userAgent).Instances.AddAccessConfig, - UpdateNetworkInterface: config.NewComputeBetaClient(userAgent).Instances.UpdateNetworkInterface, - HasChange: d.HasChange, - instanceName: instance.Name, - prefix: prefix, - project: project, - zone: zone, - } - // 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") && !d.HasChange(prefix+".network") { - network, err := niH.InferNetworkFromSubnetwork(d, config, instNetworkInterface, networkInterface) - if err != nil { - return err - } - networkInterface.Network = network - } - if !updateDuringStop { - if d.HasChange(prefix + ".access_config") { - // Access configs cannot be updated, they must first be removed - err := niH.DeleteAccessConfigs(instNetworkInterface) + 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 err + return fmt.Errorf("Cannot determine self_link for subnetwork %q: %s", subnetwork, err) } - err = niH.CreateAccessConfigs(instNetworkInterface, networkInterface) + resp, err := config.NewComputeClient(userAgent).Subnetworks.Get(sf.Project, sf.Region, sf.Name).Do() if err != nil { - return err + return errwrap.Wrapf("Error getting subnetwork value: {{err}}", err) } - // refresh the instance to get an up to date fingerprint for subsequent changes - instance, err = config.NewComputeBetaClient(userAgent).Instances.Get(project, zone, instance.Name).Do() + nf, err := ParseNetworkFieldValue(resp.Network, d, config) if err != nil { - return err + return fmt.Errorf("Cannot determine self_link for network %q: %s", resp.Network, err) } - instNetworkInterface = instance.NetworkInterfaces[i] + networkInterface.Network = nf.RelativeLink() + } + } + + if !updateDuringStop && d.HasChange(prefix+".access_config") { + // Delete current access configs + err := computeInstanceDeleteAccessConfigs(d, config, instNetworkInterface, project, zone, userAgent, instance.Name) + if err != nil { + return err + } + + // Create new ones + err = computeInstanceAddAccessConfigs(d, config, instNetworkInterface, networkInterface.AccessConfigs, project, zone, userAgent, instance.Name) + // re-read fingerprint + instance, err = config.NewComputeBetaClient(userAgent).Instances.Get(project, zone, instance.Name).Do() + if err != nil { + return err } - 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 - err := niH.DeleteAliasIPRanges(instNetworkInterface) + instNetworkInterface = instance.NetworkInterfaces[i] + } + + 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.NewComputeBetaClient(userAgent).Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, ni).Do() if err != nil { - return err + return errwrap.Wrapf("Error removing alias_ip_range: {{err}}", err) + } + opErr := computeOperationWaitTime(config, op, project, "updating alias ip ranges", userAgent, d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr } - // refresh the instance to get an up to date fingerprint for subsequent changes + // re-read fingerprint instance, err = config.NewComputeBetaClient(userAgent).Instances.Get(project, zone, instance.Name).Do() if err != nil { return err } instNetworkInterface = instance.NetworkInterfaces[i] - err = niH.CreateAliasIPRanges(instNetworkInterface, networkInterface) - if err != nil { - return err - } } - } else { - patch, err := niH.CreateUpdateWhileStoppedCall(instNetworkInterface, networkInterface, i) + + networkInterfacePatchObj := &computeBeta.NetworkInterface{} + networkInterfacePatchObj.AliasIpRanges = networkInterface.AliasIpRanges + networkInterfacePatchObj.Fingerprint = instNetworkInterface.Fingerprint + updateCall := config.NewComputeBetaClient(userAgent).Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterfacePatchObj).Do + op, err := updateCall() if err != nil { - return err + return errwrap.Wrapf("Error updating network interface: {{err}}", err) } - updatesToNIWhileStopped = append(updatesToNIWhileStopped, patch) + opErr := computeOperationWaitTime(config, op, project, "network interface to update", userAgent, d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr + } + } + + if updateDuringStop { + // Lets be explicit about what we are changing in the patch call + networkInterfacePatchObj := &computeBeta.NetworkInterface{ + Network: networkInterface.Network, + Subnetwork: networkInterface.Subnetwork, + AliasIpRanges: networkInterface.AliasIpRanges, + } + + // network_ip can be inferred if not declared. Let's only patch if it's being changed by user + // otherwise this could fail if the network ip is not compatible with the new Subnetwork/Network. + if d.HasChange(prefix + ".network_ip") { + networkInterfacePatchObj.NetworkIP = networkInterface.NetworkIP + } + + // Access config can run into some issues since we can't derive users original intent due to + // terraform limitation. Lets only update it if we need to. + accessConfigsHaveChanged := d.HasChange(prefix + ".access_config") + + updateCall := computeInstanceCreateUpdateWhileStoppedCall(d, config, networkInterfacePatchObj, networkInterface.AccessConfigs, accessConfigsHaveChanged, i, project, zone, userAgent, instance.Name) + updatesToNIWhileStopped = append(updatesToNIWhileStopped, updateCall) } } 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 f7a5b6d23b27..84f6847ab802 100644 --- a/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -1895,65 +1895,6 @@ func TestAccComputeInstance_subnetworkUpdate(t *testing.T) { }) } -func testAccCheckComputeInstanceUpdateMachineType(t *testing.T, n string) resource.TestCheckFunc { - return func(s *terraform.State) error { - rs, ok := s.RootModule().Resources[n] - if !ok { - return fmt.Errorf("Not found: %s", n) - } - - if rs.Primary.ID == "" { - return fmt.Errorf("No ID is set") - } - - config := googleProviderConfig(t) - - op, err := config.NewComputeClient(config.userAgent).Instances.Stop(config.Project, rs.Primary.Attributes["zone"], rs.Primary.Attributes["name"]).Do() - if err != nil { - return fmt.Errorf("Could not stop instance: %s", err) - } - err = computeOperationWaitTime(config, op, config.Project, "Waiting on stop", config.userAgent, 20*time.Minute) - if err != nil { - return fmt.Errorf("Could not stop instance: %s", err) - } - - machineType := compute.InstancesSetMachineTypeRequest{ - MachineType: "zones/us-central1-a/machineTypes/f1-micro", - } - - op, err = config.NewComputeClient(config.userAgent).Instances.SetMachineType( - config.Project, rs.Primary.Attributes["zone"], rs.Primary.Attributes["name"], &machineType).Do() - if err != nil { - return fmt.Errorf("Could not change machine type: %s", err) - } - err = computeOperationWaitTime(config, op, config.Project, "Waiting machine type change", config.userAgent, 20*time.Minute) - if err != nil { - return fmt.Errorf("Could not change machine type: %s", err) - } - return nil - } -} - -func testAccCheckComputeInstanceDestroyProducer(t *testing.T) func(s *terraform.State) error { - return func(s *terraform.State) error { - config := googleProviderConfig(t) - - for _, rs := range s.RootModule().Resources { - if rs.Type != "google_compute_instance" { - continue - } - - _, err := config.NewComputeClient(config.userAgent).Instances.Get( - config.Project, rs.Primary.Attributes["zone"], rs.Primary.Attributes["name"]).Do() - if err == nil { - return fmt.Errorf("Instance still exists") - } - } - - return nil - } -} - func TestComputeInstance_networkIPCustomizedDiff(t *testing.T) { t.Parallel() @@ -2082,6 +2023,65 @@ func TestComputeInstance_networkIPCustomizedDiff(t *testing.T) { } } +func testAccCheckComputeInstanceUpdateMachineType(t *testing.T, n string) resource.TestCheckFunc { + return func(s *terraform.State) error { + rs, ok := s.RootModule().Resources[n] + if !ok { + return fmt.Errorf("Not found: %s", n) + } + + if rs.Primary.ID == "" { + return fmt.Errorf("No ID is set") + } + + config := googleProviderConfig(t) + + op, err := config.NewComputeClient(config.userAgent).Instances.Stop(config.Project, rs.Primary.Attributes["zone"], rs.Primary.Attributes["name"]).Do() + if err != nil { + return fmt.Errorf("Could not stop instance: %s", err) + } + err = computeOperationWaitTime(config, op, config.Project, "Waiting on stop", config.userAgent, 20*time.Minute) + if err != nil { + return fmt.Errorf("Could not stop instance: %s", err) + } + + machineType := compute.InstancesSetMachineTypeRequest{ + MachineType: "zones/us-central1-a/machineTypes/f1-micro", + } + + op, err = config.NewComputeClient(config.userAgent).Instances.SetMachineType( + config.Project, rs.Primary.Attributes["zone"], rs.Primary.Attributes["name"], &machineType).Do() + if err != nil { + return fmt.Errorf("Could not change machine type: %s", err) + } + err = computeOperationWaitTime(config, op, config.Project, "Waiting machine type change", config.userAgent, 20*time.Minute) + if err != nil { + return fmt.Errorf("Could not change machine type: %s", err) + } + return nil + } +} + +func testAccCheckComputeInstanceDestroyProducer(t *testing.T) func(s *terraform.State) error { + return func(s *terraform.State) error { + config := googleProviderConfig(t) + + for _, rs := range s.RootModule().Resources { + if rs.Type != "google_compute_instance" { + continue + } + + _, err := config.NewComputeClient(config.userAgent).Instances.Get( + config.Project, rs.Primary.Attributes["zone"], rs.Primary.Attributes["name"]).Do() + if err == nil { + return fmt.Errorf("Instance still exists") + } + } + + return nil + } +} + func testAccCheckComputeInstanceExists(t *testing.T, n string, instance interface{}) resource.TestCheckFunc { if instance == nil { panic("Attempted to check existence of Instance that was nil.") diff --git a/third_party/terraform/utils/compute_instance_network_interface_helpers.go b/third_party/terraform/utils/compute_instance_network_interface_helpers.go index a58ca7c0a44c..88a3a7595732 100644 --- a/third_party/terraform/utils/compute_instance_network_interface_helpers.go +++ b/third_party/terraform/utils/compute_instance_network_interface_helpers.go @@ -6,57 +6,23 @@ import ( "github.com/hashicorp/errwrap" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" computeBeta "google.golang.org/api/compute/v0.beta" - "google.golang.org/api/compute/v1" ) -type networkInterfaceHelper struct { - WaitForOperation func(interface{}, string) error - GetSubnetworks func(string, string, string) *compute.SubnetworksGetCall - DeleteAccessConfig func(string, string, string, string, string) *compute.InstancesDeleteAccessConfigCall - AddAccessConfig func(string, string, string, string, *computeBeta.AccessConfig) *computeBeta.InstancesAddAccessConfigCall - UpdateNetworkInterface func(string, string, string, string, *computeBeta.NetworkInterface) *computeBeta.InstancesUpdateNetworkInterfaceCall - HasChange func(string) bool - instanceName string - prefix string - project string - zone string -} - -type networkInterfaceHelperInterface interface { - InferNetworkFromSubnetwork(d *schema.ResourceData, config *Config, instNetworkInterface, networkInterface *computeBeta.NetworkInterface) (string, error) - DeleteAccessConfigs(instNetworkInterface *computeBeta.NetworkInterface) error - CreateAccessConfigs(instNetworkInterface, networkInterface *computeBeta.NetworkInterface) error - DeleteAliasIPRanges(instNetworkInterface *computeBeta.NetworkInterface) error - CreateAliasIPRanges(instNetworkInterface, networkInterface *computeBeta.NetworkInterface) error - CreateUpdateWhileStoppedCall(instNetworkInterface, networkInterface *computeBeta.NetworkInterface, index int) (func(inst *computeBeta.Instance) error, error) -} - -func (niH *networkInterfaceHelper) InferNetworkFromSubnetwork(d *schema.ResourceData, config *Config, instNetworkInterface, networkInterface *computeBeta.NetworkInterface) (string, error) { - subnetwork := networkInterface.Subnetwork - subnetProjectField := niH.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 := niH.GetSubnetworks(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) - } - return nf.RelativeLink(), nil -} +func computeInstanceDeleteAccessConfigs(d *schema.ResourceData, config *Config, instNetworkInterface *computeBeta.NetworkInterface, project, zone, userAgent, instanceName string) error { + // TODO: This code deletes then recreates accessConfigs. This is bad because it may + // leave the machine inaccessible from either ip if the creation part fails (network + // timeout etc). However right now there is a GCE limit of 1 accessConfig so it is + // the only way to do it. In future this should be revised to only change what is + // necessary, and also add before removing. -func (niH *networkInterfaceHelper) DeleteAccessConfigs(instNetworkInterface *computeBeta.NetworkInterface) error { // Delete any accessConfig that currently exists in instNetworkInterface for _, ac := range instNetworkInterface.AccessConfigs { - op, err := niH.DeleteAccessConfig(niH.project, niH.zone, niH.instanceName, ac.Name, instNetworkInterface.Name).Do() + op, err := config.NewComputeClient(userAgent).Instances.DeleteAccessConfig( + project, zone, instanceName, ac.Name, instNetworkInterface.Name).Do() if err != nil { return fmt.Errorf("Error deleting old access_config: %s", err) } - opErr := niH.WaitForOperation(op, "old access_config to delete") + opErr := computeOperationWaitTime(config, op, project, "old access_config to delete", userAgent, d.Timeout(schema.TimeoutUpdate)) if opErr != nil { return opErr } @@ -64,32 +30,14 @@ func (niH *networkInterfaceHelper) DeleteAccessConfigs(instNetworkInterface *com return nil } -func (niH *networkInterfaceHelper) CreateAccessConfigs(instNetworkInterface, networkInterface *computeBeta.NetworkInterface) error { +func computeInstanceAddAccessConfigs(d *schema.ResourceData, config *Config, instNetworkInterface *computeBeta.NetworkInterface, accessConfigs []*computeBeta.AccessConfig, project, zone, userAgent, instanceName string) error { // Create new ones - for _, ac := range networkInterface.AccessConfigs { - op, err := niH.AddAccessConfig(niH.project, niH.zone, niH.instanceName, instNetworkInterface.Name, ac).Do() + for _, ac := range accessConfigs { + op, err := config.NewComputeBetaClient(userAgent).Instances.AddAccessConfig(project, zone, instanceName, instNetworkInterface.Name, ac).Do() if err != nil { return fmt.Errorf("Error adding new access_config: %s", err) } - opErr := niH.WaitForOperation(op, "new access_config to add") - if opErr != nil { - return opErr - } - } - return nil -} - -func (niH *networkInterfaceHelper) DeleteAliasIPRanges(instNetworkInterface *computeBeta.NetworkInterface) error { - if len(instNetworkInterface.AliasIpRanges) > 0 { - ni := &computeBeta.NetworkInterface{ - Fingerprint: instNetworkInterface.Fingerprint, - ForceSendFields: []string{"AliasIpRanges"}, - } - op, err := niH.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, instNetworkInterface.Name, ni).Do() - if err != nil { - return errwrap.Wrapf("Error removing alias_ip_range: {{err}}", err) - } - opErr := niH.WaitForOperation(op, "updating alias ip ranges") + opErr := computeOperationWaitTime(config, op, project, "new access_config to add", userAgent, d.Timeout(schema.TimeoutUpdate)) if opErr != nil { return opErr } @@ -97,64 +45,40 @@ func (niH *networkInterfaceHelper) DeleteAliasIPRanges(instNetworkInterface *com return nil } -func (niH *networkInterfaceHelper) CreateAliasIPRanges(instNetworkInterface, networkInterface *computeBeta.NetworkInterface) error { - // Lets be explicit about what we are changing in the patch call - networkInterfacePatchObj := &computeBeta.NetworkInterface{} - networkInterfacePatchObj.AliasIpRanges = networkInterface.AliasIpRanges - networkInterfacePatchObj.Fingerprint = instNetworkInterface.Fingerprint - op, err := niH.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, instNetworkInterface.Name, networkInterfacePatchObj).Do() - if err != nil { - return errwrap.Wrapf("Error updating network interface: {{err}}", err) - } - opErr := niH.WaitForOperation(op, "network interface to update") - if opErr != nil { - return opErr - } - return nil -} +func computeInstanceCreateUpdateWhileStoppedCall(d *schema.ResourceData, config *Config, networkInterfacePatchObj *computeBeta.NetworkInterface, accessConfigs []*computeBeta.AccessConfig, accessConfigsHaveChanged bool, index int, project, zone, userAgent, instanceName string) func(inst *computeBeta.Instance) error { -func (niH *networkInterfaceHelper) CreateUpdateWhileStoppedCall(instNetworkInterface, networkInterface *computeBeta.NetworkInterface, index int) (func(inst *computeBeta.Instance) error, error) { - // Lets be explicit about what we are changing in the patch call - networkInterfacePatchObj := &computeBeta.NetworkInterface{ - Network: networkInterface.Network, - Subnetwork: networkInterface.Subnetwork, - AliasIpRanges: networkInterface.AliasIpRanges, - } + // Access configs' ip changes when the instance stops invalidating our fingerprint + // expect caller to re-validate instance before calling patch this is why we expect + // instance to be passed in + return func(instance *computeBeta.Instance) error { - // network_ip can be inferred if not declared. Let's only patch if it's being changed by user - // otherwise this could fail if the network ip is not compatible with the new Subnetwork/Network. - if niH.HasChange(niH.prefix + ".network_ip") { - networkInterfacePatchObj.NetworkIP = networkInterface.NetworkIP - } + instNetworkInterface := instance.NetworkInterfaces[index] + networkInterfacePatchObj.Fingerprint = instNetworkInterface.Fingerprint - // Access config can run into some issues since we can't derive users original intent due to - // terraform limitation. Lets only update it if we need to. - if niH.HasChange(niH.prefix + ".access_config") { - err := niH.DeleteAccessConfigs(instNetworkInterface) - if err != nil { - return nil, err + // Access config can run into some issues since we can't derive users original intent due to + // terraform limitation. Lets only update it if we need to. + if accessConfigsHaveChanged { + err := computeInstanceDeleteAccessConfigs(d, config, instNetworkInterface, project, zone, userAgent, instanceName) + if err != nil { + return err + } } - } - // Access configs' ip changes when the instance stops invalidating our fingerprint - // expect caller to re-validate instance before calling patch - updateCall := func(instance *computeBeta.Instance) error { - networkInterfacePatchObj.Fingerprint = instance.NetworkInterfaces[index].Fingerprint - op, err := niH.UpdateNetworkInterface(niH.project, niH.zone, niH.instanceName, instNetworkInterface.Name, networkInterfacePatchObj).Do() + op, err := config.NewComputeBetaClient(userAgent).Instances.UpdateNetworkInterface(project, zone, instanceName, instNetworkInterface.Name, networkInterfacePatchObj).Do() if err != nil { return errwrap.Wrapf("Error updating network interface: {{err}}", err) } - opErr := niH.WaitForOperation(op, "network interface to update") + opErr := computeOperationWaitTime(config, op, project, "network interface to update", userAgent, d.Timeout(schema.TimeoutUpdate)) if opErr != nil { return opErr } - if niH.HasChange(niH.prefix + ".access_config") { - err := niH.CreateAccessConfigs(instance.NetworkInterfaces[index], networkInterface) + + if accessConfigsHaveChanged { + err := computeInstanceAddAccessConfigs(d, config, instNetworkInterface, accessConfigs, project, zone, userAgent, instanceName) if err != nil { return err } } return nil } - return updateCall, nil } From 32e332e48b8e0d5dc051331537670006056a528e Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Fri, 9 Oct 2020 02:30:41 -0700 Subject: [PATCH 21/25] error check --- .../terraform/resources/resource_compute_instance.go.erb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index d16f572d157e..d855ca304b29 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1430,6 +1430,10 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err // Create new ones err = computeInstanceAddAccessConfigs(d, config, instNetworkInterface, networkInterface.AccessConfigs, project, zone, userAgent, instance.Name) + if err != nil { + return err + } + // re-read fingerprint instance, err = config.NewComputeBetaClient(userAgent).Instances.Get(project, zone, instance.Name).Do() if err != nil { From a2d114fc86eddb0032f28133c809308880c952cb Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Fri, 9 Oct 2020 10:43:31 -0700 Subject: [PATCH 22/25] resolved some fomatting and comment concerns. --- .../resources/resource_compute_instance.go.erb | 14 +++++++++----- .../resource_compute_instance_test.go.erb | 6 +++--- ...mpute_instance_network_interface_helpers.go | 18 ++++++++++-------- 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index d855ca304b29..1c5d5b425e1c 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1466,9 +1466,10 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err instNetworkInterface = instance.NetworkInterfaces[i] } - networkInterfacePatchObj := &computeBeta.NetworkInterface{} - networkInterfacePatchObj.AliasIpRanges = networkInterface.AliasIpRanges - networkInterfacePatchObj.Fingerprint = instNetworkInterface.Fingerprint + networkInterfacePatchObj := &computeBeta.NetworkInterface{ + AliasIpRanges: networkInterface.AliasIpRanges + Fingerprint: instNetworkInterface.Fingerprint + } updateCall := config.NewComputeBetaClient(userAgent).Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterfacePatchObj).Do op, err := updateCall() if err != nil { @@ -1494,8 +1495,11 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err networkInterfacePatchObj.NetworkIP = networkInterface.NetworkIP } - // Access config can run into some issues since we can't derive users original intent due to - // terraform limitation. Lets only update it if we need to. + // Access config can run into some issues since we can't tell the difference between + // the users declared intent (config within their hcl file) and what we have inferred from the + // server (terraform state). Access configs contain an ip subproperty that can be incompatible + // with the subnetwork/network we are transitioning to. Due to this we only change access + // configs if we notice the configuration (user intent) changes. accessConfigsHaveChanged := d.HasChange(prefix + ".access_config") updateCall := computeInstanceCreateUpdateWhileStoppedCall(d, config, networkInterfacePatchObj, networkInterface.AccessConfigs, accessConfigsHaveChanged, i, project, zone, userAgent, instance.Name) 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 84f6847ab802..ef933735d9ce 100644 --- a/third_party/terraform/tests/resource_compute_instance_test.go.erb +++ b/third_party/terraform/tests/resource_compute_instance_test.go.erb @@ -1954,7 +1954,7 @@ func TestComputeInstance_networkIPCustomizedDiff(t *testing.T) { NetworkIP: "b", }, }, - "NetworkIP and Subtwork change": { + "NetworkIP and Subnetwork change": { ExpectedForceNew: false, Before: NIBefore, After: NetworkInterface{ @@ -1964,7 +1964,7 @@ func TestComputeInstance_networkIPCustomizedDiff(t *testing.T) { NetworkIP: "b", }, }, - "NetworkIP and SubtworkProject change": { + "NetworkIP and SubnetworkProject change": { ExpectedForceNew: false, Before: NIBefore, After: NetworkInterface{ @@ -1974,7 +1974,7 @@ func TestComputeInstance_networkIPCustomizedDiff(t *testing.T) { NetworkIP: "b", }, }, - "All change ": { + "All change": { ExpectedForceNew: false, Before: NIBefore, After: NetworkInterface{ diff --git a/third_party/terraform/utils/compute_instance_network_interface_helpers.go b/third_party/terraform/utils/compute_instance_network_interface_helpers.go index 88a3a7595732..75ca18c19682 100644 --- a/third_party/terraform/utils/compute_instance_network_interface_helpers.go +++ b/third_party/terraform/utils/compute_instance_network_interface_helpers.go @@ -8,13 +8,12 @@ import ( computeBeta "google.golang.org/api/compute/v0.beta" ) +// TODO: This code deletes then recreates accessConfigs. This is bad because it may +// leave the machine inaccessible from either ip if the creation part fails (network +// timeout etc). However right now there is a GCE limit of 1 accessConfig so it is +// the only way to do it. In future this should be revised to only change what is +// necessary, and also add before removing. func computeInstanceDeleteAccessConfigs(d *schema.ResourceData, config *Config, instNetworkInterface *computeBeta.NetworkInterface, project, zone, userAgent, instanceName string) error { - // TODO: This code deletes then recreates accessConfigs. This is bad because it may - // leave the machine inaccessible from either ip if the creation part fails (network - // timeout etc). However right now there is a GCE limit of 1 accessConfig so it is - // the only way to do it. In future this should be revised to only change what is - // necessary, and also add before removing. - // Delete any accessConfig that currently exists in instNetworkInterface for _, ac := range instNetworkInterface.AccessConfigs { op, err := config.NewComputeClient(userAgent).Instances.DeleteAccessConfig( @@ -55,8 +54,11 @@ func computeInstanceCreateUpdateWhileStoppedCall(d *schema.ResourceData, config instNetworkInterface := instance.NetworkInterfaces[index] networkInterfacePatchObj.Fingerprint = instNetworkInterface.Fingerprint - // Access config can run into some issues since we can't derive users original intent due to - // terraform limitation. Lets only update it if we need to. + // Access config can run into some issues since we can't tell the difference between + // the users declared intent (config within their hcl file) and what we have inferred from the + // server (terraform state). Access configs contain an ip subproperty that can be incompatible + // with the subnetwork/network we are transitioning to. Due to this we only change access + // configs if we notice the configuration (user intent) changes. if accessConfigsHaveChanged { err := computeInstanceDeleteAccessConfigs(d, config, instNetworkInterface, project, zone, userAgent, instanceName) if err != nil { From a6adabc8210acca8ce1a3565230851cdd9f1910a Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Fri, 9 Oct 2020 10:58:36 -0700 Subject: [PATCH 23/25] find a comment a nice cozy new home on the hill side amongst all its comment friends. Truly truly a beautiful site to behold. Please be well and safe comment because the world needs you. YOU TOO are important --- .../terraform/resources/resource_compute_instance.go.erb | 6 ++++++ .../utils/compute_instance_network_interface_helpers.go | 5 ----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/third_party/terraform/resources/resource_compute_instance.go.erb b/third_party/terraform/resources/resource_compute_instance.go.erb index 1c5d5b425e1c..ee6649ced60a 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1422,6 +1422,12 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err } if !updateDuringStop && d.HasChange(prefix+".access_config") { + // TODO: This code deletes then recreates accessConfigs. This is bad because it may + // leave the machine inaccessible from either ip if the creation part fails (network + // timeout etc). However right now there is a GCE limit of 1 accessConfig so it is + // the only way to do it. In future this should be revised to only change what is + // necessary, and also add before removing. + // Delete current access configs err := computeInstanceDeleteAccessConfigs(d, config, instNetworkInterface, project, zone, userAgent, instance.Name) if err != nil { diff --git a/third_party/terraform/utils/compute_instance_network_interface_helpers.go b/third_party/terraform/utils/compute_instance_network_interface_helpers.go index 75ca18c19682..b8c72d4669a5 100644 --- a/third_party/terraform/utils/compute_instance_network_interface_helpers.go +++ b/third_party/terraform/utils/compute_instance_network_interface_helpers.go @@ -8,11 +8,6 @@ import ( computeBeta "google.golang.org/api/compute/v0.beta" ) -// TODO: This code deletes then recreates accessConfigs. This is bad because it may -// leave the machine inaccessible from either ip if the creation part fails (network -// timeout etc). However right now there is a GCE limit of 1 accessConfig so it is -// the only way to do it. In future this should be revised to only change what is -// necessary, and also add before removing. func computeInstanceDeleteAccessConfigs(d *schema.ResourceData, config *Config, instNetworkInterface *computeBeta.NetworkInterface, project, zone, userAgent, instanceName string) error { // Delete any accessConfig that currently exists in instNetworkInterface for _, ac := range instNetworkInterface.AccessConfigs { From d4ac363f43a7c61f20b02270d63a8895275a902d Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Fri, 9 Oct 2020 14:27:06 -0700 Subject: [PATCH 24/25] a comma boi --- .../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 ee6649ced60a..8aa8cbe8c973 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1473,7 +1473,7 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err } networkInterfacePatchObj := &computeBeta.NetworkInterface{ - AliasIpRanges: networkInterface.AliasIpRanges + AliasIpRanges: networkInterface.AliasIpRanges, Fingerprint: instNetworkInterface.Fingerprint } updateCall := config.NewComputeBetaClient(userAgent).Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterfacePatchObj).Do From 8c800ac5ca2180516ca0d74b27478dbaf0a05507 Mon Sep 17 00:00:00 2001 From: Scott Suarez Date: Mon, 12 Oct 2020 15:07:49 -0700 Subject: [PATCH 25/25] another commma for the party --- .../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 8aa8cbe8c973..4d59dc175d5d 100644 --- a/third_party/terraform/resources/resource_compute_instance.go.erb +++ b/third_party/terraform/resources/resource_compute_instance.go.erb @@ -1474,7 +1474,7 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err networkInterfacePatchObj := &computeBeta.NetworkInterface{ AliasIpRanges: networkInterface.AliasIpRanges, - Fingerprint: instNetworkInterface.Fingerprint + Fingerprint: instNetworkInterface.Fingerprint, } updateCall := config.NewComputeBetaClient(userAgent).Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterfacePatchObj).Do op, err := updateCall()