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

Replace reqwest with hyper + tower #394

Merged
merged 31 commits into from
Feb 7, 2021
Merged

Conversation

kazk
Copy link
Member

@kazk kazk commented Feb 7, 2021

Replace reqwest with hyper (#362) and tower (#100). Supersedes #376.

Changes

kube::Client now uses kube::Service instead of reqwest::Client

 #[derive(Clone)]
 pub struct Client {
-    cluster_url: reqwest::Url,
-    default_ns: String,
-    inner: reqwest::Client,
-    config: Config,
-    #[cfg(feature = "ws")]
-    tls_connector: AsyncTlsConnector,
+    inner: kube::Service,
 }

kube::Service struct is a tower::Service<http::Request<hyper::Body>>. For convenience, a service based on hyper::Client can be created from kube::Config, so Client::try_default() is still supported. We can add a feature for this to provide an option for a sans-io Client (#204).

To allow users to use their implementation when they need something not supported out of the box, I'm planning to change Client::new(config: Config) to Client::new(service: Service) (creating a Client from Config is still possible with Client::new(config.try_into()?)). This will also allow better testing.

kube::Service uses tower::buffer::Buffer, so it's cheap to clone (#17).

Upgrade to WebSocket connection with hyper

The above change was possible because of this. No more separate client for WebSocket! The connection is upgraded with hyper and a WebSocketStream is created from the upgraded connection. All requests go through kube::Service.

Other Improvements

  • Rewrote Client::request_events with tokio_util::codec::FramedRead (Factor out chunked reader from Client::request_events #379)
  • Refactored TLS related code. It's now all isolated in kube/src/service/tls.rs. kube/src/config/auth.rs has HttpsConnector for oauth requests and kube/src/config/mod.rs has some hack for macOS, but that's about it.
  • Replaced oauth2 (GCP OAuth) module with tame-oauth and made it optional (oauth feature).
  • Refactored auth related code and moved them to config::auth.
  • Fixed a bug where refreshable token from AuthProvider was not refreshing.
  • Fixed panicking when loading AuthInfo with provider other than GCP (Panic when creating Config object with Azure provider #238).

Breaking Changes

  • oauth is now opt-in
  • gzip should probably be opt-in for now because Kubernetes doesn't have it enabled by default yet and requires more dependencies. It's behind APIResponseCompression feature gate which is in Beta since 1.16. See https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/.
  • TODO Change kube::Client::new(config: Config) to kube::Client::new(service: Service)
  • Dropped proxy option. It should be possible by using a custom Service after the above change. TODO add example.

Closes #100, #238, #362, #379

It should be possible with custom Service.
reqwest's timeout is applied from when the request starts connecting
until the response body has finished.

Use hyper-timeout to add timeout for reading the response.
@clux clux linked an issue Feb 7, 2021 that may be closed by this pull request
@clux clux removed a link to an issue Feb 7, 2021
use hyper::Body;

/// Set cluster URL.
pub fn set_cluster_url(req: Request<Body>, url: &url::Url) -> Request<Body> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be used for a (hacky) solution for #209 by adding a placeholder in Api then replacing it with default_ns before making a request.

pub fn default_namespaced(client: Client) -> Self {
    let resource = Resource::namespaced::<K>("{{default_ns}}");
    Self {
        resource,
        client,
        phantom: iter::empty(),
    }
}
let inner = ServiceBuilder::new()
    .map_request(move |r| finalize_url(r, &url, &default_ns))
    .service(client);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, that is actually a pretty good solution. The braces would mean we cannot clash with an actual namespace.

Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good, once again (original pr already had a significant amount of iteration). Everything is very cleanly done, modules delineated a lot better than before.
Less dependency multiplicity now that everything goes through the same service.
Have tested it a bit on your branch, and don't see any issues either, it just works. ✨

Happy to merge so we can rebase the remaining PRs (rather than the other way around).

kube/src/service/auth.rs Show resolved Hide resolved
kube/src/service/auth.rs Show resolved Hide resolved
@kazk
Copy link
Member Author

kazk commented Feb 7, 2021

Thanks for reviewing and testing. Glad to hear it just worked ✨
I haven't found any issues running the examples either. Let's merge it 👍
I'll open separate PRs for any follow-ups. This is already too big.

@clux
Copy link
Member

clux commented Feb 7, 2021

Awesome. It's going in! Thank you so much for digging into all of this (and effectively rewriting both the client and the config). This is a huge improvement.

I'll open separate PRs for any follow-ups. This is already too big.
Agreed, it snowballed :D
But all good.

@clux clux merged commit 5dc29b4 into kube-rs:master Feb 7, 2021
@kazk kazk deleted the use-hyper-tower branch February 7, 2021 11:24
@kazk kazk mentioned this pull request Feb 7, 2021
@clux
Copy link
Member

clux commented Feb 7, 2021

Added some basic info about this in the CHANGELOG. Feel free to update as you see fit.

Want me to wait with making a release until any of the follow-up tasks?

  • gzip opt-in (agreed if it's not default in kubernetes)
  • kube::Client From Service

The last one would good for the sans-io story, even if it's only for us being able to provide a better testing. (Not sure there are any people masochistic enough to want to change out the full client stack, but maybe I am wrong).

@kazk
Copy link
Member Author

kazk commented Feb 8, 2021

Opened #399 (gzip), #400 (Client::new), #401 (fix docs), and #402 (changelog).

@clux
Copy link
Member

clux commented Feb 8, 2021

wow that was a lot of prs in one night, thanks a lot.
i have merged them all. i'll push out a minor release now.

@clux clux linked an issue Feb 8, 2021 that may be closed by this pull request
@clux
Copy link
Member

clux commented Feb 8, 2021

All of it released in 0.49.0 ✨

@clux clux mentioned this pull request Feb 8, 2021
clux added a commit that referenced this pull request Feb 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants