feat(http-transport-reqwest): allow tls options#7798
Conversation
dosubot
Bot
added
size:XL
|
@Xuanwo Can you have a look on the default feature proposed in PR? Then I can resolve build issues in CI. |
| layers-throttle = ["dep:opendal-layer-throttle"] | ||
| layers-timeout = ["dep:opendal-layer-timeout"] | ||
| layers-tracing = ["dep:opendal-layer-tracing"] | ||
| reqwest-rustls-no-provider-tls = [ |
There was a problem hiding this comment.
We should keep them for deprecation
| # Use Rustls without a built-in crypto provider. | ||
| rustls-no-provider = ["reqwest/rustls-no-provider"] | ||
| # Use Rustls with the aws-lc-rs crypto provider and bundled Mozilla root certificates. | ||
| webpki-roots = [ |
There was a problem hiding this comment.
Should this be rustls-wekpki-roots?
| reqwest = { version = "0.13.4", features = [ | ||
| "stream", | ||
| ], default-features = false } | ||
| rustls = { version = "0.23", optional = true, default-features = false } |
There was a problem hiding this comment.
I expect we didn't depend on rustls directly
There was a problem hiding this comment.
Sure, I will change features.
| The `native-tls` feature sidesteps both axes by delegating everything to | ||
| the OS TLS library (SChannel / Secure Transport / OpenSSL). | ||
|
|
||
| ### Feature matrix |
There was a problem hiding this comment.
The matrix here seems not reflect the two axes mentioned before. Where is ring and where is native certs?
There was a problem hiding this comment.
I didn't include ring as a backend. The documentation gives information for readers to understand these concepts and crates. Do we want to provide ring as a choice for backend? Because users will have to compile their own binary. If they know that, they will be capable of building 50 line configurations. For us, what default features do we want to ship? I am only considering native certs.
native-tls is the native certs.
| ```rust | ||
| use std::time::Duration; | ||
|
|
||
| use opendal_http_transport_reqwest::ReqwestTlsBackend; |
There was a problem hiding this comment.
I didn't expect we need to implement this. Is there a reason why we didn't just use things from reqwest?
There was a problem hiding this comment.
This is mainly a consideration of what features do we want to compile and ship. e.g.:
- we ship rustls, ring, all differents certs
- we ship one feature of rustls
- ...other combinations between these 2
Bindings will use this crate mostly so compilation size is also a concern.
2 participants
Which issue does this PR close?
Closes #7762.
Rationale for this change
Allow easier configurations of TLS backend to reqwest.
What changes are included in this PR?
Are there any user-facing changes?
Yes, for http-transport-reqwest. The default configuration for http-transport-reqwest is to compile native-tls support.
Users who wants rustls must compile their own http-transport-reqwest.
AI Usage Statement
Claude Opus 4.6 helps implementation.