deislabs / bindle

Bindle: Object Storage for Collections
Apache License 2.0
263 stars 37 forks source link

Replaced reqwest's `default-tls` with `rustls-tls` #340

Closed brooksmtownsend closed 2 years ago

brooksmtownsend commented 2 years ago

This PR replaces the default TLS feature flag for reqwest with rustls-tls in order to eliminate the openssl dependency when using the reqwest crate. I went through a few iterations but couldn't quite find a way to cleanly keep the default-tls feature by default and then able rustls-tls optionally here, but I'm happy to implement that to keep the default behavior of this crate the same if desired.

My main reason for doing this is so that I can depend on bindle without bringing in the openssl requirement which can be difficult to deploy across different versions of Linux

bacongobbler commented 2 years ago

I know Krustlet uses a feature called rustls-tls and native-tls to differentiate between the two, with native-tls being the default. Would it be valuable to make this a new feature flag?

brooksmtownsend commented 2 years ago

I know Krustlet uses a feature called rustls-tls and native-tls to differentiate between the two, with native-tls being the default. Would it be valuable to make this a new feature flag?

I'd be happy to make this the way you can select the other feature, it would just require that if you disable default features you would have to specify native-tls or rustls-tls if you brought in the server or client feature

edit: if you don't specify either tls feature when bringing in client for example you get some nasty errors

error[E0277]: the size for values of type `[u8]` cannot be known at compilation time
   --> src/client/mod.rs:683:9
    |
683 |         Err(_) => return None,
    |         ^^^^^^ doesn't have a size known at compile-time
    |
    = help: the trait `Sized` is not implemented for `[u8]`
note: required by a bound in `Err`
   --> /Users/brooks/.rustup/toolchains/stable-aarch64-apple-darwin/lib/rustlib/src/rust/library/core/src/result.rs:504:17
    |
504 | pub enum Result<T, E> {
    |                 ^ required by this bound in `Err`

error[E0277]: the size for values of type `[u8]` cannot be known at compilation time
   --> src/client/tokens.rs:222:30
    |
222 |         > = toml::from_slice(&login_resp.bytes().await?)?;
    |             ---------------- ^^^^^^^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
    |             |
    |             required by a bound introduced by this call
    |
    = help: the trait `Sized` is not implemented for `[u8]`
    = note: all local variables must have a statically known size
    = help: unsized locals are gated as an unstable feature

Some errors have detailed explanations: E0277, E0282, E0412, E0425, E0432, E0433, E0599.
For more information about an error, try `rustc --explain E0277`.
error: could not compile `bindle` due to 52 previous errors
brooksmtownsend commented 2 years ago

Did some good pairing with @thomastaylor312 and got a good way to make this work with the ? syntax with features 😄 Should be ready for review / tests

thomastaylor312 commented 2 years ago

Since this is just a dependency change and I paired with @brooksmtownsend to check it, I'll go ahead and merge this once tests pass