Breaking change: Rework taint model in GKE (#9011) (#6351)

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

Author: modular-magician

.changelog/9011.txt

@@ -0,0 +1,6 @@
+```release-note:breaking-change
+container: reworked the `taint` field in `google_container_cluster` and `google_container_node_pool` to only manage a subset of taint keys based on those already in state. Most existing resources are unaffected, unless they use `sandbox_config`- see upgrade guide for details.
+```
+```release-note:enhancement
+container: added the `effective_taints` attribute to `google_container_cluster` and `google_container_node_pool`, outputting all known taint values
+```

google-beta/services/container/node_config.go

@@ -402,16 +402,10 @@ func schemaNodeConfig() *schema.Schema {
 				},
 
 				"taint": {
-					Type:     schema.TypeList,
-					Optional: true,
-					// Computed=true because GKE Sandbox will automatically add taints to nodes that can/cannot run sandboxed pods.
-					Computed: true,
-					ForceNew: true,
-					// Legacy config mode allows explicitly defining an empty taint.
-					// See https://www.terraform.io/docs/configuration/attr-as-blocks.html
-					ConfigMode:       schema.SchemaConfigModeAttr,
-					Description:      `List of Kubernetes taints to be applied to each node.`,
-					DiffSuppressFunc: containerNodePoolTaintSuppress,
+					Type:        schema.TypeList,
+					Optional:    true,
+					ForceNew:    true,
+					Description: `List of Kubernetes taints to be applied to each node.`,
 					Elem: &schema.Resource{
 						Schema: map[string]*schema.Schema{
 							"key": {
@@ -437,6 +431,31 @@ func schemaNodeConfig() *schema.Schema {
 					},
 				},
 
+				"effective_taints": {
+					Type:        schema.TypeList,
+					Computed:    true,
+					Description: `List of kubernetes taints applied to each node.`,
+					Elem: &schema.Resource{
+						Schema: map[string]*schema.Schema{
+							"key": {
+								Type:        schema.TypeString,
+								Computed:    true,
+								Description: `Key for taint.`,
+							},
+							"value": {
+								Type:        schema.TypeString,
+								Computed:    true,
+								Description: `Value for taint.`,
+							},
+							"effect": {
+								Type:        schema.TypeString,
+								Computed:    true,
+								Description: `Effect for taint.`,
+							},
+						},
+					},
+				},
+
 				"workload_metadata_config": {
 					Computed:    true,
 					Type:        schema.TypeList,
@@ -880,8 +899,10 @@ func expandNodeConfig(v interface{}) *container.NodeConfig {
 				Value:  data["value"].(string),
 				Effect: data["effect"].(string),
 			}
+
 			nodeTaints = append(nodeTaints, taint)
 		}
+
 		nc.Taints = nodeTaints
 	}
 
@@ -1071,13 +1092,24 @@ func flattenNodeConfigDefaults(c *container.NodeConfigDefaults) []map[string]int
 	return result
 }
 
-func flattenNodeConfig(c *container.NodeConfig) []map[string]interface{} {
+// v == old state of `node_config`
+func flattenNodeConfig(c *container.NodeConfig, v interface{}) []map[string]interface{} {
 	config := make([]map[string]interface{}, 0, 1)
 
 	if c == nil {
 		return config
 	}
 
+	// default to no prior taint state if there are any issues
+	oldTaints := []interface{}{}
+	oldNodeConfigSchemaContainer := v.([]interface{})
+	if len(oldNodeConfigSchemaContainer) != 0 {
+		oldNodeConfigSchema := oldNodeConfigSchemaContainer[0].(map[string]interface{})
+		if vt, ok := oldNodeConfigSchema["taint"]; ok && len(vt.([]interface{})) > 0 {
+			oldTaints = vt.([]interface{})
+		}
+	}
+
 	config = append(config, map[string]interface{}{
 		"machine_type":                       c.MachineType,
 		"disk_size_gb":                       c.DiskSizeGb,
@@ -1101,7 +1133,8 @@ func flattenNodeConfig(c *container.NodeConfig) []map[string]interface{} {
 		"spot":                               c.Spot,
 		"min_cpu_platform":                   c.MinCpuPlatform,
 		"shielded_instance_config":           flattenShieldedInstanceConfig(c.ShieldedInstanceConfig),
-		"taint":                              flattenTaints(c.Taints),
+		"taint":                              flattenTaints(c.Taints, oldTaints),
+		"effective_taints":                   flattenEffectiveTaints(c.Taints),
 		"workload_metadata_config":           flattenWorkloadMetadataConfig(c.WorkloadMetadataConfig),
 		"sandbox_config":                     flattenSandboxConfig(c.SandboxConfig),
 		"host_maintenance_policy":            flattenHostMaintenancePolicy(c.HostMaintenancePolicy),
@@ -1241,7 +1274,31 @@ func flattenGKEReservationAffinity(c *container.ReservationAffinity) []map[strin
 	return result
 }
 
-func flattenTaints(c []*container.NodeTaint) []map[string]interface{} {
+// flattenTaints records the set of taints already present in state.
+func flattenTaints(c []*container.NodeTaint, oldTaints []interface{}) []map[string]interface{} {
+	taintKeys := map[string]struct{}{}
+	for _, raw := range oldTaints {
+		data := raw.(map[string]interface{})
+		taintKey := data["key"].(string)
+		taintKeys[taintKey] = struct{}{}
+	}
+
+	result := []map[string]interface{}{}
+	for _, taint := range c {
+		if _, ok := taintKeys[taint.Key]; ok {
+			result = append(result, map[string]interface{}{
+				"key":    taint.Key,
+				"value":  taint.Value,
+				"effect": taint.Effect,
+			})
+		}
+	}
+
+	return result
+}
+
+// flattenEffectiveTaints records the complete set of taints returned from GKE.
+func flattenEffectiveTaints(c []*container.NodeTaint) []map[string]interface{} {
 	result := []map[string]interface{}{}
 	for _, taint := range c {
 		result = append(result, map[string]interface{}{
@@ -1250,6 +1307,7 @@ func flattenTaints(c []*container.NodeTaint) []map[string]interface{} {
 			"effect": taint.Effect,
 		})
 	}
+
 	return result
 }
 
@@ -1319,74 +1377,6 @@ func containerNodePoolLabelsSuppress(k, old, new string, d *schema.ResourceData)
 	return true
 }
 
-func containerNodePoolTaintSuppress(k, old, new string, d *schema.ResourceData) bool {
-	// Node configs are embedded into multiple resources (container cluster and
-	// container node pool) so we determine the node config key dynamically.
-	idx := strings.Index(k, ".taint.")
-	if idx < 0 {
-		return false
-	}
-
-	root := k[:idx]
-
-	// Right now, GKE only applies its own out-of-band labels when you enable
-	// Sandbox. We only need to perform diff suppression in this case;
-	// otherwise, the default Terraform behavior is fine.
-	o, n := d.GetChange(root + ".sandbox_config.0.sandbox_type")
-	if o == nil || n == nil {
-		return false
-	}
-
-	// Pull the entire changeset as a list rather than trying to deal with each
-	// element individually.
-	o, n = d.GetChange(root + ".taint")
-	if o == nil || n == nil {
-		return false
-	}
-
-	type taintType struct {
-		Key, Value, Effect string
-	}
-
-	taintSet := make(map[taintType]struct{})
-
-	// Add all new taints to set.
-	for _, raw := range n.([]interface{}) {
-		data := raw.(map[string]interface{})
-		taint := taintType{
-			Key:    data["key"].(string),
-			Value:  data["value"].(string),
-			Effect: data["effect"].(string),
-		}
-		taintSet[taint] = struct{}{}
-	}
-
-	// Remove all current taints, skipping GKE-managed keys if not present in
-	// the new configuration.
-	for _, raw := range o.([]interface{}) {
-		data := raw.(map[string]interface{})
-		taint := taintType{
-			Key:    data["key"].(string),
-			Value:  data["value"].(string),
-			Effect: data["effect"].(string),
-		}
-		if _, ok := taintSet[taint]; ok {
-			delete(taintSet, taint)
-		} else if !strings.HasPrefix(taint.Key, "sandbox.gke.io/") && taint.Key != "kubernetes.io/arch" {
-			// User-provided taint removed in new configuration.
-			return false
-		}
-	}
-
-	// If, at this point, the set still has elements, the new configuration
-	// added an additional taint.
-	if len(taintSet) > 0 {
-		return false
-	}
-
-	return true
-}
-
 func flattenKubeletConfig(c *container.NodeKubeletConfig) []map[string]interface{} {
 	result := []map[string]interface{}{}
 	if c != nil {

google-beta/services/container/resource_container_cluster.go

@@ -2635,7 +2635,7 @@ func resourceContainerClusterRead(d *schema.ResourceData, meta interface{}) erro
 			return fmt.Errorf("Error setting default_max_pods_per_node: %s", err)
 		}
 	}
-	if err := d.Set("node_config", flattenNodeConfig(cluster.NodeConfig)); err != nil {
+	if err := d.Set("node_config", flattenNodeConfig(cluster.NodeConfig, d.Get("node_config"))); err != nil {
 		return err
 	}
 	if err := d.Set("project", project); err != nil {

google-beta/services/container/resource_container_cluster_test.go

@@ -1222,17 +1222,19 @@ func TestAccContainerCluster_withNodeConfig(t *testing.T) {
 				Config: testAccContainerCluster_withNodeConfig(clusterName),
 			},
 			{
-				ResourceName:      "google_container_cluster.with_node_config",
-				ImportState:       true,
-				ImportStateVerify: true,
+				ResourceName:            "google_container_cluster.with_node_config",
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"node_config.0.taint"},
 			},
 			{
 				Config: testAccContainerCluster_withNodeConfigUpdate(clusterName),
 			},
 			{
-				ResourceName:      "google_container_cluster.with_node_config",
-				ImportState:       true,
-				ImportStateVerify: true,
+				ResourceName:            "google_container_cluster.with_node_config",
+				ImportState:             true,
+				ImportStateVerify:       true,
+				ImportStateVerifyIgnore: []string{"node_config.0.taint"},
 			},
 		},
 	})
@@ -1525,7 +1527,7 @@ func TestAccContainerCluster_withSandboxConfig(t *testing.T) {
 				ResourceName:            "google_container_cluster.with_sandbox_config",
 				ImportState:             true,
 				ImportStateVerify:       true,
-				ImportStateVerifyIgnore: []string{"min_master_version"},
+				ImportStateVerifyIgnore: []string{"min_master_version", "node_config.0.taint"},
 			},
 			{
 				// GKE sets automatic labels and taints on nodes. This makes

google-beta/services/container/resource_container_node_pool.go

@@ -1106,7 +1106,7 @@ func flattenNodePool(d *schema.ResourceData, config *transport_tpg.Config, np *c
 		"initial_node_count":          np.InitialNodeCount,
 		"node_locations":              schema.NewSet(schema.HashString, tpgresource.ConvertStringArrToInterface(np.Locations)),
 		"node_count":                  nodeCount,
-		"node_config":                 flattenNodeConfig(np.Config),
+		"node_config":                 flattenNodeConfig(np.Config, d.Get(prefix+"node_config")),
 		"instance_group_urls":         igmUrls,
 		"managed_instance_group_urls": managedIgmUrls,
 		"version":                     np.Version,

google-beta/services/container/resource_container_node_pool_test.go

@@ -216,7 +216,7 @@ func TestAccContainerNodePool_withNodeConfig(t *testing.T) {
 				ImportStateVerify: true,
 				// autoscaling.# = 0 is equivalent to no autoscaling at all,
 				// but will still cause an import diff
-				ImportStateVerifyIgnore: []string{"autoscaling.#"},
+				ImportStateVerifyIgnore: []string{"autoscaling.#", "node_config.0.taint"},
 			},
 			{
 				Config: testAccContainerNodePool_withNodeConfigUpdate(cluster, nodePool),
@@ -227,7 +227,7 @@ func TestAccContainerNodePool_withNodeConfig(t *testing.T) {
 				ImportStateVerify: true,
 				// autoscaling.# = 0 is equivalent to no autoscaling at all,
 				// but will still cause an import diff
-				ImportStateVerifyIgnore: []string{"autoscaling.#"},
+				ImportStateVerifyIgnore: []string{"autoscaling.#", "node_config.0.taint"},
 			},
 		},
 	})
@@ -361,32 +361,6 @@ func TestAccContainerNodePool_withSandboxConfig(t *testing.T) {
 				ImportState:       true,
 				ImportStateVerify: true,
 			},
-			{
-				// GKE sets automatic labels and taints on nodes. This makes
-				// sure we ignore the automatic ones and keep our own.
-				Config: testAccContainerNodePool_withSandboxConfig(cluster, np),
-				// When we use PlanOnly without ExpectNonEmptyPlan, we're
-				// guaranteeing that the computed fields of the resources don't
-				// force an unintentional change to the plan. That is, we
-				// expect this part of the test to pass only if the plan
-				// doesn't change.
-				PlanOnly: true,
-			},
-			{
-				// Now we'll modify the taints, which should force a change to
-				// the plan. We make sure we don't over-suppress and end up
-				// eliminating the labels or taints we asked for. This will
-				// destroy and recreate the node pool as taints are immutable.
-				Config: testAccContainerNodePool_withSandboxConfig_changeTaints(cluster, np),
-				Check: resource.ComposeTestCheckFunc(
-					resource.TestCheckResourceAttr("google_container_node_pool.with_sandbox_config",
-						"node_config.0.labels.test.terraform.io/gke-sandbox", "true"),
-					resource.TestCheckResourceAttr("google_container_node_pool.with_sandbox_config",
-						"node_config.0.taint.0.key", "test.terraform.io/gke-sandbox"),
-					resource.TestCheckResourceAttr("google_container_node_pool.with_sandbox_config",
-						"node_config.0.taint.1.key", "test.terraform.io/gke-sandbox-amended"),
-				),
-			},
 		},
 	})
 }
@@ -2501,63 +2475,6 @@ resource "google_container_node_pool" "with_sandbox_config" {
       "test.terraform.io/gke-sandbox" = "true"
     }
 
-    taint {
-      key    = "test.terraform.io/gke-sandbox"
-      value  = "true"
-      effect = "NO_SCHEDULE"
-    }
-
-    oauth_scopes = [
-      "https://www.googleapis.com/auth/logging.write",
-      "https://www.googleapis.com/auth/monitoring",
-    ]
-  }
-}
-`, cluster, np)
-}
-
-func testAccContainerNodePool_withSandboxConfig_changeTaints(cluster, np string) string {
-	return fmt.Sprintf(`
-data "google_container_engine_versions" "central1a" {
-  location = "us-central1-a"
-}
-
-resource "google_container_cluster" "cluster" {
-  name               = "%s"
-  location           = "us-central1-a"
-  initial_node_count = 1
-  min_master_version = data.google_container_engine_versions.central1a.latest_master_version
-}
-
-resource "google_container_node_pool" "with_sandbox_config" {
-  name               = "%s"
-  location           = "us-central1-a"
-  cluster            = google_container_cluster.cluster.name
-  initial_node_count = 1
-  node_config {
-    machine_type = "n1-standard-1"  // can't be e2 because of gvisor
-    image_type = "COS_CONTAINERD"
-
-    sandbox_config {
-      sandbox_type = "gvisor"
-    }
-
-    labels = {
-      "test.terraform.io/gke-sandbox" = "true"
-    }
-
-    taint {
-      key    = "test.terraform.io/gke-sandbox"
-      value  = "true"
-      effect = "NO_SCHEDULE"
-    }
-
-    taint {
-      key    = "test.terraform.io/gke-sandbox-amended"
-      value  = "also-true"
-      effect = "NO_SCHEDULE"
-    }
-
     oauth_scopes = [
       "https://www.googleapis.com/auth/logging.write",
       "https://www.googleapis.com/auth/monitoring",