AlinsRan commented on PR #13062: URL: https://github.com/apache/apisix/pull/13062#issuecomment-4920418409
Hi @moonming, both points from your review are handled: 1. #13061 is closed and folded in here — the client CA fix (`check_ssl_conf` skipping validation for secret refs, `radixtree_sni.set()` reading the resolved `client.ca`) plus its vault/env tests in `client-mtls.t` are all part of this PR now. 2. The nil check on `new_upstream_ssl` was dropped. @Baoyuantop and Copilot both pointed out it is unreachable: `fetch_secrets()` only returns nil when the input is not a table, and on a per-key lookup failure `retrieve_refs` keeps the original `$secret://` string (`refs[k] = fetch(v) or v`). So it never guarded the failure it looked like it was guarding. It now follows the same forgiving pattern as `radixtree_sni.lua`: `fetch_secrets(...) or upstream_ssl`. Also merged master and addressed @nic-6443's comments: the `schema_def.lua` hunk is gone (`secret_uri_schema` was removed in #13312, schema already lets secret refs through), and the resolve now lives in `resolve_upstream_client_cert()` gated on `secret.has_secret_ref()` so plain cert/key configs keep the zero-copy path. That helper is shared with the stream subsystem, so stream upstream mTLS gets the fix too. Could you re-review / re-run CI? -- 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]
