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 2021/05/10 08:07:05 UTC
[GitHub] [apisix] spacewander opened a new pull request #4208: feat: support passing different host headers in multiple nodes
spacewander opened a new pull request #4208:
URL: https://github.com/apache/apisix/pull/4208
Fix #2620
Fix #4197
Signed-off-by: spacewander <sp...@gmail.com>
### What this PR does / why we need it:
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
### 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?
* [ ] Have you modified the corresponding document?
* [ ] Is this PR backward compatible? **If it is not backward compatible, please discuss on the [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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] tokers commented on a change in pull request #4208: feat: support passing different host headers in multiple nodes
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #4208:
URL: https://github.com/apache/apisix/pull/4208#discussion_r631522995
##########
File path: apisix/init.lua
##########
@@ -800,6 +801,14 @@ function _M.stream_preread_phase()
core.log.error("failed to set upstream: ", err)
return ngx_exit(1)
end
+
+ local server, err = load_balancer.pick_server(matched_route, api_ctx)
+ if not server then
+ core.log.error("failed to pick server: ", err)
+ return ngx_exit(1)
Review comment:
OK, I confused with the HTTP subsystem.
--
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] spacewander commented on a change in pull request #4208: feat: support passing different host headers in multiple nodes
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4208:
URL: https://github.com/apache/apisix/pull/4208#discussion_r631501476
##########
File path: t/node/upstream.t
##########
@@ -394,3 +394,119 @@ GET /t
{"error_msg":"can not delete this upstream, route [1] is still using it now"}
--- no_error_log
[error]
+
+
+
+=== TEST 17: multi nodes with `node` mode to pass host
+--- 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": {
+ "localhost:1979": 1000,
+ "127.0.0.1:1980": 1
Review comment:
1979 is not reachable. It is used to test that we will use a new host for the retry.
--
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] gxthrj commented on a change in pull request #4208: feat: support passing different host headers in multiple nodes
Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #4208:
URL: https://github.com/apache/apisix/pull/4208#discussion_r631055257
##########
File path: apisix/balancer.lua
##########
@@ -134,6 +145,30 @@ local function pick_server(route, ctx)
end
end
+ local retries = up_conf.retries
+ if not retries or retries < 0 then
+ retries = #up_conf.nodes - 1
Review comment:
Is it possible that nodes have a length of 0 ？
--
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] gxthrj merged pull request #4208: feat: support passing different host headers in multiple nodes
Posted by GitBox <gi...@apache.org>.
gxthrj merged pull request #4208:
URL: https://github.com/apache/apisix/pull/4208
--
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] tokers commented on a change in pull request #4208: feat: support passing different host headers in multiple nodes
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #4208:
URL: https://github.com/apache/apisix/pull/4208#discussion_r631511727
##########
File path: apisix/init.lua
##########
@@ -800,6 +801,14 @@ function _M.stream_preread_phase()
core.log.error("failed to set upstream: ", err)
return ngx_exit(1)
end
+
+ local server, err = load_balancer.pick_server(matched_route, api_ctx)
+ if not server then
+ core.log.error("failed to pick server: ", err)
+ return ngx_exit(1)
Review comment:
What does `ngx.exit(1)` mean?
##########
File path: apisix/balancer.lua
##########
@@ -98,16 +98,29 @@ local function create_server_picker(upstream, checker)
end
if picker then
+ local nodes = upstream.nodes
+ local addr_to_domain = {}
+ for _, node in ipairs(nodes) do
+ if node.domain then
+ local addr = node.host .. ":" .. node.port
+ addr_to_domain[addr] = node.domain
+ end
+ end
+
local up_nodes = fetch_health_nodes(upstream, checker)
if #up_nodes._priority_index > 1 then
core.log.info("upstream nodes: ", core.json.delay_encode(up_nodes))
- return priority_balancer.new(up_nodes, upstream, picker)
+ local server_picker = priority_balancer.new(up_nodes, upstream, picker)
Review comment:
Why not add a new parameter `addr_to_domain` for `priority_balancer.new`.
--
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] spacewander commented on a change in pull request #4208: feat: support passing different host headers in multiple nodes
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4208:
URL: https://github.com/apache/apisix/pull/4208#discussion_r631522639
##########
File path: apisix/init.lua
##########
@@ -800,6 +801,14 @@ function _M.stream_preread_phase()
core.log.error("failed to set upstream: ", err)
return ngx_exit(1)
end
+
+ local server, err = load_balancer.pick_server(matched_route, api_ctx)
+ if not server then
+ core.log.error("failed to pick server: ", err)
+ return ngx_exit(1)
Review comment:
To exit the stream phase.
--
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] gxthrj commented on a change in pull request #4208: feat: support passing different host headers in multiple nodes
Posted by GitBox <gi...@apache.org>.
gxthrj commented on a change in pull request #4208:
URL: https://github.com/apache/apisix/pull/4208#discussion_r631116345
##########
File path: t/node/upstream.t
##########
@@ -394,3 +394,119 @@ GET /t
{"error_msg":"can not delete this upstream, route [1] is still using it now"}
--- no_error_log
[error]
+
+
+
+=== TEST 17: multi nodes with `node` mode to pass host
+--- 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": {
+ "localhost:1979": 1000,
+ "127.0.0.1:1980": 1
Review comment:
I am not very familiar with test grammar.
With weight 1000:1, the request should hit localhost:1979.
In the Test 18 below, should host be `localhost`?
--
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] tokers commented on a change in pull request #4208: feat: support passing different host headers in multiple nodes
Posted by GitBox <gi...@apache.org>.
tokers commented on a change in pull request #4208:
URL: https://github.com/apache/apisix/pull/4208#discussion_r631523089
##########
File path: apisix/balancer.lua
##########
@@ -98,16 +98,29 @@ local function create_server_picker(upstream, checker)
end
if picker then
+ local nodes = upstream.nodes
+ local addr_to_domain = {}
+ for _, node in ipairs(nodes) do
+ if node.domain then
+ local addr = node.host .. ":" .. node.port
+ addr_to_domain[addr] = node.domain
+ end
+ end
+
local up_nodes = fetch_health_nodes(upstream, checker)
if #up_nodes._priority_index > 1 then
core.log.info("upstream nodes: ", core.json.delay_encode(up_nodes))
- return priority_balancer.new(up_nodes, upstream, picker)
+ local server_picker = priority_balancer.new(up_nodes, upstream, picker)
Review comment:
OK.
--
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] spacewander commented on pull request #4208: feat: support passing different host headers in multiple nodes
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #4208:
URL: https://github.com/apache/apisix/pull/4208#issuecomment-837575568
@membphis
We can do it in a separate 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix] spacewander commented on a change in pull request #4208: feat: support passing different host headers in multiple nodes
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #4208:
URL: https://github.com/apache/apisix/pull/4208#discussion_r631522296
##########
File path: apisix/balancer.lua
##########
@@ -98,16 +98,29 @@ local function create_server_picker(upstream, checker)
end
if picker then
+ local nodes = upstream.nodes
+ local addr_to_domain = {}
+ for _, node in ipairs(nodes) do
+ if node.domain then
+ local addr = node.host .. ":" .. node.port
+ addr_to_domain[addr] = node.domain
+ end
+ end
+
local up_nodes = fetch_health_nodes(upstream, checker)
if #up_nodes._priority_index > 1 then
core.log.info("upstream nodes: ", core.json.delay_encode(up_nodes))
- return priority_balancer.new(up_nodes, upstream, picker)
+ local server_picker = priority_balancer.new(up_nodes, upstream, picker)
Review comment:
The `addr_to_domain` isn't relative to the `priority_balancer`. We just package it with the balancer to avoid too many returned values.
--
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