-
Notifications
You must be signed in to change notification settings - Fork 412
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add ability to override the use of the OS DNS Resolver. #545
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.
Thanks for following up!
I left some comments for you. Let me know if you have any questions for clarification.
Also, there is one more small change that I am unable to write inline: Can you also update the docstring of make-reusable-conn-manager
to include dns-resolver
.
src/clj_http/conn_mgr.clj
Outdated
(BasicHttpClientConnectionManager. (get-managers-scheme-registry req)) | ||
(BasicHttpClientConnectionManager. (get-managers-scheme-registry req) | ||
nil nil | ||
(when dns-resolver dns-resolver)) |
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.
The when
condition is not necessary here. Pass dns-resolver
directly.
(when dns-resolver dns-resolver)) | |
dns-resolver |
This change also needs to be applied to the other uses of dns-resolver
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.
Done
src/clj_http/util.clj
Outdated
(try | ||
(GZIPInputStream. b) | ||
(catch java.io.EOFException e | ||
nil)) |
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 changes some of the error handling logic for existing users and increases the difficulty of tracking unexpected errors if the clj-http
swallows these errors. For this reason, this should be included as part of this PR.
It's very strange why this is impacting your system. If you can provide a stack trace, I can potentially provide some ideas for helping diagnose the root cause.
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 think I have a bit more context about the scenario you're running into now. If I understand correctly, you're running into this error as part of lein test :integration
. I am also seeing this issue locally.
Looks like this was introduced as part of #521. I think additional investigation is required for addressing this gzip issue that is out of the scope of this current PR.
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 just noticed that some of the HTTPS tests fail if I use openjdk 11. Oracle JDK 1.8 is ok.
This is the exception I get without the try catch.
lein test :only clj-http.test.core-test/t-empty-response-coercion
ERROR in (t-empty-response-coercion) (GZIPInputStream.java:268)
Uncaught exception, not in assertion.
expected: nil
actual: java.io.EOFException: null
at java.util.zip.GZIPInputStream.readUByte (GZIPInputStream.java:268)
java.util.zip.GZIPInputStream.readUShort (GZIPInputStream.java:258)
java.util.zip.GZIPInputStream.readHeader (GZIPInputStream.java:164)
java.util.zip.GZIPInputStream. (GZIPInputStream.java:79)
java.util.zip.GZIPInputStream. (GZIPInputStream.java:91)
clj_http.util$gunzip.invokeStatic (util.clj:45)
clj_http.util$gunzip.invoke (util.clj:41)
clojure.lang.AFn.applyToHelper (AFn.java:154)
clojure.lang.AFn.applyTo (AFn.java:144)
clojure.core$apply.invokeStatic (core.clj:667)
clojure.core$apply.invoke (core.clj:660)
clj_http.client$update.invokeStatic (client.clj:140)
clj_http.client$update.doInvoke (client.clj:139)
clojure.lang.RestFn.invoke (RestFn.java:445)
clj_http.client$eval4506$fn__4507.invoke (client.clj:379)
clojure.lang.MultiFn.invoke (MultiFn.java:229)
clj_http.client$decompression_response.invokeStatic (client.clj:406)
clj_http.client$decompression_response.invoke (client.clj:402)
clj_http.client$wrap_decompression$fn__4524.invoke (client.clj:415)
clj_http.client$wrap_input_coercion$fn__4687.invoke (client.clj:628)
clj_http.client$wrap_additional_header_parsing$fn__4712.invoke (client.clj:683)
clj_http.client$wrap_output_coercion$fn__4674.invoke (client.clj:572)
clj_http.client$wrap_exceptions$fn__4471.invoke (client.clj:249)
clj_http.client$wrap_accept$fn__4727.invoke (client.clj:726)
clj_http.client$wrap_accept_encoding$fn__4734.invoke (client.clj:748)
clj_http.client$wrap_content_type$fn__4721.invoke (client.clj:709)
clj_http.client$wrap_form_params$fn__4830.invoke (client.clj:950)
clj_http.client$wrap_nested_params$fn__4851.invoke (client.clj:984)
clj_http.client$wrap_flatten_nested_params$fn__4860.invoke (client.clj:1008)
clj_http.client$wrap_method$fn__4788.invoke (client.clj:884)
clj_http.cookies$wrap_cookies$fn__1470.invoke (cookies.clj:131)
clj_http.links$wrap_links$fn__2987.invoke (links.clj:63)
clj_http.client$wrap_unknown_host$fn__4868.invoke (client.clj:1037)
clj_http.client$request_STAR_.invokeStatic (client.clj:1165)
clj_http.client$request_STAR_.invoke (client.clj:1158)
clj_http.client$get.invokeStatic (client.clj:1171)
clj_http.client$get.doInvoke (client.clj:1167)
clojure.lang.RestFn.invoke (RestFn.java:423)
clj_http.test.core_test$fn__9861.invokeStatic (core_test.clj:717)
clj_http.test.core_test/fn (core_test.clj:711)
clojure.test$test_var$fn__9707.invoke (test.clj:717)
clojure.test$test_var.invokeStatic (test.clj:717)
clojure.test$test_var.invoke (test.clj:708)
clojure.test$test_vars$fn__9733$fn__9738.invoke (test.clj:735)
clojure.test$default_fixture.invokeStatic (test.clj:687)
clojure.test$default_fixture.invoke (test.clj:683)
clojure.test$test_vars$fn__9733.invoke (test.clj:735)
clojure.test$default_fixture.invokeStatic (test.clj:687)
clojure.test$default_fixture.invoke (test.clj:683)
clojure.test$test_vars.invokeStatic (test.clj:731)
clojure.test$test_all_vars.invokeStatic (test.clj:737)
clojure.test$test_ns.invokeStatic (test.clj:758)
clojure.test$test_ns.invoke (test.clj:743)
user$eval226$fn__287.invoke (form-init1899455601973750804.clj:1)
clojure.lang.AFn.applyToHelper (AFn.java:156)
clojure.lang.AFn.applyTo (AFn.java:144)
clojure.core$apply.invokeStatic (core.clj:667)
clojure.core$apply.invoke (core.clj:660)
leiningen.core.injected$compose_hooks$fn__156.doInvoke (form-init1899455601973750804.clj:1)
clojure.lang.RestFn.applyTo (RestFn.java:137)
clojure.core$apply.invokeStatic (core.clj:665)
clojure.core$apply.invoke (core.clj:660)
leiningen.core.injected$run_hooks.invokeStatic (form-init1899455601973750804.clj:1)
leiningen.core.injected$run_hooks.invoke (form-init1899455601973750804.clj:1)
leiningen.core.injected$prepare_for_hooks$fn__161$fn__162.doInvoke (form-init1899455601973750804.clj:1)
clojure.lang.RestFn.applyTo (RestFn.java:137)
clojure.lang.AFunction$1.doInvoke (AFunction.java:31)
clojure.lang.RestFn.invoke (RestFn.java:408)
clojure.core$map$fn__5851.invoke (core.clj:2755)
clojure.lang.LazySeq.sval (LazySeq.java:42)
clojure.lang.LazySeq.seq (LazySeq.java:51)
clojure.lang.Cons.next (Cons.java:39)
clojure.lang.RT.next (RT.java:709)
clojure.core$next__5371.invokeStatic (core.clj:64)
clojure.core$reduce1.invokeStatic (core.clj:944)
clojure.core$reduce1.invokeStatic (core.clj:934)
clojure.core$merge_with.invokeStatic (core.clj:3059)
clojure.core$merge_with.doInvoke (core.clj:3051)
clojure.lang.RestFn.applyTo (RestFn.java:139)
clojure.core$apply.invokeStatic (core.clj:667)
clojure.test$run_tests.invokeStatic (test.clj:768)
clojure.test$run_tests.doInvoke (test.clj:768)
clojure.lang.RestFn.applyTo (RestFn.java:137)
clojure.core$apply.invokeStatic (core.clj:665)
clojure.core$apply.invoke (core.clj:660)
user$eval226$fn__299$fn__332.invoke (form-init1899455601973750804.clj:1)
user$eval226$fn__299$fn__300.invoke (form-init1899455601973750804.clj:1)
user$eval226$fn__299.invoke (form-init1899455601973750804.clj:1)
user$eval226.invokeStatic (form-init1899455601973750804.clj:1)
user$eval226.invoke (form-init1899455601973750804.clj:1)
clojure.lang.Compiler.eval (Compiler.java:7176)
clojure.lang.Compiler.eval (Compiler.java:7166)
clojure.lang.Compiler.load (Compiler.java:7635)
clojure.lang.Compiler.loadFile (Compiler.java:7573)
clojure.main$load_script.invokeStatic (main.clj:452)
clojure.main$init_opt.invokeStatic (main.clj:454)
clojure.main$init_opt.invoke (main.clj:454)
clojure.main$initialize.invokeStatic (main.clj:485)
clojure.main$null_opt.invokeStatic (main.clj:519)
clojure.main$null_opt.invoke (main.clj:516)
clojure.main$main.invokeStatic (main.clj:598)
clojure.main$main.doInvoke (main.clj:561)
clojure.lang.RestFn.applyTo (RestFn.java:137)
clojure.lang.Var.applyTo (Var.java:705)
clojure.main.main (main.java:37)
test/clj_http/test/core_test.clj
Outdated
(let [resp (request {:request-method :get :uri "/get" | ||
:server-name "foo.bar.com" | ||
:dns-resolver (doto (InMemoryDnsResolver.) | ||
(.add "foo.bar.com" (into-array[(InetAddress/getByAddress (byte-array [127 0 0 1]))])))})] |
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.
Move the dns-resolver
into a separate let binding variable to make it clear what is under test.
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.
Done.
test/clj_http/test/core_test.clj
Outdated
(deftest ^:integration dns-resolver-insecure | ||
(run-server) | ||
(let [resp (request {:request-method :get :uri "/get" | ||
:server-name "foo.bar.com" | ||
:insecure true | ||
:dns-resolver (doto (InMemoryDnsResolver.) | ||
(.add "foo.bar.com" (into-array[(InetAddress/getByAddress (byte-array [127 0 0 1]))])))})] | ||
(is (= 200 (:status resp))) | ||
(is (= "get" (slurp-body resp))))) | ||
|
||
(defn custom-dns-resolver | ||
"Given a map with a string hostname key and a byte array ip address vector [10 10 22 1] {\"foobar.com\" [127 0 0 1] ...} | ||
and when the :custom-dns-resolver is added to the options use the function to override normal DNS resolution. Useful when | ||
testing when you dont have write permissions to a /etc/hosts file and you need to use TLS with SNI (server name indication). | ||
Uses the system resolver if the :server-name is not found in the supplied map." | ||
[host-map] | ||
(reify | ||
DnsResolver | ||
(^"[Ljava.net.InetAddress;" resolve [this ^String host] | ||
(if (contains? host-map host) | ||
(into-array [(InetAddress/getByAddress host (byte-array (get host-map host)))]) | ||
(.resolve (SystemDefaultDnsResolver.) host))))) | ||
|
||
(deftest ^:integration dns-resolver-custom | ||
(run-server) | ||
(let [resp (request {:request-method :get :uri "/get" | ||
:server-name "foo.bar.com" | ||
:insecure true | ||
:dns-resolver (custom-dns-resolver {"foo.bar.com" [127 0 0 1] | ||
"www.google.com" [127 0 0 1]})})] | ||
(is (= 200 (:status resp))) | ||
(is (= "get" (slurp-body resp))))) | ||
|
||
|
||
|
||
(defn ipv6-interfaces | ||
"Return # of IPv6 interfaces" | ||
[] | ||
(->> (enumeration-seq (NetworkInterface/getNetworkInterfaces)) | ||
(map #(.getInterfaceAddresses %)) | ||
(apply concat) | ||
(map #(.getAddress %)) | ||
(filter #(instance? java.net.Inet6Address %)) | ||
(count))) | ||
|
||
(deftest ^:integration dns-resolver-ipv6 | ||
(if (<= (ipv6-interfaces) 0) | ||
(println "IPv6 not enabled, skipping custom-dns-resolver-ipv6 test.") | ||
(do | ||
(run-server) | ||
(let [resp (request {:request-method :get :uri "/get" | ||
:server-name "foo.bar.ipv6" | ||
:dns-resolver (doto (InMemoryDnsResolver.) | ||
(.add "foo.bar.ipv6" (into-array[(InetAddress/getByAddress (byte-array [0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1]))])))})] | ||
(is (= 200 (:status resp))) | ||
(is (= "get" (slurp-body resp))))))) | ||
|
||
(deftest ^:integration dns-resolver-ipv6-custom | ||
(if (<= (ipv6-interfaces) 0) | ||
(println "IPv6 not enabled, skipping custom-dns-resolver-ipv6 test.") | ||
(do | ||
(run-server) | ||
(let [resp (request {:request-method :get :uri "/get" | ||
:server-name "foo.bar.ipv6" | ||
:dns-resolver (custom-dns-resolver {"foo.bar.ipv6" [0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1]})})] | ||
(is (= 200 (:status resp))) | ||
(is (= "get" (slurp-body resp))))))) | ||
|
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.
With regards to testing the contract between clj-http & apache http clients, my impression is these tests do not test any scenarios that the basic tests do not cover.
For that reason, please remove these from the PR.
test/clj_http/test/core_test.clj
Outdated
(is (= {:foo "bar"} (:body resp)))) | ||
(fn [e] (is false (str "failed with " e))))) | ||
(let [cm (conn/make-reusable-conn-manager {:dns-resolver (doto (InMemoryDnsResolver.) | ||
(.add "nonexistant.google.com" (into-array[(InetAddress/getByAddress (byte-array [127 0 0 1]))])))}) |
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.
Extract out thedns-resolver
into a top level let and reuse it in the async & sync connection manager. This will improve clarity for what is being tested.
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.
Done.
README.org
Outdated
@@ -1437,6 +1437,48 @@ things: | |||
In previous versions of =clj-http= (<= 3.10.0), =clj-http= defaulted to lazily parsing JSON, but this | |||
was slow and also confused users who didn't expect laziness. | |||
|
|||
** DNS Resolution | |||
|
|||
Users may add their own DNS resolver function to override the OS provided DNS resolver. This is useful in situations where you are unable to change the name to IP Address mapping. It is analogus to the --reslove flag present in cUrl. This example uses the org.apache.http.impl.conn.InMemoryDnsResolver to allow resolution of example.com to IP Address 127.0.0.1. |
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.
Suggested copy change with some typo fixes:
Users may add their own DNS resolver function to override the OS provided DNS resolver. This is useful in situations where you are unable to change the name to IP Address mapping. It is analogus to the --reslove flag present in cUrl. This example uses the org.apache.http.impl.conn.InMemoryDnsResolver to allow resolution of example.com to IP Address 127.0.0.1. | |
Users may add their own DNS resolver function to override the default DNS Resolver. This is useful in situations where you are unable to change the name to IP Address mapping. It is analogous to the =--resolve= flag present in =curl=. This example uses =org.apache.http.impl.conn.InMemoryDnsResolver= to resolve =example.com= to IP Address =127.0.0.1=. |
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.
Done
README.org
Outdated
(.add "example.com" (into-array[(InetAddress/getByAddress (byte-array [127 0 0 1]))])))}) | ||
#+END_SRC | ||
|
||
This option is supported for all of the connection managers. You are free to implement any DnsResolver function you like. Here is a more Clojuresq example of a DnsResolver that attempts to look up the hostname in the supplied map and if it is not found passes it on to the system DNS resolver. Note how IPV6 addresses are specified. |
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 suggest breaking the paragraph into two because it expresses two different ideas:
This option is supported for all of the connection managers. You are free to implement any DnsResolver function you like. Here is a more Clojuresq example of a DnsResolver that attempts to look up the hostname in the supplied map and if it is not found passes it on to the system DNS resolver. Note how IPV6 addresses are specified. | |
This option is supported for all of the connection managers. | |
The =dns-resolver= can be any instance of =DnsResolver=. Here is an example of a custom implementation that attempts to look up the hostname in the supplied map and falls back to the default SystemDnsResolver if not found. Note how IPV6 addresses are specified. |
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.
Done
README.org
Outdated
:trust-store-type "jks" ; default jks | ||
:trust-store-pass "trustpass" | ||
:dns-resolver (custom-dns-resolver {"example.com" [127 0 0 1] | ||
"www.google.com" [0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 1]})}) |
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.
Similar feedback from above, remove keystore
& truststore
options to focus the example on the custom dns resolver.
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.
Done.
README.org
Outdated
(defn custom-dns-resolver | ||
"Given a map with a string hostname key and a byte array ip address vector [10 10 22 1] {\"foobar.com\" [127 0 0 1] ...} | ||
and when the :custom-dns-resolver is added to the options use the function to override normal DNS resolution. Useful when | ||
testing when you dont have write permissions to a /etc/hosts file and you need to use TLS with SNI (server name indication). | ||
Uses the system resolver if the :server-name is not found in the supplied map." | ||
[host-map] | ||
(reify | ||
DnsResolver | ||
(^"[Ljava.net.InetAddress;" resolve [this ^String host] | ||
(if (contains? host-map host) | ||
(into-array [(InetAddress/getByAddress host (byte-array (get host-map host)))]) | ||
(.resolve (SystemDefaultDnsResolver.) host)))))Editing clj-http_README.org at 3.x · fhitchen_clj-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.
In this section:
As an example, it should be easy to copy/paste to try out. For that reason, the java classes should be fully qualified.
Regarding the docstring, it takes focus away from the essence of the example snippet. It also repeats the sentence above the example of what the custom resolve does. For that reason, I think the docstring should be omitted here.
Lastly, I noticed there is a bit of extra Editing clj-http_README.org at 3.x · fhitchen_clj-http
.
The if
condition can also be replaced with if-let
to be slightly more idiomatic.
(defn custom-dns-resolver | |
"Given a map with a string hostname key and a byte array ip address vector [10 10 22 1] {\"foobar.com\" [127 0 0 1] ...} | |
and when the :custom-dns-resolver is added to the options use the function to override normal DNS resolution. Useful when | |
testing when you dont have write permissions to a /etc/hosts file and you need to use TLS with SNI (server name indication). | |
Uses the system resolver if the :server-name is not found in the supplied map." | |
[host-map] | |
(reify | |
DnsResolver | |
(^"[Ljava.net.InetAddress;" resolve [this ^String host] | |
(if (contains? host-map host) | |
(into-array [(InetAddress/getByAddress host (byte-array (get host-map host)))]) | |
(.resolve (SystemDefaultDnsResolver.) host)))))Editing clj-http_README.org at 3.x · fhitchen_clj-http | |
(defn custom-dns-resolver | |
[host-map] | |
(let [system-dns-resolver (org.apache.http.impl.conn.SystemDefaultDnsResolver.)] | |
(reify | |
org.apache.http.conn.DnsResolver | |
(^"[Ljava.net.InetAddress;" resolve [this ^String host] | |
(if-let [address (get host-map host)] | |
(into-array [(java.net.InetAddress/getByAddress host (byte-array address))]) | |
(.resolve system-dns-resolver host)))))) |
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.
Done.
Oh good. I added the stack trace in the review comments. I did try to look
at it. As far as I remember, it was throwing the exception as the input
stream was nil...
I have made your changes and re-requested a review.
regards, Francis.
…On Tue, Jul 7, 2020 at 12:06 PM Raymond Huang ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/clj_http/util.clj
<#545 (comment)>:
> @@ -44,7 +44,10 @@
(when b
(cond
(instance? InputStream b)
- (GZIPInputStream. b)
+ (try
+ (GZIPInputStream. b)
+ (catch java.io.EOFException e
+ nil))
I think I have a bit more context about the scenario you're running into
now. If I understand correctly, you're running into this error as part of lein
test :integration. I am also seeing this issue locally.
Looks like this was introduced as part of #521
<#521>. I think additional
investigation is required for addressing this gzip issue that is out of the
scope of this current PR.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#545 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABML24EI5WWQFRCZPSSTNMDR2NIYHANCNFSM4OLUOMUQ>
.
|
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.
@fhitchen sorry for the delay in review 🙇
The PR looks good now 👍 .
I left a few really minor comments for you to quickly take a look at. Once you get a chance to respond to those, then I can merge this in.
All done, not quite sure what the "Bump on this comment" means, but that is probably my issue? As far as I know the code no longer looks like that as I removed the inline InMemoryDnsResolver and broke it out in to a separate let variable? When can we get the GZip error fixed in the tests? lein test :only clj-http.test.core-test/t-empty-response-coercion
ERROR in (t-empty-response-coercion) (GZIPInputStream.java:269)
Uncaught exception, not in assertion.
expected: nil
actual: java.io.EOFException: null |
@fhitchen sorry about that! It does appear that Github is not linking two comments together. I wrote that as a response to this comment: https://github.com/dakrone/clj-http/pull/545/files#r450522639 Regarding the Gzip tests, I have a branch that fixes the issue 👉 https://github.com/dakrone/clj-http/pull/545/files |
…l the system resolver.
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'm good.
Looks good, thanks for the contribution 🙌! |
Hi,
I have made the changes you suggested and put them in a new PR. The dns-resolver function is now removed from the clj-http code and users are free to provide their own DNS resolver. I have provided an example of a clojuresqe dns-resolver and one using the ugly org.apache.http.impl.conn.InMemoryDnsResolver.
Let me know what needs fixing.
I had to add the try catch to ZIP test as it fails all the time on my Ubuntu 20.04 server with OpenJDK 11 and with OracleJDK 1.8.0_252.
regards, Francis.