-
Notifications
You must be signed in to change notification settings - Fork 284
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
Added unix domain socket support to http.proxy #1813
Conversation
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.
Using a URL for the proxy destination makes a lot of sense! It's a welcome side-effect that this this also enables HTTPS destinations.
http/vibe/http/client.d
Outdated
@@ -1118,7 +1118,7 @@ final class HTTPClientResponse : HTTPResponse { | |||
} | |||
|
|||
/** Returns clean host string. In case of unix socket it performs urlDecode on host. */ | |||
private auto getFilteredHost(URL url) | |||
auto getFilteredHost(URL url) |
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'd leave this as package
for now, because the place here isn't really fitting for a public API.
http/vibe/http/proxy.d
Outdated
/// The destination port to forward requests to | ||
ushort destinationPort; | ||
/// The destination URL to forward requests to | ||
URL destination; |
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.
This should get some backwards-compatibility properties:
/// Scheduled for deprecation - use `destination.host` instead.
@property ushort destinationPort() const { return destination.port; }
/// ditto
@property void destinationPort(ushort port) { destination.port = port; }
/// Scheduled for deprecation - use `destination.host` instead.
@property string destinationHost() const { return destination.host; }
/// ditto
@property void destinationHost(string host) { destination.host = host; }
To make that fully work, there would also need to be a default value of URL("http", "")
for destination
.
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.
Thanks for the review. I will address the comments.
Codecov Report
@@ Coverage Diff @@
## master #1813 +/- ##
==========================================
- Coverage 37.05% 37.01% -0.04%
==========================================
Files 81 81
Lines 10885 10895 +10
==========================================
Hits 4033 4033
- Misses 6852 6862 +10
Continue to review full report at Codecov.
|
- changed the HTTPReverseProxySettings class to hold a destination URL instead of (host, port) - added reverseProxyRequest overload with URL - updated the proxy handler logic - made getFilteredHost() package level
I have addressed the comments. |
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.
Okay, thanks! Looks good to merge now.
of (host, port)