Re: [pbs-devel] [PATCH proxmox v5 0/5] Teach HTTP client how to decode deflate

["Max Carrara" <m.carrara@proxmox.com>]

On Fri Jul 5, 2024 at 3:04 PM CEST, Maximiliano Sandoval wrote:
> This patch series adds support for decoding HTTP requests if they contain the
> Content-Encoding=deflate header. Deciding on a public API (and implementing it)
> to set this header is out-of-scope for this merge request a the moment of
> writing 🙈.
>
> Note that currently the proxmox-rest-server replies with deflated replies in the
> following functions:
>
> - handle_api_request: Called as part of Formated::handle_request and
>   H2Service::handle_request. Always compresses if the client declares that
>   supports deflate.
> - handle_unformatted_api_request: Called as part of Unformatted::handle_request.
>   It decides whether to compress the contents same as before
> - simple_static_file_download: Called as part of handle_static_file_download
>   which is called in ApiConfig::handle_request, this one decides whether to
>   compress based on the fn extension_to_content_type which is a map
>   format:should-be-compressed and whether the clients supports it.
> - chunked_static_file_download is called as part of handle_static_file_downloda
>   same as before
>
> One thing that might be worth pointing out is that according to [1], when the
> client supports `Accept-Encoding=deflate`, the HTTP server can reply with
> `Content-Encoding=deflate` and encode the body using zlib, which means that it
> should be encoded with deflate with the zlib headers included. This is somewhat
> confusing but in short it means that one should use a zlib encoder rather than a
> deflate encoder. At the moment the server always compresses using deflate
> without the zlib headers, not a zlib encoder.

Good to know, thanks! My review is below.

Review: v5
==========

All in all looks pretty good to me! I don't really have anything to
object. There's something I wanted to mention regarding the public API
of the `proxmox-compression` crate; see my response to patch 01.

Nothing that's a blocker though. ;)


Leaving my trailers here so that they don't get lost:

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


Testing
-------

* ran `cargo test --all-features` in the `proxmox-compression` crate
  * all tests passed
* ran `cargo test --all-features` in the `proxmox-http` crate
  * all tests passed
* ran `cargo test --all --all-featuers` in the workspace
  * all tests passed
* recompiled PBS with your changes
  * noticed nothing out of the ordinary

Everything works just fine, nice job!


Code Review
-----------

Also nothing out of the ordinary, the patches are easy to follow and the
code looks fine to me. Again, nice job!


Summary / Conclusion
--------------------

This can be merged as is IMO, all looks fine. My comments regarding the
public API of the `proxmox-compression` crate on patch 01 can (and
probably should) be addressed in another series.

LGTM.

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


>
> [1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Encoding#deflate
>
> Differences from v4:
>  - Rebased
>
> Differences from v3:
>  - Make deflate module private instead of pub(crate)
>
> Differences from v2:
>  - Split into multiple commits
>  - More tests
>  - Add builders for DeflateEndoder and DeflateDecoder
>  - Both the deflate encoder and decoder where moved into a folder
>
> Maximiliano Sandoval (5):
>   compression: deflate: move encoder into a mod
>   compression: deflate: add builder pattern
>   compression: deflate: add a decoder
>   compression: deflate: add test module
>   http: teach the Client how to decode deflate content
>
>  proxmox-compression/Cargo.toml                |   3 +-
>  .../src/{ => deflate}/compression.rs          |  59 +++++--
>  .../src/deflate/decompression.rs              | 141 +++++++++++++++
>  proxmox-compression/src/deflate/mod.rs        | 167 ++++++++++++++++++
>  proxmox-compression/src/lib.rs                |   4 +-
>  proxmox-compression/src/zip.rs                |   2 +-
>  proxmox-http/Cargo.toml                       |   7 +
>  proxmox-http/src/client/simple.rs             |  65 ++++++-
>  8 files changed, 430 insertions(+), 18 deletions(-)
>  rename proxmox-compression/src/{ => deflate}/compression.rs (83%)
>  create mode 100644 proxmox-compression/src/deflate/decompression.rs
>  create mode 100644 proxmox-compression/src/deflate/mod.rs



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