fix(proxy): host header including port #2045

Tieske · 2017-02-03T11:05:19Z

If the host header is updated to include the port, then what is the exact desired behaviour?

So if preserve_host is set on the api definition, we do not touch the header at all? or do we still inject a default port if it was not in the received host header?

If we don't preserve the header, we could simply insert the upstream host and port.

subnetmarco · 2017-02-03T18:36:28Z

So if preserve_host is set on the api definition, we do not touch the header at all?

We shouldn't touch it, and preserve the original Host sent by the client.

thibaultcha · 2017-02-03T21:13:22Z

kong/templates/nginx_kong.lua

@@ -103,7 +103,7 @@ server {
        proxy_set_header X-Real-IP $remote_addr;
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
        proxy_set_header X-Forwarded-Proto $scheme;
-        proxy_set_header Host $upstream_host;
+        proxy_set_header Host $upstream_host:$upstream_port;


According to the HTTP specs, the port really is part of the Host value if it is not the default port being requested for a service. This port could very well be included in the $upstream_host variable on the Lua land, to avoid dispersing that logic. Besides and to highlight that fact, this new variable is currently undeclared.

Especially since it is not required for the port to be present if it is the default one for the service being requested, and we would have more control over that from the Lua land if we wish to perform such changes.

thibaultcha · 2017-02-03T21:14:24Z

So if preserve_host is set on the api definition, we do not touch the header at all?

We shouldn't touch it, and preserve the original Host sent by the client.

Yes.

Tieske · 2017-02-06T18:52:10Z

There seems to be a bit more detail to it;

The router matches on 'host-header', not on hostname. So to the router myhost.com and myhost.com:80 are two distinct entries. But the api schema does not allow a port number in its hosts field.

thibaultcha

The main issues I have with this is how it makes the proxy retrieve the request 's headers on every call instead of doing it only when necessary, and the weirdness of the router's API now that it lost some responsibilities (see comment).

thibaultcha · 2017-02-06T19:30:21Z

kong/core/handler.lua

@@ -79,6 +79,8 @@ return {

      ctx.KONG_ACCESS_START = get_now()

+      local original_host_header = ngx.req.get_headers().host


We want to avoid retrieving the headers if not necessary. This change will cancel that behavior.

all the logic requires the hostname only, so an extra field would have to be returned, the original host-header (incl. any port info). I see what you mean, but don't see a clean solution.

thibaultcha · 2017-02-06T19:30:53Z

kong/core/handler.lua

@@ -117,14 +120,13 @@ return {
          "' with: "..tostring(err))
      end

-      if balancer_address.hostname and not api.preserve_host then
-        var.upstream_host = balancer_address.hostname
-


This line-jump was intended.

thibaultcha · 2017-02-06T21:41:56Z

kong/core/router.lua

-      end
-
-      upstream_host = headers["host"]
-    end


There is a test in the unit tests that currently ensures this behavior is taken care of by exec. It it now failing.

thibaultcha · 2017-02-06T21:42:51Z

kong/core/balancer.lua

@@ -309,6 +310,7 @@ local function execute(target)

  target.ip = ip
  target.port = port
+  target.hostname = target.host


Why not using host or rename host to hostname? They seem to ultimately be identical even though the comments in the balancer_address structure seems to say otherwise.

host is the hostname extracted from the upstream url (and might be an upstream-name)
hostname is the name that matches the final ip (never an upstream-name)
The original host field must be retained in case of a retry, whereas hostname might get a different value for each try.

whereas hostname might get a different value for each try.

Where is that bit happening then?

On retries, the balancer is called again: https://github.com/Mashape/kong/blob/189628b28c672886b1d74de9bf1857c1c7a80719/kong/kong.lua#L213-L222

thibaultcha · 2017-02-06T21:47:55Z

kong/core/router.lua

@@ -493,6 +493,9 @@ function _M.new(apis)


    local host = headers["Host"] or headers["host"]
+    if host then
+      host = host:match("^([^:]+)") -- strip port number if given


Better using match(host, pattern) in such a hot code path. Even better is to use ngx_pcre, to allow this bit to be JIT-compiled.

thibaultcha · 2017-02-06T21:55:42Z

kong/core/router.lua

    if ngx.var.http_kong_debug then
      ngx.header["Kong-Api-Name"] = api_t.api.name
    end


-    return api_t.api, api_t.upstream_scheme, upstream_host, api_t.upstream_port
+    return api_t.api, api_t.upstream_scheme, api_t.upstream_host, api_t.upstream_port


At the time of writing, the goal of this API was to return everything needed in order for Kong to forward upstream. Now with the balancing bit in between and the removal of the preserve_host logic from this module, it seems like this is un-necessarily bloated.

We should either continue encapsulating such logic in this module (might not be possible since we now need to the port from the DNS resolution to build the definitive upstream Host header), or, simply return the Kong API (api_t.api) and api_t itself (which has all the necessary properties, but exposes some underlying router implementation which we don't really want).

We should return a table containing the final api + eventually another table containing scheme, host and port, or include those properties in the 1st table being returned. Those are minor modifications.

considering this is all internal implementation I'd opt for returning api_t

at least over returning yet another table

For now I kept the list and added the host_header, seemed cleanest to me. All header fetching, including checks for preserve_host are back in the router.

Tieske · 2017-02-07T14:19:01Z

@thibaultcha imo good to go now.

Tieske · 2017-02-07T14:36:01Z

fixes #1347

thibaultcha · 2017-02-07T19:14:47Z

kong/core/handler.lua

-      if balancer_address.hostname and not api.preserve_host then
-        var.upstream_host = balancer_address.hostname
+      -- if set `host_header` is the original header to be preserved
+      var.upstream_host = host_header or 


I more and more find this syntax to be harder to read. In the future we should stick to:

if host_header then var.upstream_host = host_header else var.upstream_host = balancer_address.hostname..":"..balancer_address.port end

nitpick not using else is a more defensive programming practice

var.upstream_host = balancer_address.hostname..":"..balancer_address.port if host_header then var.upstream_host = host_header end

reference: https://github.com/TheLadders/object-calisthenics#rule-2-dont-use-the-else-keyword

thibaultcha · 2017-02-07T19:17:56Z

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

+      assert.equal("preserved.com:80", json.headers.Host)
+    end)
+
+    pending("preserves downstream host+non_default_port if enabled", function()


Why are those pending?

as per comment: httpbin does not return the host header, but a parsed version with the port stripped, hence cannot test it...

thibaultcha · 2017-02-08T17:25:08Z

I'm sorry but I don't think this even applies here. We are light years away from the examples and proposals given by the author, and the `or` pattern is still a branch, but more confusing to read. The way I understand it, this is just an exercice, and certainly not a "current best practices". We also do not have an OOP code base. Plus the fact that the author is actually *cheering* for OOP... Hum, sorry.

…

On Feb 8, 2017, at 6:41 AM, Colin Hutchinson ***@***.***> wrote: @hutchic commented on this pull request. In kong/core/handler.lua: > @@ -117,14 +119,12 @@ return { "' with: "..tostring(err)) end - if balancer_address.hostname and not api.preserve_host then - var.upstream_host = balancer_address.hostname + -- if set `host_header` is the original header to be preserved + var.upstream_host = host_header or nitpick not using else is a more defensive programming practice var.upstream_host = balancer_address.hostname..":"..balancer_address.port if host_header then var.upstream_host = host_header end reference: https://github.com/TheLadders/object-calisthenics#rule-2-dont-use-the-else-keyword — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

thibaultcha · 2017-02-08T17:32:04Z

We want to avoid a double assignment, and take advantage of branch prediction here. Hence, else. Better make it readable than not, at least, is my point.

…

On Feb 8, 2017, at 6:41 AM, Colin Hutchinson ***@***.***> wrote: @hutchic commented on this pull request. In kong/core/handler.lua: > @@ -117,14 +119,12 @@ return { "' with: "..tostring(err)) end - if balancer_address.hostname and not api.preserve_host then - var.upstream_host = balancer_address.hostname + -- if set `host_header` is the original header to be preserved + var.upstream_host = host_header or nitpick not using else is a more defensive programming practice var.upstream_host = balancer_address.hostname..":"..balancer_address.port if host_header then var.upstream_host = host_header end reference: https://github.com/TheLadders/object-calisthenics#rule-2-dont-use-the-else-keyword — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub, or mute the thread.

thibaultcha · 2017-02-08T17:42:32Z

That said, the case for not using else is made. Not that it wasn't clear before though.

…

On Feb 8, 2017, at 9:31 AM, Thibault Charbonnier ***@***.***> wrote: We want to avoid a double assignment, and take advantage of branch prediction here. Hence, else. Better make it readable than not, at least, is my point. > On Feb 8, 2017, at 6:41 AM, Colin Hutchinson ***@***.***> wrote: > > @hutchic commented on this pull request. > > In kong/core/handler.lua: > > > @@ -117,14 +119,12 @@ return { > "' with: "..tostring(err)) > end > > - if balancer_address.hostname and not api.preserve_host then > - var.upstream_host = balancer_address.hostname > + -- if set `host_header` is the original header to be preserved > + var.upstream_host = host_header or > nitpick not using else is a more defensive programming practice > > var.upstream_host = balancer_address.hostname..":"..balancer_address.port > if host_header then > var.upstream_host = host_header > end > reference: https://github.com/TheLadders/object-calisthenics#rule-2-dont-use-the-else-keyword > > — > You are receiving this because you modified the open/close state. > Reply to this email directly, view it on GitHub, or mute the thread. >

hutchic · 2017-02-08T18:44:02Z

yeah I wasn't sure how applicable OOP calisthenics were. I figure lua has polymorphism so majority of them should still work.

but like I mentioned it's a nitpick (or bike shed if you prefer) :)

add port to host header upstream

390c6be

Tieske added the pr/wip A work in progress PR opened to receive feedback label Feb 3, 2017

Tieske self-assigned this Feb 3, 2017

thibaultcha requested changes Feb 3, 2017

View reviewed changes

rewrote fix

444eefc

thibaultcha requested changes Feb 6, 2017

View reviewed changes

Tieske added 2 commits February 7, 2017 11:48

get headers only once, added unit tests

7f1618b

whitespace

2fde0ec

Tieske added pr/status/needs review and removed pr/wip A work in progress PR opened to receive feedback labels Feb 7, 2017

Tieske added this to the 0.10 RC milestone Feb 7, 2017

thibaultcha reviewed Feb 7, 2017

View reviewed changes

thibaultcha merged commit 37e8ed3 into release/0.10.0 Feb 7, 2017

thibaultcha deleted the fix/proxy-port branch February 7, 2017 19:43

Tieske mentioned this pull request Feb 10, 2017

Port number is stripped from Host http header. #1347

Closed

Tieske mentioned this pull request Feb 28, 2017

New api endpoint returning 'no API found with those values' #2107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(proxy): host header including port #2045

fix(proxy): host header including port #2045

Tieske commented Feb 3, 2017 •

edited

Loading

subnetmarco commented Feb 3, 2017

thibaultcha Feb 3, 2017

thibaultcha Feb 3, 2017

Tieske Feb 6, 2017

thibaultcha commented Feb 3, 2017

Tieske commented Feb 6, 2017

thibaultcha left a comment

thibaultcha Feb 6, 2017

Tieske Feb 6, 2017 •

edited

Loading

thibaultcha Feb 6, 2017

thibaultcha Feb 6, 2017

thibaultcha Feb 6, 2017

Tieske Feb 6, 2017

thibaultcha Feb 6, 2017

Tieske Feb 7, 2017

thibaultcha Feb 7, 2017

thibaultcha Feb 6, 2017

thibaultcha Feb 6, 2017

Tieske Feb 6, 2017

Tieske Feb 6, 2017

Tieske Feb 7, 2017

Tieske commented Feb 7, 2017

Tieske commented Feb 7, 2017

thibaultcha Feb 7, 2017

hutchic Feb 8, 2017

thibaultcha Feb 7, 2017

Tieske Feb 7, 2017

thibaultcha commented Feb 8, 2017 via email

thibaultcha commented Feb 8, 2017 via email

thibaultcha commented Feb 8, 2017 via email

hutchic commented Feb 8, 2017

		@@ -79,6 +79,8 @@ return {

		ctx.KONG_ACCESS_START = get_now()

		local original_host_header = ngx.req.get_headers().host

fix(proxy): host header including port #2045

fix(proxy): host header including port #2045

Conversation

Tieske commented Feb 3, 2017 • edited Loading

subnetmarco commented Feb 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultcha commented Feb 3, 2017

Tieske commented Feb 6, 2017

thibaultcha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tieske Feb 6, 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

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

Tieske commented Feb 7, 2017

Tieske commented Feb 7, 2017

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 Feb 8, 2017 via email

thibaultcha commented Feb 8, 2017 via email

thibaultcha commented Feb 8, 2017 via email

hutchic commented Feb 8, 2017

Tieske commented Feb 3, 2017 •

edited

Loading

Tieske Feb 6, 2017 •

edited

Loading