Fix behavior for PathPrefixStrip #1638

seryl · 2017-05-20T04:51:24Z

When pushing data to downstream proxies; malformed requests were being
sent.

The corrected behavior is as follows:

Route Stripped	URL	Passed to Backend
/	/	/

Route Stripped	URL	Passed to Backend
/stat	/stat	/
/stat	/stat/	/
/stat	/status	/status
/stat	/stat/us	/us

Route Stripped	URL	Passed to Backend
/stat/	/stat	/stat
/stat/	/stat/	/
/stat/	/status	/status
/stat/	/stat/us	/us

Prior, we could strip the prefixing /, and we'd also ignore the case
where you want to serve something like /api as both the index and as a
subpath.

Additionally, this should resolve a myriad of issues relating to
kubernetes ingress PathPrefixStrip.

Description

Should fix the following tickets:

Potentially fixes:

Error forwarding with err: EOF in logs when using pathprefixstrip on / route in kubernetes setup #1054
Redirect path to domain.com/web is not working (Docker Swarm Mode 1.13) #1123

seryl · 2017-05-20T04:59:23Z

Example of malformed requests:

10.244.0.164 - - [19/May/2017:23:58:14 +0000] "GET app.301d8f1c250fa1437ad7.js HTTP/1.1" 400 173 "-" "-" "-"
10.244.0.164 - - [19/May/2017:23:58:15 +0000] "GET app.301d8f1c250fa1437ad7.js HTTP/1.1" 400 173 "-" "-" "-"
10.244.0.164 - - [19/May/2017:23:58:17 +0000] "GET app.301d8f1c250fa1437ad7.js HTTP/1.1" 400 173 "-" "-" "-"
10.244.0.164 - - [19/May/2017:23:58:58 +0000] "GET _/public/img/pack.png HTTP/1.1" 400 173 "-" "-" "-"
10.244.0.164 - - [19/May/2017:23:59:07 +0000] "GET favicon.ico HTTP/1.1" 400 173 "-" "-" "-"
10.244.0.164 - - [20/May/2017:00:00:35 +0000] "GET app.301d8f1c250fa1437ad7.js HTTP/1.1" 400 173 "-" "-" "-"
10.244.0.164 - - [20/May/2017:00:00:35 +0000] "GET app.301d8f1c250fa1437ad7.js HTTP/1.1" 400 173 "-" "-" "-"
10.244.0.164 - - [20/May/2017:00:00:38 +0000] "GET favicon.ico HTTP/1.1" 400 173 "-" "-" "-"
10.244.0.164 - - [20/May/2017:00:00:41 +0000] "GET favicon.ico HTTP/1.1" 400 173 "-" "-" "-"
10.244.0.164 - - [20/May/2017:00:00:44 +0000] "GET favicon.ico HTTP/1.1" 400 173 "-" "-" "-

Note: They are all missing prefixed paths.

timoreimann

Thanks for the contribution -- looking forward to seeing a long-standing issue getting resolved!

I left a few comments.

timoreimann · 2017-05-20T06:36:48Z

middlewares/stripPrefix.go

-			r.Header[forwardedPrefixHeader] = []string{prefix}
-			r.RequestURI = r.URL.RequestURI()
-			s.Handler.ServeHTTP(w, r)
+		orgPrefix := strings.TrimSpace(prefix)


I suppose org stands for original? If so, could we maybe rename it to orig?

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

middlewares/stripPrefix.go

-			r.Header[forwardedPrefixHeader] = []string{prefix}
-			r.RequestURI = r.URL.RequestURI()
-			s.Handler.ServeHTTP(w, r)
+		orgPrefix := strings.TrimSpace(prefix)


I think we should also have a test for a prefix containing white spaces.

timoreimann · 2017-05-20T20:13:47Z

middlewares/stripPrefix.go

+		prefix = orgPrefix
+		if !strings.HasSuffix(prefix, "/") {
+			prefix = orgPrefix + "/"
+		}


I think a more elegant and concise way to implement the previous block would be

prefix = strings.TrimSuffix(orgPrefix, "/") + "/"

timoreimann · 2017-05-20T20:14:30Z

middlewares/stripPrefix.go

+		if p := strings.TrimPrefix(r.URL.Path, prefix); len(p) < len(r.URL.Path) {
+			if !strings.HasPrefix(p, "/") {
+				r.URL.Path = "/" + p
+			}


Similarly here:

r.URL.Path = "/" + strings.TrimPrefix(p, "/")

timoreimann · 2017-05-20T20:14:49Z

middlewares/stripPrefix_test.go

+)
+
+func TestStripPrefix(t *testing.T) {
+


No blank line here.

timoreimann · 2017-05-20T20:43:46Z

middlewares/stripPrefix_test.go

+	for _, test := range tests {
+		resp, err := http.Get(server.URL + test.url)
+		if err != nil {
+			t.Fatal(err)


Does err give enough context to understand we ran into an error while sending the GET query?

When in doubt, I'd add a bit of context like

t.Fatalf("failed to send GET request: %s", err)

timoreimann · 2017-05-20T20:44:27Z

middlewares/stripPrefix_test.go

+		}
+		response, err := ioutil.ReadAll(resp.Body)
+		if err != nil {
+			t.Fatal(err)


Context question/comment from above applies here too.

timoreimann · 2017-05-20T20:47:06Z

middlewares/stripPrefix_test.go

+		}
+
+		if test.expectedResponse != string(response) {
+			t.Errorf("Expected '%s' :  '%s'\n", test.expectedResponse, response)


I don't think we need to have a newline in the error string.

timoreimann · 2017-05-20T20:47:32Z

middlewares/stripPrefix_test.go

+		}
+
+		if test.expectedResponse != string(response) {
+			t.Errorf("Expected '%s' :  '%s'\n", test.expectedResponse, response)


In Go, error strings should name the actual value first and expected second. For instance

t.Errorf("got response '%s', expected '%s'", response, test.expectedResponse)

I understand, I was just following the other PathPrefixStripRegex example.

I'll add these fixes shortly.

timoreimann · 2017-05-20T20:56:01Z

middlewares/stripPrefix_test.go

+
+		{url: "/c/api/123/", expectedCode: 200, expectedResponse: "/"},
+		{url: "/c/api/123/test3", expectedCode: 200, expectedResponse: "/test3"},
+		{url: "/c/api/abc/test4", expectedCode: 200, expectedResponse: "/c/api/abc/test4"},


I'm wondering if we need all existing test cases here. For instance, what's the difference between /statand a/api, or /stat/ and /c/api/123/?

While it's not terribly bad to have too many test cases, ideally there should be one representative case per code path only.

timoreimann · 2017-05-20T21:09:33Z

@emilevauge @ldez what do you think, should we have this fix be part of 1.3?

seryl · 2017-05-21T05:43:37Z

@timoreimann Updated with your requested changes.

Let me know if there's anything else we need (rebase, etc).

ldez

Good job 👍

ldez · 2017-05-21T12:24:39Z

middlewares/stripPrefix_test.go

+	}
+
+	suites := []struct {
+		server *httptest.Server


To enhance readability, create 2 types at the below of the file:

type stripPrefixTestSuite struct { server *httptest.Server desc string tests []stripPrefixTestCase } type stripPrefixTestCase struct { url string expectedStatusCode int expectedBody string }

ldez · 2017-05-21T12:27:42Z

middlewares/stripPrefix_test.go

+
+			for _, test := range suite.tests {
+				resp, err := http.Get(suite.server.URL + test.url)
+				if err != nil {


You can use Testify, to enhance readability:

resp, err := http.Get(suite.server.URL + test.url) require.NoError(t, err, "Failed to send GET request") assert.Equal(t, test.expectedStatusCode, resp.StatusCode, "Unexpected status code") if test.expectedBody != "" { response, err := ioutil.ReadAll(resp.Body) require.NoError(t, err, "Failed to read response body") assert.Equal(t, test.expectedBody, string(response), "Unexpected response received") }

Alright, these changes were added as well as your above suggestions.

ldez · 2017-05-21T12:29:13Z

middlewares/stripPrefix_test.go

+			}{
+				{
+					url:      "/norules",
+					expected: "",


for the expressivity of the test, you can drop all expected: ""

ldez · 2017-05-21T12:36:25Z

middlewares/stripPrefix_test.go

+				{
+					url:      "/",
+					expected: "/",
+					code:     http.StatusOK,


can you re-odering? Put code before expected

ldez · 2017-05-21T13:21:57Z

middlewares/stripPrefix_test.go

+		})
+	}
+
+	type stripPrefixTestCase struct {


Please, put this 2 types at the bottom part of the file.

seryl · 2017-05-21T15:01:45Z

Will rebase as soon as we get a thumbs up.

timoreimann

I noticed a few problems with the new approach. See my remarks.

We also seem to have lost test cases verifying that the the first matching prefix will be applied. At the same time, I still think that a few cases are verifying existing behavior redundantly.

I went ahead and put together a proposal on what I think could be a more simple approach while solving the given problems. I pushed it here timoreimann@224f2a1 and would be curious to hear what you think.

Thanks!

timoreimann · 2017-05-22T10:10:40Z

middlewares/stripPrefix_test.go

+}
+
+type stripPrefixTestCase struct {
+	url                string


This should probably be better called path.

timoreimann · 2017-05-22T10:13:07Z

middlewares/stripPrefix_test.go

+			t.Parallel()
+			defer suite.server.Close()
+
+			for _, test := range suite.tests {


The given nested suite/test cases approach comes with a few drawbacks:

Since the description is attached to the suite, we can't see which test case exactly is failing.

For the same reason, we cannot execute individual test cases.

If one test in a suite fails, the entire suite is aborted.

Overall, I also find the nested approach more difficult to understand.

All of this should be solvable by going back to a single, flat hierarchy.

I added what I thought were the right test to begin with.

I ended up added test to validate per request status; tradeoffs are perf. I don't know if we lose anything here to be frank.

I'm definitely okay with the (unique set of) tests you have selected. (In fact, I think you included some which I would have likely forgotten.)

It's just that I think structurally, we're not reaping the benefits of sub-tests with the current approach; notably the points described in the Table-driven tests using subtests section of the introductory blog post.

timoreimann · 2017-05-22T10:15:32Z

middlewares/stripPrefix_test.go

+		stripServers[stripPath] = httptest.NewServer(&StripPrefix{
+			Prefixes: []string{stripPath},
+			Handler:  handlerPath,
+		})


Initializing the servers outside of the parallelized context could be problematic. For instance, if I execute your test like this

go test -count 20 -cpu 1,2,4 -run ^TestStripPrefix$ ./middlewares

I immediately run into too many open files problems on my Mac.

This might not be an issue now but could possibly become one in the future.

Help me understand why you're seeing multiple files opened.

I'm not sure it has anything to do with this at all. Sounds unrelated.

Linux/Unix (which Mac is, technically) represent almost everything in terms of files, including sockets. When I repeated the test 20 times and simulated multiples cores above, a higher number of sockets (and thus file handles) would be consumed concurrently by the test server.

It's perfectly okay to hit the limit at some point because my Mac machine obviously isn't optimized for highly concurrent request throughput. What's interesting though is that with the simpler approach creating one test server per more narrow-scoped test execution routine, I only see the limit when running on 32 or 64 cores.

This really isn't supposed to be a hard blocker on the given approach at all, just an indication on which implementation may be easier on the resource consumption IMHO.

ldez · 2017-05-22T11:07:41Z

@timoreimann your timoreimann@224f2a1 is a very clean way to test ❤️

timoreimann

LGTM. 👍

Thanks for the contribution. This one should make a bunch of folks happy.

ldez

LGTM

I have rebase the PR on the 1.3 branch.

When pushing data to downstream proxies; malformed requests were being sent. The corrected behavior is as follows: | Route Stripped | URL | Passed to Backend | | ----------------- | ---------------------- | ------------------ | | / | / | / | | Route Stripped | URL | Passed to Backend | | ----------------- | ---------------------- | ------------------ | | /stat | /stat | / | | /stat | /stat/ | / | | /stat | /status | /status | | /stat | /stat/us | /us | | Route Stripped | URL | Passed to Backend | | ----------------- | ---------------------- | ------------------ | | /stat/ | /stat | /stat | | /stat/ | /stat/ | / | | /stat/ | /status | /status | | /stat/ | /stat/us | /us | Prior, we could strip the prefixing `/`, and we'd also ignore the case where you want to serve something like `/api` as both the index and as a subpath. Additionally, this should resolve a myriad of issues relating to kubernetes ingress `PathPrefixStrip`.

emilevauge

Thanks very much @seryl !
Awesome contribution, this will help a lot 👏
LGTM

seryl force-pushed the fix-stripprefixpatch-behavior branch from 295c36e to ba24dc2 Compare May 20, 2017 04:52

timoreimann self-requested a review May 20, 2017 06:33

timoreimann added area/middleware kind/bug/fix a bug fix status/1-needs-design-review status/2-needs-review labels May 20, 2017

ldez removed the status/2-needs-review label May 20, 2017

timoreimann suggested changes May 20, 2017

View reviewed changes

seryl force-pushed the fix-stripprefixpatch-behavior branch from e956c3f to fcc4066 Compare May 21, 2017 05:46

ldez suggested changes May 21, 2017

View reviewed changes

seryl force-pushed the fix-stripprefixpatch-behavior branch from 0afb0cb to deb8f5c Compare May 21, 2017 20:40

timoreimann suggested changes May 22, 2017

View reviewed changes

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

seryl force-pushed the fix-stripprefixpatch-behavior branch 2 times, most recently from 024e74a to 536de0b Compare May 22, 2017 22:44

timoreimann approved these changes May 23, 2017

View reviewed changes

ldez added this to the 1.3 milestone May 23, 2017

ldez force-pushed the fix-stripprefixpatch-behavior branch from 536de0b to 7c22d5b Compare May 23, 2017 19:43

ldez changed the base branch from master to v1.3 May 23, 2017 19:43

ldez approved these changes May 23, 2017

View reviewed changes

ldez force-pushed the fix-stripprefixpatch-behavior branch from 7c22d5b to 635cb8d Compare May 23, 2017 21:37

timoreimann mentioned this pull request May 23, 2017

Get testify/require dependency. #1658

Merged

ldez force-pushed the fix-stripprefixpatch-behavior branch from 635cb8d to f814499 Compare May 23, 2017 22:56

timoreimann requested a review from emilevauge May 24, 2017 05:48

emilevauge approved these changes May 24, 2017

View reviewed changes

emilevauge merged commit bc01218 into traefik:v1.3 May 24, 2017

seryl deleted the fix-stripprefixpatch-behavior branch May 24, 2017 09:09

ldez added area/rules and removed contributor/waiting-for-corrections status/2-needs-review labels May 24, 2017

ldez mentioned this pull request Jun 12, 2017

Error when requesting an url with double slashes, right after hostname #1323

Closed

devster31 mentioned this pull request Feb 25, 2019

trailing slash redirect still necessary #4521

Closed

mylesagray mentioned this pull request Jul 25, 2019

v2.0 Request - Fix trailing slash redirection once and for all. #5159

Closed

Fix behavior for PathPrefixStrip #1638

Fix behavior for PathPrefixStrip #1638

Conversation

seryl commented May 20, 2017 • edited Loading

Description

seryl commented May 20, 2017

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

timoreimann commented May 20, 2017

seryl commented May 21, 2017

ldez 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

ldez May 21, 2017 • edited Loading

Choose a reason for hiding this comment

seryl commented May 21, 2017

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann May 22, 2017 • edited Loading

Choose a reason for hiding this comment

seryl May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez commented May 22, 2017

timoreimann left a comment

Choose a reason for hiding this comment

ldez left a comment

Choose a reason for hiding this comment

emilevauge left a comment

Choose a reason for hiding this comment

seryl commented May 20, 2017 •

edited

Loading

ldez May 21, 2017 •

edited

Loading

timoreimann May 22, 2017 •

edited

Loading

timoreimann May 22, 2017 •

edited

Loading

seryl May 22, 2017 •

edited

Loading