-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: Update reverse proxy example to support both http and https #2696
base: main
Are you sure you want to change the base?
feat: Update reverse proxy example to support both http and https #2696
Conversation
examples/reverse-proxy/src/main.rs
Outdated
//? Remove incorrect header host, hyper will add automatically for you. | ||
req.headers_mut().remove(HOST).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the question mark in front here? Personally I don't know if this is the right behavior here, @yanns can you comment on it since you approved (in addition to @afifurrohman-id)?
Even if we keep it I don't think there's any reason to .unwrap()
there though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When proxing a HTTP request, the initial HTTP request contains as HOST
the address of the proxy (locahost:4000
). If we use that value against another server, that server will refuse the HTTP request as it expects localhost:3000
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this was previously broken? Also, should the header value be added as some (de-facto) standard proxied-host header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this was previously broken?
I guess yes. I have not checked. I've built several proxy and this is one of the usual issues.
Also, should the header value be added as some (de-facto) standard proxied-host header?
If we want to go in the direction of "good behaving" proxy, yes they are several headers that should be set:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried the original example and it works.
The host
header would be an issue if the two servers were running externally visible on the same port as virtual servers with nginx or something else in front of them that would forward the requests based on the host
header.
axum
, hyper
, or anything else in the stack does not check the header, because it can't even know what to check it for. The request just arrives either on localhost:4000
or localhost:3000
and then it is processed.
Personally, I think that handling of this and the addition of an external dependency on example.com
is a bit sad and it would be better to just add an axum tls server (based on any of the other examples) and call that instead.
But even with example.com
, the header change is not really needed as curl -v https://example.com -H 'host: localhost:3000'
returns a 404 -- it's not the same page you get if you let curl use the correct host
(and it might be returned by a reverse proxy and not the actual server), but it is a response to an https
request so it does show that this works.
Generally, I think the examples should be as simple as possible to showcase the thing they mean to (here that's relaying a request, potentially with a change to the protocol), but details such as extension headers can and should be omitted for brevity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the question mark in front here?
I just for clarify why we remove HOST
header, as @yanns say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to go in the direction of "good behaving" proxy, yes they are several headers that should be set:
I also think like so, we need add it as well, like some other reverse proxy, it will also make more great example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, I think that handling of this and the addition of an external dependency on
example.com
is a bit sad and it would be better to just add an axum tls server (based on any of the other examples) and call that instead.
Yes, first because it think to simple as possible use an example.com
can add more precisely how we can interact with external service, but will add some external network request as well.
However, using other examples like tls-rustls
add another complexity because will required to handle self-signed certificate
…d-Host`, `X-Forwarded-Proto`, `Forwarded`)
Motivation
Example for reverse proxy currently only using http, but sometimes we want to proxy https server as well.
Solution
Update existing example and added it.