allow updating dataproc cluster autoscaling_policy (#3976) (#2483)

Signed-off-by: Modular Magician <[email protected]>

.changelog/3976.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+dataproc: fixed issues where updating `google_dataproc_cluster.cluster_config.autoscaling_policy` would do nothing, and where there was no way to remove a policy.
+```

google-beta/common_diff_suppress.go

@@ -5,6 +5,7 @@ package google
 import (
 	"crypto/sha256"
 	"encoding/hex"
+	"reflect"
 	"strings"
 	"time"
 
@@ -77,6 +78,42 @@ func rfc3339TimeDiffSuppress(k, old, new string, d *schema.ResourceData) bool {
 	return false
 }
 
+// Suppress diffs for blocks where one version is completely unset and the other is set
+// to an empty block. This might occur in situations where removing a block completely
+// is impossible (if it's computed or part of an AtLeastOneOf), so instead the user sets
+// its values to empty.
+func emptyOrUnsetBlockDiffSuppress(k, old, new string, d *schema.ResourceData) bool {
+	o, n := d.GetChange(strings.TrimSuffix(k, ".#"))
+	var l []interface{}
+	if old == "0" && new == "1" {
+		l = n.([]interface{})
+	} else if new == "0" && old == "1" {
+		l = o.([]interface{})
+	} else {
+		// we don't have one set and one unset, so don't suppress the diff
+		return false
+	}
+
+	contents := l[0].(map[string]interface{})
+	for _, v := range contents {
+		if !isEmptyValue(reflect.ValueOf(v)) {
+			return false
+		}
+	}
+	return true
+}
+
+// Suppress diffs for values that are equivalent except for their use of the words "location"
+// compared to "region" or "zone"
+func locationDiffSuppress(k, old, new string, d *schema.ResourceData) bool {
+	return locationDiffSuppressHelper(old, new) || locationDiffSuppressHelper(new, old)
+}
+
+func locationDiffSuppressHelper(a, b string) bool {
+	return strings.Replace(a, "/locations/", "/regions/", 1) == b ||
+		strings.Replace(a, "/locations/", "/zones/", 1) == b
+}
+
 // For managed SSL certs, if new is an absolute FQDN (trailing '.') but old isn't, treat them as equals.
 func absoluteDomainSuppress(k, old, new string, _ *schema.ResourceData) bool {
 	if strings.HasPrefix(k, "managed.0.domains.") {

google-beta/resource_dataproc_cluster.go

@@ -527,17 +527,19 @@ by Dataproc`,
 							},
 						},
 						"autoscaling_config": {
-							Type:         schema.TypeList,
-							Optional:     true,
-							AtLeastOneOf: clusterConfigKeys,
-							MaxItems:     1,
-							Description:  `The autoscaling policy config associated with the cluster.`,
+							Type:             schema.TypeList,
+							Optional:         true,
+							AtLeastOneOf:     clusterConfigKeys,
+							MaxItems:         1,
+							Description:      `The autoscaling policy config associated with the cluster.`,
+							DiffSuppressFunc: emptyOrUnsetBlockDiffSuppress,
 							Elem: &schema.Resource{
 								Schema: map[string]*schema.Schema{
 									"policy_uri": {
-										Type:        schema.TypeString,
-										Required:    true,
-										Description: `The autoscaling policy used by the cluster.`,
+										Type:             schema.TypeString,
+										Required:         true,
+										Description:      `The autoscaling policy used by the cluster.`,
+										DiffSuppressFunc: locationDiffSuppress,
 									},
 								},
 							},
@@ -1196,6 +1198,15 @@ func resourceDataprocClusterUpdate(d *schema.ResourceData, meta interface{}) err
 		updMask = append(updMask, "config.secondary_worker_config.num_instances")
 	}
 
+	if d.HasChange("cluster_config.0.autoscaling_config") {
+		desiredPolicy := d.Get("cluster_config.0.autoscaling_config.0.policy_uri").(string)
+		cluster.Config.AutoscalingConfig = &dataproc.AutoscalingConfig{
+			PolicyUri: desiredPolicy,
+		}
+
+		updMask = append(updMask, "config.autoscaling_config.policy_uri")
+	}
+
 	if d.HasChange("cluster_config.0.lifecycle_config.0.idle_delete_ttl") {
 		idleDeleteTtl := d.Get("cluster_config.0.lifecycle_config.0.idle_delete_ttl").(string)
 		cluster.Config.LifecycleConfig = &dataproc.LifecycleConfig{

google-beta/resource_dataproc_cluster_test.go

@@ -718,6 +718,35 @@ func TestAccDataprocCluster_withKerberos(t *testing.T) {
 	})
 }
 
+func TestAccDataprocCluster_withAutoscalingPolicy(t *testing.T) {
+	t.Parallel()
+
+	rnd := randString(t, 10)
+
+	var cluster dataproc.Cluster
+	vcrTest(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckDataprocClusterDestroy(t),
+		Steps: []resource.TestStep{
+			{
+				Config: testAccDataprocCluster_withAutoscalingPolicy(rnd),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckDataprocClusterExists(t, "google_dataproc_cluster.basic", &cluster),
+					testAccCheckDataprocClusterAutoscaling(t, &cluster, true),
+				),
+			},
+			{
+				Config: testAccDataprocCluster_removeAutoscalingPolicy(rnd),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckDataprocClusterExists(t, "google_dataproc_cluster.basic", &cluster),
+					testAccCheckDataprocClusterAutoscaling(t, &cluster, false),
+				),
+			},
+		},
+	})
+}
+
 func testAccCheckDataprocClusterDestroy(t *testing.T) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		config := googleProviderConfig(t)
@@ -768,6 +797,18 @@ func testAccCheckDataprocClusterHasServiceScopes(t *testing.T, cluster *dataproc
 	}
 }
 
+func testAccCheckDataprocClusterAutoscaling(t *testing.T, cluster *dataproc.Cluster, expectAutoscaling bool) func(s *terraform.State) error {
+	return func(s *terraform.State) error {
+		if cluster.Config.AutoscalingConfig == nil && expectAutoscaling {
+			return fmt.Errorf("Cluster does not contain AutoscalingConfig, expected it would")
+		} else if cluster.Config.AutoscalingConfig != nil && !expectAutoscaling {
+			return fmt.Errorf("Cluster contains AutoscalingConfig, expected it not to")
+		}
+
+		return nil
+	}
+}
+
 func validateBucketExists(bucket string, config *Config) (bool, error) {
 	_, err := config.clientStorage.Buckets.Get(bucket).Do()
 
@@ -1532,3 +1573,67 @@ resource "google_dataproc_cluster" "kerb" {
 }
 `, rnd, rnd, rnd, kmsKey)
 }
+
+func testAccDataprocCluster_withAutoscalingPolicy(rnd string) string {
+	return fmt.Sprintf(`
+resource "google_dataproc_cluster" "basic" {
+  name     = "tf-test-dataproc-policy-%s"
+  region   = "us-central1"
+
+  cluster_config {
+    autoscaling_config {
+      policy_uri = google_dataproc_autoscaling_policy.asp.id
+    }
+  }
+}
+  
+resource "google_dataproc_autoscaling_policy" "asp" {
+  policy_id = "tf-test-dataproc-policy-%s"
+  location  = "us-central1"
+
+  worker_config {
+    max_instances = 3
+  }
+
+  basic_algorithm {
+    yarn_config {
+      graceful_decommission_timeout = "30s"
+      scale_up_factor   = 0.5
+      scale_down_factor = 0.5
+    }
+  }
+}
+`, rnd, rnd)
+}
+
+func testAccDataprocCluster_removeAutoscalingPolicy(rnd string) string {
+	return fmt.Sprintf(`
+resource "google_dataproc_cluster" "basic" {
+  name     = "tf-test-dataproc-policy-%s"
+  region   = "us-central1"
+
+  cluster_config {
+    autoscaling_config {
+      policy_uri = ""
+    }
+  }
+}
+  
+resource "google_dataproc_autoscaling_policy" "asp" {
+  policy_id = "tf-test-dataproc-policy-%s"
+  location  = "us-central1"
+
+  worker_config {
+    max_instances = 3
+  }
+
+  basic_algorithm {
+    yarn_config {
+      graceful_decommission_timeout = "30s"
+      scale_up_factor   = 0.5
+      scale_down_factor = 0.5
+    }
+  }
+}
+`, rnd, rnd)
+}

website/docs/r/dataproc_cluster.html.markdown

@@ -176,6 +176,8 @@ The `cluster_config` block supports:
 * `security_config` (Optional) Security related configuration. Structure defined below.
 
 * `autoscaling_config` (Optional)  The autoscaling policy config associated with the cluster.
+   Note that once set, if `autoscaling_config` is the only field set in `cluster_config`, it can
+   only be removed by setting `policy_uri = ""`, rather than removing the whole block.
    Structure defined below.
 
 * `initialization_action` (Optional) Commands to execute on each node after config is completed.