Mask GKE Sandbox-specific labels and taints #3749

impl · 2020-07-16T09:05:55Z

Hi friends,

This change prevents the GKE Sandbox labels and taints (currently sandbox.gke.io/runtime=gvisor) from causing node pools to always need to be recreated. Without this change, once a node pool is created, it needs to be manually updated to include the additional GKE Sandbox label and taint items.

I believe this PR fixes hashicorp/terraform-provider-google#4210.

The addition of the DiffSuppressFunc on a TypeList is a little weird (not sure if it's using prescribed functionality or not; I see hashicorp/terraform-plugin-sdk#477 but it seems to work here). I'd be happy to take any suggestions on how to improve it.

Once I get an OK on the general approach, I will add a few tests as well. Thanks for taking a look!

Release Note Template for Downstream PRs (will be copied)

compute: Masked automatically applied GKE Sandbox node labels and taints on node pools

modular-magician · 2020-07-16T09:07:07Z

Hello! I am a robot who works on Magic Modules PRs.

I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review.

Thanks for your contribution! A human will be with you soon.

@danawillow, please review this PR or find an appropriate assignee.

modular-magician · 2020-07-16T09:12:45Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform Beta: Diff ( 1 file changed, 64 insertions(+), 4 deletions(-))

danawillow

Here are some comments for a first pass! I'd definitely like to see some tests so we can be sure this actually works.

danawillow · 2020-07-16T17:00:49Z

third_party/terraform/utils/node_config.go.erb

@@ -502,4 +509,58 @@ func flattenSandboxConfig(c *containerBeta.SandboxConfig) []map[string]interface
 	}
 	return result
 }
+
+func containerNodePoolLabelsSuppress(k, old, new string, d *schema.ResourceData) bool {
+	o, n := d.GetChange("node_config.0.sandbox_config.0.sandbox_type")


I believe this is trying to suggest that we only care about the sandbox label if sandbox_type is set, is that correct? If so can you add a comment to that regard?

This is indeed the case; I added a comment about this to both methods.

danawillow · 2020-07-16T17:02:24Z

third_party/terraform/utils/node_config.go.erb

+		return false
+	}
+
+	if strings.HasPrefix(k, "node_config.0.labels.sandbox.gke.io/") {


I'm not convinced this will work. It would be worth verifying (with some print statements or a test) but I'm pretty sure 'k' here is going to be the full list, rather than this func being called on each element of the list.

So the behavior here is really interesting. You actually get each key and value under the root independently and can return a different result for each. For example, dumping the k, old, and new values for each invocation you'd see something like:

k=node_config.0.labels.# old=2 new=1 k=node_config.0.labels.test.terraform.io/test old=foo new=foo k=node_config.0.labels.sandbox.gke.io/runtime old=gvisor new=

It turns out to not be important (see my additional comment below) now, but the initial pass did depend on this admittedly strange behavior. I think it has to do with old and new being strings.

danawillow · 2020-07-16T17:06:14Z

third_party/terraform/utils/node_config.go.erb

@@ -502,4 +509,58 @@ func flattenSandboxConfig(c *containerBeta.SandboxConfig) []map[string]interface
 	}
 	return result
 }
+
+func containerNodePoolLabelsSuppress(k, old, new string, d *schema.ResourceData) bool {


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.

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.

danawillow · 2020-07-16T17:08:06Z

third_party/terraform/utils/node_config.go.erb

+	return false
+}
+
+func containerNodePoolTaintSuppress(k, old, new string, d *schema.ResourceData) bool {


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?

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!

modular-magician · 2020-07-17T06:47:27Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform Beta: Diff ( 3 files changed, 274 insertions(+), 4 deletions(-))

impl · 2020-07-17T06:52:57Z

Here are some comments for a first pass! I'd definitely like to see some tests so we can be sure this actually works.

Thank you for the quick and detailed review! I've updated the code to address your concerns and added tests for both the container_cluster and container_node_pool resources.

danawillow · 2020-07-25T00:17:14Z

Hey, just wanted to pop in and let you know I haven't forgotten about this! I've had a bunch of other things on my plate, but I should be able to look at this again next week.

impl · 2020-07-27T20:05:49Z

Hi there, no worries! Totally understand. Thanks for the update -- much appreciated!

danawillow · 2020-07-30T19:38:24Z

third_party/terraform/tests/resource_container_cluster_test.go.erb

+				// GKE sets automatic labels and taints on nodes. This makes
+				// sure we ignore the automatic ones and keep our own.
+				Config:   testAccContainerCluster_withSandboxConfig(clusterName),
+				PlanOnly: true,


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

danawillow · 2020-07-30T19:39:11Z

third_party/terraform/tests/resource_container_cluster_test.go.erb

+				PlanOnly: true,
+			},
+			{
+				// Now we'll modify the labels, which should force a change to


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?

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

This change prevents the GKE Sandbox labels and taints (currently sandbox.gke.io/runtime=gvisor) from causing node pools to always need to be recreated. Without this change, once a node pool is created, it needs to be manually updated to include the additional GKE Sandbox label and taint items.

modular-magician · 2020-07-30T23:35:06Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform Beta: Diff ( 3 files changed, 286 insertions(+), 4 deletions(-))

modular-magician · 2020-07-30T23:36:08Z

Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are.

Diff report:

Terraform Beta: Diff ( 3 files changed, 286 insertions(+), 4 deletions(-))

danawillow

Looks great, thanks @impl!

googlebot added the cla: yes label Jul 16, 2020

modular-magician requested a review from danawillow July 16, 2020 09:07

danawillow suggested changes Jul 16, 2020

View reviewed changes

impl force-pushed the gke-sandbox-labels-taints branch from 44495fe to be6ed8e Compare July 17, 2020 06:40

impl requested a review from danawillow July 20, 2020 17:17

danawillow reviewed Jul 30, 2020

View reviewed changes

impl force-pushed the gke-sandbox-labels-taints branch from be6ed8e to 67d4b88 Compare July 30, 2020 23:28

impl force-pushed the gke-sandbox-labels-taints branch from 67d4b88 to c7b06d8 Compare July 30, 2020 23:29

impl requested a review from danawillow July 30, 2020 23:29

danawillow approved these changes Jul 31, 2020

View reviewed changes

danawillow merged commit de0746e into GoogleCloudPlatform:master Jul 31, 2020

modular-magician mentioned this pull request Jul 31, 2020

Mask GKE Sandbox-specific labels and taints hashicorp/terraform-provider-google-beta#2320

Merged

impl deleted the gke-sandbox-labels-taints branch July 31, 2020 18:49

rileykarson mentioned this pull request Dec 7, 2020

nvidia taint along custom taints in google_container_node_pool hashicorp/terraform-provider-google#7928

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mask GKE Sandbox-specific labels and taints #3749

Mask GKE Sandbox-specific labels and taints #3749

impl commented Jul 16, 2020 •

edited

Loading

modular-magician commented Jul 16, 2020

modular-magician commented Jul 16, 2020

danawillow left a comment

danawillow Jul 16, 2020

impl Jul 17, 2020

danawillow Jul 16, 2020

impl Jul 17, 2020 •

edited

Loading

danawillow Jul 16, 2020

impl Jul 17, 2020

danawillow Jul 16, 2020

impl Jul 17, 2020

modular-magician commented Jul 17, 2020

impl commented Jul 17, 2020

danawillow commented Jul 25, 2020

impl commented Jul 27, 2020

danawillow Jul 30, 2020

impl Jul 30, 2020

danawillow Jul 30, 2020

impl Jul 30, 2020

modular-magician commented Jul 30, 2020

modular-magician commented Jul 30, 2020

danawillow left a comment

Mask GKE Sandbox-specific labels and taints #3749

Mask GKE Sandbox-specific labels and taints #3749

Conversation

impl commented Jul 16, 2020 • edited Loading

modular-magician commented Jul 16, 2020

modular-magician commented Jul 16, 2020

Diff report:

danawillow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

impl Jul 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Jul 17, 2020

Diff report:

impl commented Jul 17, 2020

danawillow commented Jul 25, 2020

impl commented Jul 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modular-magician commented Jul 30, 2020

Diff report:

modular-magician commented Jul 30, 2020

Diff report:

danawillow left a comment

Choose a reason for hiding this comment

impl commented Jul 16, 2020 •

edited

Loading

impl Jul 17, 2020 •

edited

Loading