fix(router) wildcard host matching not considered with plain host match #4152
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
It seems like a couple of wildcard routing unit tests are currently failing in the current CI run? |
bungle
force-pushed
the
fix/router-hosts-matching
branch
from
7b67c98 to
326f81e
Compare
|
@thibaultcha thanks, it should be fixed now. |
kikito
approved these changes
Jan 8, 2019
thibaultcha
reviewed
Jan 8, 2019
| @@ -986,23 +988,24 @@ function _M.new(routes) | |||
|
|
|||
| -- host match | |||
|
|
|||
| if plain_indexes.hosts[ctx.req_host] then | |||
| req_category = bor(req_category, MATCH_RULES.HOST) | |||
There was a problem hiding this comment.
This change can be highly detrimental in many scenarios.
| @@ -512,7 +512,8 @@ end | |||
| do | |||
| local matchers = { | |||
| [MATCH_RULES.HOST] = function(route_t, ctx) | |||
| local host = route_t.hosts[ctx.hits.host or ctx.req_host] | |||
| local host = route_t.hosts[ctx.req_host] or | |||
| route_t.hosts[ctx.hits.host] | |||
There was a problem hiding this comment.
Here, we are ignoring the matched hint, which is thus rendered useless. In which case we could simply remove it. This change can be detrimental in some scenarios.
There was a problem hiding this comment.
What do you mean? If you remove route_t.hosts[ctx.hits.host] the tests will not pass, and if you remove route_t.hosts[ctx.req_host] the tests won't pass either. So it is not ignored (?).
| assert.equal(routes[2].id, res.headers["kong-route-id"]) | ||
| assert.equal(routes[2].service.id, res.headers["kong-service-id"]) | ||
| assert.equal(routes[2].service.name, res.headers["kong-service-name"]) | ||
| end) |
There was a problem hiding this comment.
This test is a duplicate of this unit test.
bungle
force-pushed
the
fix/router-hosts-matching
branch
from
326f81e to
c3787a9
Compare
Fixes issue reported here: #3094 (comment)
bungle
force-pushed
the
fix/router-hosts-matching
branch
from
c3787a9 to
3238871
Compare
bungle
added
pr/do not merge
pr/discussion
thibaultcha
added a commit
that referenced
this pull request
Jul 3, 2019
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
|
Replaced by #4775 |
thibaultcha
added a commit
that referenced
this pull request
Jul 15, 2019
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
When two routes are specified with wildcard host and plain host, the wildcard host may be skipped in some cases. See the test suite change that hilights the problem.
Issues resolved
Fixes issue reported here:
#3094 (comment)