-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
}, | ||
{ | ||
// Now we'll modify the labels, which should force a change to | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"), | ||
), | ||
}, | ||
}, | ||
}) | ||
} | ||
|
@@ -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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
// 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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 -%> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!