Update to hyper 1.0 and axum 0.7 #1595

ikrivosheev · 2024-01-10T16:15:34Z

Hello! This is continue for: #1583

Also I merged:

TODO:

Depends on:

hyper-util missing max_pending_accept_reset_streams hyper#3461

Closed: #1579

Some low-hanging import fixes

This commit is primarily converting Request and Response types within the transport module to Axum 0.7 Request/Response. There is still more to come to finish this conversion. There are also small changes such as updating the hyper service builder syntax. Over the course of this commit,it was discovered that hyper-util is missing `http2_max_pending_accept_reset_streams`.

Replaced with `tokio::net::TcpStream`. Inspired by hyperium/hyper#2850

- use `Frame`. - update `poll_data` to `poll_frame` - remove `poll_trailers` in most places I am not sure that a simple rename of `poll_data` to `poll_frame` is right. I also left one instance of `poll_trailers` in *tonic/src/codec/decode.rs*.

`is_connect` was deprecated when the higher-level client was removed from hyper. The corresponding comments are removed as well.

`http::Extensions::insert` requires `T: Clone` so we add it.

ikrivosheev · 2024-01-15T07:28:21Z

Well, now I understand. I need to re-implement two structs:

And Channel will start to compile. Great, small, but progress))

ikrivosheev · 2024-01-15T07:29:17Z

Maybe it will be useful in hyper-utils.

``` /!\ this is a work in progress and will /!\ /!\ be force pushed until ready for review /!\ ``` ## 👀 overview fixes #3627. this reorganizes the logic in pd's startup code related to automatically managed https functionality. ## 🎨 background & motivation this PR, besides cleaning up the `rustls-acme`-related auto-https logic, is also interested in *creating a state-of-affairs that will dovetail into pr #3522*. in particular, this expression to start the GRPC serve given a bound listener... ```rust tokio::task::Builder::new() .name("grpc_server") .spawn(grpc_server.serve_with_incoming(tls_incoming)) .expect("failed to spawn grpc server") ``` ...should be adjusted so as to accept an `axum::Router`. other tertiary requirements: - when no `grpc_auto_https` flag has been configured, serve without using an ACME resolver - ALPN permit http/1.1 for grpc-web backwards compatibility. ### ⚖️ `rustls-acme` and `tokio-rustls-acme` quoth the #3627 description, citing an earlier comment: > In the ~year since this code was written, there may be better options. > `tokio-rustls-acme` seems promising \- <#3522 (comment)> for reference, the repositories for each live here, and here: - <https://github.com/FlorianUekermann/rustls-acme> - <https://github.com/n0-computer/tokio-rustls-acme> after some comparison, i have come to the opinion that `rustls-acme` will still be adequate for our purposes. the latter is a fork of the former, but active development appears to have continued in the former, and i did not see any particular "_must-have_" features for us in the latter. ## 🚰 dropping down to `axum` (this part is lengthy...) as stated above, we want to switch to an `axum::Router`. this means that we won't be able to use the `AcmeConfig::incoming` function. the `rustls-acme` library provides some "low-level" examples we can check out: - <https://github.com/FlorianUekermann/rustls-acme/blob/main/examples/low_level_tokio.rs> - <https://github.com/FlorianUekermann/rustls-acme/blob/main/examples/low_level_axum.rs> `low_level_axum` sounds promising, given that #3522 itself drops down from tonic into axum. a catch is that the "axum" example is referring to `axum-server`, a library that wraps `axum`, rather than `axum` itself. more on that momentarily. we also use `tonic` 0.10.2 in pd, and elsewhere in the penumbra monorepo. tonic isn't using hyper 1.x yet. this was being worked on in hyperium/tonic#1583, continued on in hyperium/tonic#1595, and tracked in hyperium/tonic#1579. that work also depends upon hyperium/hyper#3461. this is complicated by the fact that `axum-server` 0.6.0 depends on `hyper` 1.1.0. this is particularly relevant because of hyperium/hyper#3040. so, we must be mindful that there is now a `tower::Service` and a `hyper::Service`. `hyper-util` provides facilities to convert between the two, and other helper glue. additionally, see the "_upgrading"_ migration guide for more information on the differences between pre- and post-1.0 interfaces in `hyper`. - https://hyper.rs/guides/1/upgrading/ - <https://docs.rs/hyper-util/latest/hyper_util/service/struct.TowerToHyperService.html> at a high level we need to take our `tonic::Router`, and turn that into a `MakeService`, some object that can be used to _generate_ `Service` instances to process a request. this is additionally complicated by the fact that `axum-server` defines its _own_ `MakeService` trait. that trait is sealed, blanket implemented, and is a bound included in `axum_server::server::Server::serve`. ```rust // axum_server::serve impl Server { pub async fn serve<M>(self, mut make_service: M) -> io::Result<()> where M: MakeService<SocketAddr, Request<Incoming>>, // ... { todo!() } } ``` the `SocketAddr` parameter above is of importance. `axum::Router::into_make_service` gives us `IntoMakeService<Router>` - <https://docs.rs/axum/latest/axum/routing/struct.IntoMakeService.html> ```rust impl<S, T> Service<T> for IntoMakeService<S> where S: Clone, { type Response = S; type Error = Infallible; type Future = IntoMakeServiceFuture<S>; ``` ...which means that we are faced with this error: ``` error[E0277]: the trait bound `Router: tower_service::Service<http::request::Request<hyper::body::incoming::Incoming>>` is not satisfied --> crates/bin/pd/src/auto_https.rs:62:20 | 62 | .serve(make_svc) | ----- ^^^^^^^^ the trait `tower_service::Service<http::request::Request<hyper::body::incoming::Incoming>>` is not implemented for `Router` | | | required by a bound introduced by this call | = help: the trait `tower_service::Service<axum::http::Request<axum::body::Body>>` is implemented for `Router` = help: for that trait implementation, expected `axum::http::Request<axum::body::Body>`, found `http::request::Request<hyper::body::incoming::Incoming>` = note: required for `IntoMakeService<Router>` to implement `axum_server::service::MakeService<std::net::SocketAddr, http::request::Request<hyper::body::incoming::Incoming>>` ``` `axum::Router::into_make_service` does not play well with `axum_server::Server`. ...and if we specify the new `hyper::body::incoming::Incoming` as the body type for the axum router, ``` error[E0599]: the method `into_make_service` exists for struct `Router<(), Incoming>`, but its trait bounds were not satisfied --> crates/bin/pd/src/auto_https.rs:57:31 | 57 | let make_svc = router.into_make_service(); | ^^^^^^^^^^^^^^^^^ method cannot be called on `Router<(), Incoming>` due to unsatisfied trait bounds | ::: /home/katie/.cargo/registry/src/index.crates.io-6f17d22bba15001f/hyper-1.1.0/src/body/incoming.rs:47:1 | 47 | pub struct Incoming { | ------------------- doesn't satisfy `hyper::body::Incoming: HttpBody` | = note: the following trait bounds were not satisfied: `hyper::body::Incoming: HttpBody` ``` `HttpBody` was the pre-1.0 trait, that is now `Body` post-1.0. in short, there are some incongruencies at play as different libraries catch up to the new breaking changes in `hyper`. ## 🤨 ...so now what? TODO(kate): write up course of action for tomorrow. --- 🟥 NB: i worry about breakage due to untested load-bearing behavior during a migration like this. see "tertiary requirements", above. consider this a PR to be suspect of when deploying a new testnet. Refs: #3627 Refs: #3646 Refs: #3522

## 👀 overview fixes #3627. this reorganizes the logic in pd's startup code related to automatically managed https functionality. ## 🎨 background & motivation this PR, besides cleaning up the `rustls-acme`-related auto-https logic, is also interested in *creating a state-of-affairs that will dovetail into pr #3522*. in particular, this expression to start the GRPC serve given a bound listener... ```rust tokio::task::Builder::new() .name("grpc_server") .spawn(grpc_server.serve_with_incoming(tls_incoming)) .expect("failed to spawn grpc server") ``` ...should be adjusted so as to work with an `axum::Router`. ### ⚖️ `rustls-acme` and `tokio-rustls-acme` quoth the #3627 description, citing an earlier comment: > In the ~year since this code was written, there may be better options. > `tokio-rustls-acme` seems promising \- <#3522 (comment)> for reference, the repositories for each live here, and here: - <https://github.com/FlorianUekermann/rustls-acme> - <https://github.com/n0-computer/tokio-rustls-acme> after some comparison, i have come to the opinion that `rustls-acme` will still be adequate for our purposes. the latter is a fork of the former, but active development appears to have continued in the former, and i did not see any particular "_must-have_" features for us in the latter. ## 🎴 changes this commit moves some of the auto-https related code from the `main` entrypoint, into standalone functions in `pd::main`. some constants are defined, to keep control flow clear and to help facilitate the addition of future options e.g. a flag to control the LetsEncrypt environment to use. ## 🚰 dropping down to `axum`; a brief note on future upgrades as stated above, we want to switch to an `axum::Router`. this means that we won't be able to use the `AcmeConfig::incoming` function. the `rustls-acme` library provides some "low-level" examples this work is based upon - <https://github.com/FlorianUekermann/rustls-acme/blob/main/examples/low_level_tokio.rs> - <https://github.com/FlorianUekermann/rustls-acme/blob/main/examples/low_level_axum.rs> we also use `tonic` 0.10.2 in pd, and elsewhere in the penumbra monorepo. tonic isn't using hyper 1.x yet. this was being worked on in hyperium/tonic#1583, continued on in hyperium/tonic#1595, and tracked in hyperium/tonic#1579. that work also depends upon hyperium/hyper#3461. we will need to be wait for tonic to finish its migration over to hyper 1.0, see: hyperium/tonic#1579 (comment) this is understandable, but i make note of this situation as a signpost for our future selves when considering a migration to recent versions of axum-server, axum, rustls-acme, or hyper. for now, it's easiest to stay in lock-step with tonic, and we can revisit the upgrade path(s) at a future date. === Refs: #3627 Refs: #3646 Refs: #3522

jakubadamw · 2024-03-10T16:02:27Z

@ikrivosheev do you plan on continuing with this PR?

ikrivosheev · 2024-03-10T17:35:07Z

@ikrivosheev do you plan on continuing with this PR?

@jakubadamw , hello! Yes, I do. At the moment I need to finish tasks in hyper-util (hyperium/hyper#3080)

rakshith-ravi · 2024-03-27T09:49:19Z

@ikrivosheev is there anyway I can help in pushing this forward?

ikrivosheev · 2024-03-27T12:39:16Z

@ikrivosheev is there anyway I can help in pushing this forward?

Hello! For the PR needs something like: https://docs.rs/hyper/0.14.28/hyper/client/service/struct.Connect.html. A service which return a service https://docs.rs/hyper/0.14.28/hyper/client/conn/struct.SendRequest.html for make http request.

stormshield-gt · 2024-03-28T14:21:13Z

From my understanding this PR will unlock upgrading to rustls 0.23 as hyper_rustls 0.27 uses hyper 1.0.

rakshith-ravi · 2024-03-28T14:58:46Z

Hello! For the PR needs something like: https://docs.rs/hyper/0.14.28/hyper/client/service/struct.Connect.html. A service which return a service https://docs.rs/hyper/0.14.28/hyper/client/conn/struct.SendRequest.html for make http request.

hyper-util can help us here. Can we consider using some of those structs?

ikrivosheev · 2024-03-28T21:38:51Z

Hello! For the PR needs something like: https://docs.rs/hyper/0.14.28/hyper/client/service/struct.Connect.html. A service which return a service https://docs.rs/hyper/0.14.28/hyper/client/conn/struct.SendRequest.html for make http request.

hyper-util can help us here. Can we consider using some of those structs?

Yes, I saw it. But there aren't structures like in the old hyper version.

alexrudy · 2024-03-31T15:14:45Z

I think I got the SendRequest and Connection pieces working in #1670, but I'm running into what I think is a compiler error.

In that PR I still want to finish handling poll_frame and properly add graceful shutdowns (both things I punted on while just trying to get tonic to compile)

LockedThread · 2024-03-31T16:59:36Z

I really appreciate you guys working on this. I am looking forward to multiplexing Axum and Tonic in my current project. Right now I have strict dependencies on Hyper 1.0+ and Axum 0.7+.

If there is anything I can do to help out please let me know.

ikrivosheev mentioned this pull request Jan 10, 2024

Draft: Update to hyper 1.0 and axum 0.7 #1583

Closed

11 tasks

allan2 and others added 12 commits January 10, 2024 19:17

Update deps, hyper 1.0, axum 0.7

9b37761

Update imports to use util crates

2d4595a

Some low-hanging import fixes

Chop out AddrStream

a3f9d80

Replaced with `tokio::net::TcpStream`. Inspired by hyperium/hyper#2850

Replace AddrIncoming with TcpListener

c54b484

Update impl of http_body::Body

e0b454c

- use `Frame`. - update `poll_data` to `poll_frame` - remove `poll_trailers` in most places I am not sure that a simple rename of `poll_data` to `poll_frame` is right. I also left one instance of `poll_trailers` in *tonic/src/codec/decode.rs*.

More http2 method renames

ba2299f

Use BodyExt trait

85da9ef

Remove hyper::Error::is_connect

686dbf5

`is_connect` was deprecated when the higher-level client was removed from hyper. The corresponding comments are removed as well.

Add Clone to Extensions::insert<T>

5e32e00

`http::Extensions::insert` requires `T: Clone` so we add it.

Use TcpStream instead of hyper connect

cd05673

add poll_frame, delete poll_trailers

46ee628

ikrivosheev force-pushed the hyper-1.0 branch from 2c1e04c to 46ee628 Compare January 10, 2024 16:19

ikrivosheev added 3 commits January 12, 2024 20:58

Fixed incoming

60eef43

Fixed

03a428d

Fixes

69f904c

gautam8404 mentioned this pull request Jan 20, 2024

Upgrade to http 1.0 and axum 0.7 #1579

Closed

fuyufjh mentioned this pull request Jan 25, 2024

Bump version of hyper from 0.14 to 1.0 risingwavelabs/risingwave#13695

Open

cratelyn mentioned this pull request Jan 25, 2024

pd: 🔨 rework RootCommand::start auto-https logic penumbra-zone/penumbra#3652

Merged

alexrudy mentioned this pull request Mar 31, 2024

Upgrade to Hyper 1.0 & Axum 0.7 #1670

Merged

17 tasks

tisonkun mentioned this pull request Apr 1, 2024

Upgrade Axum to 0.7 GreptimeTeam/greptimedb#3610

Open

ikrivosheev closed this Apr 2, 2024

ikrivosheev deleted the hyper-1.0 branch May 9, 2024 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to hyper 1.0 and axum 0.7 #1595

Update to hyper 1.0 and axum 0.7 #1595

ikrivosheev commented Jan 10, 2024 •

edited

Loading

ikrivosheev commented Jan 15, 2024

ikrivosheev commented Jan 15, 2024

jakubadamw commented Mar 10, 2024

ikrivosheev commented Mar 10, 2024 •

edited

Loading

rakshith-ravi commented Mar 27, 2024

ikrivosheev commented Mar 27, 2024

stormshield-gt commented Mar 28, 2024 •

edited

Loading

rakshith-ravi commented Mar 28, 2024

ikrivosheev commented Mar 28, 2024

alexrudy commented Mar 31, 2024

LockedThread commented Mar 31, 2024 •

edited

Loading

Update to hyper 1.0 and axum 0.7 #1595

Update to hyper 1.0 and axum 0.7 #1595

Conversation

ikrivosheev commented Jan 10, 2024 • edited Loading

ikrivosheev commented Jan 15, 2024

ikrivosheev commented Jan 15, 2024

jakubadamw commented Mar 10, 2024

ikrivosheev commented Mar 10, 2024 • edited Loading

rakshith-ravi commented Mar 27, 2024

ikrivosheev commented Mar 27, 2024

stormshield-gt commented Mar 28, 2024 • edited Loading

rakshith-ravi commented Mar 28, 2024

ikrivosheev commented Mar 28, 2024

alexrudy commented Mar 31, 2024

LockedThread commented Mar 31, 2024 • edited Loading

ikrivosheev commented Jan 10, 2024 •

edited

Loading

ikrivosheev commented Mar 10, 2024 •

edited

Loading

stormshield-gt commented Mar 28, 2024 •

edited

Loading

LockedThread commented Mar 31, 2024 •

edited

Loading