Enable DoT connection reuse while requests are in flight #269
Conversation
… while requests are in flight
|
This change won't work properly when you process multiple queries through a single upstream. The idea is correct, though. We should allow processing multiple queries in parallel through a single TCP/TLS connection. |
Good point. We thought that this was already handled since it's required by the DoT RFC (and the implementation requirements RFC):
We did some additional testing, and can confirm that out of order responses result in an id mismatch, hence closing the connection. We will develop a proper fix and update this PR. |
When processing multiple queries through a single DoT upstream, we might get a DNS ID mismatch if responses are received out of order (cf. PR AdguardTeam#269). We fix this by storing all responses with unknown IDs in a map from which they can be retrieved later on.
… query In order to deal with response reordering, RFC 7766 [1] requires that we match the QNAME, QCLASS, and QTYPE fields in the response to the query. [1] https://www.rfc-editor.org/rfc/rfc7766#section-7
DoT inflight connection reuse with handling for out-of-order responses as specified by RFC 7766
|
We just updated this PR with a fix for out-of-order responses, following the requirements the DoT RFC (and the implementation requirements RFC):
We now store all responses with unknown IDs in a map from which they can be retrieved later on, and we explicitly check the QNAME, QCLASS, and QTYPE fields in the responses. |
| @@ -53,14 +53,16 @@ func (p *dnsOverTLS) Exchange(m *dns.Msg) (reply *dns.Msg, err error) { | |||
| } | |||
|
|
|||
| p.RLock() | |||
| poolConn, err := p.pool.Get() | |||
| poolConnAndStore, err := p.pool.Get() | |||
| // Put the connection right back in to allow the connection to be reused while requests are in flight | |||
There was a problem hiding this comment.
This is generally the same as using a single connection and not using pool at all. Why keeping it then?
There was a problem hiding this comment.
Getting rid of the connections pool and using a single connection is possible, but it does have visible downsides. Namely, head-of-line blocking will be a huge problem when the connection is not stable.
However, if we operate locally (for instance, dnsproxy proxies a recursor on the same server) using pipelining is in theory better.
Here's what I suggest:
- Try implementing a different type of upstream that uses a single connection (in a separate file).
- Write tests for it and check that pipelining actually works and that it keeps that single connection alive.
- IMPORTANT: we need to be ready that the server can close the connection any time. In this case we should first try re-establishing the connection and repeating the request.
Then I'll examine the implementation and if it's okay we can merge it with existing DoT and DNS-over-TCP upstreams and allow enabling pipelining mode via upstream.Options.
Currently, a DoT connection is put back into the pool when the DNS response is received (or an error occurs). Hence, a new DoT connection is established if a second request should be served while a first request is in flight. While this behaviour effectively counters TCP HoLB, it is more beneficial to reuse the already established connection for the second request, since the overhead of the DoT connection establishment is pretty high (1 RTT for TCP, 0-2 RTTs for TLS (depending on TLS version, session resumption, and 0-RTT)).
This PR puts the connection right back into the pool to allow it to be reused while requests are in flight. The behaviour is now identical to DNS over HTTPS/2 (DoH2), where a single DoH2 connection is also used when multiple requests are in flight.
Attached you will find 2 pcaps showcasing the behaviour before and after the change: dot.inflight.reuse.pcaps.zip