-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[feature] Path Resolver #282
Conversation
8a0f8d1
to
c4e2435
Compare
@DavidTPate let us know if you have suggestion or feedback before we merge it. thanks. |
-- Otherwise, we look for it by path. We have to loop over all APIs and compare the requested URI. | ||
local request_uri = ngx.var.request_uri | ||
for path, api in pairs(apis_dics.path) do | ||
local m, err = ngx.re.match(request_uri, path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been awhile since I've done LUA, but it looks like path when stored in the database would be something like /status
where the /
would be added if not present to the beginning but the regular expression matching here doesn't check where /status
is.
What it looks like to me is that if I created an API with the path /status
when it gets to the comparison point it would compile the regular expression to /\/status/
(where /
is my boundary character). Which means that it will match a request to /myapi/status
, /status
, /something/status/somethingelse/status
etc. I'm not sure how you want to approach it, but Express in NodeJS land for example uses a few configuration parameters to create their Regular Expression. You can see those in path-to-regexp under the options section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is not 100% finished, and I was thinking to simply match if the request_uri starts with the path. It's a good start and good enough for this implementation (one path per API).
I originally wanted path to be an actual regex users could set, but I think it's better to do like I just described first, and implement multiple paths and regex at the same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely agree, first out simply pre-pending $
to the path would be good before doing the match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean ^
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hahaha. Yeah, sorry it's been a long day.
@sinzone Looks good to me, the only blocking issue I saw was my comments about pre-pending the path with Thanks for the quick turnaround everyone! |
👍 |
@DavidTPate thanks for the detailed feedback! |
c4e2435
to
ac267b8
Compare
The last 2 commits implement:
|
d39b830
to
feb7259
Compare
- APIs now expect either a `public_dns` or a `path`. - some schemas improvements on consumers and apis - rename the newest migration to 0.3.0, the new kong version
- Check that a path complies to unreserved RFC 3986 characters - Only match a path in the resolver if the request_uri *starts* with it
feb7259
to
5228fb2
Compare
@DavidTPate we've released 0.3.1 - pls let us know if you have any feedback. thx! https://github.com/Mashape/kong/releases/tag/0.3.1 |
[feature] Path Resolver Former-commit-id: e22da6d567657810c0dcaa54380b3277c1864685
chore(go-pluginserver) small fix for go-pluginserver --version
### Summary The 0.17.0 is not beta anymore as the 0.17.1 was released as non-beta, so lets start pinning the resty.http again. #### What's Changed Documentation is clearer about using same connection for multiple requests via 'request' function by @alexdowad in #282 - fix 100 response with headers by @catbro666 - fix: no trailing \"?\" for empty query table by @StarlightIbuki #### New Contributors @chronolaw made their first contribution @alexdowad made their first contribution @catbro666 made their first contribution @StarlightIbuki made their first contribution Signed-off-by: Aapo Talvensaari <[email protected]>
### Summary The 0.17.0 is not beta anymore as the 0.17.1 was released as non-beta, so lets start pinning the resty.http again. #### What's Changed Documentation is clearer about using same connection for multiple requests via 'request' function by @alexdowad in #282 - fix 100 response with headers by @catbro666 - fix: no trailing \"?\" for empty query table by @StarlightIbuki #### New Contributors @chronolaw made their first contribution @alexdowad made their first contribution @catbro666 made their first contribution @StarlightIbuki made their first contribution Signed-off-by: Aapo Talvensaari <[email protected]>
First implementation of #192. After discussions, it was decided to implement a basic working version of the Path resolver but a version that introduces changes that goes towards the end-goal of supporting all discussed features of #192.
Changes
public_dns
or apath
.public_dns
and 1path
for now (we can have several at the same time later, but it'll require a heavy data migration as discussed in [request] support for routing based on path? #192).public_dns
matching either theHost
orX-Host-Override
header. If nothing was found yet, it tries to compare therequest_uri
with thepath
currently in memory. Hence,public_dns
has the priority overpath
.Example:
Improvements:
strip_path
option so that/status/200
will actually resolve totarget_url/200
.path
is alphanumeric only.public_dns
andpath
(this means supporting priority and regex for paths).