reverseproxy: Refactor dial address parsing, augment command parsing (#4616)

4 files changed, 134 insertions, 119 deletions

diff --git a/caddyconfig/caddyfile/dispenser.go b/caddyconfig/caddyfile/dispenser.go
index fa7f5e755..23f6eade7 100755
--- a/caddyconfig/caddyfile/dispenser.go
+++ b/caddyconfig/caddyfile/dispenser.go
@@ -350,7 +350,11 @@ func (d *Dispenser) Err(msg string) error {
 
 // Errf is like Err, but for formatted error messages
 func (d *Dispenser) Errf(format string, args ...interface{}) error {
-	err := fmt.Errorf(format, args...)
+	return d.WrapErr(fmt.Errorf(format, args...))
+}
+
+// WrapErr takes an existing error and adds the Caddyfile file and line number.
+func (d *Dispenser) WrapErr(err error) error {
 	return fmt.Errorf("%s:%d - Error during parsing: %w", d.File(), d.Line(), err)
 }
 
diff --git a/modules/caddyhttp/reverseproxy/addresses.go b/modules/caddyhttp/reverseproxy/addresses.go
new file mode 100644
index 000000000..f15ed76da
--- /dev/null
+++ b/modules/caddyhttp/reverseproxy/addresses.go
@@ -0,0 +1,112 @@
+// Copyright 2015 Matthew Holt and The Caddy Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package reverseproxy
+
+import (
+	"fmt"
+	"net"
+	"net/url"
+	"strings"
+
+	"github.com/caddyserver/caddy/v2"
+)
+
+// parseUpstreamDialAddress parses configuration inputs for
+// the dial address, including support for a scheme in front
+// as a shortcut for the port number, and a network type,
+// for example 'unix' to dial a unix socket.
+//
+// TODO: the logic in this function is kind of sensitive, we
+// need to write tests before making any more changes to it
+func parseUpstreamDialAddress(upstreamAddr string) (string, string, error) {
+	var network, scheme, host, port string
+
+	if strings.Contains(upstreamAddr, "://") {
+		// we get a parsing error if a placeholder is specified
+		// so we return a more user-friendly error message instead
+		// to explain what to do instead
+		if strings.Contains(upstreamAddr, "{") {
+			return "", "", fmt.Errorf("due to parsing difficulties, placeholders are not allowed when an upstream address contains a scheme")
+		}
+
+		toURL, err := url.Parse(upstreamAddr)
+		if err != nil {
+			return "", "", fmt.Errorf("parsing upstream URL: %v", err)
+		}
+
+		// there is currently no way to perform a URL rewrite between choosing
+		// a backend and proxying to it, so we cannot allow extra components
+		// in backend URLs
+		if toURL.Path != "" || toURL.RawQuery != "" || toURL.Fragment != "" {
+			return "", "", fmt.Errorf("for now, URLs for proxy upstreams only support scheme, host, and port components")
+		}
+
+		// ensure the port and scheme aren't in conflict
+		urlPort := toURL.Port()
+		if toURL.Scheme == "http" && urlPort == "443" {
+			return "", "", fmt.Errorf("upstream address has conflicting scheme (http://) and port (:443, the HTTPS port)")
+		}
+		if toURL.Scheme == "https" && urlPort == "80" {
+			return "", "", fmt.Errorf("upstream address has conflicting scheme (https://) and port (:80, the HTTP port)")
+		}
+		if toURL.Scheme == "h2c" && urlPort == "443" {
+			return "", "", fmt.Errorf("upstream address has conflicting scheme (h2c://) and port (:443, the HTTPS port)")
+		}
+
+		// if port is missing, attempt to infer from scheme
+		if toURL.Port() == "" {
+			var toPort string
+			switch toURL.Scheme {
+			case "", "http", "h2c":
+				toPort = "80"
+			case "https":
+				toPort = "443"
+			}
+			toURL.Host = net.JoinHostPort(toURL.Hostname(), toPort)
+		}
+
+		scheme, host, port = toURL.Scheme, toURL.Hostname(), toURL.Port()
+	} else {
+		// extract network manually, since caddy.ParseNetworkAddress() will always add one
+		if idx := strings.Index(upstreamAddr, "/"); idx >= 0 {
+			network = strings.ToLower(strings.TrimSpace(upstreamAddr[:idx]))
+			upstreamAddr = upstreamAddr[idx+1:]
+		}
+		var err error
+		host, port, err = net.SplitHostPort(upstreamAddr)
+		if err != nil {
+			host = upstreamAddr
+		}
+		// we can assume a port if only a hostname is specified, but use of a
+		// placeholder without a port likely means a port will be filled in
+		if port == "" && !strings.Contains(host, "{") {
+			port = "80"
+		}
+	}
+
+	// for simplest possible config, we only need to include
+	// the network portion if the user specified one
+	if network != "" {
+		return caddy.JoinNetworkAddress(network, host, port), scheme, nil
+	}
+
+	// if the host is a placeholder, then we don't want to join with an empty port,
+	// because that would just append an extra ':' at the end of the address.
+	if port == "" && strings.Contains(host, "{") {
+		return host, scheme, nil
+	}
+
+	return net.JoinHostPort(host, port), scheme, nil
+}
diff --git a/modules/caddyhttp/reverseproxy/caddyfile.go b/modules/caddyhttp/reverseproxy/caddyfile.go
index d642d04e7..a1fbeb32c 100644
--- a/modules/caddyhttp/reverseproxy/caddyfile.go
+++ b/modules/caddyhttp/reverseproxy/caddyfile.go
@@ -18,7 +18,6 @@ import (
 	"log"
 	"net"
 	"net/http"
-	"net/url"
 	"reflect"
 	"strconv"
 	"strings"
@@ -123,98 +122,6 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 	// prefixed with "@" for use with "handle_response" blocks
 	h.responseMatchers = make(map[string]caddyhttp.ResponseMatcher)
 
-	// TODO: the logic in this function is kind of sensitive, we need
-	// to write tests before making any more changes to it
-	upstreamDialAddress := func(upstreamAddr string) (string, error) {
-		var network, scheme, host, port string
-
-		if strings.Contains(upstreamAddr, "://") {
-			// we get a parsing error if a placeholder is specified
-			// so we return a more user-friendly error message instead
-			// to explain what to do instead
-			if strings.Contains(upstreamAddr, "{") {
-				return "", d.Err("due to parsing difficulties, placeholders are not allowed when an upstream address contains a scheme")
-			}
-
-			toURL, err := url.Parse(upstreamAddr)
-			if err != nil {
-				return "", d.Errf("parsing upstream URL: %v", err)
-			}
-
-			// there is currently no way to perform a URL rewrite between choosing
-			// a backend and proxying to it, so we cannot allow extra components
-			// in backend URLs
-			if toURL.Path != "" || toURL.RawQuery != "" || toURL.Fragment != "" {
-				return "", d.Err("for now, URLs for proxy upstreams only support scheme, host, and port components")
-			}
-
-			// ensure the port and scheme aren't in conflict
-			urlPort := toURL.Port()
-			if toURL.Scheme == "http" && urlPort == "443" {
-				return "", d.Err("upstream address has conflicting scheme (http://) and port (:443, the HTTPS port)")
-			}
-			if toURL.Scheme == "https" && urlPort == "80" {
-				return "", d.Err("upstream address has conflicting scheme (https://) and port (:80, the HTTP port)")
-			}
-			if toURL.Scheme == "h2c" && urlPort == "443" {
-				return "", d.Err("upstream address has conflicting scheme (h2c://) and port (:443, the HTTPS port)")
-			}
-
-			// if port is missing, attempt to infer from scheme
-			if toURL.Port() == "" {
-				var toPort string
-				switch toURL.Scheme {
-				case "", "http", "h2c":
-					toPort = "80"
-				case "https":
-					toPort = "443"
-				}
-				toURL.Host = net.JoinHostPort(toURL.Hostname(), toPort)
-			}
-
-			scheme, host, port = toURL.Scheme, toURL.Hostname(), toURL.Port()
-		} else {
-			// extract network manually, since caddy.ParseNetworkAddress() will always add one
-			if idx := strings.Index(upstreamAddr, "/"); idx >= 0 {
-				network = strings.ToLower(strings.TrimSpace(upstreamAddr[:idx]))
-				upstreamAddr = upstreamAddr[idx+1:]
-			}
-			var err error
-			host, port, err = net.SplitHostPort(upstreamAddr)
-			if err != nil {
-				host = upstreamAddr
-			}
-			// we can assume a port if only a hostname is specified, but use of a
-			// placeholder without a port likely means a port will be filled in
-			if port == "" && !strings.Contains(host, "{") {
-				port = "80"
-			}
-		}
-
-		// the underlying JSON does not yet support different
-		// transports (protocols or schemes) to each backend,
-		// so we remember the last one we see and compare them
-		if commonScheme != "" && scheme != commonScheme {
-			return "", d.Errf("for now, all proxy upstreams must use the same scheme (transport protocol); expecting '%s://' but got '%s://'",
-				commonScheme, scheme)
-		}
-		commonScheme = scheme
-
-		// for simplest possible config, we only need to include
-		// the network portion if the user specified one
-		if network != "" {
-			return caddy.JoinNetworkAddress(network, host, port), nil
-		}
-
-		// if the host is a placeholder, then we don't want to join with an empty port,
-		// because that would just append an extra ':' at the end of the address.
-		if port == "" && strings.Contains(host, "{") {
-			return host, nil
-		}
-
-		return net.JoinHostPort(host, port), nil
-	}
-
 	// appendUpstream creates an upstream for address and adds
 	// it to the list. If the address starts with "srv+" it is
 	// treated as a SRV-based upstream, and any port will be
@@ -224,10 +131,21 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 		if isSRV {
 			address = strings.TrimPrefix(address, "srv+")
 		}
-		dialAddr, err := upstreamDialAddress(address)
+
+		dialAddr, scheme, err := parseUpstreamDialAddress(address)
 		if err != nil {
-			return err
+			return d.WrapErr(err)
 		}
+
+		// the underlying JSON does not yet support different
+		// transports (protocols or schemes) to each backend,
+		// so we remember the last one we see and compare them
+		if commonScheme != "" && scheme != commonScheme {
+			return d.Errf("for now, all proxy upstreams must use the same scheme (transport protocol); expecting '%s://' but got '%s://'",
+				commonScheme, scheme)
+		}
+		commonScheme = scheme
+
 		if isSRV {
 			if host, _, err := net.SplitHostPort(dialAddr); err == nil {
 				dialAddr = host
diff --git a/modules/caddyhttp/reverseproxy/command.go b/modules/caddyhttp/reverseproxy/command.go
index 9bbc681f1..121142c10 100644
--- a/modules/caddyhttp/reverseproxy/command.go
+++ b/modules/caddyhttp/reverseproxy/command.go
@@ -18,7 +18,6 @@ import (
 	"encoding/json"
 	"flag"
 	"fmt"
-	"net"
 	"net/http"
 	"strconv"
 
@@ -104,32 +103,14 @@ func cmdReverseProxy(fs caddycmd.Flags) (int, error) {
 	}
 
 	// set up the upstream address; assume missing information from given parts
-	toAddr, err := httpcaddyfile.ParseAddress(to)
+	toAddr, toScheme, err := parseUpstreamDialAddress(to)
 	if err != nil {
 		return caddy.ExitCodeFailedStartup, fmt.Errorf("invalid upstream address %s: %v", to, err)
 	}
-	if toAddr.Path != "" {
-		return caddy.ExitCodeFailedStartup, fmt.Errorf("paths are not allowed: %s", to)
-	}
-	if toAddr.Scheme == "" {
-		if toAddr.Port == httpsPort {
-			toAddr.Scheme = "https"
-		} else {
-			toAddr.Scheme = "http"
-		}
-	}
-	if toAddr.Port == "" {
-		if toAddr.Scheme == "http" {
-			toAddr.Port = httpPort
-		} else if toAddr.Scheme == "https" {
-			toAddr.Port = httpsPort
-		}
-	}
 
 	// proceed to build the handler and server
-
 	ht := HTTPTransport{}
-	if toAddr.Scheme == "https" {
+	if toScheme == "https" {
 		ht.TLS = new(TLSConfig)
 		if insecure {
 			ht.TLS.InsecureSkipVerify = true
@@ -138,7 +119,7 @@ func cmdReverseProxy(fs caddycmd.Flags) (int, error) {
 
 	handler := Handler{
 		TransportRaw: caddyconfig.JSONModuleObject(ht, "protocol", "http", nil),
-		Upstreams:    UpstreamPool{{Dial: net.JoinHostPort(toAddr.Host, toAddr.Port)}},
+		Upstreams:    UpstreamPool{{Dial: toAddr}},
 	}
 
 	if changeHost {
@@ -185,7 +166,7 @@ func cmdReverseProxy(fs caddycmd.Flags) (int, error) {
 		return caddy.ExitCodeFailedStartup, err
 	}
 
-	fmt.Printf("Caddy proxying %s -> %s\n", fromAddr.String(), toAddr.String())
+	fmt.Printf("Caddy proxying %s -> %s\n", fromAddr.String(), toAddr)
 
 	select {}
 }