ruanweihong-aaa commented on code in PR #14040:
URL: https://github.com/apache/apisix/pull/14040#discussion_r4421236352
##########
apisix/plugins/proxy-mirror.lua:
##########
@@ -91,8 +91,12 @@ end
local function enable_mirror(ctx, conf)
local uri = (ctx.var.upstream_uri and ctx.var.upstream_uri ~= "") and
- ctx.var.upstream_uri or
- ctx.var.uri .. ctx.var.is_args .. (ctx.var.args or '')
+ (ctx.servlet_upstream_uri and ctx.var.upstream_uri ==
ctx.servlet_upstream_uri and
Review Comment:
This has the same equality-check issue as the final URI assembly in
init.lua: it can re-append query arguments that proxy-rewrite explicitly
removed.
With normalize_uri_like_servlet: true, this configuration makes rewriting
run before mirroring:
{
"proxy-rewrite": {
"_meta": {
"priority": 1100
},
"use_real_request_uri_unsafe": true,
"regex_uri": ["^.*$", "/anything;jsessionid=x"]
},
"proxy-mirror": {
"host": "http://127.0.0.1:1981";,
"sample_ratio": 1
}
}
For /anything;jsessionid=x?foo=1, I verified the actual URI received by the
mirror service using the APISIX 3.19.0 container runtime with each source tree
mounted:
- Base 4a080d02: /anything;jsessionid=x.
- Head cfb07d44: /anything;jsessionid=x?foo=1.
With default priorities, mirroring runs before rewriting, and both versions
retain the query; that is not a new regression. The regression above occurs
when rewriting runs first.
Could you also preserve the explicit plugin URI override here and add a
regression test for this execution order?
##########
apisix/init.lua:
##########
@@ -1080,6 +1080,15 @@ function _M.http_access_phase()
end
span:finish(ngx_ctx)
+ if api_ctx.servlet_upstream_uri then
+ if api_ctx.var.upstream_uri == api_ctx.servlet_upstream_uri or
Review Comment:
Thanks for the update!
The equality check upstream_uri == servlet_upstream_uri cannot distinguish
an untouched URI from one explicitly rewritten to the same path without a query
string.
With normalize_uri_like_servlet: true and this configuration:
{
"proxy-rewrite": {
"use_real_request_uri_unsafe": true,
"regex_uri": ["^.*$", "/anything;jsessionid=x"]
}
}
For /anything;jsessionid=x?foo=1, proxy-rewrite explicitly sets the upstream
URI to /anything;jsessionid=x. This condition evaluates to true, and the
following concatenation re-appends ?foo=1 from var.is_args and var.args.
I verified this with real HTTP requests using the APISIX 3.19.0 container
runtime with each source tree mounted:
- Base 4a080d02: upstream receives /anything;jsessionid=x.
- Head cfb07d44: upstream receives /anything;jsessionid=x?foo=1.
Could you preserve the explicit plugin URI override and add a regression
test for this case, while keeping the existing hide_credentials tests passing?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
