From 9ab00b69e5ae83e2f4e2a1db05618ec68d8cd9b7 Mon Sep 17 00:00:00 2001 From: Cameron Thornton Date: Mon, 25 Sep 2023 18:49:52 -0500 Subject: [PATCH] changes from code review, deprecate cluster_ipv4_cidr --- .../resource_container_cluster.go.erb | 37 ++++++++++--------- .../resource_container_cluster_test.go.erb | 29 +++++++++++++-- .../docs/r/container_cluster.html.markdown | 8 ++-- 3 files changed, 48 insertions(+), 26 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 399657c1f5b2..b1bd379ca249 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 @@ -196,7 +196,6 @@ func ResourceContainerCluster() *schema.Resource { containerClusterNetworkPolicyEmptyCustomizeDiff, containerClusterSurgeSettingsCustomizeDiff, containerClusterEnableK8sBetaApisCustomizeDiff, - containerClusterNetworkingModeCustomizeDiff, ), Timeouts: &schema.ResourceTimeout{ @@ -783,9 +782,10 @@ func ResourceContainerCluster() *schema.Resource { Optional: true, Computed: true, ForceNew: true, + Deprecated: "Deprecated in favor of ip_allocation_policy.cluster_ipv4_cidr_block", ValidateFunc: verify.OrEmpty(verify.ValidateRFC1918Network(8, 32)), ConflictsWith: []string{"ip_allocation_policy"}, - Description: `The IP address range of the Kubernetes pods in this cluster in CIDR notation (e.g. 10.96.0.0/14). Leave blank to have one automatically chosen or specify a /14 block in 10.0.0.0/8. This field will only work for routes-based clusters, where ip_allocation_policy is not defined.`, + Description: `The IP address range of the Kubernetes pods in this cluster in CIDR notation (e.g. 10.96.0.0/14). Leave blank to have one automatically chosen or specify a /14 block in 10.0.0.0/8. This field will only work for routes-based clusters, where ip_allocation_policy is not defined. Deprecated in favor of ip_allocation_policy.cluster_ipv4_cidr_block`, }, "description": { @@ -1598,13 +1598,14 @@ func ResourceContainerCluster() *schema.Resource { }, }, + // Defaults to "VPC_NATIVE" during create only "networking_mode": { Type: schema.TypeString, Optional: true, Computed: true, ForceNew: true, ValidateFunc: validation.StringInSlice([]string{"VPC_NATIVE", "ROUTES"}, false), - Description: `Determines whether alias IPs or routes will be used for pod IPs in the cluster.`, + Description: `Determines whether alias IPs or routes will be used for pod IPs in the cluster. Defaults to VPC_NATIVE for new clusters.`, }, "remove_default_node_pool": { @@ -2123,6 +2124,21 @@ func resourceContainerClusterCreate(d *schema.ResourceData, meta interface{}) er clusterName := d.Get("name").(string) + // Default to VPC_NATIVE mode during initial creation + // This solution (a conditional default) should not be considered to set a precedent on its own. + // If you're considering a similar approach on a different resource, strongly weigh making the field required. + // GKE tends to require exceptional handling in general- and this default was a breaking change in their API + // that was compounded on by numerous product developments afterwards. We have not seen a similar case + // since, after several years. + networkingMode := d.Get("networking_mode").(string) + clusterIpv4Cidr := d.Get("cluster_ipv4_cidr").(string) + if networkingMode == "" && clusterIpv4Cidr == "" { + err := d.Set("networking_mode", "VPC_NATIVE") + if err != nil { + return fmt.Errorf("Error setting networking mode during creation: %s", err) + } + } + ipAllocationBlock, err := expandIPAllocationPolicy(d.Get("ip_allocation_policy"), d.Get("networking_mode").(string), d.Get("enable_autopilot").(bool)) if err != nil { return err @@ -6167,21 +6183,6 @@ func containerClusterAutopilotCustomizeDiff(_ context.Context, d *schema.Resourc return nil } -// Default to VPC_NATIVE mode during initial creation -func containerClusterNetworkingModeCustomizeDiff(_ context.Context, d *schema.ResourceDiff, meta interface{}) error { - // skip if not during initial creation - o, _ := d.GetChange("name") - if o != "" { - return nil - } - - err := d.SetNew("networking_mode", "VPC_NATIVE") - if err != nil { - return fmt.Errorf("Error setting networking mode diff: %s", err) - } - return nil -} - // node_version only applies to the default node pool, so it should conflict with remove_default_node_pool = true func containerClusterNodeVersionRemoveDefaultCustomizeDiff(_ context.Context, d *schema.ResourceDiff, meta interface{}) error { // node_version is computed, so we can only check this on initial creation 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 81d5977d66f6..7d7044f5e5f1 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 @@ -57,14 +57,18 @@ func TestAccContainerCluster_basic(t *testing.T) { func TestAccContainerCluster_networkingModeRoutes(t *testing.T) { t.Parallel() - clusterName := fmt.Sprintf("tf-test-cluster-%s", acctest.RandString(t, 10)) + firstClusterName := fmt.Sprintf("tf-test-cluster-%s", acctest.RandString(t, 10)) + secondClusterName := fmt.Sprintf("tf-test-cluster-%s", acctest.RandString(t, 10)) acctest.VcrTest(t, resource.TestCase{ PreCheck: func() { acctest.AccTestPreCheck(t) }, ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories(t), CheckDestroy: testAccCheckContainerClusterDestroyProducer(t), Steps: []resource.TestStep{ { - Config: testAccContainerCluster_networkingModeRoutes(clusterName), + Config: testAccContainerCluster_networkingModeRoutes(firstClusterName, secondClusterName), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("google_container_cluster.primary", "networking_mode", "ROUTES"), + resource.TestCheckResourceAttr("google_container_cluster.secondary", "networking_mode", "ROUTES"), ), }, { ResourceName: "google_container_cluster.primary", @@ -72,6 +76,12 @@ func TestAccContainerCluster_networkingModeRoutes(t *testing.T) { ImportStateVerify: true, ImportStateVerifyIgnore: []string{"deletion_protection"}, }, + { + ResourceName: "google_container_cluster.secondary", + ImportState: true, + ImportStateVerify: true, + ImportStateVerifyIgnore: []string{"deletion_protection"}, + }, }, }) } @@ -2713,6 +2723,9 @@ func TestAccContainerCluster_withAutopilot(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccContainerCluster_withAutopilot(pid, containerNetName, clusterName, "us-central1", true, false, ""), + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("google_container_cluster.with_autopilot", "networking_mode", "VPC_NATIVE"), + ), }, { ResourceName: "google_container_cluster.with_autopilot", @@ -4286,7 +4299,7 @@ resource "google_container_cluster" "primary" { `, name) } -func testAccContainerCluster_networkingModeRoutes(name string) string { +func testAccContainerCluster_networkingModeRoutes(firstName, secondName string) string { return fmt.Sprintf(` resource "google_container_cluster" "primary" { name = "%s" @@ -4295,7 +4308,15 @@ resource "google_container_cluster" "primary" { networking_mode = "ROUTES" deletion_protection = false } -`, name) + +resource "google_container_cluster" "secondary" { + name = "%s" + location = "us-central1-a" + initial_node_count = 1 + cluster_ipv4_cidr = 10.96.0.0/14 + deletion_protection = false + } +`, firstName, secondName) } func testAccContainerCluster_misc(name string) string { diff --git a/mmv1/third_party/terraform/website/docs/r/container_cluster.html.markdown b/mmv1/third_party/terraform/website/docs/r/container_cluster.html.markdown index c055d2573916..1014d77b9377 100644 --- a/mmv1/third_party/terraform/website/docs/r/container_cluster.html.markdown +++ b/mmv1/third_party/terraform/website/docs/r/container_cluster.html.markdown @@ -133,10 +133,11 @@ the cluster. Unless this field is set to false in Terraform state, a `false`. This field should only be enabled for Autopilot clusters (`enable_autopilot` set to `true`). -* `cluster_ipv4_cidr` - (Optional) The IP address range of the Kubernetes pods +* `cluster_ipv4_cidr` - (Optional, DEPRECATED) The IP address range of the Kubernetes pods in this cluster in CIDR notation (e.g. `10.96.0.0/14`). Leave blank to have one automatically chosen or specify a `/14` block in `10.0.0.0/8`. This field will -only work for routes-based clusters, where `ip_allocation_policy` is not defined. +default a new cluster to routes-based, where `ip_allocation_policy` is not defined. +Deprecated in favor of `ip_allocation_policy.cluster_ipv4_cidr_block`. * `cluster_autoscaling` - (Optional) Per-cluster configuration of Node Auto-Provisioning with Cluster Autoscaler to @@ -200,8 +201,7 @@ making the cluster VPC-native instead of routes-based (unless `"networking_mode` below](#nested_ip_allocation_policy). * `networking_mode` - (Optional) Determines whether alias IPs or routes will be used for pod IPs in the cluster. -Options are `VPC_NATIVE` or `ROUTES`. `VPC_NATIVE` enables [IP aliasing](https://cloud.google.com/kubernetes-engine/docs/how-to/ip-aliases). -Newly created clusters will default to `VPC_NATIVE`. +Options are `VPC_NATIVE` or `ROUTES`. Newly created clusters will default to `VPC_NATIVE`. * `logging_config` - (Optional) Logging configuration for the cluster. Structure is [documented below](#nested_logging_config).