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.

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

Mask GKE Sandbox-specific labels and taints #3749

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
75 changes: 75 additions & 0 deletions third_party/terraform/tests/resource_container_cluster_test.go.erb
Original file line number Diff line number Diff line change
Expand Up @@ -890,6 +890,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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also add a comment here about what this does, since it's a less common feature in our tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

},
{
// Now we'll modify the labels, which should force a change to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI labels aren't modifiable in-place, so this test step destroys and recreates the cluster. If that's fine for this test, can you add a note here so it doesn't surprise people if they come across this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is indeed intentional, and I've added a comment to note that!

// 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"),
),
},
},
})
}
Expand Down Expand Up @@ -2813,6 +2839,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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
),
},
},
})
}
Expand Down Expand Up @@ -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",
Expand Down
122 changes: 121 additions & 1 deletion third_party/terraform/utils/node_config.go.erb
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
package google

import (
"strconv"
"strings"

"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
Expand Down Expand Up @@ -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": {
Expand Down Expand Up @@ -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": {
Expand Down Expand Up @@ -502,4 +507,119 @@ func flattenSandboxConfig(c *containerBeta.SandboxConfig) []map[string]interface
}
return result
}

func containerNodePoolLabelsSuppress(k, old, new string, d *schema.ResourceData) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the node config schema gets used both in the node pool resource and in the container cluster resource (and within container cluster, it's top level and part of the node pool block), so the address of the field you're looking for will be different depending on where the user is specifying it. I'd recommend using the value of 'k' to help you figure out where you are.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think they both actually use the same hierarchy by chance, but it raises the well-taken point that there's no restriction on this schema being used elsewhere in the future. I have updated the code to derive the root of the node config from k and added a comment to clarify.

// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have a fairly similar function in resource_container_cluster: containerClusterAddedScopesSuppress. Can you look at that and see if using similar logic makes sense here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic makes way more sense than the initial way I put this PR together. I have updated the code to use a similar strategy. Note that it does get called multiple times -- once for each "stringable" key/value Terraform provides -- but I think the performance hit is worth the clarity in this case. Now the suppression will be consistent across the entire field, rather than changing for some sub-keys (it is not clear at all what the upward propagation behavior is). Thank you for the pointer!

// 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 -%>