Skip to content

feat: make rustls the default tls implementation #674

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

Merged
merged 3 commits into from
Sep 14, 2022

Conversation

sunng87
Copy link
Contributor

@sunng87 sunng87 commented Sep 14, 2022

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

In the recent changes of switch hyper to reqwest/ureq, the rustls feature is broken for having openssl included. This patch rearranges our feature set, by:

  • introducing native-tls feature that enabled by default, it uses openssl for reqwest (ureq has no openssl support unfortunately)
  • removing rustls-tls from reqwest because it's already included in rustls-tls-native-roots
  • turning off default features for reqwest to avoid openssl dependencies when using rustls

@Xuanwo
Copy link
Member

Xuanwo commented Sep 14, 2022

Thanks for your contribution!

I don't like openssl either. I prefer to make rustls as default and add a new feature called openssl. What do you think?

@sunng87
Copy link
Contributor Author

sunng87 commented Sep 14, 2022

Sounds good. And how about if we going one step further by totally remove native-tls and make rustls the default and the only option for TLS? Because ureq has no openssl support, naming a native-tls feature for half of fact is also quite confusing.

@Xuanwo
Copy link
Member

Xuanwo commented Sep 14, 2022

And how about if we going one step further by totally remove native-tls and make rustls the default and the only option for TLS?

Let's go for a try! We can explore all the possibilities before OpenDAL reaches v1.0.

Don't forget to update the PR's title.

@sunng87 sunng87 changed the title fix: rearrange feature rustls to exclude openssl dependencies feat: make rustls the default tls implementation Sep 14, 2022
@Xuanwo
Copy link
Member

Xuanwo commented Sep 14, 2022

Great work!

@Xuanwo Xuanwo merged commit a4beeb4 into apache:main Sep 14, 2022
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.

2 participants