-
Notifications
You must be signed in to change notification settings - Fork 170
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
Mapping Rule: Use unescaped rule #1123
Conversation
gateway/src/apicast/proxy.lua
Outdated
@@ -242,7 +242,7 @@ function _M:rewrite(service, context) | |||
return errors.no_credentials(service) | |||
end | |||
|
|||
local usage, matched_rules = service:get_usage(ngx.req.get_method(), ngx.var.uri) | |||
local usage, matched_rules = service:get_usage(ngx.req.get_method(), ngx.var.request_uri) |
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.
AFAIK $request_uri
includes arguments and $uri
doesn't.
The docs are explicit about $request_uri
, but not $uri
.
So, I think the query parameters should be stripped out from ngx.var.request_uri
first (e.g. ?q=query
).
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.
WIP PR, ngx.var.set_uri()
modified ngx.var.uri and no request.uri, so it's not valid at all ;-)
74211bd
to
259000f
Compare
18e82bd
to
2edffe0
Compare
ngx.escape functions are not valid to encode the URI, due to it also encodes characters like: /, ?, etc.. This commit uses the ngx_string.h ngx_escape function that it's what NGINX uses internally and provide the right setup for this use case. Signed-off-by: Eloy Coto <eloy.coto@gmail.com>
Signed-off-by: Eloy Coto <eloy.coto@gmail.com>
2edffe0
to
181f31a
Compare
This commit address the issues with the mapping rules that normalizes
the URL and mapping rules are not matching correctly.
Fix #1002
Fix THREESCALE-3468
Signed-off-by: Eloy Coto eloy.coto@gmail.com