changes from code review, deprecate cluster_ipv4_cidr

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,26 +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
-	oId, _ := d.GetChange("id")
-
-	if oId != "" || nNetworkingMode != "" {
-		return nil
-	}
-
-	_, nNetworkingMode := d.GetChange("networking_mode")
-
-	if nNetworkingMode == "" {
-		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

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

@@ -57,21 +57,31 @@ 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",
 				ImportState:       true,
 				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 {

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).