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

Make examples/simple.rs compatible with hyper v1 #1

Merged
merged 3 commits into from
May 19, 2024
Merged

Conversation

stefansundin
Copy link
Owner

No description provided.

Comment on lines -41 to -46
hyper-trust-dns = { version = "0.4.2", features = [
"rustls-http2",
"dnssec-ring",
"dns-over-https-rustls",
"rustls-webpki"
] }
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Motivation: hyper-trust-dns was rebranded to hyper-hickory and no longer has a HttpsConnector, so using hyper-rustls instead.

Comment on lines +31 to +32
.https_or_http()
.enable_http1()
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only enabling http1 because http2 requires that the upstream server also supports http2, since we're not doing any http2-to-http1 translation at the moment.

.build();
ReverseProxy::new(
hyper_util::client::legacy::Builder::new(TokioExecutor::new())
.pool_idle_timeout(Duration::from_secs(3))
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Motivation: The default idle timeout is 90 seconds, which is far too long for most services that you connect the reverse proxy to. My test service used 15 seconds, which means I started getting an error 15 seconds after I opened the reverse proxy.

Ideally the keep-alive header from the upstream server would be used automatically. But that can change per request, so I think it would require quite a lot of work to make this seamless. Open to ideas here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started getting an error 15 seconds after I opened the reverse proxy.

Was this an error from hyper-reverse-proxy? If so I would say the solution would be to silence that error. If upstream has decided that an idle connection should get closed that's not really an error, that's business as usual (imo).

90 seconds does initially strike me as being too long for a default, but it's what go's builtin reverse proxy uses (via the DefaultTransport.IdleConnTimeout), and they've probably thought about it more than I have.

In any case the client would ideally honor the keep alive timeout provided by the server, but that's more a concern of the hyper_util::client than of ours.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I guess I wasn't very clear on what was happening here.

The error is:

LegacyHyperError(Error { kind: SendRequest, source: Some(hyper::Error(IncompleteMessage)) })

Because the connection between the proxy and the upstream service has been closed (in my case after 15 seconds), but the connection between the client and the proxy is still open. So on the next request the proxy will attempt to use a connection that is already closed, which results in the error.

Here's a GitHub thread discussing the issue: hyperium/hyper#2136

The TL;DR seems to be that it isn't always safe to automatically retry the request.

Anyway, I thought that it might be useful to include in the example, but perhaps it requires a comment explaining the problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, maybe best to just include that link as a comment there for now 👍

Comment on lines +47 to +48
let host = req.headers().get("host").and_then(|v| v.to_str().ok());
if host.is_some_and(|host| host.starts_with("service1.localhost")) {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The motivation for using the host header over the path is because the path is forwarded to the upstream server, and most servers will return a useless 404 for the path /target/first. Meanwhile, most servers will ignore what is in the host header, and most browsers let you use subdomains on localhost now, so http://service1.localhost should work for most cases out of the box. Using starts_with since the port number will be at the end!

Copy link
Contributor

@mediocregopher mediocregopher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @stefansundin for the fixes to the example but also for the fixes to the README 🙏 🙇 I'll cherry-pick your commits now.

.build();
ReverseProxy::new(
hyper_util::client::legacy::Builder::new(TokioExecutor::new())
.pool_idle_timeout(Duration::from_secs(3))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started getting an error 15 seconds after I opened the reverse proxy.

Was this an error from hyper-reverse-proxy? If so I would say the solution would be to silence that error. If upstream has decided that an idle connection should get closed that's not really an error, that's business as usual (imo).

90 seconds does initially strike me as being too long for a default, but it's what go's builtin reverse proxy uses (via the DefaultTransport.IdleConnTimeout), and they've probably thought about it more than I have.

In any case the client would ideally honor the keep alive timeout provided by the server, but that's more a concern of the hyper_util::client than of ours.

use hyper_util::client::legacy::connect::HttpConnector;

type Connector = HttpsConnector<HttpConnector>;
type ResponseBody = UnsyncBoxBody<Bytes, std::io::Error>;

lazy_static::lazy_static! {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as you're here fixing up the example, I think it's no longer recommended to use lazy_static. From the lazy_static repo:

It is now possible to easily replicate this crate's functionality in Rust's standard library with std::sync::OnceLock.

So if anything it'd be better to use that.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, I opened #2.

@stefansundin stefansundin merged commit 2ec415e into master May 19, 2024
14 of 28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants