Support hot-reload of certificate

[jimpil]

Hi there,

These days everybody uses LetsEncrypt/certbot (or something similar), for short and auto-renewed certificates. Jetty actually supports this via the sslContextFactory.reload method, but as things stand, it's really awkward to get hold of the sslContextFactory, and the fact that ssl-context-factory is private, doesn't help either. So really the only option is for the library to handle the hot-reload. It could look like this:

• new top-level option - e.g. ssl-hot-reload? defaults to false
• If that option is true, create-server will set up a FileWatcher before returning (see further down for sample Java code)

Path keystorePath = Paths.get(URI.create(sslContextFactory.getKeyStorePath()));
FileWatcher.onFileChange(keystorePath, () -> 
        sslContextFactory.reload(scf -> log.info("Reloaded SSL cert")));

Alternatively, the new option could be some ssl-context configurator-fn, so all the above could live in user space. Any thoughts?

[EDIT]

Hmm, on second thought, it may be worth considering always setting up the FileWatcher - i.e. no new option. Or perhaps have an option to disable it...

[sunng87]

Both approach sounds good to me. But I think we can implementation the ssl-hot-reload? first and to see if there is use-case that requires a full access to ssl-context-factory as the second provides.

[jimpil]

Here is how the file-watching stuff might look like:

(defn on-file-change!
  "Sets up a WatchService, and registers the parent of <target> with it for changes.
   A separate thread constantly polls for events, and when the affected file matches
   <target>, calls <on-change> on it. Returns a cancellable Future."
  [^File target on-change!]
  {:pre [(.exists target)]}
  (let [watch-service   (-> (FileSystems/getDefault) .newWatchService)
        target-absolute (.getAbsoluteFile target)
        target-path     (-> (.toPath target-absolute) .getFileName)]
    ;; register the parent directory with the watch-service
    (-> target-absolute
        .getParent
        (Paths/get (into-array String []))
        (.register watch-service (into-array [StandardWatchEventKinds/ENTRY_MODIFY])))
    ;; start event-polling thread
    (.submit
      Agent/soloExecutor ^Runnable
      (fn []
        (while (not (.isInterrupted (Thread/currentThread)))
          (let [wk (.take watch-service)] ;; blocking call
            (run!
              (fn [^WatchEvent e]
                (let [affected-path (.context e)]
                  (when (= affected-path target-path)
                    ;; only interested in changes in 
                    ;; one file (renaming NOT included)
                    (on-change! target))))
              (.pollEvents wk))
            (.reset wk)))
        (.close watch-service)))))

Obviously, when the server is stopped, the returned Future should be cancelled (e.g. via future-cancel). hope that helps 👍

[jimpil]

I updated my last comment one last time with an implementation that actually works (didn't have the time to test it last night).

Now, I understand that it is going to be rather difficult for you to ensure that the returned Future is cancelled when the server is stopped, because stop-server is exposed as a completely independent function. If I'm honest that always kind of bothered me , but I couldn't think of a concrete use case to open a ticket for. Well, now we have one...

Here is the thing, the function that stops the server, should be returned from the function that creates the server - not defined independently. If you must return the Server object from run-jetty (perhaps you have reasons other than to be able to pass it to stop-server), you could wrap everything in a map - for example:

{:server server
 :stop-server (fn [] (.stop server) (future-cancel file-watcher-loop)) ;; now you control this
...} ;; more stuff?

Now, presumbaly you don't want to make breaking changes. I completely understand and respect that, but it does kind of complicate things. I was able to come up with the following viable approach:

The important thing to realise is that, stop-server is the singular consumer of what run-jetty returns. In other words, as long as stop-server can consume what(ever) run-jetty returns, you could argue that you're not breaking anyone - except perhaps those who use Java interop directly on the Server object.

If you agree with the above, then the implementation becomes trivial:

(defn run-jetty []) ;; returns stopping-fn
(defn stop-server [stop!] (stop!)) ;; could even do an `instance?` check in here

Let me know of your thoughts...

[sunng87]

The important thing to realise is that, stop-server is the singular consumer of what run-jetty returns. In other words, as long as stop-server can consume what(ever) run-jetty returns, you could argue that you're not breaking anyone - except perhaps those who use Java interop directly on the Server object.

Yes I totally agree. But I would suggest to wrap things in a map as your first block of code. This allows further access to Server in case that user wants customizations on it. The stop-server function uses the stop-fn from the map to stop the server, cancel the future, etc.

Can you send a pull request for this?

[jimpil]

Ok, I'll try to steal away some time this weekend 👍

[jimpil]

@sunng87 It turns out this can implemented in a much less invasive way - see sample code below:

...
(let [...
        ssl-factory (delay (ssl-context-factory options)) ;; lazy load this (if needed)
        ssl-hot-reload-future (delay
                                (let [callback (or ssl-hot-reload-callback noop) ;; this is optional so provide a default
                                      ^SslContextFactory factory @ssl-factory]
                                  (some-> (.getKeyStorePath factory)
                                          URI.
                                          io/file
                                          (on-file-change!
                                            (fn [_] ;; the file above
                                              (->> (reify Consumer (accept [_ scf] (callback scf)))
                                                   (.reload factory)))))))
        server (cond-> (doto (Server. ^ThreadPool pool)
                         (.addBean (ScheduledExecutorScheduler.))
                         (.addBean (proxy [AbstractLifeCycle] []
                                     (doStart [] (when-some [f (:lifecycle-start options)] (f)))
                                     (doStop  [] (when-some [f (:lifecycle-end options)] (f)))))
                         (.setStopAtShutdown true))
                       ;; https://github.com/sunng87/ring-jetty9-adapter/issues/90
                       (and ssl?                        ;; ssl is enabled and
                            (or ssl-hot-reload-callback ;; we either have a callback
                                (not (false? ssl-hot-reload?))))
                       (doto (.addBean (proxy [AbstractLifeCycle] []
                                     (doStart [] @ssl-hot-reload-future)
                                     (doStop  [] (some-> @ssl-hot-reload-future future-cancel))))))
        connectors ...]
    (doto server
      (.setConnectors (into-array Connector connectors))))

I do apologise for this - I made it sound like there was no other way :( Personally, I still prefer returning the stop-fn from run-jetty, but even if we were to keep that, the above implementation is much cleaner than the current one - it's all contained in a single fn, less destructuring, less manual cleanuo etc etc.

Let me know of your thoughts...

[sunng87]

Agreed. This seems better and is how jetty's api designed to deal with our scenario.

I didn't realize that many users use return value of run-jetty because that's what original ring-jetty-adapter gave. I suggest to revert our change on revert type and create a new minor-version release. WDYT?

[jimpil]

This can be further improved by using jetty's own KeyStoreScanner - see #96