From 8027ca3847e75c106b5189c1c0ab48bcf6fff50b Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Sat, 3 Oct 2020 18:38:35 +0300 Subject: [PATCH 1/2] reverseproxy: restore allowing upstream to contain replaceable placeholders --- caddytest/integration/reverseproxy_test.go | 275 ++++++++++++++++++ modules/caddyhttp/reverseproxy/caddyfile.go | 11 +- .../caddyhttp/reverseproxy/reverseproxy.go | 29 +- 3 files changed, 302 insertions(+), 13 deletions(-) diff --git a/caddytest/integration/reverseproxy_test.go b/caddytest/integration/reverseproxy_test.go index 0505becf72d..941a8543ce6 100644 --- a/caddytest/integration/reverseproxy_test.go +++ b/caddytest/integration/reverseproxy_test.go @@ -79,6 +79,281 @@ func TestSRVWithDial(t *testing.T) { `, "json", `upstream: specifying dial address is incompatible with lookup_srv: 0: {\"dial\": \"tcp/address.to.upstream:80\", \"lookup_srv\": \"srv.host.service.consul\"}`) } +func TestDialWithPlaceholderActiveHealthcheck(t *testing.T) { + caddytest.AssertLoadError(t, ` + { + "apps": { + "http": { + "servers": { + "srv0": { + "listen": [ + ":8080" + ], + "routes": [ + { + "handle": [ + { + "handler": "reverse_proxy", + "health_checks": { + "active": { + "path": "/ok" + } + }, + "upstreams": [ + { + "dial": "{http.request.header.X-Caddy-Upstream-Dial}" + } + ] + } + ] + } + ] + } + } + } + } + } + `, "json", `upstream: dial address with placeholders is incompatible with active health checks: 0: {\"dial\": \"{http.request.header.X-Caddy-Upstream-Dial}\"}`) +} + +func TestDialWithPlaceholderUnix(t *testing.T) { + + if runtime.GOOS == "windows" { + t.SkipNow() + } + + f, err := ioutil.TempFile("", "*.sock") + if err != nil { + t.Errorf("failed to create TempFile: %s", err) + return + } + // a hack to get a file name within a valid path to use as socket + socketName := f.Name() + os.Remove(f.Name()) + + server := http.Server{ + Handler: http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + w.Write([]byte("Hello, World!")) + }), + } + + unixListener, err := net.Listen("unix", socketName) + if err != nil { + t.Errorf("failed to listen on the socket: %s", err) + return + } + go server.Serve(unixListener) + t.Cleanup(func() { + server.Close() + }) + runtime.Gosched() // Allow other goroutines to run + + tester := caddytest.NewTester(t) + tester.InitServer(` + { + "apps": { + "http": { + "servers": { + "srv0": { + "listen": [ + ":8080" + ], + "routes": [ + { + "handle": [ + { + "handler": "reverse_proxy", + "upstreams": [ + { + "dial": "unix/{http.request.header.X-Caddy-Upstream-Dial}" + } + ] + } + ] + } + ] + } + } + } + } + } + `, "json") + + req, err := http.NewRequest(http.MethodGet, "http://localhost:8080", nil) + if err != nil { + t.Fail() + return + } + req.Header.Set("X-Caddy-Upstream-Dial", socketName) + tester.AssertResponse(req, 200, "Hello, World!") +} + +func TestReverseProxyWithPlaceholderDialAddress(t *testing.T) { + tester := caddytest.NewTester(t) + tester.InitServer(` + { + "apps": { + "http": { + "servers": { + "srv0": { + "listen": [ + ":8080" + ], + "routes": [ + { + "match": [ + { + "host": [ + "localhost" + ] + } + ], + "handle": [ + { + "handler": "static_response", + "body": "Hello, World!" + } + ], + "terminal": true + } + ], + "automatic_https": { + "skip": [ + "localhost" + ] + } + }, + "srv1": { + "listen": [ + ":9080" + ], + "routes": [ + { + "match": [ + { + "host": [ + "localhost" + ] + } + ], + "handle": [ + { + + "handler": "reverse_proxy", + "upstreams": [ + { + "dial": "{http.request.header.X-Caddy-Upstream-Dial}" + } + ] + } + ], + "terminal": true + } + ], + "automatic_https": { + "skip": [ + "localhost" + ] + } + } + } + } + } + } + `, "json") + + req, err := http.NewRequest(http.MethodGet, "http://localhost:9080", nil) + if err != nil { + t.Fail() + return + } + req.Header.Set("X-Caddy-Upstream-Dial", "localhost:8080") + tester.AssertResponse(req, 200, "Hello, World!") +} + +func TestReverseProxyWithPlaceholderTCPDialAddress(t *testing.T) { + tester := caddytest.NewTester(t) + tester.InitServer(` + { + "apps": { + "http": { + "servers": { + "srv0": { + "listen": [ + ":8080" + ], + "routes": [ + { + "match": [ + { + "host": [ + "localhost" + ] + } + ], + "handle": [ + { + "handler": "static_response", + "body": "Hello, World!" + } + ], + "terminal": true + } + ], + "automatic_https": { + "skip": [ + "localhost" + ] + } + }, + "srv1": { + "listen": [ + ":9080" + ], + "routes": [ + { + "match": [ + { + "host": [ + "localhost" + ] + } + ], + "handle": [ + { + + "handler": "reverse_proxy", + "upstreams": [ + { + "dial": "tcp/{http.request.header.X-Caddy-Upstream-Dial}:8080" + } + ] + } + ], + "terminal": true + } + ], + "automatic_https": { + "skip": [ + "localhost" + ] + } + } + } + } + } + } + `, "json") + + req, err := http.NewRequest(http.MethodGet, "http://localhost:9080", nil) + if err != nil { + t.Fail() + return + } + req.Header.Set("X-Caddy-Upstream-Dial", "localhost") + tester.AssertResponse(req, 200, "Hello, World!") +} + func TestSRVWithActiveHealthcheck(t *testing.T) { caddytest.AssertLoadError(t, ` { diff --git a/modules/caddyhttp/reverseproxy/caddyfile.go b/modules/caddyhttp/reverseproxy/caddyfile.go index fcaf82b9dc6..4c816d06f0a 100644 --- a/modules/caddyhttp/reverseproxy/caddyfile.go +++ b/modules/caddyhttp/reverseproxy/caddyfile.go @@ -19,6 +19,7 @@ import ( "net/http" "net/url" "reflect" + "regexp" "strconv" "strings" @@ -101,6 +102,7 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { // to write tests before making any more changes to it upstreamDialAddress := func(upstreamAddr string) (string, error) { var network, scheme, host, port string + placeholder := regexp.MustCompile(`\{[[:graph:]]+\}`) if strings.Contains(upstreamAddr, "://") { // we get a parsing error if a placeholder is specified @@ -155,7 +157,8 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { if err != nil { host = upstreamAddr } - if port == "" { + + if !placeholder.MatchString(host) && port == "" { port = "80" } } @@ -174,6 +177,12 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { if network != "" { return caddy.JoinNetworkAddress(network, host, port), nil } + // if both network and port are empty, host address contains + // placeholdler (e.g. "{http.request.header.X-Caddy-Upstream-Dial}") and doing net.JoinHostPort(host, port) will + // add an extreneous colon. + if port == "" { + return host, nil + } return net.JoinHostPort(host, port), nil } diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go index eac02b66ce4..439e19150dd 100644 --- a/modules/caddyhttp/reverseproxy/reverseproxy.go +++ b/modules/caddyhttp/reverseproxy/reverseproxy.go @@ -129,8 +129,12 @@ func (h *Handler) Provision(ctx caddy.Context) error { h.ctx = ctx h.logger = ctx.Logger(h) + re := regexp.MustCompile(`\{[[:graph:]]+\}`) // get validation out of the way for i, v := range h.Upstreams { + if v.Dial != "" && re.MatchString(v.Dial) && h.HealthChecks != nil && h.HealthChecks.Active != nil { + return fmt.Errorf(`upstream: dial address with placeholders is incompatible with active health checks: %d: {"dial": %q}`, i, v.Dial) + } // 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 == "" { @@ -219,18 +223,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) @@ -288,6 +280,19 @@ func (h *Handler) Provision(ctx caddy.Context) error { } 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 + } + // if there's an alternative port for health-check provided in the config, // then use it, otherwise use the port of upstream. if h.HealthChecks.Active.Port != 0 { From 47a85477c92d81a982b26d8c08bc5098332031d9 Mon Sep 17 00:00:00 2001 From: Mohammed Al Sahaf Date: Sat, 3 Oct 2020 20:16:23 +0300 Subject: [PATCH 2/2] spelling Co-authored-by: Francis Lavoie --- modules/caddyhttp/reverseproxy/caddyfile.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/caddyhttp/reverseproxy/caddyfile.go b/modules/caddyhttp/reverseproxy/caddyfile.go index 4c816d06f0a..b4fd4cf8659 100644 --- a/modules/caddyhttp/reverseproxy/caddyfile.go +++ b/modules/caddyhttp/reverseproxy/caddyfile.go @@ -178,7 +178,7 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { return caddy.JoinNetworkAddress(network, host, port), nil } // if both network and port are empty, host address contains - // placeholdler (e.g. "{http.request.header.X-Caddy-Upstream-Dial}") and doing net.JoinHostPort(host, port) will + // placeholder (e.g. "{http.request.header.X-Caddy-Upstream-Dial}") and doing net.JoinHostPort(host, port) will // add an extreneous colon. if port == "" { return host, nil