-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add dynamic URL provider #2426
base: master
Are you sure you want to change the base?
Add dynamic URL provider #2426
Conversation
@@ -175,6 +192,13 @@ where | |||
} | |||
} | |||
|
|||
/// Trait to provide the user the option to change parameters of the URL at runtime. | |||
/// E.g. to implement password rotation | |||
pub trait UrlProvider: Send + Sync + fmt::Debug + 'static { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why just do not use Fn/FnOnce/FnMut
trait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For better customizability of the provider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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")
}
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be easy to just provide a default implementation all types that implement Fn()
somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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? |
@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. |
@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 Do you see alternative implementations to support users with this? [1] Rotate Amazon RDS database credentials automatically with AWS Secrets Manager |
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):
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. |
I would like to reinitiate the conversation on that issue as I followed up on the alternatives suggested by @weiznich
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 Thanks |
@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. |
@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. |
@svenwb As this is a project run by volunteers in their free time we do not give any ETA's of releases. |
Setting the URL for a database connection happens quite statically via string. A URL provider could enable scenarios for, i.e., credential rotation.