Add support for regional GKE clusters in google_container_cluster #1181

ashish-amarnath · 2018-03-12T00:43:58Z

This is a work in progress PR.
An initial set of changes to share and get feedback on the approach.

danawillow

Thanks @ashish-amarnath for taking on this feature! Here are some initial comments.

danawillow · 2018-03-13T22:19:23Z

google/resource_container_cluster.go

@@ -393,7 +412,13 @@ func resourceContainerCluster() *schema.Resource {
 }

 func resourceContainerClusterCreate(d *schema.ResourceData, meta interface{}) error {
-	containerApiVersion := getContainerApiVersion(d, ContainerClusterBaseApiVersion, ContainerClusterVersionedFeatures)
+	_, isRegionalCluster := d.GetOk("region")
+	if isRegionalCluster {


This block isn't necessary- you can just add the feature to the var as defined at the top of the file. The name of it should be the name of the field in the schema, so "region"

Got it! Thanks.

danawillow · 2018-03-13T22:19:46Z

google/resource_container_cluster.go

@@ -447,19 +472,35 @@ func resourceContainerClusterCreate(d *schema.ResourceData, meta interface{}) er
 	}

 	if v, ok := d.GetOk("additional_zones"); ok {
+		if isRegionalCluster {
+			return fmt.Errorf("'additional_zones' is incompatible with regional clusters")


This should be doable with ConflictsWith in the schema

Thank you. That should simplify this.

danawillow · 2018-03-13T22:21:43Z

google/resource_container_cluster.go

+				Elem:     &schema.Schema{Type: schema.TypeString},
+			},
+
+			"num_nodes": {


This doesn't seem to get used anywhere

I see that this is not a new field added to support regional clusters. So this is not relevant to this PR. Removing this. But we'll have to add this later.

danawillow · 2018-03-13T22:26:29Z

google/resource_container_cluster.go

 			}
 		}
 		locations = append(locations, zoneName)
 		cluster.Locations = locations
 	}

+	if isRegionalCluster {
+		locations := []string{}
+		locations = append(locations, d.Get("region").(string))


I think the locations field is still supposed to be just a list of zones- this will likely still work without this entire block (and the node_locations field)

I see that this is not a new field added to support regional clusters. So this is not relevant to this PR. Removing this. But we'll have to add this later. As additional_zones is getting deprecated.

danawillow · 2018-03-13T22:26:53Z

google/resource_dataproc_cluster.go

@@ -797,7 +797,7 @@ func flattenPreemptibleInstanceGroupConfig(d *schema.ResourceData, icg *dataproc
 func flattenInstanceGroupConfig(d *schema.ResourceData, icg *dataproc.InstanceGroupConfig) []map[string]interface{} {
 	disk := map[string]interface{}{}
 	data := map[string]interface{}{
-	//"instance_names": []string{},
+		//"instance_names": []string{},


this can be reverted, it's not really relevant here

ashish-amarnath · 2018-03-19T18:04:47Z

@danawillow I took another stab at this and I need some help to update the vendor/google.golang.org/api/container/v1beta1/container-gen.go file with a field for region and also use the right API to create regional clusters so that the unit test can pass.

danawillow · 2018-03-19T22:09:04Z

The API as it is in the vendor directory should work. Instead of .Projects.Zones you'll want.Projects.Locations and then call the API as documented at https://cloud.google.com/kubernetes-engine/docs/reference/rest/v1beta1/projects.locations.clusters/create
(actual code at https://github.com/terraform-providers/terraform-provider-google/blob/master/vendor/google.golang.org/api/container/v1beta1/container-gen.go#L3623)

ashish-amarnath · 2018-03-19T23:04:10Z

Thanks for those pointers. I should be able to make more progress with this.

ashish-amarnath · 2018-03-20T23:50:07Z

The test is now able to create regional GKE clusters.
POST /v1beta1/projects/nordstrom-k8s-dev/locations/us-central1/clusters?alt=json HTTP/1.1 Host: container.googleapis.com User-Agent: google-api-go-client/0.5 (darwin amd64) Terraform/0.11.3-dev Content-Length: 464 Content-Type: application/json Accept-Encoding: gzip
this request is successful. The test however, still fails when trying to
---[ REQUEST ]--------------------------------------- GET /v1/projects/nordstrom-k8s-dev/zones//operations/operation-1521589157203-edadd8b8?alt=json HTTP/1.1 Host: container.googleapis.com User-Agent: google-api-go-client/0.5 (darwin amd64) Terraform/0.11.3-dev Accept-Encoding: gzip

This is most likely because the support is only implemented to create regional clusters but not to update, read or destroy. Next step: Make this work!

Stono · 2018-03-21T09:47:23Z

@ashish-amarnath I just wanted to say thanks for your efforts with this, I don't know enough golang to help out with the implementation but if i (we - people on my team) can help with testing in any way just ask

ashish-amarnath · 2018-03-21T20:34:15Z

@Stono Thanks for offering to help! I will surely ask for help validating this change. Right now I am trying to a checkpoint where this is can spin-up and teardown regional clusters. This is not the top priority for us right now so I am trying to backfill my time working on this. For that reason, and my unfamiliarity with this codebase are the reasons for the slow progress. I am not blocked. @danawillow has been super helpful in unblocking me :)

ashish-amarnath · 2018-03-22T23:50:52Z

Made more progress on this. I now have code and unit test to successfully create and destroy regional GKE clusters. Next, I need to work on the Update operation and then there are merge conflicts that I have to resolve, of course.

ashish-amarnath · 2018-03-23T05:40:29Z

@danawillow this PR is ready for more feedback. I need to add some more tests. Suggestions are welcome :)
@Stono Feel free to take a look and make comments.

danawillow · 2018-03-23T17:27:49Z

google/resource_container_cluster.go

-		if err != nil {
-			return err
+
+		if isRegionalCluster {


Since the location could also be the zone, I don't think we actually need this conditional- it should be fine to just always do the Projects.Locations call

That made sense. But didn't what that change to have unknown side-effects. I will give that a try.

danawillow · 2018-03-23T17:29:46Z

google/resource_container_cluster.go

 	}

 	cluster := &containerBeta.Cluster{}
+	var clust interface{}


Did this work for you when you tested it? I've tried this approach (just doing the Convert from an interface at the end) in the past and it failed for me because Convert needed the parameter passed to be a struct of a certain type (so it can recurse through the fields).

Well.. It compiled right and the tests passed. From that, I figured it works. Any other tests I can run to be doubly sure?

@danawillow What did you to break this? I'd like to make sure this will not break later.

Looks like it works! I must have done something slightly differently when I tried doing it this way in the past.

danawillow · 2018-03-23T17:32:26Z

google/resource_container_cluster.go

+		switch containerAPIVersion {
+		case v1:
+			op, err = config.clientContainer.Projects.Zones.Clusters.Update(
+				project, zoneName, clusterName, req).Do()


This change makes me nervous- it feels a bit brittle to me to have this depending on what the value of req is at the time this function gets called. If you'd like to refactor it out, I think wrapping it in another function that takes in req as a parameter and returns this func would be a safer choice.

I agree. For that exact reason, I tried going the function route but quickly realised that that number of args to that function became too many and that made me a little uncomfortable. That function would need config, project, zone, region, isRegionalCluster, request, clusterName, timeout, apiVersion.
Total of 9 args. But, collapsing zone and region into location that number could go down to 8.

Ah yeah, I was more just thinking that only req would be the parameter since it's the only thing that would change between requests.

danawillow · 2018-03-23T17:33:54Z

google/resource_container_cluster.go

@@ -1001,15 +1031,33 @@ func resourceContainerClusterUpdate(d *schema.ResourceData, meta interface{}) er
 		}

 		updateF := func() error {
-			log.Println("[DEBUG] emilyye updating enable_legacy_abac")
-			op, err := config.clientContainer.Projects.Zones.Clusters.LegacyAbac(project, zoneName, clusterName, req).Do()
+			log.Println("[DEBUG] updating enable_legacy_abac")


danawillow · 2018-03-23T17:35:20Z

google/resource_container_cluster_test.go

+		Steps: []resource.TestStep{
+			{
+				Config: testAccContainerCluster_regional(clusterName),
+			},


Can you add another test step that does an import? There are a bunch of examples elsewhere in this file.

danawillow · 2018-03-23T17:50:33Z

google/resource_container_cluster.go

+	var zoneName string
+	locations := []string{}
+	regionName, isRegionalCluster := d.GetOk("region")
+	if !isRegionalCluster {


Here's an idea- what if instead of separate "region" and "zone" fields, we just had a "location" field? Then once regional clusters have gone GA we can deprecate zone. I'm not totally set on this, but it does seem like it would simplify the logic. What do you think?

Thought about that. But because that would increase the surface area of this change I decided against it. I'll do this if that is not too bad. If that makes sense :)

Yup, that's fair. I do think that having it as two separate fields probably makes it a bit clearer to users what to put, so I'm fine with that.

ashish-amarnath · 2018-04-02T20:21:54Z

I've had other things taking up my time and haven't had a chance to work on this for a while. Just so you don't think I've vanished off the face of the Earth!

Stono · 2018-04-03T12:12:22Z

Hey,
If there is anything we can do to help progress this please let us know!

drubin · 2018-04-03T16:44:06Z

@ashish-amarnath @danawillow Thanks for all the hard work on this.

I am very interested in this PR but since there is no upgrade path from zone-based clusters to regional ones and I have to set up a few clusters in the near future, I am curious what might be the best approach to take in terms of getting them into terraform after?

I see 2 options.

Use the branch in its current state and hope we don't make breaking changes to the state management, (this looks pretty stable but I don't think this is a great option)
Hope we get a release in the next 2 weeks? (This also feels wrong any thing could happen)
Create the regional clusters with gcloud CLI and hopefully import them later?

Any recommendations or even timelines? Even if this was merged how often does the provider get released? Or do you need any help testing this?

danawillow · 2018-04-03T18:07:52Z

@ashish-amarnath, I think there were just a few small changes that were remaining, right? I don't mind making them and pushing them to your branch if you're all right with that. Seems like people are pretty eager for this :)

@drubin, we usually aim to release every 2 weeks or so, with one aimed for the next few days. So if this gets in today it'll be in the next release, if not probably not. Even though I think the schema for this is set, I'm also pretty hesitant about solution 1. If this doesn't make it into the next release, then I think solution 3 will be totally fine.

* implement operation wait for v1beta1 api * implement container clusters get for regional clusters * implement container clusters delete for regional cluster * implement container clusters update for regional cluster * simplify logic by using generic 'location' instead of 'zone' and 'region' * implement a method to generate the update function and refactor * rebase and fix

ashish-amarnath · 2018-04-03T23:29:46Z

I haven't had the time to pick-up where I left off on this.
@danawillow If you want to pick this up I would totally be OK with it. I don't want to hold this up for everyone. The last bit that was left on this was adding a test step that does an import. I remember taking an initial stab at it but somewhere while executing the test it would fail trying to use a v1 API to access the test cluster. I had to figure out where and why and then fix it.

danawillow · 2018-04-05T01:02:16Z

hey @rosbo, since the changes I made are non-trivial I wouldn't mind having you look at this as well if you don't mind

rosbo

Only one small comment.

rosbo · 2018-04-05T17:12:55Z

google/resource_container_cluster.go

-			locations = append(locations, location)
-			if location == zoneName {
-				return fmt.Errorf("additional_zones should not contain the original 'zone'.")
+			loc := v.(string)


Consider simplifying this by using convertStringSet and schema.Set#Contains.

Also I am not sure why we have this block below:

if isZone(location) { locations = append(locations, location) }

Why are we adding the "main" zone to the list of additional zones... Consider adding a comment to clarify this.

…shicorp#1181) * Add support for regional GKE clusters in google_container_cluster: * implement operation wait for v1beta1 api * implement container clusters get for regional clusters * implement container clusters delete for regional cluster * implement container clusters update for regional cluster * simplify logic by using generic 'location' instead of 'zone' and 'region' * implement a method to generate the update function and refactor * rebase and fix * reorder container_operation fns * cleanup * add import support and docs * additional locations cleanup

ghost · 2018-11-19T13:53:43Z

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!

ashish-amarnath mentioned this pull request Mar 12, 2018

Add support for regional GKE clusters in google_container_cluster #1170

Closed

danawillow self-requested a review March 12, 2018 15:51

danawillow self-assigned this Mar 12, 2018

danawillow suggested changes Mar 13, 2018

View reviewed changes

ashish-amarnath force-pushed the gke-regional-clusters-support branch from fc3d5d8 to d7734e6 Compare March 20, 2018 23:49

ashish-amarnath force-pushed the gke-regional-clusters-support branch 4 times, most recently from 47050ab to 6d1bc50 Compare March 23, 2018 05:34

danawillow reviewed Mar 23, 2018

View reviewed changes

ashish-amarnath force-pushed the gke-regional-clusters-support branch from 8e4f9c7 to fc27105 Compare March 23, 2018 22:49

ashish-amarnath closed this Apr 2, 2018

ashish-amarnath reopened this Apr 2, 2018

ashish-amarnath force-pushed the gke-regional-clusters-support branch from fc27105 to 1e1ace0 Compare April 3, 2018 23:23

danawillow added 2 commits April 4, 2018 14:17

reorder container_operation fns

109e83b

cleanup

c009d98

add import support and docs

4f7af96

danawillow changed the title ~~[WIP] Add support for regional GKE clusters in google_container_cluster~~ Add support for regional GKE clusters in google_container_cluster Apr 4, 2018

danawillow requested a review from rosbo April 5, 2018 01:01

rosbo reviewed Apr 5, 2018

View reviewed changes

additional locations cleanup

0138da9

danawillow approved these changes Apr 5, 2018

View reviewed changes

rosbo approved these changes Apr 5, 2018

View reviewed changes

danawillow merged commit f488a6e into hashicorp:master Apr 5, 2018

danawillow mentioned this pull request Apr 5, 2018

Support for regional rather than zonal clusters #829

Closed

Stono mentioned this pull request Apr 6, 2018

node_pools don't support regional clusters #1300

Closed

ashish-amarnath deleted the gke-regional-clusters-support branch April 10, 2018 18:05

michaelbannister mentioned this pull request Aug 1, 2018

Update of pod_security_policy_config fails for regional clusters #1833

Closed

ghost locked and limited conversation to collaborators Nov 19, 2018

Add support for regional GKE clusters in google_container_cluster #1181

Add support for regional GKE clusters in google_container_cluster #1181

Conversation

ashish-amarnath commented Mar 12, 2018

danawillow 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

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

ashish-amarnath commented Mar 19, 2018

danawillow commented Mar 19, 2018

ashish-amarnath commented Mar 19, 2018

ashish-amarnath commented Mar 20, 2018

Stono commented Mar 21, 2018

ashish-amarnath commented Mar 21, 2018

ashish-amarnath commented Mar 22, 2018 • edited Loading

ashish-amarnath commented Mar 23, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashish-amarnath commented Apr 2, 2018

Stono commented Apr 3, 2018

drubin commented Apr 3, 2018

danawillow commented Apr 3, 2018

ashish-amarnath commented Apr 3, 2018

danawillow commented Apr 5, 2018

rosbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Nov 19, 2018

ashish-amarnath commented Mar 22, 2018 •

edited

Loading