Changed order of updates for min_cpu_platform and machine_type (hashi…

…corp#8249) * Changed order of updates for min_cpu_platform and machine_type Resolved hashicorp#14945 * Added allow_stopping_for_update = true * Added allow_stopping_for_update to ImportStateVerifyIgnore * Added diff suppress and clarified how to 'unset' the field Signed-off-by: Modular Magician <[email protected]>
modular-magician · Jul 18, 2023 · 86e5d18 · 86e5d18
1 parent 1d483ff
commit 86e5d18
Show file tree

Hide file tree

Showing 4 changed files with 137 additions and 26 deletions.
diff --git a/.changelog/8249.txt b/.changelog/8249.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+compute: fixed logic when unsetting `google_compute_instance.min_cpu_platform` and switching to a `machine_type` that does not support `min_cpu_platform` at the same time
+```
diff --git a/google/resource_compute_instance_test.go b/google/resource_compute_instance_test.go
@@ -22,6 +22,74 @@ import (
 	"google.golang.org/api/compute/v1"
 )
 
+func TestMinCpuPlatformDiffSuppress(t *testing.T) {
+	cases := map[string]struct {
+		Old, New           string
+		ExpectDiffSuppress bool
+	}{
+		"state: empty, conf: AUTOMATIC": {
+			Old:                "",
+			New:                "AUTOMATIC",
+			ExpectDiffSuppress: true,
+		},
+		"state: empty, conf: automatic": {
+			Old:                "",
+			New:                "automatic",
+			ExpectDiffSuppress: true,
+		},
+		"state: empty, conf: AuToMaTiC": {
+			Old:                "",
+			New:                "AuToMaTiC",
+			ExpectDiffSuppress: true,
+		},
+		"state: empty, conf: Intel Haswell": {
+			Old:                "",
+			New:                "Intel Haswell",
+			ExpectDiffSuppress: false,
+		},
+		// This case should never happen due to the field being
+		// Optional + Computed; however, including for completeness.
+		"state: Intel Haswell, conf: empty": {
+			Old:                "Intel Haswell",
+			New:                "",
+			ExpectDiffSuppress: false,
+		},
+		// These cases should never happen given current API behavior; testing
+		// in case API behavior changes in the future.
+		"state: AUTOMATIC, conf: Intel Haswell": {
+			Old:                "AUTOMATIC",
+			New:                "Intel Haswell",
+			ExpectDiffSuppress: false,
+		},
+		"state: Intel Haswell, conf: AUTOMATIC": {
+			Old:                "Intel Haswell",
+			New:                "AUTOMATIC",
+			ExpectDiffSuppress: false,
+		},
+		"state: AUTOMATIC, conf: empty": {
+			Old:                "AUTOMATIC",
+			New:                "",
+			ExpectDiffSuppress: true,
+		},
+		"state: automatic, conf: empty": {
+			Old:                "automatic",
+			New:                "",
+			ExpectDiffSuppress: true,
+		},
+		"state: AuToMaTiC, conf: empty": {
+			Old:                "AuToMaTiC",
+			New:                "",
+			ExpectDiffSuppress: true,
+		},
+	}
+
+	for tn, tc := range cases {
+		if tpgcompute.ComputeInstanceMinCpuPlatformEmptyOrAutomaticDiffSuppress("min_cpu_platform", tc.Old, tc.New, nil) != tc.ExpectDiffSuppress {
+			t.Errorf("bad: %s, %q => %q expect DiffSuppress to return %t", tn, tc.Old, tc.New, tc.ExpectDiffSuppress)
+		}
+	}
+}
+
 func computeInstanceImportStep(zone, instanceName string, additionalImportIgnores []string) resource.TestStep {
 	// metadata is only read into state if set in the config
 	// importing doesn't know whether metadata.startup_script vs metadata_startup_script is set in the config,
@@ -1430,7 +1498,15 @@ func TestAccComputeInstance_minCpuPlatform(t *testing.T) {
 					testAccCheckComputeInstanceHasMinCpuPlatform(&instance, "Intel Haswell"),
 				),
 			},
-			computeInstanceImportStep("us-east1-d", instanceName, []string{}),
+			computeInstanceImportStep("us-east1-d", instanceName, []string{"allow_stopping_for_update"}),
+			{
+				Config: testAccComputeInstance_minCpuPlatform_remove(instanceName),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckComputeInstanceExists(t, "google_compute_instance.foobar", &instance),
+					testAccCheckComputeInstanceHasMinCpuPlatform(&instance, ""),
+				),
+			},
+			computeInstanceImportStep("us-east1-d", instanceName, []string{"allow_stopping_for_update"}),
 		},
 	})
 }
@@ -5212,6 +5288,35 @@ resource "google_compute_instance" "foobar" {
   }
 
   min_cpu_platform = "Intel Haswell"
+  allow_stopping_for_update = true
+}
+`, instance)
+}
+
+func testAccComputeInstance_minCpuPlatform_remove(instance string) string {
+	return fmt.Sprintf(`
+data "google_compute_image" "my_image" {
+  family  = "debian-11"
+  project = "debian-cloud"
+}
+
+resource "google_compute_instance" "foobar" {
+  name         = "%s"
+  machine_type = "e2-micro"
+  zone         = "us-east1-d"
+
+  boot_disk {
+    initialize_params {
+      image = data.google_compute_image.my_image.self_link
+    }
+  }
+
+  network_interface {
+    network = "default"
+  }
+
+  min_cpu_platform = "AuToMaTiC"
+  allow_stopping_for_update = true
 }
 `, instance)
 }

diff --git a/google/services/compute/resource_compute_instance.go b/google/services/compute/resource_compute_instance.go
@@ -94,6 +94,14 @@ func forceNewIfNetworkIPNotUpdatableFunc(d tpgresource.TerraformResourceDiff) er
 	return nil
 }
 
+// User may specify AUTOMATIC using any case; the API will accept it and return an empty string.
+func ComputeInstanceMinCpuPlatformEmptyOrAutomaticDiffSuppress(k, old, new string, d *schema.ResourceData) bool {
+	old = strings.ToLower(old)
+	new = strings.ToLower(new)
+	defaultVal := "automatic"
+	return (old == "" && new == defaultVal) || (new == "" && old == defaultVal)
+}
+
 func ResourceComputeInstance() *schema.Resource {
 	return &schema.Resource{
 		Create: resourceComputeInstanceCreate,
@@ -588,10 +596,11 @@ func ResourceComputeInstance() *schema.Resource {
 			},
 
 			"min_cpu_platform": {
-				Type:        schema.TypeString,
-				Optional:    true,
-				Computed:    true,
-				Description: `The minimum CPU platform specified for the VM instance.`,
+				Type:             schema.TypeString,
+				Optional:         true,
+				Computed:         true,
+				Description:      `The minimum CPU platform specified for the VM instance.`,
+				DiffSuppressFunc: ComputeInstanceMinCpuPlatformEmptyOrAutomaticDiffSuppress,
 			},
 
 			"project": {
@@ -1965,40 +1974,34 @@ func resourceComputeInstanceUpdate(d *schema.ResourceData, meta interface{}) err
 			}
 		}
 
-		if d.HasChange("machine_type") {
-			mt, err := tpgresource.ParseMachineTypesFieldValue(d.Get("machine_type").(string), d, config)
-			if err != nil {
-				return err
-			}
-			req := &compute.InstancesSetMachineTypeRequest{
-				MachineType: mt.RelativeLink(),
+		if d.HasChange("min_cpu_platform") {
+			minCpuPlatform := d.Get("min_cpu_platform")
+			req := &compute.InstancesSetMinCpuPlatformRequest{
+				MinCpuPlatform: minCpuPlatform.(string),
 			}
-			op, err := config.NewComputeClient(userAgent).Instances.SetMachineType(project, zone, instance.Name, req).Do()
+			op, err := config.NewComputeClient(userAgent).Instances.SetMinCpuPlatform(project, zone, instance.Name, req).Do()
 			if err != nil {
 				return err
 			}
-			opErr := ComputeOperationWaitTime(config, op, project, "updating machinetype", userAgent, d.Timeout(schema.TimeoutUpdate))
+			opErr := ComputeOperationWaitTime(config, op, project, "updating min cpu platform", userAgent, d.Timeout(schema.TimeoutUpdate))
 			if opErr != nil {
 				return opErr
 			}
 		}
 
-		if d.HasChange("min_cpu_platform") {
-			minCpuPlatform, ok := d.GetOk("min_cpu_platform")
-			// Even though you don't have to set minCpuPlatform on create, you do have to set it to an
-			// actual value on update. "Automatic" is the default. This will be read back from the API as empty,
-			// so we don't need to worry about diffs.
-			if !ok {
-				minCpuPlatform = "Automatic"
+		if d.HasChange("machine_type") {
+			mt, err := tpgresource.ParseMachineTypesFieldValue(d.Get("machine_type").(string), d, config)
+			if err != nil {
+				return err
 			}
-			req := &compute.InstancesSetMinCpuPlatformRequest{
-				MinCpuPlatform: minCpuPlatform.(string),
+			req := &compute.InstancesSetMachineTypeRequest{
+				MachineType: mt.RelativeLink(),
 			}
-			op, err := config.NewComputeClient(userAgent).Instances.SetMinCpuPlatform(project, zone, instance.Name, req).Do()
+			op, err := config.NewComputeClient(userAgent).Instances.SetMachineType(project, zone, instance.Name, req).Do()
 			if err != nil {
 				return err
 			}
-			opErr := ComputeOperationWaitTime(config, op, project, "updating min cpu platform", userAgent, d.Timeout(schema.TimeoutUpdate))
+			opErr := ComputeOperationWaitTime(config, op, project, "updating machinetype", userAgent, d.Timeout(schema.TimeoutUpdate))
 			if opErr != nil {
 				return opErr
 			}

diff --git a/website/docs/d/compute_instance.html.markdown b/website/docs/d/compute_instance.html.markdown
@@ -61,7 +61,7 @@ The following arguments are supported:
 
 * `metadata` - Metadata key/value pairs made available within the instance.
 
-* `min_cpu_platform` - The minimum CPU platform specified for the VM instance.
+* `min_cpu_platform` - The minimum CPU platform specified for the VM instance. Set to "AUTOMATIC" to remove a previously-set value.
 
 * `scheduling` - The scheduling strategy being used by the instance. Structure is [documented below](#nested_scheduling)