improve proxy healthz checking #321
Conversation
stlaz
added
the
sig-auth-acceptance
| // on a different port (--proxy-endpoints-port) | ||
| proxyEndpointsMux := http.NewServeMux() | ||
| proxyEndpointsMux.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte("ok")) }) | ||
| proxyEndpointsMux.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
I think the previous /healthz was completely fine. When the proxy go routine would finish, it would also cancel the health endpoint.
What you added might be worth to be used as liveness or whatever you call it. But the question is, if this makes sense or if this is just testing go std lib. Maybe we could ask @enj, if this makes sense.
There was a problem hiding this comment.
I am unsure of exactly what this trying to do, but I do think that you need something better than just "always return ok." For example, I would at a minimum expect a check that asserts that the upstream is working. I don't know if checking this specific listener is the correct approach though. From the YAML below I think this is acting as a readiness check and not a health check?
There was a problem hiding this comment.
I was trying to set up a few checks that make sure that the other listener is capable of responding to requests.
I am not sure we want to add an upstream healthz check here - the proxy info port is supposed to serve information about the proxy itself. The healthz of upstream should probably be checked directly at upstream (or should be accessed via a permissive proxied path).
There was a problem hiding this comment.
The previous health check is fine. This would be more like a readiness check?
At the beginning I had the opinion that @enj had, that we would need to "somehow check upstream", but your arguments are also right: this is the concern of upstream itself.
Now, I have no strong opinions. I see the reason behind a connection check and I am still not sure if adds enough usefulness.
|
|
||
| // we need the tls.Dialer otherwise the server would log EOF for TLS handshakes | ||
| // since the connection would be cut before that was ever attempted | ||
| dialer := tls.Dialer{NetDialer: &net.Dialer{}, Config: &tls.Config{InsecureSkipVerify: true}} |
There was a problem hiding this comment.
Can we do this without the InsecureSkipVerify?
There was a problem hiding this comment.
This is a loopback check, the serving cert's not likely to contain information for that endpoint. The check is more or less an HTTPS ping.
Maybe we could use SNI if we think we need to verify the certs, too.
| // on a different port (--proxy-endpoints-port) | ||
| proxyEndpointsMux := http.NewServeMux() | ||
| proxyEndpointsMux.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte("ok")) }) | ||
| proxyEndpointsMux.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) { |
There was a problem hiding this comment.
I am unsure of exactly what this trying to do, but I do think that you need something better than just "always return ok." For example, I would at a minimum expect a check that asserts that the upstream is working. I don't know if checking this specific listener is the correct approach though. From the YAML below I think this is acting as a readiness check and not a health check?
| } | ||
| } | ||
|
|
||
| func testHealthz(client kubernetes.Interface) kubetest.TestSuite { |
There was a problem hiding this comment.
A test for the failure case would make it more clear what this is checking.
3 participants
This adds an internal conn check between the proxy-endpoints and main-proxy listener.
Fixes #320