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

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

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

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

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

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

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

I don't think we need the comment.

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

server/server_test.go

Please include the error.

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

server/server_test.go

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

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

server/server_test.go

In Go, actual comes first, expected second.

Adjust in the next line too please.

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

server/server_test.go

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

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

server/server_test.go

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

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

server/server_test.go

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

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

could you add a white line between title and text?

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

middlewares/addPrefix.go

@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

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

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

We can revert to using a single line I think.

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

docs/basics.md

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

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

server/server_test.go

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

You forgot test := test. :-)

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

testhelpers/helpers.go

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

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

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

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

server/server_test.go

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

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

server/server_test.go

Grammar: don't -> doesn't.

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

middlewares/stripPrefixRegex.go

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

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

server/server_test.go

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 suggested 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

Uh oh!

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

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

Uh oh!

Conversation

aantono commented May 9, 2017

Uh oh!

emilevauge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aantono May 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timoreimann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timoreimann commented May 12, 2017

Uh oh!

aantono commented May 12, 2017

Uh oh!

timoreimann commented May 12, 2017

Uh oh!

aantono commented May 12, 2017

Uh oh!

aantono commented May 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timoreimann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aantono May 12, 2017 •

edited

Loading