Skip to content

Commit

Permalink
fix(vault): properly warmups the cache on init (#11793)
Browse files Browse the repository at this point in the history
### Summary

Fixes issue where this was logged to logs:
```
2023/10/18 13:53:33 [warn] 8714#0: [kong] vault.lua:861 error updating secret reference {vault://env/PG_USER}: could not find cached value
```

That happened for example when starting Kong with this command:
```
KONG_LOG_LEVEL=warn PG_USER=kong KONG_PG_USER={vault://env/PG_USER} ./bin/kong start
```

It auto-corrected itself, which was good in this case.
This commit makes it more robust, and does not warn anymore
as caches are properly warmed.

Signed-off-by: Aapo Talvensaari <[email protected]>
(cherry picked from commit d8bd50d)
  • Loading branch information
bungle committed Oct 24, 2023
1 parent 9f113c1 commit b44d611
Show file tree
Hide file tree
Showing 3 changed files with 131 additions and 42 deletions.
3 changes: 3 additions & 0 deletions changelog/unreleased/kong/vault-init-warmup.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
message: Properly warmup Vault caches on init
type: bugfix
scope: Core
168 changes: 126 additions & 42 deletions kong/pdk/vault.lua
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ end
local function new(self)
-- Don't put this onto the top level of the file unless you're prepared for a surprise
local Schema = require "kong.db.schema"

local ROTATION_MUTEX_OPTS = {
name = "vault-rotation",
exptime = ROTATION_INTERVAL * 1.5, -- just in case the lock is not properly released
Expand Down Expand Up @@ -682,7 +682,7 @@ local function new(self)
return nil, err
end

if kong and kong.licensing and kong.licensing:license_type() == "free" and strategy.license_required then
if strategy.license_required and self.licensing and self.licensing:license_type() == "free" then
return nil, "vault " .. name .. " requires a license to be used"
end

Expand Down Expand Up @@ -738,6 +738,35 @@ local function new(self)
return value, nil, ttl
end

---
-- Function `get_cache_value_and_ttl` returns a value for caching and its ttl
--
-- @local
-- @function get_from_vault
-- @tparam string value the vault returned value for a reference
-- @tparam table config the configuration settings to be used
-- @tparam[opt] number ttl the possible vault returned ttl
-- @treturn string value to be stored in shared dictionary
-- @treturn number shared dictionary ttl
-- @treturn number lru ttl
-- @usage local value, err = get_from_vault(reference, strategy, config, cache_key, parsed_reference)
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

else
-- 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
end


---
-- Function `get_from_vault` retrieves a value from the vault using the provided strategy.
Expand All @@ -759,19 +788,7 @@ local function new(self)
-- @usage local value, err = get_from_vault(reference, strategy, config, cache_key, parsed_reference)
local function get_from_vault(reference, strategy, config, cache_key, parsed_reference)
local value, err, ttl = invoke_strategy(strategy, config, parsed_reference)
local cache_value, shdict_ttl
if value then
-- adjust ttl to the minimum and maximum values configured
ttl = adjust_ttl(ttl, config)
shdict_ttl = max(ttl + (config.resurrect_ttl or DAO_MAX_TTL), SECRETS_CACHE_MIN_TTL)
cache_value = value

else
-- 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

local cache_value, shdict_ttl, lru_ttl = get_cache_value_and_ttl(value, config, ttl)
local ok, cache_err = SECRETS_CACHE:safe_set(cache_key, cache_value, shdict_ttl)
if not ok then
return nil, cache_err
Expand All @@ -782,7 +799,7 @@ local function new(self)
return nil, fmt("could not get value from external vault (%s)", err)
end

LRU:set(reference, value, ttl)
LRU:set(reference, value, lru_ttl)

return value
end
Expand Down Expand Up @@ -866,34 +883,22 @@ local function new(self)


---
-- Function `update` recursively updates a configuration table.
--
-- This function recursively in-place updates a configuration table by
-- replacing reference fields with values fetched from a cache. The references
-- are specified in a `$refs` field.
--
-- If a reference cannot be fetched from the cache, the corresponding field is
-- set to nil and an warning is logged.
-- Recurse over config and calls the callback for each found reference.
--
-- @local
-- @function update
-- @tparam table config a table representing the configuration to update (if `config`
-- is not a table, the function immediately returns it without any modifications)
-- @treturn table the config table (with possibly updated values).
--
-- @usage
-- local config = update(config)
-- OR
-- update(config)
local function update(config)
-- @function recurse_config_refs
-- @tparam table config config table to recurse.
-- @tparam function callback callback to call on each reference.
-- @treturn table config that might have been updated, depending on callback.
local function recurse_config_refs(config, callback)
-- silently ignores other than tables
if type(config) ~= "table" then
return config
end

for key, value in pairs(config) do
if key ~= "$refs" and type(value) == "table" then
update(value)
recurse_config_refs(value, callback)
end
end

Expand All @@ -904,11 +909,11 @@ local function new(self)

for name, reference in pairs(references) do
if type(reference) == "string" then -- a string reference
update_from_cache(reference, config, name)
callback(reference, config, name)

elseif type(reference) == "table" then -- array, set or map of references
for key, ref in pairs(reference) do
update_from_cache(ref, config[name], key)
callback(ref, config[name], key)
end
end
end
Expand All @@ -917,6 +922,31 @@ local function new(self)
end


---
-- Function `update` recursively updates a configuration table.
--
-- This function recursively in-place updates a configuration table by
-- replacing reference fields with values fetched from a cache. The references
-- are specified in a `$refs` field.
--
-- If a reference cannot be fetched from the cache, the corresponding field is
-- set to nil and an warning is logged.
--
-- @local
-- @function update
-- @tparam table config a table representing the configuration to update (if `config`
-- is not a table, the function immediately returns it without any modifications)
-- @treturn table the config table (with possibly updated values).
--
-- @usage
-- local config = update(config)
-- OR
-- update(config)
local function update(config)
return recurse_config_refs(config, update_from_cache)
end


---
-- Function `get_references` recursively iterates over options and returns
-- all the references in an array. The same reference is in array only once.
Expand Down Expand Up @@ -1105,7 +1135,7 @@ local function new(self)
-- We cannot retry, so let's just call the callback and return
return callback(options)
end

local name = "vault.try:" .. calculate_hash(concat(references, "."))
local old_updated_at = RETRY_LRU:get(name) or 0

Expand Down Expand Up @@ -1296,10 +1326,6 @@ local function new(self)

initialized = true

if self.configuration.role == "control_plane" then
return
end

if self.configuration.database ~= "off" then
self.worker_events.register(handle_vault_crud_event, "crud", "vaults")
end
Expand All @@ -1311,6 +1337,61 @@ local function new(self)
end


---
-- Called on `init` phase, and stores value in secrets cache.
--
-- @local
-- @function init_in_cache_from_value
-- @tparam string reference a vault reference.
-- @tparan value string value that is stored in secrets cache.
local function init_in_cache_from_value(reference, value)
local strategy, err, config, cache_key = get_strategy(reference)
if not strategy then
return nil, err
end

-- doesn't support vault returned ttl, but none of the vaults supports it,
-- and the support for vault returned ttl might be removed later.
local cache_value, shdict_ttl, lru_ttl = get_cache_value_and_ttl(value, config)

local ok, cache_err = SECRETS_CACHE:safe_set(cache_key, cache_value, shdict_ttl)
if not ok then
return nil, cache_err
end

if value then
LRU:set(reference, value, lru_ttl)
end

return true
end


---
-- Called on `init` phase, and used to warmup secrets cache.
--
-- @local
-- @function init_in_cache
-- @tparam string reference a vault reference.
-- @tparan table record a table that is a container for de-referenced value.
-- @tparam field string field name in a record to which to store the de-referenced value.
local function init_in_cache(reference, record, field)
local value, err = init_in_cache_from_value(reference, record[field])
if not value then
self.log.warn("error caching secret reference ", reference, ": ", err)
end
end


---
-- Called on `init` phase, and used to warmup secrets cache.
-- @local
-- @function init
local function init()
recurse_config_refs(self.configuration, init_in_cache)
end


local _VAULT = {} -- the public PDK interfaces


Expand Down Expand Up @@ -1482,6 +1563,9 @@ local function new(self)
init_worker()
end

if get_phase() == "init" then
init()
end

return _VAULT
end
Expand Down
2 changes: 2 additions & 0 deletions spec/02-integration/02-cmd/02-start_stop_spec.lua
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ describe("kong start/stop #" .. strategy, function()
}))

assert.not_matches("failed to dereference {vault://env/pg_password}", stderr, nil, true)
assert.logfile().has.no.line("[warn]", true)
assert.logfile().has.no.line("env/pg_password", true)
assert.matches("Kong started", stdout, nil, true)
assert(kong_exec("stop", {
prefix = PREFIX,
Expand Down

0 comments on commit b44d611

Please sign in to comment.