Support configuring ServerName for HTTP health checks

[cbroglie]

Some TLS servers require SNI, but the Golang HTTP client doesn't include it in the ClientHello when connecting to an IP address (TLS comes before HTTP, so it doesn't consider the Host header). This means the target of a HTTPS health check cannot be an IP address if the server requires SNI.

I believe making the server name configurable requires changing the API, so I created hashicorp/consul#9473 and hashicorp/consul#9475.

As an alternative, we could just address this in consul-esm without changing the API by looking at the Host header. It's less explicit, but isn't a new pattern. And the implementation could be as simple as:

diff --git a/check.go b/check.go
index 890578a..f5c0a37 100644
--- a/check.go
+++ b/check.go
@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"log"
+	"net/http"
 	"reflect"
 	"strings"
 	"sync"
@@ -99,7 +100,9 @@ func (c *CheckRunner) UpdateChecks(checks api.HealthChecks) {
 				Timeout:  definition.TimeoutDuration,
 				Logger:   c.logger,
 				TLSClientConfig: &tls.Config{
-					InsecureSkipVerify: definition.TLSSkipVerify},
+					InsecureSkipVerify: definition.TLSSkipVerify,
+					ServerName:         http.Header(definition.Header).Get("Host"),
+				},
 			}
 
 			if _, ok := c.checks[checkHash]; ok {

[lornasong]

@cbroglie, thanks for creating this issue and writing up all this context! If I understand correctly, it sounds like you’re using the IP address rather than the URL in the HTTP health check. If possible, I was wondering if you could share any context on using the IP address in the health check field? If not you’re not able to share, no worries.

Thanks for going ahead and making the related Consul issue and PR to add the TLSServerName field. Agree with your point that it’s more explicit. Let’s wait to see what to see what the maintainers think.

Regarding the alternative, it seems like to use this, users would be required to register the hostname in the header field of a health check. Is that right?

Thanks!

[cbroglie]

I was wondering if you could share any context on using the IP address in the health check field?

Our use case is integrating Consul with Kubernetes services which are exposed outside the cluster via Ingress or LoadBalancer service. It's a multi-tenant cluster where different Ingress resources share an IP address, and requests are routed based on SNI and Host header. Currently we have a wildcard DNS record pointing to the IP address (e.g. *.example.com -> 10.0.0.1), and we register external services in Consul using the DNS name (e.g. foo.example.com). Registering the IP address in Consul directly would alleviate the need for the DNS record.

Regarding the alternative, it seems like to use this, users would be required to register the hostname in the header field of a health check. Is that right?

Yes, that's correct. We'd need to do that for either proposal as our routing is based on both SNI and Host header.

[povils]

Hey! our usecase probably is the exact same as @cbroglie . I asked a while ago about this in consul discuss forum (https://discuss.hashicorp.com/t/consul-esm-http-health-check-by-another-host/11647) . However, not sure if ESM supports custom checks.

Anyway, is there something which stops us to move forward with OP suggestion? Explicit or implicit configuration are fine by our usecase

[lornasong]

Hi @povils, thanks for the question!

Regarding this original post, this is currently in progress. @ cbroglie created a PR with the required Consul-side changes first, which looks to be currently in review with a maintainer (you can see the progress here: hashicorp/consul#9475). I'll follow the issue to check on its progress.

Regarding custom checks in general, ESM still currently does not support custom checks. I'll share a little of what our team is thinking about: currently a lot of health check changes (like custom checks) unfortunately require changing both Consul and ESM. We'd like to think about potentially decoupling these two. This might impact the design of custom checks, such as script checks, which is still on our radar as an important feature for many in the community.

Hope that helps. Feel free to ask questions. Thanks!

[eikenb]

Fixed by #116