Add X-Forwarded-Prefix header to reverse proxies #13
Conversation
So the proxied app can know what path it's proxied at. This will allow it to do things like fix URLs in REST API responses.
1 similar comment
|
I'm pretty proud of that coverage increase. |
| @@ -135,6 +135,10 @@ def proxy(self): | |||
| headers['X-Forwarded-Host'] = self.request.get_host() | |||
| headers['X-Forwarded-For'] = self.xff | |||
|
|
|||
| for prefix, url in self.reverse_urls: | |||
| if url == self.proxy_url and self.request.path.startswith(prefix): | |||
There was a problem hiding this comment.
maybe this check is too strict
There was a problem hiding this comment.
I think it should be self.proxy_url.startswith(url), not url == self.proxy_url:
Consider the proxy generated by the following call to generate_routes:
generate_routes({
'service_one': {
'base_url': 'https://yahoo.com/',
'prefix': '/yahoo/'
}
})If I go to /yahoo/foo/bar, then proxy_url will be https://yahoo.com/foo/bar/, and won't match https://yahoo.com/.
The definition for proxy_url: https://github.com/yola/djproxy/blob/x_forwarded_prefix/djproxy/views.py#L30-L32
That attribute is used primarily to calculate what URL to hit when the request is sent upstream.
There was a problem hiding this comment.
So, the behavior you'd see with this implemented as is:
Going to /yahoo/ would be good. The prefix would be there. Going to /yahoo/yay/ would be broken, the prefix wouldn't be there.
There was a problem hiding this comment.
And that explains why my test was passing but my real use locally failed. Thanks!
On Mon, Feb 2, 2015 at 4:13 AM, Thomas Welfley notifications@github.com
wrote:
In djproxy/views.py
#13 (comment):@@ -135,6 +135,10 @@ def proxy(self):
headers['X-Forwarded-Host'] = self.request.get_host()
headers['X-Forwarded-For'] = self.xff
for prefix, url in self.reverse_urls:if url == self.proxy_url and self.request.path.startswith(prefix):So, the behavior you'd see with this implemented as is:
Going to /yahoo/ would be good. The prefix would be there. Going to
/yahoo/yay/ would be broken, the prefix wouldn't be there.—
Reply to this email directly or view it on GitHub
https://github.com/thomasw/djproxy/pull/13/files#r23913254.
There was a problem hiding this comment.
Nevermind, proxy_url doesn't seem to include the path. That's why I'm checking with ==. So I think this is working. I need to take another look at my look at my local application. I think the problem may be there and not in djproxy.
There was a problem hiding this comment.
This still passes: 6972e84
There was a problem hiding this comment.
proxy_url requires information from the route to work correctly. It can't just take the path from the request and mash it onto the upstream base url. It needs to know what subset of the request path to mash onto the url (which is typically achieved with a named pattern).
In your test, proxy_url will always equal the base url unless you specific a url kwarg to the view. The url kwarg is appended to the base_url
There was a problem hiding this comment.
Yep, I'm dumb. What you described was the exact problem.
|
Since this is a non-standard header, this shouldn't be added to djproxy itself. I currently have an implementation that adds support for proxy middleware which enable arbitrary additions/deletions to headers and much more: https://github.com/thomasw/djproxy/compare/middleware?expand=1#diff-5707531655fb014a093e0475c9472d64R32 My branch also pulls a bunch of stuff that shouldn't have lived in the HttpProxy view into its own classes. Good news: The existing test suite passes after the updates I've made. Bad news: I don't want to ship that just yet because the complexity of the test suite can be significantly reduced. |
If foo.com is proxied at bar.com/foo, before this change, requests to bar.com/foo/baz would not send the header.
|
d79369a fixes the issues I was having.
I can resubmit this after it ships (if you could comment on this issue when that point comes it would be great). In the meantime, we can use our fork. I'll close this pull. |
|
I'll let you know. Hopefully I will have something out today. |
|
2.0.0 has shipped. |
So the proxied app can know what path it's proxied at. This will allow it to do things like fix URLs in REST API responses.
@thomasw I thought this would work, and the test does pass, but when I install this to my local application that's using
generate_routes, it's not sending the header. Any ideas?