search

kube-rs / kube

Rust Kubernetes client and controller runtime
https://kube.rs
Apache License 2.0
3.03k stars 314 forks source link

Separate Connect/Read timeout setting #969

Closed goenning closed 2 years ago

goenning commented 2 years ago

Would you like to work on this feature?

maybe

What problem are you trying to solve?

I want to fail fast on unreachable api servers by using a low connect timeout, while keeping the actual response/read timeout fairly high.

Describe the solution you'd like

Connect timeout tends to be much lower than read/response timeouts, so there should be an easy way to configure different values for those, it's unlikely that both timeouts should be the same.

Describe alternatives you've considered

There is a property to set the timeout, but it's not granular enough

https://github.com/kube-rs/kube-rs/blob/79f1d7a54ade86932f10ba82429b48abd4ff82cc/kube-client/src/client/builder.rs#L103-L105

I can copy the Client::try_from(cfg) method and customize it, but it's a fairly big chunk of code to copy just to change a timeout setting. If I copy this piece, I'd need to keep it in sync with upstream kube-rs to get improvements/fixes/etc

Documentation, Adoption, Migration Strategy

deprecate timeout field and expose two new fields connect_timeout and read_timeout, give preference to new fields, while doing a fallback to timeout if set. Remove timeout on 1.0.0

Target crate for feature

kube-client

clux commented 2 years ago

I think this makes sense, strategy is reasonable to me. There's also a third timeout for write_timeout in TimeoutConnector, but we are not setting it atm which may or may not be great.

Given we are using the same value for both inside kube, then it would make sense to split this into options and also fallback to sensible defaults for these when unset. In particular see if there's values to be stolen from client-go (for any of these 3), we could maybe use those as defaults rather than no-timeouts.

Would be happy to take a PR for splitting it out!