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

Kong doesn't match the longest uri when multiple uris are defined on an api #2662

Closed
tanelso2 opened this issue Jun 29, 2017 · 3 comments · Fixed by #2664
Closed

Kong doesn't match the longest uri when multiple uris are defined on an api #2662

tanelso2 opened this issue Jun 29, 2017 · 3 comments · Fixed by #2664
Labels

Comments

@tanelso2
Copy link

Summary

If two (or more) APIs have uris that match the incoming uri, Kong will route to the API with the longest uri, even if that uri is not the one that matches.

Steps To Reproduce

  1. Fresh Kong install
  2. Setup routes:
    curl -X POST -H "Content-Type: application/json" -d '{"name":"a", "uris":"/a,/cccccc", "upstream_url":"https://google.com"}' localhost:8001/apis
    curl -X POST -H "Content-Type: application/json" -d '{"name":"b", "uris":"/a/bb", "upstream_url":"https://example.com"}' localhost:8001/apis
  3. curl localhost:8000/a/bb/123 will route to google.com instead of example.com

Note:

  • If /a and /ccccc are defined as separate APIs, curl localhost:8000/a/bb/123 will route to example.com instead.
  • Also, if you replace /a,/ccccc with /a,/cc in the first route, curl localhost:8000/a/bb/123 will route to example.com.
  • Therefore, I think that there is an issue with how Kong is calculating the length of a matching uri. I'm not familiar with the code, but it seems like when an API matches, Kong thinks the longest uri on that API is the length of the match.

Additional Details & Logs

  • Kong version 0.10.3
@thibaultcha
Copy link
Member

Good catch! I remember this edge-case now and never came back to it to fix it...

I just opened #2664 to take care of this. Thanks for the report!

thibaultcha added a commit that referenced this issue Jun 30, 2017
When matching URIs, we iterate over a list of APIs, and not a list of
URIs (for various, performance reasons, although a refactor of the
router is scheduled with some performance improvements later on).
This list of APIs is sorted per URI length. But an API can have more
than one URI. As such, the router can potentially evaluate an API with a
very long URI, and a much shorter one that matches the request URI,
instead of a following API which would be an exact match for that API.

To fix this, we sort our deserialized list of URIs to iterate on in the
select method, and if we do have a match, we conserve the URI that was
matched, to check if it belongs to the right API at API match time.

Fix #2662
thibaultcha added a commit that referenced this issue Jun 30, 2017
When matching URIs, we iterate over a list of APIs, and not a list of
URIs (for various, performance reasons, although a refactor of the
router is scheduled with some performance improvements later on).
This list of APIs is sorted per URI length. But an API can have more
than one URI. As such, the router can potentially evaluate an API with a
very long URI, and a much shorter one that matches the request URI,
instead of a following API which would be an exact match for that API.

To fix this, we sort our deserialized list of URIs to iterate on in the
select method, and if we do have a match, we conserve the URI that was
matched, to check if it belongs to the right API at API match time.

Fix #2662
thibaultcha added a commit that referenced this issue Jun 30, 2017
When matching URIs, we iterate over a list of APIs, and not a list of
URIs (for various, performance reasons, although a refactor of the
router is scheduled with some performance improvements later on).
This list of APIs is sorted per URI length. But an API can have more
than one URI. As such, the router can potentially evaluate an API with a
very long URI, and a much shorter one that matches the request URI,
instead of a following API which would be an exact match for that API.

To fix this, we sort our deserialized list of URIs to iterate on in the
select method, and if we do have a match, we conserve the URI that was
matched, to check if it belongs to the right API at API match time.

Fix #2662
@thibaultcha
Copy link
Member

Thanks for the report, I just merged #2664 which takes care of this and should land in 0.11 and 0.10.4.

@tanelso2
Copy link
Author

tanelso2 commented Jul 6, 2017

@thibaultcha Thanks for fixing this!

thibaultcha added a commit that referenced this issue Jul 20, 2017
When matching URIs, we iterate over a list of APIs, and not a list of
URIs (for various, performance reasons, although a refactor of the
router is scheduled with some performance improvements later on).
This list of APIs is sorted per URI length. But an API can have more
than one URI. As such, the router can potentially evaluate an API with a
very long URI, and a much shorter one that matches the request URI,
instead of a following API which would be an exact match for that API.

To fix this, we sort our deserialized list of URIs to iterate on in the
select method, and if we do have a match, we conserve the URI that was
matched, to check if it belongs to the right API at API match time.

Fix #2662
thibaultcha added a commit that referenced this issue Oct 19, 2017
When matching URIs, we iterate over a list of APIs, and not a list of
URIs (for various, performance reasons, although a refactor of the
router is scheduled with some performance improvements later on).
This list of APIs is sorted per URI length. But an API can have more
than one URI. As such, the router can potentially evaluate an API with a
very long URI, and a much shorter one that matches the request URI,
instead of a following API which would be an exact match for that API.

To fix this, we sort our deserialized list of URIs to iterate on in the
select method, and if we do have a match, we conserve the URI that was
matched, to check if it belongs to the right API at API match time.

Fix #2662
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants