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/01 13:00:00 UTC
[GitHub] [apisix] biakecw opened a new pull request, #8226: allow customize x_Forwarded_Port while handling upstream headers
biakecw opened a new pull request, #8226:
URL: https://github.com/apache/apisix/pull/8226
### Description
Before this time, the x-Forward-* related settings in proxy rewrite will be overwritten by the default values in the nginx context. Add simple code to allow setting x-forward-port header in handle_upstream stage
Fixes #4942
### 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] biakecw commented on a diff in pull request #8226: feat: allow customize x_Forwarded_Port while handling upstream headers
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8226:
URL: https://github.com/apache/apisix/pull/8226#discussion_r1012564833
##########
t/plugin/proxy-rewrite2.t:
##########
@@ -207,3 +207,26 @@ X-Forwarded-Proto: http
X-Forwarded-Proto: grpc
--- response_headers
X-Forwarded-Proto: http
+
+=== TEST 7: customize X-Forwarded-Port
+--- apisix_yaml
+routes:
+ -
+ id: 1
+ uri: /echo
+ plugins:
+ proxy-rewrite:
+ headers:
+ X-Forwarded-Port: 10080
+ upstream_id: 1
+upstreams:
+ -
+ id: 1
+ nodes:
+ "127.0.0.1:1980": 1
+ type: roundrobin
+#END
+--- request
+GET /echo
Review Comment:
Whether two different case are required, one is the X-Forwarded-Port header in original request, another is in plugin proxy-rewrite.
--
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 merged pull request #8226: feat: allow customize x_Forwarded_Port while handling upstream headers
Posted by GitBox <gi...@apache.org>.
spacewander merged PR #8226:
URL: https://github.com/apache/apisix/pull/8226
--
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 #8226: feat: allow customize x_Forwarded_Port while handling upstream headers
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8226:
URL: https://github.com/apache/apisix/pull/8226#discussion_r1011250501
##########
apisix/init.lua:
##########
@@ -258,6 +258,10 @@ local function set_upstream_headers(api_ctx, picked_server)
if proto then
api_ctx.var.var_x_forwarded_proto = proto
end
+ local port = api_ctx.var.http_x_forwarded_port
Review Comment:
Some extra changes are required. See https://github.com/apache/apisix/pull/8200/files
--
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 #8226: feat: allow customize x_Forwarded_Port while handling upstream headers
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8226:
URL: https://github.com/apache/apisix/pull/8226#discussion_r1013727905
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -677,8 +677,8 @@ http {
if ($http_x_forwarded_for != "") {
set $var_x_forwarded_for "${http_x_forwarded_for}, ${realip_remote_addr}";
}
- if ($http_x_forwarded_port != "") {
- set $var_x_forwarded_port $http_x_forwarded_port;
+ if ($http_x_forwarded_host != "") {
+ set $var_x_forwarded_host $http_x_forwarded_host;
Review Comment:
Do we need to add the removed directive back?
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -69,10 +69,10 @@ lua {
{% if enabled_stream_plugins["prometheus"] and not enable_http then %}
http {
lua_package_path "{*extra_lua_path*}$prefix/deps/share/lua/5.1/?.lua;$prefix/deps/share/lua/5.1/?/init.lua;]=]
- .. [=[{*apisix_lua_home*}/?.lua;{*apisix_lua_home*}/?/init.lua;;{*lua_path*};";
+ .. [=[{*apisix_lua_home*}/?.lua;{*apisix_lua_home*}/?/init.lua;;{*lua_path*};";
Review Comment:
Could you disable the auto-format in your editor? This feature breaks a lot of indentations.
##########
apisix/init.lua:
##########
@@ -250,22 +243,22 @@ local function set_upstream_host(api_ctx, picked_server)
api_ctx.var.upstream_host = picked_server.upstream_host
end
-
local function set_upstream_headers(api_ctx, picked_server)
set_upstream_host(api_ctx, picked_server)
-
local proto = api_ctx.var.http_x_forwarded_proto
if proto then
api_ctx.var.var_x_forwarded_proto = proto
end
-
Review Comment:
Please keep the original blank lines
##########
t/plugin/proxy-rewrite2.t:
##########
@@ -207,3 +207,31 @@ X-Forwarded-Proto: http
X-Forwarded-Proto: grpc
--- response_headers
X-Forwarded-Proto: http
+
+
+
+=== TEST 7: customize X-Forwarded-Port
+--- apisix_yaml
+routes:
+ -
+ id: 1
+ uri: /echo
+ plugins:
+ proxy-rewrite:
+ headers:
+ X-Forwarded-Port: 10080
+ upstream_id: 1
+upstreams:
+ -
+ id: 1
+ nodes:
+ "127.0.0.1:1980": 1
+ type: roundrobin
+#END
+--- request
+GET /echo
+--- more_headers
+X-Forwarded-Port: 8080
+--- response_headers
+X-Forwarded-Proto: 10080
Review Comment:
```suggestion
X-Forwarded-Port: 10080
```
##########
apisix/init.lua:
##########
@@ -250,22 +243,22 @@ local function set_upstream_host(api_ctx, picked_server)
api_ctx.var.upstream_host = picked_server.upstream_host
end
-
local function set_upstream_headers(api_ctx, picked_server)
set_upstream_host(api_ctx, picked_server)
-
local proto = api_ctx.var.http_x_forwarded_proto
if proto then
api_ctx.var.var_x_forwarded_proto = proto
end
-
+ local port = api_ctx.var.http_x_forwarded_port
+ if port then
+ api_ctx.var.var_x_forwarded_port = port
+ end
Review Comment:
Better to add a blank line between the `if end` blocks.
--
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 #8226: feat: allow customize x_Forwarded_Port while handling upstream headers
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8226:
URL: https://github.com/apache/apisix/pull/8226#discussion_r1014813372
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -677,9 +677,7 @@ http {
if ($http_x_forwarded_for != "") {
set $var_x_forwarded_for "${http_x_forwarded_for}, ${realip_remote_addr}";
}
- if ($http_x_forwarded_port != "") {
- set $var_x_forwarded_port $http_x_forwarded_port;
- }
+
Review Comment:
Let's remove unexpected trailing whitespace
--
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] biakecw commented on a diff in pull request #8226: feat: allow customize x_Forwarded_Port while handling upstream headers
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8226:
URL: https://github.com/apache/apisix/pull/8226#discussion_r1011296821
##########
apisix/init.lua:
##########
@@ -258,6 +258,10 @@ local function set_upstream_headers(api_ctx, picked_server)
if proto then
api_ctx.var.var_x_forwarded_proto = proto
end
+ local port = api_ctx.var.http_x_forwarded_port
Review Comment:
Code has been updated,thx.
--
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 #8226: feat: allow customize x_Forwarded_Port while handling upstream headers
Posted by GitBox <gi...@apache.org>.
spacewander commented on PR #8226:
URL: https://github.com/apache/apisix/pull/8226#issuecomment-1304779164
> the error in Code Lint : =====bad style===== reindex: t/plugin/proxy-rewrite2.t: done.
>
> I'm out of ideas, asking for guidance.
You can download reindex and run it in the test file. For your info: https://github.com/apache/apisix/blob/master/CONTRIBUTING.md#check-code-style-and-test-case-style
--
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] biakecw commented on a diff in pull request #8226: feat: allow customize x_Forwarded_Port while handling upstream headers
Posted by GitBox <gi...@apache.org>.
biakecw commented on code in PR #8226:
URL: https://github.com/apache/apisix/pull/8226#discussion_r1013745781
##########
apisix/cli/ngx_tpl.lua:
##########
@@ -677,8 +677,8 @@ http {
if ($http_x_forwarded_for != "") {
set $var_x_forwarded_for "${http_x_forwarded_for}, ${realip_remote_addr}";
}
- if ($http_x_forwarded_port != "") {
- set $var_x_forwarded_port $http_x_forwarded_port;
+ if ($http_x_forwarded_host != "") {
+ set $var_x_forwarded_host $http_x_forwarded_host;
Review Comment:
My understanding is that this directive is already implemented in apisix.http_access_phase() and is redundant here. Is my understanding correct? If not, I am not sure what you mean " Some extra changes are required. See https://github.com/apache/apisix/pull/8200/files" 2 days ago.
--
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 #8226: feat: allow customize x_Forwarded_Port while handling upstream headers
Posted by GitBox <gi...@apache.org>.
spacewander commented on code in PR #8226:
URL: https://github.com/apache/apisix/pull/8226#discussion_r1012540225
##########
t/plugin/proxy-rewrite2.t:
##########
@@ -207,3 +207,26 @@ X-Forwarded-Proto: http
X-Forwarded-Proto: grpc
--- response_headers
X-Forwarded-Proto: http
+
+=== TEST 7: customize X-Forwarded-Port
+--- apisix_yaml
+routes:
+ -
+ id: 1
+ uri: /echo
+ plugins:
+ proxy-rewrite:
+ headers:
+ X-Forwarded-Port: 10080
+ upstream_id: 1
+upstreams:
+ -
+ id: 1
+ nodes:
+ "127.0.0.1:1980": 1
+ type: roundrobin
+#END
+--- request
+GET /echo
Review Comment:
Would you fix the lint: https://github.com/apache/apisix/actions/runs/3375534175/jobs/5602706983 and update the test to send a X-Forwarded-Port header like https://github.com/apache/apisix/blob/03e7606b6fd0fafa707d9ada6d6318c2e5c30bd8/t/plugin/proxy-rewrite3.t#L334?
--
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] biakecw commented on pull request #8226: feat: allow customize x_Forwarded_Port while handling upstream headers
Posted by GitBox <gi...@apache.org>.
biakecw commented on PR #8226:
URL: https://github.com/apache/apisix/pull/8226#issuecomment-1303784106
the error in Code Lint :
=====bad style=====
reindex: t/plugin/proxy-rewrite2.t: done.
I'm out of ideas, asking for guidance.
--
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