Skip to content

Commit 2a8f0fe

Browse files
jshajsha
authored
Rename several items in bdns (letsencrypt#5260)
[Go style says](https://blog.golang.org/package-names): > Avoid stutter. Since client code uses the package name as a prefix > when referring to the package contents, the names for those contents > need not repeat the package name. The HTTP server provided by the > http package is called Server, not HTTPServer. Client code refers to > this type as http.Server, so there is no ambiguity. Rename DNSClient, DNSClientImpl, NewDNSClientImpl, NewTestDNSClientImpl, DNSError, and MockDNSClient to follow those guidelines. Unexport DNSClientImpl and MockTimeoutError (was only used internally). Make New and NewTest return the Client interface rather than a concrete `impl` type.
1 parent 2a6cb72 commit 2a8f0fe

File tree

11 files changed

+76
-74
lines changed

11 files changed

+76
-74
lines changed

‎bdns/dns.go‎

Lines changed: 25 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -146,15 +146,15 @@ var (
146146
}
147147
)
148148

149-
// DNSClient queries for DNS records
150-
type DNSClient interface {
149+
// Client queries for DNS records
150+
type Client interface {
151151
LookupTXT(context.Context, string) (txts []string, err error)
152152
LookupHost(context.Context, string) ([]net.IP, error)
153153
LookupCAA(context.Context, string) ([]*dns.CAA, string, error)
154154
}
155155

156-
// DNSClientImpl represents a client that talks to an external resolver
157-
type DNSClientImpl struct {
156+
// impl represents a client that talks to an external resolver
157+
type impl struct {
158158
dnsClient exchanger
159159
servers []string
160160
allowRestrictedAddresses bool
@@ -168,22 +168,22 @@ type DNSClientImpl struct {
168168
idMismatchCounter *prometheus.CounterVec
169169
}
170170

171-
var _ DNSClient = &DNSClientImpl{}
171+
var _ Client = &impl{}
172172

173173
type exchanger interface {
174174
Exchange(m *dns.Msg, a string) (*dns.Msg, time.Duration, error)
175175
}
176176

177-
// NewDNSClientImpl constructs a new DNS resolver object that utilizes the
177+
// New constructs a new DNS resolver object that utilizes the
178178
// provided list of DNS servers for resolution.
179-
func NewDNSClientImpl(
179+
func New(
180180
readTimeout time.Duration,
181181
servers []string,
182182
stats prometheus.Registerer,
183183
clk clock.Clock,
184184
maxTries int,
185185
log blog.Logger,
186-
) *DNSClientImpl {
186+
) Client {
187187
dnsClient := new(dns.Client)
188188

189189
// Set timeout for underlying net.Conn
@@ -222,7 +222,7 @@ func NewDNSClientImpl(
222222
)
223223
stats.MustRegister(queryTime, totalLookupTime, timeoutCounter, idMismatchCounter)
224224

225-
return &DNSClientImpl{
225+
return &impl{
226226
dnsClient: dnsClient,
227227
servers: servers,
228228
allowRestrictedAddresses: false,
@@ -236,26 +236,26 @@ func NewDNSClientImpl(
236236
}
237237
}
238238

239-
// NewTestDNSClientImpl constructs a new DNS resolver object that utilizes the
239+
// NewTest constructs a new DNS resolver object that utilizes the
240240
// provided list of DNS servers for resolution and will allow loopback addresses.
241241
// This constructor should *only* be called from tests (unit or integration).
242-
func NewTestDNSClientImpl(
242+
func NewTest(
243243
readTimeout time.Duration,
244244
servers []string,
245245
stats prometheus.Registerer,
246246
clk clock.Clock,
247247
maxTries int,
248-
log blog.Logger) *DNSClientImpl {
249-
resolver := NewDNSClientImpl(readTimeout, servers, stats, clk, maxTries, log)
250-
resolver.allowRestrictedAddresses = true
248+
log blog.Logger) Client {
249+
resolver := New(readTimeout, servers, stats, clk, maxTries, log)
250+
resolver.(*impl).allowRestrictedAddresses = true
251251
return resolver
252252
}
253253

254254
// exchangeOne performs a single DNS exchange with a randomly chosen server
255255
// out of the server list, returning the response, time, and error (if any).
256256
// We assume that the upstream resolver requests and validates DNSSEC records
257257
// itself.
258-
func (dnsClient *DNSClientImpl) exchangeOne(ctx context.Context, hostname string, qtype uint16) (resp *dns.Msg, err error) {
258+
func (dnsClient *impl) exchangeOne(ctx context.Context, hostname string, qtype uint16) (resp *dns.Msg, err error) {
259259
m := new(dns.Msg)
260260
// Set question type
261261
m.SetQuestion(dns.Fqdn(hostname), qtype)
@@ -385,15 +385,15 @@ type dnsResp struct {
385385
// LookupTXT sends a DNS query to find all TXT records associated with
386386
// the provided hostname which it returns along with the returned
387387
// DNS authority section.
388-
func (dnsClient *DNSClientImpl) LookupTXT(ctx context.Context, hostname string) ([]string, error) {
388+
func (dnsClient *impl) LookupTXT(ctx context.Context, hostname string) ([]string, error) {
389389
var txt []string
390390
dnsType := dns.TypeTXT
391391
r, err := dnsClient.exchangeOne(ctx, hostname, dnsType)
392392
if err != nil {
393-
return nil, &DNSError{dnsType, hostname, err, -1}
393+
return nil, &Error{dnsType, hostname, err, -1}
394394
}
395395
if r.Rcode != dns.RcodeSuccess {
396-
return nil, &DNSError{dnsType, hostname, nil, r.Rcode}
396+
return nil, &Error{dnsType, hostname, nil, r.Rcode}
397397
}
398398

399399
for _, answer := range r.Answer {
@@ -425,13 +425,13 @@ func isPrivateV6(ip net.IP) bool {
425425
return false
426426
}
427427

428-
func (dnsClient *DNSClientImpl) lookupIP(ctx context.Context, hostname string, ipType uint16) ([]dns.RR, error) {
428+
func (dnsClient *impl) lookupIP(ctx context.Context, hostname string, ipType uint16) ([]dns.RR, error) {
429429
resp, err := dnsClient.exchangeOne(ctx, hostname, ipType)
430430
if err != nil {
431-
return nil, &DNSError{ipType, hostname, err, -1}
431+
return nil, &Error{ipType, hostname, err, -1}
432432
}
433433
if resp.Rcode != dns.RcodeSuccess {
434-
return nil, &DNSError{ipType, hostname, nil, resp.Rcode}
434+
return nil, &Error{ipType, hostname, nil, resp.Rcode}
435435
}
436436
return resp.Answer, nil
437437
}
@@ -442,7 +442,7 @@ func (dnsClient *DNSClientImpl) lookupIP(ctx context.Context, hostname string, i
442442
// requests in the case of temporary network errors. It can return net package,
443443
// context.Canceled, and context.DeadlineExceeded errors, all wrapped in the
444444
// DNSError type.
445-
func (dnsClient *DNSClientImpl) LookupHost(ctx context.Context, hostname string) ([]net.IP, error) {
445+
func (dnsClient *impl) LookupHost(ctx context.Context, hostname string) ([]net.IP, error) {
446446
var recordsA, recordsAAAA []dns.RR
447447
var errA, errAAAA error
448448
var wg sync.WaitGroup
@@ -487,15 +487,15 @@ func (dnsClient *DNSClientImpl) LookupHost(ctx context.Context, hostname string)
487487
// the provided hostname and the complete dig-style RR `response`. This
488488
// response is quite verbose, however it's only populated when the CAA
489489
// response is non-empty.
490-
func (dnsClient *DNSClientImpl) LookupCAA(ctx context.Context, hostname string) ([]*dns.CAA, string, error) {
490+
func (dnsClient *impl) LookupCAA(ctx context.Context, hostname string) ([]*dns.CAA, string, error) {
491491
dnsType := dns.TypeCAA
492492
r, err := dnsClient.exchangeOne(ctx, hostname, dnsType)
493493
if err != nil {
494-
return nil, "", &DNSError{dnsType, hostname, err, -1}
494+
return nil, "", &Error{dnsType, hostname, err, -1}
495495
}
496496

497497
if r.Rcode == dns.RcodeServerFailure {
498-
return nil, "", &DNSError{dnsType, hostname, nil, r.Rcode}
498+
return nil, "", &Error{dnsType, hostname, nil, r.Rcode}
499499
}
500500

501501
var CAAs []*dns.CAA

‎bdns/dns_test.go‎

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -242,31 +242,31 @@ func TestMain(m *testing.M) {
242242
}
243243

244244
func TestDNSNoServers(t *testing.T) {
245-
obj := NewTestDNSClientImpl(time.Hour, []string{}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
245+
obj := NewTest(time.Hour, []string{}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
246246

247247
_, err := obj.LookupHost(context.Background(), "letsencrypt.org")
248248

249249
test.AssertError(t, err, "No servers")
250250
}
251251

252252
func TestDNSOneServer(t *testing.T) {
253-
obj := NewTestDNSClientImpl(time.Second*10, []string{dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
253+
obj := NewTest(time.Second*10, []string{dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
254254

255255
_, err := obj.LookupHost(context.Background(), "letsencrypt.org")
256256

257257
test.AssertNotError(t, err, "No message")
258258
}
259259

260260
func TestDNSDuplicateServers(t *testing.T) {
261-
obj := NewTestDNSClientImpl(time.Second*10, []string{dnsLoopbackAddr, dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
261+
obj := NewTest(time.Second*10, []string{dnsLoopbackAddr, dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
262262

263263
_, err := obj.LookupHost(context.Background(), "letsencrypt.org")
264264

265265
test.AssertNotError(t, err, "No message")
266266
}
267267

268268
func TestDNSLookupsNoServer(t *testing.T) {
269-
obj := NewTestDNSClientImpl(time.Second*10, []string{}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
269+
obj := NewTest(time.Second*10, []string{}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
270270

271271
_, err := obj.LookupTXT(context.Background(), "letsencrypt.org")
272272
test.AssertError(t, err, "No servers")
@@ -279,7 +279,7 @@ func TestDNSLookupsNoServer(t *testing.T) {
279279
}
280280

281281
func TestDNSServFail(t *testing.T) {
282-
obj := NewTestDNSClientImpl(time.Second*10, []string{dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
282+
obj := NewTest(time.Second*10, []string{dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
283283
bad := "servfail.com"
284284

285285
_, err := obj.LookupTXT(context.Background(), bad)
@@ -294,7 +294,7 @@ func TestDNSServFail(t *testing.T) {
294294
}
295295

296296
func TestDNSLookupTXT(t *testing.T) {
297-
obj := NewTestDNSClientImpl(time.Second*10, []string{dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
297+
obj := NewTest(time.Second*10, []string{dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
298298

299299
a, err := obj.LookupTXT(context.Background(), "letsencrypt.org")
300300
t.Logf("A: %v", a)
@@ -308,7 +308,7 @@ func TestDNSLookupTXT(t *testing.T) {
308308
}
309309

310310
func TestDNSLookupHost(t *testing.T) {
311-
obj := NewTestDNSClientImpl(time.Second*10, []string{dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
311+
obj := NewTest(time.Second*10, []string{dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
312312

313313
ip, err := obj.LookupHost(context.Background(), "servfail.com")
314314
t.Logf("servfail.com - IP: %s, Err: %s", ip, err)
@@ -368,16 +368,16 @@ func TestDNSLookupHost(t *testing.T) {
368368
ip, err = obj.LookupHost(context.Background(), hostname)
369369
t.Logf("%s - IP: %s, Err: %s", hostname, ip, err)
370370
test.AssertError(t, err, "Should be an error")
371-
expectedErr := &DNSError{dns.TypeA, hostname, nil, dns.RcodeRefused}
371+
expectedErr := &Error{dns.TypeA, hostname, nil, dns.RcodeRefused}
372372
test.AssertDeepEquals(t, err, expectedErr)
373373
}
374374

375375
func TestDNSNXDOMAIN(t *testing.T) {
376-
obj := NewTestDNSClientImpl(time.Second*10, []string{dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
376+
obj := NewTest(time.Second*10, []string{dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
377377

378378
hostname := "nxdomain.letsencrypt.org"
379379
_, err := obj.LookupHost(context.Background(), hostname)
380-
expected := &DNSError{dns.TypeA, hostname, nil, dns.RcodeNameError}
380+
expected := &Error{dns.TypeA, hostname, nil, dns.RcodeNameError}
381381
test.AssertDeepEquals(t, err, expected)
382382

383383
_, err = obj.LookupTXT(context.Background(), hostname)
@@ -386,7 +386,7 @@ func TestDNSNXDOMAIN(t *testing.T) {
386386
}
387387

388388
func TestDNSLookupCAA(t *testing.T) {
389-
obj := NewTestDNSClientImpl(time.Second*10, []string{dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
389+
obj := NewTest(time.Second*10, []string{dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), 1, blog.UseMock())
390390
removeIDExp := regexp.MustCompile(" id: [[:digit:]]+")
391391

392392
caas, resp, err := obj.LookupCAA(context.Background(), "bracewel.net")
@@ -596,7 +596,8 @@ func TestRetry(t *testing.T) {
596596
}
597597

598598
for i, tc := range tests {
599-
dr := NewTestDNSClientImpl(time.Second*10, []string{dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), tc.maxTries, blog.UseMock())
599+
testClient := NewTest(time.Second*10, []string{dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), tc.maxTries, blog.UseMock())
600+
dr := testClient.(*impl)
600601
dr.dnsClient = tc.te
601602
_, err := dr.LookupTXT(context.Background(), "example.com")
602603
if err == errTooManyRequests {
@@ -623,7 +624,8 @@ func TestRetry(t *testing.T) {
623624
}
624625
}
625626

626-
dr := NewTestDNSClientImpl(time.Second*10, []string{dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), 3, blog.UseMock())
627+
testClient := NewTest(time.Second*10, []string{dnsLoopbackAddr}, metrics.NoopRegisterer, clock.NewFake(), 3, blog.UseMock())
628+
dr := testClient.(*impl)
627629
dr.dnsClient = &testExchanger{errs: []error{isTempErr, isTempErr, nil}}
628630
ctx, cancel := context.WithCancel(context.Background())
629631
cancel()
@@ -716,7 +718,7 @@ func TestRotateServerOnErr(t *testing.T) {
716718
// number of dnsServers to ensure we always get around to trying the one
717719
// working server
718720
maxTries := 5
719-
client := NewTestDNSClientImpl(time.Second*10, dnsServers, metrics.NoopRegisterer, clock.NewFake(), maxTries, blog.UseMock())
721+
client := NewTest(time.Second*10, dnsServers, metrics.NoopRegisterer, clock.NewFake(), maxTries, blog.UseMock())
720722

721723
// Configure a mock exchanger that will always return a retryable error for
722724
// the A and B servers. This will force the C server to do all the work once
@@ -725,7 +727,7 @@ func TestRotateServerOnErr(t *testing.T) {
725727
brokenAddresses: map[string]bool{"a": true, "b": true},
726728
lookups: make(map[string]int),
727729
}
728-
client.dnsClient = mock
730+
client.(*impl).dnsClient = mock
729731

730732
// Perform a bunch of lookups. We choose the initial server randomly. Any time
731733
// A or B is chosen there should be an error and a retry using the next server

‎bdns/mocks.go‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,13 @@ import (
1212
blog "github.com/letsencrypt/boulder/log"
1313
)
1414

15-
// MockDNSClient is a mock
16-
type MockDNSClient struct {
15+
// MockClient is a mock
16+
type MockClient struct {
1717
Log blog.Logger
1818
}
1919

2020
// LookupTXT is a mock
21-
func (mock *MockDNSClient) LookupTXT(_ context.Context, hostname string) ([]string, error) {
21+
func (mock *MockClient) LookupTXT(_ context.Context, hostname string) ([]string, error) {
2222
if hostname == "_acme-challenge.servfail.com" {
2323
return nil, fmt.Errorf("SERVFAIL")
2424
}
@@ -50,8 +50,8 @@ func (mock *MockDNSClient) LookupTXT(_ context.Context, hostname string) ([]stri
5050
return []string{"hostname"}, nil
5151
}
5252

53-
// MockTimeoutError returns a a net.OpError for which Timeout() returns true.
54-
func MockTimeoutError() *net.OpError {
53+
// makeTimeoutError returns a a net.OpError for which Timeout() returns true.
54+
func makeTimeoutError() *net.OpError {
5555
return &net.OpError{
5656
Err: os.NewSyscallError("ugh timeout", timeoutError{}),
5757
}
@@ -67,13 +67,13 @@ func (t timeoutError) Timeout() bool {
6767
}
6868

6969
// LookupHost is a mock
70-
func (mock *MockDNSClient) LookupHost(_ context.Context, hostname string) ([]net.IP, error) {
70+
func (mock *MockClient) LookupHost(_ context.Context, hostname string) ([]net.IP, error) {
7171
if hostname == "always.invalid" ||
7272
hostname == "invalid.invalid" {
7373
return []net.IP{}, nil
7474
}
7575
if hostname == "always.timeout" {
76-
return []net.IP{}, &DNSError{dns.TypeA, "always.timeout", MockTimeoutError(), -1}
76+
return []net.IP{}, &Error{dns.TypeA, "always.timeout", makeTimeoutError(), -1}
7777
}
7878
if hostname == "always.error" {
7979
err := &net.OpError{
@@ -86,7 +86,7 @@ func (mock *MockDNSClient) LookupHost(_ context.Context, hostname string) ([]net
8686
m.AuthenticatedData = true
8787
m.SetEdns0(4096, false)
8888
logDNSError(mock.Log, "mock.server", hostname, m, nil, err)
89-
return []net.IP{}, &DNSError{dns.TypeA, hostname, err, -1}
89+
return []net.IP{}, &Error{dns.TypeA, hostname, err, -1}
9090
}
9191
if hostname == "id.mismatch" {
9292
err := dns.ErrId
@@ -100,7 +100,7 @@ func (mock *MockDNSClient) LookupHost(_ context.Context, hostname string) ([]net
100100
record.A = net.ParseIP("127.0.0.1")
101101
r.Answer = append(r.Answer, record)
102102
logDNSError(mock.Log, "mock.server", hostname, m, r, err)
103-
return []net.IP{}, &DNSError{dns.TypeA, hostname, err, -1}
103+
return []net.IP{}, &Error{dns.TypeA, hostname, err, -1}
104104
}
105105
// dual-homed host with an IPv6 and an IPv4 address
106106
if hostname == "ipv4.and.ipv6.localhost" {
@@ -119,6 +119,6 @@ func (mock *MockDNSClient) LookupHost(_ context.Context, hostname string) ([]net
119119
}
120120

121121
// LookupCAA returns mock records for use in tests.
122-
func (mock *MockDNSClient) LookupCAA(_ context.Context, domain string) ([]*dns.CAA, string, error) {
122+
func (mock *MockClient) LookupCAA(_ context.Context, domain string) ([]*dns.CAA, string, error) {
123123
return nil, "", nil
124124
}

‎bdns/problem.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,20 +8,20 @@ import (
88
"github.com/miekg/dns"
99
)
1010

11-
// DNSError wraps a DNS error with various relevant information
12-
type DNSError struct {
11+
// Error wraps a DNS error with various relevant information
12+
type Error struct {
1313
recordType uint16
1414
hostname string
1515
// Exactly one of rCode or underlying should be set.
1616
underlying error
1717
rCode int
1818
}
1919

20-
func (d DNSError) Underlying() error {
20+
func (d Error) Underlying() error {
2121
return d.underlying
2222
}
2323

24-
func (d DNSError) Error() string {
24+
func (d Error) Error() string {
2525
var detail, additional string
2626
if d.underlying != nil {
2727
if netErr, ok := d.underlying.(*net.OpError); ok {
@@ -50,7 +50,7 @@ func (d DNSError) Error() string {
5050
}
5151

5252
// Timeout returns true if the underlying error was a timeout
53-
func (d DNSError) Timeout() bool {
53+
func (d Error) Timeout() bool {
5454
if netErr, ok := d.underlying.(*net.OpError); ok {
5555
return netErr.Timeout()
5656
} else if d.underlying == context.Canceled || d.underlying == context.DeadlineExceeded {

0 commit comments

Comments
 (0)