add support for ip aliasing in `google_container_cluster` #654

davidquarles · 2017-10-30T23:06:15Z

Fixes #618.

Note that there are two mutually exclusive paths for GKE clusters with IP aliasing enabled:

pre-allocated subnets
dynamically created subnets, which are a bit more magical and strictly tied to cluster lifecycle

I chose to implement only (1), for now, because:

It seemed like the most flexible path
There are several tiers of conflicting and dependent attributes. ConflictsWith halfway solves the problem, but AFAIK there's no good abstraction on the other side for dependencies/requirements, making it difficult to implement both strategies and detect failures at plan-time. There are cases where I wasn't able to do so even with the single strategy.

davidquarles · 2017-10-30T23:13:36Z

I can run the full acceptance suite later tonight, but for now:

$ make testacc TEST=./google TESTARGS='-run=TestAccContainerCluster_withIPAllocationPolicy'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccContainerCluster_withIPAllocationPolicy -timeout 120m
=== RUN   TestAccContainerCluster_withIPAllocationPolicy
--- PASS: TestAccContainerCluster_withIPAllocationPolicy (643.20s)
PASS
ok  github.com/terraform-providers/terraform-provider-google/google643.235s

danawillow · 2017-11-07T20:57:42Z

google/resource_container_cluster.go

+							Type:     schema.TypeBool,
+							Optional: true,
+							Computed: true,
+							Default:  nil,


Whoa, I'm surprised Terraform lets you set the default for a boolean type to nil. If you're looking for a false default, you don't really need to have one explicitly set.

🤦‍♂️

danawillow · 2017-11-07T20:58:52Z

google/resource_container_cluster.go

+							ForceNew: true,
+							DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
+								if new == "" {
+									return true


Why suppress the diff if new is empty?

🤦‍♂️

danawillow · 2017-11-07T21:03:12Z

google/resource_container_cluster.go

+								if new == "" {
+									return true
+								}
+								return ipCidrRangeDiffSuppress(k, old, new, d)


Isn't this field the name, not the range itself?

🤦‍♂️

danawillow · 2017-11-07T21:16:59Z

google/resource_container_cluster.go

+			return err
+		}
+
+		region := getRegionFromZone(zoneName)


I'm not sure we actually need these checks inside of Terraform. If I get rid of them I get this error from the API: Google Compute Engine: services secondary range "pods" not found in subnet "container-net-asdfafw" which serves the same purpose

Sounds good to me.

danawillow · 2017-11-07T21:19:12Z

google/resource_container_cluster.go

+		"services_secondary_range_name": c.ServicesSecondaryRangeName,
+	}
+
+	//if c.UseIpAliases {


These can be removed :)

danawillow · 2017-11-07T21:24:02Z

google/resource_container_cluster_test.go

+}
+
+resource "google_compute_subnetwork" "container_subnetwork" {
+    name          = "${google_compute_network.container_network.name}"


can you fix the indentation here?

danawillow · 2017-11-07T21:27:13Z

google/resource_container_cluster_test.go

+	}`, rangeName, cidr))
+	}
+
+	var ipAllocationPolicy bytes.Buffer


I'm fine leaving this as is, but a potential alternative would be just passing in the configuration block as a string that looked like:

ip_allocation_policy { use_ip_aliases = "true" cluster_secondary_range_name = "pods" services_secondary_range_name = "services" }

danawillow · 2017-11-07T21:32:28Z

google/resource_container_cluster.go

+			"ip_allocation_policy": {
+				Type:     schema.TypeList,
+				Optional: true,
+				Computed: true,


Any reason why this is (and the others are) computed?

🤦‍♂️

davidquarles · 2017-11-15T09:25:12Z

@danawillow My apologies for the shoddy / incomplete initial PR. I was a bit sleep-deprived that week; all of your comments were right on. I made all the changes you asked for, and I removed use_ip_aliases param in favor of simply checking existence of the outer config block (as you suggested with master_authorized_networks_config). Let me know what you think. Thanks!

$ make testacc TEST=./google TESTARGS='-run=TestAccContainerCluster_withIPAllocationPolicy'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccContainerCluster_withIPAllocationPolicy -timeout 120m
=== RUN   TestAccContainerCluster_withIPAllocationPolicy
--- PASS: TestAccContainerCluster_withIPAllocationPolicy (1336.11s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	1336.136s

danawillow · 2017-11-15T18:07:06Z

website/docs/r/container_cluster.html.markdown

@@ -184,6 +186,18 @@ maintenance_policy {
 }
 ```

+The `ip_allocation_policy` block supports:
+
+* `cluster_secondary_range_name` - (Optional) The name of the secondary range to be


The last sentence of this (and the next) are no longer applicable.

danawillow · 2017-11-15T18:22:40Z

google/resource_container_cluster_test.go

+}
+
+resource "google_container_cluster" "with_ip_allocation_policy" {
+	name = "with-ip-allocation-policy"


Can you give this a name with some randomness? We usually do that so if the test fails and the resource is lingering, we can still run the test before cleaning it up again.

danawillow · 2017-11-15T18:40:20Z

google/resource_container_cluster.go

+			"ip_allocation_policy": {
+				Type:     schema.TypeList,
+				Optional: true,
+				MaxItems: 1,


Not sure it really makes a difference, but let's make this ForceNew as well

danawillow · 2017-11-15T18:43:20Z

google/resource_container_cluster.go

@@ -589,6 +609,10 @@ func resourceContainerClusterRead(d *schema.ResourceData, meta interface{}) erro
 	}
 	d.Set("node_pool", nps)

+	if cluster.IpAllocationPolicy != nil && cluster.IpAllocationPolicy.UseIpAliases {


Actually I think we should set it no matter what (and this might be a bug elsewhere in the file). If I create a cluster with IP Aliasing, then remove IP Aliasing outside of Terraform, I would want my state to get updated. It's sort of a moot point since IPAllocationPolicy doesn't support update right now, but this will help future-proof it.

Here's what I think flattenIPAllocationPolicy should look like:

func flattenIPAllocationPolicy(c *container.IPAllocationPolicy) []map[string]interface{} { result := []map[string]interface{}{} if c != nil && c.UseIpAliases { result = append(result, map[string]interface{}{ "cluster_secondary_range_name": c.ClusterSecondaryRangeName, "services_secondary_range_name": c.ServicesSecondaryRangeName, }) } return result }

and then the call to set it would be:

if err := d.Set("ip_allocation_policy", flattenIPAllocationPolicy(cluster.IpAllocationPolicy)); err != nil { return err }

You still need to do the nil check, otherwise the code will panic when trying to access the struct's fields (you can prove it by running TestAccContainerCluster_basic)

I just realized this comment slipped through the cracks and fixed it.

==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./google -v -run=TestAccContainerCluster_basic -timeout 120m === RUN TestAccContainerCluster_basic --- PASS: TestAccContainerCluster_basic (356.83s) PASS ok github.com/terraform-providers/terraform-provider-google/google 356.853s

danawillow · 2017-11-15T18:43:34Z

google/resource_container_cluster.go

@@ -925,6 +949,30 @@ func expandClusterAddonsConfig(configured interface{}) *container.AddonsConfig {
 	return ac
 }

+func expandIPAllocationPolicy(configured interface{}) (*container.IPAllocationPolicy, error) {


I think you accidentally got rid of the call to this function

Yikes, yeah... Fixed!

danawillow · 2017-11-15T18:45:17Z

google/resource_container_cluster_test.go

+					},
+				),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckContainerCluster(


Can you add checks for ip_allocation_policy into testAccCheckContainerCluster? I also wouldn't mind seeing some resource.TestCheckResourceAttrs but I won't block on those

Done and done

davidquarles

$ make testacc TEST=./google TESTARGS='-run=TestAccContainerCluster_withIPAllocationPolicy'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccContainerCluster_withIPAllocationPolicy -timeout 120m
=== RUN   TestAccContainerCluster_withIPAllocationPolicy
--- PASS: TestAccContainerCluster_withIPAllocationPolicy (914.36s)
PASS
ok  	github.com/terraform-providers/terraform-provider-google/google	914.391s

davidquarles · 2017-11-16T05:05:51Z

google/resource_container_cluster.go

+			"ip_allocation_policy": {
+				Type:     schema.TypeList,
+				Optional: true,
+				MaxItems: 1,


davidquarles · 2017-11-16T05:06:17Z

google/resource_container_cluster.go

@@ -589,6 +609,10 @@ func resourceContainerClusterRead(d *schema.ResourceData, meta interface{}) erro
 	}
 	d.Set("node_pool", nps)

+	if cluster.IpAllocationPolicy != nil && cluster.IpAllocationPolicy.UseIpAliases {


davidquarles · 2017-11-16T05:07:04Z

google/resource_container_cluster.go

@@ -925,6 +949,30 @@ func expandClusterAddonsConfig(configured interface{}) *container.AddonsConfig {
 	return ac
 }

+func expandIPAllocationPolicy(configured interface{}) (*container.IPAllocationPolicy, error) {


Yikes, yeah... Fixed!

davidquarles · 2017-11-17T02:46:31Z

website/docs/r/container_cluster.html.markdown

@@ -184,6 +186,18 @@ maintenance_policy {
 }
 ```

+The `ip_allocation_policy` block supports:
+
+* `cluster_secondary_range_name` - (Optional) The name of the secondary range to be


…_policy`

danawillow · 2017-11-27T23:15:00Z

Thanks @davidquarles!

) * add support for ip aliasing in `google_container_cluster` * [review] cleanup galore, infer feature enablement from `ip_allocation_policy` * [review] cleanup, round 2 * add nil check back (when reading ip allocation policy from API)

…end-service-changelog Add region_backend_service to CHANGELOG

ghost · 2020-03-30T14:11:20Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

danawillow self-requested a review November 7, 2017 20:44

danawillow self-assigned this Nov 7, 2017

danawillow suggested changes Nov 7, 2017

View reviewed changes

paddycarver added the waiting-response label Nov 8, 2017

davidquarles force-pushed the enable-ip-alias branch from 78fb4ac to d6cffe2 Compare November 15, 2017 09:09

danawillow suggested changes Nov 15, 2017

View reviewed changes

davidquarles commented Nov 17, 2017

View reviewed changes

davidquarles added 4 commits November 22, 2017 05:56

add support for ip aliasing in google_container_cluster

9cc4636

[review] cleanup galore, infer feature enablement from `ip_allocation…

0ecb973

…_policy`

[review] cleanup, round 2

8825913

add nil check back (when reading ip allocation policy from API)

a210966

davidquarles force-pushed the enable-ip-alias branch from d05e74b to a210966 Compare November 22, 2017 14:16

danawillow approved these changes Nov 27, 2017

View reviewed changes

danawillow removed the waiting-response label Nov 27, 2017

danawillow merged commit d57db91 into hashicorp:master Nov 27, 2017

luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019

Merge pull request hashicorp#654 from terraform-providers/region-back…

45a5978

…end-service-changelog Add region_backend_service to CHANGELOG

ghost locked and limited conversation to collaborators Mar 30, 2020

add support for ip aliasing in google_container_cluster #654

add support for ip aliasing in google_container_cluster #654

Conversation

davidquarles commented Oct 30, 2017

davidquarles commented Oct 30, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidquarles commented Nov 15, 2017

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

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

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

davidquarles 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

Choose a reason for hiding this comment

danawillow commented Nov 27, 2017

ghost commented Mar 30, 2020

add support for ip aliasing in `google_container_cluster` #654

add support for ip aliasing in `google_container_cluster` #654