Copilot commented on code in PR #14018:
URL: https://github.com/apache/apisix/pull/14018#discussion_r4218289130
##########
apisix/discovery/consul/client.lua:
##########
@@ -120,6 +120,9 @@ local function get_opts(consul_server, is_catalog)
local opts = {
host = consul_server.host,
port = consul_server.port,
+ ssl = consul_server.ssl,
+ ssl_verify = true,
+ sni_host = consul_server.host,
connect_timeout = consul_server.connect_timeout,
read_timeout = consul_server.read_timeout,
default_args = {
Review Comment:
`ssl_verify` and `sni_host` are being set unconditionally even when the
server is plain HTTP (`ssl = false`). To avoid surprising behavior (and to
reduce reliance on downstream library ignoring these fields), set
`ssl_verify`/`sni_host` only when `consul_server.ssl` is true.
##########
apisix/discovery/consul/client.lua:
##########
@@ -249,15 +252,19 @@ function _M.format_consul_params(consul_conf)
local consul_server_list = core.table.new(0, #servers)
for _, v in pairs(servers) do
- local scheme, host, port, path = unpack(http.parse_uri(nil, v))
- if scheme ~= "http" then
- return nil, "only support consul http schema address, eg:
http://address:port";
+ -- parse_uri only accepts http and https, any other scheme yields nil
+ local scheme, host, port, path = unpack(http.parse_uri(nil, v) or {})
+ if scheme ~= "http" and scheme ~= "https" then
+ return nil, "only support consul http or https schema address, "
+ .. "eg: http://address:port or https://address:port";
elseif path ~= "/" or core.string.has_suffix(v, '/') then
- return nil, "invalid consul server address, the valid format:
http://address:port";
+ return nil, "invalid consul server address, the valid format: "
+ .. "http://address:port or https://address:port";
Review Comment:
`http.parse_uri` can return `nil, err` not only for non-http(s) schemes but
also for malformed URLs. Using `unpack(... or {})` discards the parse error and
will report an unsupported-scheme message for malformed `http(s)` inputs.
Consider capturing and returning the parse error (or mapping it to the existing
\"invalid consul server address\" message) so invalid URIs are diagnosed
accurately.
##########
apisix/discovery/consul/client.lua:
##########
@@ -303,6 +310,9 @@ function _M.fetch_services_from_server(consul_server,
options)
local consul_client = resty_consul:new({
host = consul_server.host,
port = consul_server.port,
+ ssl = consul_server.ssl,
+ ssl_verify = true,
+ sni_host = consul_server.host,
connect_timeout = consul_server.connect_timeout,
read_timeout = consul_server.read_timeout,
default_args = {
Review Comment:
Same as `get_opts`: `ssl_verify`/`sni_host` are set even for `http://`
servers. Keeping TLS-only fields conditional on `ssl` improves clarity and
avoids coupling to the behavior of `lua-resty-consul` for non-TLS connections.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
