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.

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): always calculate match_t.upstream_uri to avoid wrong cache #12307

Merged
merged 8 commits into from
Jan 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 17 additions & 7 deletions kong/router/atc.lua
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,19 @@ local add_debug_headers = utils.add_debug_headers
local get_upstream_uri_v0 = utils.get_upstream_uri_v0


local function set_upstream_uri(req_uri, match_t)
local matched_route = match_t.route

local request_prefix = match_t.prefix or "/"
local request_postfix = sanitize_uri_postfix(req_uri:sub(#request_prefix + 1))

local upstream_base = match_t.upstream_url_t.path or "/"

match_t.upstream_uri = get_upstream_uri_v0(matched_route, request_postfix,
req_uri, upstream_base)
end


function _M:matching(params)
local req_uri = params.uri
local req_host = params.host
Expand Down Expand Up @@ -439,12 +452,6 @@ function _M:matching(params)
service_hostname_type, service_path = get_service_info(service)

local request_prefix = matched_route.strip_path and matched_path or nil
local request_postfix = request_prefix and req_uri:sub(#matched_path + 1) or req_uri:sub(2, -1)
request_postfix = sanitize_uri_postfix(request_postfix) or ""
local upstream_base = service_path or "/"

local upstream_uri = get_upstream_uri_v0(matched_route, request_postfix, req_uri,
upstream_base)

return {
route = matched_route,
Expand All @@ -457,9 +464,9 @@ function _M:matching(params)
type = service_hostname_type,
host = service_host,
port = service_port,
path = service_path,
},
upstream_scheme = service_protocol,
upstream_uri = upstream_uri,
upstream_host = matched_route.preserve_host and req_host or nil,
}
end
Expand Down Expand Up @@ -546,6 +553,9 @@ function _M:exec(ctx)

-- found a match

-- update upstream_uri in cache result
set_upstream_uri(req_uri, match_t)

-- debug HTTP request header logic
add_debug_headers(var, header, match_t)

Expand Down
50 changes: 43 additions & 7 deletions spec/01-unit/08-router_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -4945,10 +4945,8 @@ do
describe("Router (flavor = " .. flavor .. ")", function()
reload_router(flavor)

local use_case, router

lazy_setup(function()
use_case = {
it("[cache hit should be case sensitive]", function()
local use_case = {
{
service = service,
route = {
Expand All @@ -4962,10 +4960,8 @@ do
},
},
}
end)

it("[cache hit should be case sensitive]", function()
router = assert(new_router(use_case))
local router = assert(new_router(use_case))

local ctx = {}
local _ngx = mock_ngx("GET", "/foo", { test1 = "QUOTE", })
Expand Down Expand Up @@ -4995,6 +4991,46 @@ do
-- cache miss, case sensitive
assert.falsy(ctx.route_match_cached)
end)

it("[cache hit should have correct match_t.upstream_uri]", function()
local host = "example.com"
local use_case = {
{
service = service,
route = {
id = "e8fb37f1-102d-461e-9c51-6608a6bb8101",
hosts = { host },
preserve_host = true,
},
},
}

local router = assert(new_router(use_case))

local ctx = {}
local _ngx = mock_ngx("GET", "/foo", { host = host, })
router._set_ngx(_ngx)

-- first match
local match_t = router:exec(ctx)
assert.truthy(match_t)
assert.same(use_case[1].route, match_t.route)
assert.falsy(ctx.route_match_cached)
assert.equal(host, match_t.upstream_host)
assert.same("/foo", match_t.upstream_uri)

local ctx = {}
local _ngx = mock_ngx("GET", "/bar", { host = host, })
router._set_ngx(_ngx)

-- cache hit
local match_t = router:exec(ctx)
assert.truthy(match_t)
assert.same(use_case[1].route, match_t.route)
assert.same(ctx.route_match_cached, "pos")
assert.equal(host, match_t.upstream_host)
assert.same("/bar", match_t.upstream_uri)
end)
end)
end -- local flavor = "traditional_compatible"

Expand Down
1 change: 0 additions & 1 deletion spec/02-integration/01-helpers/01-helpers_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ for _, strategy in helpers.each_strategy() do
bp.routes:insert {
hosts = { "mock_upstream" },
protocols = { "http" },
paths = { "/" },
service = service
}

Expand Down
2 changes: 0 additions & 2 deletions spec/02-integration/05-proxy/02-router_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -881,7 +881,6 @@ for _, strategy in helpers.each_strategy() do
routes = insert_routes(bp, {
{
hosts = { "mock_upstream" },
paths = { "/" },
},
})
end)
Expand Down Expand Up @@ -1302,7 +1301,6 @@ for _, strategy in helpers.each_strategy() do
routes = insert_routes(bp, {
{
protocols = { "https" },
paths = { "/" },
snis = { "www.example.org" },
service = {
name = "service_behind_www.example.org"
Expand Down
1 change: 0 additions & 1 deletion spec/02-integration/05-proxy/03-upstream_headers_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -278,7 +278,6 @@ for _, strategy in helpers.each_strategy() do

assert(bp.routes:insert {
hosts = { "headers-charset.test" },
paths = { "/" },
service = service,
})

Expand Down
1 change: 0 additions & 1 deletion spec/02-integration/05-proxy/14-server_tokens_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,6 @@ describe("headers [#" .. strategy .. "]", function()
return function()
bp.routes:insert {
hosts = { "headers-inspect.test" },
paths = { "/" },
}

local service = bp.services:insert({
Expand Down
2 changes: 0 additions & 2 deletions spec/03-plugins/03-http-log/01-log_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ for _, strategy in helpers.each_strategy() do

local route1 = bp.routes:insert {
hosts = { "http_logging.test" },
paths = { "/" },
service = service1
}

Expand Down Expand Up @@ -628,7 +627,6 @@ for _, strategy in helpers.each_strategy() do

local route = bp.routes:insert {
hosts = { "http_queue_logging.test" },
paths = { "/" },
service = service
}

Expand Down
1 change: 0 additions & 1 deletion spec/03-plugins/07-loggly/01-log_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ for _, strategy in helpers.each_strategy() do

local route1 = bp.routes:insert {
hosts = { "logging.test" },
paths = { "/" },
}

local route2 = bp.routes:insert {
Expand Down
1 change: 0 additions & 1 deletion spec/03-plugins/13-cors/01-access_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,6 @@ for _, strategy in helpers.each_strategy() do

local route1 = bp.routes:insert({
hosts = { "cors1.test" },
paths = { "/" },
})

local route2 = bp.routes:insert({
Expand Down
1 change: 0 additions & 1 deletion spec/03-plugins/25-oauth2/04-invalidations_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ for _, strategy in helpers.each_strategy() do
route = assert(admin_api.routes:insert {
hosts = { "oauth2.com" },
protocols = { "http", "https" },
paths = { "/" },
service = service,
})

Expand Down
1 change: 0 additions & 1 deletion spec/03-plugins/31-proxy-cache/02-access_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ do

local route1 = assert(bp.routes:insert {
hosts = { "route-1.test" },
paths = { "/" },
})
local route2 = assert(bp.routes:insert {
hosts = { "route-2.test" },
Expand Down
Loading