Vault backend integration#958
Conversation
|
|
| /// Path to a PEM-encoded CA certificate bundle used to verify the Vault | ||
| /// server's TLS certificate. When provided, the bundle is added on top of | ||
| /// the system-wide certificate store. When omitted, only system certs are used. | ||
| pub tls_server_ca_certificate: Option<String>, |
There was a problem hiding this comment.
nit: you can use PathBuf, it implements Deserialize.
| /// Authenticate to Vault via AppRole and return a client token. | ||
| pub async fn approle_login( | ||
| client: &reqwest::Client, | ||
| addr: &str, |
There was a problem hiding this comment.
nit: when you have so many args of the same type, the caller can get confused and pass the wrong one. Couple ways around this:
- Define a struct that keeps those as its own fields and make this method use
&self. - Combine the args into a
struct, so the caller has to do something like this:
struct Args<'a> {
addr: &'a str,
role_id: &'a str,
}
approve_login(&Args { addr: "localhost", role_id: "1234" }).await?;
|
|
||
| // Fetch Vault credentials BEFORE creating pools so Address::new() picks up | ||
| // the vault-generated username/password from the very first connection. | ||
| let cfg = config::config(); |
There was a problem hiding this comment.
nit: re-use cfg below for let general =
|
Really cool, thanks for implementing this! I'm still reviewing, will get back to you with more comments if any asap. |
| "vault: kubernetes_role is required for Kubernetes auth".into(), | ||
| ) | ||
| })?; | ||
| let jwt = tokio::fs::read_to_string(self.cfg.jwt_path()) |
There was a problem hiding this comment.
nit: consider importing functions and structs, e.g.:
use tokio::fs::read_to_string;
read_to_string(&path).await?;
| if cred.lease_duration == 0 { | ||
| tracing::warn!( | ||
| pool = %pool_name, | ||
| "vault: lease_duration is 0 — credentials may not be renewable; check Vault backend config" |
There was a problem hiding this comment.
| "vault: lease_duration is 0 — credentials may not be renewable; check Vault backend config" | |
| "[vault] "lease_duration" is 0, credentials may not be renewable, check Vault backend configuration" |
|
|
||
| match client.fetch_credentials(&pools, update_config).await { | ||
| Ok(min_lease) => { | ||
| info!(pools = pools.len(), "vault: initial credentials fetched"); |
There was a problem hiding this comment.
| info!(pools = pools.len(), "vault: initial credentials fetched"); | |
| info!(pools = pools.len(), "[vault] initial credentials fetched for {} users", users.len()); |
| // the vault-generated username/password from the very first connection. | ||
| let cfg = config::config(); | ||
| let vault_initial_delay = if let Some(vault_cfg) = cfg.config.vault.as_ref() { | ||
| pgdog::backend::auth::vault::init(vault_cfg, &cfg.users.users).await |
There was a problem hiding this comment.
Reloading the config (RELOAD admin command) will erase these I think. You may want to run the vault loop externally and fetch credentials from an in-memory cache instead, using the database and user as key.
|
Great start. A few comments on style and one important architectural issue: the vault credentials are loaded on boot only, but pgdog config is dynamic and can be reloaded without restarting, I think the current implementation will reset the credentials when this happens. Fixed my incorrect merge conflict fix in |
Ah I see, was not aware of it. Makes sense, thanks! |
larrywax
force-pushed
the
vault-backend-integration
branch
from
3d52f90 to
d3ac996
Compare
| for (pool_name, cred) in &creds { | ||
| cache_set(pool_name, &cred.username, &cred.password); | ||
| } | ||
| if let Err(e) = reload_from_existing() { |
There was a problem hiding this comment.
I think you may want to reload the connection pools if and only if the credentials actually changed. I think vault would rotate them every 15min or so (not sure), but this would log it every second and reload the pools once per second too? Please let me know if I misunderstood.
3 participants
Draft implementation as discussed in #938