Vault backend integration#958
Conversation
|
|
|
Really cool, thanks for implementing this! I'm still reviewing, will get back to you with more comments if any asap. |
|
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.
There was a problem hiding this comment.
I haven't implemented credentials refresh yet. For now, we fetch a new set of credentials every time they are close to expiration.
This will only log the error and then wait rotation_interval seconds to refresh the credentials and reload the connections. The backoff kicks in only when we fail to get credentials from vault.
Perhaps spawning a single task that refreshes credentials in a loop is not ideal for error handling, it would probably be better to spawn a task for each pool
There was a problem hiding this comment.
I think I agree. You can spawn per-pool tasks in monitor.rs (feel free to create a separate module under backend::pool).
There was a problem hiding this comment.
Are you sure about moving the tasks into backend::pool? I'm trying to implement it, but I’m not sure if calling reload_from_existing from this point in the code is 'correct.' Also, this way I’d have to 'leak' some Vault details into the underlying structures
There was a problem hiding this comment.
Ah, good point. I forgot that Pool is immutable, so reloading it from there isn't right. We recently added a cache for AWS IAM and Azure Worload Identity tokens, maybe that's a good place for Vault credentials refreshing too? Check it out: https://github.com/pgdogdev/pgdog/blob/main/pgdog/src/backend/pool/token_cache.rs
There was a problem hiding this comment.
I've updated the code to use the token cache. I had to add some minor extensions because Vault, unlike other "providers", returns both username and password.
For credential refreshing, since I can no longer use reload_from_existing, I introduced the concept of credentials generation.
Every time credentials are rotated, I bump the generation counter so that checked-out connections are dropped as soon as they return to the pool -> https://github.com/pgdogdev/pgdog/pull/958/changes#diff-2521cf07ff94037c585d263e602354bdec098576f95fa82b051cb44157992a9dR386
Let me know what you think!
larrywax
force-pushed
the
vault-backend-integration
branch
from
d3ac996 to
4c76f84
Compare
3 participants
Draft implementation as discussed in #938