WIP: DNS-over-HTTP support for Client

[tmthrgd]

This is a quick attempt at adding DNS-over-HTTP support to Client, inspired by #647 (comment).

This method uses http.Client and stubs the behaviour of the net.Conn present in Conn, it should be otherwise fully functional. It's entirely possible to build this same system from a raw *tls.Conn using the x/net/http2 package, but it's not clear to me how much benefit this would have.

Note: When using DNS-over-HTTPS, errors won't be surfaced from Dial, instead they'll show up in Write.

[tmthrgd]

I'm not convinced this timeout code is remotely correct and I'd rather rip it out, although I'm not sure if there's any other place it can be inserted.

[tmthrgd]

It would be useful to allow the http.Client to be specified, perhaps by adding a field to Conn.

[tmthrgd]

I'm pretty sure this is a false positive, but then again I've left (*dohConn).Close a stub.

[tmthrgd]

It would also be possible to simply push this upwards a layer and only support DNS-over-HTTPS in the various Exchange methods.

[codecov-io]

Codecov Report

Merging #649 into master will increase coverage by 0.03%.
The diff coverage is 46.29%.

@@            Coverage Diff             @@
##           master     #649      +/-   ##
==========================================
+ Coverage   57.91%   57.94%   +0.03%     
==========================================
  Files          37       37              
  Lines        9991    10045      +54     
==========================================
+ Hits         5786     5821      +35     
- Misses       3155     3172      +17     
- Partials     1050     1052       +2

Impacted Files	Coverage Δ
client.go	58.17% <46.29%> (-1.68%)	⬇️
msg.go	78.6% <0%> (+1.29%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b169d1...67ba6d6. Read the comment docs.

[miekg]

[ Quoting <notifications@github.com> in "Re: [miekg/dns] WIP: DNS-over-HTTP ..." ]

It would also be possible to simply push this upwards a layer and only support DNS-over-HTTPS in the various `Exchange` methods.

I have some misgivings on supporting a transport in only some methods. A quite like the hackiness of the approach you take :) But doing the actual Dial inside the WriteMsg method is icky. Not sure if this will fit this library ever. Maybe it's time for the big v2 rewrite

[tmthrgd]

"I have some misgivings on supporting a transport in only some methods."

That's definitely understandable, although it would be the cleanest place to support this. I'd suggest any v2 rewrite (as you mentioned later) should probably only support Exchange style methods and drop the lower-level Read and Write methods.

"A quite like the hackiness of the approach you take :) But doing the actual Dial inside the WriteMsg method is icky."

It works 🤷‍♂️. I can try a different implementation that came to mind, that would move the Dial into Dial. I don't think it would be cleaner than this implementation though.

"Not sure if this will fit this library ever."

I think DOH support would be worthwhile, but it obviously depends on how cleanly it can be integrated—baring a rewrite.

[miekg]

[ Quoting <notifications@github.com> in "Re: [miekg/dns] WIP: DNS-over-HTTP ..." ]

> "I have some misgivings on supporting a transport in only some methods." That's definitely understandable, although it would be the cleanest place to support this. I'd suggest any v2 rewrite (as you mentioned later) should probably only support `Exchange` style methods and drop the lower-level `Read` and `Write` methods.

Back in the day Exchange *was* the only thing, then stuff hapenend and PRs got merged. If you have connection pooling in a http.Transport like thing, Exchange will be enough and you don't need to expose the lower level bits.

> "Not sure if this will fit this library ever." I think DOH support would be worthwhile, but it obviously depends on how cleanly it can be integrated—baring a rewrite.

True. I would be nice if this worked; even though the implementation is not the cleanest. For any rewrite I would like to retain the API as much as possible. Also worth looking what's happening in x/dns (the new upstream "official" dns package) and integrate build on that - that said I don't have a whole lot of time to start doing that.