Dynamic or short lived configs based on temporary credentials in deadpool_postgres

[Aiden-Ziegelaar]

I was looking at issue #396 and it struck me that there might be a wider use case here. For tokio_postgres the config is scoped to a single connection so it makes sense to be static as it's used for initiating the connection. For scenarios where the config may only be valid for a short time window (i.e. RDS IAM) would it make sense to have a trait that could be implemented by consumers that produces a tokio_postgres config, then a generic implementing this trait is used for new connections? This would allow for much more dynamic configuration of the application i.e. maintaining hosts, etc. This would allow all of the temporary credential use cases to be met without implementing specific logic in the crate for each one.

Something like:

pub trait TokioPostgresConfigProvider {
    /// Returns a [`tokio_postgres::Config`] instance.
    fn get_pg_config(&self) -> Result<tokio_postgres::Config, ConfigError>;
}

I have a vested interest in this as I'd like it for a personal project, so happy to help out here.

[Aiden-Ziegelaar]

Also I am aware of #180 and #166 but both seem to be reactive rather that proactive approaches to maintaining the pool config.

[bikeshedder]

#180 and #166 are about changing the pool configuration regarding timeouts, max_size, etc. It's generic to all pool implementations (deadpool-postgres, deadpool-redis, etc.)

Changing those reactively doesn't make a lot of sense, I guess. I really should look into merging this soon. Even though it's a niche feature it's really nice for feature completeness.

Regarding the config provider I'm 100% aligned with you. Initially I thought swapping the manager with a new config was the way to go. But what if some auth scheme required a extremely short lived config and/or maybe even a unique token that must only be used once. A ConfigSource trait is the only proper way to implement that.

I've been thinking about this for a while now and I guess the trait should look somewhat like that:

pub trait PostgresConfigSource {
    async fn get_config(&self) -> Result<Arc<tokio_postgres::Config>, ConfigSourceError>;
}

Q: Does it need to be async?
Maybe getting the config does involve some kind of API call and/or disk I/O. So I'd rather use a async fn there.

Q: Does the config need to be wrapped in an Arc<...>
Not necessarily. I was just thinking about the StaticConfigSource which always returns the same configuration and the strings don't need to be cloned for every connection.

[Aiden-Ziegelaar]

Yeah sorry the signature I put together was not really usable, more to illustrate the point.

Q: Does it need to be async?
Agreed with the async function as it would be required to do any of the operations required to communicate with the token provider.

Q: Does the config need to be wrapped in an Arc<...>
I think even in the short lived credentials Arc is useful as you'd probably want to avoid calling the token service too often and I think for RDS the default is 15 mins, so ideally you would have a TTL on the provider and would return the cached config else call out to the token provider or maintain a config proactively before the TTL expiry.

[bikeshedder]

Any ideas how this could stay compatible with the config feature of deadpool-postgres (and possibly the other crates where I would also like to add this feature).

I'm thinking of changing Config to Config<S: ConfigSource> and add a associative type to the ConfigSource that is put in the Config.

trait ConfigSource {
    type Config: Deserialize;
}

struct Config<S: ConfigSource> {
    // (...)
    #[serde(flatten)]
    config_source_config: C::Config,

And the concrete config source also needs to implement From<Config> so the config can be turned into an actual config.

Oh ... and we need some better names. There are way too many Config structs. Maybe call the PG specific config connection_config instead (that's how it's called in the deadpool-redis crate)? Or as you suggested PostgresConfig (I'd just leave out the Tokio part as that doesn't add any information to it).

[bikeshedder]

The current config struct is a bit messy. The tokio-postgres config is flattened in there but the manager and pool config aren't. I really think the connection specific bits should be moved into a struct anyways.

btw. It would be really nice if the config could support multiple config sources. It would be awesome if there was a way to make this somehow pluggable. 🙈

We're getting close to bikeshedding territory here. Sorry for that...

[Aiden-Ziegelaar]

I'd have to dive deeper into the crate to understand the context for this one, I'll take a look tonight.

Not Rust related but it would be possible to use the strangler fig pattern to a degree here to prevent additional complexity. A new method takes a compatible PostgresConfigSource allowing for non breaking changes to existing code. The current method changes to creating the static PostgresConfigSource case and passing it to the new method. Then that way there's only one config access method to deal with fetching configs in the code. I think that would provide easy access patterns in the crate internals whilst still allowing for backwards compatibility between versions allowing for minor version SemVer bump.

On the pluggable side not sure how you would implement this with multiple config sources as pool connections would need to be differentiated depending on their config source. I'm wondering if there's an advantage to managing a hashmap of config providers and connections within the pool as opposed to managing a hashmap of pools given that deadpool is already such a low overhead. In the scenario where it doesn't matter what config provider a connection comes from you could proxy this in the single config provider implementation case, in which case an aggregation abstraction to combine providers into a single structure might be more useful than making the config provider pluggable itself.

[Aiden-Ziegelaar]

I put together a draft POC PR for Postgres: #405

[zbeok]

Hello! I’m interested in this feature as well for RDS IAM; has there been any developments on this? Is there anything I can do to help this along?

[bikeshedder]

Short heads up from my side. I had a look into that over the weekend and realized a two things need to be changed:

1. The Manager trait needs a different error type. Ideally split the current Error into CreateError and RecycleError.
2. A couple of new code:
   • ConfigProvider trait with an async get_config method which returns a Result<Arc<Config>, ConfigError>
   • IntoConfigProvider with a into_config_provider method.
   • Implement ConfigProvider for Arc<Config> that just returns self.clone()
   • Implement IntoConfigProvider for Config which just returns Arc::new(self)

The second step is pretty clear to me but I really want to get the error handling right this time.

I definitely want to close #266 for good and make the RecycleError either useful or remove it altogether.

I'm working on it and plan to make a deadpool release somewhere around this or the next week. This overhaul is long overdue.