hyper-util missing max_pending_accept_reset_streams
#3461
Labels
C-feature
Category: feature. This is adding a new feature.
E-easy
Effort: easy. A task that would be a great starting point for a new contributor.
K-hyper-util
Crate: hyper-util
Comments
11 tasks
seanmonstar
added
E-easy
|
It appears that the referenced PR's changes didn't make the cut into 1.0.0. Is there any reason why the pattern of calling the I'm no git wizard, but I could copy/paste those changes into a new PR for |
This was referenced Jan 10, 2024
cratelyn
added a commit
to penumbra-zone/penumbra
that referenced
this issue
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 issue
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 issue
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
cratelyn
added a commit
to cratelyn/hyper
that referenced
this issue
Nov 27, 2024
…builder NB: this commit is based on the `0.14.x` release series. the `server::conn::http2` submodule provides types that are conditionally compiled when the `backports` feature is active, to facilitate upgrading to the `1.x` release. i've been working on upgrading the [`linkerd2-proxy`](https://github.com/linkerd/linkerd2-proxy/) project to use hyper 1.0, but encountered some gaps in `Builder<E>`'s interface when setting the `backports` and `deprecated` features. this adds a `max_pending_accept_reset_streams(..)` method (_added in #3201_) to this builder, so that 0.14.x users relying on this functionality can prepare to upgrade to hyper 1.x safely. this is effectively a backport of hyperium#3507, which restored this interface to the 1.0 release. for more information, see: * hyperium#3201 * hyperium#3507 * hyperium#3461 * https://hyper.rs/guides/1/upgrading/ Signed-off-by: katelyn martin <[email protected]>
cratelyn
added a commit
to cratelyn/hyper
that referenced
this issue
Nov 27, 2024
…builder NB: this commit is based on the `0.14.x` release series. the `server::conn::http2` submodule provides types that are conditionally compiled when the `backports` feature is active, to facilitate upgrading to the `1.x` release. i've been working on upgrading the [`linkerd2-proxy`](https://github.com/linkerd/linkerd2-proxy/) project to use hyper 1.0, but encountered some gaps in `Builder<E>`'s interface when setting the `backports` and `deprecated` features. this adds a `max_pending_accept_reset_streams(..)` method (_added in #3201_) to this builder, so that 0.14.x users relying on this functionality can prepare to upgrade to hyper 1.x safely. this is effectively a backport of hyperium#3507, which restored this interface to the 1.0 release. for more information, see: * hyperium#3201 * hyperium#3507 * hyperium#3461 * https://hyper.rs/guides/1/upgrading/ Signed-off-by: katelyn martin <[email protected]>
seanmonstar
pushed a commit
that referenced
this issue
Nov 29, 2024
#3796) the `server::conn::http2` submodule provides types that are conditionally compiled when the `backports` feature is active, to facilitate upgrading to the `1.x` release. i've been working on upgrading the [`linkerd2-proxy`](https://github.com/linkerd/linkerd2-proxy/) project to use hyper 1.0, but encountered some gaps in `Builder<E>`'s interface when setting the `backports` and `deprecated` features. this adds a `max_pending_accept_reset_streams(..)` method (_added in #3201_) to this builder, so that 0.14.x users relying on this functionality can prepare to upgrade to hyper 1.x safely. this is effectively a backport of #3507, which restored this interface to the 1.0 release. for more information, see: * #3201 * #3507 * #3461 * https://hyper.rs/guides/1/upgrading/ Signed-off-by: katelyn martin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
hyper 0.14.27 has it [1]. It was merged here.
hyper-util 0.1.1 is missing it [2].
The other 10 http2 methods are present. This is the only one missing.
PR incoming
The text was updated successfully, but these errors were encountered: