Re: [pbs-devel] [RFC proxmox 00/23] upgrade to hyper/http 1.0

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

On Wed Mar 26, 2025 at 4:23 PM CET, Fabian Grünbichler wrote:
> this RFC series adapts proxmox and proxmox-backup to hyper/http 1.0. I
> also have similar patches for PDM, but those require an update of gloo
> and proxmox-yew-comp and the basic approach is the same as with the
> patches here, and since I expect some feedback to incorporate anyway I
> saved those for the first "proper" version.
>
> hyper 1.0 came with a lot of changes, the most notable ones:
>
> Body is now a trait, not a struct
> - there's a new Incoming impl for incoming requests on the server side,
> and incoming responses on the client side
> - http-body-util has some more impls
> - proxmox-http has a new impl covering our two common use cases, see
> the patch there for details
>
> hyper now doesn't expose tower's Service or tokio's
> AsyncRead/AsyncWrite, but has its own variants for both with
> corresponding wrappers/adapters.
>
> the previous Accept trait for translation from a listening socket to
> connections is gone, an accept loop should be used instead.
>
> the pooling client is moved from hyper to hyper-util. despite its
> "legacy" label we still use it, as we'd need to either implement a ton
> of code ourself or switch to reqwest otherwise.
>
> graceful shutdown of connections is handled differently, so are
> connection ugprades.
>
> I did some rough testing of the usual things without noticing any
> breakage, but I am sure I missed some parts. there's also room for
> improvement for sure, in particular surrounding the rest-server and
> connection accepting part - suggestions welcome!

All the changes seem pretty solid to me; switching to hyper/1.0 was
never really going to be pretty, but at least hyper-util provides a lot
of the compat wrappers for that.

(Note: The proxmox-backup patches only apply with `git am -3` for me.)

I will give this a spin on my PBS dev VM and let it run on there for a
while to see if I notice anything off. I haven't spotted anything sus in
the code, so I don't expect anything to come up, but still.

Also, I haven't really had any concrete ideas yet for the return value
trait of `accept_tls_optional()` and family, but I'll let you know once
(or if) I do. Previously, all of the individual parts could just be
nicely composed together (incoming connection receiver, connection
handler, etc.), so perhaps we could cook up something similar. The new
pattern with the explicitly defined loop makes that a little hard,
though.

Apart from that, there are a few comments inline which can IMO be
addressed in a follow-up (if at all). Nothing major. This otherwise
looks pretty good to me, nice work! Especially the solution with our
"own" `Body` implementation is pretty nice and a good middle ground. 

Should nothing else come up and this be merged without a refresh,
consider:

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


Feel free to ping me for a re-review / more testing, should you refresh
this series.

>
> proxmox workspace:
>
> Fabian Grünbichler (17):
>   http: order feature values
>   http: rate-limited-stream: update to hyper/http 1.0
>   http: adapt MaybeTlsStream to hyper 1.x
>   http: adapt connector to hyper 1.x
>   http: add Body implementation
>   http: adapt simple client to hyper 1.x
>   http: websocket: update to http/hyper 1
>   openid: use http 0.2 to avoid openidconnect update
>   proxmox-login: switch to http 1.x
>   client: switch to hyper/http 1.0
>   metrics: update to hyper/http 1.0
>   acme: switch to http/hyper 1.0
>   proxmox-router: update to hyper 1.0
>   proxmox-rest-server: update to hyper 1.0
>   proxmox-rest-server: fix and extend example
>   proxmox-auth-api: update to hyper 1.0
>   proxmox-acme-api: update to hyper 1.0
>
>  Cargo.toml                                    |   8 +-
>  proxmox-acme-api/Cargo.toml                   |   4 +
>  proxmox-acme-api/src/acme_plugin.rs           |  63 +++++--
>  proxmox-acme/Cargo.toml                       |   3 +-
>  proxmox-acme/src/async_client.rs              |  11 +-
>  proxmox-auth-api/Cargo.toml                   |   2 +
>  proxmox-auth-api/src/api/access.rs            |   4 +-
>  proxmox-client/Cargo.toml                     |   1 +
>  proxmox-client/src/client.rs                  |  22 +--
>  proxmox-http/Cargo.toml                       |  45 +++--
>  proxmox-http/src/body.rs                      | 133 ++++++++++++++
>  proxmox-http/src/client/connector.rs          |  44 +++--
>  proxmox-http/src/client/simple.rs             |  93 +++++++---
>  proxmox-http/src/client/tls.rs                |   2 +-
>  proxmox-http/src/lib.rs                       |   5 +
>  proxmox-http/src/rate_limited_stream.rs       |   2 +-
>  proxmox-http/src/websocket/mod.rs             |   6 +-
>  proxmox-login/Cargo.toml                      |   2 +-
>  proxmox-metrics/src/influxdb/http.rs          |   5 +-
>  proxmox-openid/Cargo.toml                     |   3 +-
>  proxmox-rest-server/Cargo.toml                |   9 +-
>  .../examples/minimal-rest-server.rs           |  48 ++++-
>  proxmox-rest-server/src/api_config.rs         |  44 ++---
>  proxmox-rest-server/src/connection.rs         |  14 +-
>  proxmox-rest-server/src/formatter.rs          |   8 +-
>  proxmox-rest-server/src/h2service.rs          |  15 +-
>  proxmox-rest-server/src/lib.rs                |   2 +-
>  proxmox-rest-server/src/rest.rs               | 164 +++++++++++-------
>  proxmox-router/Cargo.toml                     |   6 +-
>  proxmox-router/src/router.rs                  |  19 +-
>  proxmox-router/src/stream/parsing.rs          |  16 +-
>  31 files changed, 567 insertions(+), 236 deletions(-)
>  create mode 100644 proxmox-http/src/body.rs
>
> proxmox-backup:
>
> Fabian Grünbichler (6):
>   Revert "h2: switch to legacy feature"
>   pbs-client: adapt http client to hyper/http 1.0
>   pbs-client: vsock: adapt to hyper/http 1.0
>   restore daemon: adapt to hyper/http 1.0
>   adapt to hyper/http 1.0
>   adapt examples to hyper/http 1.0
>
>  Cargo.toml                                    | 10 ++-
>  examples/h2client.rs                          |  6 +-
>  examples/h2s-client.rs                        |  6 +-
>  examples/h2s-server.rs                        | 28 +++-----
>  examples/h2server.rs                          | 28 +++-----
>  pbs-client/Cargo.toml                         |  4 +-
>  pbs-client/src/backup_writer.rs               |  8 +--
>  pbs-client/src/http_client.rs                 | 38 +++++-----
>  pbs-client/src/pipe_to_stream.rs              |  2 +-
>  pbs-client/src/vsock_client.rs                | 27 +++----
>  proxmox-backup-client/Cargo.toml              |  1 +
>  proxmox-backup-client/src/snapshot.rs         |  2 +-
>  proxmox-restore-daemon/Cargo.toml             |  2 +
>  proxmox-restore-daemon/src/main.rs            | 24 +++++--
>  .../src/proxmox_restore_daemon/api.rs         |  6 +-
>  .../src/proxmox_restore_daemon/auth.rs        |  5 +-
>  src/acme/client.rs                            |  6 +-
>  src/acme/plugin.rs                            | 62 +++++++++++-----
>  src/api2/admin/datastore.rs                   | 20 +++---
>  src/api2/backup/environment.rs                |  3 +-
>  src/api2/backup/mod.rs                        | 10 +--
>  src/api2/backup/upload_chunk.rs               | 47 +++++++------
>  src/api2/helpers.rs                           |  3 +-
>  src/api2/node/mod.rs                          |  7 +-
>  src/api2/node/tasks.rs                        |  7 +-
>  src/api2/reader/mod.rs                        | 17 +++--
>  src/bin/proxmox-backup-api.rs                 | 40 +++++++----
>  src/bin/proxmox-backup-proxy.rs               | 70 +++++++++++++++----
>  28 files changed, 297 insertions(+), 192 deletions(-)



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