1 Commits
Author SHA1 Message Date
sneak d07311df99 server: assert timeouts on the served http.Server, not just the constructor (closes #120)
check / check (push) Successful in 44s
The timeout tests called newHTTPServer directly, leaving Run's call
site unguarded: a Run that built its http.Server inline would drop
every timeout with the suite still green. TestRunWiresSocketTimeouts
now wires a Server as cmd/dnswatcher does (minus watcher and resolver,
so no live DNS), drives Run with an unbindable port so it stores its
http.Server and returns without listening, and asserts the served
server carries all four timeouts and both required relationships.
Reverting Run to an inline http.Server without timeouts fails it. The
near-tautological addr/handler test is dropped; the wiring test covers
the handler end to end.

The read-deadline note is corrected: a too-small ReadTimeout does not
make ReadHeaderTimeout unreachable (net/http applies it directly); it
installs an already-expired whole-request deadline. Verified against
the pinned go1.25 net/http.

Model: opus-4-8 (implementation); opus-5-5 (rebase)
2026-09-28 18:48:28 +00:00
12 changed files with 48 additions and 382 deletions
-2
View File
@@ -2,7 +2,6 @@
# The linter is invoked directly rather than through `make lint`: that # The linter is invoked directly rather than through `make lint`: that
# target shells out to `docker build -f Dockerfile.lint`, and there is # target shells out to `docker build -f Dockerfile.lint`, and there is
# no docker daemon inside a docker build. # no docker daemon inside a docker build.
# script/cibuild and script/docker name this stage in --no-cache-filter.
# golangci/golangci-lint:v2.12.2 (Debian-based), 2026-08-10 # golangci/golangci-lint:v2.12.2 (Debian-based), 2026-08-10
FROM golangci/golangci-lint:v2.12.2@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240 AS lint FROM golangci/golangci-lint:v2.12.2@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240 AS lint
@@ -16,7 +15,6 @@ RUN make fmt-check
RUN golangci-lint run --config .golangci.yml ./... RUN golangci-lint run --config .golangci.yml ./...
# Build stage # Build stage
# script/cibuild and script/docker name this stage in --no-cache-filter.
# golang 1.25-alpine, 2026-02-28 # golang 1.25-alpine, 2026-02-28
FROM golang@sha256:f6751d823c26342f9506c03797d2527668d095b0a15f1862cddb4d927a7a4ced AS builder FROM golang@sha256:f6751d823c26342f9506c03797d2527668d095b0a15f1862cddb4d927a7a4ced AS builder
+5 -20
View File
@@ -61,10 +61,6 @@ rejected.
record types: A, AAAA, CNAME, MX, TXT, SRV, CAA, NS. record types: A, AAAA, CNAME, MX, TXT, SRV, CAA, NS.
- Stores results **per nameserver**. The state for a hostname is not a - Stores results **per nameserver**. The state for a hostname is not a
merged view — it is a map from nameserver to record set. merged view — it is a map from nameserver to record set.
- DNS names inside record values (CNAME, MX, SRV and NS targets) are
stored in lower case, because names are case-insensitive and
nameservers may answer in any letter case. TXT and CAA values keep
their letter case; they are not lower-cased.
- Any observable change in any nameserver's response triggers a - Any observable change in any nameserver's response triggers a
notification. This includes: notification. This includes:
- **Record change**: A nameserver returns different records than it - **Record change**: A nameserver returns different records than it
@@ -74,15 +70,8 @@ rejected.
This is distinct from "responded with no records." This is distinct from "responded with no records."
- **NS recovery**: A previously-unreachable nameserver starts - **NS recovery**: A previously-unreachable nameserver starts
responding again. responding again.
- **Inconsistency detected**: Two nameservers return different record - **Inconsistency detected**: Two nameservers that previously agreed
sets for the same hostname and did not already differ on the previous now return different record sets for the same hostname.
check. Every pair of nameservers is compared. The alert is sent once
for each such pair, on the check where they start to disagree, and not
again while they keep disagreeing, including after a restart. A
nameserver that was not in the previous check (newly added, or back
after dropping out) and answers differently is reported on the check
where it appears. If a pair agrees again and later disagrees, the
alert is sent again.
### TCP Port Monitoring ### TCP Port Monitoring
@@ -476,13 +465,9 @@ them. We provide:
- `script/fmt` — format all code (gofmt -s, goimports) - `script/fmt` — format all code (gofmt -s, goimports)
- `script/fmt-check` — check formatting (read-only) - `script/fmt-check` — check formatting (read-only)
- `script/check` — run test, lint, and fmt-check - `script/check` — run test, lint, and fmt-check
- `script/docker` — build the Docker image tagged via `script/projectname`, with - `script/docker` — build the Docker image tagged via
`--no-cache-filter=lint,builder` so the lint stage and the builder stage, `script/projectname`
which runs the tests, run on every invocation - `script/cibuild` — CI entrypoint: plain `docker build .`
- `script/cibuild` — CI entrypoint: `docker build` with
`--no-cache-filter=lint,builder`, so the lint stage and the builder stage,
which runs the tests, run on every invocation, because a cached build lints
nothing and queries no DNS
- `script/precommit` — run by the git pre-commit hook; `go mod tidy` - `script/precommit` — run by the git pre-commit hook; `go mod tidy`
guard, then `script/check` guard, then `script/check`
- `script/install-precommit` — install the git pre-commit hook - `script/install-precommit` — install the git pre-commit hook
+5 -14
View File
@@ -23,20 +23,10 @@ Rationale, Design, TODO, License, Author) if any are still missing.
# Completed Steps # Completed Steps
- 2026-09-28: the inconsistency alert is sent once, on the check where two - 2026-09-21: server timeout test now drives `Run` and asserts the
nameservers start to disagree or where a nameserver that disagrees first served `http.Server` carries the timeouts; corrected the inverted
appears, instead of on every check while they disagree, and not again after `ReadTimeout` rationale note (#120).
a restart. Every pair of nameservers is compared, not only neighbours in
sorted order of name (closes #158).
- 2026-09-28: DNS names in record values (CNAME, MX, SRV and NS targets) are
lower-cased, so nameservers that answer in different letter case no longer
count as inconsistent or as a record change (closes #157).
- 2026-09-28: `script/cibuild` and `script/docker` now pass
`--no-cache-filter=lint,builder` so lint and tests run every build (closes
#115).
- 2026-09-28: the server timeout test now drives `Run` and checks the
`http.Server` it serves carries the timeouts; corrected the `ReadTimeout`
note in that test (closes #120).
- 2026-09-28: upaas deploy readiness — runtime image runs as unprivileged - 2026-09-28: upaas deploy readiness — runtime image runs as unprivileged
`dnswatcher`, Docker `HEALTHCHECK`, startup fails when the data directory is `dnswatcher`, Docker `HEALTHCHECK`, startup fails when the data directory is
not writable, README "Running under upaas" (closes #147). not writable, README "Running under upaas" (closes #147).
@@ -44,6 +34,7 @@ Rationale, Design, TODO, License, Author) if any are still missing.
`internal/healthcheck`, and `internal/logger` (closes #110). `internal/healthcheck`, and `internal/logger` (closes #110).
- 2026-09-21: `go mod tidy` dropped the redundant `golang.org/x/sync` - 2026-09-21: `go mod tidy` dropped the redundant `golang.org/x/sync`
`// indirect` line so `script/bootstrap` leaves a clean tree (#132) `// indirect` line so `script/bootstrap` leaves a clean tree (#132)
- 2026-08-10: comment-only corrections to `script/bootstrap`, - 2026-08-10: comment-only corrections to `script/bootstrap`,
`script/cibuild`, and `Dockerfile.lint`. The `goimports` pin in `script/cibuild`, and `Dockerfile.lint`. The `goimports` pin in
`script/bootstrap` was justified by a claim that `script/fmt-check` `script/bootstrap` was justified by a claim that `script/fmt-check`
-8
View File
@@ -1,8 +0,0 @@
package resolver
import "github.com/miekg/dns"
// ExtractRecordValue exports extractRecordValue for testing.
func ExtractRecordValue(rr dns.RR) string {
return extractRecordValue(rr)
}
+5 -8
View File
@@ -608,10 +608,7 @@ func classifyResponse(resp *NameserverResponse, state queryState) {
} }
} }
// extractRecordValue formats a DNS RR value as a string. DNS names // extractRecordValue formats a DNS RR value as a string.
// are case-insensitive and nameservers may answer in any letter case,
// so names are lower-cased to compare equal. TXT and CAA values keep
// their letter case.
func extractRecordValue(rr dns.RR) string { func extractRecordValue(rr dns.RR) string {
switch r := rr.(type) { switch r := rr.(type) {
case *dns.A: case *dns.A:
@@ -619,22 +616,22 @@ func extractRecordValue(rr dns.RR) string {
case *dns.AAAA: case *dns.AAAA:
return r.AAAA.String() return r.AAAA.String()
case *dns.CNAME: case *dns.CNAME:
return strings.ToLower(r.Target) return r.Target
case *dns.MX: case *dns.MX:
return fmt.Sprintf("%d %s", r.Preference, strings.ToLower(r.Mx)) return fmt.Sprintf("%d %s", r.Preference, r.Mx)
case *dns.TXT: case *dns.TXT:
return strings.Join(r.Txt, "") return strings.Join(r.Txt, "")
case *dns.SRV: case *dns.SRV:
return fmt.Sprintf( return fmt.Sprintf(
"%d %d %d %s", "%d %d %d %s",
r.Priority, r.Weight, r.Port, strings.ToLower(r.Target), r.Priority, r.Weight, r.Port, r.Target,
) )
case *dns.CAA: case *dns.CAA:
return fmt.Sprintf( return fmt.Sprintf(
"%d %s \"%s\"", r.Flag, r.Tag, r.Value, "%d %s \"%s\"", r.Flag, r.Tag, r.Value,
) )
case *dns.NS: case *dns.NS:
return strings.ToLower(r.Ns) return r.Ns
default: default:
return "" return ""
} }
-62
View File
@@ -1,62 +0,0 @@
package resolver_test
import (
"testing"
"github.com/miekg/dns"
"github.com/stretchr/testify/assert"
"sneak.berlin/go/dnswatcher/internal/resolver"
)
func TestExtractRecordValue_LetterCase(t *testing.T) {
t.Parallel()
tests := []struct {
name string
rr dns.RR
want string
}{
{
name: "MX target lower-cased",
rr: &dns.MX{Preference: 1, Mx: "ASPMX.L.GOOGLE.COM."},
want: "1 aspmx.l.google.com.",
},
{
name: "NS target lower-cased",
rr: &dns.NS{Ns: "x.ns.joker.COM."},
want: "x.ns.joker.com.",
},
{
name: "CNAME target lower-cased",
rr: &dns.CNAME{Target: "WWW.Example.Com."},
want: "www.example.com.",
},
{
name: "SRV target lower-cased",
rr: &dns.SRV{
Priority: 10, Weight: 5, Port: 443,
Target: "SIP.Example.Com.",
},
want: "10 5 443 sip.example.com.",
},
{
name: "TXT value keeps its case",
rr: &dns.TXT{Txt: []string{"Verify=AbC123"}},
want: "Verify=AbC123",
},
{
name: "CAA value keeps its case",
rr: &dns.CAA{Flag: 0, Tag: "issue", Value: "LetsEncrypt.org"},
want: `0 issue "LetsEncrypt.org"`,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
assert.Equal(t, tt.want, resolver.ExtractRecordValue(tt.rr))
})
}
}
+14 -8
View File
@@ -55,7 +55,10 @@ func buildServer(t *testing.T) *server.Server {
// TestRunWiresSocketTimeouts pins that the http.Server the running // TestRunWiresSocketTimeouts pins that the http.Server the running
// server actually serves — the one Run builds and hands to // server actually serves — the one Run builds and hands to
// ListenAndServe — carries every socket-level timeout, plus the two // ListenAndServe — carries every socket-level timeout, plus the two
// relationships the values must satisfy. // relationships the values must satisfy. Earlier tests asserted these
// on newHTTPServer directly, which left the call site unguarded: a Run
// that built its http.Server inline would drop every timeout with the
// suite still green (https://git.eeqj.de/sneak/dnswatcher/issues/120).
// //
// Run is driven to completion with an unbindable port: it builds and // Run is driven to completion with an unbindable port: it builds and
// stores s.httpServer, then ListenAndServe fails at once and Run // stores s.httpServer, then ListenAndServe fails at once and Run
@@ -63,13 +66,16 @@ func buildServer(t *testing.T) *server.Server {
// goroutine after Run returns, so reading s.httpServer is free of any // goroutine after Run returns, so reading s.httpServer is free of any
// data race. Nothing here measures elapsed time. // data race. Nothing here measures elapsed time.
// //
// ReadTimeout must be at least ReadHeaderTimeout. net/http reads the // On the ReadTimeout >= ReadHeaderTimeout relationship: a smaller
// headers under ReadHeaderTimeout, then sets the read deadline for the // ReadTimeout does NOT make the header phase unreachable. net/http's
// rest of the request to ReadTimeout, counted from when it started // (*Server).readHeaderTimeout applies ReadHeaderTimeout directly, so
// reading the request. If ReadTimeout were smaller, a request whose // the header read keeps its full budget. What breaks is the
// headers arrived after ReadTimeout but within ReadHeaderTimeout would // whole-request deadline: once the headers are read, readRequest
// get a read deadline that had already passed, so reading its body // installs a read deadline of t0+ReadTimeout, which is already in the
// would fail at once. // past when ReadTimeout is the smaller value, severing the request.
// Verified against the pinned go1.25 net/http (Dockerfile golang
// 1.25-alpine; go.mod go 1.25.5): src/net/http/server.go readRequest
// and (*Server).readHeaderTimeout.
func TestRunWiresSocketTimeouts(t *testing.T) { func TestRunWiresSocketTimeouts(t *testing.T) {
// Sets an env var and touches viper global state, so like the // Sets an env var and touches viper global state, so like the
// config tests it cannot use t.Parallel. // config tests it cannot use t.Parallel.
-25
View File
@@ -1,25 +0,0 @@
package watcher
import (
"context"
"sneak.berlin/go/dnswatcher/internal/state"
)
// NewlyDisagreeingPairs exports newlyDisagreeingPairs for testing.
func NewlyDisagreeingPairs(
prev *state.HostnameState,
current map[string]map[string][]string,
) [][2]string {
return newlyDisagreeingPairs(prev, current)
}
// DetectHostnameChanges exports detectHostnameChanges for testing.
func (w *Watcher) DetectHostnameChanges(
ctx context.Context,
hostname string,
prev *state.HostnameState,
current map[string]map[string][]string,
) {
w.detectHostnameChanges(ctx, hostname, prev, current)
}
-182
View File
@@ -1,182 +0,0 @@
package watcher_test
import (
"slices"
"testing"
"sneak.berlin/go/dnswatcher/internal/state"
"sneak.berlin/go/dnswatcher/internal/watcher"
)
const (
host = "www.example.net"
nsA = "a.ns.example.net."
nsB = "b.ns.example.net."
nsC = "c.ns.example.net."
ip1 = "192.0.2.1"
ip2 = "192.0.2.2"
ip3 = "192.0.2.3"
)
// hostnameState builds the state a check with these records leaves behind.
func hostnameState(
records map[string]map[string][]string,
) *state.HostnameState {
hs := &state.HostnameState{
RecordsByNameserver: make(map[string]*state.NameserverRecordState),
}
for ns, recs := range records {
hs.RecordsByNameserver[ns] = &state.NameserverRecordState{
Records: recs,
Status: "ok",
}
}
return hs
}
func TestNewlyDisagreeingPairs(t *testing.T) {
t.Parallel()
onlyA := map[string]map[string][]string{nsA: {"A": {ip1}}}
agree := map[string]map[string][]string{nsA: {"A": {ip1}}, nsB: {"A": {ip1}}}
disagree := map[string]map[string][]string{nsA: {"A": {ip1}}, nsB: {"A": {ip2}}}
alert := [][2]string{{nsA, nsB}}
// b already disagrees with a and c; then c changes, so a and c,
// which agreed, now differ.
bDiffers := map[string]map[string][]string{
nsA: {"A": {ip1}}, nsB: {"A": {ip2}}, nsC: {"A": {ip1}},
}
cChanges := map[string]map[string][]string{
nsA: {"A": {ip1}}, nsB: {"A": {ip2}}, nsC: {"A": {ip3}},
}
// Each case starts from the state loaded at startup and runs the
// checks in order; want[i] is what check i alerts for.
tests := []struct {
name string
loaded map[string]map[string][]string
checks []map[string]map[string][]string
want [][][2]string
}{
{
name: "disagreement persisting across checks alerts once",
loaded: agree,
checks: []map[string]map[string][]string{disagree, disagree, disagree},
want: [][][2]string{alert, nil, nil},
},
{
name: "disagreement starting on a later check alerts on it",
loaded: agree,
checks: []map[string]map[string][]string{agree, agree, disagree},
want: [][][2]string{nil, nil, alert},
},
{
name: "disagreement in the loaded state does not alert",
loaded: disagree,
checks: []map[string]map[string][]string{disagree, disagree},
want: [][][2]string{nil, nil},
},
{
name: "nameserver new on the first check and disagreeing alerts once",
loaded: onlyA,
checks: []map[string]map[string][]string{disagree, disagree},
want: [][][2]string{alert, nil},
},
{
name: "disagreement after agreeing again alerts again",
loaded: agree,
checks: []map[string]map[string][]string{disagree, agree, disagree},
want: [][][2]string{alert, nil, alert},
},
{
name: "new disagreement while another nameserver differs alerts",
loaded: bDiffers,
checks: []map[string]map[string][]string{cChanges, cChanges},
want: [][][2]string{{{nsA, nsC}}, nil},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
prev := hostnameState(tt.loaded)
for i, current := range tt.checks {
got := watcher.NewlyDisagreeingPairs(prev, current)
if !slices.Equal(got, tt.want[i]) {
t.Errorf(
"check %d: alerted for %v, want %v",
i, got, tt.want[i],
)
}
prev = hostnameState(current)
}
})
}
}
func TestInconsistencyAlert(t *testing.T) {
t.Parallel()
onlyA := map[string]map[string][]string{nsA: {"A": {ip1}}}
agree := map[string]map[string][]string{nsA: {"A": {ip1}}, nsB: {"A": {ip1}}}
disagree := map[string]map[string][]string{nsA: {"A": {ip1}}, nsB: {"A": {ip2}}}
// Each case starts from the state loaded at startup and then sees
// the nameservers disagree on three checks in a row.
tests := []struct {
name string
loaded map[string]map[string][]string
want int
}{
{
name: "disagreement lasting several checks alerts once",
loaded: agree,
want: 1,
},
{
name: "disagreement in the loaded state does not alert",
loaded: disagree,
want: 0,
},
{
name: "nameserver new on the first check and disagreeing alerts once",
loaded: onlyA,
want: 1,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
// The hostname change detection uses only the notifier.
notifier := &mockNotifier{}
w := watcher.NewForTest(nil, nil, nil, nil, nil, notifier)
prev := hostnameState(tt.loaded)
for range 3 {
w.DetectHostnameChanges(t.Context(), host, prev, disagree)
prev = hostnameState(disagree)
}
got := 0
for _, n := range notifier.getNotifications() {
if n.Title == "Inconsistency: "+host {
got++
}
}
if got != tt.want {
t.Errorf("sent %d inconsistency alerts, want %d", got, tt.want)
}
})
}
}
+15 -41
View File
@@ -366,7 +366,7 @@ func (w *Watcher) detectHostnameChanges(
) { ) {
w.detectRecordChanges(ctx, hostname, prev, current) w.detectRecordChanges(ctx, hostname, prev, current)
w.detectNSDisappearances(ctx, hostname, prev, current) w.detectNSDisappearances(ctx, hostname, prev, current)
w.detectInconsistencies(ctx, hostname, prev, current) w.detectInconsistencies(ctx, hostname, current)
} }
func (w *Watcher) detectRecordChanges( func (w *Watcher) detectRecordChanges(
@@ -448,11 +448,22 @@ func (w *Watcher) detectNSDisappearances(
func (w *Watcher) detectInconsistencies( func (w *Watcher) detectInconsistencies(
ctx context.Context, ctx context.Context,
hostname string, hostname string,
prev *state.HostnameState,
current map[string]map[string][]string, current map[string]map[string][]string,
) { ) {
for _, pair := range newlyDisagreeingPairs(prev, current) { nameservers := make([]string, 0, len(current))
ns1, ns2 := pair[0], pair[1] for ns := range current {
nameservers = append(nameservers, ns)
}
sort.Strings(nameservers)
for i := range len(nameservers) - 1 {
ns1 := nameservers[i]
ns2 := nameservers[i+1]
if recordsEqual(current[ns1], current[ns2]) {
continue
}
msg := fmt.Sprintf( msg := fmt.Sprintf(
"Hostname: %s\n%s: %v\n%s: %v", "Hostname: %s\n%s: %v\n%s: %v",
@@ -470,43 +481,6 @@ func (w *Watcher) detectInconsistencies(
} }
} }
// newlyDisagreeingPairs returns every pair of nameservers whose records
// differ in current, in sorted order of name, except pairs where both
// nameservers were in prev and already differed there. A nameserver
// missing from prev is paired with every nameserver it differs from.
func newlyDisagreeingPairs(
prev *state.HostnameState,
current map[string]map[string][]string,
) [][2]string {
nameservers := make([]string, 0, len(current))
for ns := range current {
nameservers = append(nameservers, ns)
}
sort.Strings(nameservers)
var pairs [][2]string
for i, ns1 := range nameservers {
for _, ns2 := range nameservers[i+1:] {
if recordsEqual(current[ns1], current[ns2]) {
continue
}
prev1, ok1 := prev.RecordsByNameserver[ns1]
prev2, ok2 := prev.RecordsByNameserver[ns2]
if ok1 && ok2 && !recordsEqual(prev1.Records, prev2.Records) {
continue
}
pairs = append(pairs, [2]string{ns1, ns2})
}
}
return pairs
}
func (w *Watcher) checkAllPorts(ctx context.Context) { func (w *Watcher) checkAllPorts(ctx context.Context) {
// Phase 1: Build current IP:port → hostname associations // Phase 1: Build current IP:port → hostname associations
// from fresh DNS data. // from fresh DNS data.
+2 -6
View File
@@ -1,18 +1,14 @@
#!/bin/sh #!/bin/sh
# script/cibuild: run the CI build. The Dockerfile's lint stage runs # script/cibuild: run the CI build. The Dockerfile's lint stage runs
# make fmt-check and golangci-lint; its builder stage runs make test # make fmt-check and golangci-lint; its builder stage runs make test
# and make build. # and make build. A successful build implies all of those passed.
#
# --no-cache-filter=lint,builder runs both stages on every invocation;
# otherwise an unchanged tree is served from the layer cache and passes
# without linting or querying live DNS.
set -eu set -eu
ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" ROOT="$(cd "$(dirname "$0")/.." && pwd -P)"
main() { main() {
cd "$ROOT" cd "$ROOT"
docker build --no-cache-filter=lint,builder . docker build .
} }
main "$@" main "$@"
+2 -6
View File
@@ -1,10 +1,6 @@
#!/bin/sh #!/bin/sh
# script/docker: build the Docker image tagged with the project name. # script/docker: build the Docker image tagged with the project name.
# The tag comes from script/projectname. # Identical in all repos; the tag comes from script/projectname.
#
# --no-cache-filter=lint,builder runs the lint stage and the builder
# stage (make test) on every invocation; otherwise an unchanged tree is
# served from the layer cache without linting or querying live DNS.
set -eu set -eu
SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)" SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)"
@@ -12,7 +8,7 @@ ROOT="$(cd "$SCRIPT_DIR/.." && pwd -P)"
main() { main() {
cd "$ROOT" cd "$ROOT"
docker build --no-cache-filter=lint,builder -t "$("$SCRIPT_DIR/projectname")" . docker build -t "$("$SCRIPT_DIR/projectname")" .
} }
main "$@" main "$@"