sqlx icon indicating copy to clipboard operation
sqlx copied to clipboard

Add dynamic credentials support to Postgres

Open NeelChotai opened this issue 3 years ago • 5 comments

Fixes https://github.com/launchbadge/sqlx/issues/445.

We've been using this in development to refresh RDS tokens and it works pretty well. Some outstanding work is left to do error typing properly.

NeelChotai avatar Jun 30 '22 10:06 NeelChotai

I just closed #1349 as not-planned but it just occurred to me that if we pivot this to being callback that provides ConnectOptions on-demand then it can cover both use-cases plus pretty much any thing else conceivable, and also wouldn't require baking this feature into every database's ConnectOptions separately.

abonander avatar Jul 01 '22 20:07 abonander

I just closed #1349 as not-planned but it just occurred to me that if we pivot this to being callback that provides ConnectOptions on-demand then it can cover both use-cases plus pretty much any thing else conceivable, and also wouldn't require baking this feature into every database's ConnectOptions separately.

That seems like a good idea! I had a quick look at generalizing this approach in the way you described and the only thing that immediately jumps out as a potential blocker is: https://github.com/launchbadge/sqlx/blob/79ebd3005a404d2a4452ee2a0284b039e96d84a2/sqlx-core/src/pool/mod.rs#L512-L519

We would need to execute the callback in order to determine the kind, and if the callback returns a future this method would have to become async as well. This feels like it would be a surprising change if I were using the Any driver.

jamiebrynes7 avatar Jul 04 '22 11:07 jamiebrynes7

Hmm, that is a problem, yeah. I would probably say that we shouldn't have the callback just return AnyConnectOptions in that case as changing database kinds on the fly sounds like a nightmare to reason about.

I'm conceiving of a new trait and impls for this:

pub trait ConnectOptionsProvider<DB: Database>: Send + Sync + 'static {
    type Error: Into<Box<dyn Error>>;
    type Future: Future<Output = Result<DB::ConnectOptions, Self::Error>>;

    fn any_kind(&self) -> AnyKind;

    fn provide_connect_options(&self) -> Self::Future;
}

#[cfg(feature = "postgres")]
impl<F, Fut, E> ConnectOptionsProvider<Postgres> for F
where
    F: Fn() -> Fut + Send + Sync + 'static,
    E: Into<Box<dyn Error>>,
    Fut: Future<Output = Result<PgConnectOptions, E>> 
{
    type Error = E;
    type Future = Fut;

    fn any_kind(&self) -> AnyKind { AnyKind::Postgres }

    fn provide_connect_options(&self) -> Self::Future { (self)() }
}

// plus impls for other dbs

We can't have a blanket impl of ConnectOptionsProvider<Any> for these types because of coherence issues, but we can use wrapper types. I'm thinking of something like this:

// Bikeshedding?
#[cfg(feature = "postgres")]
pub fn provide_pg_options_as_any<P: ConnectOptionsProvider<Postgres>>(provider: P) -> impl ConnectOptionsProvider<Any> {
     struct PgWrapper<P>(P);

     impl<P> ConnectOptionsProvider<Any> for PgWrapper<P>
     where P: ConnectOptionsProvider<Postgres> {
         type Error = P::Error;
         type Future = futures_util::future::OkInto<P::Future, AnyConnectOptions>;

         fn any_kind(&self) -> AnyKind { AnyKind::Postgres }

         fn provide_connect_options(&self) -> Self::Future { self.0.provide_connect_options().ok_into() }
}

And then sqlx::Error would gain a new error variant, ProviderError(Box<dyn Error>)

abonander avatar Jul 04 '22 22:07 abonander

@abonander I took that idea and ran with it. I think its definitely feasible and I was able to thread it through until I ran into one blocker which is the following:

https://github.com/launchbadge/sqlx/blob/58712ae55263857c60c0df5f1adac8a076a74fab/sqlx-core/src/pool/mod.rs#L493

If the pool holds onto the ConnectOptionsProvider, we cannot evaluate this method synchronously. I expect we wouldn't want to fork the Pool<DB> type, but maybe we could put this feature (ConnectOptionProvider) behind a feature flag which could remove that method/make it async.

What do you think?

jamiebrynes7 avatar Jul 26 '22 14:07 jamiebrynes7

I think the easiest fix would just be to panic if an async ConnectOptionsProvider was set. As long as it's well documented I don't see any problem with that.

abonander avatar Aug 04 '22 01:08 abonander

Closing due to inactivity from the original author. @jamiebrynes7 I don't think I ever saw a PR from you, I'm still interested in your approach.

abonander avatar Aug 26 '22 21:08 abonander