-
Notifications
You must be signed in to change notification settings - Fork 136
Listen on unix sockets #618
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
base: main
Are you sure you want to change the base?
Conversation
f041fc7 to
18e056b
Compare
1c93488 to
b2e4405
Compare
for more information, see https://pre-commit.ci
|
Not seeing these test failures locally -- advice very welcome! |
| }); | ||
|
|
||
| it("unix socket HTTP request", function (done) { | ||
| http |
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 http.request because I couldn't get the fetch API to work with a unix socket.
minrk
left a comment
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.
Nice! Some suggestions about how to handle the mutual exclusivity using commander's conflicts to implement the mutual exclusivity, rather than unconditional warnings, but otherwise looks great!
|
|
||
| if (args.socket) { | ||
| listen.proxyTarget = [args.socket]; | ||
| logger.warn( |
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.
Probably best to warn only if conflicting args are specified. It's a bit confusing to get a warning when everything is behaving as intended.
I think commander can help with this by adding .conflicts() to express these conflicts: https://github.com/tj/commander.js?tab=readme-ov-file#more-configuration
|
|
||
| if (args.apiSocket) { | ||
| listen.apiSocket = [args.apiSocket]; | ||
| logger.warn( |
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.
same here
|
|
||
| if (args.metricsSocket) { | ||
| listen.metricsSocket = [args.metricsSocket]; | ||
| logger.warn( |
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.
and here
Where #592 introduced the ability to proxy to servers running on a unix domain socket, this PR allows the proxy itself to listen on a unix socket.
--socket,--api-socket, and--metrics-socketto provide socket path for the proxy, api, and metrics servers.setupProxytest helper method to allow passing in a unix socket instead of a TCP port for test servers to listen on.Using the changes in this PR, we can securely run JupyterHub on a shared server with multiple users, using a proxy auth strategy.