sqlx-postgres, sqlx-aws: impl dynamic passwords#3851
sqlx-postgres, sqlx-aws: impl dynamic passwords#3851marcbowes wants to merge 1 commit intolaunchbadge:mainfrom
Conversation
marcbowes
left a comment
marcbowes
left a comment
There was a problem hiding this comment.
Left some comments for the reviewers :)
| async fn main() -> Result<(), BoxError> { | ||
| let hostname = std::env::var("DSQL_CLUSTER_ENDPOINT") | ||
| .expect("please set DSQL_CLUSTER_ENDPOINT is your environment"); |
There was a problem hiding this comment.
For reviewers: I didn't want to write an automated test for this, because the test would fail unless the developer had AWS credentials. This seems like a good compromise for now. Would be interested to know if there's another viable approach.
| /// [`DsqlIamProvider::new`]. | ||
| /// | ||
| /// | ||
| /// ```ignore |
There was a problem hiding this comment.
For reviewers: I don't want doc tests to try and load AWS credentials :)
| let password = match self.password.get() { | ||
| Ok(password) => Some(password), | ||
| Err(err) => { | ||
| // XXX: This method is infallible. To avoid a backwards breaking |
| # "sqlx-bench", | ||
| "sqlx-mysql", | ||
| "sqlx-postgres", | ||
| "sqlx-aws", |
There was a problem hiding this comment.
As per the PR overview, I was originally going to build this crate out-of-tree (building on the new functionality), but implemented it in-tree as a way of verifying the new functionality was actually useful. After doing all that work, it now seems like leaving it in-tree is a reasonable idea. Here are the pros and cons I thought of:
pros:
- it's easy to align versions in-tree because of the Cargo workspace
- it makes the ecosystem of sqlx-* crates more easy to grok
cons:
- automated testing would require AWS credentials in this repo
- you (maintainers) might not want to maintain this
This commit extends PgConnectOptions to support dynamic passwords, while maintaining the API and memory layout of the previous (static-only) implementation. `PgConnectOptions::password` now takes anything that can be turned into a `PasswordOption`, which has support for providers. Providers have a single `fn password(&self)`. I've chosen to implement the `PasswordProvider` trait as a sync function, because there are usage paths (like `ConnectionOptions::to_url_lossy`) that are not `async`. This decision means that providers that need to do async IO need to do so internally, such as by spawning a task. I've included a provider for AWS Aurora DSQL as part of this commit, to show how a more complex provider can be implemented. I was originally thinking of publishing this crate separately (outside the sqlx tree), but it might make sense to keep it in-tree.
We're not prepared to accept new in-tree drivers at this time, especially for a proprietary product like AWS Aurora. Writing the driver is one thing, maintaining it long-term is another. For details see the recently added FAQ answer: https://github.com/launchbadge/sqlx/blob/main/FAQ.md#can-sqlx-add-support-for-new-databases As for the core use-case of this PR, I already have a more generic solution that works for all databases in #3582 (the |
Makes sense :)
Can I help out with #3582? Sounds like I should close this PR. |
|
See other PR |
2 participants
This commit extends PgConnectOptions to support dynamic passwords, while maintaining the API and memory layout of the previous (static-only) implementation.
PgConnectOptions::passwordnow takes anything that can be turned into aPasswordOption, which has support for providers. Providers have a singlefn password(&self).I've chosen to implement the
PasswordProvidertrait as a sync function, because there are usage paths (likeConnectionOptions::to_url_lossy) that are notasync. This decision means that providers that need to do async IO need to do so internally, such as by spawning a task.I've included a provider for AWS Aurora DSQL as part of this commit, to show how a more complex provider can be implemented. I was originally thinking of publishing this crate separately (outside the sqlx tree), but it might make sense to keep it in-tree.
Does your PR solve an issue?
fixes #445
Is this a breaking change?
No. At least, I tried really hard to make it not be!