Skip to content

Commit

Permalink
Lift ssl-context coercion to connection-pool fn
Browse files Browse the repository at this point in the history
As suggested by @DerGuteMoritz in clj-commons#728. This fixes the issue and makes
the test added in the previous commit pass.

Keeping the `client-ssl-context` call in `http-connection` as is,
even though it might seem superfluous considering the code path taken in
the test, but `http-connection` is a public API, so we have to keep
the call (which for us is a no-op, if we ignore the repeated ALPN check)
even for our case when the protocol is https and `ssl-context` is supplied.

NOTE: This highlights a difference we are introducing here. Previously,
if we specified ssl-context, but the protocol wasn't https, we would just
ignore the ssl-context. Currently, we are coercing it ahead-of-time,
before knowing the request protocol. This could be alleviated by wrapping
the coercion in a `delay`, so it wouldn't happen until needed. Yet, given
how unlikely this scenario seems, I have doubts whether it'd be worth it.

I slightly dislike the repetition of `[:http1]` default value,
but since it serves as documentation in `http-connection`,
I decided to keep it as is rather than to extract it out.

Also, I slightly dislike the repetition of a pattern to call
`ensure-consistent-alpn-config` and then `coerce-ssl-client-context`
but it's only now in 2 places, which I think is a better alternative
than adding yet another ssl-coercion layer/wrapping function.
Obviously, we cannot just move `ensure-consistent-alpn-config` to
`ssl-client-context`, since ALPN is only for HTTP.
  • Loading branch information
PawelStroinski committed Jul 27, 2024
1 parent 3e6fda1 commit e8361bd
Showing 1 changed file with 11 additions and 2 deletions.
13 changes: 11 additions & 2 deletions src/aleph/http.clj
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,10 @@
(when (and force-h2c? (not-any? #{:http2} http-versions))
(throw (IllegalArgumentException. "force-h2c? may only be true when HTTP/2 is enabled."))))

(let [log-activity (:log-activity connection-options)
(let [{:keys [log-activity
ssl-context
http-versions]
:or {http-versions [:http1]}} connection-options
dns-options' (if-not (and (some? dns-options)
(not (or (contains? dns-options :transport)
(contains? dns-options :epoll?))))
Expand All @@ -241,7 +244,13 @@
(assoc :name-resolver (netty/dns-resolver-group dns-options'))

(some? log-activity)
(assoc :log-activity (netty/activity-logger "aleph-client" log-activity)))
(assoc :log-activity (netty/activity-logger "aleph-client" log-activity))

(some? ssl-context)
(update :ssl-context
#(-> %
(common/ensure-consistent-alpn-config http-versions)
(netty/coerce-ssl-client-context))))
p (promise)
create-pool-fn (or pool-builder-fn
flow/instrumented-pool)
Expand Down

0 comments on commit e8361bd

Please sign in to comment.