juzhiyuan commented on code in PR #13707:
URL: https://github.com/apache/apisix/pull/13707#discussion_r3618955816
##########
apisix/consumer.lua:
##########
@@ -190,6 +191,200 @@ end
end
+-- Incremental consumer plugin tree rebuild.
+-- Instead of rebuilding the full O(N) tree on every conf_version change,
+-- only process consumers that were created/updated/deleted.
+-- Full rebuild runs only on bootstrap (first request).
+-- A background timer runs a lightweight consistency check every 30s:
+-- it compares tracked IDs against consumers.values without reconstructing
data.
+-- Only triggers a full rebuild if it finds stale entries (extremely rare).
+-- Motivation: stock APISIX rebuilds this whole O(N) tree on every
conf_version change.
+
+local cached_plugins
+local cached_conf_version = 0
+local node_index = {} -- "plugin\0eid" -> position in nodes array
+local entry_plugins = {} -- eid -> { [plugin_name] = true }
+local pending_set = {} -- eid -> consumer item (from filter callback)
+local has_pending = false
+local pending_delete = false -- a delete event was seen; reconcile removed
ids
+local tracked_count = 0 -- count of consumers.values at last sync
+local FULL_SYNC_INTERVAL = 30 -- seconds between background consistency
checks
+
+-- key-auth (and the other auth plugins) look consumers up by a key value, not
by
+-- position, via _M.consumers_kv() -> a per-plugin {key_value -> consumer} map.
+-- That map was rebuilt in full on every conf_version change (O(N) over all
+-- consumers, each cloned through a fill lru). We instead maintain it
incrementally
+-- alongside the node arrays: kv_upsert on add/update, kv_remove on delete. The
+-- map lives on the per-plugin data table (pd.kv) and is populated lazily on
the
+-- first lookup (which also teaches us pd.key_attr for that plugin).
+local function kv_upsert(pd, consumer)
+ if not pd.kv or not pd.key_attr then
+ return
+ end
+ local nc = core.table.clone(consumer)
+ nc.auth_conf = secret.fetch_secrets(nc.auth_conf, false)
+ -- fail closed: skip unset credentials or unresolved secret refs so a
client
+ -- can never authenticate with a literal "$ENV://..." reference string.
+ if secret.has_secret_ref(nc.auth_conf) then
+ return
+ end
+ local key_value = nc.auth_conf[pd.key_attr]
+ if key_value == nil then
+ return
+ end
+ pd.kv[key_value] = nc
+ consumer._kvkey = key_value
+end
+
+local function kv_remove(pd, consumer)
+ if pd.kv and consumer and consumer._kvkey ~= nil then
+ pd.kv[consumer._kvkey] = nil
+ end
+end
+
+-- Collect current consumer/credential IDs from consumers.values.
+local function collect_current_ids()
+ local ids = {}
+ for _, val in ipairs(consumers.values or {}) do
+ if type(val) == "table" and val.value and val.value.id then
+ ids[val.value.id] = true
+ end
+ end
+ return ids
+end
+
+-- Remove all plugin node entries for a given consumer/credential ID.
+-- Uses swap-with-last for O(1) array removal without holes.
+local function remove_consumer_entries(eid)
+ local pset = entry_plugins[eid]
+ if not pset then return end
+ for pname in pairs(pset) do
+ local pd = cached_plugins[pname]
+ if pd then
+ local key = pname .. "\0" .. eid
+ local pos = node_index[key]
+ if pos and pos <= pd.len then
+ kv_remove(pd, pd.nodes[pos])
+ if pos < pd.len then
+ -- swap with last element
+ local last = pd.nodes[pd.len]
+ pd.nodes[pos] = last
+ node_index[pname .. "\0" .. last._eid] = pos
+ end
+ pd.nodes[pd.len] = nil
+ pd.len = pd.len - 1
+ end
+ node_index[key] = nil
+ end
+ end
+ entry_plugins[eid] = nil
+end
+
+-- Add a consumer/credential to the appropriate auth plugin nodes.
+local function add_consumer_entry(val)
+ if type(val) ~= "table" or not val.value then return end
+ local eid = val.value.id
+ if not eid then return end
+ for name, config in pairs(val.value.plugins or {}) do
+ local plugin_obj = plugin.get(name)
+ if not plugin_obj or plugin_obj.type ~= "auth" then
+ goto next_plugin
+ end
+ if not cached_plugins[name] then
+ cached_plugins[name] = {
+ nodes = {}, len = 0,
+ conf_version = consumers.conf_version
+ }
+ end
+ local consumer, err = construct_consumer_data(val, name, config)
+ if not consumer then
+ core.log.error("incremental: failed to construct consumer for
plugin ",
+ name, ": ", err)
+ goto next_plugin
+ end
+ consumer._eid = eid
+ local pd = cached_plugins[name]
+ pd.len = pd.len + 1
+ pd.nodes[pd.len] = consumer
+ node_index[name .. "\0" .. eid] = pd.len
+ if not entry_plugins[eid] then entry_plugins[eid] = {} end
+ entry_plugins[eid][name] = true
+ kv_upsert(pd, consumer)
+ ::next_plugin::
+ end
+end
+
+-- Full rebuild: construct entire tree and build indexes from scratch.
+local function full_rebuild()
+ cached_plugins = plugin_consumer()
+ node_index = {}
+ entry_plugins = {}
+ for pname, pd in pairs(cached_plugins) do
+ for i = 1, pd.len do
+ local c = pd.nodes[i]
+ local eid = c.credential_id or c.consumer_name
Review Comment:
`val.value.id` used by incremental events is the key relative to
`/consumers` (for example, `jack/credentials/cred-1`), but this full-rebuild
path stores only `c.credential_id` (`cred-1`). Consequently an update cannot
remove a bootstrapped credential, and the consistency timer will classify it as
stale on every run. Please normalize both paths to the same full entity ID;
this also avoids collisions between consumers using the same credential ID.
##########
apisix/consumer.lua:
##########
@@ -272,10 +475,19 @@ end
function _M.consumers_kv(plugin_name, consumer_conf, key_attr)
- local consumers = lrucache("consumers_key#" .. plugin_name,
consumer_conf.conf_version,
- create_consume_cache, consumer_conf, key_attr)
+ -- consumer_conf is the per-plugin node set from _M.plugin(); the
key_value ->
+ -- consumer map is cached on it and maintained incrementally by kv_upsert/
+ -- kv_remove, so it is not rebuilt on every conf_version change. Rebuild
only on
+ -- first use, a key_attr change, or a version gap the incremental path
missed.
+ if consumer_conf.kv and consumer_conf.key_attr == key_attr
Review Comment:
With this fast path, the per-plugin map has no time-based expiry, and
changes to `/secrets` or an external secret do not affect
`consumer_conf.conf_version`. Secret-backed auth values can therefore remain
cached indefinitely. Please preserve the previous refresh bound or include a
secret version or TTL in the cache validity check.
##########
apisix/consumer.lua:
##########
@@ -309,10 +521,63 @@ end
local function filter(consumer)
- if not consumer.value or not consumer.value.plugins then
+ -- A delete arrives as a value-less event (the etcd watch sets value = nil
on
+ -- removal). Flag it so the next incremental apply reconciles removed ids
and
+ -- the deleted consumer stops authenticating on the next request.
+ if not consumer.value then
+ if cached_plugins then
+ pending_delete = true
+ has_pending = true
+ end
+ return
+ end
+
+ if not consumer.value.plugins then
Review Comment:
Removing all auth plugins produces a consumer value without `plugins`, so
this early return never records the update. `_M.plugin()` will still advance
`cached_conf_version` with an empty pending set, leaving the previous auth node
and key active indefinitely. Please enqueue the entity before this check so the
old entry is removed even when the new plugin set is empty.
##########
apisix/consumer.lua:
##########
@@ -309,10 +521,63 @@ end
local function filter(consumer)
- if not consumer.value or not consumer.value.plugins then
+ -- A delete arrives as a value-less event (the etcd watch sets value = nil
on
+ -- removal). Flag it so the next incremental apply reconciles removed ids
and
+ -- the deleted consumer stops authenticating on the next request.
+ if not consumer.value then
+ if cached_plugins then
+ pending_delete = true
+ has_pending = true
+ end
+ return
+ end
+
+ if not consumer.value.plugins then
return
end
plugin.set_plugins_meta_parent(consumer.value.plugins, consumer)
+
+ -- Track changed consumer for incremental rebuild
+ if cached_plugins and consumer.value.id then
+ pending_set[consumer.value.id] = consumer
Review Comment:
This only queues the changed consumer entry. Credential nodes clone their
parent consumer, so changing the parent's `group_id`, labels, or `custom_id`
must invalidate or rebuild all child credential entries as well. Otherwise
requests authenticated by a credential keep using stale group policy and
headers indefinitely.
--
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]
