Fixed ReplacePath rule executing out of order, when combined with PathPrefixStrip #1569

docs/basics.md

@@ -191,6 +191,25 @@ backend = "backend2"
   rule = "Path:/test1,/test2"
 ```
 
+### Rules Order
+
+When combining `Modifier` rules with `Matcher` rules, it is important to remember that `Modifier` rules **ALWAYS** apply after the `Matcher` rules.  
+The following rules are both `Matchers` and `Modifiers`, so the `Matcher` portion of the rule will apply first, and the `Modifier` will apply later.
+
+- `PathStrip`
+- `PathStripRegex`
+- `PathPrefixStrip`
+- `PathPrefixStripRegex`
+
+`Modifiers` will be applied in a pre-determined order regardless of their order in the `rule` configuration section.
+
+1. `PathStrip`
+2. `PathPrefixStrip`
+3. `PathStripRegex`
+4. `PathPrefixStripRegex`
+5. `AddPrefix`
+6. `ReplacePath` 
+
 ### Priorities
 
 By default, routes will be sorted (in descending order) using rules length (to avoid path overlap):

server/server.go

@@ -776,7 +776,17 @@ func (server *Server) loadConfig(configurations configs, globalConfiguration Glo
 }
 
 func (server *Server) wireFrontendBackend(serverRoute *serverRoute, handler http.Handler) {
-	// add prefix
+	// path replace - This needs to always be the very last on the handler chain (first in the order in this function)
+	// -- Replacing Path should happen at the very end of the Modifier chain, after all the Matcher+Modifiers ran
+	if len(serverRoute.replacePath) > 0 {
+		handler = &middlewares.ReplacePath{
+			Path:    serverRoute.replacePath,
+			Handler: handler,
+		}
+	}
+
+	// add prefix - This needs to always be right before ReplacePath on the chain (second in order in this function)
+	// -- Adding Path Prefix should happen after all *Strip Matcher+Modifiers ran, but before Replace (in case it's configured)
 	if len(serverRoute.addPrefix) > 0 {
 		handler = &middlewares.AddPrefix{
 			Prefix:  serverRoute.addPrefix,
@@ -797,14 +807,6 @@ func (server *Server) wireFrontendBackend(serverRoute *serverRoute, handler http
 		handler = middlewares.NewStripPrefixRegex(handler, serverRoute.stripPrefixesRegex)
 	}
 
-	// path replace
-	if len(serverRoute.replacePath) > 0 {
-		handler = &middlewares.ReplacePath{
-			Path:    serverRoute.replacePath,
-			Handler: handler,
-		}
-	}
-
 	serverRoute.route.Handler(handler)
 }
 

server/server_test.go

@@ -2,13 +2,16 @@ package server
 
 import (
 	"fmt"
+	"net/http"
 	"net/url"
 	"reflect"
 	"testing"
 	"time"
 
 	"github.com/containous/flaeg"
+	"github.com/containous/mux"
 	"github.com/containous/traefik/healthcheck"
+	"github.com/containous/traefik/testhelpers"
 	"github.com/containous/traefik/types"
 	"github.com/vulcand/oxy/roundrobin"
 )
@@ -27,6 +30,85 @@ func (lb *testLoadBalancer) Servers() []*url.URL {
 	return []*url.URL{}
 }
 
+func TestServerMultipleFrontendRules(t *testing.T) {
+	cases := []struct {
+		expression  string
+		requestURL  string
+		expectedURL string
+	}{
+		{
+			expression:  "Host:foo.bar",
+			requestURL:  "http://foo.bar",
+			expectedURL: "http://foo.bar",
+		},
+		{
+			expression:  "PathPrefix:/management;ReplacePath:/health",
+			requestURL:  "http://foo.bar/management",
+			expectedURL: "http://foo.bar/health",
+		},
+		{
+			expression:  "Host:foo.bar;AddPrefix:/blah",
+			requestURL:  "http://foo.bar/baz",
+			expectedURL: "http://foo.bar/blah/baz",
+		},
+		{
+			expression:  "PathPrefixStripRegex:/one/{two}/{three:[0-9]+}",
+			requestURL:  "http://foo.bar/one/some/12345/four",
+			expectedURL: "http://foo.bar/four",
+		},
+		{
+			expression:  "PathPrefixStripRegex:/one/{two}/{three:[0-9]+};AddPrefix:/zero",
+			requestURL:  "http://foo.bar/one/some/12345/four",
+			expectedURL: "http://foo.bar/zero/four",
+		},
+		{
+			expression:  "AddPrefix:/blah;ReplacePath:/baz",
+			requestURL:  "http://foo.bar/hello",
+			expectedURL: "http://foo.bar/baz",
+		},
+		{
+			expression:  "PathPrefixStrip:/management;ReplacePath:/health",
+			requestURL:  "http://foo.bar/management",
+			expectedURL: "http://foo.bar/health",
+		},
+	}
+
+	for _, test := range cases {
+		test := test
+		t.Run(test.expression, func(t *testing.T) {
+			t.Parallel()
+
+			router := mux.NewRouter()
+			route := router.NewRoute()
+			serverRoute := &serverRoute{route: route}
+			rules := &Rules{route: serverRoute}
+
+			expression := test.expression
+			routeResult, err := rules.Parse(expression)
+
+			if err != nil {
+				t.Fatalf("Error while building route for %s: %+v", expression, err)
+			}
+
+			request := testhelpers.MustNewRequest(http.MethodGet, test.requestURL, nil)
+			routeMatch := routeResult.Match(request, &mux.RouteMatch{Route: routeResult})
+
+			if !routeMatch {
+				t.Fatalf("Rule %s doesn't match", expression)
+			}
+
+			server := new(Server)
+
+			server.wireFrontendBackend(serverRoute, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				if r.URL.String() != test.expectedURL {
+					t.Fatalf("got URL %s, expected %s", r.URL.String(), test.expectedURL)
+				}
+			}))
+			serverRoute.route.GetHandler().ServeHTTP(nil, request)
+		})
+	}
+}
+
 func TestServerLoadConfigHealthCheckOptions(t *testing.T) {
 	healthChecks := []*types.HealthCheck{
 		nil,

testhelpers/helpers.go

@@ -1,6 +1,21 @@
 package testhelpers
 
+import (
+	"fmt"
+	"io"
+	"net/http"
+)
+
 // Intp returns a pointer to the given integer value.
 func Intp(i int) *int {
 	return &i
 }
+
+// MustNewRequest creates a new http get request or panics if it can't
+func MustNewRequest(method, urlStr string, body io.Reader) *http.Request {
+	request, err := http.NewRequest(method, urlStr, body)
+	if err != nil {
+		panic(fmt.Sprintf("failed to create HTTP %s Request for '%s': %s", method, urlStr, err))
+	}
+	return request
+}