Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(download): remove curl backend #3788

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Conversation

rami3l
Copy link
Member

@rami3l rami3l commented Apr 25, 2024

@rami3l rami3l added this to the 1.28.0 milestone Apr 25, 2024
@djc
Copy link
Contributor

djc commented Apr 25, 2024

Have you taken stock of open issues referencing being solved by reverting to the curl backend?

Have you considered the alternative of using curl with the rustls TLS backend?

(Note: I am very much in favor of ditch curl and OpenSSL in favor of maximally-Rust solutions, but want to make sure we minimize the risk of getting users stuck.)

@rami3l rami3l removed this from the 1.28.0 milestone Apr 25, 2024
@rami3l
Copy link
Member Author

rami3l commented Apr 25, 2024

Have you taken stock of open issues referencing being solved by reverting to the curl backend?

@djc I've added a new section to track this.

Have you considered the alternative of using curl with the rustls TLS backend?

Hmmm, with this option we are basically introducing curl/rustls to replace curl/openssl, and eliminating the notion of TLS backend (so no more reqwest/openssl) in our repo, right?

First, I'm not sure if this would cause more problems or not, since we're introducing something new. In other words, can these existing issues be worked around with curl/rustls?
Also, I'm not quite familiar with the building process, will we be shipping rustls twice with curl/rustls + reqwest/rustls with this approach? Looks like we won't.

But indeed, this would allow the removal of openssl to happen sooner. I guess I can try both...

PS: The 1.28.0 milestone is indicative, it only means I think this should be considered in this release cycle.

@djc
Copy link
Contributor

djc commented Apr 25, 2024

So as I understand it:

  • The default currently is to use reqwest with native-tls (OpenSSL on Linux, native stacks on macOS and Windows)
  • If RUSTUP_USE_CURL is set, we use the curl backend, which defaults to openssl
  • If RUSTUP_USE_RUSTLS is set, we use reqwest with rustls

There is no way of knowing whether the curl + rustls option would solve some of the issues that people have seen with using reqwest.

Perhaps we should compile or release builds with tracing support built in so that people can give us more detailed logging without compiling their own rustup.

At least for some of the issues, there's the suggestion that native-tls might actually be at fault, such that reqwest + rustls might actually do better than the default reqwest + native-tls option.

I would advocate making the reqwest + rustls option the default first and shipping that -- I have high trust in the rustls maintainers (there are two full-time maintainers and myself) so I think any rustls issuers could be resolved pretty quickly. The one thing I would like is to ship support for rustls-platform-verifier in reqwest.

@rami3l rami3l added this to the 1.29.0 milestone Apr 26, 2024
@rami3l
Copy link
Member Author

rami3l commented Apr 26, 2024

Perhaps we should compile or release builds with tracing support built in so that people can give us more detailed logging without compiling their own rustup.

@djc I guess that's a plus, yeah.

At least for some of the issues, there's the suggestion that native-tls might actually be at fault, such that reqwest + rustls might actually do better than the default reqwest + native-tls option.

I would advocate making the reqwest + rustls option the default first and shipping that -- I have high trust in the rustls maintainers (there are two full-time maintainers and myself) so I think any rustls issuers could be resolved pretty quickly. The one thing I would like is to ship support for rustls-platform-verifier in reqwest.

Do you think now is the right time to try out rustls even without rustls-platform-verifier?

@djc
Copy link
Contributor

djc commented Apr 26, 2024

At least for some of the issues, there's the suggestion that native-tls might actually be at fault, such that reqwest + rustls might actually do better than the default reqwest + native-tls option.
I would advocate making the reqwest + rustls option the default first and shipping that -- I have high trust in the rustls maintainers (there are two full-time maintainers and myself) so I think any rustls issuers could be resolved pretty quickly. The one thing I would like is to ship support for rustls-platform-verifier in reqwest.

Do you think now is the right time to try out rustls even without rustls-platform-verifier?

Yes, I think it would make sense to flip the default for the reqwest backend to rustls, but in that case I feel like we should keep the curl backend around for now as a backup option. And should definitely include a good tracing setup so that we can ask folks for more detailed information if issues occur.

@rami3l
Copy link
Member Author

rami3l commented Apr 26, 2024

Yes, I think it would make sense to flip the default for the reqwest backend to rustls, but in that case I feel like we should keep the curl backend around for now as a backup option. And should definitely include a good tracing setup so that we can ask folks for more detailed information if issues occur.

@djc In that case I guess we can still keep this PR as a potential second step. Just to be sure, should I remove reqwest/openssl as well while changing curl/openssl to curl/rustls in that first step?

@djc
Copy link
Contributor

djc commented Apr 26, 2024

Yes, I think it would make sense to flip the default for the reqwest backend to rustls, but in that case I feel like we should keep the curl backend around for now as a backup option. And should definitely include a good tracing setup so that we can ask folks for more detailed information if issues occur.

@djc In that case I guess we can still keep this PR as a potential second step. Just to be sure, should I remove reqwest/openssl as well while changing curl/openssl to curl/rustls in that first step?

Yes, I think this could be a good second step. To be clear, I don't think we should make the curl + openssl -> curl + rustls change at the same time as promoting the reqwest + rustls backend to be the primary, since curl + openssl is currently a reliable fallback for scenarios where the default doesn't work, so I think we should keep it for a while longer. So I think the steps are:

  1. Include tracing support with tracing-subscriber (I don't think we need opentelemetry stuff?) by default, make sure we don't constrain the trace!() level statically (using tracing Cargo features)
  2. Switch the default from reqwest + native-tls to reqwest + rustls, keeping the curl + openssl fallback for now
    .. put out a release and wait a few months for feedback
  3. Try again if we can ditch curl + openssl (and reqwest + native-tls)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants