Skip to content

Commit

Permalink
Compute - Add update support for Network IP when changing network/sub…
Browse files Browse the repository at this point in the history 
…network (#4030)

* 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 <[email protected]>

* Update third_party/terraform/tests/resource_compute_instance_test.go.erb

Co-authored-by: Cameron Thornton <[email protected]>

* 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 <[email protected]>
  • Loading branch information
@ScottSuarez @c2thorn
ScottSuarez and c2thorn authored Oct 13, 2020
1 parent 5487250 commit 9055425
Show file tree
Hide file tree
Showing 5 changed files with 306 additions and 54 deletions.
136 changes: 83 additions & 53 deletions third_party/terraform/resources/resource_compute_instance.go.erb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,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,
Expand Down Expand Up @@ -256,7 +288,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.`,
},
Expand Down Expand Up @@ -717,6 +748,7 @@ func resourceComputeInstance() *schema.Resource {
suppressEmptyGuestAcceleratorDiff,
),
desiredStatusDiff,
forceNewIfNetworkIPNotUpdatable,
),
}
}
Expand Down Expand Up @@ -1347,7 +1379,8 @@ 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]
Expand Down Expand Up @@ -1388,50 +1421,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
Expand All @@ -1442,11 +1448,7 @@ 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") {
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 {
Expand All @@ -1470,8 +1472,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)
Expand All @@ -1480,9 +1485,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)
}
}
Expand Down Expand Up @@ -1748,14 +1774,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
}
}

Expand Down
137 changes: 136 additions & 1 deletion third_party/terraform/tests/resource_compute_instance_test.go.erb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1887,10 +1887,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]
Expand Down Expand Up @@ -4905,7 +5037,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"
Expand Down
Loading

0 comments on commit 9055425

Please sign in to comment.