fix(router) fix trailing slash getting automatically appended #2315

bungle · 2017-03-31T19:59:46Z

Summary

Trailing slashes get automatically appended to proxied uris and sometimes double (//) slashes. This PR is meant to follow the original request uri semantics regarding the trailing slash, and forward it as is the proxied service. Making the proxying more transparent.

Revert some of the changes introduced in #2115.

Issues resolved

Fix: #2211

thibaultcha

Apart from the few style issues, the biggest blocker is moving the tests to where they belong imho (the unit suite where some already exists for this URI behavior). Additionally, I believe a small perf improvement could be done as well. It's all in the comments! :)

thibaultcha · 2017-04-04T01:50:52Z

kong/core/router.lua

+    if upstream_path then
+      if new_uri == "/" then
+        new_uri = upstream_path
+      else


style: could we add the missing line jump before that else?

I can do that, is it so that we are basically using new style on new code (or changes) even if the rest of the file is in other style?

thibaultcha · 2017-04-04T01:57:44Z

kong/core/router.lua

+    if new_uri_slash and not req_uri_slash and new_uri ~= "/" then
+      new_uri = sub(new_uri, 1, -2)
+    elseif not new_uri_slash and req_uri_slash and uri ~= "/" then
+      new_uri = new_uri .. "/"


Could we enclose this whole block in such a condition:

if new_uri ~= "/" then local req_uri_slash = local new_uri_slash = -- ... end

It seems like this is not necessary in such a case, and additionally, we avoid repeating the and new_uri ~= "/" condition (which apparently has a typo on L.618 btw?).

It is not a typo, so this cannot be done.

thibaultcha · 2017-04-04T01:58:10Z

kong/core/router.lua

+    local new_uri_slash = sub(new_uri, -1) == "/"
+    if new_uri_slash and not req_uri_slash and new_uri ~= "/" then
+      new_uri = sub(new_uri, 1, -2)
+    elseif not new_uri_slash and req_uri_slash and uri ~= "/" then


style: missing line jump prior to this elseif statement

thibaultcha · 2017-04-04T01:59:29Z

kong/core/router.lua

+
+    local req_uri_slash = sub(uri,     -1) == "/"
+    local new_uri_slash = sub(new_uri, -1) == "/"
+    if new_uri_slash and not req_uri_slash and new_uri ~= "/" then


style: a line jump before such statements if complex is also preferred for readability. The only exception is for testing nil out of a recently retrieved value, such as:

local upstream_path = ... if upstream_path then end

or the famous:

local ok, err = thing() if not ok then end

ok, i will make a change.

maybe add one final commit with all the style changes, updating the entire file?

@Tieske I once asked @coopr, and he said that we don't intently have boy scout practices in place. And I believe I will end down to a rabbit hole with this patch, if I try. As the style guide is for me mostly a fuzzy logic with many many exceptions like the above. Even the above comment makes me think, what is "complex".

E.g. is a function call with many arguments a complex?

local api_t = find_api(method, uri, req_host)

What about:

local api_t = find_api(method, uri, { something = 1 })

(is that still simple?)

I think @thibaultcha meant

local req_uri_slash = sub(uri, -1) == "/" local new_uri_slash = sub(new_uri, -1) == "/" if new_uri_slash and not req_uri_slash and new_uri ~= "/" then

just a line jump, to make it visually easier to read

thibaultcha · 2017-04-04T02:20:09Z

spec/02-integration/05-proxy/01-router_spec.lua

+      it(config .. " is not appended to upstream uri " .. args[1] .. space .. " when requesting " .. args[2],
+        check(unpack(args)))
+    end
+  end)


I feel like this test suite is quite complicated to maintain. Maybe more that just "quite", even.

Couldn't we come up with something more descriptive, like:

local checks = { { api = { uris = { "/", "/foo/bar" }, upstream_url = "http://localhost:9999/", }, -- request_uri expected_upstream_uri { "/", "/" }, { "/foo/", "/foo/" }, }, { api = { uris = { "/foo/bar/" }, upstream_url = "http://localhost:9999/foo", }, -- request_uri expected_upstream_uri { "/foo/", "/foo/" }, { "/foo", "/foo" }, } }

(with better alignment of the comments of course).

Also, let's note that an API with uris=/,/foo/bar does not really make sense in those tests since we are focusing on post-matching behavior. We should probably only declare APIs with uris=/foo/bar or uris=/foo/bar/ with both strip_uri=true|false for the sake of simplicity?

Structuring the tests this way would have the benefits of:

grouping APIs declaration, input values and expected outputs together.

adding a comment header to each value of the table that has declarative tests input/output values, so we know what they refer to.

avoid the i = i + n logic in the check() closure, which is asks for a lot of cognitive load when debugging.

we can still build a nice loop that generates tests and messages as already done

it's easier to separate the use cases per API in the the checks table (currently they are all grouped together and we don't know which test belongs to which API without reading the check() closure.

Additionally, I strongly believe those tests should be part of the 01-unit test suite, as per the router tests. They should most probably be moved in the router_spec tests over there, which already mocks the ngx object and allows for testing of the newly set URI (as currently already tested there). Since this feature extends the same part of the code, the tests should be over there. They will also be much faster, and avoid the need for extending the custom_nginx.template file (which was a quick fix but should be avoided as much as possible for now).

Just a quick note, router specs are not going through the whole pipeline to the API/service, hence it really does not test that slash at the end really behaves correctly all the way to the end. Router is just a one component in this equation. There are Nginx configs, proxy etc. in addition.

thibaultcha · 2017-04-04T15:28:44Z

(The router has always been written with the new code style)

thibaultcha · 2017-04-04T15:37:46Z

I don't think any of those components are touching the URI. Testing with unit tests is a huge benefit for us because of the speed. Ideally this should be tested in both places. Modifying the router is dangerous and be able to do so and simply progress with the quick unit tests is very valuable, hence why the router is mainly tested with unit tests.

…

On Apr 4, 2017, at 2:32 AM, Aapo Talvensaari ***@***.***> wrote: @bungle commented on this pull request. In spec/02-integration/05-proxy/01-router_spec.lua: > + for _, args in ipairs(checks) do + + local config = "(strip_uri = n/a)" + + if args[4] == true then + config = "(strip_uri = on) " + elseif args[4] == false then + config = "(strip_uri = off)" + end + + local space = string.sub(args[1], -1) == "/" and "" or " " + + it(config .. " is not appended to upstream uri " .. args[1] .. space .. " when requesting " .. args[2], + check(unpack(args))) + end + end) Just a quick note, router specs are not going through the whole pipeline to the API/service, hence it really does not test that slash at the end really behaves correctly all the way to the end. Router is just a one component in this equation. There are Nginx configs, proxy etc. in addition. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

bungle · 2017-04-04T16:22:02Z

On my machine the added tests run in 250 ms, while the router tests in total take 11,570 ms.

thibaultcha · 2017-04-04T16:45:17Z

Regardless, such tests belong to the router unit tests considering they are the tests for this component and all of the other features of the router (except this one) are being tested there.

thibaultcha · 2017-04-04T16:47:04Z

Not doing so would make future contributors having to run both tests suites now. It also is slower to iterate with it in the sense that the integration test suite needs a running DB, a running Kong instance, migrations to be run before the test, etc...

bungle · 2017-04-04T17:56:40Z

Tests added to unit test suite as well.

Fixes: #2211

…xtensive testing suite Triggers #2334 (01-router_spec.lua)

bungle · 2017-04-05T14:48:50Z

@thibaultcha

The latest commit:
62e0091

has performance tunings
enhances readability of router.lua
has more extensive testing suite

But, it will also trigger a bug in a router, possibly related to #2334.

It is already done, as requested.

thibaultcha · 2017-04-07T19:55:11Z

Very cool! I just merged this in master for a 0.10.2 release, with minor changes in the commit messages and a few conflicts fixed. Thank you @bungle!

bungle mentioned this pull request Mar 31, 2017

trailing slashes appended to requests to root of a configured API #2211

Closed

bungle added area/router labels Mar 31, 2017

tpb-dev approved these changes Mar 31, 2017

View reviewed changes

bungle force-pushed the fix/trailing-slashes branch 2 times, most recently from dc46fd7 to 29f172b Compare April 3, 2017 09:36

Tieske added this to the 0.10.2 milestone Apr 3, 2017

thibaultcha previously requested changes Apr 4, 2017

View reviewed changes

thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/needs review labels Apr 4, 2017

bungle force-pushed the fix/trailing-slashes branch 4 times, most recently from 6822d93 to 1045892 Compare April 4, 2017 13:40

bungle added pr/status/needs review and removed pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. labels Apr 4, 2017

bungle force-pushed the fix/trailing-slashes branch from 1045892 to 7ba162c Compare April 4, 2017 17:55

fix(router) fix trailing slash getting automatically appended

54ce75d

Fixes: #2211

bungle force-pushed the fix/trailing-slashes branch from 7ba162c to 54ce75d Compare April 4, 2017 18:16

Tieske mentioned this pull request Apr 5, 2017

Kong 0.10.0 #2340

Closed

performance enhancements, style modifications (on router.lua), more e…

62e0091

…xtensive testing suite Triggers #2334 (01-router_spec.lua)

bungle mentioned this pull request Apr 5, 2017

Router fails to resolve when longer path shadows another #2334

Closed

bungle mentioned this pull request Apr 7, 2017

fix(router) handle a matching edge-case with uris #2343

Merged

thibaultcha closed this Apr 7, 2017

thibaultcha deleted the fix/trailing-slashes branch April 7, 2017 19:55

avihaibd mentioned this pull request Dec 3, 2017

Kong redirects a request to the upstream although the url does not match the registered api #3056

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(router) fix trailing slash getting automatically appended #2315

fix(router) fix trailing slash getting automatically appended #2315

bungle commented Mar 31, 2017

thibaultcha left a comment •

edited

Loading

thibaultcha Apr 4, 2017

bungle Apr 4, 2017

thibaultcha Apr 4, 2017

bungle Apr 4, 2017 •

edited

Loading

thibaultcha Apr 4, 2017

bungle Apr 4, 2017

thibaultcha Apr 4, 2017

bungle Apr 4, 2017

Tieske Apr 4, 2017

bungle Apr 4, 2017 •

edited

Loading

Tieske Apr 4, 2017

thibaultcha Apr 4, 2017 •

edited

Loading

bungle Apr 4, 2017

thibaultcha commented Apr 4, 2017

thibaultcha commented Apr 4, 2017 via email

bungle commented Apr 4, 2017 •

edited

Loading

thibaultcha commented Apr 4, 2017

thibaultcha commented Apr 4, 2017

bungle commented Apr 4, 2017

bungle commented Apr 5, 2017

thibaultcha commented Apr 7, 2017

fix(router) fix trailing slash getting automatically appended #2315

fix(router) fix trailing slash getting automatically appended #2315

Conversation

bungle commented Mar 31, 2017

Summary

Issues resolved

thibaultcha left a comment • edited Loading

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

bungle Apr 4, 2017 • edited Loading

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

bungle Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha commented Apr 4, 2017

thibaultcha commented Apr 4, 2017 via email

bungle commented Apr 4, 2017 • edited Loading

thibaultcha commented Apr 4, 2017

thibaultcha commented Apr 4, 2017

bungle commented Apr 4, 2017

bungle commented Apr 5, 2017

thibaultcha commented Apr 7, 2017

thibaultcha left a comment •

edited

Loading

bungle Apr 4, 2017 •

edited

Loading

bungle Apr 4, 2017 •

edited

Loading

thibaultcha Apr 4, 2017 •

edited

Loading

bungle commented Apr 4, 2017 •

edited

Loading