From 5545f2806b8e061260b77f71d78a7f007b01555f Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Fri, 16 Oct 2026 11:11:21 -0700 Subject: [PATCH] bdns: add metric for doh_transport_errors. This will let us add clear alerting for when Unbound is having problems, as opposed to our subscribers' authoritative servers having problems. The new counter, `doh_transport_errors`, counts things like connection refused, connection timeout, TLS errors, and so on. It does not count cases where we successfully received a DNS response but the RCODE was NXDOMAIN, REFUSED, SERVFAIL, etc. It also does not count cases where the HTTP _response_ timed out, because those are typically "authoritative server was slow" cases, and not our fault. Replace http.Client.Timeout (a whole-request timeout) with these three: - Dial timeout, hardcoded at 2s - TLS handshake timeout, hardcoded at 2s - ResponseHeaderTimeout, set to the configured dnsTimeout. Add sentinel errors in `ExchangeContext` to indicate when an HTTP request timed out or was canceled. These replace the typecasting net.OpError / url.Error cases, and make them more precise: we only want to tell the user "query timed out" when it was their fault. Change the retry logic significantly: previously we would retry only on net.Error where Timeout() was true. Now we retry on any error so long as the context is not done. Reasoning: the old code retried both on user-caused errors and _some_ internal errors. Rearranging error returns made it necessary to tweak this and I decided to go with "retry everything up to maxTries." Practical impact: a DNS request may now take at most 2s + 2s + maxTries * requestTimeout. Previously it would take at most maxTries * requestTimeout. In practice, dials are usually fast and we should not see much impact. --- bdns/dns.go | 123 ++++++++++++---- bdns/dns_test.go | 323 +++++++++++++++++++++++-------------------- bdns/problem.go | 27 +--- bdns/problem_test.go | 21 +-- va/config/config.go | 5 +- 5 files changed, 279 insertions(+), 220 deletions(-) diff --git a/bdns/dns.go b/bdns/dns.go index ae7c0aadee1..cf3880db32e 100644 --- a/bdns/dns.go +++ b/bdns/dns.go @@ -70,9 +70,10 @@ type impl struct { clk clock.Clock log blog.Logger - queryTime *prometheus.HistogramVec - totalLookupTime *prometheus.HistogramVec - timeoutCounter *prometheus.CounterVec + queryTime *prometheus.HistogramVec + totalLookupTime *prometheus.HistogramVec + timeoutCounter *prometheus.CounterVec + dohTransportErrors *prometheus.CounterVec } var _ Client = &impl{} @@ -81,7 +82,7 @@ var _ Client = &impl{} // DNS servers for resolution, and the provided tlsConfig to speak DoH to those // servers. func New( - readTimeout time.Duration, + responseTimeout time.Duration, servers ServerProvider, stats prometheus.Registerer, clk clock.Clock, @@ -90,19 +91,31 @@ func New( log blog.Logger, tlsConfig *tls.Config, ) Client { - // Clone the default transport because it comes with various settings that we - // like, which are different from the zero value of an `http.Transport`. Then - // set it to force HTTP/2, because Unbound will reject non-HTTP/2 DoH - // requests. - transport := http.DefaultTransport.(*http.Transport).Clone() - transport.TLSClientConfig = tlsConfig - transport.ForceAttemptHTTP2 = true + transport := &http.Transport{ + // Set short hardcoded timeouts for TCP connection and handshaking, since + // we expect our recursive resolver to be local. If it takes a long + // time to dial, something's wrong. + TLSHandshakeTimeout: 2 * time.Second, + DialContext: (&net.Dialer{ + Timeout: 2 * time.Second, + }).DialContext, + // Set a configurable timeout for how long to wait for an HTTP response. + // Since the HTTP response blocks on recursive resolution, which relies on + // potentially slow external servers, this will typically be larger than + // the connection timeout. + ResponseHeaderTimeout: responseTimeout, + // Force HTTP/2 because Unbound will reject non-HTTP/2 DoH requests. + ForceAttemptHTTP2: true, + MaxIdleConns: 100, + IdleConnTimeout: 90 * time.Second, + TLSClientConfig: tlsConfig, + } exchanger := &dohExchanger{ clk: clk, hc: http.Client{ - Timeout: readTimeout, Transport: transport, + Timeout: 0, }, userAgent: userAgent, } @@ -130,6 +143,12 @@ func New( }, []string{"qtype", "result", "resolver", "isTLD"}, ) + dohTransportErrors := promauto.With(stats).NewCounterVec( + prometheus.CounterOpts{ + Name: "doh_transport_errors", + Help: "Number of internal errors in DoH (e.g. connection refused, dial timeout, 400)", + }, []string{"resolver"}, + ) if maxTries < 1 { // Allowing negative or zero total attempts makes no sense, so default to 1. @@ -137,14 +156,15 @@ func New( } return &impl{ - exchanger: exchanger, - servers: servers, - maxTries: maxTries, - clk: clk, - queryTime: queryTime, - totalLookupTime: totalLookupTime, - timeoutCounter: timeoutCounter, - log: log, + exchanger: exchanger, + servers: servers, + maxTries: maxTries, + clk: clk, + queryTime: queryTime, + totalLookupTime: totalLookupTime, + timeoutCounter: timeoutCounter, + dohTransportErrors: dohTransportErrors, + log: log, } } @@ -227,12 +247,14 @@ func (c *impl) exchangeOne(ctx context.Context, hostname string, qtype uint16) ( }).Observe(rtt.Seconds()) if err != nil { + if !errors.Is(err, errDohQueryTimeout) && !errors.Is(err, errDohQueryCanceled) { + c.dohTransportErrors.With(prometheus.Labels{"resolver": chosenServerIP}).Inc() + } + c.log.Infof("logDNSError chosenServer=[%s] hostname=[%s] queryType=[%s] err=[%s]", chosenServer, hostname, qtypeStr, err) - // Check if the error is a network timeout, rather than a local context - // timeout. If it is, retry instead of giving up. - netErr, isNetError := errors.AsType[net.Error](err) - isRetryable := ctx.Err() == nil && isNetError && netErr.Timeout() + // Retry all errors up to the maxTries limit and the context deadline or cancellation. + isRetryable := ctx.Err() == nil hasRetriesLeft := tries < c.maxTries if isRetryable && hasRetriesLeft { continue @@ -243,14 +265,14 @@ func (c *impl) exchangeOne(ctx context.Context, hostname string, qtype uint16) ( "resolver": chosenServerIP, "isTLD": fmt.Sprintf("%t", !strings.Contains(hostname, ".")), }).Inc() - } else if errors.Is(err, context.DeadlineExceeded) { + } else if errors.Is(err, errDohQueryTimeout) { c.timeoutCounter.With(prometheus.Labels{ "qtype": qtypeStr, "result": "deadline exceeded", "resolver": chosenServerIP, "isTLD": fmt.Sprintf("%t", !strings.Contains(hostname, ".")), }).Inc() - } else if errors.Is(err, context.Canceled) { + } else if errors.Is(err, errDohQueryCanceled) { c.timeoutCounter.With(prometheus.Labels{ "qtype": qtypeStr, "result": "canceled", @@ -346,7 +368,22 @@ type dohExchanger struct { userAgent string } +// errDohQueryTimeout indicates an HTTP response timeout from Unbound, typically due to +// a slow authoritative server. It is also returned when the context deadline is +// exceeded during the HTTP query. +var errDohQueryTimeout = errors.New("query timed out") + +// errDohQueryCanceled indicates a context cancellation while waiting for an HTTP response. +// This doesn't indicate a communication failure with Unbound, so we don't want it to +// count as a DoH transport error. +var errDohQueryCanceled = errors.New("query timed out (and was canceled)") + // ExchangeContext sends a DoH query to the provided DoH server and returns the response. +// +// Returns `dohQueryTimeout` or `dohQueryCanceled` if the HTTP request times out or is +// canceled (which generally indicates Somebody Else's Problem). All other errors, including +// connection refused, TLS handshake failures, or dial timeouts talking to Unbound, are +// of other error types and can be considered a DoH transport error. func (d *dohExchanger) ExchangeContext(ctx context.Context, query *dns.Msg, server string) (*dns.Msg, time.Duration, error) { q, err := query.Pack() if err != nil { @@ -367,8 +404,40 @@ func (d *dohExchanger) ExchangeContext(ctx context.Context, query *dns.Msg, serv start := d.clk.Now() resp, err := d.hc.Do(req) + elapsed := d.clk.Since(start) if err != nil { - return nil, d.clk.Since(start), err + // If the context is done, we'll consider that a user-caused error. Some of these + // will actually be our fault. For instance, if a request came in with a + // short deadline (in secondary validation), we could hit the context deadline + // during Dial. In general we want Dial timeouts to be our fault, but since these + // are ambiguous we'll err on the side of "don't alert," or in other words "consider + // it a user-caused error". Most requests come in with plenty of deadline left, and + // if Unbound is not responding, those other requests will show up in the "our fault" + // bucket. + if ctx.Err() != nil { + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + return nil, elapsed, errDohQueryTimeout + } + if errors.Is(ctx.Err(), context.Canceled) { + return nil, elapsed, errDohQueryCanceled + } + } + // Hack: The most common case for a user-caused error is that we hit + // `http.Transport.ResponseHeaderTimeout`. This is what happens when we have + // successfully connected to Unbound and sent a request, but Unbound is slow + // to respond because the authoritative is slow to respond. + // + // The best way to distinguish these definitively is a string match on the error. + // https://cs.opensource.google/go/go/+/refs/tags/go1.27.2:src/net/http/internal/http2/http2.go;l=374 + // https://cs.opensource.google/go/go/+/refs/tags/go1.27.2:src/net/http/internal/http2/transport.go;l=1383-1384 + // + // Note in particular that matching on DeadlineExceeded would be wrong + // because we can get an error matching `DeadlineExceeded` if the Dial times out. + // See TestDialTimeoutError for confirmation. + if strings.Contains(err.Error(), "timeout awaiting response headers") { + return nil, elapsed, errDohQueryTimeout + } + return nil, elapsed, err } defer resp.Body.Close() diff --git a/bdns/dns_test.go b/bdns/dns_test.go index 0210b0efef4..7a7bca4dec4 100644 --- a/bdns/dns_test.go +++ b/bdns/dns_test.go @@ -10,11 +10,12 @@ import ( "log" "net" "net/http" - "net/url" + "net/http/httptest" "os" "regexp" "strings" "sync" + "syscall" "testing" "time" @@ -608,28 +609,26 @@ type testExchanger struct { var errTooManyRequests = errors.New("too many requests") -func (te *testExchanger) ExchangeContext(ctx context.Context, m *dns.Msg, a string) (*dns.Msg, time.Duration, error) { - if ctx.Err() != nil { - return nil, 0, ctx.Err() - } - +func (te *testExchanger) ExchangeContext(_ context.Context, m *dns.Msg, a string) (*dns.Msg, time.Duration, error) { te.Lock() defer te.Unlock() msg := &dns.Msg{ MsgHdr: dns.MsgHdr{Rcode: dns.RcodeSuccess}, } - if len(te.errs) <= te.count { + // Count every call, including ones past the end of errs, so that tests + // notice extra requests. + te.count++ + if te.count > len(te.errs) { return nil, 0, errTooManyRequests } - err := te.errs[te.count] - te.count++ + err := te.errs[te.count-1] return msg, 2 * time.Millisecond, err } func TestRetry(t *testing.T) { - isTimeoutErr := &url.Error{Op: "read", Err: testTimeoutError(true)} - nonTimeoutErr := &url.Error{Op: "read", Err: testTimeoutError(false)} + // transportErr represents any internal error communicating with Unbound, e.g. TLS handshake or dial failure. + transportErr := errors.New("transport error") servFailError := errors.New("DNS problem: server failure at resolver looking up TXT for example.com") timeoutFailError := errors.New("DNS problem: query timed out looking up TXT for example.com") type testCase struct { @@ -651,41 +650,6 @@ func TestRetry(t *testing.T) { expected: nil, expectedCount: 1, }, - // Immediate non-OpError, error returns immediately - { - name: "non-operror", - maxTries: 3, - te: &testExchanger{ - errs: []error{errors.New("nope")}, - }, - expected: servFailError, - expectedCount: 1, - }, - // Timeout err, then non-OpError stops at two tries - { - name: "err-then-non-operror", - maxTries: 3, - te: &testExchanger{ - errs: []error{isTimeoutErr, errors.New("nope")}, - }, - expected: servFailError, - expectedCount: 2, - }, - // Timeout error given always - { - name: "persistent-timeout-error", - maxTries: 3, - te: &testExchanger{ - errs: []error{ - isTimeoutErr, - isTimeoutErr, - isTimeoutErr, - }, - }, - expected: timeoutFailError, - expectedCount: 3, - metricsAllRetries: 1, - }, // Even with maxTries at 0, we should still let a single request go // through { @@ -697,61 +661,53 @@ func TestRetry(t *testing.T) { expected: nil, expectedCount: 1, }, - // Timeout error given just once causes two tries { - name: "single-timeout-error", + name: "transport-error-then-success", maxTries: 3, te: &testExchanger{ - errs: []error{ - isTimeoutErr, - nil, - }, + errs: []error{transportErr, nil}, }, expected: nil, expectedCount: 2, }, - // Timeout error given twice causes three tries { - name: "double-timeout-error", + name: "two-transport-errors-then-success", maxTries: 3, te: &testExchanger{ - errs: []error{ - isTimeoutErr, - isTimeoutErr, - nil, - }, + errs: []error{transportErr, transportErr, nil}, }, expected: nil, expectedCount: 3, }, - // Timeout error given thrice causes three tries and fails { - name: "triple-timeout-error", + name: "persistent-transport-error", maxTries: 3, te: &testExchanger{ - errs: []error{ - isTimeoutErr, - isTimeoutErr, - isTimeoutErr, - }, + errs: []error{transportErr, transportErr, transportErr}, }, - expected: timeoutFailError, + expected: servFailError, expectedCount: 3, metricsAllRetries: 1, }, - // timeout then non-timeout error causes two retries { - name: "timeout-nontimeout-error", + name: "response-timeout-then-success", maxTries: 3, te: &testExchanger{ - errs: []error{ - isTimeoutErr, - nonTimeoutErr, - }, + errs: []error{errDohQueryTimeout, nil}, }, - expected: servFailError, + expected: nil, expectedCount: 2, }, + { + name: "persistent-response-timeout", + maxTries: 3, + te: &testExchanger{ + errs: []error{errDohQueryTimeout, errDohQueryTimeout, errDohQueryTimeout}, + }, + expected: timeoutFailError, + expectedCount: 3, + metricsAllRetries: 1, + }, } for i, tc := range tests { @@ -763,7 +719,7 @@ func TestRetry(t *testing.T) { dr := testClient.(*impl) dr.exchanger = tc.te _, _, err = dr.LookupTXT(context.Background(), "example.com") - if err == errTooManyRequests { + if tc.te.count > len(tc.te.errs) { t.Errorf("#%d, sent more requests than the test case handles", i) } expectedErr := tc.expected @@ -792,13 +748,11 @@ func TestRetryMetrics(t *testing.T) { staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr}) test.AssertNotError(t, err, "Got error creating StaticProvider") - // This lookup should not be retried, because the error comes from the - // context itself being cancelled. It should never see the error in the - // testExchanger, because the fake exchanger (like the real http package) - // checks for cancellation before doing any work. + // This lookup should not be retried, because the context is canceled before we start. testClient := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 3, "", blog.UseMock(), tlsConfig) dr := testClient.(*impl) - dr.exchanger = &testExchanger{errs: []error{errors.New("oops")}} + te := &testExchanger{errs: []error{errDohQueryCanceled}} + dr.exchanger = te ctx, cancel := context.WithCancel(t.Context()) cancel() _, _, err = dr.LookupTXT(ctx, "example.com") @@ -806,38 +760,17 @@ func TestRetryMetrics(t *testing.T) { err.Error() != "DNS problem: query timed out (and was canceled) looking up TXT for example.com" { t.Errorf("expected %s, got %s", context.Canceled, err) } - test.AssertMetricWithLabelsEquals( - t, dr.timeoutCounter, prometheus.Labels{ - "qtype": "TXT", - "result": "canceled", - "resolver": "127.0.0.1", - }, 1) - - // Same as above, except rather than cancelling the context ourselves, we - // let the go runtime cancel it as a result of a deadline in the past. - testClient = New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 3, "", blog.UseMock(), tlsConfig) - dr = testClient.(*impl) - dr.exchanger = &testExchanger{errs: []error{errors.New("oops")}} - ctx, cancel = context.WithTimeout(t.Context(), -10*time.Hour) - defer cancel() - _, _, err = dr.LookupTXT(ctx, "example.com") - if err == nil || - err.Error() != "DNS problem: query timed out looking up TXT for example.com" { - t.Errorf("expected %s, got %s", context.DeadlineExceeded, err) + if te.count != 1 { + t.Errorf("got %d tries, want 1", te.count) } test.AssertMetricWithLabelsEquals( t, dr.timeoutCounter, prometheus.Labels{ "qtype": "TXT", - "result": "deadline exceeded", + "result": "canceled", "resolver": "127.0.0.1", }, 1) } -type testTimeoutError bool - -func (t testTimeoutError) Timeout() bool { return bool(t) } -func (t testTimeoutError) Error() string { return fmt.Sprintf("Timeout: %t", t) } - // rotateFailureExchanger is a dns.Exchange implementation that tracks a count // of the number of calls to `Exchange` for a given address in the `lookups` // map. For all addresses in the `brokenAddresses` map, a retryable error is @@ -849,7 +782,7 @@ type rotateFailureExchanger struct { } // ExchangeContext for rotateFailureExchanger tracks the `a` argument in `lookups` and -// if present in `brokenAddresses`, returns a timeout error. +// if present in `brokenAddresses`, returns an error. func (e *rotateFailureExchanger) ExchangeContext(_ context.Context, m *dns.Msg, a string) (*dns.Msg, time.Duration, error) { e.Lock() defer e.Unlock() @@ -857,10 +790,9 @@ func (e *rotateFailureExchanger) ExchangeContext(_ context.Context, m *dns.Msg, // Track that exchange was called for the given server e.lookups[a]++ - // If its a broken server, return a retryable error + // If its a broken server, return an error if e.brokenAddresses[a] { - isTimeoutErr := &url.Error{Op: "read", Err: testTimeoutError(true)} - return nil, 2 * time.Millisecond, isTimeoutErr + return nil, 2 * time.Millisecond, errors.New("broken server") } return m, 2 * time.Millisecond, nil @@ -927,47 +859,6 @@ func TestRotateServerOnErr(t *testing.T) { } -type mockTimeoutURLError struct{} - -func (m *mockTimeoutURLError) Error() string { return "whoops, oh gosh" } -func (m *mockTimeoutURLError) Timeout() bool { return true } - -type dohAlwaysRetryExchanger struct { - sync.Mutex - err error -} - -func (dohE *dohAlwaysRetryExchanger) ExchangeContext(_ context.Context, m *dns.Msg, a string) (*dns.Msg, time.Duration, error) { - dohE.Lock() - defer dohE.Unlock() - - timeoutURLerror := &url.Error{ - Op: "GET", - URL: "https://example.com", - Err: &mockTimeoutURLError{}, - } - - return nil, time.Second, timeoutURLerror -} - -func TestDOHMetric(t *testing.T) { - staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr}) - test.AssertNotError(t, err, "Got error creating StaticProvider") - - testClient := New(time.Second*11, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 0, "", blog.UseMock(), tlsConfig) - resolver := testClient.(*impl) - resolver.exchanger = &dohAlwaysRetryExchanger{err: &url.Error{Op: "read", Err: testTimeoutError(true)}} - - // Starting out, we should count 0 "out of retries" errors. - test.AssertMetricWithLabelsEquals(t, resolver.timeoutCounter, prometheus.Labels{"qtype": "None", "type": "out of retries", "resolver": "127.0.0.1", "isTLD": "false"}, 0) - - // Trigger the error. - _, _, _ = resolver.exchangeOne(context.Background(), "example.com", 0) - - // Now, we should count 1 "out of retries" errors. - test.AssertMetricWithLabelsEquals(t, resolver.timeoutCounter, prometheus.Labels{"qtype": "None", "type": "out of retries", "resolver": "127.0.0.1", "isTLD": "false"}, 1) -} - // truncatedExchanger returns a truncated (TC bit set) response with the given // Rcode. If a caller failed to check for truncation on a CAA query, it would // otherwise be fooled into trusting an incomplete set of records, potentially @@ -1003,3 +894,137 @@ func TestDNSCAATruncatedResponse(t *testing.T) { test.AssertError(t, err, "expected error for truncated CAA response, even when NXDOMAIN-shaped") test.AssertContains(t, err.Error(), "response was truncated") } + +// newDoHExchanger calls New and extracts the exchanger and transport as pointers +// so tests can tweak timeout parameters (and therefore run faster). +func newDoHExchanger(t *testing.T) (*dohExchanger, *http.Transport) { + t.Helper() + staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr}) + test.AssertNotError(t, err, "Got error creating StaticProvider") + client := New(time.Second, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 1, "", blog.UseMock(), tlsConfig).(*impl) + exchanger, ok := client.exchanger.(*dohExchanger) + if !ok { + t.Fatalf("exchanger is %T, want *dohExchanger", client.exchanger) + } + transport, ok := exchanger.hc.Transport.(*http.Transport) + if !ok { + t.Fatalf("transport is %T, want *http.Transport", exchanger.hc.Transport) + } + return exchanger, transport +} + +// TestDialTimeoutError makes every dial exceed net.Dialer.Timeout and +// confirms that this results in a DeadlineExceeded error. This is really testing +// Go's behavior, but it's useful for us to confirm Go's behavior since it +// justifies a string-matching hack in how we detect HTTP response timeouts. +func TestDialTimeoutError(t *testing.T) { + // We set up a real listener so the kernel doesn't reply with "connection refused". + // But we don't expect any requests to actually reach this. + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listening: %s", err) + } + defer ln.Close() + + exchanger, transport := newDoHExchanger(t) + transport.DialContext = (&net.Dialer{ + Timeout: 10 * time.Millisecond, + // "If Control is not nil, it is called after creating the network connection but before actually dialing." + // This lets us simulate a dial timeout. + Control: func(string, string, syscall.RawConn) error { + time.Sleep(50 * time.Millisecond) + return nil + }, + }).DialContext + + req, err := http.NewRequestWithContext(t.Context(), "POST", "https://"+ln.Addr().String()+"/dns-query", nil) + if err != nil { + t.Fatalf("creating request: %s", err) + } + _, err = exchanger.hc.Do(req) + if err == nil { + t.Errorf("got no error, want error") + } + + if !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("errors.Is(%s, context.DeadlineExceeded) = false, want true", err) + } +} + +// TestDoHExchangerTimeouts starts a real HTTP/2 server and tests timeouts +// and cancellations against it. This tests the error-matching behavior +// with an actual http.Client doing real work. +func TestDoHExchangerTimeouts(t *testing.T) { + // exchange starts a server that accepts requests but never answers them, + // sets up an exchanger and transport, sets the ResponseHeaderTimeout on + // transport, and sends a query. If cancel is non-nil, it is called once the + // server has received the request. + // + // Each call gets its own server, so a request that never reaches the + // server (e.g. because ctx expired during the dial) can't be mistaken for + // a later call's request. + exchange := func(t *testing.T, ctx context.Context, responseHeaderTimeout time.Duration, cancel context.CancelFunc) error { + t.Helper() + // The `received` channel tells the test that the server got a request. + received := make(chan *http.Request, 1) + srv := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + received <- r + select { + case <-r.Context().Done(): + case <-t.Context().Done(): // unblock waiting requests when the test returns. + } + })) + srv.EnableHTTP2 = true + srv.StartTLS() + t.Cleanup(srv.Close) + + exchanger, transport := newDoHExchanger(t) + transport.TLSClientConfig = srv.Client().Transport.(*http.Transport).TLSClientConfig.Clone() + transport.ResponseHeaderTimeout = responseHeaderTimeout + defer transport.CloseIdleConnections() + + go func() { + select { + case receivedReq := <-received: + if receivedReq.ProtoMajor != 2 { + t.Errorf("server got an HTTP/%d request, want HTTP/2", receivedReq.ProtoMajor) + } + if cancel != nil { + cancel() + } + case <-t.Context().Done(): + } + }() + + query := new(dns.Msg) + query.SetQuestion("example.com.", dns.TypeA) + + _, _, err := exchanger.ExchangeContext(ctx, query, srv.Listener.Addr().String()) + return err + } + + t.Run("response header timeout", func(t *testing.T) { + err := exchange(t, t.Context(), 50*time.Millisecond, nil) + if !errors.Is(err, errDohQueryTimeout) { + t.Errorf("got %v, want dohQueryTimeout", err) + } + }) + + t.Run("context deadline exceeded", func(t *testing.T) { + ctx, cancel := context.WithTimeout(t.Context(), 50*time.Millisecond) + defer cancel() + err := exchange(t, ctx, time.Minute, nil) + if !errors.Is(err, errDohQueryTimeout) { + t.Errorf("got %v, want dohQueryTimeout", err) + } + }) + + t.Run("context canceled", func(t *testing.T) { + ctx, cancel := context.WithCancel(t.Context()) + defer cancel() + err := exchange(t, ctx, time.Minute, cancel) + if !errors.Is(err, errDohQueryCanceled) { + t.Errorf("got %v, want dohQueryCanceled", err) + } + }) +} diff --git a/bdns/problem.go b/bdns/problem.go index 19dfe89fe9a..13e8482c746 100644 --- a/bdns/problem.go +++ b/bdns/problem.go @@ -1,11 +1,8 @@ package bdns import ( - "context" "errors" "fmt" - "net" - "net/url" "github.com/miekg/dns" ) @@ -112,25 +109,8 @@ var extendedErrorCodeToString = map[uint16]string{ func (d Error) Error() string { var detail, additional string if d.underlying != nil { - netErr, netOk := errors.AsType[*net.OpError](d.underlying) - urlErr, urlOk := errors.AsType[*url.Error](d.underlying) - if netOk { - if netErr.Timeout() { - detail = detailDNSTimeout - } else { - detail = detailDNSNetFailure - } - // Note: we check d.underlying here even though `Timeout()` does this because the call to `netErr.Timeout()` above only - // happens for `*net.OpError` underlying types! - } else if urlOk && urlErr.Timeout() { - // For DOH queries, we can get back a `*url.Error` that wraps the unexported type - // `http.httpError`. Unfortunately `http.httpError` doesn't wrap any errors (like - // context.DeadlineExceeded), we can't check for that; instead we need to call Timeout(). - detail = detailDNSTimeout - } else if errors.Is(d.underlying, context.DeadlineExceeded) { - detail = detailDNSTimeout - } else if errors.Is(d.underlying, context.Canceled) { - detail = detailCanceled + if errors.Is(d.underlying, errDohQueryTimeout) || errors.Is(d.underlying, errDohQueryCanceled) { + detail = d.underlying.Error() } else { detail = detailServerFailure } @@ -162,9 +142,6 @@ func (d Error) Error() string { return result } -const detailDNSTimeout = "query timed out" -const detailCanceled = "query timed out (and was canceled)" -const detailDNSNetFailure = "networking error" const detailServerFailure = "server failure at resolver" const detailDNSTruncated = "response was truncated" diff --git a/bdns/problem_test.go b/bdns/problem_test.go index 1c534c91425..0887572c9a2 100644 --- a/bdns/problem_test.go +++ b/bdns/problem_test.go @@ -1,10 +1,8 @@ package bdns import ( - "context" "errors" "net" - "net/url" "testing" "github.com/miekg/dns" @@ -19,15 +17,15 @@ func TestError(t *testing.T) { }{ { &Error{dns.TypeMX, "hostname", &net.OpError{Err: errors.New("some net error")}, -1, nil, false}, - "DNS problem: networking error looking up MX for hostname", + "DNS problem: server failure at resolver looking up MX for hostname", }, { &Error{dns.TypeTXT, "hostname", nil, dns.RcodeNameError, nil, false}, "DNS problem: NXDOMAIN looking up TXT for hostname - check that a DNS record exists for this domain", }, { - &Error{dns.TypeTXT, "hostname", context.DeadlineExceeded, -1, nil, false}, + &Error{dns.TypeTXT, "hostname", errDohQueryTimeout, -1, nil, false}, "DNS problem: query timed out looking up TXT for hostname", }, { - &Error{dns.TypeTXT, "hostname", context.Canceled, -1, nil, false}, + &Error{dns.TypeTXT, "hostname", errDohQueryCanceled, -1, nil, false}, "DNS problem: query timed out (and was canceled) looking up TXT for hostname", }, { &Error{dns.TypeCAA, "hostname", nil, dns.RcodeServerFailure, nil, false}, @@ -50,9 +48,6 @@ func TestError(t *testing.T) { }, { &Error{dns.TypeA, "hostname", nil, dns.RcodeFormatError, nil, false}, "DNS problem: FORMERR looking up A for hostname", - }, { - &Error{dns.TypeA, "hostname", &url.Error{Op: "GET", URL: "https://example.com/", Err: dohTimeoutError{}}, -1, nil, false}, - "DNS problem: query timed out looking up A for hostname", }, { &Error{dns.TypeCAA, "hostname", nil, dns.RcodeSuccess, nil, true}, "DNS problem: response was truncated looking up CAA for hostname", @@ -65,16 +60,6 @@ func TestError(t *testing.T) { } } -type dohTimeoutError struct{} - -func (dohTimeoutError) Error() string { - return "doh no" -} - -func (dohTimeoutError) Timeout() bool { - return true -} - func TestWrapErr(t *testing.T) { err := wrapErr(dns.TypeA, "hostname", &dns.Msg{ MsgHdr: dns.MsgHdr{Rcode: dns.RcodeSuccess}, diff --git a/va/config/config.go b/va/config/config.go index ccb8576a800..46b79d0242a 100644 --- a/va/config/config.go +++ b/va/config/config.go @@ -31,7 +31,10 @@ type Common struct { // DNSStaticResolvers is a list of DNS resolvers. Each entry must // be a host or IP and port separated by a colon. IPv6 addresses // must be enclosed in square brackets. - DNSStaticResolvers []string `validate:"required_without=DNSProvider,dive,hostname_port"` + DNSStaticResolvers []string `validate:"required_without=DNSProvider,dive,hostname_port"` + // DNSTimeout controls how long to wait for a reply from a DOH server. + // Dial time and TLS handshake time are not included in this. They are hardcoded + // separately. DNSTimeout config.Duration `validate:"required"` DNSAllowLoopbackAddresses bool