Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
May 15, 2017

Conversation

aantono
Copy link
Contributor

@aantono aantono commented May 9, 2017

Addresses issue raised in #1569

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

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@aantono aantono May 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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? 😃

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

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

import (
"github.com/containous/traefik/log"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Similar for other import changes below.

},
}

for _, test := range cases {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot test := test. :-)

serverRoute := &serverRoute{route: route}
rules := &Rules{route: serverRoute}

expression := test.expression //"Host:foo.bar"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need the comment.

routeResult, err := rules.Parse(expression)

if err != nil {
t.Fatal("Error while building route for Host:foo.bar")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please include the error.

t.Fatal("Error while building route for Host:foo.bar")
}

request, err := http.NewRequest("GET", test.requestURL, nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

server := new(Server)

server.wireFrontendBackend(serverRoute, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if test.expectedURL != r.URL.String() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In Go, actual comes first, expected second.

Adjust in the next line too please.

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
}

for _, lbMethod := range []string{"Wrr", "Drr"} {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

}
}

func TestServerLoadConfigMultipleFrontendRules(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@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:

  1. Create a branch from upstream/v1.3 locally.
  2. 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.)
  3. Force-push the changes to your repo.
  4. 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
Copy link
Contributor Author

aantono commented May 12, 2017

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
Copy link
Contributor

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
Copy link
Contributor Author

aantono commented May 12, 2017

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

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

aantono commented May 12, 2017

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.

docs/basics.md Outdated
@@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you add a white line between title and text?

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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? 😃

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

import (
"github.com/containous/traefik/log"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

import (
"github.com/containous/traefik/log"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another round. :)

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can revert to using a single line I think.

docs/basics.md Outdated
### 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 👍

},
}

for _, test := range cases {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You forgot test := test. :-)

// 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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't used anywhere, or is it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!!!)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's my thinking too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the code to remove it from this particular place, so hopefully it's good to 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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@aantono aantono force-pushed the Issue1569 branch 3 times, most recently from 1b14f8e to 0ff0689 Compare May 13, 2017 03:40
routeMatch := routeResult.Match(request, &mux.RouteMatch{Route: routeResult})

if !routeMatch {
t.Fatalf("Rule %s don't match", expression)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammar: don't -> doesn't.

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

aantono commented May 15, 2017

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

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks complete from my perspective.

You have my LGTM.

@ldez ldez added this to the 1.3 milestone May 15, 2017
@aantono
Copy link
Contributor Author

aantono commented May 15, 2017

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.

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @aantono for your great contribution 👏
LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug/fix a bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants