container: fix node_config.kubelet_config updates in google_container_cluster (GoogleCloudPlatform#11697)

Author: wyardley

mmv1/third_party/terraform/services/container/resource_container_cluster.go.erb

@@ -3844,44 +3844,38 @@ func resourceContainerClusterUpdate(d *schema.ResourceData, meta interface{}) er
 			// Acquire write-lock on nodepool.
 			npLockKey := nodePoolInfo.nodePoolLockKey(defaultPool)
 
-			// Note: probably long term this should be handled broadly for all the
-			// items in kubelet_config in a simpler / DRYer way.
+			// Still should be further consolidated / DRYed up
 			// See b/361634104
-			if d.HasChange("node_config.0.kubelet_config.0.insecure_kubelet_readonly_port_enabled") {
-				it := d.Get("node_config.0.kubelet_config.0.insecure_kubelet_readonly_port_enabled").(string)
-
-				// While we're getting the value from the drepcated field in
-				// node_config.kubelet_config, the actual setting that needs to be updated
-				// is on the default nodepool.
-				req := &container.UpdateNodePoolRequest{
-					Name: defaultPool,
-					KubeletConfig: &container.NodeKubeletConfig{
-						InsecureKubeletReadonlyPortEnabled: expandInsecureKubeletReadonlyPortEnabled(it),
-						ForceSendFields:                    []string{"InsecureKubeletReadonlyPortEnabled"},
-					},
-				}
+			it := d.Get("node_config.0.kubelet_config")
 
-				updateF := func() error {
-					clusterNodePoolsUpdateCall := config.NewContainerClient(userAgent).Projects.Locations.Clusters.NodePools.Update(nodePoolInfo.fullyQualifiedName(defaultPool), req)
-					if config.UserProjectOverride {
-						clusterNodePoolsUpdateCall.Header().Add("X-Goog-User-Project", nodePoolInfo.project)
-					}
-					op, err := clusterNodePoolsUpdateCall.Do()
-					if err != nil {
-						return err
-					}
+			// While we're getting the value from fields in
+			// node_config.kubelet_config, the actual setting that needs to be
+			// updated is on the default nodepool.
+			req := &container.UpdateNodePoolRequest{
+				Name: defaultPool,
+				KubeletConfig: expandKubeletConfig(it),
+			}
 
-					// Wait until it's updated
-					return ContainerOperationWait(config, op, nodePoolInfo.project, nodePoolInfo.location,
-						"updating GKE node pool insecure_kubelet_readonly_port_enabled", userAgent, timeout)
+			updateF := func() error {
+				clusterNodePoolsUpdateCall := config.NewContainerClient(userAgent).Projects.Locations.Clusters.NodePools.Update(nodePoolInfo.fullyQualifiedName(defaultPool), req)
+				if config.UserProjectOverride {
+					clusterNodePoolsUpdateCall.Header().Add("X-Goog-User-Project", nodePoolInfo.project)
 				}
-
-				if err := retryWhileIncompatibleOperation(timeout, npLockKey, updateF); err != nil {
+				op, err := clusterNodePoolsUpdateCall.Do()
+				if err != nil {
 					return err
 				}
 
-				log.Printf("[INFO] GKE cluster %s: default-pool setting for insecure_kubelet_readonly_port_enabled updated to %s", d.Id(), it)
-      }
+				// Wait until it's updated
+				return ContainerOperationWait(config, op, nodePoolInfo.project, nodePoolInfo.location,
+					"updating GKE node pool kubelet_config", userAgent, timeout)
+			}
+
+			if err := retryWhileIncompatibleOperation(timeout, npLockKey, updateF); err != nil {
+				return err
+			}
+
+			log.Printf("[INFO] GKE cluster %s: kubelet_config updated", d.Id())
 		}
 
 		if d.HasChange("node_config.0.gcfs_config") {

mmv1/third_party/terraform/services/container/resource_container_cluster_test.go.erb

@@ -1579,12 +1579,7 @@ func TestAccContainerCluster_withNodeConfigGcfsConfig(t *testing.T) {
 	})
 }
 
-// Note: Updates for these are currently known to be broken (b/361634104), and
-// so are not tested here.
-// They can probably be made similar to, or consolidated with,
-// TestAccContainerCluster_withInsecureKubeletReadonlyPortEnabledInNodeConfigUpdates
-// after that's resolved.
-func TestAccContainerCluster_withNodeConfigKubeletConfigSettings(t *testing.T) {
+func TestAccContainerCluster_withNodeConfigKubeletConfigSettingsUpdates(t *testing.T) {
 	t.Parallel()
 	clusterName := fmt.Sprintf("tf-test-cluster-%s", acctest.RandString(t, 10))
 	networkName := acctest.BootstrapSharedTestNetwork(t, "gke-cluster")
@@ -1596,7 +1591,7 @@ func TestAccContainerCluster_withNodeConfigKubeletConfigSettings(t *testing.T) {
 		CheckDestroy: testAccCheckContainerClusterDestroyProducer(t),
 		Steps: []resource.TestStep{
 			{
-				Config: testAccContainerCluster_withNodeConfigKubeletConfigSettings(clusterName, networkName, subnetworkName),
+				Config: testAccContainerCluster_withNodeConfigKubeletConfigSettingsBaseline(clusterName, networkName, subnetworkName),
 				ConfigPlanChecks: resource.ConfigPlanChecks{
 					PreApply: []plancheck.PlanCheck{
 						acctest.ExpectNoDelete(),
@@ -1609,42 +1604,30 @@ func TestAccContainerCluster_withNodeConfigKubeletConfigSettings(t *testing.T) {
 				ImportStateVerify:       true,
 				ImportStateVerifyIgnore: []string{"deletion_protection"},
 			},
-		},
-	})
-}
-
-// This is for node_config.kubelet_config, which affects the default node-pool
-// (default-pool) when created via the google_container_cluster resource
-func TestAccContainerCluster_withInsecureKubeletReadonlyPortEnabledInNodeConfigUpdates(t *testing.T) {
-	t.Parallel()
-	clusterName := fmt.Sprintf("tf-test-cluster-%s", acctest.RandString(t, 10))
-	networkName := acctest.BootstrapSharedTestNetwork(t, "gke-cluster")
-	subnetworkName := acctest.BootstrapSubnet(t, "gke-cluster", networkName)
-
-	acctest.VcrTest(t, resource.TestCase{
-		PreCheck:     func() { acctest.AccTestPreCheck(t) },
-		ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t),
-		CheckDestroy: testAccCheckContainerClusterDestroyProducer(t),
-		Steps: []resource.TestStep{
 			{
-				Config: testAccContainerCluster_withInsecureKubeletReadonlyPortEnabledInNodeConfig(clusterName, networkName, subnetworkName, "TRUE"),
+				Config: testAccContainerCluster_withNodeConfigKubeletConfigSettingsUpdates(clusterName, "none", "100ms", "TRUE", networkName, subnetworkName, 2048, true),
 				ConfigPlanChecks: resource.ConfigPlanChecks{
 					PreApply: []plancheck.PlanCheck{
 						acctest.ExpectNoDelete(),
 					},
 				},
 			},
 			{
-				ResourceName:            "google_container_cluster.with_insecure_kubelet_readonly_port_enabled_in_node_config",
+				ResourceName:            "google_container_cluster.with_node_config_kubelet_config_settings",
 				ImportState:             true,
 				ImportStateVerify:       true,
 				ImportStateVerifyIgnore: []string{"deletion_protection"},
 			},
 			{
-				Config: testAccContainerCluster_withInsecureKubeletReadonlyPortEnabledInNodeConfig(clusterName, networkName, subnetworkName, "FALSE"),
+				Config: testAccContainerCluster_withNodeConfigKubeletConfigSettingsUpdates(clusterName, "static", "", "FALSE", networkName, subnetworkName, 1024, true),
+				ConfigPlanChecks: resource.ConfigPlanChecks{
+					PreApply: []plancheck.PlanCheck{
+						acctest.ExpectNoDelete(),
+					},
+				},
 			},
 			{
-				ResourceName:            "google_container_cluster.with_insecure_kubelet_readonly_port_enabled_in_node_config",
+				ResourceName:            "google_container_cluster.with_node_config_kubelet_config_settings",
 				ImportState:             true,
 				ImportStateVerify:       true,
 				ImportStateVerifyIgnore: []string{"deletion_protection"},
@@ -6782,7 +6765,7 @@ resource "google_container_cluster" "with_node_config_gcfs_config" {
 `, clusterName, enabled, networkName, subnetworkName)
 }
 
-func testAccContainerCluster_withNodeConfigKubeletConfigSettings(clusterName, networkName, subnetworkName string) string {
+func testAccContainerCluster_withNodeConfigKubeletConfigSettingsBaseline(clusterName, networkName, subnetworkName string) string {
 	return fmt.Sprintf(`
 resource "google_container_cluster" "with_node_config_kubelet_config_settings" {
   name               = "%s"
@@ -6791,10 +6774,7 @@ resource "google_container_cluster" "with_node_config_kubelet_config_settings" {
 
   node_config {
     kubelet_config {
-      cpu_manager_policy   = "static"
-      cpu_cfs_quota        = true
-      cpu_cfs_quota_period = "100ms"
-      pod_pids_limit       = 2048
+      pod_pids_limit = 1024
     }
   }
   deletion_protection = false
@@ -6804,23 +6784,27 @@ resource "google_container_cluster" "with_node_config_kubelet_config_settings" {
 `, clusterName, networkName, subnetworkName)
 }
 
-func testAccContainerCluster_withInsecureKubeletReadonlyPortEnabledInNodeConfig(clusterName, networkName, subnetworkName, insecureKubeletReadonlyPortEnabled string) string {
+func testAccContainerCluster_withNodeConfigKubeletConfigSettingsUpdates(clusterName, cpuManagerPolicy, cpuCfsQuotaPeriod, insecureKubeletReadonlyPortEnabled, networkName, subnetworkName string, podPidsLimit int, cpuCfsQuota bool) string {
 	return fmt.Sprintf(`
-resource "google_container_cluster" "with_insecure_kubelet_readonly_port_enabled_in_node_config" {
+resource "google_container_cluster" "with_node_config_kubelet_config_settings" {
   name               = "%s"
   location           = "us-central1-f"
   initial_node_count = 1
 
   node_config {
     kubelet_config {
+      cpu_manager_policy                     = "%s"
+      cpu_cfs_quota                          = %v
+      cpu_cfs_quota_period                   = "%s"
       insecure_kubelet_readonly_port_enabled = "%s"
+      pod_pids_limit                         = %v
     }
   }
   deletion_protection = false
-  network    = "%s"
-  subnetwork    = "%s"
+  network             = "%s"
+  subnetwork          = "%s"
 }
-`, clusterName, insecureKubeletReadonlyPortEnabled, networkName, subnetworkName)
+`, clusterName, cpuManagerPolicy, cpuCfsQuota, cpuCfsQuotaPeriod, insecureKubeletReadonlyPortEnabled, podPidsLimit, networkName, subnetworkName)
 }
 
 func testAccContainerCluster_withInsecureKubeletReadonlyPortEnabledInNodePool(clusterName, nodePoolName, networkName, subnetworkName, insecureKubeletReadonlyPortEnabled string) string {