Copilot commented on code in PR #13062:
URL: https://github.com/apache/apisix/pull/13062#discussion_r2876926381
##########
apisix/init.lua:
##########
@@ -552,9 +552,22 @@ function _M.handle_upstream(api_ctx, route,
enable_websocket)
return ngx_exit(1)
end
+ local new_upstream_ssl = apisix_secret.fetch_secrets(upstream_ssl,
true)
+
+ if not new_upstream_ssl then
Review Comment:
`apisix_secret.fetch_secrets` only returns `nil` when the input is missing
or not a table; for a valid `upstream_ssl` table it will always return a
(deep-copied) table even if individual secret lookups fail (it leaves the
original `$env://`/`$secret://` string in place and logs). As a result, the `if
not new_upstream_ssl` branch is effectively unreachable and won’t prevent the
later TLS/decrypt errors this PR is trying to avoid. Consider removing this
check, or updating the secrets API/usage to surface lookup failures (e.g.,
return `nil, err` or explicitly detect unresolved secret refs after
substitution) and handle that here.
```suggestion
local function has_unresolved_secret_ref(value)
if type(value) ~= "table" then
return false
end
for _, v in pairs(value) do
local vt = type(v)
if vt == "string" then
if core.string.has_prefix(v, "$env://")
or core.string.has_prefix(v, "$secret://") then
return true
end
elseif vt == "table" then
if has_unresolved_secret_ref(v) then
return true
end
end
end
return false
end
if not new_upstream_ssl or
has_unresolved_secret_ref(new_upstream_ssl) then
```
##########
t/node/upstream-mtls.t:
##########
@@ -783,3 +809,125 @@ passed
GET /hello
--- response_body
hello world
+
+=== TEST 24: get cert by tls.client_cert_id with secrets using secrets
Review Comment:
Test title is redundant/grammatically awkward: "get cert by
tls.client_cert_id with secrets using secrets". Consider renaming to something
like "... with secrets using Vault" or "... with $secret:// refs" so it reads
clearly in test output.
```suggestion
=== TEST 24: get cert by tls.client_cert_id with $secret:// refs
```
##########
t/node/upstream-mtls.t:
##########
@@ -783,3 +809,125 @@ passed
GET /hello
--- response_body
hello world
+
+=== TEST 24: get cert by tls.client_cert_id with secrets using secrets
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin")
+ local json = require("toolkit.json")
+
+ local data = {
+ type = "client",
+ cert = "$secret://vault/test/ssl/mtls_client.crt",
+ key = "$secret://vault/test/ssl/mtls_client.key"
+ }
Review Comment:
This file writes the cert/key into Vault and then later references
`$secret://vault/test/...`, but it never configures the Vault secret backend in
APISIX (e.g., via `PUT /apisix/admin/secrets/vault/test` with
`uri/prefix/token` like other tests do). Without that, secret resolution will
fail and APISIX will keep the `$secret://...` strings, causing the mTLS
handshake to fail for reasons unrelated to the upstream `client_cert_id` fix.
Add a step to create the Vault secret config before using
`$secret://vault/test/...`.
--
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]
