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

default-tls reqwest feature necessary? #111

Closed
mwilliammyers opened this issue Jun 19, 2020 · 3 comments
Closed

default-tls reqwest feature necessary? #111

mwilliammyers opened this issue Jun 19, 2020 · 3 comments
Labels
discuss General discussion to reach decision

Comments

@mwilliammyers
Copy link
Contributor

I spent a bit of time figuring out why my static musl binary required openssl even though all of my dependencies that use reqwest use the rustls-tls feature and disabled the default feature.

I think I traced it to elasticsearch adding the default-tls feature, which brings in hyper-tls, which brings in native-tls, which depends on openssl (on linux) :/

TL;DR: Do we need the default-tls feature?

@russcam
Copy link
Contributor

russcam commented Jun 23, 2020

The TLS configuration of the client followed the TLS configuration of reqwest, but it looks like this changed in seanmonstar/reqwest#749.

I think I traced it to elasticsearch adding the default-tls feature, which brings in hyper-tls, which brings in native-tls, which depends on openssl (on linux) :/

Looking at https://github.com/seanmonstar/reqwest/blame/71386d8734f7904e48e4b3238f918eaecdabe7f3/Cargo.toml#L30, it looks like reqwest's default-tls brings in hyper-tls, so if default-tls were removed from elasticsearch, reqwest's default-tls will still bring in native-tls.

I think we want support for TLS by default. If I understand the issue correctly, if

[features]
default = ["native-tls"]

# optional TLS
native-tls = ["reqwest/native-tls"]
rustls-tls = ["reqwest/rustls-tls"]

[dependencies]
reqwest = { version = "~0.10", default-features = false, features = ["default-tls", "gzip", "json"] }

is changed to

[features]
default = ["native-tls"]

# optional TLS
native-tls = ["reqwest/native-tls"]
rustls-tls = ["reqwest/rustls-tls"]

[dependencies]
reqwest = { version = "~0.10", default-features = false, features = ["gzip", "json"] }

Then

[dependencies]
elasticsearch = { version = "*", default-features = false, features = ["rustls-tls"] }

should not pull in native-tls dependencies?

@mwilliammyers
Copy link
Contributor Author

Yep, I think you are correct, we should change it to:

[features]
default = [“native-tls”]

# optional TLS
native-tls = [“reqwest/native-tls”]
rustls-tls = [“reqwest/rustls-tls”]

[dependencies]
reqwest = { version = “~0.10”, default-features = false, features = [“gzip”, “json”] }

I can open a PR to do that tomorrow...

mwilliammyers added a commit to mwilliammyers/elasticsearch-rs that referenced this issue Jul 2, 2020
@russcam russcam added the discuss General discussion to reach decision label Jul 13, 2020
@russcam
Copy link
Contributor

russcam commented Jul 13, 2020

Closing as #115 is merged

@russcam russcam closed this as completed Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss General discussion to reach decision
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants