Skip to content

Commit

Permalink
fix(router) preserve_host always fetches the request's Host (#2832)
Browse files Browse the repository at this point in the history
Summary
-------

It was reported in #2825 that `preserve_host` seemed to behave wrongly
upon subsequent call of a same API if that API had no `hosts`.

Since matches APIs are cached, and so are their `upstream_host` values,
I investigated around the LRU cache of our matches in the router. What I
discovered is, considering the following API:

    {
        "name": "example",
        "upstream_url": "http://httpbin.org",
        "uris": ["/example"]
    }

The router would try to save cycles by *not* reading the
`ngx.var.http_host` variable, since no API is configured to match on a
Host header.

However, the Host is also part of the cache key since the introduction
of the LRU cache in the router. But in the above case, considering the
following two requests:

    GET /example HTTP/1.1
    Host: domain1.com

    GET /example HTTP/1.1
    Host: domain2.com

Both matches would be cached under the **same** key (since the
`req_host` argument of `find_api` is `nil` because the router does not
read the Host header):

    GET:/example:

Notice the lack of Host part after the last colon.

Hence, once this API would be matched after the first request, its
cached `upstream_host` would be `domain1.com`, and subsequent calls with
`Host: domain2.com` would hit the same key, and yield the same
`upstream_host` as the first request.

The approach taken by this fix is to **always** retrieve the Host
header. This choice is driven by the fact that cache keys should have as
high a cardinality as possible, and to ensure we avoid such duplicate
keys in the future.

Changes
-------

* remove `grab_header` optimization from `router.lua`
* remove `grab_header` tests from unit test suite
* make `find_api()`'s 3rd argument (Host header) mandatory
* update related unit tests to reflect the above change
* new regression test for this issue

---

Fix #2824
  • Loading branch information
thibaultcha authored Aug 23, 2017
1 parent c543dd5 commit 83bcda0
Show file tree
Hide file tree
Showing 2 changed files with 103 additions and 110 deletions.
14 changes: 2 additions & 12 deletions kong/core/router.lua
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ local tonumber = tonumber
local ipairs = ipairs
local pairs = pairs
local type = type
local next = next
local band = bit.band
local bor = bit.bor
local ERR = ngx.ERR
Expand Down Expand Up @@ -588,17 +587,14 @@ function _M.new(apis)
end)


local grab_host = #wildcard_hosts > 0 or next(plain_indexes.hosts) ~= nil


local function find_api(req_method, req_uri, req_host, ngx)
if type(req_method) ~= "string" then
return error("arg #1 method must be a string")
end
if type(req_uri) ~= "string" then
return error("arg #2 uri must be a string")
end
if req_host and type(req_host) ~= "string" then
if type(req_host) ~= "string" then
return error("arg #3 host must be a string")
end

Expand Down Expand Up @@ -831,13 +827,7 @@ function _M.new(apis)
end
end

--print("grab host header: ", grab_host)

local req_host

if grab_host then
req_host = ngx.var.http_host
end
local req_host = ngx.var.http_host

local match_t = find_api(method, uri, req_host, ngx)
if not match_t then
Expand Down
Loading

0 comments on commit 83bcda0

Please sign in to comment.