ChrisMacNaughton / vault-rs

https://docs.rs/hashicorp_vault
61 stars 27 forks source link

Usage from async contexts #54

Open SafariMonkey opened 3 years ago

SafariMonkey commented 3 years ago

I have been looking into laying the groundwork for moving some Vault-dependent microservices to Rust, likely using async. I was somewhat surprised to see that this client still exposes a totally blocking API, even though e.g. reqwest's primary API is now blocking, and (I imagine) many of the applications which would want to talk to Vault would be using async. As it stands, an async application would have to spawn_blocking the request, which would then internally use async anyway (in the reqwest implementation).

Until #51, I believe writing an async implementation and basing the (current) synchronous API on it could have been done in a non-breaking way, but it seems that ship has now sailed for 2.x. Such an approach would now require two separate implementations.

What are your thoughts on catering to the async use case?

SafariMonkey commented 3 years ago

I made an attempt at an async conversion here. It is a breaking change (obviously), but it's almost all just adding .await and async, plus some minor workarounds to extract the response body, as the async body doesn't implement Read or AsyncRead. The tests all passed as soon as they built (and once an async runtime compatibility issue was resolved).

This is not a PR as such, just me demonstrating what an async-ifying change could look like.

ChrisMacNaughton commented 3 years ago

While I'm conceptually not against doing async, this library is likely to stay with sync in the HTTP client, even on the next major version, as I intend to use ureq to minimize dependencies of the crate. Ureq's stance on async is that it does, and always will, use synchronous IO for operations as it dramatically simplifies the requirements and dependencies.

ehiggs commented 1 year ago

The reasons for not doing async in the Ureq repo make sense for them. But Vault is often used by backend services to manage private keys. These backend services manage many connections already using async. Keeping the vault client sync means calling:

let cloned_client = client.clone(); // client is in an arc to allow it to be cloned since it doesnt support cloning itself.. :(
let cloned_key = key.clone();
let result = tokio::task::spawn_blocking( move || {cloned_client.get_secret(cloned_key}).await??;

What a pain the bum for each call to grab a secret! Also, most people using web stuff have reqwests already installed and the interface is very well done and easy to use. So adding a whole other stack seems like adding more dependencies than you hope to save here. So I hope you reconsider - or whomever maintains this reconsiders.