-
Notifications
You must be signed in to change notification settings - Fork 520
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
Add support for serving on Unix domain socket #478
base: master
Are you sure you want to change the base?
Conversation
I think this is reasonable in theory. Perhaps |
I was under the impression that there was now unix socket support on Windows, but indeed it appears not to work. Changes
Let me know if there's anything else you'd like to see! |
Whoops, wasn't my intention to check in the |
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 your hard work on this, and apologies for taking a while to get around to reviewing it.
[org.eclipse.jetty.util BlockingArrayQueue] | ||
[org.eclipse.jetty.util.thread ThreadPool QueuedThreadPool] | ||
[org.eclipse.jetty.util.ssl SslContextFactory$Server KeyStoreScanner] | ||
[javax.servlet AsyncContext DispatcherType AsyncEvent AsyncListener] | ||
[javax.servlet.http HttpServletRequest HttpServletResponse])) | ||
(set! *warn-on-reflection* true) |
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.
Can you remove this line, please.
@@ -130,6 +133,25 @@ | |||
(.setHost (options :host)) | |||
(.setIdleTimeout (options :max-idle-time 200000))))) | |||
|
|||
(defn- socket-connector | |||
^UnixSocketConnector [^Server server {:keys [unix-socket] :as options}] |
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.
Let's use a consistent format in this file, so:
(defn- ^UnixSocketConnector unix-socket-connector
[^Server server {:keys [unix-socket] :as options}]
Also I think unix-socket-connector
would help differentiate it from TCP sockets.
(when (->> (System/getProperty "os.name") | ||
(re-find #"(?i)^windows")) | ||
(throw (ex-info "Unix sockets not supported on windows" | ||
{:os (System/getProperty "os.name") | ||
:unix-socket unix-socket}))) |
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.
Let's factor this out into its own function (and also capitalize Windows):
(defn- check-os-socket-compatibility []
(when (->> (System/getProperty "os.name")
(re-find #"(?i)^windows"))
(throw (ex-info "Unix sockets not supported on Windows"
{:os (System/getProperty "os.name")
:unix-socket unix-socket}))))
(.deleteOnExit socket) | ||
(doto (UnixSocketConnector. | ||
server | ||
#^"[Lorg.eclipse.jetty.server.ConnectionFactory;" (into-array ConnectionFactory [http-factory])) |
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.
Can you split the line over 80 characters into two lines? Also can you fix the indentation so it's according to the style guide:
(doto (UnixSocketConnector.
server
^"[Lorg.eclipse.jetty.server.ConnectionFactory;"
(into-array ConnectionFactory [http-factory]))
(when (:http? options (or ;; default is enabled when no socket specified | ||
(not (:unix-socket options)) | ||
;; enabled with socket when host/port set | ||
(some options [:host :port]))) |
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 feels too complex, and isn't what the documentation says. I think let's keep it simple and just leave it defaulting to true. Let's add a note to the :unix-socket
option instead to tell people they may want to set :http?
to false.
:ssl? - allow connections over HTTPS | ||
:ssl-port - the SSL port to listen on (defaults to 443, implies | ||
:ssl? is true) | ||
:unix-socket - File to be used as a Unix domain socket; will be | ||
passed to [io/file]. Ensure there is no file at |
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 don't think [io/file]
should be a link, as there's nothing to link it to.
IOException | ||
File] |
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.
These two lines should be one.
(with-server hello-world {:unix-socket sock} | ||
(is (= (-> (HttpClient/create) | ||
(.remoteAddress | ||
(reify Supplier |
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.
Indentation looks off here.
"Hello World") | ||
"able to serve basic request") | ||
(is (thrown-with-msg? | ||
clojure.lang.ExceptionInfo #"File already exists" |
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.
Indentation looks off. Please see Clojure style guide.
(when (.exists socket) | ||
(throw (ex-info "File already exists at socket path; should be deleted before starting server" | ||
{:unix-socket unix-socket}))) |
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.
Let's factor this out as well:
(defn- check-socket-file-does-not-exist [socket]
(when (.exists socket)
(throw (ex-info "Could not create socket file: file already exists"
{:socket-file (str socket)}))))
0d7f29a
to
0103527
Compare
It seems that |
Usage of the new APIs is documented here: |
Summary
These changes take advantage of
jetty-unixsocket
to allow serving on a socket, as in the following example:## hostname is still required for making requests curl --unix-socket /run/someapp.sock http://my-expected-hostname/url-path
Rationale
Unix sockets are simpler to implement access controls to than TCP sockets, as you can simply use file permissions. They are supported by common reverse proxies such as Apache and NGINX, and are thus reasonable for exposing a service through a proxy that may be handling authentication, without making that service available to all users on the host machine.
Possible enhancements:
:http?
default false when:socket
is set, unless:host
or:port
are explicitly configured.jetty-unixsocket
library. It is not an overwhelmingly heavy dependency, though.Note: it has proved nontrivial to get a test working, as clj-http does not support sockets, and the Clojure implementations that I've found do not seem to work as intended out of the box. I have verified that this implementation is operational manually, and I am willing to work through the process of getting a test working as long as you're interested in incorporating this functionality.