[pbs-devel] [PATCH proxmox] rest-server: Encode with zlib headers

[pbs-devel] [PATCH proxmox] rest-server: Encode with zlib headers

[Maximiliano Sandoval]

As per [RFC9110] the Deflate encoding is a "zlib" data format. This
makes the rest-server compatible with the http-client.

[RFC9110] https://www.rfc-editor.org/rfc/rfc9110#field.content-encoding

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---

By default the builder will use the default compression level. Please let me
know if the Level::Default should be set explicitly.

 proxmox-rest-server/src/rest.rs | 35 ++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/proxmox-rest-server/src/rest.rs b/proxmox-rest-server/src/rest.rs
index 3a3c287c..4be36074 100644
--- a/proxmox-rest-server/src/rest.rs
+++ b/proxmox-rest-server/src/rest.rs
@@ -30,7 +30,7 @@ use proxmox_router::{http_bail, http_err};
 use proxmox_schema::{ObjectSchemaType, ParameterSchema};
 
 use proxmox_async::stream::AsyncReaderStream;
-use proxmox_compression::{DeflateEncoder, Level};
+use proxmox_compression::DeflateEncoder;
 use proxmox_log::FileLogger;
 
 use crate::{
@@ -557,12 +557,13 @@ pub(crate) async fn handle_api_request<Env: RpcEnvironment, S: 'static + BuildHa
                 CompressionMethod::Deflate.content_encoding(),
             );
             resp.map(|body| {
-                Body::wrap_stream(DeflateEncoder::with_quality(
-                    TryStreamExt::map_err(body, |err| {
+                Body::wrap_stream(
+                    DeflateEncoder::builder(TryStreamExt::map_err(body, |err| {
                         proxmox_lang::io_format_err!("error during compression: {}", err)
-                    }),
-                    Level::Default,
-                ))
+                    }))
+                    .zlib(true)
+                    .build(),
+                )
             })
         }
         None => resp,
@@ -651,12 +652,13 @@ async fn handle_unformatted_api_request<Env: RpcEnvironment, S: 'static + BuildH
                 CompressionMethod::Deflate.content_encoding(),
             );
             resp.map(|body| {
-                Body::wrap_stream(DeflateEncoder::with_quality(
-                    TryStreamExt::map_err(body, |err| {
+                Body::wrap_stream(
+                    DeflateEncoder::builder(TryStreamExt::map_err(body, |err| {
                         proxmox_lang::io_format_err!("error during compression: {}", err)
-                    }),
-                    Level::Default,
-                ))
+                    }))
+                    .zlib(true)
+                    .build(),
+                )
             })
         }
         None => resp,
@@ -711,7 +713,7 @@ async fn simple_static_file_download(
 
     let mut response = match compression {
         Some(CompressionMethod::Deflate) => {
-            let mut enc = DeflateEncoder::with_quality(data, Level::Default);
+            let mut enc = DeflateEncoder::builder(data).zlib(true).build();
             enc.compress_vec(&mut file, CHUNK_SIZE_LIMIT as usize)
                 .await?;
             let mut response = Response::new(enc.into_inner().into());
@@ -752,10 +754,11 @@ async fn chunked_static_file_download(
                 header::CONTENT_ENCODING,
                 CompressionMethod::Deflate.content_encoding(),
             );
-            Body::wrap_stream(DeflateEncoder::with_quality(
-                AsyncReaderStream::new(file),
-                Level::Default,
-            ))
+            Body::wrap_stream(
+                DeflateEncoder::builder(AsyncReaderStream::new(file))
+                    .zlib(true)
+                    .build(),
+            )
         }
         None => Body::wrap_stream(AsyncReaderStream::new(file)),
     };
-- 
2.39.2



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox] rest-server: Encode with zlib headers

[Max Carrara]

On Tue Jul 16, 2024 at 11:22 AM CEST, Maximiliano Sandoval wrote:
> As per [RFC9110] the Deflate encoding is a "zlib" data format. This
> makes the rest-server compatible with the http-client.
>
> [RFC9110] https://www.rfc-editor.org/rfc/rfc9110#field.content-encoding
>
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>
> By default the builder will use the default compression level. Please let me
> know if the Level::Default should be set explicitly.

This patch looks simple enough to me and does what it says on the tin;
I can't really spot anything wrong here.

Regarding your comment: I think the default level is perfectly fine.
Of course, one could be pedantic and try to benchmark any differences
between the levels, but IMHO that's rather redundant - if there are any
potential gains here, they're most likely miniscule.

In other words: LGTM.

Reviewed-by: Max Carrara <m.carrara@proxmox.com>

>
>  proxmox-rest-server/src/rest.rs | 35 ++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 16 deletions(-)
>
> diff --git a/proxmox-rest-server/src/rest.rs b/proxmox-rest-server/src/rest.rs
> index 3a3c287c..4be36074 100644
> --- a/proxmox-rest-server/src/rest.rs
> +++ b/proxmox-rest-server/src/rest.rs
> @@ -30,7 +30,7 @@ use proxmox_router::{http_bail, http_err};
>  use proxmox_schema::{ObjectSchemaType, ParameterSchema};
>  
>  use proxmox_async::stream::AsyncReaderStream;
> -use proxmox_compression::{DeflateEncoder, Level};
> +use proxmox_compression::DeflateEncoder;
>  use proxmox_log::FileLogger;
>  
>  use crate::{
> @@ -557,12 +557,13 @@ pub(crate) async fn handle_api_request<Env: RpcEnvironment, S: 'static + BuildHa
>                  CompressionMethod::Deflate.content_encoding(),
>              );
>              resp.map(|body| {
> -                Body::wrap_stream(DeflateEncoder::with_quality(
> -                    TryStreamExt::map_err(body, |err| {
> +                Body::wrap_stream(
> +                    DeflateEncoder::builder(TryStreamExt::map_err(body, |err| {
>                          proxmox_lang::io_format_err!("error during compression: {}", err)
> -                    }),
> -                    Level::Default,
> -                ))
> +                    }))
> +                    .zlib(true)
> +                    .build(),
> +                )
>              })
>          }
>          None => resp,
> @@ -651,12 +652,13 @@ async fn handle_unformatted_api_request<Env: RpcEnvironment, S: 'static + BuildH
>                  CompressionMethod::Deflate.content_encoding(),
>              );
>              resp.map(|body| {
> -                Body::wrap_stream(DeflateEncoder::with_quality(
> -                    TryStreamExt::map_err(body, |err| {
> +                Body::wrap_stream(
> +                    DeflateEncoder::builder(TryStreamExt::map_err(body, |err| {
>                          proxmox_lang::io_format_err!("error during compression: {}", err)
> -                    }),
> -                    Level::Default,
> -                ))
> +                    }))
> +                    .zlib(true)
> +                    .build(),
> +                )
>              })
>          }
>          None => resp,
> @@ -711,7 +713,7 @@ async fn simple_static_file_download(
>  
>      let mut response = match compression {
>          Some(CompressionMethod::Deflate) => {
> -            let mut enc = DeflateEncoder::with_quality(data, Level::Default);
> +            let mut enc = DeflateEncoder::builder(data).zlib(true).build();
>              enc.compress_vec(&mut file, CHUNK_SIZE_LIMIT as usize)
>                  .await?;
>              let mut response = Response::new(enc.into_inner().into());
> @@ -752,10 +754,11 @@ async fn chunked_static_file_download(
>                  header::CONTENT_ENCODING,
>                  CompressionMethod::Deflate.content_encoding(),
>              );
> -            Body::wrap_stream(DeflateEncoder::with_quality(
> -                AsyncReaderStream::new(file),
> -                Level::Default,
> -            ))
> +            Body::wrap_stream(
> +                DeflateEncoder::builder(AsyncReaderStream::new(file))
> +                    .zlib(true)
> +                    .build(),
> +            )
>          }
>          None => Body::wrap_stream(AsyncReaderStream::new(file)),
>      };



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox] rest-server: Encode with zlib headers

[Thomas Lamprecht]

Am 17/07/2024 um 17:49 schrieb Max Carrara:
> On Tue Jul 16, 2024 at 11:22 AM CEST, Maximiliano Sandoval wrote:
>> As per [RFC9110] the Deflate encoding is a "zlib" data format. This
>> makes the rest-server compatible with the http-client.
>>
>> [RFC9110] https://www.rfc-editor.org/rfc/rfc9110#field.content-encoding
>>
>> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
>> ---
>>
>> By default the builder will use the default compression level. Please let me
>> know if the Level::Default should be set explicitly.
> 
> This patch looks simple enough to me and does what it says on the tin;
> I can't really spot anything wrong here.

Does the "does what it says on the tin" part also mean you tested this
explicitly? Just wondering why no T-b was given and how closely I need
to check this out ;-)


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox] rest-server: Encode with zlib headers

[Max Carrara]

On Thu Jul 18, 2024 at 6:08 PM CEST, Thomas Lamprecht wrote:
> Am 17/07/2024 um 17:49 schrieb Max Carrara:
> > On Tue Jul 16, 2024 at 11:22 AM CEST, Maximiliano Sandoval wrote:
> >> As per [RFC9110] the Deflate encoding is a "zlib" data format. This
> >> makes the rest-server compatible with the http-client.
> >>
> >> [RFC9110] https://www.rfc-editor.org/rfc/rfc9110#field.content-encoding
> >>
> >> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> >> ---
> >>
> >> By default the builder will use the default compression level. Please let me
> >> know if the Level::Default should be set explicitly.
> > 
> > This patch looks simple enough to me and does what it says on the tin;
> > I can't really spot anything wrong here.
>
> Does the "does what it says on the tin" part also mean you tested this
> explicitly? Just wondering why no T-b was given and how closely I need
> to check this out ;-)

Maybe I should use less colorful sayings next time, but no, I hadn't
tested this, hence the missing T-b tag ;)

Though, for good measure I tested it now - all responses use the
"Content-Encoding: deflate" header and compress the correctly in
Firefox. Furthermore, when making a request without the header, the
content is not encoded at all (tested with curl).

So, my trailers are now as follows:

Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Max Carrara <m.carrara@proxmox.com>



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] applied: [PATCH proxmox] rest-server: Encode with zlib headers

[Thomas Lamprecht]

Am 16/07/2024 um 11:22 schrieb Maximiliano Sandoval:
> As per [RFC9110] the Deflate encoding is a "zlib" data format. This
> makes the rest-server compatible with the http-client.
> 
> [RFC9110] https://www.rfc-editor.org/rfc/rfc9110#field.content-encoding
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
> 
> By default the builder will use the default compression level. Please let me
> know if the Level::Default should be set explicitly.
> 
>  proxmox-rest-server/src/rest.rs | 35 ++++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 16 deletions(-)
> 
>

applied, with Max's R-b and T-b, thanks!


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox] rest-server: Encode with zlib headers

[Thomas Lamprecht]

Am 22/07/2024 um 07:56 schrieb Max Carrara:
>> Does the "does what it says on the tin" part also mean you tested this
>> explicitly? Just wondering why no T-b was given and how closely I need
>> to check this out 😉
>
> Maybe I should use less colorful sayings next time, but no, I hadn't
> tested this, hence the missing T-b tag 😉

:-)
 
> Though, for good measure I tested it now - all responses use the
> "Content-Encoding: deflate" header and compress the correctly in
> Firefox. Furthermore, when making a request without the header, the
> content is not encoded at all (tested with curl).
> 
> So, my trailers are now as follows:
> 
> Reviewed-by: Max Carrara <m.carrara@proxmox.com>
> Tested-by: Max Carrara <m.carrara@proxmox.com>

Many thanks! While I appreciate the R-b, for such changes that change
semantics one some public interface/protocol/service I'd slightly prefer
the T-b over it, at least if one would have only time to do either
review or testing; doing both test and review is naturally always nicer.



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox] rest-server: Encode with zlib headers

[Maximiliano Sandoval]

Thomas Lamprecht <t.lamprecht@proxmox.com> writes:

> Am 22/07/2024 um 07:56 schrieb Max Carrara:
>>> Does the "does what it says on the tin" part also mean you tested this
>>> explicitly? Just wondering why no T-b was given and how closely I need
>>> to check this out 😉
>>
>> Maybe I should use less colorful sayings next time, but no, I hadn't
>> tested this, hence the missing T-b tag 😉
>
> :-)
>  
>> Though, for good measure I tested it now - all responses use the
>> "Content-Encoding: deflate" header and compress the correctly in
>> Firefox. Furthermore, when making a request without the header, the
>> content is not encoded at all (tested with curl).
>> 
>> So, my trailers are now as follows:
>> 
>> Reviewed-by: Max Carrara <m.carrara@proxmox.com>
>> Tested-by: Max Carrara <m.carrara@proxmox.com>
>
> Many thanks! While I appreciate the R-b, for such changes that change
> semantics one some public interface/protocol/service I'd slightly prefer
> the T-b over it, at least if one would have only time to do either
> review or testing; doing both test and review is naturally always nicer.

One thing that I forgot to mention is that as per the note in [RFC 9110,
8.4.1.2] it is relatively common to have non-conformant implementations.
Hence, some clients (notably browsers) might support decoding
non-conformant payloads without the zlib header set, explaining why this
was working up to this point.

[RFC 9110] https://www.rfc-editor.org/rfc/rfc9110#field.content-encoding

-- 
Maximiliano


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel