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) handle a matching edge-case with uris #2343

Merged
merged 3 commits into from
Apr 7, 2017

Conversation

thibaultcha
Copy link
Member

Summary

When a request URI is matching (as a prefix) the uris of several APIs, but
other properties (say, hosts) differ, there was a possibility of
running into an edge-case, and try to match APIs with values of another
API. Doing so would result in a miss, and not return any API.

Full changelog

  • fix edge-case
  • simpler de-normalization logic when possible.
  • simpler matching logic with updated reducer returning all APIs
    for a category as well as a minimalistic plain subset.
  • use ngx.re.find instead of ngx.re.match when searching for hosts
    wildcard matching when categorizing.
  • update variable names for clarifications
  • some more comments to detail the logic

Issues resolved

Fix #2334

When a request URI is matching (as a prefix) the `uris` of several APIs, but
other properties (say, `hosts`) differ, there was a possibility of
running into an edge-case, and try to match APIs with values of another
API. Doing so would result in a miss, and not return any API.

Fix #2334
* simpler de-normalization logic when possible.
* simpler matching logic with updated reducer returning all APIs
  for a category as well as a minimalistic plain subset.
* use `ngx.re.find` instead of `ngx.re.match` when searching for hosts
  wildcard matching when categorizing.
* update variable names for clarifications
* some more comments to detail the logic
@Tieske
Copy link
Member

Tieske commented Apr 6, 2017

@bungle assigned you for rreview as you've been working on the router already

@thibaultcha
Copy link
Member Author

@bungle Could you give this patch a try on your side and make sure it still fixes the issue you were experiencing in your integration tests? Thank you!

@bungle
Copy link
Member

bungle commented Apr 7, 2017

@Tieske and @thibaultcha, I can confirm that this corrects the errors exposed by the integration test suite for #2315.

@bungle
Copy link
Member

bungle commented Apr 7, 2017

One comment though, not particularly for this PR.

I do not think we need to have ngx.req.get_headers() anywhere in a router code. Only header we ever read is ngx.var.http_host, and we possibly call it on hot path. At the same time we could get rid of that ugly grab_headers variable. exec doesn't need to pass any headers to find_api.

Or move that grab thing to find:

local host
if headers then
    host = headers["host"] or headers["Host"]

elseif grab_headers then
    host = ngx.var.http_host

end

-- or just use ngx.var.host and check that it is not "kong"

Then we don't need that empty_t anymore either.

@bungle
Copy link
Member

bungle commented Apr 7, 2017

Also on find_api we have this (totally unrelated):

local host = headers["host"] or headers["Host"]
if host then
  -- strip port number if given
  local m, err = re_match(host, "([^:]+)", "ajo")
  if not m then
    log(ERR, "could not strip port from Host header: ", err)
  end

  if m[0] then
    host = m[0]
  end
end

Why not just:

if host then
  local s = find(host, ":", nil, true)
  if s then
    host = sub(host, 1, s -1)
  end
end

Or:

local host = ngx.var.host
if host == "kong" then
  host = nil
end

Will also JIT compile.

@thibaultcha
Copy link
Member Author

@bungle

I do not think we need to have ngx.req.get_headers() anywhere in a router code.

Yes, planning on changing this in another contribution as well.

Why not just:

if host then
local s = find(host, ":", nil, true)
if s then
host = sub(host, 1, s -1)
end
end

Good point! We should definitely do that as well.

@thibaultcha thibaultcha merged commit 739654e into master Apr 7, 2017
@thibaultcha thibaultcha deleted the fix/router-matching branch April 7, 2017 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Router fails to resolve when longer path shadows another
3 participants