fix(vault): resurrect positive results in lru cache for ttl + resurre…

…ct ttl (#11815)

### Summary

The vault is rotating secrets on every minute which updates the shared dictionary
cache with new values, both negative and positive results. This commit changes the
Negative results handling on LRU. Previously the LRU was cleared for negative
results, and we just used to cache for config.ttl amount of time. This commit changes
it so that LRU values are deleted, and we cache things config.ttl +
config.resurrect_ttl amount of time in lru cache too.

It was reported by @Hayk-S on KAG-2833.

Signed-off-by: Aapo Talvensaari <aapo.talvensaari@gmail.com>

changelog/unreleased/kong/vault-resurrect.yml

@@ -0,0 +1,3 @@
+message: Vault resurrect time is respected in case a vault secret is deleted from a vault
+type: bugfix
+scope: Core

kong/pdk/vault.lua

@@ -29,6 +29,7 @@ local replace_dashes = string_tools.replace_dashes
 local ngx = ngx
 local get_phase = ngx.get_phase
 local max = math.max
+local min = math.min
 local fmt = string.format
 local sub = string.sub
 local byte = string.byte
@@ -754,15 +755,25 @@ local function new(self)
   local function get_cache_value_and_ttl(value, config, ttl)
     local cache_value, shdict_ttl, lru_ttl
     if value then
-      -- adjust ttl to the minimum and maximum values configured
-      lru_ttl = adjust_ttl(ttl, config)
-      shdict_ttl = max(lru_ttl + (config.resurrect_ttl or DAO_MAX_TTL), SECRETS_CACHE_MIN_TTL)
       cache_value = value
 
+      -- adjust ttl to the minimum and maximum values configured
+      ttl = adjust_ttl(ttl, config)
+
+      if config.resurrect_ttl then
+        lru_ttl = min(ttl + config.resurrect_ttl, DAO_MAX_TTL)
+        shdict_ttl = max(lru_ttl, SECRETS_CACHE_MIN_TTL)
+
+      else
+        lru_ttl = ttl
+        shdict_ttl = DAO_MAX_TTL
+      end
+
     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)
-      cache_value = NEGATIVELY_CACHED_VALUE
     end
 
     return cache_value, shdict_ttl, lru_ttl
@@ -795,14 +806,13 @@ local function new(self)
       return nil, cache_err
     end
 
-    if not value then
-      LRU:delete(reference)
+    if cache_value == NEGATIVELY_CACHED_VALUE then
       return nil, fmt("could not get value from external vault (%s)", err)
     end
 
-    LRU:set(reference, value, lru_ttl)
+    LRU:set(reference, cache_value, lru_ttl)
 
-    return value
+    return cache_value
   end
 
 
@@ -824,8 +834,7 @@ local function new(self)
   -- @usage
   -- local value, err = get(reference, cache_only)
   local function get(reference, cache_only)
-    -- the LRU stale value is ignored as the resurrection logic
-    -- is deferred to the shared dictionary
+    -- the LRU stale value is ignored
     local value = LRU:get(reference)
     if value then
       return value
@@ -1360,8 +1369,8 @@ local function new(self)
       return nil, cache_err
     end
 
-    if value then
-      LRU:set(reference, value, lru_ttl)
+    if cache_value ~= NEGATIVELY_CACHED_VALUE then
+      LRU:set(reference, cache_value, lru_ttl)
     end
 
     return true

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

@@ -64,7 +64,7 @@ local VAULTS = {
     },
 
     create_secret = function(self, _, value)
-      -- Currently, crate_secret is called _before_ starting Kong.
+      -- Currently, create_secret is called _before_ starting Kong.
       --
       -- This means our backend won't be available yet because it is
       -- piggy-backing on Kong as an HTTP mock fixture.

spec/02-integration/13-vaults/07-resurrect_spec.lua

@@ -0,0 +1,240 @@
+local helpers = require "spec.helpers"
+
+-- using the full path so that we don't have to modify package.path in
+-- this context
+local test_vault = require "spec.fixtures.custom_vaults.kong.vaults.test"
+
+local CUSTOM_VAULTS = "./spec/fixtures/custom_vaults"
+local CUSTOM_PLUGINS = "./spec/fixtures/custom_plugins"
+
+local LUA_PATH = CUSTOM_VAULTS .. "/?.lua;" ..
+                 CUSTOM_VAULTS .. "/?/init.lua;" ..
+                 CUSTOM_PLUGINS .. "/?.lua;" ..
+                 CUSTOM_PLUGINS .. "/?/init.lua;;"
+
+local DUMMY_HEADER = "Dummy-Plugin"
+local fmt = string.format
+
+
+
+--- A vault test harness is a driver for vault backends, which implements
+--- all the necessary glue for initializing a vault backend and performing
+--- secret read/write operations.
+---
+--- All functions defined here are called as "methods" (e.g. harness:fn()), so
+--- it is permitted to keep state on the harness object (self).
+---
+---@class vault_test_harness
+---
+---@field name string
+---
+--- this table is passed directly to kong.db.vaults:insert()
+---@field config table
+---
+--- create_secret() is called once per test run for a given secret
+---@field create_secret fun(self: vault_test_harness, secret: string, value: string, opts?: table)
+---
+--- 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)
+---
+--- 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
+---@field setup fun(self: vault_test_harness)
+---
+--- teardown() is exactly what you'd expect
+---@field teardown fun(self: vault_test_harness)
+---
+--- fixtures() output is passed directly to `helpers.start_kong()`
+---@field fixtures fun(self: vault_test_harness):table|nil
+---
+---
+---@field prefix   string   # generated by the test suite
+---@field host     string   # generated by the test suite
+
+
+---@type vault_test_harness[]
+local VAULTS = {
+  {
+    name = "test",
+
+    config = {
+      default_value = "DEFAULT",
+      default_value_ttl = 1,
+    },
+
+    create_secret = function(self, _, value)
+      -- Currently, create_secret is called _before_ starting Kong.
+      --
+      -- This means our backend won't be available yet because it is
+      -- piggy-backing on Kong as an HTTP mock fixture.
+      --
+      -- We can, however, inject a default value into our configuration.
+      self.config.default_value = value
+    end,
+
+    update_secret = function(_, secret, value, opts)
+      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 = {
+          test_vault = test_vault.http_mock,
+        }
+      }
+    end,
+  },
+}
+
+
+local noop = function(...) end
+
+for _, vault in ipairs(VAULTS) do
+  -- fill out some values that we'll use in route/service/plugin config
+  vault.prefix     = vault.name .. "-ttl-test"
+  vault.host       = vault.name .. ".vault-ttl.test"
+
+  -- ...and fill out non-required methods
+  vault.setup      = vault.setup or noop
+  vault.teardown   = vault.teardown or noop
+  vault.fixtures   = vault.fixtures or noop
+end
+
+
+for _, strategy in helpers.each_strategy() do
+for _, vault in ipairs(VAULTS) do
+
+
+describe("vault resurrect_ttl and rotation (#" .. strategy .. ") #" .. vault.name, function()
+  local client
+  local secret = "my-secret"
+
+
+  local function http_get(path)
+    path = path or "/"
+
+    local res = client:get(path, {
+      headers = {
+        host = assert(vault.host),
+      },
+    })
+
+    assert.response(res).has.status(200)
+
+    return res
+  end
+
+
+  lazy_setup(function()
+    helpers.setenv("KONG_LUA_PATH_OVERRIDE", LUA_PATH)
+    helpers.setenv("KONG_VAULT_ROTATION_INTERVAL", "1")
+
+    helpers.test_conf.loaded_plugins = {
+      dummy = true,
+    }
+
+    vault:setup()
+    vault:create_secret(secret, "init")
+
+    local bp = helpers.get_db_utils(strategy,
+            { "vaults", "routes", "services", "plugins" },
+            { "dummy" },
+            { vault.name })
+
+
+    assert(bp.vaults:insert({
+      name     = vault.name,
+      prefix   = vault.prefix,
+      config   = vault.config,
+    }))
+
+    local route = assert(bp.routes:insert({
+      name      = vault.host,
+      hosts     = { vault.host },
+      paths     = { "/" },
+      service   = assert(bp.services:insert()),
+    }))
+
+
+    -- used by the plugin config test case
+    assert(bp.plugins:insert({
+      name = "dummy",
+      config = {
+        resp_header_value = fmt("{vault://%s/%s?ttl=%d&resurrect_ttl=%d}",
+                                vault.prefix, secret, 2, 2),
+      },
+      route = { id = route.id },
+    }))
+
+    assert(helpers.start_kong({
+      database       = strategy,
+      nginx_conf     = "spec/fixtures/custom_nginx.template",
+      vaults         = vault.name,
+      plugins        = "dummy",
+      log_level      = "info",
+    }, nil, nil, vault:fixtures() ))
+
+    client = helpers.proxy_client()
+  end)
+
+
+  lazy_teardown(function()
+    if client then
+      client:close()
+    end
+
+    helpers.stop_kong(nil, true)
+    vault:teardown()
+
+    helpers.unsetenv("KONG_LUA_PATH_OVERRIDE")
+  end)
+
+
+  it("resurrects plugin config references when secret is deleted (backend: #" .. vault.name .. ")", function()
+    local function check_plugin_secret(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
+      if expect == "" then
+        value = res.headers[DUMMY_HEADER] or ""
+        if value == "" then
+          return true
+        end
+
+      else
+        value = assert.response(res).has.header(DUMMY_HEADER)
+        if value == expect then
+          return true
+        end
+      end
+
+      return nil, { expected = expect, got = value }
+    end)
+    .is_truthy("expected plugin secret to be updated to '" .. tostring(expect) .. "' "
+            .. "within " .. tostring(timeout) .. " seconds")
+  end
+
+    vault:update_secret(secret, "old", { ttl = 2, resurrect_ttl = 2 })
+    check_plugin_secret("old", 5)
+    vault:delete_secret(secret)
+    ngx.sleep(2.5)
+    check_plugin_secret("old", 5)
+    check_plugin_secret("", 5)
+  end)
+end)
+
+
+end -- each vault backend
+end -- each strategy

spec/fixtures/custom_vaults/kong/vaults/test/schema.lua

@@ -1,3 +1,6 @@
+local typedefs = require "kong.db.schema.typedefs"
+
+
 return {
   name = "test",
   fields = {
@@ -7,6 +10,9 @@ return {
         fields = {
           { default_value     = { type = "string", required = false } },
           { default_value_ttl = { type = "number", required = false } },
+          { ttl                 = typedefs.ttl },
+          { neg_ttl             = typedefs.ttl },
+          { resurrect_ttl       = typedefs.ttl },
         },
       },
     },