Skip to content

Commit

Permalink
fix(router) avoid conflicts between wildcard and plain host matches
Browse files Browse the repository at this point in the history
Background
----------

The rules of wildcard hosts matching in the router state that:

1. Routes with more matching attributes take precedence over Routes with
   fewer matching attributes
2. Plain hosts take precedence over wildcard hosts

Consider the following scenario (which is a simplification of this
issue, and already covered **prior** to this patch):

    r1:
        hosts = { "*.route.com" }

    r2:
        hosts = { "plain.route.com" }

    local matched_route = router.select("GET", "/", "plain.route.com")
    -- matched_route == r2

In the above case, both Routes have a single matching attribute,
resulting in a tie for rule 1. described above.  Therefore, we fallback
to rule 2.: the plain host takes precedence over the wildcard one.

The above case is already covered by our test suite ("does not take
precedence over a plain host").

Issue
-----

For the case at hand, consider the following scenario:

    r1:
        hosts = { "*.route.com" }
        paths = { "/path1" }

    r2:
        hosts = { "plain.route.com" }
        paths = { "/path2" }

    local matched_route = router.select("GET", "/path1", "plain.route.com")
    -- /!\ matched_route should be r1, but prior to this patch was 'nil'

    local matched_route = router.select("GET", "/path2", "plain.route.com")
    -- matched_route == r2

In the above case, both Routes have two matching attributes, resulting
in a tie for rule 1; both Routes will therefore be evaluated at the same
time (as part of the same matching category).

**However**, the Hosts matcher failed to evaluate a Route's wildcard
`hosts` because it expected the indexer to have evaluated it and stored
the matching wildcard host in `ctx.hits.host`. But since a plain host
was matched (r1's `plain.route.com`), no such wildcard host header was
matched in the indexes, and `ctx.hits.host` was `nil`. Having not
matched r1, we evaluate r2 which doesn't match either (`/path1` !=
`/path2`).

It is worth noting that the router should work with empty indexes
anyway (in tests only), something that we may want to explore in the
future, for the sake of hardening its behavior and maybe catching a
couple of bugs along the way.

Notes
-----

It is worth noting that this patch can be breaking in its nature, given
that the router behavior highlighted by the new test will differ before
and after the patch.

This patches marks a test as pending because fixing the matcher in this
patch made another issue surfaced: categorized Routes are not sorted by
"sub-matching" priority order (see rule 2.); Routes with plain hosts
only should be evaluated first since they have precedence over Routes
with one or more wildcards. A subsequent commit will fix this and
re-enable the test case.

This patch also replaces the one in
#4152, which is more expensive and
relies on indexes to fix the issue, instead of fixing the matcher. It
was also lacking unit tests, which are more appropriate (and mandatory)
for router changes.

Replaces #4152
Fix #3090
  • Loading branch information
thibaultcha committed Jul 3, 2019
1 parent 7884689 commit 01b1cb8
Show file tree
Hide file tree
Showing 2 changed files with 86 additions and 2 deletions.
18 changes: 17 additions & 1 deletion kong/router.lua
Original file line number Diff line number Diff line change
Expand Up @@ -572,9 +572,25 @@ do
local host = route_t.hosts[ctx.hits.host or ctx.req_host]
if host then
ctx.matches.host = host

return true
end

for i = 1, #route_t.hosts do
local host_t = route_t.hosts[i]

if host_t.wildcard then
local from, _, err = re_find(ctx.req_host, host_t.regex, "ajo")
if err then
log(ERR, "could not evaluate wildcard host regex: ", err)
return
end

if from then
ctx.matches.host = host_t.value
return true
end
end
end
end,

[MATCH_RULES.URI] = function(route_t, ctx)
Expand Down
70 changes: 69 additions & 1 deletion spec/01-unit/08-router_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,9 @@ describe("Router", function()
assert.equal(use_case[2].route, match_t.route)
end)

it("does not take precedence over a plain host", function()
pending("does not take precedence over a plain host", function()
-- Pending: temporarily pending in the current commit, awaiting a fix
-- in a subsequent commit.
table.insert(use_case, 1, {
service = service,
route = {
Expand Down Expand Up @@ -593,6 +595,72 @@ describe("Router", function()
assert.same(nil, match_t.matches.uri_captures)
end)

it("matches [wildcard host + path] even if a similar [plain host] exists", function()
local use_case = {
{
service = service,
headers = {
host = { "*.route.com" },
},
route = {
paths = { "/path1" },
},
},
{
service = service,
headers = {
host = { "plain.route.com" },
},
route = {
paths = { "/path2" },
},
},
}

router = assert(Router.new(use_case))

local match_t = router.select("GET", "/path1", "plain.route.com")
assert.truthy(match_t)
assert.equal(use_case[1].route, match_t.route)
assert.same("*.route.com", match_t.matches.host)
assert.same(nil, match_t.matches.method)
assert.same("/path1", match_t.matches.uri)
assert.same(nil, match_t.matches.uri_captures)
end)

it("matches [plain host + path] even if a matching [wildcard host] exists", function()
local use_case = {
{
service = service,
headers = {
host = { "*.route.com" },
},
route = {
paths = { "/path1" },
},
},
{
service = service,
headers = {
host = { "plain.route.com" },
},
route = {
paths = { "/path2" },
},
},
}

router = assert(Router.new(use_case))

local match_t = router.select("GET", "/path2", "plain.route.com")
assert.truthy(match_t)
assert.equal(use_case[2].route, match_t.route)
assert.same("plain.route.com", match_t.matches.host)
assert.same(nil, match_t.matches.method)
assert.same("/path2", match_t.matches.uri)
assert.same(nil, match_t.matches.uri_captures)
end)

it("matches [wildcard/plain + uri + method]", function()
finally(function()
table.remove(use_case)
Expand Down

0 comments on commit 01b1cb8

Please sign in to comment.