[ENHANCEMENT] Add pass through features for reqwest

[mwilliammyers]

In an effort to reduce dependency duplication as reqwest is a very commonly used crate; it would be handy to have some features we can pass through to reqwest.

Some candidates are:

• rustls-tls = ["reqwest/rustls-tls"]
• cookies = ["reqwest/cookies"]
• socks = ["reqwest/socks"]

Do we need the native-tls feature specifically? It looks like it adds some more features vs. default-tls? Can rustls-tls be used instead? rusttls-tls makes building a static binary via musl libc much easier...

[russcam]

Thanks for opening @mwilliammyers. I hope you don't mind, but I have some questions about pass through features, as I'm not that familiar with this convention. Is there a good resource where I can learn more?

Is it common to allow pass through features in this manner? What are the benefits in doing so?

Some candidates are: cookies = ["reqwest/cookies"] socks = ["reqwest/socks"]

These features aren't needed by the client; does it make sense to also make them pass through?

Do we need the native-tls feature specifically? It looks like it adds some more features vs. default-tls? Can rustls-tls be used instead? rusttls-tls makes building a static binary via musl libc much easier...

Is rustls-tls considered a better option for consumers?

[mwilliammyers]

No problem!

They are mentioned briefly in the Cargo.toml manifest format docs. There are some here for elastic-rs/elastic.

The main benefit of using them allows the end user more control over the dependencies. It is fairly common to see this feature used for popular crates, like reqwest, because it helps cargo de-duplicate dependencies. Without it, a reqwest+default-tls (with openssl) and a reqwest+rustls-tls will be compiled. You can imagine how this can end up spiraling out of control as more dependencies are added (my project has close to 300 and counting). This will most likely be pipelined so that they will built at the same time (on a multi-core machine at least) and they will probably not both end up in the resulting binary, but at the same time, it is handy to ensure a faster build by giving the end user more control over the dependencies.

rustls-tls basically just makes building a static binary much easier (which we do in production) because you don't need openssl, which can be a pain to install. Although it, nor ring which does the heavy crypto lifting, haven't been officially audited for security; they are both high quality crates and are really popular (although admittedly, they are not the default for reqwest...)

Really I am just reaching for low-hanging "nice-to-haves" 😃

[russcam]

Thanks for the links @mwilliammyers. Is there a way to specify that some dependency features are required, whilst some are optional based on being opted into through passthrough features? It's not clear to me reading the manifest format docs how this should be constructed.

For example, for the reqwest dependency, gzip and json features must always be required. In addition, I'd like to pass through a feature as default (native-tls), and then a bunch as optional (ones you've mentioned above)

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

default-tls = ["reqwest/default-tls"]
native-tls = ["reqwest/native-tls"]
rustls-tls = ["reqwest/rustls-tls"]
cookies = ["reqwest/cookies"]
socks = ["reqwest/socks"]

[dependencies]
# required features
reqwest = { version = "^0.10", features = ["gzip", "json"  ] }

Should the reqwest dependency also specify all of the optional features, and they'll only be included if they're features opted into in elasticsearch? I think I could solve it by making gzip and json default features, but then a consumer could opt out of them, which for json at least, would break the client.

[mwilliammyers]

Good question. Off the top of my head that looked correct but I wasn't 100% sure, so I used the above in a test project and it compiled just fine. When I run cargo tree --features socks, it adds in tokio-socks and its dependencies, so I think it is working as you would expect? 🤷‍♂

[russcam]

I've implemented native-tls and rustls-tls features in the client for now, passing through to reqwest's features and attributing in the elasticsearch source for code relying on these features.

I've been thinking about having pass-through features for the other features of reqwest, and it doesn't feel right to expose these in the client. On one hand, I can see how they could help with compilation, but on the other, the elasticsearch crate would then be exposing features of dependencies that are not relevant to the crate.

[mwilliammyers]

Yeah, sounds good to me.