refactor(improvement): use secret URI as key for cache and refactor lrucache (#12682)

Author: Revolyssup

apisix/cli/config.lua

@@ -83,7 +83,9 @@ local _M = {
     lru = {
       secret = {
         ttl = 300,
-        count = 512
+        count = 512,
+        neg_ttl = 60,
+        neg_count = 512
       }
     }
   },

apisix/core/lrucache.lua

@@ -98,9 +98,23 @@ local function new_lru_fun(opts)
     local refresh_stale = opts and opts.refresh_stale
     local serial_creating = opts and opts.serial_creating
     local lru_obj = lru_new(item_count)
+
+    local neg_lru_obj
+    if opts and opts.neg_ttl and opts.neg_count then
+        neg_lru_obj = lru_new(opts.neg_count)
+    end
+
     stale_obj_pool[lru_obj] = {}
 
     return function (key, version, create_obj_fun, ...)
+        -- check negative cache first
+        if neg_lru_obj then
+            local neg_obj = neg_lru_obj:get(key)
+            if neg_obj and neg_obj.ver == version then
+                return nil, neg_obj.err
+            end
+        end
+
         if not serial_creating or not can_yield_phases[get_phase()] then
             local cache_obj = fetch_valid_cache(lru_obj, invalid_stale, refresh_stale,
                                 item_ttl, key, version, create_obj_fun, ...)
@@ -111,6 +125,9 @@ local function new_lru_fun(opts)
             local obj, err = create_obj_fun(...)
             if obj ~= nil then
                 lru_obj:set(key, {val = obj, ver = version}, item_ttl)
+            elseif neg_lru_obj then
+                -- cache the failure in negative cache
+                neg_lru_obj:set(key, {err = err, ver = version}, opts.neg_ttl)
             end
 
             return obj, err
@@ -146,6 +163,9 @@ local function new_lru_fun(opts)
         local obj, err = create_obj_fun(...)
         if obj ~= nil then
             lru_obj:set(key, {val = obj, ver = version}, item_ttl)
+        elseif neg_lru_obj then
+            -- cache the failure in negative cache
+            neg_lru_obj:set(key, {err = err, ver = version}, opts.neg_ttl)
         end
         lock:unlock()
         log.info("unlock with key ", key_s)

apisix/plugins/ai-aws-content-moderation.lua

@@ -88,7 +88,7 @@ end
 
 
 function _M.rewrite(conf, ctx)
-    conf = fetch_secrets(conf, true, conf, "")
+    conf = fetch_secrets(conf, true)
     if not conf then
         return HTTP_INTERNAL_SERVER_ERROR, "failed to retrieve secrets from conf"
     end

apisix/plugins/authz-keycloak.lua

@@ -764,7 +764,7 @@ end
 
 function _M.access(conf, ctx)
     -- resolve secrets
-    conf = fetch_secrets(conf, true, conf, "")
+    conf = fetch_secrets(conf, true)
     local headers = core.request.headers(ctx)
     local need_grant_token = conf.password_grant_token_generation_incoming_uri and
         ctx.var.request_uri == conf.password_grant_token_generation_incoming_uri and

apisix/plugins/limit-count.lua

@@ -34,7 +34,7 @@ end
 
 
 function _M.access(conf, ctx)
-    conf = fetch_secrets(conf, true, conf, "")
+    conf = fetch_secrets(conf, true)
     return limit_count.rate_limit(conf, ctx, plugin_name, 1)
 end
 

apisix/plugins/openid-connect.lua

@@ -550,7 +550,7 @@ end
 
 function _M.rewrite(plugin_conf, ctx)
     local conf_clone = core.table.clone(plugin_conf)
-    local conf = fetch_secrets(conf_clone, true, plugin_conf, "")
+    local conf = fetch_secrets(conf_clone, true)
 
     -- Previously, we multiply conf.timeout before storing it in etcd.
     -- If the timeout is too large, we should not multiply it again.

apisix/secret.lua

@@ -51,7 +51,7 @@ local function check_secret(conf)
 end
 
 
- local function secret_kv(manager, confid)
+local function secret_kv(manager, confid)
     local secret_values
     secret_values = core.config.fetch_created_obj("/secrets")
     if not secret_values or not secret_values.values then
@@ -136,7 +136,7 @@ local function parse_secret_uri(secret_uri)
 end
 
 
-local function fetch_by_uri(secret_uri)
+local function fetch_by_uri_secret(secret_uri)
     core.log.info("fetching data from secret uri: ", secret_uri)
     local opts, err = parse_secret_uri(secret_uri)
     if not opts then
@@ -162,81 +162,93 @@ local function fetch_by_uri(secret_uri)
 end
 
 -- for test
-_M.fetch_by_uri = fetch_by_uri
-
-
-local function fetch(uri)
-    -- do a quick filter to improve retrieval speed
-    if byte(uri, 1, 1) ~= byte('$') then
-        return nil
-    end
-
-    local val, err
-    if string.has_prefix(upper(uri), core.env.PREFIX) then
-        val, err = core.env.fetch_by_uri(uri)
-    elseif string.has_prefix(uri, PREFIX) then
-        val, err = fetch_by_uri(uri)
-    end
-
-    if err then
-        core.log.error("failed to fetch secret value: ", err)
-        return
-    end
-
-    return val
-end
+_M.fetch_by_uri = fetch_by_uri_secret
 
 
 local function new_lrucache()
     local ttl = core.table.try_read_attr(local_conf, "apisix", "lru", "secret", "ttl")
     if not ttl then
         ttl = 300
     end
+
     local count = core.table.try_read_attr(local_conf, "apisix", "lru", "secret", "count")
     if not count then
         count = 512
     end
-    core.log.info("secret lrucache ttl: ", ttl, ", count: ", count)
+
+    local neg_ttl = core.table.try_read_attr(local_conf, "apisix", "lru", "secret", "neg_ttl")
+    if not neg_ttl then
+        neg_ttl = 60  -- 1 minute default for failures
+    end
+
+    local neg_count = core.table.try_read_attr(local_conf, "apisix", "lru", "secret", "neg_count")
+    if not neg_count then
+        neg_count = 512
+    end
+
+    core.log.info("secret lrucache ttl: ", ttl, ", count: ", count,
+                  ", neg_ttl: ", neg_ttl, ", neg_count: ", neg_count)
+
     return core.lrucache.new({
-        ttl = ttl, count = count, invalid_stale = true, refresh_stale = true
+        ttl = ttl,
+        count = count,
+        neg_ttl = neg_ttl,
+        neg_count = neg_count,
+        invalid_stale = true,
+        refresh_stale = true
     })
 end
-local secrets_lrucache = new_lrucache()
-
-
-local fetch_secrets
-do
-    local retrieve_refs
-    function retrieve_refs(refs)
-        for k, v in pairs(refs) do
-            local typ = type(v)
-            if typ == "string" then
-                refs[k] = fetch(v) or v
-            elseif typ == "table" then
-                retrieve_refs(v)
-            end
-        end
-        return refs
-    end
 
-    local function retrieve(refs)
-        core.log.info("retrieve secrets refs")
+local secrets_cache = new_lrucache()
 
-        local new_refs = core.table.deepcopy(refs)
-        return retrieve_refs(new_refs)
+
+
+local function fetch(uri, use_cache)
+    -- do a quick filter to improve retrieval speed
+    if byte(uri, 1, 1) ~= byte('$') then
+        return nil
     end
 
-    function fetch_secrets(refs, cache, key, version)
-        if not refs or type(refs) ~= "table" then
+    local fetch_by_uri
+    if string.has_prefix(upper(uri), core.env.PREFIX) then
+        fetch_by_uri = core.env.fetch_by_uri
+    elseif string.has_prefix(uri, PREFIX) then
+        fetch_by_uri = fetch_by_uri_secret
+    else
+        return nil
+    end
+
+    if not use_cache then
+        local val, err = fetch_by_uri(uri)
+        if err then
+            core.log.error("failed to fetch secret value: ", err)
             return nil
         end
-        if not cache then
-            return retrieve(refs)
+        return val
+    end
+
+    return secrets_cache(uri, "", fetch_by_uri, uri)
+end
+
+local function retrieve_refs(refs, use_cache)
+    for k, v in pairs(refs) do
+        local typ = type(v)
+        if typ == "string" then
+            refs[k] = fetch(v, use_cache) or v
+        elseif typ == "table" then
+            retrieve_refs(v, use_cache)
         end
-        return secrets_lrucache(key, version, retrieve, refs)
     end
+    return refs
 end
 
-_M.fetch_secrets = fetch_secrets
+function _M.fetch_secrets(refs, use_cache)
+    if not refs or type(refs) ~= "table" then
+        return nil
+    end
+
+    local new_refs = core.table.deepcopy(refs)
+    return retrieve_refs(new_refs, use_cache)
+end
 
 return _M

apisix/ssl/router/radixtree_sni.lua

@@ -241,7 +241,7 @@ function _M.set(matched_ssl, sni)
     end
     ngx_ssl.clear_certs()
 
-    local new_ssl_value = secret.fetch_secrets(matched_ssl.value, true, matched_ssl.value, "")
+    local new_ssl_value = secret.fetch_secrets(matched_ssl.value, true)
                             or matched_ssl.value
 
     ok, err = _M.set_cert_and_key(sni, new_ssl_value)

conf/config.yaml.example

@@ -148,8 +148,10 @@ apisix:
   # fine tune the parameters of LRU cache for some features like secret
   lru:
     secret:
-      ttl: 300 # seconds
-      count: 512
+      ttl: 300          # Global TTL fallback
+      count: 512        # Cache size
+      neg_ttl: 60       # Negative cache TTL
+      neg_count: 512    # Negative cache size
 nginx_config:                     # Config for render the template to generate nginx.conf
   # user: root                    # Set the execution user of the worker process. This is only
                                   # effective if the master process runs with super-user privileges.

t/config-center-json/secret.t

@@ -385,7 +385,7 @@ qr/retrieve secrets refs/
 
 
 
-=== TEST 14: fetch_secrets env: cache
+=== TEST 14: fetch_secrets env: cache (fetch data should be only called once and next call return from cache)
 --- main_config
 env secret=apisix;
 --- config
@@ -396,9 +396,8 @@ env secret=apisix;
                 key = "jack",
                 secret = "$env://secret"
             }
-            local refs_1 = secret.fetch_secrets(refs, true, "key", 1)
-            local refs_2 = secret.fetch_secrets(refs, true, "key", 1)
-            assert(refs_1 == refs_2)
+            local refs_1 = secret.fetch_secrets(refs, true)
+            local refs_2 = secret.fetch_secrets(refs, true)
             ngx.say(refs_1.secret)
             ngx.say(refs_2.secret)
         }
@@ -409,9 +408,9 @@ GET /t
 apisix
 apisix
 --- grep_error_log eval
-qr/retrieve secrets refs/
+qr/fetching data from env uri/
 --- grep_error_log_out
-retrieve secrets refs
+fetching data from env uri