Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gce: HTTPS health checks (feature request and implementation details) #2190

Closed
dcarley opened this issue Jun 2, 2015 · 23 comments
Closed

gce: HTTPS health checks (feature request and implementation details) #2190

dcarley opened this issue Jun 2, 2015 · 23 comments

Comments

@dcarley
Copy link
Contributor

dcarley commented Jun 2, 2015

We have google_compute_http_health_check which configures GCE HttpHealthChecks but nothing for HttpsHealthChecks.

It seems that this could be implemented in one of two ways, each of which have a downside:

  1. Create a new google_compute_https_health_check resource which would duplicate all of the code in google_compute_http_health_check and need to be maintained in parallel.
  2. Add a protocol attribute to the existing google_compute_http_health_check resource which would involve a lot of conditionals or meta-programming to use the right type from the GCE library.

Do you have any preferences?

@dcarley
Copy link
Contributor Author

dcarley commented Jun 5, 2015

@sparkprime @dhilton do you have any opinions?

@dhilton
Copy link
Contributor

dhilton commented Jun 5, 2015

I'd 👍 an attribute - that way the code base would be more concise plus us consistent with how the AWS ELB provider makes use of an attribute: https://www.terraform.io/docs/providers/aws/r/elb.html

@sparkprime
Copy link
Contributor

I would try to follow the official API as closely as possible. What if future versions of the two resources diverge more than they do now? If we add our own extra protocol field, then what if GCE adds its own protocol field to one of the resources.

I would not worry too much about maintenance of the Terraform code. We can factor out common elements if necessary to avoid duplicating code / work. I looked into doing this for instance <-> instance_template but decided it wouldn't be worth the hassle. It may well be worth the hassle in this case.

@sparkprime
Copy link
Contributor

By the way, I am very interesting in helping to implement HTTP load balancing support for GCE in Terraform. Now that HTTPs is public, it seems like a great time to do it.

@dcarley
Copy link
Contributor Author

dcarley commented Jun 8, 2015

Thanks.

Turns out this might be a moot point right now, because the httpsHealthChecks endpoint is only available from the beta API and thus not supported by the compute/v1 library

🐼

@sparkprime
Copy link
Contributor

That's ok, it is really the resource that is beta, we can port everything over to the beta version of the API which should be backwards compatible. We would mark the beta resource as beta in the documentation, as I did in my unmerged PR to merge the autoscaler / instance group manager stuff.

@dhilton
Copy link
Contributor

dhilton commented Jun 9, 2015

Cool - so how do we make this happen?

@sparkprime
Copy link
Contributor

Ok let's just switch everything to compute/beta, I'll do that now and run the full set of tests. Then it should be just a case of refactoring the HTTP resource and using this to make an HTTPs resource.

@sparkprime
Copy link
Contributor

Turns out this was not a good idea. Changing the API to compute/beta means that explicit URLs given in the .tf with /v1/ in them will no-longer be valid, which is unnecessarily disruptive. I'll instead add a new API endpoint called betaCompute specifically for making HTTPs resources.

@sparkprime
Copy link
Contributor

Ok that is there in #2297

keymon added a commit to alphagov/paas-alpha-tsuru-terraform that referenced this issue Jun 11, 2015
Currently HTTPS health checks in GCE are part of the GCE beta API and are not
supported by terraform.

There's a [feature request](hashicorp/terraform#2190)
where these possible implementations are discussed:

 1. Alternate resource type: google_compute_https_health_check
 2. Add a new attribute `protocol`

In this commit remove the HTTPS check and use only HTTP on port 8080, for
both the `HTTP/8080` and `HTTPS/443` forwarding rules. The drawbacks are:

 * We don't check the nginx SSL terminator service in the health check.
 * The insecure API must be open, at least for the LB layer.
keymon added a commit to alphagov/paas-alpha-tsuru-terraform that referenced this issue Jun 11, 2015
Currently HTTPS health checks in GCE are part of the GCE beta API and are not
supported by terraform.

There's a [feature request](hashicorp/terraform#2190)
where these possible implementations are discussed:

 1. Alternate resource type: google_compute_https_health_check
 2. Add a new attribute `protocol`

In this commit remove the HTTPS check and use only HTTP on port 8080, for
both the `HTTP/8080` and `HTTPS/443` forwarding rules. The drawbacks are:

 * We don't check the nginx SSL terminator service in the health check.
 * The insecure API must be open, at least for the LB layer.
@dcarley
Copy link
Contributor Author

dcarley commented Jul 14, 2015

@sparkprime I had a crack at this: master...dcarley:f-https_health_check

However I ran into some problems, described in these two commits:

Any thoughts?

@dcarley
Copy link
Contributor Author

dcarley commented Jul 21, 2015

Welp, compute/v0.beta was removed and it hasn't been added to compute/v1, so I guess this is blocked.

@sparkprime
Copy link
Contributor

I'll try to find out internally what's going on

On Tue, Jul 21, 2015, 4:47 PM Dan Carley notifications@github.com wrote:

Welp, compute/v0.beta was removed and it hasn't been added to compute/v1,
so I guess this is blocked.


Reply to this email directly or view it on GitHub
#2190 (comment)
.

@dcarley
Copy link
Contributor Author

dcarley commented Jul 23, 2015

Thanks!

@sparkprime
Copy link
Contributor

The v0.beta API is still there, but not discoverable. That means a new build of the go client libraries didn't pick it up. That is a bug and people are working on it, so it should re-appear soon.

@sparkprime
Copy link
Contributor

For large values of "soon", apparently v0.beta is back now. Sorry for the chaos :)

I recommend reverse-applying

https://github.com/svanharmelen/terraform/commit/e1d5af8ccf83935bcab3380b3b9822e7e0338d68

to your branch in order to get unblocked again

@sparkprime
Copy link
Contributor

@lwander is this necessary for usage of https loadbalancer?

@lwander
Copy link
Contributor

lwander commented Nov 10, 2015

Unfortunately I didn't know we had this work open, so it's been reimplemented and merged into master already.

@sparkprime
Copy link
Contributor

I can't find it -- this is the health check resource, not the proxy

@lwander
Copy link
Contributor

lwander commented Nov 10, 2015

Ah, right - it's not required since the URL Map resource corresponding to an HTTPS proxy can have a Backend Service checked by HTTP health checks, but it would be good to have. I'll add it in.

@lwander
Copy link
Contributor

lwander commented Nov 13, 2015

@sparkprime this has been merged - #3883

@sparkprime
Copy link
Contributor

Thanks for the initial work, @dcarley

@ghost
Copy link

ghost commented Apr 30, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants