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

Feature: Add config setting to disable gzip compression #1627 #1628

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions kube-client/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ form_urlencoded = { workspace = true, optional = true }
k8s-openapi= { workspace = true, features = [] }

[dev-dependencies]
hyper = { workspace = true, features = ["server"] }
kube = { path = "../kube", features = ["derive", "client", "ws"], version = "<1.0.0, >=0.61.0" }
tempfile.workspace = true
futures = { workspace = true, features = ["async-await"] }
Expand Down
75 changes: 74 additions & 1 deletion kube-client/src/client/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,13 @@
#[cfg(feature = "gzip")]
let stack = ServiceBuilder::new()
.layer(stack)
.layer(tower_http::decompression::DecompressionLayer::new())
.layer(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

trying to wrap the whole layer in an "option_layer" block ran head first into a load of compile issues - fortunately the Compression Layer supports enabling/disabling through configuration hence that path was chosen

tower_http::decompression::DecompressionLayer::new()
.no_br()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

intentionally disable anything but GZIP compression -

reading kubernetes/kubernetes#112296 show there isn't like to be any other option in Kube for a while, but these flags could easily be refitted when/if Kube gains alternatives.

Copy link
Member

Choose a reason for hiding this comment

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

agreed. think this is sensible also.

.no_deflate()
.no_zstd()

Check warning on line 165 in kube-client/src/client/builder.rs

View check run for this annotation

Codecov / codecov/patch

kube-client/src/client/builder.rs#L163-L165

Added lines #L163 - L165 were not covered by tests
.gzip(!config.disable_compression),
)
.into_inner();

let service = ServiceBuilder::new()
Expand Down Expand Up @@ -226,3 +232,70 @@
default_ns,
))
}

#[cfg(test)]
mod tests {
#[cfg(feature = "gzip")] use super::*;

#[cfg(feature = "gzip")]
#[tokio::test]
async fn test_no_accept_encoding_header_sent_when_compression_disabled(
) -> Result<(), Box<dyn std::error::Error>> {
use http::Uri;
use std::net::SocketAddr;
use tokio::net::{TcpListener, TcpStream};

// setup a server that echoes back any encoding header value
let addr: SocketAddr = ([127, 0, 0, 1], 0).into();
let listener = TcpListener::bind(addr).await?;
let local_addr = listener.local_addr()?;
let uri: Uri = format!("http://{}", local_addr).parse()?;

tokio::spawn(async move {
use http_body_util::Full;
use hyper::{server::conn::http1, service::service_fn};
use hyper_util::rt::{TokioIo, TokioTimer};
use std::convert::Infallible;

loop {
let (tcp, _) = listener.accept().await.unwrap();
let io: TokioIo<TcpStream> = TokioIo::new(tcp);

tokio::spawn(async move {
let _ = http1::Builder::new()
.timer(TokioTimer::new())
.serve_connection(
io,
service_fn(|req| async move {
let response = req
.headers()
.get(http::header::ACCEPT_ENCODING)
.map(|b| Bytes::copy_from_slice(b.as_bytes()))
.unwrap_or_default();
Ok::<_, Infallible>(Response::new(Full::new(response)))
}),
)
.await
.unwrap();

Check warning on line 279 in kube-client/src/client/builder.rs

View check run for this annotation

Codecov / codecov/patch

kube-client/src/client/builder.rs#L279

Added line #L279 was not covered by tests
});
}
});

// confirm gzip echoed back with default config
let config = Config { ..Config::new(uri) };
let client = make_generic_builder(HttpConnector::new(), config.clone())?.build();
let response = client.request_text(http::Request::default()).await?;
assert_eq!(&response, "gzip");

// now disable and check empty string echoed back
let config = Config {
disable_compression: true,
..config
};
let client = make_generic_builder(HttpConnector::new(), config)?.build();
let response = client.request_text(http::Request::default()).await?;
assert_eq!(&response, "");
Comment on lines +284 to +297
Copy link
Member

Choose a reason for hiding this comment

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

wow great test. 👍


Ok(())
}
}
8 changes: 8 additions & 0 deletions kube-client/src/config/file_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,14 @@ pub struct Cluster {
#[serde(rename = "proxy-url")]
#[serde(skip_serializing_if = "Option::is_none")]
pub proxy_url: Option<String>,
/// Compression is enabled by default with the `gzip` feature.
/// `disable_compression` allows client to opt-out of response compression for all requests to the server.
/// This is useful to speed up requests (specifically lists) when client-server network bandwidth is ample,
/// by saving time on compression (server-side) and decompression (client-side):
/// https://github.com/kubernetes/kubernetes/issues/112296
#[serde(rename = "disable-compression")]
#[serde(skip_serializing_if = "Option::is_none")]
pub disable_compression: Option<bool>,
/// Name used to check server certificate.
///
/// If `tls_server_name` is `None`, the hostname used to contact the server is used.
Expand Down
7 changes: 7 additions & 0 deletions kube-client/src/config/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,8 @@ pub struct Config {
pub accept_invalid_certs: bool,
/// Stores information to tell the cluster who you are.
pub auth_info: AuthInfo,
/// Whether to disable compression (would only have an effect when the `gzip` feature is enabled)
pub disable_compression: bool,
/// Optional proxy URL. Proxy support requires the `socks5` feature.
pub proxy_url: Option<http::Uri>,
/// If set, apiserver certificate will be validated to contain this string
Expand All @@ -177,6 +179,7 @@ impl Config {
write_timeout: Some(DEFAULT_WRITE_TIMEOUT),
accept_invalid_certs: false,
auth_info: AuthInfo::default(),
disable_compression: false,
proxy_url: None,
tls_server_name: None,
headers: Vec::new(),
Expand Down Expand Up @@ -259,6 +262,7 @@ impl Config {
token_file: Some(incluster_config::token_file()),
..Default::default()
},
disable_compression: false,
proxy_url: None,
tls_server_name: None,
headers: Vec::new(),
Expand Down Expand Up @@ -302,6 +306,8 @@ impl Config {
.unwrap_or_else(|| String::from("default"));

let accept_invalid_certs = loader.cluster.insecure_skip_tls_verify.unwrap_or(false);
let disable_compression = loader.cluster.disable_compression.unwrap_or(false);

let mut root_cert = None;

if let Some(ca_bundle) = loader.ca_bundle()? {
Expand All @@ -316,6 +322,7 @@ impl Config {
read_timeout: Some(DEFAULT_READ_TIMEOUT),
write_timeout: Some(DEFAULT_WRITE_TIMEOUT),
accept_invalid_certs,
disable_compression,
proxy_url: loader.proxy_url()?,
auth_info: loader.user,
tls_server_name: loader.cluster.tls_server_name,
Expand Down
Loading