reverseproxy: allow no port for SRV; fix regression in d55d50b (#3756)

* reverseproxy: fix breakage in handling SRV lookup introduced by 3695

* reverseproxy: validate against incompatible config options with lookup_srv

* reverseproxy: add integration test cases for validations involving lookup_srv

* reverseproxy: clarify the reason for skipping an iteration

* grammar.. Oxford comma

Co-authored-by: Francis Lavoie <lavofr@gmail.com>

Co-authored-by: Francis Lavoie <lavofr@gmail.com>

Fixes #3753

2 files changed, 128 insertions, 9 deletions

diff --git a/caddytest/integration/reverseproxy_test.go b/caddytest/integration/reverseproxy_test.go
index 8000546..0505bec 100644
--- a/caddytest/integration/reverseproxy_test.go
+++ b/caddytest/integration/reverseproxy_test.go
@@ -13,6 +13,109 @@ import (
 	"github.com/caddyserver/caddy/v2/caddytest"
 )
 
+func TestSRVReverseProxy(t *testing.T) {
+	tester := caddytest.NewTester(t)
+	tester.InitServer(`
+	{
+		"apps": {
+		  "http": {
+			"servers": {
+			  "srv0": {
+				"listen": [
+				  ":8080"
+				],
+				"routes": [
+				  {
+					"handle": [
+					  {
+						"handler": "reverse_proxy",
+						"upstreams": [
+						  {
+							"lookup_srv": "srv.host.service.consul"
+						  }
+						]
+					  }
+					]
+				  }
+				]
+			  }
+			}
+		  }
+		}
+	  }
+  `, "json")
+}
+
+func TestSRVWithDial(t *testing.T) {
+	caddytest.AssertLoadError(t, `
+	{
+		"apps": {
+		  "http": {
+			"servers": {
+			  "srv0": {
+				"listen": [
+				  ":8080"
+				],
+				"routes": [
+				  {
+					"handle": [
+					  {
+						"handler": "reverse_proxy",
+						"upstreams": [
+						  {
+							"dial": "tcp/address.to.upstream:80",
+							"lookup_srv": "srv.host.service.consul"
+						  }
+						]
+					  }
+					]
+				  }
+				]
+			  }
+			}
+		  }
+		}
+	  }
+	`, "json", `upstream: specifying dial address is incompatible with lookup_srv: 0: {\"dial\": \"tcp/address.to.upstream:80\", \"lookup_srv\": \"srv.host.service.consul\"}`)
+}
+
+func TestSRVWithActiveHealthcheck(t *testing.T) {
+	caddytest.AssertLoadError(t, `
+	{
+		"apps": {
+		  "http": {
+			"servers": {
+			  "srv0": {
+				"listen": [
+				  ":8080"
+				],
+				"routes": [
+				  {
+					"handle": [
+					  {
+						"handler": "reverse_proxy",
+						"health_checks": {
+							"active": {
+								"path": "/ok"
+							}
+						},
+						"upstreams": [
+						  {
+							"lookup_srv": "srv.host.service.consul"
+						  }
+						]
+					  }
+					]
+				  }
+				]
+			  }
+			}
+		  }
+		}
+	  }
+	`, "json", `upstream: lookup_srv is incompatible with active health checks: 0: {\"dial\": \"\", \"lookup_srv\": \"srv.host.service.consul\"}`)
+}
+
 func TestReverseProxyHealthCheck(t *testing.T) {
 	tester := caddytest.NewTester(t)
 	tester.InitServer(`
diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go
index 0f93df0..eac02b6 100644
--- a/modules/caddyhttp/reverseproxy/reverseproxy.go
+++ b/modules/caddyhttp/reverseproxy/reverseproxy.go
@@ -129,6 +129,21 @@ func (h *Handler) Provision(ctx caddy.Context) error {
 	h.ctx = ctx
 	h.logger = ctx.Logger(h)
 
+	// get validation out of the way
+	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
+		}
+		if h.HealthChecks != nil && h.HealthChecks.Active != nil {
+			return fmt.Errorf(`upstream: lookup_srv is incompatible with active health checks: %d: {"dial": %q, "lookup_srv": %q}`, i, v.Dial, v.LookupSRV)
+		}
+		if v.Dial != "" {
+			return fmt.Errorf(`upstream: specifying dial address is incompatible with lookup_srv: %d: {"dial": %q, "lookup_srv": %q}`, i, v.Dial, v.LookupSRV)
+		}
+	}
+
 	// start by loading modules
 	if h.TransportRaw != nil {
 		mod, err := ctx.LoadModule(h, "TransportRaw")
@@ -204,17 +219,18 @@ func (h *Handler) Provision(ctx caddy.Context) error {
 
 	// set up upstreams
 	for _, upstream := range h.Upstreams {
-		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)
-		}
+		if upstream.LookupSRV == "" {
+			addr, err := caddy.ParseNetworkAddress(upstream.Dial)
+			if err != nil {
+				return err
+			}
 
-		upstream.networkAddress = addr
+			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)