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/09/30 08:49:06 UTC
[GitHub] [apisix] zhendongcmss opened a new pull request #5163: add hostheader
zhendongcmss opened a new pull request #5163:
URL: https://github.com/apache/apisix/pull/5163
### 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:
* [ ] Did you explain what problem does this PR solve? Or what new features have been added?
* [ ] 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.
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 #5163: fix: pass correct host header to health checker target nodes
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #5163:
URL: https://github.com/apache/apisix/pull/5163#issuecomment-937738584
@elvis-cai
What about this one?
https://github.com/apache/apisix/pull/5175
--
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] elvis-cai commented on pull request #5163: fix: pass correct host header to health checker target nodes
Posted by GitBox <gi...@apache.org>.
elvis-cai commented on pull request #5163:
URL: https://github.com/apache/apisix/pull/5163#issuecomment-937736847
thanks for fixing the issue I raised in https://github.com/apache/apisix/issues/4906,
did a quick test with this change, still using the same example route config from the issue, unfortunately, health check doesn't work as expected, adding some debug info, I found https://github.com/apache/apisix/pull/5163/files#diff-9af33577e041ece8fef32d8f8193f46eac83dab9e947a2cc940f36b83d5150e8R115 `hostheader` is always "httpbin.org", no matter upstream node domain is `apple.internal.com` or `banana.internal.com`.
another test I did is still the same line https://github.com/apache/apisix/pull/5163/files#diff-9af33577e041ece8fef32d8f8193f46eac83dab9e947a2cc940f36b83d5150e8R115, use `node.domain` could output the correct host header for health checking.
--
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 #5163: fix: pass correct host header to health checker target nodes
Posted by GitBox <gi...@apache.org>.
spacewander commented on pull request #5163:
URL: https://github.com/apache/apisix/pull/5163#issuecomment-937738584
@elvis-cai
What about this one?
https://github.com/apache/apisix/pull/5175
--
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] Qizhd commented on a change in pull request #5163: fix: pass correct host header to health checker target nodes
Posted by GitBox <gi...@apache.org>.
Qizhd commented on a change in pull request #5163:
URL: https://github.com/apache/apisix/pull/5163#discussion_r722014551
##########
File path: apisix/upstream.lua
##########
@@ -293,7 +293,7 @@ function _M.set_by_route(route, api_ctx)
end
if nodes_count > 1 then
- local checker = fetch_healthchecker(up_conf)
+ local checker = fetch_healthchecker(up_conf, api_ctx.var.host)
Review comment:
@spacewander ok, will update
--
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 change in pull request #5163: fix: pass correct host header to health checker target nodes
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5163:
URL: https://github.com/apache/apisix/pull/5163#discussion_r720665587
##########
File path: apisix/upstream.lua
##########
@@ -293,7 +293,7 @@ function _M.set_by_route(route, api_ctx)
end
if nodes_count > 1 then
- local checker = fetch_healthchecker(up_conf)
+ local checker = fetch_healthchecker(up_conf, api_ctx.var.host)
Review comment:
```suggestion
local checker = fetch_healthchecker(up_conf, api_ctx.var.upstream_host)
```
Not the one sent by client
--
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] elvis-cai commented on pull request #5163: fix: pass correct host header to health checker target nodes
Posted by GitBox <gi...@apache.org>.
elvis-cai commented on pull request #5163:
URL: https://github.com/apache/apisix/pull/5163#issuecomment-938218753
> @elvis-cai What about this one? #5175
Thanks @spacewander for the quick update, it works perfectly with this PR #5175 👍 , tested following scenarios:
- two healthy external urls with pass_host set to "node", it load balances to each of them.
- make one url to return 404, it could only return the healthy url.
- recover 404 url, it will load balance them again.
--
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] leslie-tsang commented on a change in pull request #5163: fix: add hostheader parameter to health checker target nodes
Posted by GitBox <gi...@apache.org>.
leslie-tsang commented on a change in pull request #5163:
URL: https://github.com/apache/apisix/pull/5163#discussion_r719349520
##########
File path: apisix/upstream.lua
##########
@@ -112,7 +112,7 @@ local function create_checker(upstream)
local host = upstream.checks and upstream.checks.active and upstream.checks.active.host
local port = upstream.checks and upstream.checks.active and upstream.checks.active.port
for _, node in ipairs(upstream.nodes) do
- local ok, err = checker:add_target(node.host, port or node.port, host)
+ local ok, err = checker:add_target(node.host, port or node.port, host, true, hostheader)
Review comment:
I am afraid this can't be done as easily as this. Ref to
https://github.com/Kong/lua-resty-healthcheck/blob/e1f2196faa7d57f2ad62fa5d0ed2b59beeb8396d/lib/resty/healthcheck.lua#L297-L311
```suggestion
local ok, err = checker:add_target(node.host, port or node.port, host, hostheader)
```
Maybe this will work as expected?
##########
File path: apisix/upstream.lua
##########
@@ -112,7 +112,7 @@ local function create_checker(upstream)
local host = upstream.checks and upstream.checks.active and upstream.checks.active.host
local port = upstream.checks and upstream.checks.active and upstream.checks.active.port
for _, node in ipairs(upstream.nodes) do
- local ok, err = checker:add_target(node.host, port or node.port, host)
+ local ok, err = checker:add_target(node.host, port or node.port, host, true, hostheader)
Review comment:
BTW, use `hostname` instead of `hostheader` will be better. func `add_target` third params original name is `hostname`.
--
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] leslie-tsang commented on a change in pull request #5163: fix: add hostheader parameter to health checker target nodes
Posted by GitBox <gi...@apache.org>.
leslie-tsang commented on a change in pull request #5163:
URL: https://github.com/apache/apisix/pull/5163#discussion_r719349520
##########
File path: apisix/upstream.lua
##########
@@ -112,7 +112,7 @@ local function create_checker(upstream)
local host = upstream.checks and upstream.checks.active and upstream.checks.active.host
local port = upstream.checks and upstream.checks.active and upstream.checks.active.port
for _, node in ipairs(upstream.nodes) do
- local ok, err = checker:add_target(node.host, port or node.port, host)
+ local ok, err = checker:add_target(node.host, port or node.port, host, true, hostheader)
Review comment:
I am afraid this can't be done as easily as this. Ref to
https://github.com/Kong/lua-resty-healthcheck/blob/e1f2196faa7d57f2ad62fa5d0ed2b59beeb8396d/lib/resty/healthcheck.lua#L297-L311
```suggestion
local ok, err = checker:add_target(node.host, port or node.port, host, hostheader)
```
Maybe this will work as expected?
##########
File path: apisix/upstream.lua
##########
@@ -112,7 +112,7 @@ local function create_checker(upstream)
local host = upstream.checks and upstream.checks.active and upstream.checks.active.host
local port = upstream.checks and upstream.checks.active and upstream.checks.active.port
for _, node in ipairs(upstream.nodes) do
- local ok, err = checker:add_target(node.host, port or node.port, host)
+ local ok, err = checker:add_target(node.host, port or node.port, host, true, hostheader)
Review comment:
BTW, use `hostname` instead of `hostheader` will be better. func `add_target` third params original name is `hostname`.
--
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 change in pull request #5163: fix: pass correct host header to health checker target nodes
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5163:
URL: https://github.com/apache/apisix/pull/5163#discussion_r724128784
##########
File path: apisix/upstream.lua
##########
@@ -293,7 +293,7 @@ function _M.set_by_route(route, api_ctx)
end
if nodes_count > 1 then
- local checker = fetch_healthchecker(up_conf)
+ local checker = fetch_healthchecker(up_conf, api_ctx.var.host)
Review comment:
I am sorry, the solution I gave is incorrect. I tried to make a new one here: https://github.com/apache/apisix/pull/5175
--
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] zhendongcmss commented on pull request #5163: fix: pass correct host header to health checker target nodes
Posted by GitBox <gi...@apache.org>.
zhendongcmss commented on pull request #5163:
URL: https://github.com/apache/apisix/pull/5163#issuecomment-938268284
@spacewander 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] spacewander commented on a change in pull request #5163: fix: pass correct host header to health checker target nodes
Posted by GitBox <gi...@apache.org>.
spacewander commented on a change in pull request #5163:
URL: https://github.com/apache/apisix/pull/5163#discussion_r724128784
##########
File path: apisix/upstream.lua
##########
@@ -293,7 +293,7 @@ function _M.set_by_route(route, api_ctx)
end
if nodes_count > 1 then
- local checker = fetch_healthchecker(up_conf)
+ local checker = fetch_healthchecker(up_conf, api_ctx.var.host)
Review comment:
I am sorry, the solution I gave is incorrect. I tried to make a new one here: https://github.com/apache/apisix/pull/5175
--
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] elvis-cai commented on pull request #5163: fix: pass correct host header to health checker target nodes
Posted by GitBox <gi...@apache.org>.
elvis-cai commented on pull request #5163:
URL: https://github.com/apache/apisix/pull/5163#issuecomment-937736847
thanks for fixing the issue I raised in https://github.com/apache/apisix/issues/4906,
did a quick test with this change, still using the same example route config from the issue, unfortunately, health check doesn't work as expected, adding some debug info, I found https://github.com/apache/apisix/pull/5163/files#diff-9af33577e041ece8fef32d8f8193f46eac83dab9e947a2cc940f36b83d5150e8R115 `hostheader` is always "httpbin.org", no matter upstream node domain is `apple.internal.com` or `banana.internal.com`.
another test I did is still the same line https://github.com/apache/apisix/pull/5163/files#diff-9af33577e041ece8fef32d8f8193f46eac83dab9e947a2cc940f36b83d5150e8R115, use `node.domain` could output the correct host header for health checking.
--
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] zhendongcmss closed pull request #5163: fix: pass correct host header to health checker target nodes
Posted by GitBox <gi...@apache.org>.
zhendongcmss closed pull request #5163:
URL: https://github.com/apache/apisix/pull/5163
--
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