Expose additional properties during cluster creation #384

brianhealeyRMN · 2017-09-03T18:47:22Z

Allow for sending IssueClientCertificate as part of the cluster creation request
Allow for sending the Management object as part of the cluster creation request
Allow for sending the AutoScale object as part of the cluster creation request

Allow for sending IssueClientCertificate as part of the cluster creation request Allow for sending the Management object as part of the cluster creation request Allow for sending the AutoScale object as part of the cluster creation request

danawillow

Thanks so much @brianhealeyRMN for the PR! I'm excited to see these improvements to our GKE handling in Terraform.

This PR had a few test failures, mind fixing them?

$ make testacc TEST=./google TESTARGS='-run=TestAccContainerCluster_withNodePoolManagement'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccContainerCluster_withNodePoolManagement -timeout 120m
=== RUN   TestAccContainerCluster_withNodePoolManagement
--- FAIL: TestAccContainerCluster_withNodePoolManagement (2.65s)
	testing.go:435: Step 0 error: Error applying: 1 error(s) occurred:

		* google_container_cluster.with_node_pool_node_management: 1 error(s) occurred:

		* google_container_cluster.with_node_pool_node_management: googleapi: Error 400: Cannot have autoUpgrade or autoRepair set while image is CONTAINER_VM., badRequest
FAIL

$ make testacc TEST=./google TESTARGS='-run=TestAccContainerCluster_withNodePoolAutoScaling'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./google -v -run=TestAccContainerCluster_withNodePoolAutoScaling -timeout 120m
=== RUN   TestAccContainerCluster_withNodePoolAutoScaling
--- FAIL: TestAccContainerCluster_withNodePoolAutoScaling (0.02s)
	testing.go:435: Step 0 error: Configuration is invalid.

		Warnings: []string(nil)

		Errors: []string{"google_container_cluster.with_node_pool_node_auto_scaling: node_pool.0.node_config.0: invalid or unknown key: auto_scaling"}
FAIL

danawillow · 2017-09-11T07:51:41Z

google/resource_container_cluster.go

+						"issue_client_certificate": {
+							Type:             schema.TypeBool,
+							Optional:         true,
+							DiffSuppressFunc: linkDiffSuppress,


Since this is a boolean, it shouldn't need this particular DiffSuppressFunc

danawillow · 2017-09-11T07:53:45Z

google/resource_container_cluster.go

@@ -66,6 +66,11 @@ func resourceContainerCluster() *schema.Resource {
 							Required: true,
 							ForceNew: true,
 						},
+						"issue_client_certificate": {


Looks like the other elements here are roughly alphabetical- mind maintaining that?

Updated the order for the schema to be alphabetical

danawillow · 2017-09-11T07:55:58Z

google/resource_container_cluster.go

@@ -266,6 +271,60 @@ func resourceContainerCluster() *schema.Resource {
 						},

 						"node_config": schemaNodeConfig,
+
+						"management": {


likewise (alphabetical)

danawillow · 2017-09-11T07:59:22Z

google/resource_container_cluster.go

+							Elem: &schema.Resource{
+								Schema: map[string]*schema.Schema{
+
+									"enabled": {


Consider removing this boolean and using the presence of the autoscaling object instead (this is what was done in #157)

Modified as suggested

danawillow · 2017-09-11T08:04:39Z

google/resource_container_cluster.go

+							},
+						},
+
+						"auto_scaling": {


Can this be autoscaling to match what's in google_container_node_pool and the API docs (which has it as one word)?

Modified as suggested

danawillow · 2017-09-11T08:07:38Z

google/resource_container_cluster.go

+			"client_certificate":       cluster.MasterAuth.ClientCertificate,
+			"client_key":               cluster.MasterAuth.ClientKey,
+			"cluster_ca_certificate":   cluster.MasterAuth.ClusterCaCertificate,
+			"issue_client_certificate": true,


Instead of setting it to true here and potentially overriding it later, how about just set it later?

Not a problem. Default behavior is "true". This is a flag vs an actual setting so I added code so that it behaves "as a setting" by checking for the existence of the client certificate on the response

danawillow · 2017-09-11T08:11:30Z

google/resource_container_cluster_test.go

@@ -635,6 +687,7 @@ resource "google_container_cluster" "with_master_auth" {
 	master_auth {
 		username = "mr.yoda"
 		password = "adoy.rm"
+		issue_client_certificate = false


Does this change anything from what was there before? I think if you want to test issue_client_certificate it probably makes more sense to set it to true

True is the default behavior, so setting it to false does change, however I think the larger concern is that both cases were not being tested. I added test specifically to test true/false

danawillow · 2017-09-11T08:13:48Z

google/resource_container_cluster.go

@@ -66,6 +66,11 @@ func resourceContainerCluster() *schema.Resource {
 							Required: true,
 							ForceNew: true,
 						},
+						"issue_client_certificate": {
+							Type:             schema.TypeBool,
+							Optional:         true,


I think this needs to be ForceNew as well.

Ordered the schema alphabetically. Modified the behavior of issue_client_certificate to not default then override the value Updated integration tests to include tests with and without issue_client_certificate Fixed the integration tests for Management and AutoScale Updated autoscale tage to match the google schema convention.

brianhealeyRMN

Please see the updated PR based upon your feedback

brianhealeyRMN · 2017-09-16T19:39:19Z

Thanks @danawillow
I updated the the PR with your suggestions and fixed the integration tests. I also added additional tests to provide better coverage.

danawillow · 2017-09-26T16:43:57Z

google/resource_container_cluster.go

@@ -304,6 +360,12 @@ func resourceContainerClusterCreate(d *schema.ResourceData, meta interface{}) er
 			Password: masterAuth["password"].(string),
 			Username: masterAuth["username"].(string),
 		}
+
+		if v, ok := masterAuth["issue_client_certificate"]; ok {


nit: since you set a default, you can remove this if statement

danawillow · 2017-09-26T16:44:51Z

google/resource_container_cluster.go

+
+				// default behavior is disabled. Set to true as the cluster has it defined giving the intent to enable
+				// it
+				nodePool.Autoscaling.Enabled = true


nit: you can put this in the block on line 480

danawillow · 2017-09-26T16:45:35Z

google/resource_container_cluster.go

+				// it
+				nodePool.Autoscaling.Enabled = true
+
+				var minNodeCount int


I don't think you need this var

danawillow · 2017-09-26T16:47:00Z

google/resource_container_cluster.go

+								Schema: map[string]*schema.Schema{
+									"min_node_count": {
+										Type:         schema.TypeInt,
+										Optional:     true,


I'm pretty sure the API requires these to be set, no? https://github.com/terraform-providers/terraform-provider-google/blob/master/google/resource_container_node_pool.go#L78 has it as Required

danawillow · 2017-09-26T16:49:05Z

google/resource_container_cluster.go

+			}
+		}
+
+		if np.Autoscaling != nil {


you probably also want to check that np.Autoscaling.Enabled

davidquarles · 2017-10-27T04:29:40Z

Yeah, this is awesome @brianhealeyRMN! @danawillow and I were discussing version upgrades in #633, which led me here. Would it be possible to extend the nodePool.Management work to target the node pools directly, rather than (or in addition to?) the parent cluster resource, and to handle the update lifecycle as well? I can totally help, if you'd like to collaborate. I can also open a new issue and start from scratch, if this is entirely out of scope here.

justinsb · 2018-02-07T03:19:27Z

I would love to see the issue_client_certificate support land, that was implemented in this PR. @brianhealeyRMN do you think you might rebase or split this PR up? Or would you mind if I (or someone else, if anyone else is keen) added that support?

danawillow · 2018-02-07T17:39:23Z

I'm not @brianhealeyRMN, but I think you should go for it @justinsb :)

paddycarver · 2018-06-20T00:37:24Z

This PR has been inactive for a long time, with no response, so I'm going to close it out. If the author wants to carry on with it, feel free to post in this thread, and we can re-open it. If someone else is interested in picking this up, feel free to open a new PR for it.

ghost · 2018-11-17T14:16:32Z

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 👉 hashibot-feedback@hashicorp.com. Thanks!

Expose additional cluster properties

f5c8229

Allow for sending IssueClientCertificate as part of the cluster creation request Allow for sending the Management object as part of the cluster creation request Allow for sending the AutoScale object as part of the cluster creation request

brianhealeyRMN changed the title ~~Expose additional cluster properties~~ Expose additional properties during cluster creation Sep 3, 2017

rosbo requested review from danawillow and rosbo September 7, 2017 20:20

paddycarver requested review from paddycarver and removed request for rosbo September 7, 2017 20:20

danawillow suggested changes Sep 11, 2017

View reviewed changes

brianhealeyRMN commented Sep 16, 2017

View reviewed changes

Ran make fmt on the source

5b20621

danawillow suggested changes Sep 26, 2017

View reviewed changes

danawillow self-assigned this Sep 26, 2017

radeksimko added the enhancement label Oct 8, 2017

davidquarles mentioned this pull request Oct 27, 2017

Add support for version upgrades in google_container_node_pool #633

Closed

This was referenced Oct 30, 2017

Add support for auto-{upgrade,healing} in google_container_node_pool #655

Closed

Add support for google_container_node_pool management #669

Merged

paddycarver added the waiting-response label Nov 7, 2017

paddycarver closed this Jun 20, 2018

ghost locked and limited conversation to collaborators Nov 17, 2018

ghost removed the waiting-response label Nov 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose additional properties during cluster creation #384

Expose additional properties during cluster creation #384

brianhealeyRMN commented Sep 3, 2017

danawillow left a comment

danawillow Sep 11, 2017

brianhealeyRMN Sep 16, 2017

danawillow Sep 11, 2017

brianhealeyRMN Sep 16, 2017 •

edited

Loading

danawillow Sep 11, 2017

brianhealeyRMN Sep 16, 2017

danawillow Sep 11, 2017

brianhealeyRMN Sep 16, 2017

danawillow Sep 11, 2017

brianhealeyRMN Sep 16, 2017

danawillow Sep 11, 2017

brianhealeyRMN Sep 16, 2017 •

edited

Loading

danawillow Sep 11, 2017

brianhealeyRMN Sep 16, 2017

danawillow Sep 11, 2017

brianhealeyRMN Sep 16, 2017

brianhealeyRMN left a comment

brianhealeyRMN commented Sep 16, 2017

danawillow Sep 26, 2017

danawillow Sep 26, 2017

danawillow Sep 26, 2017

danawillow Sep 26, 2017

danawillow Sep 26, 2017

davidquarles commented Oct 27, 2017

justinsb commented Feb 7, 2018

danawillow commented Feb 7, 2018

paddycarver commented Jun 20, 2018

ghost commented Nov 17, 2018

Expose additional properties during cluster creation #384

Expose additional properties during cluster creation #384

Conversation

brianhealeyRMN commented Sep 3, 2017

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

brianhealeyRMN Sep 16, 2017 • edited Loading

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

brianhealeyRMN Sep 16, 2017 • edited Loading

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

brianhealeyRMN left a comment

Choose a reason for hiding this comment

brianhealeyRMN commented Sep 16, 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

davidquarles commented Oct 27, 2017

justinsb commented Feb 7, 2018

danawillow commented Feb 7, 2018

paddycarver commented Jun 20, 2018

ghost commented Nov 17, 2018

brianhealeyRMN Sep 16, 2017 •

edited

Loading

brianhealeyRMN Sep 16, 2017 •

edited

Loading