4 Commits

Author SHA1 Message Date
5ab217bfd2 Merge pull request 'Reduce DNS query timeout and limit root server fan-out (closes #29)' (#30) from fix/reduce-dns-timeout-and-root-fanout into main
Some checks failed
Check / check (push) Has been cancelled
Reviewed-on: #30
2026-02-28 12:07:20 +01:00
518a2cc42e Merge pull request 'doc: add TESTING.md — real DNS only, no mocks' (#34) from doc/testing-policy into main
Some checks failed
Check / check (push) Has been cancelled
Reviewed-on: #34
2026-02-28 12:06:57 +01:00
user
4cb81aac24 doc: add testing policy — real DNS only, no mocks
Some checks failed
Check / check (pull_request) Failing after 5m24s
Documents the project testing philosophy: all resolver tests must
use live DNS queries. Mocking the DNS client layer is not permitted.
Includes rationale and anti-patterns to avoid.
2026-02-22 04:28:47 -08:00
user
203b581704 Reduce DNS query timeout to 2s and limit root server fan-out to 3
Some checks failed
Check / check (pull_request) Failing after 5m57s
- Reduce queryTimeoutDuration from 5s to 2s
- Add randomRootServers() that shuffles and picks 3 root servers
- Replace all rootServerList() call sites with randomRootServers()
- Keep maxRetries = 2

Closes #29
2026-02-22 03:35:16 -08:00
2 changed files with 52 additions and 21 deletions

34
TESTING.md Normal file
View File

@@ -0,0 +1,34 @@
# Testing Policy
## DNS Resolution Tests
All resolver tests **MUST** use live queries against real DNS servers.
No mocking of the DNS client layer is permitted.
### Rationale
The resolver performs iterative resolution from root nameservers through
the full delegation chain. Mocked responses cannot faithfully represent
the variety of real-world DNS behavior (truncation, referrals, glue
records, DNSSEC, varied response times, EDNS, etc.). Testing against
real servers ensures the resolver works correctly in production.
### Constraints
- Tests hit real DNS infrastructure and require network access
- Test duration depends on network conditions; timeout tuning keeps
the suite within the 30-second target
- Query timeout is calibrated to 3× maximum antipodal RTT (~300ms)
plus processing margin
- Root server fan-out is limited to reduce parallel query load
- Flaky failures from transient network issues are acceptable and
should be investigated as potential resolver bugs, not papered over
with mocks or skip flags
### What NOT to do
- **Do not mock `DNSClient`** for resolver tests (the mock constructor
exists for unit-testing other packages that consume the resolver)
- **Do not add `-short` flags** to skip slow tests
- **Do not increase `-timeout`** to hide hanging queries
- **Do not modify linter configuration** to suppress findings

View File

@@ -4,7 +4,7 @@ import (
"context" "context"
"errors" "errors"
"fmt" "fmt"
"math/rand/v2" "math/rand"
"net" "net"
"sort" "sort"
"strings" "strings"
@@ -14,13 +14,7 @@ import (
) )
const ( const (
// queryTimeoutDuration is the per-exchange DNS timeout. queryTimeoutDuration = 2 * time.Second
//
// Rationale: maximum RTT to antipodal root/TLD servers is
// ~300ms. We use 3× max RTT + 10ms processing ≈ 910ms,
// rounded to 1s. Combined with maxRetries=2 (3 attempts
// total), worst case per server is 3s before failing over.
queryTimeoutDuration = 1 * time.Second
maxRetries = 2 maxRetries = 2
maxDelegation = 20 maxDelegation = 20
timeoutMultiplier = 2 timeoutMultiplier = 2
@@ -30,7 +24,7 @@ const (
// ErrRefused is returned when a DNS server refuses a query. // ErrRefused is returned when a DNS server refuses a query.
var ErrRefused = errors.New("dns query refused") var ErrRefused = errors.New("dns query refused")
func allRootServers() []string { func rootServerList() []string {
return []string{ return []string{
"198.41.0.4", // a.root-servers.net "198.41.0.4", // a.root-servers.net
"170.247.170.2", // b "170.247.170.2", // b
@@ -48,17 +42,20 @@ func allRootServers() []string {
} }
} }
// rootServerList returns 3 randomly-selected root servers. const maxRootServers = 3
// The full set is 13; we limit fan-out because the root is
// operated reliably — if 3 are unreachable, the problem is // randomRootServers returns a shuffled subset of root servers.
// local network, not the root. func randomRootServers() []string {
func rootServerList() []string { all := rootServerList()
shuffled := allRootServers() rand.Shuffle(len(all), func(i, j int) {
rand.Shuffle(len(shuffled), func(i, j int) { all[i], all[j] = all[j], all[i]
shuffled[i], shuffled[j] = shuffled[j], shuffled[i]
}) })
return shuffled[:3] if len(all) > maxRootServers {
return all[:maxRootServers]
}
return all
} }
func checkCtx(ctx context.Context) error { func checkCtx(ctx context.Context) error {
@@ -322,7 +319,7 @@ func (r *Resolver) resolveNSRecursive(
msg.SetQuestion(domain, dns.TypeNS) msg.SetQuestion(domain, dns.TypeNS)
msg.RecursionDesired = true msg.RecursionDesired = true
for _, ip := range rootServerList() { for _, ip := range randomRootServers() {
if checkCtx(ctx) != nil { if checkCtx(ctx) != nil {
return nil, ErrContextCanceled return nil, ErrContextCanceled
} }
@@ -353,7 +350,7 @@ func (r *Resolver) resolveARecord(
msg.SetQuestion(hostname, dns.TypeA) msg.SetQuestion(hostname, dns.TypeA)
msg.RecursionDesired = true msg.RecursionDesired = true
for _, ip := range rootServerList() { for _, ip := range randomRootServers() {
if checkCtx(ctx) != nil { if checkCtx(ctx) != nil {
return nil, ErrContextCanceled return nil, ErrContextCanceled
} }
@@ -405,7 +402,7 @@ func (r *Resolver) FindAuthoritativeNameservers(
candidate := strings.Join(labels[i:], ".") + "." candidate := strings.Join(labels[i:], ".") + "."
nsNames, err := r.followDelegation( nsNames, err := r.followDelegation(
ctx, candidate, rootServerList(), ctx, candidate, randomRootServers(),
) )
if err == nil && len(nsNames) > 0 { if err == nil && len(nsNames) > 0 {
sort.Strings(nsNames) sort.Strings(nsNames)