Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(router) ensure transparent proxying of URIs #2519

Merged
merged 2 commits into from
Jun 9, 2017
Merged

fix(router) ensure transparent proxying of URIs #2519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
146d677
fix(router) ensure transparent proxying of URIs
thibaultcha Jun 9, 2017
14f6297
refactor(router) simplify URI stripping logic
thibaultcha Jun 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions kong/core/handler.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ local balancer_execute = require("kong.core.balancer").execute

local router, router_err
local ngx_now = ngx.now
local sub = string.sub

local server_header = _KONG._NAME.."/".._KONG._VERSION


Expand Down Expand Up @@ -88,7 +90,7 @@ return {

ctx.KONG_ACCESS_START = get_now()

local api, upstream, host_header = router.exec(ngx)
local api, upstream, host_header, uri = router.exec(ngx)
if not api then
return responses.send_HTTP_NOT_FOUND("no API found with those values")
end
Expand Down Expand Up @@ -128,14 +130,32 @@ return {
"' with: "..tostring(err))
end

-- if set `host_header` is the original header to be preserved
-- `uri` is the URI with which to call upstream, as returned by the
-- router, which might have truncated it (`strip_uri`).
-- `host_header` is the original header to be preserved if set.
var.upstream_uri = uri
var.upstream_host = host_header or
balancer_address.hostname..":"..balancer_address.port

end,
-- Only executed if the `router` module found an API and allows nginx to proxy it.
after = function()
local ctx = ngx.ctx
local var = ngx.var

do
-- Nginx's behavior when proxying a request with an empty querystring
-- `/foo?` is to keep `$is_args` an empty string, hence effectively
-- stripping the empty querystring.
-- We overcome this behavior with our own logic, to preserve user
-- desired semantics.
local upstream_uri = var.upstream_uri

if var.is_args == "?" or sub(var.request_uri, -1) == "?" then
var.upstream_uri = upstream_uri .. "?" .. (var.args or "")
end
end

local now = get_now()

ctx.KONG_ACCESS_TIME = now - ctx.KONG_ACCESS_START -- time spent in Kong's access_by_lua
Expand Down
40 changes: 16 additions & 24 deletions kong/core/router.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,8 +632,9 @@ function _M.new(apis)


function self.exec(ngx)
local method = ngx.req.get_method()
local uri = ngx.var.request_uri
local method = ngx.req.get_method()
local request_uri = ngx.var.request_uri
local uri = request_uri


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

local new_uri
local uri_root = uri == "/"
local uri_root = request_uri == "/"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@bungle bungle Jun 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)


if uri_root or not api_t.strip_uri_regex then
new_uri = uri

else
if not uri_root and api_t.strip_uri_regex then
local _, err
new_uri, _, err = re_sub(uri, api_t.strip_uri_regex, "/$1", "ajo")
if not new_uri then
uri, _, err = re_sub(uri, api_t.strip_uri_regex, "/$1", "ajo")
if not uri then
log(ERR, "could not strip URI: ", err)
return
end
Expand All @@ -677,35 +674,30 @@ function _M.new(apis)

local upstream = api_t.upstream
if upstream.path and upstream.path ~= "/" then
if new_uri ~= "/" then
new_uri = upstream.file .. new_uri
if uri ~= "/" then
uri = upstream.file .. uri

else
if upstream.path ~= upstream.file then
if uri_root or sub(uri, -1) == "/" then
new_uri = upstream.path
if uri_root or sub(request_uri, -1) == "/" then
uri = upstream.path

else
new_uri = upstream.file
uri = upstream.file
end

else
if uri_root or sub(uri, -1) ~= "/" then
new_uri = upstream.file
if uri_root or sub(request_uri, -1) ~= "/" then
uri = upstream.file

else
new_uri = upstream.file .. new_uri
uri = upstream.file .. uri
end
end
end
end


if new_uri ~= uri then
ngx.req.set_uri(new_uri)
end


local host_header

if api_t.preserve_host then
Expand All @@ -717,7 +709,7 @@ function _M.new(apis)
ngx.header["Kong-Api-Name"] = api_t.api.name
end

return api_t.api, api_t.upstream, host_header
return api_t.api, api_t.upstream, host_header, uri
end


Expand Down
3 changes: 2 additions & 1 deletion kong/templates/nginx_kong.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ server {
location / {
set $upstream_host nil;
set $upstream_scheme nil;
set $upstream_uri nil;

rewrite_by_lua_block {
kong.rewrite()
Expand All @@ -128,7 +129,7 @@ server {

proxy_ssl_name $upstream_host;

proxy_pass $upstream_scheme://kong_upstream;
proxy_pass $upstream_scheme://kong_upstream$upstream_uri;

header_filter_by_lua_block {
kong.header_filter()
Expand Down
65 changes: 33 additions & 32 deletions spec/01-unit/11-router_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -798,9 +798,6 @@ describe("Router", function()
end
}),
req = {
set_uri = function(request_uri)
_ngx.var.request_uri = request_uri
end,
get_method = function()
return method
end,
Expand All @@ -813,7 +810,7 @@ describe("Router", function()
return _ngx
end

it("returns api/scheme/host/port", function()
it("returns api/upstream info/host/uri", function()
local use_case_apis = {
{
name = "api-1",
Expand All @@ -830,18 +827,22 @@ describe("Router", function()
local router = assert(Router.new(use_case_apis))

local _ngx = mock_ngx("GET", "/my-api", {})
local api, upstream = router.exec(_ngx)
local api, upstream, host_header, uri = router.exec(_ngx)
assert.same(use_case_apis[1], api)
assert.equal("http", upstream.scheme)
assert.equal("httpbin.org", upstream.host)
assert.equal(80, upstream.port)
assert.is_nil(host_header) -- only when `preserve_host = true`
assert.equal("/my-api", uri)

local _ngx = mock_ngx("GET", "/my-api-2", {})
api, upstream = router.exec(_ngx)
api, upstream, host_header, uri = router.exec(_ngx)
assert.same(use_case_apis[2], api)
assert.equal("https", upstream.scheme)
assert.equal("httpbin.org", upstream.host)
assert.equal(443, upstream.port)
assert.is_nil(host_header) -- only when `preserve_host = true`
assert.equal("/my-api-2", uri)
end)

it("parses path component from upstream_url property", function()
Expand Down Expand Up @@ -897,9 +898,9 @@ describe("Router", function()
local router = assert(Router.new(use_case_apis))

local _ngx = mock_ngx("GET", "/endel%C3%B8st", {})
local api = router.exec(_ngx)
local api, _, _, uri = router.exec(_ngx)
assert.same(use_case_apis[1], api)
assert.equal("/endel%C3%B8st", _ngx.var.request_uri)
assert.equal("/endel%C3%B8st", uri)
end)

describe("grab_headers", function()
Expand Down Expand Up @@ -965,25 +966,25 @@ describe("Router", function()
it("strips the specified uris from the given uri if matching", function()
local _ngx = mock_ngx("GET", "/my-api/hello/world", {})

local api = router.exec(_ngx)
local api, _, _, uri = router.exec(_ngx)
assert.same(use_case_apis[1], api)
assert.equal("/hello/world", _ngx.var.request_uri)
assert.equal("/hello/world", uri)
end)

it("strips if matched URI is plain (not a prefix)", function()
local _ngx = mock_ngx("GET", "/my-api", {})

local api = router.exec(_ngx)
local api, _, _, uri = router.exec(_ngx)
assert.same(use_case_apis[1], api)
assert.equal("/", _ngx.var.request_uri)
assert.equal("/", uri)
end)

it("doesn't strip if 'strip_uri' is not enabled", function()
local _ngx = mock_ngx("POST", "/my-api/hello/world", {})

local api = router.exec(_ngx)
local api, _, _, uri = router.exec(_ngx)
assert.same(use_case_apis[2], api)
assert.equal("/my-api/hello/world", _ngx.var.request_uri)
assert.equal("/my-api/hello/world", uri)
end)

it("does not strips root / URI", function()
Expand All @@ -999,45 +1000,45 @@ describe("Router", function()

local _ngx = mock_ngx("POST", "/my-api/hello/world", {})

local api = router.exec(_ngx)
local api, _, _, uri = router.exec(_ngx)
assert.same(use_case_apis[1], api)
assert.equal("/my-api/hello/world", _ngx.var.request_uri)
assert.equal("/my-api/hello/world", uri)
end)

it("can find an API with stripped URI several times in a row", function()
local _ngx = mock_ngx("GET", "/my-api", {})

local api = router.exec(_ngx)
local api, _, _, uri = router.exec(_ngx)
assert.same(use_case_apis[1], api)
assert.equal("/", _ngx.var.request_uri)
assert.equal("/", uri)

_ngx = mock_ngx("GET", "/my-api", {})
local api2 = router.exec(_ngx)
local api2, _, _, uri = router.exec(_ngx)
assert.same(use_case_apis[1], api2)
assert.equal("/", _ngx.var.request_uri)
assert.equal("/", uri)
end)

it("can proxy an API with stripped URI with different URIs in a row", function()
local _ngx = mock_ngx("GET", "/my-api", {})

local api = router.exec(_ngx)
local api, _, _, uri = router.exec(_ngx)
assert.same(use_case_apis[1], api)
assert.equal("/", _ngx.var.request_uri)
assert.equal("/", uri)

_ngx = mock_ngx("GET", "/this-api", {})
api = router.exec(_ngx)
api, _, _, uri = router.exec(_ngx)
assert.same(use_case_apis[1], api)
assert.equal("/", _ngx.var.request_uri)
assert.equal("/", uri)

_ngx = mock_ngx("GET", "/my-api", {})
api = router.exec(_ngx)
api, _, _, uri = router.exec(_ngx)
assert.same(use_case_apis[1], api)
assert.equal("/", _ngx.var.request_uri)
assert.equal("/", uri)

_ngx = mock_ngx("GET", "/this-api", {})
api = router.exec(_ngx)
api, _, _, uri = router.exec(_ngx)
assert.same(use_case_apis[1], api)
assert.equal("/", _ngx.var.request_uri)
assert.equal("/", uri)
end)

it("strips url encoded uris", function()
Expand All @@ -1052,9 +1053,9 @@ describe("Router", function()
local router = assert(Router.new(use_case_apis))

local _ngx = mock_ngx("GET", "/endel%C3%B8st", {})
local api = router.exec(_ngx)
local api, _, _, uri = router.exec(_ngx)
assert.same(use_case_apis[1], api)
assert.equal("/", _ngx.var.request_uri)
assert.equal("/", uri)
end)
end)

Expand Down Expand Up @@ -1211,10 +1212,10 @@ describe("Router", function()
local router = assert(Router.new(use_case_apis) )

local _ngx = mock_ngx("GET", args[3], {})
local api, upstream = router.exec(_ngx)
local api, upstream, _, uri = router.exec(_ngx)
assert.same(use_case_apis[1], api)
assert.equal(args[1], upstream.path)
assert.equal(args[4], _ngx.var.request_uri)
assert.equal(args[4], uri)
end)
end
end)
Expand Down
Loading