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.

Sign up for GitHub

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_instance] - Allow updating of network and subnetwork properties #4011

Merged
merged 12 commits into from
Sep 25, 2020
Merged

[compute_instance] - Allow updating of network and subnetwork properties #4011

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b8c225e
[compute_instance] - Allow updating of network and subnetwork properties
ScottSuarez Sep 23, 2020
aafc4f1
fix spelling error
ScottSuarez Sep 23, 2020
464c82e
resolve comments
ScottSuarez Sep 24, 2020
f13e905
switch to use id over self link
ScottSuarez Sep 24, 2020
8099c39
remove unused var
ScottSuarez Sep 24, 2020
4987c92
resolve comments
ScottSuarez Sep 24, 2020
a1641a1
take into account fingerprint refresh before update call.
ScottSuarez Sep 24, 2020
314005e
spelling fix
ScottSuarez Sep 24, 2020
d21f704
Update third_party/terraform/resources/resource_compute_instance.go.erb
ScottSuarez Sep 25, 2020
ebf565d
Update third_party/terraform/resources/resource_compute_instance.go.erb
ScottSuarez Sep 25, 2020
a04588c
Add comment explaining subnetwork/network relationship. Changed test …
ScottSuarez Sep 25, 2020
1fbb1ef
Merge branch 'scsuarez/update-network-interface' of https://github.co…
ScottSuarez Sep 25, 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
120 changes: 87 additions & 33 deletions third_party/terraform/resources/resource_compute_instance.go.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/mitchellh/hashstructure"
computeBeta "google.golang.org/api/compute/v0.beta"
"google.golang.org/api/compute/v1"
"google.golang.org/api/googleapi"
)

var (
Expand Down Expand Up @@ -233,7 +234,6 @@ func resourceComputeInstance() *schema.Resource {
Type: schema.TypeString,
Optional: true,
Computed: true,
ForceNew: true,
DiffSuppressFunc: compareSelfLinkOrResourceName,
Description: `The name or self_link of the network attached to this interface.`,
},
Expand All @@ -242,7 +242,6 @@ func resourceComputeInstance() *schema.Resource {
Type: schema.TypeString,
Optional: true,
Computed: true,
ForceNew: true,
DiffSuppressFunc: compareSelfLinkOrResourceName,
Description: `The name or self_link of the subnetwork attached to this interface.`,
},
Expand All @@ -251,7 +250,6 @@ func resourceComputeInstance() *schema.Resource {
Type: schema.TypeString,
Optional: true,
Computed: true,
ForceNew: true,
Description: `The project in which the subnetwork belongs.`,
},

ScottSuarez marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -1329,21 +1327,57 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err
}
}

networkInterfacesCount := d.Get("network_interface.#").(int)
networkInterfaces, err := expandNetworkInterfaces(d, config)
if err != nil {
return fmt.Errorf("Error getting network interface from config: %s", err)
}

// Sanity check
if networkInterfacesCount != len(instance.NetworkInterfaces) {
if len(networkInterfaces) != len(instance.NetworkInterfaces) {
return fmt.Errorf("Instance had unexpected number of network interfaces: %d", len(instance.NetworkInterfaces))
}
for i := 0; i < networkInterfacesCount; i++ {

var updatesToNIWhileStopped []func(...googleapi.CallOption) (*computeBeta.Operation, error)
for i := 0; i < len(networkInterfaces); i++ {
prefix := fmt.Sprintf("network_interface.%d", i)
networkInterface := networkInterfaces[i]
instNetworkInterface := instance.NetworkInterfaces[i]

networkName := d.Get(prefix + ".name").(string)
subnetwork := networkInterface.Subnetwork
updateDuringStop := d.HasChange(prefix+".subnetwork") || d.HasChange(prefix+".network") || d.HasChange(prefix+".subnetwork_project")

// Sanity check
if networkName != instNetworkInterface.Name {
return fmt.Errorf("Instance networkInterface had unexpected name: %s", instNetworkInterface.Name)
}

// On creation the network is inferred if only subnetwork is given.
// Unforunately for us there is no way to determine if the user is
// explicitly asigning network or we are reusing the one that was inferred
// from state. So here we check if subnetwork changed and network did not.
// In the scenario we assume network was inferred and attempt to figure out
ScottSuarez marked this conversation as resolved.
Show resolved Hide resolved
// the new corresponding network.
ScottSuarez marked this conversation as resolved.
Show resolved Hide resolved

if d.HasChange(prefix + ".subnetwork") {
if !d.HasChange(prefix + ".network") {
ScottSuarez marked this conversation as resolved.
Show resolved Hide resolved
subnetProjectField := prefix + ".subnetwork_project"
sf, err := ParseSubnetworkFieldValueWithProjectField(subnetwork, subnetProjectField, d, config)
if err != nil {
return fmt.Errorf("Cannot determine self_link for subnetwork %q: %s", subnetwork, err)
}
resp, err := config.clientCompute.Subnetworks.Get(sf.Project, sf.Region, sf.Name).Do()
if err != nil {
return errwrap.Wrapf("Error getting subnetwork value: {{err}}", err)
}
nf, err := ParseNetworkFieldValue(resp.Network, d, config)
if err != nil {
return fmt.Errorf("Cannot determine self_link for network %q: %s", resp.Network, err)
}
networkInterface.Network = nf.RelativeLink()
}
}

if d.HasChange(prefix + ".access_config") {
ScottSuarez marked this conversation as resolved.
Show resolved Hide resolved

// TODO: This code deletes then recreates accessConfigs. This is bad because it may
Expand Down Expand Up @@ -1389,12 +1423,22 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err
return opErr
}
}
}

if d.HasChange(prefix + ".alias_ip_range") {
rereadFingerprint := false
// re-read fingerprint
instance, err = config.clientComputeBeta.Instances.Get(project, zone, instance.Name).Do()
if err != nil {
return err
}
instNetworkInterface = instance.NetworkInterfaces[i]
ScottSuarez marked this conversation as resolved.
Show resolved Hide resolved
}

// 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
ScottSuarez marked this conversation as resolved.
Show resolved Hide resolved
if len(instNetworkInterface.AliasIpRanges) > 0 {
ni := &computeBeta.NetworkInterface{
Fingerprint: instNetworkInterface.Fingerprint,
Expand All @@ -1408,31 +1452,29 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err
if opErr != nil {
return opErr
}
rereadFingerprint = true
}

ranges := d.Get(prefix + ".alias_ip_range").([]interface{})
if len(ranges) > 0 {
if rereadFingerprint {
instance, err = config.clientComputeBeta.Instances.Get(project, zone, instance.Name).Do()
if err != nil {
return err
}
instNetworkInterface = instance.NetworkInterfaces[i]
}
ni := &computeBeta.NetworkInterface{
AliasIpRanges: expandAliasIpRanges(ranges),
Fingerprint: instNetworkInterface.Fingerprint,
}
op, err := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, ni).Do()
// re-read fingerprint
instance, err = config.clientComputeBeta.Instances.Get(project, zone, instance.Name).Do()
if err != nil {
return errwrap.Wrapf("Error adding alias_ip_range: {{err}}", err)
}
opErr := computeOperationWaitTime(config, op, project, "updating alias ip ranges", d.Timeout(schema.TimeoutUpdate))
if opErr != nil {
return opErr
return err
}
instNetworkInterface = instance.NetworkInterfaces[i]
}

networkInterface.Fingerprint = instNetworkInterface.Fingerprint
updateCall := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterface).Do
op, err := updateCall()
ScottSuarez marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return errwrap.Wrapf("Error updating network interface: {{err}}", err)
}
opErr := computeOperationWaitTime(config, op, project, "network interface to update", d.Timeout(schema.TimeoutUpdate))
if opErr != nil {
return opErr
}
} else {

networkInterface.Fingerprint = instNetworkInterface.Fingerprint
updateCall := config.clientComputeBeta.Instances.UpdateNetworkInterface(project, zone, instance.Name, networkName, networkInterface).Do
updatesToNIWhileStopped = append(updatesToNIWhileStopped, updateCall)
}
}

Expand Down Expand Up @@ -1558,7 +1600,7 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err
}
}

needToStopInstanceBeforeUpdating := scopesChange || d.HasChange("service_account.0.email") || d.HasChange("machine_type") || d.HasChange("min_cpu_platform") || d.HasChange("enable_display") || d.HasChange("shielded_instance_config")
needToStopInstanceBeforeUpdating := scopesChange || d.HasChange("service_account.0.email") || d.HasChange("machine_type") || d.HasChange("min_cpu_platform") || d.HasChange("enable_display") || d.HasChange("shielded_instance_config") || len(updatesToNIWhileStopped) > 0

if d.HasChange("desired_status") && !needToStopInstanceBeforeUpdating {
desiredStatus := d.Get("desired_status").(string)
Expand Down Expand Up @@ -1592,7 +1634,8 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err
desiredStatus := d.Get("desired_status").(string)

if statusBeforeUpdate == "RUNNING" && desiredStatus != "TERMINATED" && !d.Get("allow_stopping_for_update").(bool) {
return fmt.Errorf("Changing the machine_type, min_cpu_platform, service_account, enable_display, or shielded_instance_config on a started instance requires stopping it. " +
return fmt.Errorf("Changing the machine_type, min_cpu_platform, service_account, enable_display, shielded_instance_config, " +
"or network_interface.[#d].(network/subnetwork/subnetwork_project) on a started instance requires stopping it. " +
"To acknowledge this, please set allow_stopping_for_update = true in your config. " +
"You can also stop it by setting desired_status = \"TERMINATED\", but the instance will not be restarted after the update.")
}
Expand Down Expand Up @@ -1696,6 +1739,17 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err
}
}

for _, updateCall := range updatesToNIWhileStopped {
op, err := updateCall()
if err != nil {
return errwrap.Wrapf("Error updating network interface: {{err}}", err)
}
opErr := computeOperationWaitTime(config, op, project, "network interface to update", d.Timeout(schema.TimeoutUpdate))
if opErr != nil {
return opErr
}
}

if (statusBeforeUpdate == "RUNNING" && desiredStatus != "TERMINATED") ||
(statusBeforeUpdate == "TERMINATED" && desiredStatus == "RUNNING") {
op, err := startInstanceOperation(d, config)
Expand Down
171 changes: 171 additions & 0 deletions third_party/terraform/tests/resource_compute_instance_test.go.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1876,6 +1876,28 @@ func TestAccComputeInstance_resourcePolicyCollocate(t *testing.T) {
})
}

func TestAccComputeInstance_subnetworkChange(t *testing.T) {
ScottSuarez marked this conversation as resolved.
Show resolved Hide resolved
t.Parallel()
instanceName := fmt.Sprintf("tf-test-%s", randString(t, 10))
suffix := fmt.Sprintf("%s", randString(t, 10))

vcrTest(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t) },
Providers: testAccProviders,
CheckDestroy: testAccCheckComputeInstanceDestroyProducer(t),
Steps: []resource.TestStep{
{
Config: testAccComputeInstance_subnetworkChange(suffix, instanceName),
danawillow marked this conversation as resolved.
Show resolved Hide resolved
},
computeInstanceImportStep("us-east1-d", instanceName, []string{"allow_stopping_for_update"}),
{
Config: testAccComputeInstance_subnetworkChangeUpdate(suffix, instanceName),
},
computeInstanceImportStep("us-east1-d", instanceName, []string{"allow_stopping_for_update"}),
},
})
}

func testAccCheckComputeInstanceUpdateMachineType(t *testing.T, n string) resource.TestCheckFunc {
return func(s *terraform.State) error {
rs, ok := s.RootModule().Resources[n]
Expand Down Expand Up @@ -4750,3 +4772,152 @@ resource "google_compute_resource_policy" "foo" {

`, instance, instance, suffix)
}

func testAccComputeInstance_subnetworkChange(suffix, instance string) string {
return fmt.Sprintf(`
data "google_compute_image" "my_image" {
family = "debian-9"
project = "debian-cloud"
}

resource "google_compute_network" "inst-test-network" {
ScottSuarez marked this conversation as resolved.
Show resolved Hide resolved
name = "tf-test-network-%s"
auto_create_subnetworks = false
}

resource "google_compute_network" "inst-test-network2" {
name = "tf-test-network2-%s"
auto_create_subnetworks = false
}

resource "google_compute_subnetwork" "inst-test-subnetwork" {
name = "tf-test-compute-subnet-%s"
ip_cidr_range = "10.0.0.0/16"
region = "us-east1"
network = google_compute_network.inst-test-network.id
secondary_ip_range {
range_name = "inst-test-secondary"
ip_cidr_range = "172.16.0.0/20"
}
secondary_ip_range {
range_name = "inst-test-tertiary"
ip_cidr_range = "10.1.0.0/16"
}
}

resource "google_compute_subnetwork" "inst-test-subnetwork2" {
name = "tf-test-compute-subnet2-%s"
ip_cidr_range = "10.3.0.0/16"
region = "us-east1"
network = google_compute_network.inst-test-network2.id
secondary_ip_range {
range_name = "inst-test-secondary2"
ip_cidr_range = "173.16.0.0/20"
}
secondary_ip_range {
range_name = "inst-test-tertiary2"
ip_cidr_range = "10.4.0.0/16"
}
}

resource "google_compute_instance" "foobar" {
name = "%s"
machine_type = "n1-standard-1"
zone = "us-east1-d"
allow_stopping_for_update = true

boot_disk {
initialize_params {
image = data.google_compute_image.my_image.id
}
}

network_interface {
subnetwork = google_compute_subnetwork.inst-test-subnetwork.id
access_config {
network_tier = "STANDARD"
}
alias_ip_range {
subnetwork_range_name = google_compute_subnetwork.inst-test-subnetwork.secondary_ip_range[0].range_name
ip_cidr_range = "172.16.0.0/24"
}

alias_ip_range {
subnetwork_range_name = google_compute_subnetwork.inst-test-subnetwork.secondary_ip_range[1].range_name
ip_cidr_range = "10.1.0.0/20"
}
}
}
`, suffix, suffix, suffix, suffix, instance)
}

func testAccComputeInstance_subnetworkChangeUpdate(suffix, instance string) string {
return fmt.Sprintf(`
data "google_compute_image" "my_image" {
family = "debian-9"
project = "debian-cloud"
}

resource "google_compute_network" "inst-test-network" {
name = "tf-test-network-%s"
auto_create_subnetworks = false
}

resource "google_compute_network" "inst-test-network2" {
name = "tf-test-network2-%s"
auto_create_subnetworks = false
}

resource "google_compute_subnetwork" "inst-test-subnetwork" {
name = "tf-test-compute-subnet-%s"
ip_cidr_range = "10.0.0.0/16"
region = "us-east1"
network = google_compute_network.inst-test-network.id
secondary_ip_range {
range_name = "inst-test-secondary"
ip_cidr_range = "172.16.0.0/20"
}
secondary_ip_range {
range_name = "inst-test-tertiary"
ip_cidr_range = "10.1.0.0/16"
}
}

resource "google_compute_subnetwork" "inst-test-subnetwork2" {
name = "tf-test-compute-subnet2-%s"
ip_cidr_range = "10.3.0.0/16"
region = "us-east1"
network = google_compute_network.inst-test-network2.id
secondary_ip_range {
range_name = "inst-test-secondary2"
ip_cidr_range = "173.16.0.0/20"
}
secondary_ip_range {
range_name = "inst-test-tertiary2"
ip_cidr_range = "10.4.0.0/16"
}
}

resource "google_compute_instance" "foobar" {
name = "%s"
machine_type = "n1-standard-1"
zone = "us-east1-d"
allow_stopping_for_update = true

boot_disk {
initialize_params {
image = data.google_compute_image.my_image.id
}
}

network_interface {
subnetwork = google_compute_subnetwork.inst-test-subnetwork2.id

alias_ip_range {
subnetwork_range_name = google_compute_subnetwork.inst-test-subnetwork2.secondary_ip_range[0].range_name
ip_cidr_range = "173.16.0.0/24"
}
}
}
`, suffix, suffix, suffix, suffix, instance)
}