elastic / elasticsearch-rs

Official Elasticsearch Rust Client
https://www.elastic.co/guide/en/elasticsearch/client/rust-api/current/index.html
Apache License 2.0
695 stars 71 forks source link

default-tls reqwest feature necessary? #111

Closed mwilliammyers closed 4 years ago

mwilliammyers commented 4 years ago

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 commented 4 years ago

The TLS configuration of the client followed the TLS configuration of reqwest, but it looks like this changed in https://github.com/seanmonstar/reqwest/pull/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 commented 4 years ago

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...

russcam commented 4 years ago

Closing as #115 is merged