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 2020/08/29 15:39:43 UTC
[GitHub] [apisix] nic-chen opened a new pull request #2132: feat: support different modes to pass host to upstream
nic-chen opened a new pull request #2132:
URL: https://github.com/apache/apisix/pull/2132
### What this PR does / why we need it:
support different modes to pass host to upstream
### Pre-submission checklist:
* [x] Did you explain what problem does this PR solve? Or what new features have been added?
* [x] Have you added corresponding test cases?
* [x] Have you modified the corresponding document?
* [x] Is this PR backward compatible?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] juzhiyuan commented on a change in pull request #2132: feat: support different modes to pass host to upstream
Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on a change in pull request #2132:
URL: https://github.com/apache/apisix/pull/2132#discussion_r479698324
##########
File path: apisix/init.lua
##########
@@ -297,6 +300,32 @@ local function return_direct(...)
end
+local function set_upstream_host(api_ctx)
+ local pass_host = api_ctx.pass_host
+ if pass_host and pass_host ~= "pass" then
Review comment:
How about check pass_host directly? Because there has no logics to handle the case when pass_host doesn’t exist 🤔
```lua
local host
If pass_host == “xx” then
xxx
If pass_host == “xx” then
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2132: feat: support different modes to pass host to upstream
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2132:
URL: https://github.com/apache/apisix/pull/2132#discussion_r479778454
##########
File path: doc/admin-api.md
##########
@@ -504,6 +504,9 @@ In addition to the basic complex equalization algorithm selection, APISIX's Upst
|timeout|optional| Set the timeout for connection, sending and receiving messages. |
|name |optional|Identifies upstream names|
|desc |optional|upstream usage scenarios, and more.|
+|pass_host |optional|`pass` pass the client request host, `node` not pass the client request host, using the upstream node host, `rewrite` rewrite host by the configured `upstream_host`.|
Review comment:
> `pass` pass the client request host
that is the default value, we should write this to the doc.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2132: feat: support different modes to pass host to upstream
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2132:
URL: https://github.com/apache/apisix/pull/2132#discussion_r479717054
##########
File path: apisix/init.lua
##########
@@ -297,6 +298,35 @@ local function return_direct(...)
end
+local function set_upstream_host(api_ctx)
+ local pass_host = api_ctx.pass_host
+ if pass_host and pass_host ~= "pass" then
Review comment:
bad style, we should return asap. here is an example:
```lua
local pass_host = api_ctx.pass_host or "pass"
if pass_host == "pass" then
return
end
if pass_host == "rewrite" then
... ...
return
end
-- node model
... ...
return
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis merged pull request #2132: feat: support different modes to pass host to upstream
Posted by GitBox <gi...@apache.org>.
membphis merged pull request #2132:
URL: https://github.com/apache/apisix/pull/2132
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2132: feat: support different modes to pass host to upstream
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2132:
URL: https://github.com/apache/apisix/pull/2132#discussion_r479747784
##########
File path: apisix/admin/upstreams.lua
##########
@@ -112,6 +117,7 @@ local function check_conf(id, conf, need_id)
core.log.info("conf : ", core.json.delay_encode(conf))
local ok, err = check_upstream_conf(conf)
+
Review comment:
we need to delete this blank line
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] nic-chen commented on pull request #2132: feat: support different modes to pass host to upstream
Posted by GitBox <gi...@apache.org>.
nic-chen commented on pull request #2132:
URL: https://github.com/apache/apisix/pull/2132#issuecomment-683306603
will update docs later.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2132: feat: support different modes to pass host to upstream
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2132:
URL: https://github.com/apache/apisix/pull/2132#discussion_r479735530
##########
File path: apisix/admin/upstreams.lua
##########
@@ -80,7 +80,7 @@ local function check_upstream_conf(conf)
-- only support single node for `node` mode currently
if conf.pass_host == "node" and conf.nodes and #conf.nodes > 1 then
Review comment:
I think `#conf.nodes ~= 1` is better for current case
##########
File path: apisix/admin/upstreams.lua
##########
@@ -80,7 +80,7 @@ local function check_upstream_conf(conf)
-- only support single node for `node` mode currently
if conf.pass_host == "node" and conf.nodes and #conf.nodes > 1 then
- return false, "only support single node for `node` mode currently."
+ return false, "only support single node for `node` mode currently"
Review comment:
need test case
##########
File path: apisix/init.lua
##########
@@ -299,34 +299,38 @@ end
local function set_upstream_host(api_ctx)
- local pass_host = api_ctx.pass_host
- if pass_host and pass_host ~= "pass" then
- local host
- -- only support single node for `node` mode currently
- if pass_host == "node" then
- core.log.info("upstream host mod: node")
- local up_conf = api_ctx.upstream_conf
- local nodes_count = up_conf.nodes and #up_conf.nodes or 0
- if nodes_count == 1 then
- local node = up_conf.nodes[1]
- if node.domain and #node.domain > 0 then
- host = node.domain
- else
- host = node.host
- end
- end
- elseif pass_host == "rewrite" then
- core.log.info("upstream host mod: rewrite")
- host = api_ctx.upstream_host and api_ctx.upstream_host
- end
+ local pass_host = api_ctx.pass_host or "pass"
+ if pass_host == "pass" then
+ return
+ end
+ local host
+ if pass_host == "rewrite" then
Review comment:
this style is simpler.
```lua
if pass_host == "rewrite" then
api_ctx.var.upstream_host = api_ctx.upstream_host
retturn
end
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2132: feat: support different modes to pass host to upstream
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2132:
URL: https://github.com/apache/apisix/pull/2132#discussion_r479716656
##########
File path: apisix/admin/upstreams.lua
##########
@@ -77,6 +77,12 @@ local function check_upstream_conf(conf)
return false, "invalid configuration: " .. err
end
end
+
+ -- only support single node for `node` mode currently
+ if conf.pass_host == "node" and conf.nodes and #conf.nodes > 1 then
+ return false, "only support single node for `node` mode currently."
Review comment:
remove dot
`"only support single node for `node` mode currently"`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2132: feat: support different modes to pass host to upstream
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2132:
URL: https://github.com/apache/apisix/pull/2132#discussion_r479717762
##########
File path: t/node/upstream.t
##########
@@ -262,3 +262,150 @@ GET /t
[delete] code: 404
--- no_error_log
[error]
+
+
+
+=== TEST 11: set upstream(id: 1) nodes
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/upstreams/1',
+ ngx.HTTP_PUT,
+ [[{
+ "nodes": {
+ "httpbin.org:80": 1
+ },
+ "type": "roundrobin",
+ "desc": "new upstream",
+ "pass_host": "node"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 12: set route(id: 1)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "uri": "/get",
+ "upstream_id": "1"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 13: hit route
+--- request
+GET /get
+--- error_log
+upstream host mod: node
+set upstream host: httpbin.org
+--- response_body eval
+qr/"Host": "httpbin.org"/
+--- no_error_log
+[error]
+
+
+
+=== TEST 14: set upstream(id: 1) nodes
Review comment:
better name
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2132: feat: support different modes to pass host to upstream
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2132:
URL: https://github.com/apache/apisix/pull/2132#discussion_r479779242
##########
File path: doc/admin-api.md
##########
@@ -504,6 +504,9 @@ In addition to the basic complex equalization algorithm selection, APISIX's Upst
|timeout|optional| Set the timeout for connection, sending and receiving messages. |
|name |optional|Identifies upstream names|
|desc |optional|upstream usage scenarios, and more.|
+|pass_host |optional|`pass` pass the client request host, `node` not pass the client request host, using the upstream node host, `rewrite` rewrite host by the configured `upstream_host`.|
Review comment:
I have created an issue about this: https://github.com/apache/apisix/issues/2134
feel free to fix 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] membphis commented on a change in pull request #2132: feat: support different modes to pass host to upstream
Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #2132:
URL: https://github.com/apache/apisix/pull/2132#discussion_r479717108
##########
File path: apisix/init.lua
##########
@@ -391,6 +421,7 @@ function _M.http_access_phase()
local enable_websocket
local up_id = route.value.upstream_id
if up_id then
+
Review comment:
we do not need this blank line
##########
File path: apisix/init.lua
##########
@@ -429,6 +460,10 @@ function _M.http_access_phase()
if upstream.value.enable_websocket then
enable_websocket = true
end
+ if upstream.value.pass_host then
Review comment:
need one blank line before `if ... then`
##########
File path: apisix/init.lua
##########
@@ -455,6 +490,10 @@ function _M.http_access_phase()
if route.value.upstream and route.value.upstream.enable_websocket then
enable_websocket = true
end
+ if route.value.upstream and route.value.upstream.pass_host then
Review comment:
ditto
##########
File path: t/node/route-domain.t
##########
@@ -80,3 +80,95 @@ hello world
[error]
--- error_log eval
qr/dns resolver domain: baidu.com to \d+.\d+.\d+.\d+/
+
+
+
+=== TEST 4: set route(id: 1)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin",
+ "pass_host": "rewrite",
+ "upstream_host": "apache.org"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 5: hit routes
+--- request
+GET /hello
+--- error_log
+upstream host mod: rewrite
+set upstream host: apache.org
+--- no_error_log
+[error]
+
+
+
+=== TEST 6: set route(id: 1)
Review comment:
needs a better name too
##########
File path: t/node/route-domain.t
##########
@@ -80,3 +80,95 @@ hello world
[error]
--- error_log eval
qr/dns resolver domain: baidu.com to \d+.\d+.\d+.\d+/
+
+
+
+=== TEST 4: set route(id: 1)
Review comment:
we need a better title
##########
File path: t/node/route-domain.t
##########
@@ -80,3 +80,95 @@ hello world
[error]
--- error_log eval
qr/dns resolver domain: baidu.com to \d+.\d+.\d+.\d+/
+
+
+
+=== TEST 4: set route(id: 1)
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/routes/1',
+ ngx.HTTP_PUT,
+ [[{
+ "upstream": {
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin",
+ "pass_host": "rewrite",
+ "upstream_host": "apache.org"
+ },
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- request
+GET /t
+--- response_body
+passed
+--- no_error_log
+[error]
+
+
+
+=== TEST 5: hit routes
Review comment:
change to `hit route` is better, only one route here
##########
File path: t/node/upstream.t
##########
@@ -262,3 +262,150 @@ GET /t
[delete] code: 404
--- no_error_log
[error]
+
+
+
+=== TEST 11: set upstream(id: 1) nodes
Review comment:
need a better name
##########
File path: apisix/schema_def.lua
##########
@@ -332,6 +332,12 @@ local upstream_schema = {
description = "enable websocket for request",
type = "boolean"
},
+ pass_host = {
+ description = "mod of host passing",
+ type = "string",
+ enum = {"pass", "node", "rewrite"}
Review comment:
set default value `pass`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org