Skip to content

Commit

Permalink
fix(router) don't append trailing slashes unconditionally
Browse files Browse the repository at this point in the history 
We now let the consumer of an API decide whether or not a trailing slash
should be required. If the requests' URI ends up being "/", (post URI
stripping), we then defer the decision to the `upstream_url` configured
on the API. The details of the implementation are discussed in Kong#2211.

Fix Kong#2211
  • Loading branch information
@bungle @thibaultcha
bungle authored and thibaultcha committed Apr 7, 2017
1 parent 78aa900 commit 984a7f9
Show file tree
Hide file tree
Showing 6 changed files with 215 additions and 10 deletions.
22 changes: 20 additions & 2 deletions kong/core/router.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ local upper = string.upper
local lower = string.lower
local find = string.find
local fmt = string.format
local sub = string.sub
local tonumber = tonumber
local ipairs = ipairs
local pairs = pairs
Expand Down Expand Up @@ -640,8 +641,25 @@ function _M.new(apis)
end


if api_t.upstream.path then
new_uri = api_t.upstream.path .. new_uri
local upstream_path = api_t.upstream.path
if upstream_path then
if new_uri == "/" then
new_uri = upstream_path

else
new_uri = upstream_path .. (sub(upstream_path, -1) == "/" and sub(new_uri, 2) or new_uri)
end
end


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
new_uri = sub(new_uri, 1, -2)

elseif not new_uri_slash and req_uri_slash and uri ~= "/" then
new_uri = new_uri .. "/"
end


Expand Down
4 changes: 0 additions & 4 deletions kong/dao/schemas/apis.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,6 @@ local function validate_upstream_url(value)
end
end

if parsed_url.path and string.sub(value, #value) == "/" then
return false, "Cannot end with a slash"
end

return true
end

Expand Down
7 changes: 3 additions & 4 deletions spec/01-unit/07-entities_schemas_spec.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,15 +78,14 @@ describe("Entities Schemas", function()
assert.equal("Supported protocols are HTTP and HTTPS", errors.upstream_url)
end)

it("should return error with final slash in upstream_url", function()
it("should not return error with final slash in upstream_url", function()
local valid, errors = validate_entity({
name = "mockbin",
upstream_url = "http://mockbin.com/",
hosts = { "mockbin.com" },
}, api_schema)
assert.is_false(valid)
assert.equal("Cannot end with a slash", errors.upstream_url)

assert.is_nil(errors)
assert.is_true(valid)
end)

it("should validate with upper case protocol", function()
Expand Down
53 changes: 53 additions & 0 deletions spec/01-unit/11-router_spec.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1017,5 +1017,58 @@ describe("Router", function()
end)
end)
end)

describe("trailing slash", function()
local checks = {
-- upstream url request path expected path strip uri
{ "/", "/", "/", true },
{ "/", "/foo/bar", "/", true },
{ "/", "/foo/bar/", "/", true },
{ "/foo/bar", "/", "/foo/bar", true },
{ "/foo/bar/", "/", "/foo/bar/", true },
{ "/foo/bar", "/foo/bar", "/foo/bar", true },
{ "/foo/bar/", "/foo/bar", "/foo/bar", true },
{ "/foo/bar", "/foo/bar/", "/foo/bar/", true },
{ "/foo/bar/", "/foo/bar/", "/foo/bar/", true },
{ "/", "/", "/", false },
{ "/", "/foo/bar", "/foo/bar", false },
{ "/", "/foo/bar/", "/foo/bar/", false },
{ "/foo/bar", "/", "/foo/bar", false },
{ "/foo/bar/", "/", "/foo/bar/", false },
{ "/foo/bar", "/foo/bar", "/foo/bar/foo/bar", false },
{ "/foo/bar/", "/foo/bar", "/foo/bar/foo/bar", false },
{ "/foo/bar", "/foo/bar/", "/foo/bar/foo/bar/", false },
{ "/foo/bar/", "/foo/bar/", "/foo/bar/foo/bar/", false },
}

for i, args in ipairs(checks) do

local config = args[4] == true and "(strip_uri = on) " or "(strip_uri = off)"
local space = string.sub(args[1], -1) == "/" and "" or " "

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

local use_case_apis = {
{
name = "api-1",
strip_uri = args[4],
upstream_url = "http://httpbin.org" .. args[1],
uris = {
args[2],
},
}
}

local router = assert(Router.new(use_case_apis) )

local _ngx = mock_ngx("GET", args[2], {})
local api, upstream = router.exec(_ngx)
assert.same(use_case_apis[1], api)
assert.equal(args[1], upstream.path)
assert.equal(args[3], _ngx.var.uri)
end)
end
end)
end)
end)
94 changes: 94 additions & 0 deletions spec/02-integration/05-proxy/01-router_spec.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -388,4 +388,98 @@ describe("Router", function()
assert.equal("fixture-api", res.headers["kong-api-name"])
end)
end)

describe("trailing slash", function()
local checks = {
-- upstream url request path expected path strip uri
{ "/", "/", "/", nil },
{ "/", "/foo/bar", "/", nil },
{ "/", "/foo/bar/", "/", nil },
{ "/foo/bar", "/", "/foo/bar", nil },
{ "/foo/bar/", "/", "/foo/bar/", nil },
{ "/foo/bar", "/foo/bar", "/foo/bar", nil },
{ "/foo/bar/", "/foo/bar", "/foo/bar", nil },
{ "/foo/bar", "/foo/bar/", "/foo/bar/", nil },
{ "/foo/bar/", "/foo/bar/", "/foo/bar/", nil },
{ "/", "/", "/", true },
{ "/", "/foo/bar", "/", true },
{ "/", "/foo/bar/", "/", true },
{ "/foo/bar", "/", "/foo/bar", true },
{ "/foo/bar/", "/", "/foo/bar/", true },
{ "/foo/bar", "/foo/bar", "/foo/bar", true },
{ "/foo/bar/", "/foo/bar", "/foo/bar", true },
{ "/foo/bar", "/foo/bar/", "/foo/bar/", true },
{ "/foo/bar/", "/foo/bar/", "/foo/bar/", true },
{ "/", "/", "/", false },
{ "/", "/foo/bar", "/foo/bar", false },
{ "/", "/foo/bar/", "/foo/bar/", false },
{ "/foo/bar", "/", "/foo/bar", false },
{ "/foo/bar/", "/", "/foo/bar/", false },
{ "/foo/bar", "/foo/bar", "/foo/bar/foo/bar", false },
{ "/foo/bar/", "/foo/bar", "/foo/bar/foo/bar", false },
{ "/foo/bar", "/foo/bar/", "/foo/bar/foo/bar/", false },
{ "/foo/bar/", "/foo/bar/", "/foo/bar/foo/bar/", false },
}

setup(function()
helpers.dao:truncate_tables()

for i, args in ipairs(checks) do
assert(helpers.dao.apis:insert {
name = "localbin-" .. i,
strip_uri = args[4],
upstream_url = "http://localhost:9999" .. args[1],
hosts = {
"localbin-" .. i .. ".com",
},
uris = {
args[2],
}
})
end

assert(helpers.start_kong {
nginx_conf = "spec/fixtures/custom_nginx.template",
})
end)

teardown(function()
helpers.stop_kong()
end)

local function check(i, request_uri, expected_uri)
return function()
local res = assert(client:send {
method = "GET",
path = request_uri,
headers = {
["Host"] = "localbin-" .. i .. ".com",
}
})

local json = assert.res_status(200, res)
local data = cjson.decode(json)

assert.equal(expected_uri, data.vars.request_uri)
end
end

for i, 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(i, args[2], args[3]))
end
end)
end)
45 changes: 45 additions & 0 deletions spec/fixtures/custom_nginx.template
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,51 @@ http {
return 200;
}

location / {
content_by_lua_block {
local cjson = require "cjson"
local var = ngx.var
local req = ngx.req

req.read_body()

local json = cjson.encode {
vars = {
uri = var.uri,
host = var.host,
hostname = var.hostname,
https = var.https,
scheme = var.scheme,
is_args = var.is_args,
server_addr = var.server_addr,
server_port = var.server_port,
server_name = var.server_name,
server_protocol = var.server_protocol,
remote_addr = var.remote_addr,
remote_port = var.remote_port,
realip_remote_addr = var.realip_remote_addr,
realip_remote_port = var.realip_remote_port,
binary_remote_addr = var.binary_remote_addr,
request = var.request,
request_uri = var.request_uri,
request_time = var.request_time,
request_length = var.request_length,
request_method = var.request_method,
bytes_received = var.bytes_received,
},
method = req.get_method(),
headers = req.get_headers(0),
uri_args = req.get_uri_args(0),
post_args = req.get_post_args(0),
http_version = req.http_version(),
}

ngx.status = 200
ngx.say(json)
ngx.exit(200)
}
}

location /headers-inspect {
content_by_lua_block {
local cjson = require "cjson"
Expand Down

0 comments on commit 984a7f9

Please sign in to comment.