You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/06/23 12:46:31 UTC
[GitHub] [apisix] panhow opened a new pull request, #7317: bugfix: when both route and global rules setted proxy-rewrite plugin,…
panhow opened a new pull request, #7317:
URL: https://github.com/apache/apisix/pull/7317
when proxy-rewrite plugin is both set in route and global rule, uri rewrite won't work, cause `ctx.var.upstream_uri` will only sync once to `ngx.var.upstream_uri`
### Description
<!-- Please include a summary of the change and which issue is fixed. -->
<!-- Please also include relevant motivation and context. -->
Fixes # (issue)
How to reproduce it?
```bash
# create a global_rules
curl 127.0.0.1:8081/apisix/admin/global_rules/proxy_set_header -H 'x-api-key: xxx' -XPUT \
-d '{"plugins":{"proxy-rewrite":{"headers":{"bb": "cc"}}}}'
# create a route for debug
curl 127.0.0.1:8081/apisix/admin/routes/proxy-rewrite-test -H 'x-api-key: xxx' -XPUT -d '{
"uri": "/server/*",
"host": "apisix.debug",
"plugins": {
"proxy-rewrite":{
"regex_uri": [
"/server/(.*)",
"/$1"
]
}
},
"upstream": {
"type": "roundrobin",
"nodes": {
"127.0.0.1:7777": 1
}
}
}'
# run an http server
python -m SimpleHTTPServer 7777 &
# send a requests
curl 127.0.0.1/server/123123 -H 'host: apisix.debug'
```
```bash
# expect access log
/123123
# actual log
/server/123123
```
```bash
# remove global rules, everything works fine
curl 127.0.0.1:8081/apisix/admin/global_rules/proxy_set_header -H 'x-api-key: xxx' -DELETE
curl 127.0.0.1/server/123123 -H 'host: apisix.debug'
```
### Checklist
- [ ] I have explained the need for this PR and the problem it solves
- [ ] I have explained the changes or the new features added to this PR
- [ ] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [ ] I have verified that this change is backward compatible (If not, please discuss on the [APISIX mailing list](https://github.com/apache/apisix/tree/master#community) first)
<!--
Note
1. Mark the PR as draft until it's ready to be reviewed.
2. Always add/update tests for any changes unless you have a good reason.
3. Always update the documentation to reflect the changes made in the PR.
4. Make a new commit to resolve conversations instead of `push -f`.
5. To resolve merge conflicts, merge master instead of rebasing.
6. Use "request review" to notify the reviewer after making changes.
7. Only a reviewer can mark a conversation as resolved.
-->
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Jary-mf commented on a diff in pull request #7317: bugfix: when both route and global rules setted proxy-rewrite plugin,…
Posted by GitBox <gi...@apache.org>.
Jary-mf commented on code in PR #7317:
URL: https://github.com/apache/apisix/pull/7317#discussion_r905753481
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -185,6 +185,7 @@ function _M.rewrite(conf, ctx)
upstream_uri = core.utils.uri_safe_encode(upstream_uri)
end
+ ctx.var.upstream_uri = nil
Review Comment:
maybe ctx.var.upstream_uri = nil is fine ? because there will be unconditionally assigned to ctx.var.upstream_uri again。see lines 189 to 197
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Jary-mf commented on pull request #7317: bugfix: when both route and global rules setted proxy-rewrite plugin,…
Posted by GitBox <gi...@apache.org>.
Jary-mf commented on PR #7317:
URL: https://github.com/apache/apisix/pull/7317#issuecomment-1165246942
BTW，although there is no uri or regex_uri configuration in global_rules, the code logic still affects upstream_uri, which seems a bit unreasonable :)
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #7317: bugfix: when both route and global rules setted proxy-rewrite plugin,…
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7317:
URL: https://github.com/apache/apisix/pull/7317#issuecomment-1165279270
> BTW，although there is no uri or regex_uri configuration in global_rules, the code logic still affects upstream_uri, which seems a bit unreasonable :)
We submit another PR to solve it.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] github-actions[bot] commented on pull request #7317: bugfix: when both route and global rules setted proxy-rewrite plugin,…
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #7317:
URL: https://github.com/apache/apisix/pull/7317#issuecomment-1259278638
This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] withlin commented on pull request #7317: bugfix: when both route and global rules setted proxy-rewrite plugin,…
Posted by GitBox <gi...@apache.org>.
withlin commented on PR #7317:
URL: https://github.com/apache/apisix/pull/7317#issuecomment-1164374859
@tokers @spacewander ping
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7317: bugfix: when both route and global rules setted proxy-rewrite plugin,…
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7317:
URL: https://github.com/apache/apisix/pull/7317#discussion_r905789857
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -185,6 +185,7 @@ function _M.rewrite(conf, ctx)
upstream_uri = core.utils.uri_safe_encode(upstream_uri)
end
+ ctx.var.upstream_uri = nil
Review Comment:
> @panhow We need some test cast cases to verify the changes are effective.
@panhow
You can add the test in https://github.com/apache/apisix/blob/master/t/plugin/proxy-rewrite3.t.
You can refer to https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md to write and run the test locally.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tzssangglass commented on a diff in pull request #7317: bugfix: when both route and global rules setted proxy-rewrite plugin,…
Posted by GitBox <gi...@apache.org>.
tzssangglass commented on code in PR #7317:
URL: https://github.com/apache/apisix/pull/7317#discussion_r905660999
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -185,6 +185,7 @@ function _M.rewrite(conf, ctx)
upstream_uri = core.utils.uri_safe_encode(upstream_uri)
end
+ ctx.var.upstream_uri = nil
Review Comment:
If this line is added, it seems that `upstream_uri` will be set to nil whenever the `proxy-rewrite` plugin is used.
It doesn't look like a good fix. We should consider multiple scenarios.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] github-actions[bot] closed pull request #7317: bugfix: when both route and global rules setted proxy-rewrite plugin,…
Posted by GitBox <gi...@apache.org>.
github-actions[bot] closed pull request #7317: bugfix: when both route and global rules setted proxy-rewrite plugin,…
URL: https://github.com/apache/apisix/pull/7317
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] Jary-mf commented on pull request #7317: bugfix: when both route and global rules setted proxy-rewrite plugin,…
Posted by GitBox <gi...@apache.org>.
Jary-mf commented on PR #7317:
URL: https://github.com/apache/apisix/pull/7317#issuecomment-1164372784
nice!
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a diff in pull request #7317: bugfix: when both route and global rules setted proxy-rewrite plugin,…
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #7317:
URL: https://github.com/apache/apisix/pull/7317#discussion_r905784885
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -185,6 +185,7 @@ function _M.rewrite(conf, ctx)
upstream_uri = core.utils.uri_safe_encode(upstream_uri)
end
+ ctx.var.upstream_uri = nil
Review Comment:
The fix LGTM ( I can't give a better fix yet). Would be better to add a comment that we purge the cache by assigning a nil value.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] github-actions[bot] commented on pull request #7317: bugfix: when both route and global rules setted proxy-rewrite plugin,…
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #7317:
URL: https://github.com/apache/apisix/pull/7317#issuecomment-1231449897
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@apisix.apache.org list. Thank you for your contributions.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] panhow commented on a diff in pull request #7317: bugfix: when both route and global rules setted proxy-rewrite plugin,…
Posted by GitBox <gi...@apache.org>.
panhow commented on code in PR #7317:
URL: https://github.com/apache/apisix/pull/7317#discussion_r905756805
##########
apisix/plugins/proxy-rewrite.lua:
##########
@@ -185,6 +185,7 @@ function _M.rewrite(conf, ctx)
upstream_uri = core.utils.uri_safe_encode(upstream_uri)
end
+ ctx.var.upstream_uri = nil
Review Comment:
it DOES NOT a good fix, but works
actually there are two problems to fix:
1. `proxy-rewrite` should not set `ctx.var.upstream_uri` when the `uri` or `regex_uri` isn't set
2. `ngx.var.xxx` can be set once at the time when `ctx.var.xxx` is first assign, after the metamthod `__newIndex` run , other assignment won't pass to the `ngx.var.xxx`
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on pull request #7317: bugfix: when both route and global rules setted proxy-rewrite plugin,…
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #7317:
URL: https://github.com/apache/apisix/pull/7317#issuecomment-1165284239
> @panhow We need some test cast cases to verify the changes are effective.
@panhow
You can add the test in https://github.com/apache/apisix/blob/master/t/plugin/proxy-rewrite3.t.
You can refer to https://github.com/apache/apisix/blob/master/docs/en/latest/internal/testing-framework.md to write and run the test locally.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on pull request #7317: bugfix: when both route and global rules setted proxy-rewrite plugin,…
Posted by GitBox <gi...@apache.org>.
tokers commented on PR #7317:
URL: https://github.com/apache/apisix/pull/7317#issuecomment-1164385948
@panhow We need some test cast cases to verify the changes are effective.
--
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: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org