wyyerd / stripe-rs

Rust API bindings for the Stripe HTTP API.
Apache License 2.0
223 stars 88 forks source link

Add rustls support behind a non-default feature. #125

Closed kiljacken closed 4 years ago

kiljacken commented 4 years ago

This commit adds support for using rustls with stripe-rs. This avoids a dependency on native OpenSSL for people who'd rather avoid that. This is useful when building a static binary as you don't need to build a statically linkable OpenSSL first.

kiljacken commented 4 years ago

This should be ready for review now.

This is a breaking change for users using no-default-features as we need the default tls implementation to be a feature to not pull in OpenSSL no matter what. If rust-lang/cargo#5954 was implemented this would not be necessary, but that's not how the world is currently.

kestred commented 4 years ago

This is a breaking change for users using no-default-features as we need the default tls implementation to be a feature to not pull in OpenSSL no matter what.

Backward compatibility can be maintained by using a similar pattern that I adopted for blocking vs. async (where previously blocking was the default).

In this pattern, the newly introduced option becomes:

#[cfg(feature = "rustls-tls")]

And the existing feature is handled with:

#[cfg(any(feature = "default-tls", not(feature = "rustls-tls")))]

// Alternatively, just the below, with no separate feature flag:
#[cfg(not(feature = "rustls-tls"))]
kiljacken commented 4 years ago

The main issue with this is that we can't make the hyper-tls dependency optional if it's not behind a feature, so you'll have to link a native openssl no matter what with that approach. I'm not sure if there's a great way to make this change without being breaking, without the linked cargo issue being implemented at some point.

The main reason i opened this PR was to eleminate the need to link to OpenSSL to simplify the flow of building a static binary for e.g. container or AWS Lambda use.

kestred commented 4 years ago

The next release will be a "major" version, so I'll merge this in.