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

Make Client configurable #540

Merged
merged 36 commits into from Jun 5, 2021
Merged

Make Client configurable #540

merged 36 commits into from Jun 5, 2021

Conversation

kazk
Copy link
Member

@kazk kazk commented May 25, 2021

Customize client with Layers:

use kube::{client::ConfigExt, Client, Config};
use tower::ServiceBuilder;
use tower_http::{decompression::DecompressionLayer, trace::TraceLayer};

let config = Config::infer().await?;
let https = config.native_tls_https_connector()?;
let client = Client::new(
    ServiceBuilder::new()
        .layer(config.base_uri_layer())
        .option_layer(config.auth_layer()?)
        .layer(DecompressionLayer::new())
        .layer(TraceLayer::new_for_http())
        .service(hyper::Client::builder().build(https)),
);

Customize HttpsConnector:

let https = ServiceBuilder::new()
    .layer(layer_fn(|c| {
        let mut conn = TimeoutConnector::new(c);
        conn.set_connect_timeout(Some(Duration::from_secs(5)));
        conn
    }))
    .service(config.native_tls_https_connector()?);

It's also possible to create HttpsConnector from TlsConnector:

let https = {
    let tls = tokio_native_tls::TlsConnector::from(
        config.native_tls_connector()?
    );
    let mut http = HttpConnector::new();
    http.enforce_http(false);
    HttpsConnector::from((http, tls))
};

TLS Enhancements

  • Add kube::client::ConfigExt extending Config for custom Client. This includes methods to configure TLS connection when building a custom client (Make Client configurable with Layers #539)
    • native-tls: Config::native_tls_https_connector and Config::native_tls_connector
    • rustls-tls: Config::rustls_https_connector and Config::rustls_client_config
  • Remove the requirement of having native-tls or rustls-tls enabled when client is enabled. Allow one, both or none.
    • When both, the default Service will use native-tls because of rustls cannot reach a cluster through ip #153. rustls can be still used with a custom client. Users will have an option to configure TLS at runtime.
    • When none, HTTP connector is used.
  • Remove TLS features from kube-runtime
    • BREAKING: Features must be removed if specified
  • Remove client feature from native-tls and rust-tls features
    • config + native-tls/rustls-tls can be used independently, e.g., to create a simple HTTP client
    • BREAKING: client feature must be added if default-features = false

Layers

Changes

  • BREAKING: Remove headers from Config. It was originally added to allow injecting arbitrary headers, but now a custom client can be used to do that.
  • Add support for loading proxy URL from kube config. Proxy itself is not supported yet.

Dependency Changes

  • Remove static_assertions since it's no longer used
  • Replace tokio_rustls with rustls and webpki since we're not using tokio_rustls directly
  • Replace uses of rustls::internal::pemfile with rustls-pemfile
  • Remove url and always use http::Uri
    • BREAKING: Config::cluster_url is now http::Uri
    • BREAKING: Error::InternalUrlError(url::ParseError) and Error::MalformedUrl(url::ParseError) replaced by Error::InvalidUri(http::uri::InvalidUri)

Closes #539

@kazk kazk mentioned this pull request May 25, 2021
10 tasks
kube/src/lib.rs Outdated Show resolved Hide resolved
@kazk kazk force-pushed the polish-tls-support branch 2 times, most recently from 0a570c5 to 56735a0 Compare May 27, 2021 03:31
@kazk kazk changed the title Improve TLS support Make Client configurable May 27, 2021
@kazk kazk force-pushed the polish-tls-support branch 3 times, most recently from 96bf9b9 to 357938b Compare May 31, 2021 22:48
Comment on lines 88 to 91
// Transform response body to `hyper::Body` and use type erased error to avoid type parameters.
let service = MapResponseBodyLayer::new(|b| hyper::Body::wrap_stream(body::IntoStream::new(b)))
.layer(service)
.map_err(|e| e.into());
Copy link
Member Author

@kazk kazk Jun 1, 2021

Choose a reason for hiding this comment

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

Took me a few days to come up with this, but should be worth it. This change allows us to take advantage of the Tower ecosystem much more without introducing type parameters to Client.

Shouldn't be a breaking change, but need to test more to make sure.

examples/pod_api.rs Outdated Show resolved Hide resolved
@kazk kazk force-pushed the polish-tls-support branch 2 times, most recently from 9505fcb to 5d52d12 Compare June 2, 2021 04:31
examples/custom_client.rs Outdated Show resolved Hide resolved
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 great. just small philosophical comments. have not dug through the tls stuff much yet.

examples/Cargo.toml Show resolved Hide resolved
examples/custom_client_trace.rs Outdated Show resolved Hide resolved
examples/custom_client_tls.rs Outdated Show resolved Hide resolved
examples/custom_client_trace.rs Show resolved Hide resolved
examples/custom_client_trace.rs Outdated Show resolved Hide resolved
kube-runtime/Cargo.toml Show resolved Hide resolved
kube/src/client/body.rs Show resolved Hide resolved
kube/src/client/mod.rs Outdated Show resolved Hide resolved
kube/src/client/mod.rs Outdated Show resolved Hide resolved
kube/src/client/mod.rs Outdated Show resolved Hide resolved
kazk added 12 commits June 3, 2021 13:11
- Add `Config::native_tls_connector` and `Config::rustls_client_config`
- Remove the requirement of having `native-tls` or `rustls-tls` enabled when
  `client` is enabled. Allow one, both or none.
  - When both, the default Service will use `native-tls` because of kube-rs#153.
    `rustls` can be still used with a custom client.
    Users will have an option to configure TLS at runtime.
  - When none, HTTP connector is used.
- Note that `oauth` feature still requires tls feature.
- Remove tls features from kube-runtime
Still a dependency of hyper-rustls, but we're not using tokio-rustls.
Depend on rustls directly instead.
`config` + `native-tls`/`rustls-tls` can be used independently. For
example, to create a simple HTTP client.
Allow using more from the Tower ecosystem.
Keeping this simple for now by default, but it's fully customizable.
kube/src/client/config_ext.rs Outdated Show resolved Hide resolved
examples/custom_client.rs Outdated Show resolved Hide resolved
@kazk kazk mentioned this pull request Jun 5, 2021
@kazk kazk force-pushed the polish-tls-support branch 2 times, most recently from 709d605 to 614c7c4 Compare June 5, 2021 04:15
@kazk kazk marked this pull request as ready for review June 5, 2021 06:13
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 amazing. Huge work 💯

Had another go over. TLS looks all great, good error handling everywhere, docs everywhere, things with TODOs are all pub(crate).

Left (another) philosiphical naming nit, but beyond this I am all good with this.

Comment on lines +12 to +17
/// Extensions to [`Config`](crate::Config) for custom [`Client`](crate::Client).
///
/// See [`Client::new`](crate::Client::new) for an example.
///
/// This trait is sealed and cannot be implemented.
pub trait ConfigExt: private::Sealed {
Copy link
Member

Choose a reason for hiding this comment

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

I didn't know about this pattern, but am a fan. Feels like a very smart way to avoid having all the public methods in a big namespace 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, the downside is the extra import, but I think the explicit kube::client::ConfigExt actually makes sense for this. I also wanted to keep client stuff out of the config module.

kube/src/client/middleware/base_uri.rs Outdated Show resolved Hide resolved
@clux
Copy link
Member

clux commented Jun 5, 2021

I'll leave it to you to make the final call on naming, and will make a release after you merge this :-)

@kazk kazk merged commit afd032d into kube-rs:master Jun 5, 2021
@kazk kazk deleted the polish-tls-support branch June 5, 2021 21:23
clux added a commit that referenced this pull request Jun 5, 2021
@clux
Copy link
Member

clux commented Jun 5, 2021

Release in 0.56.0! Thank you so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Client configurable with Layers
2 participants