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 |
3 participants
Draft implementation as discussed in #938