Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Compute - Add update support for Network IP when changing network/subnetwork #4030

Merged
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
8fa2b40
Compute - Add update support for Network IP when changing network/sub…
ScottSuarez Sep 29, 2020
b81129f
merge with master
ScottSuarez Sep 29, 2020
c758f19
add field required by beta provider
ScottSuarez Sep 29, 2020
be26563
error check for d.ForceNew
ScottSuarez Sep 29, 2020
d43335b
refactor functions into own file. introduce network-interface-helper
ScottSuarez Sep 29, 2020
8de60c0
refresh instance after deleting alias config
ScottSuarez Sep 30, 2020
305fad0
merge with master
ScottSuarez Sep 30, 2020
d6a1176
correct bad merge
ScottSuarez Sep 30, 2020
465b8d5
spelling fix
ScottSuarez Sep 30, 2020
566c7ba
check error
ScottSuarez Sep 30, 2020
849d0c6
Add forcenew unit test
ScottSuarez Sep 30, 2020
cf00d31
resolve build issue
ScottSuarez Sep 30, 2020
0880d41
Update third_party/terraform/tests/resource_compute_instance_test.go.erb
ScottSuarez Sep 30, 2020
f6160ea
Update third_party/terraform/tests/resource_compute_instance_test.go.erb
ScottSuarez Sep 30, 2020
09c87d9
error couldn't convert to string
ScottSuarez Sep 30, 2020
55a87da
Merge branch 'compute_instance_network_ip' of https://github.com/Scot…
ScottSuarez Sep 30, 2020
f613684
to type string
ScottSuarez Sep 30, 2020
7eb0af6
Refactored some test code for cleaner style and condensed some if sta…
ScottSuarez Oct 1, 2020
9877285
merge with megan's changes
ScottSuarez Oct 1, 2020
f340254
resolve merge issue with megan's change
ScottSuarez Oct 1, 2020
6c0e819
update network interface helper to promote better go readability
ScottSuarez Oct 2, 2020
1f1a506
fixed instances where incorrect name was referenced
ScottSuarez Oct 2, 2020
903ffe6
Merge branch 'master' of https://github.com/GoogleCloudPlatform/magic…
ScottSuarez Oct 6, 2020
17a4d83
removed extraneous dependencies and extrapolated self mutating operat…
ScottSuarez Oct 7, 2020
9963199
Merge branch 'master' of https://github.com/GoogleCloudPlatform/magic…
ScottSuarez Oct 9, 2020
8cf7348
scrap refactor. only extrapolate functions when needed.
ScottSuarez Oct 9, 2020
32e332e
error check
ScottSuarez Oct 9, 2020
a2d114f
resolved some fomatting and comment concerns.
ScottSuarez Oct 9, 2020
a6adabc
find a comment a nice cozy new home on the hill side amongst all its …
ScottSuarez Oct 9, 2020
d4ac363
a comma boi
ScottSuarez Oct 9, 2020
29ee2ff
Merge branch 'master' of https://github.com/GoogleCloudPlatform/magic…
ScottSuarez Oct 12, 2020
8c800ac
another commma for the party
ScottSuarez Oct 12, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 83 additions & 59 deletions third_party/terraform/resources/resource_compute_instance.go.erb
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) {
ScottSuarez marked this conversation as resolved.
Show resolved Hide resolved
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,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.

// 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
}
if !updateDuringStop && d.HasChange(prefix+".access_config") {
// Delete current access configs
ScottSuarez marked this conversation as resolved.
Show resolved Hide resolved
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 +1442,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 +1466,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 +1479,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 +1768,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 {
ScottSuarez marked this conversation as resolved.
Show resolved Hide resolved
return err
}
}

Expand Down
137 changes: 136 additions & 1 deletion third_party/terraform/tests/resource_compute_instance_test.go.erb
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