Add dynamic URL provider

[ch-sc]

Setting the URL for a database connection happens quite statically via string. A URL provider could enable scenarios for, i.e., credential rotation.

[Mingun]

Why just do not use Fn/FnOnce/FnMut trait?

[ch-sc]

For better customizability of the provider.

[ch-sc]

For example a provider such as this one would enable password rotation.

#[derive(Clone, Debug)]
struct CustomUrlProvider {
    password: Arc<RwLock<String>>,
    host: String,
    port: u32,
    user: String,
    database: String,
    region: Region
}

impl CustomUrlProvider {

    // ...

    fn rotate_password(&self) -> Result<()> {
        let new_token = crate::iam_authentication::generate_db_auth_token(
            &self.region,
            &self.host,
            &self.port,
            &self.user,
            &WebIdentityProvider::from_k8s_env()
        );

        if let Ok(mut pw) = self.password.write() {
            *pw = crate::iam_authentication::urlencode(&new_token?);
            Ok(())
        } else {
            Err(CustomError::DbPasswordRotationError())
        }
    }
}

impl UrlProvider for CustomUrlProvider {
    fn provide_url(&self) -> String {
        loop {
            if let Ok(pw) = self.password.read() {
                break format!(
                    "postgres://{}:{}@{}:{}/{}",
                    self.user, *pw, self.host, self.port, self.database
                );
            } else {
                warn!("Couldn't acquire lock to build DB password")
            }
        }
    }
}

[weiznich]

I think it should be easy to just provide a default implementation all types that implement Fn() somehow.

[ch-sc]

Sure, it's possible to use a closure instead. Nonetheless, to me it seems more handy to use a new trait. A custom provider can have a state and easily be dealing with connection updates or different test scenarios.

On the contrary, it's really just about making the db connection more dynamic and a Fn() would suffice.

[weiznich]

To be clear: I'm not talking about removing UrlProvider, just about adding a generic impl<T:Fn()> UrlProvider for T to allow both variants.

[weiznich]

Can you please provide some more justification why having this feature is important for the general use case and why this needs to be implemented in diesel itself and not in some third party crate?

[weiznich]

@ch-sc I'm still waiting for a justification here why this is a feature that is important and why this need to be implemented in diesel itself.

[ch-sc]

@weiznich Sorry it took me so long coming back to you.

I believe it is good practice to use credential rotation for databases [1]. Therefore, I would expect diesel to support users with a dynamic way on how to establish connection. An UrlProvider is not only a simply way on how to implement this, it is also widely used by other libraries that follow a similar purpose like diesel.

Do you see alternative implementations to support users with this?

[1] Rotate Amazon RDS database credentials automatically with AWS Secrets Manager

[weiznich]

As already written above: I think I understand why key rotation is useful in general. What I'm looking for is a justification why this feature needs to be implemented/supported in diesel itself. From what I see there are the following alternatives (likely incomplete list):

• Replacing the connection pool on key rotation. This would be possible work around issues with stale connections due to changed credentials.
• Everything inside of diesel::r2d2 can also be implemented as third party crate, so just provide a additional crate that implements a diesel connection pool that supports key rotation.

As adding additional code is adding a code that needs to be maintained by the diesel developers itself I just want to be sure that's truly necessary here.

[svenwb]

I would like to reinitiate the conversation on that issue as I followed up on the alternatives suggested by @weiznich

• replacing the whole pool
  • it might be a rather inefficient workaround as it re-initiate all connection every x minutes while the new credentials are only required if the pool needs to open a new connection. Existing connections remain valid beyond credential rotation. So I would park this alternative for now
• third party crate / external code
  • I prefer this solution as well. Downside of the current release 1.4.6 is that you have duplicate most of the code from diesel/src/r2d2.rs. Mainly because the Connection trait is only specifically implement for ConnectionManager

  impl<C> Connection for PooledConnection<ConnectionManager<C>> 
  where
     C: Connection<TransactionManager = AnsiTransactionManager> + Send + 'static,
     C::Backend: UsesAnsiSavepointSyntax,

  • This part was simplified a lot in the current master

  impl<M> Connection for PooledConnection<M>
  where
      M: ManageConnection,
      M::Connection: Connection + R2D2Connection + Send + 'static,

  • which allows to only provide a custom implementation for ManageConnection with a simple third party ConnectionManager

struct ConnectionManager {
    url_provider: Arc<CustomUrlProvider>,
}

impl ManageConnection for ConnectionManager {
    type Connection = PgConnection;
    type Error = Error;

    fn connect(&self) -> result::Result<PgConnection, Self::Error> {
        let db_url = self.url_provider.provide_url();
        PgConnection::establish(&db_url).map_err(Error::ConnectionError)
    }

    fn is_valid(&self, conn: &mut Self::Connection) -> result::Result<(), Self::Error> {
        conn.execute("SELECT 1")
            .map(|_| ())
            .map_err(Error::QueryError)
    }

    fn has_broken(&self, _conn: &mut Self::Connection) -> bool {
        false
    }
}

Are there any plans to put this into a release version? It seem there is a lot going on for some time already to make it a version 2 release. Would it be possible to include this particular change in a 1.4.x patch?

Thanks
Sven

[weiznich]

@svenwb Thanks for that writeup. We don't plan to release any feature release before the upcoming 2.0 release, so there is not really a chance to include that particular change there. Even if we would plan another release for the 1.x series this change would likely not be included as it isn't a semver compatible change.

[svenwb]

@weiznich thanks for the quick reply and feedback. Do you have a rough timeline for the 2.0 release by any chance? It is not necessarily urgent on my side, but it would help me to coordinate a bit.

[weiznich]

@svenwb As this is a project run by volunteers in their free time we do not give any ETA's of releases.