Copilot commented on code in PR #14047:
URL: https://github.com/apache/apisix/pull/14047#discussion_r4291296815
##########
apisix/secret.lua:
##########
@@ -171,6 +176,55 @@ end
_M.fetch_by_uri = fetch_by_uri_secret
+-- A provider resolves one field per call but reads the whole container (a
+-- vault path, a JSON secret, ...) to do it, so a certificate and its key are
+-- two reads that a rotation can fall between. This cache keeps a container
read
+-- for a moment and makes concurrent readers in the worker share the read in
+-- flight, so fields resolved together come from the same read.
+local CONTAINER_TTL = 1
+local INFLIGHT_WAIT = 10
+
+function _M.new_container_cache()
+ local lru = lru_new(64)
+ local inflight = {}
+
+ return function (key, ver, read, ...)
+ local hit = lru:get(key)
+ if hit and hit.ver == ver then
+ return hit.val
+ end
+
+ local pending = inflight[key]
+ if pending and pending.ver == ver then
+ if pending.sema:wait(INFLIGHT_WAIT) then
+ return pending.val, pending.err, pending.extra
+ end
+ -- the reader may have been aborted along with its request
+ if inflight[key] == pending then
+ inflight[key] = nil
+ end
+ end
+
+ pending = {ver = ver, sema = semaphore.new()}
+ inflight[key] = pending
+ local val, err, extra = read(...)
+ pending.val, pending.err, pending.extra = val, err, extra
+ if inflight[key] == pending then
+ inflight[key] = nil
+ end
+ if val ~= nil then
+ lru:set(key, {val = val, ver = ver}, CONTAINER_TTL)
+ end
+
+ local waiting = -pending.sema:count()
+ if waiting > 0 then
+ pending.sema:post(waiting)
+ end
+ return val, err, extra
+ end
Review Comment:
The wake-up logic for in-flight readers is racy: computing `waiting =
-pending.sema:count()` and posting that amount can miss waiters that start
waiting after the `count()` snapshot but before `post()`, causing them to block
until timeout even though the read completed. Consider switching to a
completion flag + broadcast pattern that cannot miss late waiters (e.g., set
`pending.done = true` and make waiters check `done` before waiting; use a
chained semaphore release or an explicit waiters counter updated before
sleeping), so completion reliably releases all current/future waiters for that
`pending` instance.
##########
apisix/ssl/router/radixtree_sni.lua:
##########
@@ -243,8 +243,11 @@ function _M.set(matched_ssl, sni)
end
ngx_ssl.clear_certs()
- local new_ssl_value = secret.fetch_secrets(matched_ssl.value, true)
- or matched_ssl.value
+ local new_ssl_value
+ new_ssl_value, err = secret.fetch_ssl_secrets(matched_ssl.value)
+ if not new_ssl_value then
+ return false, err
Review Comment:
`fetch_ssl_secrets()` deep-copies and walks the SSL object; calling it
unconditionally on every handshake can add avoidable overhead for inline PEM
SSL objects (no secret refs). To prevent a per-handshake deepcopy in the common
case, gate this call with a `has_secret_ref(matched_ssl.value)` check (fall
back to `matched_ssl.value` when false), or alternatively change
`fetch_ssl_secrets()` to fast-path return when none of the SSL material fields
contain secret refs.
--
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]
