fix(router) ensure transparent proxying of URIs #2519

thibaultcha · 2017-05-13T01:58:03Z

Summary

Since ngx.req.set_uri will make Nginx's proxying escape the upstream URI, we do not use this API anymore. As it was the case prior to Kong 0.10, we go back to inlining an Nginx variable containing the desired, plain URI, and thus ensure Nginx will not unconditionally percent-encode before proxying.

Full Changelog

The router now returns a 4th value: the URI to use for the upstream
call.
The proxy_pass directive now inlines a new $upstream_uri variable
which contains a raw string with both the Nginx $request_uri and
$args variables.
Update the router value return test
New test to ensure we proxy the request's querystring
New regression test to ensure we do not double percent-encode request
URIs.
simplify URI stripping logic

Issues resolved

Fix #2512

p0pr0ck5 · 2017-06-07T01:09:19Z

kong/core/handler.lua

+
+      if uri_args then
+        uri = uri .. "?" .. uri_args
+      end


rather than the string concatentation here, how do we feel about defining the proxy_pass directive as

proxy_pass $upstream_scheme://kong_upstream$upstream_url$is_args$args;

i dont have strong feels one way or another, just wondering about idiomatic approach and performance (ngx.var access)

p0pr0ck5 · 2017-06-07T01:10:37Z

kong/core/router.lua

@@ -659,16 +660,12 @@ function _M.new(apis)
      return nil
    end

-    local new_uri
-    local uri_root = uri == "/"
+    local uri_root = request_uri == "/"


some of this logic feels ever-so-slightly hand wavey. some comments here about what's going on would be useful, particularly given the subtlety and complexity of the bug resolved here.

All these seem just to be variable renames, and that's it.

yes, i spose this comment was in reference to the desire to have some more commentary on this module in general. certainly not a merge blocker though :)

whups, wrong button, didnt mean to approve. LGTM overall though

bungle · 2017-06-07T08:03:00Z

This looks good to me (except that one $is_args change that I would like to see). I know this idea pretty well as I proposed it as a second approach to fix this. It was originally shot down because of worries if the plugins modify the uri arguments (and those get screwed, or overwritten, or encodings get bad). It looks like it is taken care in setting the final variable before proxying AND after the plugins have done their access handlers.

bungle · 2017-06-07T08:13:07Z

What this will break are the plugins that call ngx.req.set_uri in their access handlers (or does it?). But it looks like they have ngx.ctx.uri now.

thibaultcha · 2017-06-07T16:59:33Z

Well, this is just a revert back to the implementation we used all along 0.1 to 0.9, and that was only changed in 0.10, nothing groundbreaking. It just handles the URI arguments slightly better, which was one of the original concerns, but there was another big one. The reason I changed it in 0.10 was to avoid the Nginx variable, and benefit from a somewhat nicer encapsulation from ngx.req.set_uri(), which feels nicer to use from a plugin than setting a ctx variable. This is mitigated since as soon as we have the plugins API, we will bring back this encapsulation by creating our own kong.set_uri() API.

I am not sure what the benefit of using multiple Nginx variables could be. Performance because of ngx.var access: we would actually trigger 3 such ngx.var accesses instead of 1. GC seems to be about the same, or slightly better in the current approach even. Nginx variables also have their own cost on the Nginx side, and creating those variables would triple such overhead.

bungle · 2017-06-08T00:20:59Z

The $is_args in Nginx is mainly because if there wasn't such you had to use e.g. ?$args and that means that in some cases (empty querystring) you would still append ? even if the original request didn't have it. Which reminds me that do you have a test for that?

when original query had ? at the end without actual querystring, are we still proxying with ?
when original query didn't have ? are we only adding it in case where Kong introduces new querystring parameters

I'm pretty sure 2. is handled, but whatabout that 1.?

Other than that, I agree, we don't need to have $is_args here even if it looks familiar to Nginx users.

thibaultcha · 2017-06-08T01:20:57Z

@bungle Good point, I will add tests for 1. and 2. The later is handled, the former, I am less sure.

p0pr0ck5 · 2017-06-08T21:37:47Z

(relabelling given the notes above ^)

thibaultcha · 2017-06-08T21:42:22Z

Yep, thanks! Actually implementing those tests.

thibaultcha · 2017-06-08T22:02:32Z

Pushed a new version with a test for 1. Tests for 2. already exist in the form of the request-transformer plugin.

p0pr0ck5 · 2017-06-08T22:29:49Z

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

+      assert.equal("world", json.args.hello)
+    end)
+
+    it("does not proxy '$is_args' if URI does not contain arguments", function()


my only comment here is that stripping the ? char from the query string when there are no args does not align with our desire to be as transparent as possible in proxying the request. we could build our own version of is_args that examines the state of args and the downstream query, and act appropriate. or we could not because i think we all just want this to go away ;)

Hmm yeah I did think about it too, and wanted to explain why not, but I forgot.

In my thinking, as much as we want transparent proxying to happen (and you know how much I pushed for this), I just cannot, for the life of me, think of a use case for why someone would want to send a request with an empty querystring /foo?. Hence, I did not take care of this and simply judged the Nginx behavior to be acceptable in this case. If we ever receive such a requirement from a use-case, beer is on me!

So, i did find one case where someone noted that the presence of ? was semantically significant (granted, not our project or even our lang): psf/requests#2912

In addition, RFC 3986 1.2.3 points out:

The generic syntax uses the slash ("/"), question mark ("?"), and number sign ("#") characters to delimit components that are significant to the generic parser's hierarchical interpretation of an identifier.

And from section 6.2.3:

Normalization should not remove delimiters when their associated component is empty unless licensed to do so by the scheme specification... the presence or absence of delimiters within a userinfo subcomponent is usually significant to its interpretation.

If we ever receive such a requirement from a use-case, beer is on me!

http://beerme.pointlessmicroservic.es

That 6.2.3 section is quite relevant indeed. Now I kind of want to take care of this use-case actually... Thoughts?

This originally started because Nginx $uri is a bad normalization semantics-wise. We changes to $request_uri only to find out that decoding and encoding doesn't work 1:1. Thats why we will skip that with this patch, but I do think we can do better here. Following what Nginx does here doesn't feel right.

@bungle under "Normalizations that change semantics"

Removing the "?" when the query is empty. When the query is empty, there may be no need for the "?". Example: http://www.example.com/display? → http://www.example.com/display

The point here is that we want to avoid changing semantic.

Something like this patch resolves the issue from what I can tell (hastily whipped together): https://gist.github.com/p0pr0ck5/e742a478a8bfcaeabeefddafd54af947

However, the test in question fails, assumedly because of upstream normalization (both httpbin and mockbin strip ? when no query args are present). tshark proof: https://gist.github.com/p0pr0ck5/d19fb99c88ed5e2b1efdc53707912bb1#file-gistfile1-txt-L375

but I do think we can do better here.

Yep, let's do it then. Updating the patch

@p0pr0ck5 The patch would work, but would also add extra calls to ngx.var, so at this point, I think we should group the querystring in upstream_uri as the original implementation of this patch.

New patch is up!

thibaultcha · 2017-06-09T00:55:51Z

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

+
+      local body = assert.res_status(200, res)
+      local json = cjson.decode(body)
+      assert.matches("/get%?$", json.vars.request_uri)


We test this behavior with our custom template since both httpbin and mockbin strip the querystring delimiter if it is empty.

p0pr0ck5

LGTM, wouldnt mind seeing a few comments clarified but not a blocker for merge at this point

p0pr0ck5 · 2017-06-09T16:07:44Z

kong/core/handler.lua

+        -- `/foo?` is to keep `$is_args` an empty string, hence effectively
+        -- stripping the empty querystring.
+        -- We overcome this behavior with our own logic, to prevent user
+        -- desired semantics.


"to prevent user desired semantics" doesnt quite make sense to me

I meant "preserve"

p0pr0ck5 · 2017-06-09T16:09:05Z

kong/core/handler.lua

+      -- `uri` is the URI with which to call upstream, as returned by the
+      -- router, which might have truncated it (`strip_uri`).
+      --
+      -- If set `host_header` is the original header to be preserved.


the alignment of the comments can be a bit confusing; might me sense to have

-- comment upstream_uri
var.upstream_uri = uri

-- comment about host_headr
var.upstream_host = ...

I like grouping multiple var or ctx assignments together, as is done multiple times in that function

* The router now returns a 4th value: the URI to use for the upstream call. * The `proxy_pass` directive now inlines a new `$upstream_uri` variable which contains a raw string with both the Nginx `$request_uri` and `$args` variables. * Update the router value return test * New test to ensure we proxy the request's querystring * New regression test to ensure we do not double percent-encode request URIs. Fix #2512

thibaultcha · 2017-06-09T17:58:30Z

Rebuilt the history since the refactor commit accidentally got some diff from the fix one. + clarified comments

thibaultcha added the pr/status/do not merge (to discuss) label May 13, 2017

thibaultcha mentioned this pull request May 13, 2017

Kong is adding URL encoding before sending upstream #2512

Closed

thibaultcha force-pushed the fix/router-double-encoded-uris branch from 54000c0 to abbb128 Compare May 13, 2017 02:05

thibaultcha self-assigned this May 30, 2017

thibaultcha force-pushed the fix/router-double-encoded-uris branch 2 times, most recently from 7754e78 to cf9156a Compare June 6, 2017 21:05

thibaultcha changed the title ~~tests(router) new regression test for double percent-encoding of upstream URLs~~ fix(router) ensure transparent proxying of URIs Jun 6, 2017

thibaultcha added pr/status/please review and removed pr/status/do not merge (to discuss) labels Jun 6, 2017

thibaultcha force-pushed the fix/router-double-encoded-uris branch 3 times, most recently from a980e6f to 3b6c05f Compare June 6, 2017 21:37

p0pr0ck5 previously approved these changes Jun 7, 2017

View reviewed changes

p0pr0ck5 mentioned this pull request Jun 8, 2017

Memory leak or overly-agressive consumption #2194

Closed

p0pr0ck5 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/please review labels Jun 8, 2017

thibaultcha force-pushed the fix/router-double-encoded-uris branch 2 times, most recently from 3dcb7a6 to d85843c Compare June 8, 2017 22:01

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

p0pr0ck5 reviewed Jun 8, 2017

View reviewed changes

thibaultcha force-pushed the fix/router-double-encoded-uris branch 2 times, most recently from d48fb66 to 7192697 Compare June 9, 2017 00:54

thibaultcha commented Jun 9, 2017

View reviewed changes

p0pr0ck5 approved these changes Jun 9, 2017

View reviewed changes

thibaultcha added 2 commits June 9, 2017 10:48

refactor(router) simplify URI stripping logic

14f6297

thibaultcha force-pushed the fix/router-double-encoded-uris branch from 7192697 to 14f6297 Compare June 9, 2017 17:49

thibaultcha merged commit 8ca38dc into master Jun 9, 2017

thibaultcha deleted the fix/router-double-encoded-uris branch June 9, 2017 19:01

thibaultcha mentioned this pull request Aug 5, 2017

Using ngx.req.set_uri from :access does not work on master as it does on 0.10.3 #2779

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(router) ensure transparent proxying of URIs #2519

fix(router) ensure transparent proxying of URIs #2519

thibaultcha commented May 13, 2017 •

edited

Loading

p0pr0ck5 Jun 7, 2017

p0pr0ck5 Jun 7, 2017

bungle Jun 7, 2017 •

edited

Loading

p0pr0ck5 Jun 7, 2017

bungle commented Jun 7, 2017 •

edited

Loading

bungle commented Jun 7, 2017 •

edited

Loading

thibaultcha commented Jun 7, 2017 •

edited

Loading

bungle commented Jun 8, 2017 •

edited

Loading

thibaultcha commented Jun 8, 2017

p0pr0ck5 commented Jun 8, 2017

thibaultcha commented Jun 8, 2017

thibaultcha commented Jun 8, 2017

p0pr0ck5 Jun 8, 2017

thibaultcha Jun 8, 2017 •

edited

Loading

p0pr0ck5 Jun 8, 2017

thibaultcha Jun 8, 2017

bungle Jun 8, 2017

p0pr0ck5 Jun 8, 2017

p0pr0ck5 Jun 8, 2017 •

edited

Loading

thibaultcha Jun 8, 2017

thibaultcha Jun 8, 2017

thibaultcha Jun 8, 2017

thibaultcha Jun 9, 2017

p0pr0ck5 left a comment

p0pr0ck5 Jun 9, 2017

thibaultcha Jun 9, 2017

p0pr0ck5 Jun 9, 2017

thibaultcha Jun 9, 2017

thibaultcha commented Jun 9, 2017

fix(router) ensure transparent proxying of URIs #2519

fix(router) ensure transparent proxying of URIs #2519

Conversation

thibaultcha commented May 13, 2017 • edited Loading

Summary

Full Changelog

Issues resolved

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bungle Jun 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bungle commented Jun 7, 2017 • edited Loading

bungle commented Jun 7, 2017 • edited Loading

thibaultcha commented Jun 7, 2017 • edited Loading

bungle commented Jun 8, 2017 • edited Loading

thibaultcha commented Jun 8, 2017

p0pr0ck5 commented Jun 8, 2017

thibaultcha commented Jun 8, 2017

thibaultcha commented Jun 8, 2017

Choose a reason for hiding this comment

thibaultcha Jun 8, 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

p0pr0ck5 Jun 8, 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

p0pr0ck5 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

thibaultcha commented Jun 9, 2017

thibaultcha commented May 13, 2017 •

edited

Loading

bungle Jun 7, 2017 •

edited

Loading

bungle commented Jun 7, 2017 •

edited

Loading

bungle commented Jun 7, 2017 •

edited

Loading

thibaultcha commented Jun 7, 2017 •

edited

Loading

bungle commented Jun 8, 2017 •

edited

Loading

thibaultcha Jun 8, 2017 •

edited

Loading

p0pr0ck5 Jun 8, 2017 •

edited

Loading