providers/google: Add support for Internal Load Balancing #10453

danawillow · 2016-12-01T00:58:43Z

This change enables Internal Load Balancing with the introduction of two new resources: google_compute_region_backend_service and google_compute_health_check.

For all intents and purposes, this is ready for review. I still need to add documentation.

@evandbrown

cblecker · 2016-12-01T18:46:27Z

@danawillow: Looks like it needs to be rebased on master, due to vendor updates :)

… it can only be INTERNAL

danawillow · 2016-12-01T20:34:29Z

All right, now it's ready.

paddycarver

Hi! Thanks for the PR! It brings a lot of good stuff, and it's really exciting to be able to add all this to Terraform! My apologies for taking so long to get to it--it's been hard finding a large enough chunk of time for it. I think smaller PRs are easier to review, but sometimes they don't split up easily, so it's always a judgement call. :)

Anyways this looks great! There were a few things I noted, hopefully not too nitpicky, that I could totally be wrong about. But I just wanted to draw your attention to them.

Thanks!

paddycarver · 2016-12-07T19:38:53Z

builtin/providers/google/resource_compute_forwarding_rule.go

@@ -93,13 +127,24 @@ func resourceComputeForwardingRuleCreate(d *schema.ResourceData, meta interface{
 		return err
 	}

+	ps := d.Get("ports").(*schema.Set).List()


This is an optional attribute. What if it's not specified--wouldn't this panic?

It doesn't look like it does- the tests that don't set ports still pass, like https://github.com/danawillow/terraform/blob/2009d6a1357fe16b1f98e3a652fd13631242ac5b/builtin/providers/google/resource_compute_forwarding_rule_test.go#L119.

You're totally right, and I learned some new stuff about *ResourceData.Get and *schema.Set.List today. Thanks!

paddycarver · 2016-12-07T19:39:44Z

builtin/providers/google/resource_compute_forwarding_rule.go

-		Name:        d.Get("name").(string),
-		PortRange:   d.Get("port_range").(string),
-		Target:      d.Get("target").(string),
+		BackendService:      d.Get("backend_service").(string),


Same here--isn't this nil if it's not set? It's an optional attribute, I think?

The only required attribute in the whole list is Name, so it seems there's a precedent for doing it this way.

You're totally right, I misunderstood the situations in which d.Get would return nil. Whoops! Thanks for correcting me. :D

paddycarver · 2016-12-07T19:40:28Z

builtin/providers/google/resource_compute_forwarding_rule.go

+		Description:         d.Get("description").(string),
+		LoadBalancingScheme: d.Get("load_balancing_scheme").(string),
+		Name:                d.Get("name").(string),
+		Network:             d.Get("network").(string),


Same, what if this isn't set?

Yup, you're totally right on this.

paddycarver · 2016-12-07T19:40:36Z

builtin/providers/google/resource_compute_forwarding_rule.go

+		PortRange:           d.Get("port_range").(string),
+		Ports:               ports,
+		Subnetwork:          d.Get("subnetwork").(string),
+		Target:              d.Get("target").(string),


What if this isn't set?

paddycarver · 2016-12-07T19:40:59Z

builtin/providers/google/resource_compute_forwarding_rule.go

+		Network:             d.Get("network").(string),
+		PortRange:           d.Get("port_range").(string),
+		Ports:               ports,
+		Subnetwork:          d.Get("subnetwork").(string),


What if this isn't set?

paddycarver · 2016-12-07T19:49:13Z

builtin/providers/google/resource_compute_health_check.go

+				},
+			},
+
+			"https_health_check": &schema.Schema{


Naming nit: do we really need _health_check. as a suffix to all of these, inside of the health_check resource?

That's the name in the GCP resource (https://cloud.google.com/compute/docs/reference/latest/healthChecks), but I don't mind changing it. I don't think just calling it http or https is a good naming choice though, since to me that feels more like a boolean. it could be http_details, but then it's not much shorter than the current version. What do you think?

Hm, these are some good points. My instincts are based on the Go philosophy of not repeating things, but that may be a bad instinct here. Looking at what the aws_route53_health_check resource does (as a close equivalent), it uses a single property for all its health checks, and uses "type" to distinguish them, as you previously had. Though that seems to be a reflection of the underlying API.

Thinking this through, I think you're right. It's probably better to stick closer to the API than to try to abstract the repetition out. Good call. :)

paddycarver · 2016-12-07T19:51:06Z

builtin/providers/google/resource_compute_health_check.go

+		if hchk.Type != "HTTP" {
+			return fmt.Errorf("HTTP health check declared but type is listed as %s", hchk.Type)
+		}
+		httpcheck := v.([]interface{})[0].(map[string]interface{})


It may prevent a future crash to check that

v.([]interface{}) has at least one item

that item isn't nil

before we try to cast it to a map[string]interface{}.

I don't totally understand this comment, can you give me an example configuration that could cause this issue?

paddycarver · 2016-12-07T19:53:26Z

builtin/providers/google/resource_compute_health_check.go

+		if hchk.Type != "TCP" {
+			return fmt.Errorf("TCP health check declared but type is listed as %s", hchk.Type)
+		}
+		tcpcheck := v.([]interface{})[0].(map[string]interface{})


Same thing in update as in create, re: potentially preventing crashes down the road.

paddycarver · 2016-12-07T20:02:20Z

builtin/providers/google/resource_compute_health_check_test.go

+		_, err := config.clientCompute.HealthChecks.Get(
+			config.Project, rs.Primary.ID).Do()
+		if err == nil {
+			return fmt.Errorf("HealthCheck still exists")


It'd be nice to log the ID of the health check that still exists, to make manual clean-up and debugging easier. :)

paddycarver · 2016-12-07T20:41:46Z

website/source/docs/providers/google/r/compute_region_backend_service.html.markdown

+
+# google\_compute\_region\_backend\_service
+
+A Region Backend Service defines a regionally-scoped group of virtual machines that will serve traffic for load balancing.


A link to the docs would be awesome here!

danawillow

Thanks for the review!

danawillow · 2016-12-08T18:26:27Z

builtin/providers/google/resource_compute_forwarding_rule.go

@@ -93,13 +127,24 @@ func resourceComputeForwardingRuleCreate(d *schema.ResourceData, meta interface{
 		return err
 	}

+	ps := d.Get("ports").(*schema.Set).List()


It doesn't look like it does- the tests that don't set ports still pass, like https://github.com/danawillow/terraform/blob/2009d6a1357fe16b1f98e3a652fd13631242ac5b/builtin/providers/google/resource_compute_forwarding_rule_test.go#L119.

danawillow · 2016-12-08T18:29:20Z

builtin/providers/google/resource_compute_forwarding_rule.go

-		Name:        d.Get("name").(string),
-		PortRange:   d.Get("port_range").(string),
-		Target:      d.Get("target").(string),
+		BackendService:      d.Get("backend_service").(string),


The only required attribute in the whole list is Name, so it seems there's a precedent for doing it this way.

danawillow · 2016-12-08T18:29:34Z

builtin/providers/google/resource_compute_forwarding_rule.go

+		Network:             d.Get("network").(string),
+		PortRange:           d.Get("port_range").(string),
+		Ports:               ports,
+		Subnetwork:          d.Get("subnetwork").(string),


danawillow · 2016-12-08T18:29:39Z

builtin/providers/google/resource_compute_forwarding_rule.go

+		PortRange:           d.Get("port_range").(string),
+		Ports:               ports,
+		Subnetwork:          d.Get("subnetwork").(string),
+		Target:              d.Get("target").(string),


danawillow · 2016-12-08T18:37:51Z

builtin/providers/google/resource_compute_health_check_test.go

+		_, err := config.clientCompute.HealthChecks.Get(
+			config.Project, rs.Primary.ID).Do()
+		if err == nil {
+			return fmt.Errorf("HealthCheck still exists")


danawillow · 2016-12-08T18:45:42Z

website/source/docs/providers/google/r/compute_region_backend_service.html.markdown

+
+# google\_compute\_region\_backend\_service
+
+A Region Backend Service defines a regionally-scoped group of virtual machines that will serve traffic for load balancing.


danawillow · 2016-12-08T19:07:00Z

builtin/providers/google/resource_compute_forwarding_rule.go

+		Description:         d.Get("description").(string),
+		LoadBalancingScheme: d.Get("load_balancing_scheme").(string),
+		Name:                d.Get("name").(string),
+		Network:             d.Get("network").(string),


danawillow · 2016-12-08T19:09:41Z

builtin/providers/google/resource_compute_health_check.go

+				},
+			},
+
+			"https_health_check": &schema.Schema{


That's the name in the GCP resource (https://cloud.google.com/compute/docs/reference/latest/healthChecks), but I don't mind changing it. I don't think just calling it http or https is a good naming choice though, since to me that feels more like a boolean. it could be http_details, but then it's not much shorter than the current version. What do you think?

danawillow · 2016-12-08T21:36:23Z

builtin/providers/google/resource_compute_health_check.go

+	}
+	if v, ok := d.GetOk("tcp_health_check"); ok {
+		if hchk.Type != "TCP" {
+			return fmt.Errorf("TCP health check declared but type is listed as %s", hchk.Type)


Oh neat, great idea! Done.

danawillow · 2016-12-08T21:45:49Z

builtin/providers/google/resource_compute_health_check.go

+		if hchk.Type != "HTTP" {
+			return fmt.Errorf("HTTP health check declared but type is listed as %s", hchk.Type)
+		}
+		httpcheck := v.([]interface{})[0].(map[string]interface{})


I don't totally understand this comment, can you give me an example configuration that could cause this issue?

paddycarver · 2016-12-09T00:46:29Z

website/source/docs/providers/google/r/compute_health_check.html.markdown

+
+## Example Usage
+
+```js


You can use tf here (I was surprised, too!):

resource "google_compute_health_check" "default" { name = "test" }

I won't comment on all the others, but they can all be updated :)

paddycarver · 2016-12-09T00:46:46Z

website/source/docs/providers/google/r/compute_health_check.html.markdown

+
+  timeout_sec        = 1
+  check_interval_sec = 1
+  type               = "TCP"


Now that this isn't a thing, we should remove it from the example.

paddycarver · 2016-12-09T00:49:51Z

website/source/docs/providers/google/r/compute_health_check.html.markdown

+* `timeout_sec` - (Optional) The number of seconds to wait before declaring
+    failure (default 5).
+
+* `type` - (Optional) The type of HealthCheck, either TCP, SSL, HTTP or HTTPS


We should probably remove this, as it no longer exists. :)

Yes, good call. Done.

paddycarver · 2016-12-09T02:07:07Z

Thanks for all the changes and comments. Taught me a few things. :) I added a second, shorter round of reviews, mostly about documentation and examples. Other than that, I couldn't see any problems. :)

I do notice that we can't run TestAccComputeForwardingRule_internalLoadBalancing on our project, probably because it's in beta and we need to be whitelisted. I know @stack72 is working with @evandbrown and others to change the way our GCP tests are run, to take advantage of features like this. Unfortunately, I don't think we can merge this until that happens, as it'll break our tests.

paddycarver · 2016-12-09T02:09:55Z

Ah, you updated the docs, thanks! Took me too long to investigate. :) Running the tests locally, I'm getting this from make testacc TEST=./builtin/providers/google TESTARGS='-run=TestAccComputeRegionBackendService_*'

==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2016/12/08 18:08:09 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/google -v -run=TestAccComputeRegionBackendService_* -timeout 120m
=== RUN   TestAccComputeRegionBackendService_basic
--- PASS: TestAccComputeRegionBackendService_basic (66.68s)
=== RUN   TestAccComputeRegionBackendService_withBackend
--- FAIL: TestAccComputeRegionBackendService_withBackend (0.01s)
	testing.go:265: Step 0 error: Configuration is invalid.

		Warnings: []string(nil)

		Errors: []string{"google_compute_health_check.default: : invalid or unknown key: type"}
	resource_compute_region_backend_service_test.go:66: Expected TimeoutSec == 10, got 0
	resource_compute_region_backend_service_test.go:69: Expected Protocol to be TCP, got ""
	resource_compute_region_backend_service_test.go:72: Expected 1 backend, got 0
=== RUN   TestAccComputeRegionBackendService_withBackendAndUpdate
--- FAIL: TestAccComputeRegionBackendService_withBackendAndUpdate (0.01s)
	testing.go:265: Step 0 error: Configuration is invalid.

		Warnings: []string(nil)

		Errors: []string{"google_compute_health_check.default: : invalid or unknown key: type"}
	resource_compute_region_backend_service_test.go:107: Expected TimeoutSec == 20, got 0
	resource_compute_region_backend_service_test.go:110: Expected Protocol to be TCP, got ""
	resource_compute_region_backend_service_test.go:113: Expected 1 backend, got 0
FAIL
exit status 1
FAIL	github.com/hashicorp/terraform/builtin/providers/google	66.719s
make: *** [testacc] Error 1

Looks like we need to update the tests, too. :( Sorry for the piecemeal review!

danawillow · 2016-12-09T04:02:38Z

Actually, I'm pretty sure the reason you can't run TestAccComputeForwardingRule_internalLoadBalancing is because I accidentally broke the test. Give it another try :)

(for what it's worth, ILB is in GA now. Even if it were in beta though, I don't think you would need a whitelist- that's just for alpha [citation needed though].)

paddycarver · 2016-12-09T18:57:26Z

Cool! Sorry for the confusion, I couldn't find solid info on that, and when the test failed with ILB stuff, I just assumed. Whoops. I'll re-run the tests and do a final check. :)

danawillow · 2016-12-09T19:06:37Z

No worries!

evandbrown · 2016-12-10T00:26:44Z

Thanks @danawillow and @paddyforan for the hard work on this! Excited to see it in the next release!

paddycarver · 2016-12-12T17:47:14Z

Tests all pass for me. Thanks for sticking with the lengthy review process, and for contributing the features!

ghost · 2020-04-18T02:33:48Z

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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

danawillow added 2 commits December 1, 2016 11:14

First set of changes to enable internal load balancing using beta apis

a04ed89

vendor: revendor google apis for ILB in GA

2d48c44

danawillow force-pushed the google-ilb branch from 852d003 to 093f20b Compare December 1, 2016 19:25

danawillow added 9 commits December 1, 2016 12:27

vendor: update google apis

d319d40

use v1 apis instead of beta

39809fc

clean up RegionBackendService and tests

adafa72

Add the other HealthCheck types

20ee56b

fix bugs in health check and its tests

77f0ef4

add one more test for updating a health check

fd2b1fc

remove loadBalancingScheme as an option in RegionBackendService since…

0e03f59

… it can only be INTERNAL

Add documentation for Internal Load Balancing

c28f042

Actually add documentation for Internal Load Balancing

2009d6a

danawillow force-pushed the google-ilb branch from 093f20b to 2009d6a Compare December 1, 2016 20:28

stack72 added enhancement new-resource provider/google-cloud labels Dec 7, 2016

paddycarver reviewed Dec 7, 2016

View reviewed changes

danawillow added 3 commits December 8, 2016 10:40

log id of health check that still exists on destroy

35664ff

add links to docs on region backend service

2960856

use conflictswith for health check instead of separate type field

985b4e2

danawillow commented Dec 8, 2016

View reviewed changes

paddycarver reviewed Dec 9, 2016

View reviewed changes

Update docs

73177e8

fix tests

2ad3042

paddycarver merged commit 8d046c7 into hashicorp:master Dec 12, 2016

danawillow mentioned this pull request Dec 12, 2016

providers/google: Add missing doc links #10676

Merged

ghost locked and limited conversation to collaborators Apr 18, 2020


		# google\_compute\_region\_backend\_service

		A Region Backend Service defines a regionally-scoped group of virtual machines that will serve traffic for load balancing.

providers/google: Add support for Internal Load Balancing #10453

providers/google: Add support for Internal Load Balancing #10453

Conversation

danawillow commented Dec 1, 2016

cblecker commented Dec 1, 2016

danawillow commented Dec 1, 2016

paddycarver 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

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

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

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

paddycarver commented Dec 9, 2016

paddycarver commented Dec 9, 2016

danawillow commented Dec 9, 2016 • edited Loading

paddycarver commented Dec 9, 2016

danawillow commented Dec 9, 2016

evandbrown commented Dec 10, 2016

paddycarver commented Dec 12, 2016

ghost commented Apr 18, 2020

danawillow commented Dec 9, 2016 •

edited

Loading