A couple issues creating a Postgresql wrapper connection for Amazon Aurora DSQL

[txase]

Setup

Versions

• Rust: 1.83.0
• Diesel: 2.2.6
• Database: Amazon Aurora DSQL Preview
• Operating System: MacOS

Feature Flags

• diesel: postgres, i-implement-a-third-party-backend-and-opt-into-breaking-changes

Problem Description

I hit a couple annoying API issues with the diesel Connection traits while creating a wrapper for DSQL around the built-in Postgresql connection type. I'm raising these concerns together as a single issue, but I'm happy to fork them to separate issues if the maintainers find them to be valid and want to deal with them separately. None of these are show-stoppers, they just made it "annoying" to implement.

I may also be implementing this at the wrong layer of the stack. If so, please provide guidance and I'll happily close this issue!

What are you trying to accomplish?

I wanted to play with DSQL for a side project that already uses Postgresql. FWIW, after removing foreign key constraints, Diesel seemed to "just work" once I got my connection driver working. I added this code directly to my side project, but I assume a production-ready version should exist as a separate crate.

What issues were found?

• Connection::establish is synchronous, but we need to do some async stuff for DSQL to generate temporary auth tokens. I made it work by spawning and blocking on threads as needed, but it feels like this method should be async.
• ConnectionError doesn't implement the Clone Trait, which makes error handling onerous.
• It feels like a wrapper connection, like this is, shouldn't need to forward so many traits and opt into the i-implement-a-third-party-backend-and-opt-into-breaking-changes feature flag. All this does is transform one connection string into a postgresql connection string. The breaking-changes feature flag concerns me most.

Steps to reproduce

You can find my code at this gist. See line 96 for annoyance in handling ConnectionError without Clone. Search for thread to find the two places we need to perform asynchronous processing to generate a postgres connection string.

Checklist

• I have already looked over the issue tracker and the discussion forum for similar possible closed issues.

[weiznich]

I've converted this to a discussion as this is not a bug report but a support question. Our issue tracker is reserved for actual bug reports, so please don't fill support questions there again.

Connection::establish is synchronous, but we need to do some async stuff for DSQL to generate temporary auth tokens. I made it work by spawning and blocking on threads as needed, but it feels like this method should be async.

If you want an async connection interface you need to use diesel_async::AsyncConnection. The interface provided by diesel is on purpose not async.

ConnectionError doesn't implement the Clone Trait, which makes error handling onerous.

That's on purpose. Error types in rust are often not Clone as they often contain type erased data, which are in turn hard to clone.

It feels like a wrapper connection, like this is, shouldn't need to forward so many traits and opt into the i-implement-a-third-party-backend-and-opt-into-breaking-changes feature flag. All this does is transform one connection string into a postgresql connection string. The breaking-changes feature flag concerns me most.

I'm sorry but that's again on purpose I we don't want to commit to stability for these interfaces. This won't change just because you feel that it should be stable. Stabilizing these kind of interfaces likely requires a lot of work to stabilize potentially wanted language features. As for the number of traits: Again that's on purpose as we need this granularity to support all the different features we want to support with diesel.

That all written: I'm not sure if you really want to even have a custom connection implementation for this or if it wouldn't be much easier to just have a single free standing function that is called before establish and that transforms the URL as required.

[txase]

The async connection part looks helpful! Thanks for pointing that out :).

I haven’t been a Rust developer long enough to know whether what you say about errors and cloning makes sense. I only know that I can’t remember hitting another error handling issue like this elsewhere. It smells off to me, but I may be wrong.

Thanks for the feedback overall, it’s much appreciated. I’ll think about whether this should be implemented before the connection is built rather than after. But if it’s before, then every project has to be updated individually rather than just providing a new connection driver.

[marcbowes]

but we need to do some async stuff for DSQL to generate temporary auth tokens

Generating auth tokens is a CPU bound task. You don't need it to be async.

[txase]

In case anyone wanders by this issue: No, auth tokens in this case are not a CPU bound task. Auth tokens are fetched from an external service. This is the best I could manage, which blocks on a thread spawned in order to asynchronously fetch auth tokens:

https://github.com/txase/vaultwarden/blob/3e886e670e7e026c188af1d856ca8ae2cc3113b5/src/db/dsql.rs#L163-L170

I still need to poke more at diesel_async, but it's a heavy lift to rebase an existing synchronous codebase onto.

[txase]

Actually, I take that back (partly). It looks like the DSQL Rust SDK generates auth tokens synchronously after the AWS SDK Credentials are loaded asynchronously (https://github.com/awslabs/aws-sdk-rust/blob/42c177d0d9af71180965bdca2e62648088d4b1ae/sdk/dsql/src/auth_token.rs#L88).

This feels a bit like two libraries that are in disagreement about how each other should work. I don't think the DSQL Rust SDK is inherently wrong to provide an easy-to-use asynchronous method that takes an AWS Config object. But by not providing a synchronous version it forces pure-synchronous consumers to create a blocking thread to generate auth tokens.

I now realize the workaround for a synchronous user of the DSQL SDK would be to load the AWS Credentials elsewhere then copy the rest of the auth token generation function that is synchronous into the diesel connection implementation. But the AWS DSQL SDK should also provide a method that takes a previously-resolved AWS Credentials object and synchronously generate SDQL auth tokens.

[marcbowes]

The problem is that credentials expire, and have to be reloaded when they do. Most AWS customers use https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/iam-roles-for-amazon-ec2.html or similar. In this setup, EC2 automatically pushes temporary credentials to your instance which are only valid for some time. Before they expire, EC2 will refresh them.

The SDK loads these credentials and keeps track of their expiry, reloading them as needed. Loading creds is a HTTP request, and is built on top of async. Thus, the entire credentials provider system is async, because otherwise providers wouldn't be able to do async loads.

This all bubbles up to the token generator, which may have to refresh creds. If you request token right at the moment your temporary credentials expire, you must refresh the AWS creds otherwise your token will not be valid.

There are only two ways to expose a sync token generator (that I'm aware of). The first way is to require static credentials, or at least a guaranteed-sync credential provider chain. The second way is to hide the async by moving generation to a background task. The second way still requires an async runtime, but makes it possible to slot into sync interfaces. For example see https://github.com/launchbadge/sqlx/pull/3851/files (this PR is canceled, but the pattern still works).

I hope that helps

[marcbowes]

But the AWS DSQL SDK should also provide a method that takes a previously-resolved AWS Credentials object and synchronously generate SDQL auth tokens.

This seems reasonable to me, I've let the team know.