Skip to content

Commit f02b35c

Browse files
aarongableaarongable
committed
Review comments
1 parent fac947a commit f02b35c

File tree

4 files changed

+78
-93
lines changed

4 files changed

+78
-93
lines changed

‎bdns/dns.go‎

Lines changed: 62 additions & 70 deletions
Original file line numberDiff line numberDiff line change
@@ -168,22 +168,23 @@ func (c *impl) exchangeOne(ctx context.Context, hostname string, qtype uint16) (
168168
// present.
169169
req.SetEdns0(4096, false)
170170

171-
var resp *dns.Msg
172-
173171
servers, err := c.servers.Addrs()
174172
if err != nil {
175173
return nil, "", fmt.Errorf("failed to list DNS servers: %w", err)
176174
}
177175

178176
// Prepare to increment a latency metric no matter whether we succeed or fail.
179-
// The deferred function closes over result, chosenServerIP, and tries, which
177+
// The deferred function closes over resp, chosenServerIP, and tries, which
180178
// are all modified in the loop below.
181179
start := c.clk.Now()
182180
qtypeStr := dns.TypeToString[qtype]
183-
result := "failed"
184-
chosenServerIP := ""
185-
tries := 0
181+
var (
182+
resp *dns.Msg
183+
chosenServerIP string
184+
tries int
185+
)
186186
defer func() {
187+
result := "failed"
187188
if resp != nil {
188189
result = dns.RcodeToString[resp.Rcode]
189190
}
@@ -195,12 +196,6 @@ func (c *impl) exchangeOne(ctx context.Context, hostname string, qtype uint16) (
195196
}).Observe(c.clk.Since(start).Seconds())
196197
}()
197198

198-
type dnsRes struct {
199-
resp *dns.Msg
200-
err error
201-
}
202-
ch := make(chan dnsRes, 1)
203-
204199
for i := range c.maxTries {
205200
tries = i + 1
206201
chosenServer := servers[i%len(servers)]
@@ -212,64 +207,61 @@ func (c *impl) exchangeOne(ctx context.Context, hostname string, qtype uint16) (
212207
// and ensures that chosenServer can't be a bare port, e.g. ":1337"
213208
chosenServerIP, _, err = net.SplitHostPort(chosenServer)
214209
if err != nil {
215-
return nil, "", err
210+
return nil, chosenServer, err
216211
}
217212

218-
go func() {
219-
resp, rtt, err := c.exchanger.Exchange(req, chosenServer)
220-
result := "failed"
221-
if resp != nil {
222-
result = dns.RcodeToString[resp.Rcode]
223-
}
224-
if err != nil {
225-
c.log.Infof("logDNSError chosenServer=[%s] hostname=[%s] queryType=[%s] err=[%s]", chosenServer, hostname, qtypeStr, err)
226-
}
227-
c.queryTime.With(prometheus.Labels{
228-
"qtype": qtypeStr,
229-
"result": result,
230-
"resolver": chosenServerIP,
231-
}).Observe(rtt.Seconds())
232-
ch <- dnsRes{resp: resp, err: err}
233-
}()
234-
select {
235-
case <-ctx.Done():
236-
switch ctx.Err() {
237-
case context.DeadlineExceeded:
238-
result = "deadline exceeded"
239-
case context.Canceled:
240-
result = "canceled"
241-
default:
242-
result = "unknown"
243-
}
244-
c.timeoutCounter.With(prometheus.Labels{
245-
"qtype": qtypeStr,
246-
"result": result,
247-
"resolver": chosenServerIP,
248-
"isTLD": fmt.Sprintf("%t", !strings.Contains(hostname, ".")),
249-
}).Inc()
250-
return nil, "", ctx.Err()
251-
case r := <-ch:
252-
if r.err != nil {
253-
// Check if the error is a timeout error, which we want to retry.
254-
// Network errors that can timeout implement the net.Error interface.
255-
var netErr net.Error
256-
isRetryable := errors.As(r.err, &netErr) && netErr.Timeout()
257-
hasRetriesLeft := tries < c.maxTries
258-
if isRetryable && hasRetriesLeft {
259-
continue
260-
} else if isRetryable && !hasRetriesLeft {
261-
c.timeoutCounter.With(prometheus.Labels{
262-
"qtype": qtypeStr,
263-
"result": "out of retries",
264-
"resolver": chosenServerIP,
265-
"isTLD": fmt.Sprintf("%t", !strings.Contains(hostname, ".")),
266-
}).Inc()
267-
}
213+
// Do a bare assignment (not :=) to populate the `resp` used by the defer above.
214+
var rtt time.Duration
215+
resp, rtt, err = c.exchanger.ExchangeContext(ctx, req, chosenServer)
216+
217+
// Do some metrics handling before we do error handling.
218+
result := "failed"
219+
if resp != nil {
220+
result = dns.RcodeToString[resp.Rcode]
221+
}
222+
c.queryTime.With(prometheus.Labels{
223+
"qtype": qtypeStr,
224+
"result": result,
225+
"resolver": chosenServerIP,
226+
}).Observe(rtt.Seconds())
227+
228+
if err != nil {
229+
c.log.Infof("logDNSError chosenServer=[%s] hostname=[%s] queryType=[%s] err=[%s]", chosenServer, hostname, qtypeStr, err)
230+
231+
// Check if the error is a timeout error, which we want to retry.
232+
// Network errors that can timeout implement the net.Error interface.
233+
var netErr net.Error
234+
isRetryable := errors.As(err, &netErr) && netErr.Timeout() && !errors.Is(err, context.DeadlineExceeded)
235+
hasRetriesLeft := tries < c.maxTries
236+
if isRetryable && hasRetriesLeft {
237+
continue
238+
} else if isRetryable && !hasRetriesLeft {
239+
c.timeoutCounter.With(prometheus.Labels{
240+
"qtype": qtypeStr,
241+
"result": "out of retries",
242+
"resolver": chosenServerIP,
243+
"isTLD": fmt.Sprintf("%t", !strings.Contains(hostname, ".")),
244+
}).Inc()
245+
} else if errors.Is(err, context.DeadlineExceeded) {
246+
c.timeoutCounter.With(prometheus.Labels{
247+
"qtype": qtypeStr,
248+
"result": "deadline exceeded",
249+
"resolver": chosenServerIP,
250+
"isTLD": fmt.Sprintf("%t", !strings.Contains(hostname, ".")),
251+
}).Inc()
252+
} else if errors.Is(err, context.Canceled) {
253+
c.timeoutCounter.With(prometheus.Labels{
254+
"qtype": qtypeStr,
255+
"result": "canceled",
256+
"resolver": chosenServerIP,
257+
"isTLD": fmt.Sprintf("%t", !strings.Contains(hostname, ".")),
258+
}).Inc()
268259
}
269260

270-
// This is either a success or a non-retryable error; return either way.
271-
return r.resp, chosenServer, r.err
261+
return nil, chosenServer, err
272262
}
263+
264+
return resp, chosenServer, nil
273265
}
274266

275267
// It's impossible to get past the bottom of the loop: on the last attempt
@@ -286,7 +278,7 @@ func (c *impl) LookupA(ctx context.Context, hostname string) (*Result[*dns.A], s
286278
return nil, resolver, err
287279
}
288280

289-
return resultFromMsg[*dns.A](resp), resolver, wrapErr(dns.TypeA, hostname, resp, err)
281+
return resultFromMsg[*dns.A](resp), resolver, nil
290282
}
291283

292284
// LookupAAAA sends a DNS query to find all AAAA records associated with the
@@ -339,7 +331,7 @@ func (c *impl) LookupTXT(ctx context.Context, hostname string) (*Result[*dns.TXT
339331
// exchanger represents an underlying DNS client. This interface exists solely
340332
// so that its implementation can be swapped out in unit tests.
341333
type exchanger interface {
342-
Exchange(m *dns.Msg, a string) (*dns.Msg, time.Duration, error)
334+
ExchangeContext(ctx context.Context, m *dns.Msg, a string) (*dns.Msg, time.Duration, error)
343335
}
344336

345337
// dohExchanger implements the exchanger interface. It routes all of its DNS
@@ -351,16 +343,16 @@ type dohExchanger struct {
351343
userAgent string
352344
}
353345

354-
// Exchange sends a DoH query to the provided DoH server and returns the response.
355-
func (d *dohExchanger) Exchange(query *dns.Msg, server string) (*dns.Msg, time.Duration, error) {
346+
// ExchangeContext sends a DoH query to the provided DoH server and returns the response.
347+
func (d *dohExchanger) ExchangeContext(ctx context.Context, query *dns.Msg, server string) (*dns.Msg, time.Duration, error) {
356348
q, err := query.Pack()
357349
if err != nil {
358350
return nil, 0, err
359351
}
360352

361353
// The default Unbound URL template
362354
url := fmt.Sprintf("https://%s/dns-query", server)
363-
req, err := http.NewRequest("POST", url, strings.NewReader(string(q)))
355+
req, err := http.NewRequestWithContext(ctx, "POST", url, strings.NewReader(string(q)))
364356
if err != nil {
365357
return nil, 0, err
366358
}

‎bdns/dns_test.go‎

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -608,7 +608,7 @@ type testExchanger struct {
608608

609609
var errTooManyRequests = errors.New("too many requests")
610610

611-
func (te *testExchanger) Exchange(m *dns.Msg, a string) (*dns.Msg, time.Duration, error) {
611+
func (te *testExchanger) ExchangeContext(_ context.Context, m *dns.Msg, a string) (*dns.Msg, time.Duration, error) {
612612
te.Lock()
613613
defer te.Unlock()
614614
msg := &dns.Msg{
@@ -785,16 +785,13 @@ func TestRetry(t *testing.T) {
785785
}
786786

787787
func TestRetryMetrics(t *testing.T) {
788-
isTimeoutErr := &url.Error{Op: "read", Err: testTimeoutError(true)}
789788
staticProvider, err := NewStaticProvider([]string{dnsLoopbackAddr})
790789
test.AssertNotError(t, err, "Got error creating StaticProvider")
791790

792791
testClient := New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 3, "", blog.UseMock(), tlsConfig)
793792
dr := testClient.(*impl)
794-
dr.exchanger = &testExchanger{errs: []error{isTimeoutErr, isTimeoutErr, nil}}
795-
ctx, cancel := context.WithCancel(context.Background())
796-
cancel()
797-
_, _, err = dr.LookupTXT(ctx, "example.com")
793+
dr.exchanger = &testExchanger{errs: []error{context.Canceled}}
794+
_, _, err = dr.LookupTXT(t.Context(), "example.com")
798795
if err == nil ||
799796
err.Error() != "DNS problem: query timed out (and was canceled) looking up TXT for example.com" {
800797
t.Errorf("expected %s, got %s", context.Canceled, err)
@@ -808,10 +805,8 @@ func TestRetryMetrics(t *testing.T) {
808805

809806
testClient = New(time.Second*10, staticProvider, metrics.NoopRegisterer, clock.NewFake(), 3, "", blog.UseMock(), tlsConfig)
810807
dr = testClient.(*impl)
811-
dr.exchanger = &testExchanger{errs: []error{isTimeoutErr, isTimeoutErr, nil}}
812-
ctx, cancel = context.WithTimeout(context.Background(), -10*time.Hour)
813-
defer cancel()
814-
_, _, err = dr.LookupTXT(ctx, "example.com")
808+
dr.exchanger = &testExchanger{errs: []error{context.DeadlineExceeded}}
809+
_, _, err = dr.LookupTXT(t.Context(), "example.com")
815810
if err == nil ||
816811
err.Error() != "DNS problem: query timed out looking up TXT for example.com" {
817812
t.Errorf("expected %s, got %s", context.DeadlineExceeded, err)
@@ -839,9 +834,9 @@ type rotateFailureExchanger struct {
839834
brokenAddresses map[string]bool
840835
}
841836

842-
// Exchange for rotateFailureExchanger tracks the `a` argument in `lookups` and
837+
// ExchangeContext for rotateFailureExchanger tracks the `a` argument in `lookups` and
843838
// if present in `brokenAddresses`, returns a timeout error.
844-
func (e *rotateFailureExchanger) Exchange(m *dns.Msg, a string) (*dns.Msg, time.Duration, error) {
839+
func (e *rotateFailureExchanger) ExchangeContext(_ context.Context, m *dns.Msg, a string) (*dns.Msg, time.Duration, error) {
845840
e.Lock()
846841
defer e.Unlock()
847842

@@ -922,7 +917,7 @@ type dohAlwaysRetryExchanger struct {
922917
err error
923918
}
924919

925-
func (dohE *dohAlwaysRetryExchanger) Exchange(m *dns.Msg, a string) (*dns.Msg, time.Duration, error) {
920+
func (dohE *dohAlwaysRetryExchanger) ExchangeContext(_ context.Context, m *dns.Msg, a string) (*dns.Msg, time.Duration, error) {
926921
dohE.Lock()
927922
defer dohE.Unlock()
928923

‎va/caa.go‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -274,12 +274,10 @@ func (va *ValidationAuthorityImpl) getCAA(ctx context.Context, hostname string)
274274
// validates them. If the identifier argument's value has a wildcard prefix then
275275
// the prefix is stripped and validation will be performed against the base
276276
// domain, honouring any issueWild CAA records encountered as appropriate.
277-
// checkCAARecords returns four values: the first is a string indicating at
277+
// checkCAARecords returns three values: the first is a string indicating at
278278
// which name (i.e. FQDN or parent thereof) CAA records were found, if any. The
279-
// second is a bool indicating whether issuance for the identifier is valid. The
280-
// unmodified *dns.CAA records that were processed/filtered are returned as the
281-
// third argument. Any errors encountered are returned as the fourth return
282-
// value (or nil).
279+
// second is a bool indicating whether issuance for the identifier is valid. Any
280+
// errors encountered are returned as the last return value (or nil).
283281
func (va *ValidationAuthorityImpl) checkCAARecords(
284282
ctx context.Context,
285283
ident identifier.ACMEIdentifier,

‎va/dns.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,11 +21,11 @@ import (
2121
"github.com/letsencrypt/boulder/identifier"
2222
)
2323

24-
// getAddr will query for all A/AAAA records associated with hostname and return
25-
// all valid addresses resolved. This is the same choice made by the Go internal
26-
// resolution library used by net/http. If there is an error resolving the
27-
// hostname, or if no usable IP addresses are available then a berrors.DNSError
28-
// instance is returned with a nil netip.Addr slice.
24+
// getAddr queries for all A/AAAA records associated with hostname, and returns
25+
// all valid addresses resolved and the addresses of all resolvers used. If
26+
// there is an error resolving the hostname, or if no usable IP addresses are
27+
// available then a berrors.DNSError instance is returned with a nil netip.Addr
28+
// slice.
2929
func (va ValidationAuthorityImpl) getAddrs(ctx context.Context, hostname string) ([]netip.Addr, []string, error) {
3030
// Kick off both the A and AAAA lookups in parallel.
3131
var wg sync.WaitGroup

0 commit comments

Comments
 (0)