Upgrade to Hyper 1.0 & Axum 0.7 #1670

alexrudy · 2024-03-31T15:12:27Z

Current Status: This is ready for CI & review!

This is a continuation of PR #1595 by @ikrivosheev.

TODO:

alexrudy · 2024-04-03T23:55:41Z

I'd like to propose that this be reviewed as-is, and that we add follow-up issues for the remaining todos, which will all require PRs to land and then releases to happen for hyper and/or hyper-utils.

karuna · 2024-04-06T05:58:34Z

works perfectly after cargo update

remove `--ignore` option when pr(when hyperium/tonic#1670) is merged. Signed-off-by: Phoeniix Zhao <Phoenix500526@163.com>

aumetra · 2024-04-08T20:33:18Z

examples/Cargo.toml

-rustls-pemfile = { version = "1", optional = true }
-tower-http = { version = "0.4", optional = true }
+tokio-rustls = { version = "0.26.0", optional = true }
+hyper-rustls = { version = "0.27.0", features = ["http2"], optional = true }


It would be nice if there were feature flags to switch between the crypto backends.
Defaulting to aws-lc-rs pulls in a cmake, nasm requirement, impacts platform support, etc.
Trading that off for FIPS compliance isn't really needed in most cases.

Which crypto provider tonic should default to is up to the maintainers, I guess.
But reqwest seems to be going the route of sticking to ring as the default crypto backend.

I think that is a good idea, but I don't think it should be included in this PR - this PR is already complicated as is. We can create an issue and work on multi-backend support separately once this lands.

Yeah, definitely. I just felt like the comment fit best here for the time being since the PR isn't merged yet and I probably would have forgotten about it if I hadn't written it down

Following up here - I reverted the default to use ring and not aws-lc-rs for exactly the same reason as reqwest

tisonkun · 2024-04-16T08:35:50Z

cc @tottoto @LucioFranco can we get this patch a review? This should be one of the last blockers for many downstreams upgrade to hyper 1.0.

tisonkun · 2024-04-25T01:00:55Z

@alexrudy FYI hyperium/hyper-util#102 is merged now.

hubertshelley · 2024-05-15T14:18:32Z

@alexrudy Example h2c failed.
Terminal output:

called `Result::unwrap()` on an `Err` value: hyper::Error(User(ManualUpgrade))

alexrudy · 2024-05-16T16:32:14Z

@alexrudy Example h2c failed.

Thanks! 5a579e6 contains the fix.

djc · 2024-05-22T11:59:59Z

@alexrudy and @ikrivosheev thanks for all your work on this!

I'm going to be helping @LucioFranco out with tonic reviews, and obviously this is a desirable change. As such, I've made an initial pass of the changes here. Here are some thoughts:

Since this is already such a large change, I'd like to avoid intertwining the changes that are necessary to upgrade hyper/axum with any other changes that can be done independently. For example, reformatting of Cargo.toml files, upgrading to a different nightly, reordering imports and other small changes like adding type annotations. To facilitate review I think all of those changes should either be reverted in this PR or submitted as part of separate PRs (with one commit or PR per logical change) where we can judge them properly -- the goal is to make this PR as small and focused as possible.
I think it might make sense for now to just delete any of the examples that rely on warp, probably in a separate PR? While warp was one of the newer options when tonic got started, today it's much less popular than Axum -- and by virtue of doing this in a separate commit/PR it should be relatively straightforward to bring back those examples once warp upgrades to hyper/http 1.
I would like to avoid further entangling of Axum and Tonic proper. Ideally this change should be neutral in how much of Axum is used in -- if that's hard for some reason let's discuss that separately? Especially for client-only users, we should be working in a direction where it's possible to avoid depending on Axum.

LucioFranco

I am still working through the PR, I think what @djc makes a lot of sense, I am going to continue reviewing the core web and tonic parts since it seems like those changes are somewhat non trivial. So it will take some time just as an FYI.

tests/integration_tests/tests/connection.rs

alexrudy · 2024-05-23T21:12:24Z

@djc Thanks so much for your initial comments.

Overall, I tried to minimally scope the changes here based on what was required to make the tests pass with hyper >= 1.

For instance:

The change in the nightly compiler version from 2024-02-06 to 2024-04-16 is required to make Axum >= 0.7 compile on nightly (it otherwise requires #![feature(diagnostic_namespace)] to compile, and we can't change the nightly features requested by a dependency IIUC.
There are type annotations added where type inference was no longer sufficient to allow code to compile - e.g. in the h2c/server.rs example, due to the move from calling hyper's server directly to calling TcpListener:bind(), type inference is no longer able to resolve the FromStr::Err type (which will get transformed to Box<dyn Error>, and in tls_rustls examples, types have to be added because rustls and rustls_pemfile have changed their public API types, which in turn are required upgrades (I think) for compatibility with hyper-rustls for hyper 1.0.

I will fix the cargo.toml formatting (this came from my IDE's auto formatter, and I should have caught that before).

For warp – I'm happy to delete those examples entirely, either in this PR or another one - deleting them is a pre-requisite to the CI tests passing.

This also brings up @LucioFranco's point about the changed error type, which we might want to leave failing: that sounds fine by me, I don't know of a good way though to leave a test in an expected failure state in Rust. Also, just letting the test fail will be problematic, as CI is currently configured to fail-fast, meaning that one failure will cause a lot of other tests to be skipped entirely. We could change that setting as well, if that is what is desired.

On Axum: I don't think that this PR increases the degree to which the two libraries are intertwined - there is some private adaptor code added, but the tonic::transport::service::router part has always been built on the axum::Router class - are you suggesting that we should be implementing our own routing? That feels like a separate endeavor to me. Or was there somewhere else that this PR ends up intertwining the two further? From my perspective, I thought it was pretty neutral about how much of Axum is required by tonic.

Finally, it seems like you both have a preference for a re-written history here to break commits into more logical chunks - I'll take a stab at that as well, I had defaulted to preserving history to start, but am fine with either strategy.

Overall, @djc, I would love specific comments on parts of this PR that are really separable - or the decision that we'd rather merge a series of PRs, some of which contain the pre-requistites to the upgrade to hyper-1.0, and the final one that contains the final upgrade. Given the scope of the API changes, I'm personally opposed to that latter solution, since sometimes solving for code which is compatible with pre-1.0 hyper and post-1.0 hyper adds additional complexity to the implementation, which we'd then have to clean up as well.

tisonkun · 2024-05-25T00:55:14Z

@alexrudy FYI hyper-util v0.1.4 should contain the API you need above.

ikrivosheev · 2024-05-25T11:53:25Z

@alexrudy as I can see, hyper-util v0.1.4 is released. Can you upgrade the PR?

https://crates.io/crates/hyper-util

alexrudy · 2024-05-29T17:34:30Z

@djc Super helpful comments - especially since I've been knee-deep in this PR, and some of it I inherited from the previous attempts. Thanks for taking the time. I've spent some time minimizing the formatting changes etc. that you commented on.

The Body trait has changed (removed `poll_data` and `poll_trailers`, they are now combined in `poll_frame`) and so the codec must be re-written to merge those two methods. Co-authored-by: Ivan Krivosheev <py.krivosheev@gmail.com> Co-authored-by: Allan Zhang <allanzhang7@gmail.com>

…ody types The Body trait has changed (removed `poll_data` and `poll_trailers`, they are now combined in `poll_frame`) and so the codec must be re-written to merge those two methods. This also handles the return types which should now be wrapped in `Frame` when appropriate. Co-authored-by: Ivan Krivosheev <py.krivosheev@gmail.com> Co-authored-by: Allan Zhang <allanzhang7@gmail.com>

Here, we must update some body types which are no longer valid. (A) BoxBody no longer has an `empty` method, instead we provide a helper in `tonic::body` for creating an empty boxed body via `http_body_util`. As well, `hyper::Body` is no longer a type, and instead, `hyper::Incoming` is used when directly recieving a Request from hyper, and `BoxBody` is used when the request may have passed through an axum router. In tonic, we prefer `BoxBody` as it allows for services to be used downstream from other components which enforce a specific body type (e.g. Axum), at the cost of making Body streaming opaque. Co-authored-by: Ivan Krivosheev <py.krivosheev@gmail.com> Co-authored-by: Allan Zhang <allanzhang7@gmail.com>

… types The Body trait has changed (removed `poll_data` and `poll_trailers`, they are now combined in `poll_frame`) and so the codec must be re-written to merge those two methods. Co-authored-by: Ivan Krivosheev <py.krivosheev@gmail.com> Co-authored-by: Ludea <ludovicw35@hotmail.com>

In h2c, when a service is receiving from hyper, it has to accept a `hyper::body::Incoming` in hyper >= 1. Additionally, response bodies must be built from `http_body_util` combinators and become BoxBody objects.

The Body trait has changed (removed `poll_data` and `poll_trailers`, they are now combined in `poll_frame`) and so the codec must be re-written to merge those two methods.

hyper >= 1 provides its own I/O traits (Read & Write) instead of relying on the equivalent traits from `tokio`. Then, `hyper-util` provides adaptor structs to wrap `tokio` I/O objects and implement the hyper equivalents. Therefore, we update the appropriate bounds to use the hyper traits, and update the I/O objects so that they are wrapped in the tokio to hyper adaptor. Co-authored-by: Ivan Krivosheev <py.krivosheev@gmail.com> Co-authored-by: Allan Zhang <allanzhang7@gmail.com>

Axum must be >= 0.7 to support hyper >= 1 Doing this also involves changing the Body type used. Since hyper >= 1 does not provide a generic body type, Axum and tonic both use `BoxBody` to provide a pointer to a Body. This changes the trait bounds required for methods which accept additional Serivces to be run alongside the primary GRPC service, since those will be routed with Axum, and therefore must accept a BoxBody. Co-authored-by: Ivan Krivosheev <py.krivosheev@gmail.com> Co-authored-by: Allan Zhang <allanzhang7@gmail.com>

Hyper >= 1 no longer includes automatic http2/http1 combined connections, and so we must swtich to the `http2::Builder` type (this is okay, we set http2_only(true) anyhow). As well, hyper >= 1 is generic over executors and does not directly depend on tokio. Since http2 connections can be multiplexed, they require some additional background task to handle sending and receiving requests. Additionally, these background tasks do not natively implement `tower::Service` since hyper >= 1 does not depend on `tower`. Therefore, we re-implement the `SendRequest` task as a tower::Service, so that it can be used within `Connection`, which expects to operate on a tower::Service to serve connections. Co-authored-by: Ivan Krivosheev <py.krivosheev@gmail.com> Co-authored-by: Allan Zhang <allanzhang7@gmail.com>

`hyper::Client` has been moved to `hyper_util::legacy::Client` in version 1. Co-authored-by: Ivan Krivosheev <py.krivosheev@gmail.com> Co-authored-by: Allan Zhang <allanzhang7@gmail.com>

hyper::Error no longer provides information about Connect errors, especially since hyper_util now contains the connection implementation, it does not provide a separate error type. Instead, we create an internal Error type which is used in our own connectors, and then checked when figuring out what the gRPC status should be.

hyper >= 1 has deprecated all of `hyper::server`, including `AddrStream` Co-authored-by: Ivan Krivosheev <py.krivosheev@gmail.com> Co-authored-by: Allan Zhang <allanzhang7@gmail.com> Replace hyper::server::Accept hyper::server is deprectaed. Instead, we implement our own TCP-incoming based on the now removed hyper::server::Accept. In order to set `TCP_KEEPALIVE` we require the socket2 crate, since this option is not exposed in the standard library’s API. The implementaiton is inspired by that of hyper v0.14

hyper::Server is deprecated, with no current common replacement. Instead of implementing (or using tonic’s new) full server in here, we write a simple accept loop, which is sufficient to demonstrate the functionality of h2c.

hyper-rustls requires version 0.27.0 to support hyper >= 1, bringing a few other tls bumps along. Importantly, we add the “ring” and “tls12” features to use ring as the crypto backend, consistent with previous versions of tonic. A future version of tonic might support selecting backends via features. Co-authored-by: Ivan Krivosheev <py.krivosheev@gmail.com>

LucioFranco · 2024-05-29T20:43:27Z

tonic/src/codec/decode.rs

+                    Ok(Some(()))
+                }
+                frame if frame.is_trailers() => {
+                    self.trailers = Some(frame.into_trailers().unwrap());


Do we want to potentially merge the trailers rather than replace the old ones? This sounds like another lack of guarantee from the Body trait that isn't super clear what we should do. I think the safe bet is to merge?

I think that makes sense - I can't find any specific documentation on what the semantics of poll_frame should be. The Body trait vaugely implies that trailers should only come at the end, but even in that case it might still be legal to end a body with two trailer frames.

@seanmonstar do you have any context on this? I'd be happy to submit a docs PR for http_body if you can clarify the intended semantics?

There should only be one trailers frame. That's what h2 and h3 do. But some things are overly complicated to encode into the type system... So, I guess someone could make a body that does weird things.

LucioFranco · 2024-05-29T20:47:22Z

tonic/src/codec/encode.rs

                }

+                self.is_end_stream = true;


This seems to be a change not related to upgrading, can you explain why this was moved out of the if statement?

Ah I see let me know if this is correct, now that the data/trailers are combined into the same stateless poll fn we can always assume that when we call trailers that this is when the incoming stream of bytes has ended so the stream is always over at this point.

Yeah, we only call trailers after the stream has returned None, so it is safe to assume it is done. Otherwise, we don't ever mark is_end_stream when the stream ends normally without an error.

LucioFranco · 2024-05-29T20:51:10Z

tonic/src/transport/mod.rs

@@ -38,29 +39,31 @@
 //!     .connect()
 //!     .await?;
 //!
-//! channel.call(Request::new(BoxBody::empty())).await?;
+//! channel.call(Request::new(empty_body())).await?;


nit: should we just fully qualify this instead of hiding the import. Feels a bit confusing as to where this empty_body fn comes from.

LucioFranco · 2024-05-29T20:55:29Z

I've gotten about halfway through the commits and will continue reviewing likely tomorrow depending on time. Left some feedback but mostly questions. Overall looks great. Thank you so much for breaking this up into commits, it has made it so much easier to review!

alexrudy mentioned this pull request Mar 31, 2024

Update to hyper 1.0 and axum 0.7 #1595

Closed

11 tasks

dswij mentioned this pull request Mar 31, 2024

feat(h2/client): add max_pending_accept_reset_streams option hyperium/hyper#3617

Merged

alexrudy force-pushed the hyper-1.0 branch from f4f57d2 to 01c055d Compare April 3, 2024 03:40

alexrudy marked this pull request as ready for review April 3, 2024 03:41

davidli2010 mentioned this pull request Apr 4, 2024

Update tonic to 0.11 etcdv3/etcd-client#75

Merged

tisonkun mentioned this pull request Apr 7, 2024

Upgrade Axum to 0.7 GreptimeTeam/greptimedb#3610

Open

Phoenix500526 added a commit to Phoenix500526/Xline that referenced this pull request Apr 7, 2024

ci: ignore RUSTSEC-2024-0332 in audit

fb22752

remove `--ignore` option when pr(when hyperium/tonic#1670) is merged. Signed-off-by: Phoeniix Zhao <Phoenix500526@163.com>

Phoenix500526 mentioned this pull request Apr 7, 2024

ci: ignore RUSTSEC-2024-0332 in audit xline-kv/Xline#759

Closed

Phoenix500526 added a commit to xline-kv/Xline that referenced this pull request Apr 8, 2024

ci: ignore RUSTSEC-2024-0332 in audit

e50dad9

remove `--ignore` option when pr(when hyperium/tonic#1670) is merged. Signed-off-by: Phoeniix Zhao <Phoenix500526@163.com>

aumetra reviewed Apr 8, 2024

View reviewed changes

aumetra mentioned this pull request Apr 22, 2024

Upgrade to hyper/http v1.0 open-telemetry/opentelemetry-rust#1674

Open

4 tasks

tisonkun mentioned this pull request Apr 24, 2024

Adds max_pending_accept_reset_streams for legacy hyperium/hyper-util#102

Merged

hseeberger mentioned this pull request May 6, 2024

2023-08-28-dist-tracing-3/ hseeberger/comments#2

Open

aran mentioned this pull request May 9, 2024

Upgrade to http 1.0 and axum 0.7 #1579

Open

aumetra mentioned this pull request May 11, 2024

Lower dependency count kitsune-soc/kitsune#533

Merged

1 task

tisonkun mentioned this pull request May 18, 2024

feat: add {http1,http2}_only for auto conn hyperium/hyper-util#111

Merged

alexrudy force-pushed the hyper-1.0 branch 2 times, most recently from 5f59197 to d50ce89 Compare May 21, 2024 04:30

LucioFranco reviewed May 22, 2024

View reviewed changes

tests/integration_tests/tests/connection.rs Outdated Show resolved Hide resolved

jplatte mentioned this pull request May 23, 2024

Update gRPC multixplex example for axum 0.7 and hyper 1 tokio-rs/axum#2736

Open

1 task

alexrudy force-pushed the hyper-1.0 branch 6 times, most recently from 5b4445c to ab01356 Compare May 29, 2024 17:32

alexrudy and others added 18 commits May 29, 2024 19:04

Convert h2c examples to use http >= 1 body types

c61bf04

In h2c, when a service is receiving from hyper, it has to accept a `hyper::body::Incoming` in hyper >= 1. Additionally, response bodies must be built from `http_body_util` combinators and become BoxBody objects.

[tests] Convert MergeTrailers body wrapper in interop server

b6c900e

The Body trait has changed (removed `poll_data` and `poll_trailers`, they are now combined in `poll_frame`) and so the codec must be re-written to merge those two methods.

[tests] Convert compression tests to use hyper 1 body types

f388657

The Body trait has changed (removed `poll_data` and `poll_trailers`, they are now combined in `poll_frame`) and so the codec must be re-written to merge those two methods.

[tests] Convert complex_tower_middleware Body for hyper 1

60594e7

The Body trait has changed (removed `poll_data` and `poll_trailers`, they are now combined in `poll_frame`) and so the codec must be re-written to merge those two methods.

[tests] Convert integration_tests::origin to use http >= 1 body types

86d55af

The Body trait has changed (removed `poll_data` and `poll_trailers`, they are now combined in `poll_frame`) and so the codec must be re-written to merge those two methods.

Convert tonic-web to use http >= 1 body types

917cfe7

The Body trait has changed (removed `poll_data` and `poll_trailers`, they are now combined in `poll_frame`) and so the codec must be re-written to merge those two methods.

Convert hyper::Client to hyper_util::legacy::Client

c1cd975

`hyper::Client` has been moved to `hyper_util::legacy::Client` in version 1. Co-authored-by: Ivan Krivosheev <py.krivosheev@gmail.com> Co-authored-by: Allan Zhang <allanzhang7@gmail.com>

alexrudy force-pushed the hyper-1.0 branch from ab01356 to 3d44ca8 Compare May 29, 2024 19:04

LucioFranco reviewed May 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Hyper 1.0 & Axum 0.7 #1670

Upgrade to Hyper 1.0 & Axum 0.7 #1670

alexrudy commented Mar 31, 2024 •

edited

alexrudy commented Apr 3, 2024

karuna commented Apr 6, 2024

aumetra Apr 8, 2024

aumetra Apr 8, 2024 •

edited

alexrudy Apr 9, 2024

aumetra Apr 9, 2024

alexrudy May 26, 2024

tisonkun commented Apr 16, 2024

tisonkun commented Apr 25, 2024

hubertshelley commented May 15, 2024 •

edited

alexrudy commented May 16, 2024

djc commented May 22, 2024

LucioFranco left a comment

alexrudy commented May 23, 2024

tisonkun commented May 25, 2024 •

edited

ikrivosheev commented May 25, 2024

alexrudy commented May 29, 2024

LucioFranco May 29, 2024

alexrudy May 31, 2024

djc May 31, 2024

seanmonstar May 31, 2024

LucioFranco May 29, 2024

LucioFranco May 29, 2024

alexrudy May 31, 2024

LucioFranco May 29, 2024

LucioFranco commented May 29, 2024

Upgrade to Hyper 1.0 & Axum 0.7 #1670

Are you sure you want to change the base?

Upgrade to Hyper 1.0 & Axum 0.7 #1670

Conversation

alexrudy commented Mar 31, 2024 • edited

alexrudy commented Apr 3, 2024

karuna commented Apr 6, 2024

Choose a reason for hiding this comment

aumetra Apr 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tisonkun commented Apr 16, 2024

tisonkun commented Apr 25, 2024

hubertshelley commented May 15, 2024 • edited

alexrudy commented May 16, 2024

djc commented May 22, 2024

LucioFranco left a comment

Choose a reason for hiding this comment

alexrudy commented May 23, 2024

tisonkun commented May 25, 2024 • edited

ikrivosheev commented May 25, 2024

alexrudy commented May 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucioFranco commented May 29, 2024

alexrudy commented Mar 31, 2024 •

edited

aumetra Apr 8, 2024 •

edited

hubertshelley commented May 15, 2024 •

edited

tisonkun commented May 25, 2024 •

edited