Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Breaking changes] Update hyper from 0.14 to 1.0 #23

Closed
junkurihara opened this issue Mar 23, 2023 · 14 comments · Fixed by #115
Closed

[Breaking changes] Update hyper from 0.14 to 1.0 #23

junkurihara opened this issue Mar 23, 2023 · 14 comments · Fixed by #115
Labels
dependencies Pull requests that update a dependency file help wanted Extra attention is needed

Comments

@junkurihara
Copy link
Owner

rust-rpxy currently depends on hyper of version 0.14.x. hyper project is planning to release version 1.0.x shortly and already released 1.0.0-rc.x.

The version up involves a lot of breaking changes, so we need to rethink the architecture of rpxy to support the brand new version of hyper.

@junkurihara junkurihara changed the title Change Hyper from 0.14 to 1.0 Update Hyper from 0.14 to 1.0 Mar 23, 2023
@junkurihara junkurihara added the dependencies Pull requests that update a dependency file label May 19, 2023
@junkurihara junkurihara added the help wanted Extra attention is needed label Nov 17, 2023
@junkurihara
Copy link
Owner Author

hyper 1.0 was published yesterday with tons of drastic changes of its APIs. In my different project with hyper, I already tested the migration flow from 0.1.4 to 1.0, and the migrated project works flawlessly at this point.

However, in rpxy, it takes time for the migration since the structure is more complicated. Also, we should wait for upgrades of the hyper-rustls with hyper-1.0. or change our tls connector to backend servers to hyper-tls that uses native tls module.

We also should use hyper-util to handle both http/1.1. and http/2 connections, and have to change handling of h3 connections as well.

@junkurihara junkurihara changed the title Update Hyper from 0.14 to 1.0 [Breaking changes] Update hyper from 0.14 to 1.0 Nov 17, 2023
@junkurihara junkurihara pinned this issue Nov 17, 2023
@junkurihara junkurihara mentioned this issue Nov 18, 2023
5 tasks
@paulocoghi
Copy link

Also, we should wait for upgrades of the hyper-rustls with hyper-1.0. or change our tls connector to backend servers to hyper-tls that uses native tls module.

Wait for upgrades of the hyper-rustls with hyper-1.0 seems to be the better option here.

@junkurihara
Copy link
Owner Author

Hi Paulo,

Yes, I agree with you. It makes sense.
Also a draft pull-request was just submitted to its repo, and we hope it will be merged to the repo.

@junkurihara
Copy link
Owner Author

I am preparing to release 0.7.0-alpha.0 with support of native-tls instead of rustls for connection with backend applications. In my environment, native-tls works flawlessly at this point.

I will, of course, support again rustls backend connection as the default feature when hyper-rustls with hyper-1.0 is out.

@junkurihara
Copy link
Owner Author

junkurihara commented Dec 15, 2023

Currently we have a problem on websocket connection: it always says:

upgrade expected but low level API in use

to poll the Upgraded from OnUpgrade in response..

This might relate to hyperium/hyper#1752. However, our code uses hyper::upgrade::on and stream io implementing Sync...

The problem happens here:

let res_on_upgrade = hyper::upgrade::on(&mut res_backend);
self.globals.runtime_handle.spawn(async move {
let mut response_upgraded = TokioIo::new(res_on_upgrade.await.map_err(|e| {
error!("Failed to upgrade response: {}", e);
RpxyError::FailedToUpgradeResponse(e.to_string())
})?);

@paulocoghi
Copy link

I believe that, for most of the users, it doesn't matter so much the internal implementations.

What I'm trying to say is that, if you believe that using only native-tls for now is good enough, it is good for the users as well.

@junkurihara
Copy link
Owner Author

Hi Paulo,

Yes I totally agree with you. Also I am not sure that how many users needs TLS connection to backend applications (I don't use it lol)

However, from dev side, I think we should make rustls default since native-tls doesn't support TLSv1.3.

@junkurihara
Copy link
Owner Author

junkurihara commented Dec 15, 2023

Currently we have a problem on websocket connection: it always says:

upgrade expected but low level API in use

to poll the Upgraded from OnUpgrade in response..

This might relate to hyperium/hyper#1752. However, our code uses hyper::upgrade::on and stream io implementing Sync...

The problem happens here:

let res_on_upgrade = hyper::upgrade::on(&mut res_backend);
self.globals.runtime_handle.spawn(async move {
let mut response_upgraded = TokioIo::new(res_on_upgrade.await.map_err(|e| {
error!("Failed to upgrade response: {}", e);
RpxyError::FailedToUpgradeResponse(e.to_string())
})?);

That's a bug which was just fixed :-) hyperium/hyper-util@1427d1b

@paulocoghi
Copy link

That's a bug which was just fixed :-)

Good news!

@junkurihara
Copy link
Owner Author

junkurihara commented Jan 14, 2024

hyper-rustls v0.26 supporting hyper v1.0 was just integrated with rpxy #137 . Yet rustls v0.22 is not supported, I believe hyper v1.0 integration was almost done!

@junkurihara
Copy link
Owner Author

junkurihara commented Jan 22, 2024

Should we release 0.7.0 without waiting for releases of new quinn and s2n-quic supporting rustls-0.22?

@paulocoghi
Copy link

For our usage scenario, we can wait.

@junkurihara
Copy link
Owner Author

Recently I've checked the code of rustls-0.22 and s2n-quic-rustls based on the previous version of rustls. As long as I read their code, IMHO, quite huge time is required for s2n-quic developers to update s2n-quic-rustls using the newest version of rustls.

So I think I should release rpxy-0.7 without waiting for rustls-0.22 and its related updates. I will do that shortly after some load-tests with my servers.

@junkurihara junkurihara mentioned this issue Feb 15, 2024
@junkurihara junkurihara unpinned this issue Feb 15, 2024
@paulocoghi
Copy link

I will do that shortly after some load-tests with my servers.

I can't wait for the results!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants