Make tls features additive

[MikailBag]

Changes: create an enum with available TLS backends. Library user explicitly chooses it and passes it to kube.

Draft: I haven't updated reqwest & websocket creation code yet.

[MikailBag]

Turns out, async-tungstenite does not currently support several TLS backends. If several features are enabled, it just selects one.

[MikailBag]

Filed sdroege/async-tungstenite#78

[clux]

comments

[clux]

comments here are switched around

[clux]

This seems to be used in examples as a main interface contrary to this comment.

[clux]

Because this is an interface for Client construction, this effectively pushes a compile time error to a runtime error?

[clux]

Having an argument to try_default makes it no longer a default.

[clux]

This is an interesting one, thanks for doing this. I like the way this factors out tls into its own module, it was starting to grow infect a lot of other files. I am a bit on the fence of the Tls::pick mechanism as it stands. Left some questions/observations.

[clux]

One point here: having this be a non-additive feature gave us a guarantee that you only got one of the TLS stacks into the dependency trees if it built. In an additive mode, we run the risk of multiple kube uses in various Cargo.tomls selecting multiple stacks.

[MikailBag]

Regarding Tls::pick, it can still be written in a compile-time-checked way (with smth like #[cfg(or(and(rustls, not(openssl))), and(openssl, not(rustls)))]). I've imagined it as doc(hidden) test-utility so I've chosen dynamic checking because it's easier to implement.

In an additive mode, we run the risk of multiple kube uses in various Cargo.tomls selecting multiple stacks.

Well, I don't consider it to be a risk :)

The situation when several TLS stacks are being pulled is not OK, but it still works.

[nightkr]

To be honest, I don't really see the point of the current selection mechanism. Of the two provided backends, Rustls is generally Better (supports more modern features like HTTP/2, is more consistent across platforms, and is arguably more trustworthy because more of it is auditable and Rust). If we want to have an escape hatch for when that might not work (which I'd kind of agree with), then IMO that should be something more open-ended, like Hyper's Connect trait.

One point here: having this be a non-additive feature gave us a guarantee that you only got one of the TLS stacks into the dependency trees if it built. In an additive mode, we run the risk of multiple kube uses in various Cargo.tomls selecting multiple stacks.

IMO, this is really a con of the current approach. Library crates that depend on kube either need to pick a backend (which will make it hard-conflict with crates that depend on the other backend), or it will need to depend with default-features = false (easy to forget), and require the leaf (application) crate to depend on kube and opt into the preferred backend.

[MikailBag]

Anyway, this PR is blocked on the async-tungstenite issue, so I'd prefer to fix this PR later when it's unblocked.

[clux]

To be honest, I don't really see the point of the current selection mechanism. Of the two provided backends, Rustls is generally Better (supports more modern features like HTTP/2, is more consistent across platforms, and is arguably more trustworthy because more of it is auditable and Rust).

Yes, I agree, and hope that we can move towards rustls as a general rust default in the future, but due to bugs like #153, it's not* really viable to drop openssl. Also a bit hesitant to be the first mover on such a change.

@MikailBag : While async_tungstenite has been swapped out for tokio_tungstenite - so this might be theoretically possible - I think it's probably best we put a pin in this one for now unless we can find something more ergonomic when rusttls is sufficiently compatible. Until then, I would prefer the compile time selection for the safety it provides.

Closing for now, but feel free to re-open to discuss further.

[kazk]

should be something more open-ended, like Hyper's Connect trait.

Yeah, we're already using it internally in kube::Service composing TimeoutConnector and HttpsConnector.

let connector = ServiceBuilder::new().layer(timeout).service(https); // Connect
let client: HyperClient<_, Body> = HyperClient::builder().build(connector);

In the future, after figuring out how to let users compose layers nicely, we can optionally let users use custom HttpsConnector, and provide functions like rustls_https and native_https to create HttpsConnector from config values to compose with.