fix(vault): fix vault config neg_ttl behavior

changelog/unreleased/kong/fix-vault-neg-ttl.yml

@@ -0,0 +1,3 @@
+message: "**vault**: Fixed an issue where neg_ttl config doesn't work"
+type: bugfix
+scope: Core

kong/pdk/vault.lua

@@ -797,8 +797,9 @@ local function new(self)
     else
       cache_value = NEGATIVELY_CACHED_VALUE
 
-      -- negatively cached values will be rotated on each rotation interval
-      shdict_ttl = max(config.neg_ttl or 0, SECRETS_CACHE_MIN_TTL)
+      -- Negatively cached values will be rotated when after neg_ttl and not before expiration from shdict.
+      -- Adding SECRETS_CACHE_MIN_TTL to neg_ttl to make sure the value doesn't expire when after neg_ttl.
+      shdict_ttl = (config.neg_ttl or 0) + SECRETS_CACHE_MIN_TTL
     end
 
     return cache_value, shdict_ttl, lru_ttl
@@ -1280,7 +1281,9 @@ local function new(self)
     -- negatively cached.
     local resurrect
     local ttl = SECRETS_CACHE:ttl(new_cache_key)
-    if ttl and SECRETS_CACHE:get(new_cache_key) ~= NEGATIVELY_CACHED_VALUE then
+    -- Note that `ttl` variable means remaining time in shdict, and there's an extra time in the shdict ttl
+    -- to make sure that the cache entry doesn't expire when after actual ttl.
+    if ttl then
       local resurrect_ttl = max(config.resurrect_ttl or DAO_MAX_TTL, SECRETS_CACHE_MIN_TTL)
       -- the secret is still within ttl, no need to refresh
       if ttl > resurrect_ttl then
@@ -1291,6 +1294,11 @@ local function new(self)
       -- we do not forciblly override it with a negative value, so that the cached value
       -- can be resurrected
       resurrect = ttl > SECRETS_CACHE_MIN_TTL
+
+      -- Only refresh negatively cached values after neg_ttl.
+      if ttl > SECRETS_CACHE_MIN_TTL and SECRETS_CACHE:get(new_cache_key) == NEGATIVELY_CACHED_VALUE then
+        return true
+      end
     end
 
     strategy = caching_strategy(strategy, config_hash)

spec/02-integration/13-vaults/05-ttl_spec.lua

@@ -38,6 +38,9 @@ local fmt = string.format
 --- update_secret() may be called more than once per test run for a given secret
 ---@field update_secret fun(self: vault_test_harness, secret: string, value: string, opts?: table)
 ---
+--- delete_secret() may be called more than once per test run for a given secret
+---@field delete_secret fun(self: vault_test_harness, secret: string)
+---
 --- setup() is called before kong is started and before any DB entities
 --- have been created and is best used for things like validating backend
 --- credentials and establishing a connection to a backend
@@ -78,6 +81,10 @@ local VAULTS = {
       return test_vault.client.put(secret, value, opts)
     end,
 
+    delete_secret = function(_, secret)
+      return test_vault.client.delete(secret)
+    end,
+
     fixtures = function()
       return {
         http_mock = {
@@ -157,8 +164,8 @@ describe("vault ttl and rotation (#" .. strategy .. ") #" .. vault.name, functio
     assert(bp.plugins:insert({
       name = "dummy",
       config = {
-        resp_header_value = fmt("{vault://%s/%s?ttl=%s}",
-                                vault.prefix, secret, 10),
+        resp_header_value = fmt("{vault://%s/%s?ttl=%s&resurrect_ttl=0&neg_ttl=%s}",
+                                vault.prefix, secret, 10, 10),
       },
       route = { id = route.id },
     }))
@@ -187,6 +194,65 @@ describe("vault ttl and rotation (#" .. strategy .. ") #" .. vault.name, functio
   end)
 
 
+  it("missing secrets should be cached for neg_ttl long (backend: #" .. vault.name .. ")", function()
+    local function check_plugin_secret_any(expect, ttl, leeway)
+      leeway = leeway or 0.25 -- 25%
+
+      local timeout = ttl + (ttl * leeway)
+
+      assert
+        .with_timeout(timeout)
+        .with_step(0.5)
+        .eventually(function()
+          local res = http_get("/")
+          local value = res.headers[DUMMY_HEADER]
+
+          if value == expect then
+            return true
+          end
+
+          return nil, { expected = expect, got = value }
+        end)
+        .is_truthy("expected plugin secret to be updated to '" .. tostring(expect) .. "' "
+                .. "' within " .. tostring(timeout) .. "seconds")
+    end
+
+    local function check_plugin_secret_all(expect, ttl, leeway)
+      leeway = leeway or 0.25 -- 25%
+
+      local timeout = ttl + (ttl * leeway)
+
+      -- The secret value is supposed to be not refreshed
+      -- after several rotations
+      assert.has_error(function()
+        assert
+         .with_timeout(timeout)
+         .with_step(0.5)
+         .eventually(function()
+            local res = http_get("/")
+            local value = res.headers[DUMMY_HEADER]
+
+            if value == expect then
+              return true
+            end
+
+            return false
+         end)
+         .is_falsy("expected plugin secret not to be updated to '" .. tostring(expect) .. "' "
+                 .. "' within " .. tostring(timeout) .. "seconds")
+        end)
+    end
+
+    vault:delete_secret(secret)
+    check_plugin_secret_any(nil, 1)
+
+    vault:update_secret(secret, "new", { ttl = 5 })
+    -- The negative cache is supposed to be not refreshed during neg_ttl (10s)
+    check_plugin_secret_all(nil, 9, 0)
+
+    check_plugin_secret_any("new", 5)
+  end)
+
   it("updates plugin config references (backend: #" .. vault.name .. ")", function()
     local function check_plugin_secret(expect, ttl, leeway)
       leeway = leeway or 0.25 -- 25%