From de0746ebfdfe227127e1d8cdc3bdbcf71ee68d1a Mon Sep 17 00:00:00 2001 From: Noah Fontes Date: Fri, 31 Jul 2020 10:45:31 -0700 Subject: [PATCH] Mask GKE Sandbox-specific labels and taints (#3749) --- .../resource_container_cluster_test.go.erb | 75 +++++++++++ .../resource_container_node_pool_test.go.erb | 88 +++++++++++++ .../terraform/utils/node_config.go.erb | 122 +++++++++++++++++- 3 files changed, 284 insertions(+), 1 deletion(-) diff --git a/third_party/terraform/tests/resource_container_cluster_test.go.erb b/third_party/terraform/tests/resource_container_cluster_test.go.erb index fa47e1117f75..c1b72fae4eb6 100644 --- a/third_party/terraform/tests/resource_container_cluster_test.go.erb +++ b/third_party/terraform/tests/resource_container_cluster_test.go.erb @@ -902,6 +902,32 @@ func TestAccContainerCluster_withSandboxConfig(t *testing.T) { ImportStateVerify: true, ImportStateVerifyIgnore: []string{"min_master_version"}, }, + { + // GKE sets automatic labels and taints on nodes. This makes + // sure we ignore the automatic ones and keep our own. + Config: testAccContainerCluster_withSandboxConfig(clusterName), + // 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 labels, 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 cluster as labels are immutable. + Config: testAccContainerCluster_withSandboxConfig_changeLabels(clusterName), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("google_container_cluster.with_sandbox_config", + "node_config.0.labels.test.terraform.io/gke-sandbox", "true"), + resource.TestCheckResourceAttr("google_container_cluster.with_sandbox_config", + "node_config.0.labels.test.terraform.io/gke-sandbox-amended", "also-true"), + resource.TestCheckResourceAttr("google_container_cluster.with_sandbox_config", + "node_config.0.taint.0.key", "test.terraform.io/gke-sandbox"), + ), + }, }, }) } @@ -2825,6 +2851,55 @@ resource "google_container_cluster" "with_sandbox_config" { sandbox_config { sandbox_type = "gvisor" } + + labels = { + "test.terraform.io/gke-sandbox" = "true" + } + + taint { + key = "test.terraform.io/gke-sandbox" + value = "true" + effect = "NO_SCHEDULE" + } + } +} +`, clusterName) +} + +func testAccContainerCluster_withSandboxConfig_changeLabels(clusterName string) string { + return fmt.Sprintf(` +data "google_container_engine_versions" "central1a" { + location = "us-central1-a" +} + +resource "google_container_cluster" "with_sandbox_config" { + name = "%s" + location = "us-central1-a" + initial_node_count = 1 + min_master_version = data.google_container_engine_versions.central1a.latest_master_version + + node_config { + oauth_scopes = [ + "https://www.googleapis.com/auth/logging.write", + "https://www.googleapis.com/auth/monitoring", + ] + + image_type = "COS_CONTAINERD" + + sandbox_config { + sandbox_type = "gvisor" + } + + labels = { + "test.terraform.io/gke-sandbox" = "true" + "test.terraform.io/gke-sandbox-amended" = "also-true" + } + + taint { + key = "test.terraform.io/gke-sandbox" + value = "true" + effect = "NO_SCHEDULE" + } } } `, clusterName) diff --git a/third_party/terraform/tests/resource_container_node_pool_test.go.erb b/third_party/terraform/tests/resource_container_node_pool_test.go.erb index 8a1c1185f239..acf568a3e44f 100644 --- a/third_party/terraform/tests/resource_container_node_pool_test.go.erb +++ b/third_party/terraform/tests/resource_container_node_pool_test.go.erb @@ -235,6 +235,32 @@ 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"), + ), + }, }, }) } @@ -1256,9 +1282,71 @@ resource "google_container_node_pool" "with_sandbox_config" { initial_node_count = 1 node_config { 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" + } + + 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 { + 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", diff --git a/third_party/terraform/utils/node_config.go.erb b/third_party/terraform/utils/node_config.go.erb index 4bc6aba67442..0c139786d94e 100644 --- a/third_party/terraform/utils/node_config.go.erb +++ b/third_party/terraform/utils/node_config.go.erb @@ -2,7 +2,6 @@ package google import ( - "strconv" "strings" "github.com/hashicorp/terraform-plugin-sdk/helper/schema" @@ -83,6 +82,9 @@ func schemaNodeConfig() *schema.Schema { Computed: true, ForceNew: true, Elem: &schema.Schema{Type: schema.TypeString}, + <% unless version.nil? || version == 'ga' -%> + DiffSuppressFunc: containerNodePoolLabelsSuppress, + <% end -%> }, "local_ssd_count": { @@ -183,6 +185,9 @@ func schemaNodeConfig() *schema.Schema { // Legacy config mode allows explicitly defining an empty taint. // See https://www.terraform.io/docs/configuration/attr-as-blocks.html ConfigMode: schema.SchemaConfigModeAttr, + <% unless version.nil? || version == 'ga' -%> + DiffSuppressFunc: containerNodePoolTaintSuppress, + <% end -%> Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "key": { @@ -502,4 +507,119 @@ func flattenSandboxConfig(c *containerBeta.SandboxConfig) []map[string]interface } return result } + +func containerNodePoolLabelsSuppress(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, ".labels.") + 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 + ".labels") + if o == nil || n == nil { + return false + } + + labels := n.(map[string]interface{}) + + // Remove all current labels, skipping GKE-managed ones if not present in + // the new configuration. + for key, value := range o.(map[string]interface{}) { + if nv, ok := labels[key]; ok && nv == value { + delete(labels, key) + } else if !strings.HasPrefix(key, "sandbox.gke.io/") { + // User-provided label removed in new configuration. + return false + } + } + + // If, at this point, the map still has elements, the new configuration + // added an additional taint. + if len(labels) > 0 { + return false + } + + 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/") { + // 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 +} <% end -%>