Proxy header handling 2 #72
Conversation
| @@ -8,6 +8,16 @@ import ( | |||
|
|
|||
| func newHTTPProxy(t *url.URL, tr http.RoundTripper) http.Handler { | |||
| rp := httputil.NewSingleHostReverseProxy(t) | |||
|
|
|||
| originalDirector := rp.Director | |||
There was a problem hiding this comment.
Can you explain better in the comment why this is necessary and which problem it solves? Also, s/originalDirector/d/
|
Looks OK. Some minor nitpicks in formatting and naming. Is there a way to squash the commits into one before submitting? I don't need/want the full history on how we got there. (used to gerrit) |
|
OK, I have done the style changes. But I got one question in mind, concerning the host header change, I made to the |
|
If I understand the host header change correctly then this behaves as follows: Given the route If that is the case than I cannot merge this since it would break our app big time. We rely on the original host header to be present. This discussion is along the same line I have with people asking for path prefix stripping. By doing this you move a central piece of configuration into the LB and you rely on this function to be there. Without that your application does not work. fabio is trying hard not to do that by not offering these options as the app should work the exactly same with and without fabio. This means that an app should handle all routes and it should also understand all host names. I understand that other LBs offer this but I think this should be solved in the app itself. Please let me know whether I understood this correctly. |
|
Also, I don't think that this behavior is "correct" since it depends on what the LB is supposed to be doing. Sure, you can rewrite the host header but if the application needs to generate links that contain the full external host name then you would have to configure that separately since it can't extract that information from the request anymore. IMO, the whole path mangling is the wrong approach. Just like NAT on the IP level. It works but it causes all kinds of problems downstream. |
|
Hi @magiconair magiconair, In my opinion it would be a bad practice, not to rewrite the host header, since this is how http usually woks in such an architecture. If an application wants to construct the url's, it has to respect the Forwading header. Every serious framework is doing so. Even without rewriting, you have to respect the headers, for the correct protocol scheme (http/https). But the world is full of opinions ;-) |
|
Lets leave it out for now. I need to think about this a bit more and TBH so far nobody has complained about it but I'd like to stick to RFCs where possible. Can you add another issue about this? Then we can have that discussion there. If I understand correctly I should be able to do the squashing in github now. Lets try that. Thank you |
* The X-Forwarded-For header is currently not set for websocket connections which is probably a regression of #72. * The X-Forwarded-Proto header should be set to ws and wss for websocket connections. * The X-Forwarded-Port header should be set to complete the X-Forwarded-* headers. * The Forwarded header should set proto to either ws or wss for websocket connections.
Here is a more clean implementation of the former pull request #67.
Please look over it and tell me, what you think about it, now.
I was unsure, if I should add a user documentation on the headers to the readme?
Maybe it's to much detail for the readme?