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

investigate swapping out reqwest for hyper #362

Closed
clux opened this issue Dec 31, 2020 · 3 comments · Fixed by #394
Closed

investigate swapping out reqwest for hyper #362

clux opened this issue Dec 31, 2020 · 3 comments · Fixed by #394
Assignees
Labels
client kube Client related help wanted Not immediately prioritised, please help! question Direction unclear; possibly a bug, possibly could be improved.

Comments

@clux
Copy link
Member

clux commented Dec 31, 2020

We are doing a whole lot of manual TLS work ourselves, along with painful re-exporting of feature sets which selects a lot of the same dependencies (which are further re-ordered by #360 where we get a second client for websockets).

It would be interesting to see how feasible it is to just run straight hyper within kube and see what the fallout of that would be.
What does this drop? What does this let us solve better?

I imagine this will force us to make a better encapsulation interface to the Client (to avoid impulses to go down #17), as we are now putting a bunch of parameters within it, making Clone more expensive, even more so after #360.

Most of the dependencies of reqwest we already have anyway; tokio, serde, various tls deps, pin, url, futures*, but there is more. What extra dependencies would this mean we would have to lift into kube?

  • async gzip compression, almost certainly - probably a good default to have
  • cookies, can probably drop?
  • dns resolver?
  • error types: ReqwestError is re-exported in kube::Error due to its nice interface. But looks like Hyper::Error could be usable unless we want to unpack the errors out out of reqwest/hyper entirely.

If anyone wants to investigate and/or provide some PoC PRs, this would be appreciated.

@clux clux added help wanted Not immediately prioritised, please help! question Direction unclear; possibly a bug, possibly could be improved. client kube Client related labels Dec 31, 2020
@kazk
Copy link
Member

kazk commented Jan 1, 2021

@jbg
Copy link
Contributor

jbg commented Jan 2, 2021

One small request regarding TLS: please expose the vendored feature on hyper-tls, as it's the easiest way to handle the openssl dependency when cross-compiling. At the moment we have to add a direct dep on reqwest with the native-tls-vendored feature in order to get this feature activated, as it's not passed through by kube.

kazk added a commit to kazk/kube-rs that referenced this issue Jan 6, 2021
@kazk kazk self-assigned this Jan 6, 2021
@kazk kazk linked a pull request Jan 6, 2021 that will close this issue
kazk added a commit to kazk/kube-rs that referenced this issue Jan 7, 2021
kazk added a commit to kazk/kube-rs that referenced this issue Jan 9, 2021
kazk added a commit to kazk/kube-rs that referenced this issue Jan 17, 2021
kazk added a commit to kazk/kube-rs that referenced this issue Jan 29, 2021
@kazk kazk linked a pull request Feb 7, 2021 that will close this issue
@clux clux closed this as completed in #394 Feb 7, 2021
@kazk
Copy link
Member

kazk commented Feb 7, 2021

@jbg Can you open a new issue for vendored? We should be able to add a feature for it later. Comments are easy to forget.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client kube Client related help wanted Not immediately prioritised, please help! question Direction unclear; possibly a bug, possibly could be improved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants