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/11/25 08:35:10 UTC
[GitHub] [apisix] monkeyDluffy6017 opened a new pull request, #8404: fix: the x-forwarded-* header will be influenced by ai plugin
monkeyDluffy6017 opened a new pull request, #8404:
URL: https://github.com/apache/apisix/pull/8404
### Description
Because the ai plugin will replace the `handle_upstream` function, and the `x-forwarded-*` header is set in `handle_upstream` function, so the `x-forwarded-*` header from downstream will be ignored.
Now move the function that set x-forwarded-* header outside the `handle_upstream` function.
### Checklist
- [x] I have explained the need for this PR and the problem it solves
- [x] I have explained the changes or the new features added to this PR
- [x] I have added tests corresponding to this change
- [ ] I have updated the documentation to reflect this change
- [x] 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)
--
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] monkeyDluffy6017 commented on a diff in pull request #8404: fix: the x-forwarded-* header will be influenced by ai plugin
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #8404:
URL: https://github.com/apache/apisix/pull/8404#discussion_r1034340773
##########
t/plugin/proxy-rewrite3.t:
##########
@@ -455,3 +454,51 @@ passed
GET /echo HTTP/1.1
--- response_headers
test: test_in_set
+
+
+
+=== TEST 21: set route (test if X-Forwarded-Port can be set before proxy)
+--- config
+ location /t {
Review Comment:
You could check this, the test case failed when the ai module is triggered in the ci env, but works fine locally. when i remove the ai module, the ci test passed.
https://github.com/apache/apisix/actions/runs/3421218504/jobs/5697063260
I think it maybe satisfy the condition in ci env.
https://github.com/apache/apisix/blob/master/apisix/plugins/ai.lua#L277
--
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] monkeyDluffy6017 commented on a diff in pull request #8404: fix: the x-forwarded-* header will be influenced by ai plugin
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #8404:
URL: https://github.com/apache/apisix/pull/8404#discussion_r1035558005
##########
t/plugin/proxy-rewrite3.t:
##########
@@ -455,3 +454,51 @@ passed
GET /echo HTTP/1.1
--- response_headers
test: test_in_set
+
+
+
+=== TEST 21: set route (test if X-Forwarded-Port can be set before proxy)
+--- config
+ location /t {
Review Comment:
ok, i'll figure it out
--
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 #8404: fix: the x-forwarded-* header will be influenced by ai plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8404:
URL: https://github.com/apache/apisix/pull/8404#discussion_r1035556505
##########
t/plugin/proxy-rewrite3.t:
##########
@@ -455,3 +454,51 @@ passed
GET /echo HTTP/1.1
--- response_headers
test: test_in_set
+
+
+
+=== TEST 21: set route (test if X-Forwarded-Port can be set before proxy)
+--- config
+ location /t {
Review Comment:
@monkeyDluffy6017
Interesting. The test case here uses `plugins`, which should disable the ai module:
https://github.com/apache/apisix/blob/bd6cbef5e53e7127d6393c8013d66c60a3c1677e/apisix/plugins/ai.lua#L260
So the test doesn't pass through the ai module in local environment.
IMHO, the ci env won't bring any difference as they are the same ai plugin.
Could you point out the reason why the same code acts differently in the ci env?
--
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] monkeyDluffy6017 closed pull request #8404: fix: the x-forwarded-* header will be influenced by ai plugin
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 closed pull request #8404: fix: the x-forwarded-* header will be influenced by ai plugin
URL: https://github.com/apache/apisix/pull/8404
--
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 #8404: fix: the x-forwarded-* header will be influenced by ai plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8404:
URL: https://github.com/apache/apisix/pull/8404#discussion_r1033436822
##########
t/plugin/proxy-rewrite3.t:
##########
@@ -455,3 +454,51 @@ passed
GET /echo HTTP/1.1
--- response_headers
test: test_in_set
+
+
+
+=== TEST 21: set route (test if X-Forwarded-Port can be set before proxy)
+--- config
+ location /t {
Review Comment:
I run the test locally and it can pass even without this bugfix.
--
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] monkeyDluffy6017 commented on a diff in pull request #8404: fix: the x-forwarded-* header will be influenced by ai plugin
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #8404:
URL: https://github.com/apache/apisix/pull/8404#discussion_r1033148245
##########
t/plugin/proxy-rewrite3.t:
##########
@@ -455,3 +454,51 @@ passed
GET /echo HTTP/1.1
--- response_headers
test: test_in_set
+
+
+
+=== TEST 21: set route (test if X-Forwarded-Port can be set before proxy)
Review Comment:
`var_x_forwarded_port` is set in `set_upstream_headers`, `set_upstream_headers` is called in `handle_upstream`, and `handle_upstream` is replaced in the ai module.
It will fail in the ci env if the ai module is triggered.
--
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] monkeyDluffy6017 commented on a diff in pull request #8404: fix: the x-forwarded-* header will be influenced by ai plugin
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #8404:
URL: https://github.com/apache/apisix/pull/8404#discussion_r1061302696
##########
t/plugin/proxy-rewrite3.t:
##########
@@ -455,3 +454,51 @@ passed
GET /echo HTTP/1.1
--- response_headers
test: test_in_set
+
+
+
+=== TEST 21: set route (test if X-Forwarded-Port can be set before proxy)
+--- config
+ location /t {
Review Comment:
@spacewander i can not reproduce the problem any more, even fallback to the old version, I will close this PR
--
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] monkeyDluffy6017 commented on a diff in pull request #8404: fix: the x-forwarded-* header will be influenced by ai plugin
Posted by GitBox <gi...@apache.org>.
monkeyDluffy6017 commented on code in PR #8404:
URL: https://github.com/apache/apisix/pull/8404#discussion_r1033471736
##########
t/plugin/proxy-rewrite3.t:
##########
@@ -455,3 +454,51 @@ passed
GET /echo HTTP/1.1
--- response_headers
test: test_in_set
+
+
+
+=== TEST 21: set route (test if X-Forwarded-Port can be set before proxy)
+--- config
+ location /t {
Review Comment:
Yes, the ai module is not triggered locally, try the test case in ci env
--
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 #8404: fix: the x-forwarded-* header will be influenced by ai plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8404:
URL: https://github.com/apache/apisix/pull/8404#discussion_r1033138149
##########
t/plugin/proxy-rewrite3.t:
##########
@@ -455,3 +454,51 @@ passed
GET /echo HTTP/1.1
--- response_headers
test: test_in_set
+
+
+
+=== TEST 21: set route (test if X-Forwarded-Port can be set before proxy)
Review Comment:
It seems the newly added test is not relative to the ai module? Can it pass without your modification?
--
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 #8404: fix: the x-forwarded-* header will be influenced by ai plugin
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8404:
URL: https://github.com/apache/apisix/pull/8404#discussion_r1034314871
##########
t/plugin/proxy-rewrite3.t:
##########
@@ -455,3 +454,51 @@ passed
GET /echo HTTP/1.1
--- response_headers
test: test_in_set
+
+
+
+=== TEST 21: set route (test if X-Forwarded-Port can be set before proxy)
+--- config
+ location /t {
Review Comment:
I am sorry, I can't get your point. This test contains route with plugins, it won't go through the ai module whether in the ci env or not.
--
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