fix: handle consul nil port cases by defaulting to port 80 #12304
Conversation
dosubot
bot
added
size:M
|
|
||
| ::RETRY:: | ||
| local watch_result, watch_err = client:get(consul_server.consul_watch_catalog_url) | ||
| local watch_error_info = (watch_err ~= nil and watch_err) |
There was a problem hiding this comment.
shorter is clearer, you can take a look. if you would like my way, you can make a try
local res, err = client:get(consul_server.consul_watch_catalog_url)
err = err or (res and res.status ~= 200 and res.status)
|
|
||
| local svc_address, svc_port = node.Service.Address, node.Service.Port | ||
| -- Handle nil or 0 port case - default to 80 for HTTP services | ||
| if not svc_port or svc_port == 0 then |
There was a problem hiding this comment.
TCP and UDP ports can be 0.
Although it stands for "special use", it should be clearly distinguished from port is null. If this value is explicitly configured as 0, then we should accept it and not change it.
There was a problem hiding this comment.
We have verified that nginx/openresty does not allow 0 as a port, even though TCP/UDP is designed to allow it.
As verified by @oil-oil, Consul's default behavior on Port has changed; it used to be null when registered without entering it; now it auto-populates with 0 as the default.
Since APISIX is primarily oriented towards HTTP scenarios, we fallback to 80 (the HTTP default) if the Port is null (the old behavior) or 0 (the new behavior, but not allowed by nginx).
This behavior needs to be documented.
|
|
||
| local svc_address, svc_port = node.Service.Address, node.Service.Port | ||
| -- Handle nil or 0 port case - default to 80 for HTTP services | ||
| if not svc_port or svc_port == 0 then |
There was a problem hiding this comment.
We have verified that nginx/openresty does not allow 0 as a port, even though TCP/UDP is designed to allow it.
As verified by @oil-oil, Consul's default behavior on Port has changed; it used to be null when registered without entering it; now it auto-populates with 0 as the default.
Since APISIX is primarily oriented towards HTTP scenarios, we fallback to 80 (the HTTP default) if the Port is null (the old behavior) or 0 (the new behavior, but not allowed by nginx).
This behavior needs to be documented.
github-project-automation
bot
moved this from 👀 In review
to ✅ Done
in ⚡️ Apache APISIX Roadmap
6 participants
Description
When using consul as a service registry, if a user registers a service without filling in the port number, the default value is not set in apisix, resulting in
svc_portpossibly beingnil, which in turn results inlocal service_id = svc_address ... “:” ... svc_portcannot be spliced.Which issue(s) this PR fixes:
Fixes #11134
Checklist