Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(vault): properly warmups the cache on init #11793

Merged
merged 1 commit into from
Oct 24, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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