feat(config): Add Vault-backed configuration#1051
Conversation
4cbc673 to
4d3126b
Compare
|
4d3126b to
7253cd5
Compare
gtema
left a comment
There was a problem hiding this comment.
very nice, need small changes though
| let deadline = vault_runtime | ||
| .as_ref() | ||
| .map(vault::VaultRuntime::next_deadline) | ||
| .unwrap_or_else(|| Instant::now() + std::time::Duration::from_secs(31_536_000)); |
There was a problem hiding this comment.
nit: 31_536_000s (1yr) magic sentinel for "no active vault runtime, never fire" deadline in tokio::select!. Prefer std::future::pending() for the no-vault branch — clearer intent, no magic number.
Something like this can be used, but feel free to use different approach
// 1. Define the future dynamically before the select block
let vault_deadline_fut = match &vault_runtime {
Some(runtime) => {
let deadline = runtime.next_deadline();
futures::future::Either::Left(tokio::time::sleep_until(deadline))
}
None => futures::future::Either::Right(std::future::pending::<()>()),
};
tokio::select! {
event = sync_rx.recv() => {
// ... (your existing sync_rx logic)
}
// 2. Await the dynamic future directly
_ = vault_deadline_fut => {
// Safe to unwrap here because this branch is unreachable if vault_runtime was None
let runtime = vault_runtime.as_mut().unwrap();
if runtime.renew_if_due().await.is_err() {
error!("Vault token renewal failed; retrying while retaining current configuration");
}
// ... (rest of your renewal / polling logic)
}
}
There was a problem hiding this comment.
Done. Dropped the sentinel. The select! now arms sleep_until(runtime.next_deadline()) only when a Vault runtime is present, and falls back to std::future::pending() otherwise:
let vault_deadline = vault_runtime.as_ref().map(vault::VaultRuntime::next_deadline);
let vault_tick = async {
match vault_deadline {
Some(deadline) => tokio::time::sleep_until(deadline).await,
None => std::future::pending::<()>().await,
}
};Used a plain async block instead of future::Either to avoid pulling in futures.
| let raw = Self::build_raw(path)?; | ||
| if vault::contains_vault_references(&raw.cache)? { | ||
| return Err(vault::VaultConfigError::AsyncLoaderRequired.into()); | ||
| } |
There was a problem hiding this comment.
I am not fan of such logic (raising error on sync call requiring async. This requires callers to know how to deal with the error with none of the user having any logic for this. Basically it renders the feature right now as useless. Let's make it async-only accepting wider interface change (mostly only adding .await for callers)
There was a problem hiding this comment.
Agreed. Made the loaders async-only. Config::new and Config::load_all are now async and resolve Vault references directly, so the AsyncLoaderRequired error path is gone from the file loaders. Removed the redundant load_all_async and folded it into load_all, with a shared resolve_vault_references helper. Callers just gained .await.
The only remaining sync guard is on TryFrom<ConfigBuilder>, an in-memory test-only conversion that can't be made async. Happy to drop it too if you prefer.
c2262e4 to
6c59bab
Compare
Resolve complete Vault references from KV v2 after source merging. Poll metadata for rotations and retain the last complete configuration. Co-Authored-By: Claude Opus 4.8 <[email protected]> Signed-off-by: Yousef Hussein <[email protected]>
6c59bab to
92508bb
Compare
TL;DR
Resolve complete
vault://mount/path#keyconfiguration values directly from Vault KV v2.Changes
[vault]bootstrap configuration and async resolution after configuration source precedence is merged.keystone-managestartup to the asynchronous loader.Tests
Closes #502