From c3dbb2d6a33163dccf9de5ae77a02e14ca4e0300 Mon Sep 17 00:00:00 2001 From: The Magician Date: Tue, 13 Oct 2020 15:35:01 -0700 Subject: [PATCH] Compute - Add update support for Network IP when changing network/subnetwork (#4030) (#7515) * Compute - Add update support for Network IP when changing network/subnetwork * add field required by beta provider * error check for d.ForceNew * refactor functions into own file. introduce network-interface-helper * refresh instance after deleting alias config * correct bad merge * spelling fix * check error * Add forcenew unit test * resolve build issue * Update third_party/terraform/tests/resource_compute_instance_test.go.erb Co-authored-by: Cameron Thornton * Update third_party/terraform/tests/resource_compute_instance_test.go.erb Co-authored-by: Cameron Thornton * error couldn't convert to string * to type string * Refactored some test code for cleaner style and condensed some if statements * resolve merge issue with megan's change * update network interface helper to promote better go readability * fixed instances where incorrect name was referenced * removed extraneous dependencies and extrapolated self mutating operations * scrap refactor. only extrapolate functions when needed. * error check * resolved some fomatting and comment concerns. * 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 * a comma boi * another commma for the party Co-authored-by: Cameron Thornton Signed-off-by: Modular Magician Co-authored-by: Cameron Thornton --- .changelog/4030.txt | 4 + ...pute_instance_network_interface_helpers.go | 81 +++++++++++ google/resource_compute_instance.go | 134 ++++++++++------- google/resource_compute_instance_test.go | 137 +++++++++++++++++- google/test_utils.go | 5 + google/utils.go | 1 + 6 files changed, 308 insertions(+), 54 deletions(-) create mode 100644 .changelog/4030.txt create mode 100644 google/compute_instance_network_interface_helpers.go diff --git a/.changelog/4030.txt b/.changelog/4030.txt new file mode 100644 index 00000000000..494e18fe388 --- /dev/null +++ b/.changelog/4030.txt @@ -0,0 +1,4 @@ +```release-note:enhancement +compute: added support for updating `network_interface.[d].network_ip` on `google_compute_instance` when changing network or subnetwork + +``` diff --git a/google/compute_instance_network_interface_helpers.go b/google/compute_instance_network_interface_helpers.go new file mode 100644 index 00000000000..b8c72d4669a --- /dev/null +++ b/google/compute_instance_network_interface_helpers.go @@ -0,0 +1,81 @@ +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" +) + +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 { + 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 := computeOperationWaitTime(config, op, project, "old access_config to delete", userAgent, d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr + } + } + return nil +} + +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 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 := computeOperationWaitTime(config, op, project, "new access_config to add", userAgent, d.Timeout(schema.TimeoutUpdate)) + 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 { + + // 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 { + + instNetworkInterface := instance.NetworkInterfaces[index] + networkInterfacePatchObj.Fingerprint = instNetworkInterface.Fingerprint + + // 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 { + return err + } + } + + 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 := computeOperationWaitTime(config, op, project, "network interface to update", userAgent, d.Timeout(schema.TimeoutUpdate)) + if opErr != nil { + return opErr + } + + if accessConfigsHaveChanged { + err := computeInstanceAddAccessConfigs(d, config, instNetworkInterface, accessConfigs, project, zone, userAgent, instanceName) + if err != nil { + return err + } + } + return nil + } +} diff --git a/google/resource_compute_instance.go b/google/resource_compute_instance.go index 360cc446261..8a64f028828 100644 --- a/google/resource_compute_instance.go +++ b/google/resource_compute_instance.go @@ -19,7 +19,6 @@ import ( "github.com/mitchellh/hashstructure" computeBeta "google.golang.org/api/compute/v0.beta" "google.golang.org/api/compute/v1" - "google.golang.org/api/googleapi" ) var ( @@ -54,6 +53,38 @@ var ( } ) +// 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 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 + } + + 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) { + if err := d.ForceNew(networkIPKey); err != nil { + return err + } + } + } + } + + return nil +} + func resourceComputeInstance() *schema.Resource { return &schema.Resource{ Create: resourceComputeInstanceCreate, @@ -252,7 +283,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.`, }, @@ -687,6 +717,7 @@ func resourceComputeInstance() *schema.Resource { suppressEmptyGuestAcceleratorDiff, ), desiredStatusDiff, + forceNewIfNetworkIPNotUpdatable, ), } } @@ -1309,7 +1340,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(inst *computeBeta.Instance) error for i := 0; i < len(networkInterfaces); i++ { prefix := fmt.Sprintf("network_interface.%d", i) networkInterface := networkInterfaces[i] @@ -1350,50 +1381,23 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err } } - if d.HasChange(prefix + ".access_config") { - + 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 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 - } + // Delete current access configs + err := computeInstanceDeleteAccessConfigs(d, config, instNetworkInterface, project, zone, userAgent, instance.Name) + if err != nil { + return err } // 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) - } - - 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 - } + err = computeInstanceAddAccessConfigs(d, config, instNetworkInterface, networkInterface.AccessConfigs, project, zone, userAgent, instance.Name) + if err != nil { + return err } // re-read fingerprint @@ -1404,10 +1408,6 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err instNetworkInterface = instance.NetworkInterfaces[i] } - // Setting NetworkIP to empty and AccessConfigs to nil. - // This will opt them out from being modified in the patch call. - 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 @@ -1432,8 +1432,11 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err instNetworkInterface = instance.NetworkInterfaces[i] } - networkInterface.Fingerprint = instNetworkInterface.Fingerprint - updateCall := config.NewComputeBetaClient(userAgent).Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterface).Do + 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 { return errwrap.Wrapf("Error updating network interface: {{err}}", err) @@ -1442,9 +1445,30 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err if opErr != nil { return opErr } - } else if updateDuringStop { - networkInterface.Fingerprint = instNetworkInterface.Fingerprint - updateCall := config.NewComputeBetaClient(userAgent).Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterface).Do + } + + 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 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) updatesToNIWhileStopped = append(updatesToNIWhileStopped, updateCall) } } @@ -1710,14 +1734,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.NewComputeBetaClient(userAgent).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", userAgent, d.Timeout(schema.TimeoutUpdate)) - if opErr != nil { - return opErr + } + for _, patch := range updatesToNIWhileStopped { + err := patch(instance) + if err != nil { + return err } } diff --git a/google/resource_compute_instance_test.go b/google/resource_compute_instance_test.go index a3bfa4df53b..446475a57f5 100644 --- a/google/resource_compute_instance_test.go +++ b/google/resource_compute_instance_test.go @@ -1861,10 +1861,142 @@ 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"}), }, }) } +func TestComputeInstance_networkIPCustomizedDiff(t *testing.T) { + t.Parallel() + + d := &ResourceDiffMock{ + Before: map[string]interface{}{ + "network_interface.#": 0, + }, + After: map[string]interface{}{ + "network_interface.#": 1, + }, + } + + err := forceNewIfNetworkIPNotUpdatableFunc(d) + if err != nil { + t.Error(err) + } + + if d.IsForceNew { + 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 { + ExpectedForceNew bool + Before NetworkInterface + After NetworkInterface + }{ + "NetworkIP only change": { + ExpectedForceNew: true, + Before: NIBefore, + After: NetworkInterface{ + Network: "a", + Subnetwork: "a", + SubnetworkProject: "a", + NetworkIP: "b", + }, + }, + "NetworkIP and Network change": { + ExpectedForceNew: false, + Before: NIBefore, + After: NetworkInterface{ + Network: "b", + Subnetwork: "a", + SubnetworkProject: "a", + NetworkIP: "b", + }, + }, + "NetworkIP and Subnetwork change": { + ExpectedForceNew: false, + Before: NIBefore, + After: NetworkInterface{ + Network: "a", + Subnetwork: "b", + SubnetworkProject: "a", + NetworkIP: "b", + }, + }, + "NetworkIP and SubnetworkProject change": { + ExpectedForceNew: false, + Before: NIBefore, + After: NetworkInterface{ + Network: "a", + Subnetwork: "a", + SubnetworkProject: "b", + NetworkIP: "b", + }, + }, + "All change": { + ExpectedForceNew: false, + Before: NIBefore, + After: NetworkInterface{ + Network: "b", + Subnetwork: "b", + SubnetworkProject: "b", + NetworkIP: "b", + }, + }, + "No change": { + ExpectedForceNew: false, + Before: NIBefore, + After: NetworkInterface{ + Network: "a", + Subnetwork: "a", + SubnetworkProject: "a", + NetworkIP: "a", + }, + }, + } + + for tn, tc := range cases { + d := &ResourceDiffMock{ + Before: map[string]interface{}{ + "network_interface.#": 1, + "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.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.Error(err) + } + if tc.ExpectedForceNew != d.IsForceNew { + t.Errorf("%v: expected d.IsForceNew to be %v, but was %v", tn, tc.ExpectedForceNew, d.IsForceNew) + } + } +} + func testAccCheckComputeInstanceUpdateMachineType(t *testing.T, n string) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[n] @@ -4816,7 +4948,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/google/test_utils.go b/google/test_utils.go index 8eb6f2de708..2f4fa5d79de 100644 --- a/google/test_utils.go +++ b/google/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/google/utils.go b/google/utils.go index c67cdd2ebac..f812bc8d922 100644 --- a/google/utils.go +++ b/google/utils.go @@ -25,6 +25,7 @@ type TerraformResourceData interface { } type TerraformResourceDiff interface { + HasChange(string) bool GetChange(string) (interface{}, interface{}) Get(string) interface{} Clear(string) error