reverseproxy: Fix dial placeholders, SRV, active health checks (#3780)

* reverseproxy: Fix dial placeholders, SRV, active health checks

Supercedes #3776
Partially reverts or updates #3756, #3693, and #3695

* reverseproxy: add integration tests

Co-authored-by: Mohammed Al Sahaf <msaa1990@gmail.com>

4 files changed, 32 insertions, 33 deletions

diff --git a/modules/caddyhttp/reverseproxy/healthchecks.go b/modules/caddyhttp/reverseproxy/healthchecks.go
index 410b9d4..4e93320 100644
--- a/modules/caddyhttp/reverseproxy/healthchecks.go
+++ b/modules/caddyhttp/reverseproxy/healthchecks.go
@@ -154,23 +154,42 @@ func (h *Handler) doActiveHealthCheckForAllHosts() {
 				}
 			}()
 
-			portStr := strconv.Itoa(upstream.activeHealthCheckPort)
-			hostAddr := net.JoinHostPort(upstream.networkAddress.Host, portStr)
-			if upstream.networkAddress.IsUnixNetwork() {
+			networkAddr, err := caddy.NewReplacer().ReplaceOrErr(upstream.Dial, true, true)
+			if err != nil {
+				h.HealthChecks.Active.logger.Error("invalid use of placeholders in dial address for active health checks",
+					zap.String("address", networkAddr),
+					zap.Error(err),
+				)
+				return
+			}
+			addr, err := caddy.ParseNetworkAddress(networkAddr)
+			if err != nil {
+				h.HealthChecks.Active.logger.Error("bad network address",
+					zap.String("address", networkAddr),
+					zap.Error(err),
+				)
+				return
+			}
+			if hcp := uint(upstream.activeHealthCheckPort); hcp != 0 {
+				if addr.IsUnixNetwork() {
+					addr.Network = "tcp" // I guess we just assume TCP since we are using a port??
+				}
+				addr.StartPort, addr.EndPort = hcp, hcp
+			}
+			if upstream.LookupSRV == "" && addr.PortRangeSize() != 1 {
+				h.HealthChecks.Active.logger.Error("multiple addresses (upstream must map to only one address)",
+					zap.String("address", networkAddr),
+				)
+				return
+			}
+			hostAddr := addr.JoinHostPort(0)
+			if addr.IsUnixNetwork() {
 				// this will be used as the Host portion of a http.Request URL, and
 				// paths to socket files would produce an error when creating URL,
 				// so use a fake Host value instead; unix sockets are usually local
 				hostAddr = "localhost"
 			}
-
-			dialInfo := DialInfo{
-				Upstream: upstream,
-				Network:  upstream.networkAddress.Network,
-				Host:     upstream.networkAddress.Host,
-				Port:     portStr,
-				Address:  hostAddr,
-			}
-			err := h.doActiveHealthCheck(dialInfo, hostAddr, upstream.Host)
+			err = h.doActiveHealthCheck(DialInfo{Network: addr.Network, Address: hostAddr}, hostAddr, upstream.Host)
 			if err != nil {
 				h.HealthChecks.Active.logger.Error("active health check failed",
 					zap.String("address", hostAddr),
diff --git a/modules/caddyhttp/reverseproxy/hosts.go b/modules/caddyhttp/reverseproxy/hosts.go
index b7b8c9b..b9817d2 100644
--- a/modules/caddyhttp/reverseproxy/hosts.go
+++ b/modules/caddyhttp/reverseproxy/hosts.go
@@ -92,7 +92,6 @@ type Upstream struct {
 	// HeaderAffinity string
 	// IPAffinity     string
 
-	networkAddress        caddy.NetworkAddress
 	activeHealthCheckPort int
 	healthCheckPolicy     *PassiveHealthChecks
 	cb                    CircuitBreaker
diff --git a/modules/caddyhttp/reverseproxy/httptransport.go b/modules/caddyhttp/reverseproxy/httptransport.go
index 7e3bb69..dce7b9e 100644
--- a/modules/caddyhttp/reverseproxy/httptransport.go
+++ b/modules/caddyhttp/reverseproxy/httptransport.go
@@ -182,9 +182,6 @@ func (h *HTTPTransport) NewTransport(ctx caddy.Context) (*http.Transport, error)
 			if dialInfo, ok := GetDialInfo(ctx); ok {
 				network = dialInfo.Network
 				address = dialInfo.Address
-				if dialInfo.Upstream.networkAddress.IsUnixNetwork() {
-					address = dialInfo.Host
-				}
 			}
 			conn, err := dialer.DialContext(ctx, network, address)
 			if err != nil {
diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go
index eac02b6..305a0b3 100644
--- a/modules/caddyhttp/reverseproxy/reverseproxy.go
+++ b/modules/caddyhttp/reverseproxy/reverseproxy.go
@@ -129,10 +129,8 @@ func (h *Handler) Provision(ctx caddy.Context) error {
 	h.ctx = ctx
 	h.logger = ctx.Logger(h)
 
-	// get validation out of the way
+	// verify SRV compatibility
 	for i, v := range h.Upstreams {
-		// Having LookupSRV non-empty conflicts with 2 other config parameters: active health checks, and upstream dial address.
-		// Therefore if LookupSRV is empty, then there are no immediately apparent config conflicts, and the iteration can be skipped.
 		if v.LookupSRV == "" {
 			continue
 		}
@@ -219,18 +217,6 @@ func (h *Handler) Provision(ctx caddy.Context) error {
 
 	// set up upstreams
 	for _, upstream := range h.Upstreams {
-		if upstream.LookupSRV == "" {
-			addr, err := caddy.ParseNetworkAddress(upstream.Dial)
-			if err != nil {
-				return err
-			}
-
-			if addr.PortRangeSize() != 1 {
-				return fmt.Errorf("multiple addresses (upstream must map to only one address): %v", addr)
-			}
-
-			upstream.networkAddress = addr
-		}
 		// create or get the host representation for this upstream
 		var host Host = new(upstreamHost)
 		existingHost, loaded := hosts.LoadOrStore(upstream.String(), host)
@@ -292,8 +278,6 @@ func (h *Handler) Provision(ctx caddy.Context) error {
 				// then use it, otherwise use the port of upstream.
 				if h.HealthChecks.Active.Port != 0 {
 					upstream.activeHealthCheckPort = h.HealthChecks.Active.Port
-				} else {
-					upstream.activeHealthCheckPort = int(upstream.networkAddress.StartPort)
 				}
 			}