From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 38E8ABBCB9 for ; Wed, 27 Mar 2024 10:00:30 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 190211F4DB for ; Wed, 27 Mar 2024 10:00:00 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 27 Mar 2024 09:59:58 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 5596542484 for ; Wed, 27 Mar 2024 09:59:58 +0100 (CET) Message-ID: Date: Wed, 27 Mar 2024 09:59:55 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Maximiliano Sandoval , pbs-devel@lists.proxmox.com References: <20240326152818.639452-1-m.sandoval@proxmox.com> <20240326152818.639452-2-m.sandoval@proxmox.com> Content-Language: de-AT, en-US From: Lukas Wagner In-Reply-To: <20240326152818.639452-2-m.sandoval@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.007 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pbs-devel] [PATCH proxmox 2/2] http: Teach client how to speak deflate X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 27 Mar 2024 09:00:30 -0000 Hello, thanks for tackling this! Most of my comments also apply to the first commit. Regarding the commit message, I think it would be good to mention the `Accept-Encoding` and `Content-Encoding` headers (e.g that you set `Accept-Encoding` on the request on decode the response body based on `Content-Encoding`). These are both quite well-known and it makes it clearer what these commits are about. Thanks for including some tests, that's always good. Of course it's hard to unit-test this in a more 'realistic' scenario. :) On 2024-03-26 16:28, Maximiliano Sandoval wrote: > The Backup Server can speak deflate so we implement that. > > Note that the spec [1] allows the server to encode the content multiple > times with different algorithms. > > [1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding > > Suggested-by: Lukas Wagner > Signed-off-by: Maximiliano Sandoval > --- > proxmox-http/src/client/simple.rs | 98 ++++++++++++++++++++++++------- > 1 file changed, 78 insertions(+), 20 deletions(-) > > diff --git a/proxmox-http/src/client/simple.rs b/proxmox-http/src/client/simple.rs > index b33154be..c3afa8d0 100644 > --- a/proxmox-http/src/client/simple.rs > +++ b/proxmox-http/src/client/simple.rs > @@ -4,7 +4,7 @@ use std::io::Read; > #[cfg(all(feature = "client-trait", feature = "proxmox-async"))] > use std::str::FromStr; > > -use flate2::read::GzDecoder; > +use flate2::read::{DeflateDecoder, GzDecoder}; > > use futures::*; > #[cfg(all(feature = "client-trait", feature = "proxmox-async"))] > @@ -76,7 +76,7 @@ impl Client { > > request.headers_mut().insert( > hyper::header::ACCEPT_ENCODING, > - HeaderValue::from_static("gzip"), > + HeaderValue::from_static("gzip, deflate"), > ); > request > .headers_mut() > @@ -149,22 +149,24 @@ impl Client { > match response { > Ok(res) => { > let (mut parts, body) = res.into_parts(); > - let is_gzip_encoded = parts > - .headers > - .remove(&hyper::header::CONTENT_ENCODING) > - .is_some_and(|h| h == "gzip"); > - > - let buf = hyper::body::to_bytes(body).await?; > - let new_body = if is_gzip_encoded { > - let mut gz = GzDecoder::new(&buf[..]); > - let mut s = String::new(); > - gz.read_to_string(&mut s)?; > - s > - } else { > - String::from_utf8(buf.to_vec()) > - .map_err(|err| format_err!("Error converting HTTP result data: {}", err))? > + let mut buf = hyper::body::to_bytes(body).await?.to_vec(); > + let content_encoding = parts.headers.remove(&hyper::header::CONTENT_ENCODING); > + > + if let Some(content_encoding) = content_encoding { > + let encodings = content_encoding.to_str()?; > + for encoding in encodings.rsplit([',', ' ']) { > + buf = match encoding { > + "" => buf, // "a, b" splits into ["a", "", "b"]. > + "gzip" => decode_gzip(&buf[..])?, > + "deflate" => decode_deflate(&buf[..])?, > + other => anyhow::bail!("Unknown format: {other}"), `anyhow::bail!` is already in scope, so you can just use `bail!` > + } > + } I would suggest moving the decompression to the `request` method (maybe as a separate helper function though), transforming the `Response`, with the body decompressed. Right now, the decompression only happens in `convert_body_to_string`, which means that this breaks users - which use the public `Client::request` directly (e.g. the `proxmox-client` crate) - which use the `HttpClient` trait impl of `Client` If the decompression happens directly in `request`, the users for the crate should not notice any difference, at least from my understanding :) > }; > > + let new_body = String::from_utf8(buf) > + .map_err(|err| format_err!("Error converting HTTP result data: {}", err))?; > + > Ok(Response::from_parts(parts, new_body)) > } > Err(err) => Err(err), > @@ -267,6 +269,10 @@ impl crate::HttpClient for Client { > mod test { > use super::*; > > + use flate2::write::{DeflateEncoder, GzEncoder}; > + use flate2::Compression; > + use std::io::Write; > + > const BODY: &str = "hello world"; > > #[tokio::test] > @@ -288,14 +294,66 @@ mod test { > assert_eq!(Client::response_body_string(response).await.unwrap(), BODY); > } > > - fn encode_gzip(bytes: &[u8]) -> Result, std::io::Error> { > - use flate2::write::GzEncoder; > - use flate2::Compression; > - use std::io::Write; > + #[tokio::test] > + async fn test_parse_response_deflate() { > + let encoded = encode_deflate(BODY.as_bytes()).unwrap(); > + let body = Body::from(encoded); > + > + let response = Response::builder() > + .header(hyper::header::CONTENT_ENCODING, "deflate") > + .body(body) > + .unwrap(); > + assert_eq!(Client::response_body_string(response).await.unwrap(), BODY); > + } > + > + #[tokio::test] > + async fn test_parse_response_deflate_gzip() { > + let deflate_encoded = encode_deflate(BODY.as_bytes()).unwrap(); > + let gzip_encoded = encode_gzip(&deflate_encoded).unwrap(); > + let body = Body::from(gzip_encoded); > + > + let response = Response::builder() > + .header(hyper::header::CONTENT_ENCODING, "deflate, gzip") > + .body(body) > + .unwrap(); > + assert_eq!(Client::response_body_string(response).await.unwrap(), BODY); > > + let gzip_encoded = encode_gzip(BODY.as_bytes()).unwrap(); > + let deflate_encoded = encode_deflate(&gzip_encoded).unwrap(); > + let body = Body::from(deflate_encoded); > + > + let response = Response::builder() > + .header(hyper::header::CONTENT_ENCODING, "gzip, deflate") > + .body(body) > + .unwrap(); > + assert_eq!(Client::response_body_string(response).await.unwrap(), BODY); > + } > + > + fn encode_deflate(bytes: &[u8]) -> Result, std::io::Error> { > + let mut e = DeflateEncoder::new(Vec::new(), Compression::default()); > + e.write_all(bytes).unwrap(); > + > + e.finish() > + } > + > + fn encode_gzip(bytes: &[u8]) -> Result, std::io::Error> { > let mut e = GzEncoder::new(Vec::new(), Compression::default()); > e.write_all(bytes).unwrap(); > > e.finish() > } > } > + > +fn decode_gzip(buf: &[u8]) -> Result, std::io::Error> { > + let mut dec = GzDecoder::new(buf); > + let mut v = Vec::new(); > + dec.read_to_end(&mut v)?; > + Ok(v) > +} > + > +fn decode_deflate(buf: &[u8]) -> Result, std::io::Error> { > + let mut dec = DeflateDecoder::new(buf); > + let mut v = Vec::new(); > + dec.read_to_end(&mut v)?; > + Ok(v) > +} ^ I'd put both of them into the `impl Client` block (as associated static helper functions, not methods (so no self parameter) - but no hard feelings -- - Lukas