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

aantono · 2017-05-09T20:32:20Z

Addresses issue raised in #1569

emilevauge

@aantono Thanks for your contribution :)
Few comments, but looks great!

emilevauge · 2017-05-12T14:29:37Z

middlewares/addPrefix.go

@@ -11,7 +12,9 @@ type AddPrefix struct {
 }

 func (s *AddPrefix) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	r.URL.Path = s.Prefix + r.URL.Path
+	newPath := s.Prefix + r.URL.Path
+	log.Debugf("Adding Prefix %s to %s = %s", s.Prefix, r.URL.Path, newPath)


Could you remove this Debug log? This is way too verbose to keep it in production code :)

Hello @emilevauge, I definitely can, I just added it because I thought it would be very useful to see in the debug mode (in production) how the modifiers get applied. It actually helped a lot while testing the config to make sure it works properly, and once deployed in prod, the debug mode is typically silenced, so the logs are not too verbose. But if you think that's not necessary, I'll surely take it out. Just let me know what your thoughts are on this.

@aantono we think it's not necessary and this can be a problem in some case (If you use some logs tools (like Logstash, Graylog,...) you possibly want to always use debug in production).

Could you remove? 😃

emilevauge · 2017-05-12T14:30:04Z

middlewares/replace_path.go

@@ -15,6 +16,12 @@ const ReplacedPathHeader = "X-Replaced-Path"

 func (s *ReplacePath) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	r.Header.Add(ReplacedPathHeader, r.URL.Path)
+	log.Debugf("Replacing Path %s with %s", r.URL.Path, s.Path)


Could you remove this Debug log? This is way too verbose to keep it in production code :)

emilevauge · 2017-05-12T14:30:11Z

middlewares/stripPrefix.go

@@ -18,6 +19,7 @@ type StripPrefix struct {
 func (s *StripPrefix) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	for _, prefix := range s.Prefixes {
 		if p := strings.TrimPrefix(r.URL.Path, strings.TrimSpace(prefix)); len(p) < len(r.URL.Path) {
+			log.Debugf("Striping Path Prefix %s from %s = %s", prefix, r.URL.Path, p)


Could you remove this Debug log? This is way too verbose to keep it in production code :)

emilevauge · 2017-05-12T14:30:17Z

middlewares/stripPrefixRegex.go

@@ -39,7 +39,9 @@ func (s *StripPrefixRegex) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			return
 		}

-		r.URL.Path = r.URL.Path[len(prefix.Path):]
+		newPath := r.URL.Path[len(prefix.Path):]
+		log.Debugf("Striping Path Prefix Regex %s from %s = %s", prefix.Path, r.URL.Path, newPath)


Could you remove this Debug log? This is way too verbose to keep it in production code :)

I think we can revert the entire change list block to its original state?

timoreimann

Left a few comments.

General question: Should we leave a remark at some place of the documentation describing the behavior when certain modifiers/matchers are used together (including that the order does not matter)?

timoreimann · 2017-05-12T13:41:54Z

middlewares/addPrefix.go

@@ -1,6 +1,7 @@
 package middlewares

 import (
+	"github.com/containous/traefik/log"


Can you add the non-stdlib imports after the stdlib ones, separated by a blank line?

Similar for other import changes below.

timoreimann · 2017-05-12T16:21:54Z

server/server_test.go

+		},
+	}
+
+	for _, test := range cases {


Please use sub-tests and call t.Parallel() inside. (Make sure you don't forget test := test to capture the loop variable.)

You forgot test := test. :-)

timoreimann · 2017-05-12T16:22:33Z

server/server_test.go

+		serverRoute := &serverRoute{route: route}
+		rules := &Rules{route: serverRoute}
+
+		expression := test.expression //"Host:foo.bar"


I don't think we need the comment.

timoreimann · 2017-05-12T16:22:47Z

server/server_test.go

+		routeResult, err := rules.Parse(expression)
+
+		if err != nil {
+			t.Fatal("Error while building route for Host:foo.bar")


Please include the error.

timoreimann · 2017-05-12T16:23:06Z

server/server_test.go

+			t.Fatal("Error while building route for Host:foo.bar")
+		}
+
+		request, err := http.NewRequest("GET", test.requestURL, nil)


Suggestion: Use http.MethodGet instead of the literal method name.

timoreimann · 2017-05-12T16:34:45Z

server/server_test.go

+		server := new(Server)
+
+		server.wireFrontendBackend(serverRoute, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			if test.expectedURL != r.URL.String() {


In Go, actual comes first, expected second.

Adjust in the next line too please.

timoreimann · 2017-05-12T16:37:47Z

server/server_test.go

+	}
+
+	for _, lbMethod := range []string{"Wrr", "Drr"} {
+		for _, rule := range rules {


You need to have rule := rule between line 128 and 129.

timoreimann · 2017-05-12T16:38:59Z

server/server_test.go

+		},
+	}
+
+	for _, lbMethod := range []string{"Wrr", "Drr"} {


I'd suggest to export loadBalancerMethodNames and iterate through that.

timoreimann · 2017-05-12T16:39:53Z

server/server_test.go

+					EntryPoints: EntryPoints{
+						"http": &EntryPoint{},
+					},
+					HealthCheck: &HealthCheckConfig{Interval: flaeg.Duration(5 * time.Second)},


We shouldn't need the health check for this test.

(By the way: In a singular, unified test, we'd just pass nil for "your" test cases.)

timoreimann · 2017-05-12T16:44:34Z

server/server_test.go

+	}
+}
+
+func TestServerLoadConfigMultipleFrontendRules(t *testing.T) {


Instead of duplicating lots of code from TestServerLoadConfigHealthCheckOptions, can we try to handle both test functions in a single test with separate test cases?

Let me know if you need help.

I'll remove this test, as I just realized that it is actually testing the same thing as TestServerMultipleFrontendRules does, so no need to duplicate.

timoreimann · 2017-05-12T16:56:13Z

@aantono we'd like this fix to still make it into the 1.3 release. Could you please rebase your PR onto the v1.3 branch?

Since some people tend to struggle with rebasing, here's a quick how-to if a simple git rebase does not work:

Create a branch from upstream/v1.3 locally.
Put your commit on top of that branch. (One easy way to do this is to check out the new branch and do git cherry-pick <commit from your feature branch>; another one is to check out the existing feature branch and do git rebase --onto <name of the new 1.3 branch> <commit from your feature branch>^. All of this assumes you still have a single commit only.)
Force-push the changes to your repo.
Click Edit at the top of this Github page and change the base to v1.3.

I can also do it for you if you like. Just let me know.

aantono · 2017-05-12T17:38:04Z

Will rebase and force push as soon as tests finish running locally. Will you cherry-pick the changes back into master, or do you want me to create a separate PR for that?

timoreimann · 2017-05-12T17:43:41Z

We'll merge the v1.3 branch back into master once this PR (and one or two others currently in the making) have landed. So you don't need to worry about getting things into master, I'll handle that. ☺️

aantono · 2017-05-12T17:44:27Z

Sounds good, just pushed the updates, please let me know if that looks as what you expect.

aantono · 2017-05-12T18:02:50Z

Also took a stab at adding documentation section about Rules Order, so let me know if that is what you were looking for as well.

ldez · 2017-05-12T18:04:50Z

docs/basics.md

@@ -191,6 +191,10 @@ 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.  Some `Path*Strip*` rules, like `PathPrefixStripRegex`, or `PathStrip` for example, are both `Matchers` and `Modifiers`, so in this case the `Matcher` portion of the rule will apply first, and the `Modifier` will apply later.


could you add a white line between title and text?

ldez · 2017-05-12T18:25:03Z

middlewares/addPrefix.go

@@ -11,7 +12,9 @@ type AddPrefix struct {
 }

 func (s *AddPrefix) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	r.URL.Path = s.Prefix + r.URL.Path
+	newPath := s.Prefix + r.URL.Path
+	log.Debugf("Adding Prefix %s to %s = %s", s.Prefix, r.URL.Path, newPath)


@aantono we think it's not necessary and this can be a problem in some case (If you use some logs tools (like Logstash, Graylog,...) you possibly want to always use debug in production).

Could you remove? 😃

ldez · 2017-05-12T18:25:50Z

middlewares/replace_path.go

@@ -1,6 +1,7 @@
 package middlewares

 import (
+	"github.com/containous/traefik/log"


Can you add the non-stdlib imports after the stdlib ones, separated by a blank line?

ldez · 2017-05-12T18:26:09Z

middlewares/stripPrefix.go

@@ -1,6 +1,7 @@
 package middlewares

 import (
+	"github.com/containous/traefik/log"


Can you add the non-stdlib imports after the stdlib ones, separated by a blank line?

timoreimann

Another round. :)

timoreimann · 2017-05-12T19:54:07Z

middlewares/addPrefix.go

@@ -11,7 +11,8 @@ type AddPrefix struct {
 }

 func (s *AddPrefix) ServeHTTP(w http.ResponseWriter, r *http.Request) {
-	r.URL.Path = s.Prefix + r.URL.Path
+	newPath := s.Prefix + r.URL.Path
+	r.URL.Path = newPath


We can revert to using a single line I think.

timoreimann · 2017-05-12T19:58:26Z

docs/basics.md

+### Rules Order
+
+When combining `Modifier` rules with `Matcher` rules, it is important to remember that `Modifier` rules **ALWAYS** apply after the `Matcher` rules.  Some `Path*Strip*` rules, like `PathPrefixStripRegex`, or `PathStrip` for example, are both `Matchers` and `Modifiers`, so in this case the `Matcher` portion of the rule will apply first, and the `Modifier` will apply later.
+`Modifiers` will be applied in a pre-determined order, `PathPrefixStripRegex` followed by `PathPrefixStrip`, followed by `AddPrefix` and `ReplacePath` being the last, regardless of their order in the `rule` configuration section. 


Can we have a real list here (one line per item) for better readability?

timoreimann · 2017-05-12T20:00:53Z

server/server_test.go

+			expectedURL: "http://foo.bar/blah/baz",
+		},
+		{
+			expression:  "PathPrefixStripRegex:/one/{two}/{three:[0-9]+}",


Wow, you are totally right -- we do offer PathPrefixStripRegex!

I must have missed it as it seemingly only entered the primary documentation when we released 1.3.0-rc1.

So all good. 👍

timoreimann · 2017-05-12T20:01:37Z

server/server_test.go

+		},
+	}
+
+	for _, test := range cases {


You forgot test := test. :-)

timoreimann · 2017-05-12T20:02:53Z

testhelpers/helpers.go

 // 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(rawurl string) *http.Request {


The method should match the parameters of http.NewRequest, i.e., accept a method and body parameter that would then be passed through.

timoreimann · 2017-05-12T20:03:25Z

middlewares/replace_path.go

@@ -18,3 +18,8 @@ func (s *ReplacePath) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	r.URL.Path = s.Path
 	s.Handler.ServeHTTP(w, r)
 }
+
+// SetHandler sets handler
+func (s *ReplacePath) SetHandler(Handler http.Handler) {


This isn't used anywhere, or is it?

To be honest, I'm not sure if this is being used or not, just saw it missing from this rule and present in addPrefix.go and stripPrefix.go, so added it here for consistency. If you think it's not needed, I'll be glad to remove it. (love deleting code!!!)

It should not be in use since you've added it only but there's no call happening.

If I'm wrong, the CI will tell us. :-)

I'll go ahead and remove it. Does that mean that it is not really needed in the other 2 places also? (addPrefix.go and stripPrefix.go)

That's my thinking too.

I've updated the code to remove it from this particular place, so hopefully it's good to go. ;)

timoreimann · 2017-05-12T20:04:54Z

server/server_test.go

+
+			server.wireFrontendBackend(serverRoute, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				if r.URL.String() != test.expectedURL {
+					t.Fatalf("Expected URL for [%s]: %s, got %s", test.expression, test.expectedURL, r.URL.String())


Please revert the reporting order (got first, expected second) here too.

timoreimann · 2017-05-12T20:06:10Z

server/server_test.go

+				if r.URL.String() != test.expectedURL {
+					t.Fatalf("Expected URL for [%s]: %s, got %s", test.expression, test.expectedURL, r.URL.String())
+				}
+				t.Logf("Received request to: %s", r.URL.String())


IMHO we don't need this anymore now that we use sub-tests.

timoreimann · 2017-05-13T04:03:33Z

server/server_test.go

+			routeMatch := routeResult.Match(request, &mux.RouteMatch{Route: routeResult})
+
+			if !routeMatch {
+				t.Fatalf("Rule %s don't match", expression)


Grammar: don't -> doesn't.

timoreimann · 2017-05-13T04:04:12Z

middlewares/stripPrefixRegex.go

@@ -39,7 +39,9 @@ func (s *StripPrefixRegex) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			return
 		}

-		r.URL.Path = r.URL.Path[len(prefix.Path):]
+		newPath := r.URL.Path[len(prefix.Path):]
+		log.Debugf("Striping Path Prefix Regex %s from %s = %s", prefix.Path, r.URL.Path, newPath)


I think we can revert the entire change list block to its original state?

timoreimann · 2017-05-13T04:07:53Z

server/server_test.go

+
+			server.wireFrontendBackend(serverRoute, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				if r.URL.String() != test.expectedURL {
+					t.Fatalf("Expected URL for [%s]: got %s, expected %s", test.expression, r.URL.String(), test.expectedURL)


We don't need to mention the expression since the sub test description already includes it and adds it automatically. So let's remove everything before and including the colon.

Let's just say got URL %s, expected %s.

aantono · 2017-05-15T03:57:07Z

I see this is still marked as waiting for corrections, is there anything else that I've missed?

timoreimann

It looks complete from my perspective.

You have my LGTM.

…hPrefixStrip traefik#1569

aantono · 2017-05-15T15:51:46Z

I've rebased the PR against the trunk of v1.3 branch to make it mergable...
Not sure why the auto-merge is not working, but looks like all the changes are in good order.

emilevauge

Thanks @aantono for your great contribution 👏
LGTM!

aantono force-pushed the Issue1569 branch 2 times, most recently from 4205655 to dab6b22 Compare May 9, 2017 21:08

ldez added kind/enhancement a new or improved feature. status/1-needs-design-review labels May 9, 2017

emilevauge requested changes May 12, 2017

View reviewed changes

timoreimann suggested changes May 12, 2017

View reviewed changes

ldez added contributor/waiting-for-corrections status/2-needs-review and removed status/1-needs-design-review labels May 12, 2017

aantono force-pushed the Issue1569 branch from dab6b22 to ccd44fe Compare May 12, 2017 17:38

aantono changed the base branch from master to v1.3 May 12, 2017 17:38

aantono force-pushed the Issue1569 branch from ccd44fe to 020d27d Compare May 12, 2017 17:43

aantono force-pushed the Issue1569 branch 2 times, most recently from 602b39a to 095d1ac Compare May 12, 2017 18:02

aantono force-pushed the Issue1569 branch from 095d1ac to af98890 Compare May 12, 2017 18:15

ldez requested changes May 12, 2017

View reviewed changes

aantono force-pushed the Issue1569 branch from af98890 to e599bb3 Compare May 12, 2017 18:38

timoreimann suggested changes May 12, 2017

View reviewed changes

aantono force-pushed the Issue1569 branch 3 times, most recently from 1b14f8e to 0ff0689 Compare May 13, 2017 03:40

timoreimann suggested changes May 13, 2017

View reviewed changes

aantono force-pushed the Issue1569 branch from 0ff0689 to 676610c Compare May 13, 2017 04:07

timoreimann reviewed May 13, 2017

View reviewed changes

aantono force-pushed the Issue1569 branch from 676610c to d086f72 Compare May 13, 2017 04:09

timoreimann added kind/bug/fix a bug fix and removed contributor/waiting-for-corrections labels May 15, 2017

timoreimann approved these changes May 15, 2017

View reviewed changes

ldez approved these changes May 15, 2017

View reviewed changes

ldez added this to the 1.3 milestone May 15, 2017

aantono force-pushed the Issue1569 branch from d086f72 to a4b9ab9 Compare May 15, 2017 14:45

Fixed ReplacePath rule executing out of order, when combined with Pat…

3f68e38

…hPrefixStrip traefik#1569

aantono force-pushed the Issue1569 branch from a4b9ab9 to 3f68e38 Compare May 15, 2017 15:08

emilevauge approved these changes May 15, 2017

View reviewed changes

emilevauge added status/3-needs-merge and removed status/2-needs-review labels May 15, 2017

emilevauge merged commit 30aa5a8 into traefik:v1.3 May 15, 2017

ldez mentioned this pull request May 15, 2017

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

Closed

ldez removed status/3-needs-merge kind/enhancement a new or improved feature. labels May 19, 2017

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

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

Conversation

aantono commented May 9, 2017

emilevauge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aantono May 12, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann commented May 12, 2017

aantono commented May 12, 2017

timoreimann commented May 12, 2017

aantono commented May 12, 2017

aantono commented May 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aantono commented May 15, 2017

timoreimann left a comment

Choose a reason for hiding this comment

aantono commented May 15, 2017 • edited

emilevauge left a comment

Choose a reason for hiding this comment

aantono May 12, 2017 •

edited

aantono commented May 15, 2017 •

edited