Ensure upstreams are sorted by longest first

pkg/upstream/proxy.go

@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"net/url"
 	"regexp"
+	"sort"
 	"strings"
 
 	"github.com/gorilla/mux"
@@ -26,7 +27,7 @@ func NewProxy(upstreams options.Upstreams, sigData *options.SignatureData, write
 		serveMux: mux.NewRouter(),
 	}
 
-	for _, upstream := range upstreams {
+	for _, upstream := range sortByPathLongest(upstreams) {
 		if upstream.Static {
 			if err := m.registerStaticResponseHandler(upstream, writer); err != nil {
 				return nil, fmt.Errorf("could not register static upstream %q: %v", upstream.ID, err)
@@ -145,3 +146,33 @@ func registerTrailingSlashHandler(serveMux *mux.Router) {
 		http.Redirect(rw, req, req.URL.String()+"/", http.StatusMovedPermanently)
 	}))
 }
+
+// sortByPathLongest ensures that the upstreams are sorted by longest path.
+// If rewrites are involved, a rewrite takes precedence over a non-rewrite.
+// When two upstreams define rewrites, whichever has the longest path will take
+// precedence (note this is the input to the rewrite logic).
+// This does not account for when a rewrite would actually make the path shorter.
+// This should maintain the sorting behaviour of the standard go serve mux.
+func sortByPathLongest(in options.Upstreams) options.Upstreams {
+	sort.Slice(in, func(i, j int) bool {
+		iRW := in[i].RewriteTarget
+		jRW := in[j].RewriteTarget
+
+		switch {
+		case iRW != "" && jRW != "":
+			// If both have a rewrite target, whichever has the longest pattern
+			// should go first
+			return len(in[i].Path) > len(in[j].Path)
+		case iRW != "" && jRW == "":
+			// Only one has rewrite, it goes first
+			return true
+		case iRW == "" && jRW != "":
+			// Only one has rewrite, it goes first
+			return false
+		default:
+			// Default to longest Path wins
+			return len(in[i].Path) > len(in[j].Path)
+		}
+	})
+	return in
+}

pkg/upstream/proxy_test.go

@@ -18,249 +18,369 @@ import (
 var _ = Describe("Proxy Suite", func() {
 	var upstreamServer http.Handler
 
-	BeforeEach(func() {
+	Context("multiUpstreamProxy", func() {
-		sigData := &options.SignatureData{Hash: crypto.SHA256, Key: "secret"}
+		BeforeEach(func() {
+			sigData := &options.SignatureData{Hash: crypto.SHA256, Key: "secret"}
 
-		writer := &pagewriter.WriterFuncs{
+			writer := &pagewriter.WriterFuncs{
-			ProxyErrorFunc: func(rw http.ResponseWriter, _ *http.Request, _ error) {
+				ProxyErrorFunc: func(rw http.ResponseWriter, _ *http.Request, _ error) {
-				rw.WriteHeader(502)
+					rw.WriteHeader(502)
-				rw.Write([]byte("Proxy Error"))
+					rw.Write([]byte("Proxy Error"))
-			},
+				},
-		}
-
-		ok := http.StatusOK
-		accepted := http.StatusAccepted
-
-		upstreams := options.Upstreams{
-			{
-				ID:   "http-backend",
-				Path: "/http/",
-				URI:  serverAddr,
-			},
-			{
-				ID:   "file-backend",
-				Path: "/files/",
-				URI:  fmt.Sprintf("file:///%s", filesDir),
-			},
-			{
-				ID:         "static-backend",
-				Path:       "/static/",
-				Static:     true,
-				StaticCode: &ok,
-			},
-			{
-				ID:         "static-backend-no-trailing-slash",
-				Path:       "/static",
-				Static:     true,
-				StaticCode: &accepted,
-			},
-			{
-				ID:   "bad-http-backend",
-				Path: "/bad-http/",
-				URI:  "http://::1",
-			},
-			{
-				ID:         "single-path-backend",
-				Path:       "/single-path",
-				Static:     true,
-				StaticCode: &ok,
-			},
-			{
-				ID:            "backend-with-rewrite-prefix",
-				Path:          "^/rewrite-prefix/(.*)",
-				RewriteTarget: "/different/backend/path/$1",
-				URI:           serverAddr,
-			},
-		}
-
-		var err error
-		upstreamServer, err = NewProxy(upstreams, sigData, writer)
-		Expect(err).ToNot(HaveOccurred())
-	})
-
-	type proxyTableInput struct {
-		target   string
-		response testHTTPResponse
-		upstream string
-	}
-
-	DescribeTable("Proxy ServeHTTP",
-		func(in *proxyTableInput) {
-			req := middlewareapi.AddRequestScope(
-				httptest.NewRequest("", in.target, nil),
-				&middlewareapi.RequestScope{},
-			)
-			rw := httptest.NewRecorder()
-			// Don't mock the remote Address
-			req.RemoteAddr = ""
-
-			upstreamServer.ServeHTTP(rw, req)
-
-			scope := middlewareapi.GetRequestScope(req)
-			Expect(scope.Upstream).To(Equal(in.upstream))
-
-			Expect(rw.Code).To(Equal(in.response.code))
-
-			// Delete extra headers that aren't relevant to tests
-			testSanitizeResponseHeader(rw.Header())
-			Expect(rw.Header()).To(Equal(in.response.header))
-
-			body := rw.Body.Bytes()
-			// If the raw body is set, check that, else check the Request object
-			if in.response.raw != "" {
-				Expect(string(body)).To(Equal(in.response.raw))
-				return
 			}
 
-			// Compare the reflected request to the upstream
+			ok := http.StatusOK
-			request := testHTTPRequest{}
+			accepted := http.StatusAccepted
-			Expect(json.Unmarshal(body, &request)).To(Succeed())
+
-			testSanitizeRequestHeader(request.Header)
+			upstreams := options.Upstreams{
-			Expect(request).To(Equal(in.response.request))
+				{
-		},
+					ID:   "http-backend",
-		Entry("with a request to the HTTP service", &proxyTableInput{
+					Path: "/http/",
-			target: "http://example.localhost/http/1234",
+					URI:  serverAddr,
-			response: testHTTPResponse{
-				code: 200,
-				header: map[string][]string{
-					contentType: {applicationJSON},
 				},
-				request: testHTTPRequest{
+				{
-					Method: "GET",
+					ID:   "file-backend",
-					URL:    "http://example.localhost/http/1234",
+					Path: "/files/",
-					Header: map[string][]string{
+					URI:  fmt.Sprintf("file:///%s", filesDir),
-						"Gap-Auth":      {""},
+				},
-						"Gap-Signature": {"sha256 ofB1u6+FhEUbFLc3/uGbJVkl7GaN4egFqVvyO3+2I1w="},
+				{
+					ID:         "static-backend",
+					Path:       "/static/",
+					Static:     true,
+					StaticCode: &ok,
+				},
+				{
+					ID:         "static-backend-no-trailing-slash",
+					Path:       "/static",
+					Static:     true,
+					StaticCode: &accepted,
+				},
+				{
+					ID:         "static-backend-long",
+					Path:       "/static/long",
+					Static:     true,
+					StaticCode: &accepted,
+				},
+				{
+					ID:   "bad-http-backend",
+					Path: "/bad-http/",
+					URI:  "http://::1",
+				},
+				{
+					ID:         "single-path-backend",
+					Path:       "/single-path",
+					Static:     true,
+					StaticCode: &ok,
+				},
+				{
+					ID:            "backend-with-rewrite-prefix",
+					Path:          "^/rewrite-prefix/(.*)",
+					RewriteTarget: "/different/backend/path/$1",
+					URI:           serverAddr,
+				},
+				{
+					ID:   "double-match-plain",
+					Path: "/double-match/",
+					URI:  serverAddr,
+				},
+				{
+					ID:            "double-match-rewrite",
+					Path:          "^/double-match/(.*)",
+					RewriteTarget: "/double-match/rewrite/$1",
+					URI:           serverAddr,
+				},
+			}
+
+			var err error
+			upstreamServer, err = NewProxy(upstreams, sigData, writer)
+			Expect(err).ToNot(HaveOccurred())
+		})
+
+		type proxyTableInput struct {
+			target   string
+			response testHTTPResponse
+			upstream string
+		}
+
+		DescribeTable("Proxy ServeHTTP",
+			func(in *proxyTableInput) {
+				req := middlewareapi.AddRequestScope(
+					httptest.NewRequest("", in.target, nil),
+					&middlewareapi.RequestScope{},
+				)
+				rw := httptest.NewRecorder()
+				// Don't mock the remote Address
+				req.RemoteAddr = ""
+
+				upstreamServer.ServeHTTP(rw, req)
+
+				scope := middlewareapi.GetRequestScope(req)
+				Expect(scope.Upstream).To(Equal(in.upstream))
+
+				Expect(rw.Code).To(Equal(in.response.code))
+
+				// Delete extra headers that aren't relevant to tests
+				testSanitizeResponseHeader(rw.Header())
+				Expect(rw.Header()).To(Equal(in.response.header))
+
+				body := rw.Body.Bytes()
+				// If the raw body is set, check that, else check the Request object
+				if in.response.raw != "" {
+					Expect(string(body)).To(Equal(in.response.raw))
+					return
+				}
+
+				// Compare the reflected request to the upstream
+				request := testHTTPRequest{}
+				Expect(json.Unmarshal(body, &request)).To(Succeed())
+				testSanitizeRequestHeader(request.Header)
+				Expect(request).To(Equal(in.response.request))
+			},
+			Entry("with a request to the HTTP service", &proxyTableInput{
+				target: "http://example.localhost/http/1234",
+				response: testHTTPResponse{
+					code: 200,
+					header: map[string][]string{
+						contentType: {applicationJSON},
 					},
-					Body:       []byte{},
+					request: testHTTPRequest{
-					Host:       "example.localhost",
+						Method: "GET",
-					RequestURI: "http://example.localhost/http/1234",
+						URL:    "http://example.localhost/http/1234",
-				},
+						Header: map[string][]string{
-			},
+							"Gap-Auth":      {""},
-			upstream: "http-backend",
+							"Gap-Signature": {"sha256 ofB1u6+FhEUbFLc3/uGbJVkl7GaN4egFqVvyO3+2I1w="},
-		}),
+						},
-		Entry("with a request to the File backend", &proxyTableInput{
+						Body:       []byte{},
-			target: "http://example.localhost/files/foo",
+						Host:       "example.localhost",
-			response: testHTTPResponse{
+						RequestURI: "http://example.localhost/http/1234",
-				code: 200,
-				header: map[string][]string{
-					contentType: {textPlainUTF8},
-				},
-				raw: "foo",
-			},
-			upstream: "file-backend",
-		}),
-		Entry("with a request to the Static backend", &proxyTableInput{
-			target: "http://example.localhost/static/bar",
-			response: testHTTPResponse{
-				code:   200,
-				header: map[string][]string{},
-				raw:    "Authenticated",
-			},
-			upstream: "static-backend",
-		}),
-		Entry("with a request to the bad HTTP backend", &proxyTableInput{
-			target: "http://example.localhost/bad-http/bad",
-			response: testHTTPResponse{
-				code:   502,
-				header: map[string][]string{},
-				// This tests the error handler
-				raw: "Proxy Error",
-			},
-			upstream: "bad-http-backend",
-		}),
-		Entry("with a request to the to an unregistered path", &proxyTableInput{
-			target: "http://example.localhost/unregistered",
-			response: testHTTPResponse{
-				code: 404,
-				header: map[string][]string{
-					"X-Content-Type-Options": {"nosniff"},
-					contentType:              {textPlainUTF8},
-				},
-				raw: "404 page not found\n",
-			},
-		}),
-		Entry("with a request to the to backend registered to a single path", &proxyTableInput{
-			target: "http://example.localhost/single-path",
-			response: testHTTPResponse{
-				code:   200,
-				header: map[string][]string{},
-				raw:    "Authenticated",
-			},
-			upstream: "single-path-backend",
-		}),
-		Entry("with a request to the to a subpath of a backend registered to a single path", &proxyTableInput{
-			target: "http://example.localhost/single-path/unregistered",
-			response: testHTTPResponse{
-				code: 404,
-				header: map[string][]string{
-					"X-Content-Type-Options": {"nosniff"},
-					contentType:              {textPlainUTF8},
-				},
-				raw: "404 page not found\n",
-			},
-		}),
-		Entry("with a request to the rewrite prefix server", &proxyTableInput{
-			target: "http://example.localhost/rewrite-prefix/1234",
-			response: testHTTPResponse{
-				code: 200,
-				header: map[string][]string{
-					contentType: {applicationJSON},
-				},
-				request: testHTTPRequest{
-					Method: "GET",
-					URL:    "http://example.localhost/different/backend/path/1234",
-					Header: map[string][]string{
-						"Gap-Auth":      {""},
-						"Gap-Signature": {"sha256 jeAeM7wHSj2ab/l9YPvtTJ9l/8q1tpY2V/iwXF48bgw="},
 					},
-					Body:       []byte{},
-					Host:       "example.localhost",
-					RequestURI: "http://example.localhost/different/backend/path/1234",
 				},
-			},
+				upstream: "http-backend",
-			upstream: "backend-with-rewrite-prefix",
+			}),
-		}),
+			Entry("with a request to the File backend", &proxyTableInput{
-		Entry("with a request to a subpath of the rewrite prefix server", &proxyTableInput{
+				target: "http://example.localhost/files/foo",
-			target: "http://example.localhost/rewrite-prefix/1234/abc",
+				response: testHTTPResponse{
-			response: testHTTPResponse{
+					code: 200,
-				code: 200,
+					header: map[string][]string{
-				header: map[string][]string{
+						contentType: {textPlainUTF8},
-					contentType: {applicationJSON},
-				},
-				request: testHTTPRequest{
-					Method: "GET",
-					URL:    "http://example.localhost/different/backend/path/1234/abc",
-					Header: map[string][]string{
-						"Gap-Auth":      {""},
-						"Gap-Signature": {"sha256 rAkAc9gp7EndoOppJuvbuPnYuBcqrTkBnQx6iPS8xTA="},
 					},
-					Body:       []byte{},
+					raw: "foo",
-					Host:       "example.localhost",
-					RequestURI: "http://example.localhost/different/backend/path/1234/abc",
 				},
-			},
+				upstream: "file-backend",
-			upstream: "backend-with-rewrite-prefix",
+			}),
-		}),
+			Entry("with a request to the Static backend", &proxyTableInput{
-		Entry("with a request to a path, missing the trailing slash", &proxyTableInput{
+				target: "http://example.localhost/static/bar",
-			target: "http://example.localhost/http",
+				response: testHTTPResponse{
-			response: testHTTPResponse{
+					code:   200,
-				code: 301,
+					header: map[string][]string{},
-				header: map[string][]string{
+					raw:    "Authenticated",
-					contentType: {textHTMLUTF8},
-					"Location":  {"http://example.localhost/http/"},
 				},
-				raw: "<a href=\"http://example.localhost/http/\">Moved Permanently</a>.\n\n",
+				upstream: "static-backend",
+			}),
+			Entry("with a request to the bad HTTP backend", &proxyTableInput{
+				target: "http://example.localhost/bad-http/bad",
+				response: testHTTPResponse{
+					code:   502,
+					header: map[string][]string{},
+					// This tests the error handler
+					raw: "Proxy Error",
+				},
+				upstream: "bad-http-backend",
+			}),
+			Entry("with a request to the to an unregistered path", &proxyTableInput{
+				target: "http://example.localhost/unregistered",
+				response: testHTTPResponse{
+					code: 404,
+					header: map[string][]string{
+						"X-Content-Type-Options": {"nosniff"},
+						contentType:              {textPlainUTF8},
+					},
+					raw: "404 page not found\n",
+				},
+			}),
+			Entry("with a request to the to backend registered to a single path", &proxyTableInput{
+				target: "http://example.localhost/single-path",
+				response: testHTTPResponse{
+					code:   200,
+					header: map[string][]string{},
+					raw:    "Authenticated",
+				},
+				upstream: "single-path-backend",
+			}),
+			Entry("with a request to the to a subpath of a backend registered to a single path", &proxyTableInput{
+				target: "http://example.localhost/single-path/unregistered",
+				response: testHTTPResponse{
+					code: 404,
+					header: map[string][]string{
+						"X-Content-Type-Options": {"nosniff"},
+						contentType:              {textPlainUTF8},
+					},
+					raw: "404 page not found\n",
+				},
+			}),
+			Entry("with a request to the rewrite prefix server", &proxyTableInput{
+				target: "http://example.localhost/rewrite-prefix/1234",
+				response: testHTTPResponse{
+					code: 200,
+					header: map[string][]string{
+						contentType: {applicationJSON},
+					},
+					request: testHTTPRequest{
+						Method: "GET",
+						URL:    "http://example.localhost/different/backend/path/1234",
+						Header: map[string][]string{
+							"Gap-Auth":      {""},
+							"Gap-Signature": {"sha256 jeAeM7wHSj2ab/l9YPvtTJ9l/8q1tpY2V/iwXF48bgw="},
+						},
+						Body:       []byte{},
+						Host:       "example.localhost",
+						RequestURI: "http://example.localhost/different/backend/path/1234",
+					},
+				},
+				upstream: "backend-with-rewrite-prefix",
+			}),
+			Entry("with a request to a subpath of the rewrite prefix server", &proxyTableInput{
+				target: "http://example.localhost/rewrite-prefix/1234/abc",
+				response: testHTTPResponse{
+					code: 200,
+					header: map[string][]string{
+						contentType: {applicationJSON},
+					},
+					request: testHTTPRequest{
+						Method: "GET",
+						URL:    "http://example.localhost/different/backend/path/1234/abc",
+						Header: map[string][]string{
+							"Gap-Auth":      {""},
+							"Gap-Signature": {"sha256 rAkAc9gp7EndoOppJuvbuPnYuBcqrTkBnQx6iPS8xTA="},
+						},
+						Body:       []byte{},
+						Host:       "example.localhost",
+						RequestURI: "http://example.localhost/different/backend/path/1234/abc",
+					},
+				},
+				upstream: "backend-with-rewrite-prefix",
+			}),
+			Entry("with a request to a path, missing the trailing slash", &proxyTableInput{
+				target: "http://example.localhost/http",
+				response: testHTTPResponse{
+					code: 301,
+					header: map[string][]string{
+						contentType: {textHTMLUTF8},
+						"Location":  {"http://example.localhost/http/"},
+					},
+					raw: "<a href=\"http://example.localhost/http/\">Moved Permanently</a>.\n\n",
+				},
+			}),
+			Entry("with a request to a path, missing the trailing slash, but registered separately", &proxyTableInput{
+				target: "http://example.localhost/static",
+				response: testHTTPResponse{
+					code:   202,
+					header: map[string][]string{},
+					raw:    "Authenticated",
+				},
+				upstream: "static-backend-no-trailing-slash",
+			}),
+			Entry("should match longest path first", &proxyTableInput{
+				target: "http://example.localhost/static/long",
+				response: testHTTPResponse{
+					code:   202,
+					header: map[string][]string{},
+					raw:    "Authenticated",
+				},
+				upstream: "static-backend-long",
+			}),
+			Entry("should match rewrite path first", &proxyTableInput{
+				target: "http://example.localhost/double-match/foo",
+				response: testHTTPResponse{
+					code: 200,
+					header: map[string][]string{
+						contentType: {applicationJSON},
+					},
+					request: testHTTPRequest{
+						Method: "GET",
+						URL:    "http://example.localhost/double-match/rewrite/foo",
+						Header: map[string][]string{
+							"Gap-Auth":      {""},
+							"Gap-Signature": {"sha256 eYyUNdsrTmnvFpavpP8AdHGUGzqJ39QEjqn0/3fQPHA="},
+						},
+						Body:       []byte{},
+						Host:       "example.localhost",
+						RequestURI: "http://example.localhost/double-match/rewrite/foo",
+					},
+				},
+				upstream: "double-match-rewrite",
+			}),
+		)
+	})
+
+	Context("sortByPathLongest", func() {
+		type sortByPathLongestTableInput struct {
+			input          options.Upstreams
+			expectedOutput options.Upstreams
+		}
+
+		var httpPath = options.Upstream{
+			Path: "/http/",
+		}
+
+		var httpSubPath = options.Upstream{
+			Path: "/http/subpath/",
+		}
+
+		var longerPath = options.Upstream{
+			Path: "/longer-than-http",
+		}
+
+		var shortPathWithRewrite = options.Upstream{
+			Path:          "^/h/(.*)",
+			RewriteTarget: "/$1",
+		}
+
+		var shortSubPathWithRewrite = options.Upstream{
+			Path:          "^/h/bar/(.*)",
+			RewriteTarget: "/$1",
+		}
+
+		DescribeTable("short sort into the correct order",
+			func(in sortByPathLongestTableInput) {
+				Expect(sortByPathLongest(in.input)).To(Equal(in.expectedOutput))
 			},
-		}),
+			Entry("with a mix of paths registered", sortByPathLongestTableInput{
-		Entry("with a request to a path, missing the trailing slash, but registered separately", &proxyTableInput{
+				input:          options.Upstreams{httpPath, httpSubPath, shortSubPathWithRewrite, longerPath, shortPathWithRewrite},
-			target: "http://example.localhost/static",
+				expectedOutput: options.Upstreams{shortSubPathWithRewrite, shortPathWithRewrite, longerPath, httpSubPath, httpPath},
-			response: testHTTPResponse{
+			}),
-				code:   202,
+			Entry("when a subpath is registered (in order)", sortByPathLongestTableInput{
-				header: map[string][]string{},
+				input:          options.Upstreams{httpSubPath, httpPath},
-				raw:    "Authenticated",
+				expectedOutput: options.Upstreams{httpSubPath, httpPath},
-			},
+			}),
-			upstream: "static-backend-no-trailing-slash",
+			Entry("when a subpath is registered (out of order)", sortByPathLongestTableInput{
-		}),
+				input:          options.Upstreams{httpPath, httpSubPath},
-	)
+				expectedOutput: options.Upstreams{httpSubPath, httpPath},
+			}),
+			Entry("when longer paths are registered (in order)", sortByPathLongestTableInput{
+				input:          options.Upstreams{longerPath, httpPath},
+				expectedOutput: options.Upstreams{longerPath, httpPath},
+			}),
+			Entry("when longer paths are registered (out of order)", sortByPathLongestTableInput{
+				input:          options.Upstreams{httpPath, longerPath},
+				expectedOutput: options.Upstreams{longerPath, httpPath},
+			}),
+			Entry("when a rewrite target is registered (in order)", sortByPathLongestTableInput{
+				input:          options.Upstreams{shortPathWithRewrite, longerPath},
+				expectedOutput: options.Upstreams{shortPathWithRewrite, longerPath},
+			}),
+			Entry("when a rewrite target is registered (out of order)", sortByPathLongestTableInput{
+				input:          options.Upstreams{longerPath, shortPathWithRewrite},
+				expectedOutput: options.Upstreams{shortPathWithRewrite, longerPath},
+			}),
+			Entry("with multiple rewrite targets registered (in order)", sortByPathLongestTableInput{
+				input:          options.Upstreams{shortSubPathWithRewrite, shortPathWithRewrite},
+				expectedOutput: options.Upstreams{shortSubPathWithRewrite, shortPathWithRewrite},
+			}),
+			Entry("with multiple rewrite targets registered (out of order)", sortByPathLongestTableInput{
+				input:          options.Upstreams{shortPathWithRewrite, shortSubPathWithRewrite},
+				expectedOutput: options.Upstreams{shortSubPathWithRewrite, shortPathWithRewrite},
+			}),
+		)
+	})
 })