Draft: Update to hyper 1.0 and axum 0.7 #1583
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
1 task
1 task
|
@allan2 hello. Do you work on the PR? Maybe do you need help? |
|
@ikrivosheev I haven't yet. Feel free to contribute :) |
|
@allan2 Is hyperium/hyper-util#78 blocked until hyperium/hyper@a9fa893 is released? If so, can you conceive of a method utilizing the |
11 tasks
|
I opened PR: #1595. I will continue working on it. |
Use TcpStream instead of hyper connect
add poll_frame, delete poll_trailers
cratelyn
added a commit
to penumbra-zone/penumbra
that referenced
this pull request
Jan 24, 2024
```
/!\ 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
cratelyn
added a commit
to penumbra-zone/penumbra
that referenced
this pull request
Jan 25, 2024
## 👀 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
conorsch
pushed a commit
to penumbra-zone/penumbra
that referenced
this pull request
Jan 26, 2024
## 👀 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
|
This was superseded by #1670, which is now merged. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi, apologies for the delay.
This PR is to update
hyperto 1.0,httpto 1.0,http-bodyto 1.0, andaxumto 0.7.-utilcratesAddrStreamandAddrIncomingRequestandResponsein transport (in-progress)poll_frametoimpl http_body::Body(in-progress)poll_trailersfromimpl http_body::Body(in-progress)hyper::client::service::Connect)Depends on:
max_pending_accept_reset_streamshyper#3461Under consideration:
tonic::body::BoxBodytotonic::body::Bodyto match axumTcpIncominghasTcpListeneras the inner type instead of the deprecatedAddrIncoming, canTcpIncomingbe simplified?I tried to break it up so that it is easier to review commit-by-commit.
There are some things I am still working through. Any feedback would be helpful.
Closes #1579