diff --git a/pkg/upstream/proxy.go b/pkg/upstream/proxy.go index 3907eb0a..594824e2 100644 --- a/pkg/upstream/proxy.go +++ b/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 +} diff --git a/pkg/upstream/proxy_test.go b/pkg/upstream/proxy_test.go index 5ac9842c..597f5422 100644 --- a/pkg/upstream/proxy_test.go +++ b/pkg/upstream/proxy_test.go @@ -18,249 +18,369 @@ import ( var _ = Describe("Proxy Suite", func() { var upstreamServer http.Handler - BeforeEach(func() { - sigData := &options.SignatureData{Hash: crypto.SHA256, Key: "secret"} + Context("multiUpstreamProxy", func() { + BeforeEach(func() { + sigData := &options.SignatureData{Hash: crypto.SHA256, Key: "secret"} - writer := &pagewriter.WriterFuncs{ - ProxyErrorFunc: func(rw http.ResponseWriter, _ *http.Request, _ error) { - rw.WriteHeader(502) - 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 + writer := &pagewriter.WriterFuncs{ + ProxyErrorFunc: func(rw http.ResponseWriter, _ *http.Request, _ error) { + rw.WriteHeader(502) + rw.Write([]byte("Proxy Error")) + }, } - // 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}, + ok := http.StatusOK + accepted := http.StatusAccepted + + upstreams := options.Upstreams{ + { + ID: "http-backend", + Path: "/http/", + URI: serverAddr, }, - request: testHTTPRequest{ - Method: "GET", - URL: "http://example.localhost/http/1234", - Header: map[string][]string{ - "Gap-Auth": {""}, - "Gap-Signature": {"sha256 ofB1u6+FhEUbFLc3/uGbJVkl7GaN4egFqVvyO3+2I1w="}, + { + 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: "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{}, - Host: "example.localhost", - RequestURI: "http://example.localhost/http/1234", - }, - }, - upstream: "http-backend", - }), - Entry("with a request to the File backend", &proxyTableInput{ - target: "http://example.localhost/files/foo", - response: testHTTPResponse{ - 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="}, + request: testHTTPRequest{ + Method: "GET", + URL: "http://example.localhost/http/1234", + Header: map[string][]string{ + "Gap-Auth": {""}, + "Gap-Signature": {"sha256 ofB1u6+FhEUbFLc3/uGbJVkl7GaN4egFqVvyO3+2I1w="}, + }, + Body: []byte{}, + Host: "example.localhost", + RequestURI: "http://example.localhost/http/1234", }, - 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="}, + upstream: "http-backend", + }), + Entry("with a request to the File backend", &proxyTableInput{ + target: "http://example.localhost/files/foo", + response: testHTTPResponse{ + code: 200, + header: map[string][]string{ + contentType: {textPlainUTF8}, }, - Body: []byte{}, - Host: "example.localhost", - RequestURI: "http://example.localhost/different/backend/path/1234/abc", + raw: "foo", }, - }, - 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/"}, + 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", }, - raw: "Moved Permanently.\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: "Moved Permanently.\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 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("with a mix of paths registered", sortByPathLongestTableInput{ + input: options.Upstreams{httpPath, httpSubPath, shortSubPathWithRewrite, longerPath, shortPathWithRewrite}, + expectedOutput: options.Upstreams{shortSubPathWithRewrite, shortPathWithRewrite, longerPath, httpSubPath, httpPath}, + }), + Entry("when a subpath is registered (in order)", sortByPathLongestTableInput{ + input: options.Upstreams{httpSubPath, httpPath}, + expectedOutput: options.Upstreams{httpSubPath, httpPath}, + }), + 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}, + }), + ) + }) })