From 4a61921e22a9e9cab994f3e3f5ae719bd267c827 Mon Sep 17 00:00:00 2001 From: William Yardley Date: Thu, 12 Sep 2024 22:45:22 -0700 Subject: [PATCH] container: fix `node_config.kubelet_config` updates - Fix updates for fields within `node_config.kubelet_config` where updates didn't function properly (basically all except for `insecure_kubelet_readonly_port_enabled`. - Consolidate test cases for items under `node_config.kubelet_config` with the one for `node_config.kubelet_config.insecure_kubelet_readonly_port_enabled` Part of https://github.com/hashicorp/terraform-provider-google/issues/19225 --- .../resource_container_cluster.go.erb | 56 ++++++++--------- .../resource_container_cluster_test.go.erb | 60 +++++++------------ 2 files changed, 47 insertions(+), 69 deletions(-) diff --git a/mmv1/third_party/terraform/services/container/resource_container_cluster.go.erb b/mmv1/third_party/terraform/services/container/resource_container_cluster.go.erb index 4cdf23a8d97f..50c043ae877d 100644 --- a/mmv1/third_party/terraform/services/container/resource_container_cluster.go.erb +++ b/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") { diff --git a/mmv1/third_party/terraform/services/container/resource_container_cluster_test.go.erb b/mmv1/third_party/terraform/services/container/resource_container_cluster_test.go.erb index 3388dba3af02..c860e2710456 100644 --- a/mmv1/third_party/terraform/services/container/resource_container_cluster_test.go.erb +++ b/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,25 +1604,8 @@ 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(), @@ -1635,16 +1613,21 @@ func TestAccContainerCluster_withInsecureKubeletReadonlyPortEnabledInNodeConfigU }, }, { - 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"}, @@ -6756,7 +6739,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" @@ -6765,10 +6748,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 @@ -6778,23 +6758,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 {