From cc86473410454da21c425b5fae25307760e9b05e Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 10 Aug 2026 12:37:47 +0000 Subject: [PATCH 01/15] build: run all linting in Docker via Dockerfile.lint (closes #134) golangci-lint is no longer installed or run on the host. script/lint is now a thin wrapper that builds the new root Dockerfile.lint, which COPYs the repo into the digest-pinned golangci/golangci-lint:v2.12.2 image and lints as a build step, so a successful build is a clean lint. This works even where the docker daemon is remote and bind mounts are impossible. Dockerfile.lint is split into a deps stage (base image, go mod download) and a lint stage (source copy, linter run). script/lint passes --no-cache-filter=lint so the lint stage executes on every invocation: caching is explicitly waived for linting, and a cached build lints nothing. The deps stage stays cached and no global cache invalidation is performed. --progress=plain keeps the linter's own output visible. golangci-lint config verify is deliberately omitted: it fetches its JSON schema over a live, unpinned HTTPS call, which would make linting network-dependent and defeat hash-pinning. script/bootstrap no longer installs golangci-lint and warns instead when docker is absent. The goimports install stays, since script/fmt and script/fmt-check still run it on the host. The root Dockerfile ran make check in its builder stage, which would now recurse into script/lint and shell out to docker build with no daemon available. It gains its own lint stage on the same pinned image, invoked directly, with the builder depending on it via COPY --from=lint and running make fmt-check, make test and make build. --- Dockerfile | 38 ++++++++++++++++++++++++++------------ Dockerfile.lint | 27 +++++++++++++++++++++++++++ README.md | 15 +++++++++++---- TODO.md | 10 ++++++++++ script/bootstrap | 22 +++++++++++++++------- script/lint | 20 ++++++++++++++++++-- 6 files changed, 107 insertions(+), 25 deletions(-) create mode 100644 Dockerfile.lint diff --git a/Dockerfile b/Dockerfile index ec33b34..94b5b57 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,13 +1,9 @@ -# Build stage -# golang 1.25-alpine, 2026-02-28 -FROM golang@sha256:f6751d823c26342f9506c03797d2527668d095b0a15f1862cddb4d927a7a4ced AS builder - -RUN apk add --no-cache git make gcc musl-dev binutils-gold - -# golangci-lint v2.12.2, 2026-08-07 -RUN go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@c0d3ddc9cf3faa61a4e378e879ece580256d76e5 -# goimports v0.42.0 -RUN go install golang.org/x/tools/cmd/goimports@009367f5c17a8d4c45a961a3a509277190a9a6f0 +# Lint stage - fast feedback on lint issues, before the build starts. +# The linter is invoked directly rather than through `make lint`: that +# target shells out to `docker build -f Dockerfile.lint`, and there is +# no docker daemon inside a docker build. +# golangci/golangci-lint:v2.12.2 (Debian-based), 2026-08-10 +FROM golangci/golangci-lint:v2.12.2@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240 AS lint WORKDIR /src COPY go.mod go.sum ./ @@ -15,8 +11,26 @@ RUN go mod download COPY . . -# Run all checks - build fails if any check fails -RUN make check +RUN make fmt-check +RUN golangci-lint run --config .golangci.yml ./... + +# Build stage +# golang 1.25-alpine, 2026-02-28 +FROM golang@sha256:f6751d823c26342f9506c03797d2527668d095b0a15f1862cddb4d927a7a4ced AS builder + +RUN apk add --no-cache git make gcc musl-dev binutils-gold + +# Force BuildKit to run the lint stage before proceeding +COPY --from=lint /src/go.sum /dev/null + +WORKDIR /src +COPY go.mod go.sum ./ +RUN go mod download + +COPY . . + +# Run the tests - build fails if any test fails +RUN make test # Build the binary RUN make build diff --git a/Dockerfile.lint b/Dockerfile.lint new file mode 100644 index 0000000..5c236de --- /dev/null +++ b/Dockerfile.lint @@ -0,0 +1,27 @@ +# Lint-only image: used by script/lint. golangci-lint is never run on +# the host — the repo is COPYed into the build context and the linter +# runs as a build step, so a successful build IS a clean lint. This +# also works where the docker daemon is remote and bind mounts are +# impossible. +# +# `golangci-lint config verify` is deliberately NOT run here: it +# fetches its JSON schema over a live, unpinned HTTPS call, which would +# make linting network-dependent and defeat hash-pinning. +# +# golangci/golangci-lint:v2.12.2 (Debian-based), 2026-08-10 +FROM golangci/golangci-lint:v2.12.2@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240 AS deps + +WORKDIR /src + +# Dependencies first, so this stage stays cached across lint runs. +COPY go.mod go.sum ./ +RUN go mod download + +# Everything below is invalidated on every run by the +# --no-cache-filter=lint that script/lint passes: caching is explicitly +# waived for linting, and a cached build lints nothing. +FROM deps AS lint + +COPY . . + +RUN golangci-lint run --config .golangci.yml ./... diff --git a/README.md b/README.md index 83f9226..96141f9 100644 --- a/README.md +++ b/README.md @@ -380,14 +380,21 @@ standard: normalized scripts in `script/` are the entrypoints for the development workflow, and the Makefile targets are thin shims that call them. We provide: -- `script/bootstrap` — install all dependencies (go, pinned - golangci-lint and goimports, `go mod download`) +- `script/bootstrap` — install all dependencies (go, pinned goimports, + `go mod download`). It does not install golangci-lint: see + `script/lint` below. - `script/setup` — make a fresh clone ready for development: bootstrap plus the git pre-commit hook - `script/projectname` — print the project name (used for the Docker image tag) - `script/test` — run the test suite (race detector, coverage) -- `script/lint` — run golangci-lint +- `script/lint` — run golangci-lint, always inside Docker: it builds + `Dockerfile.lint`, which COPYs the repo into the digest-pinned + `golangci-lint` image and lints as a build step, so a successful + build is a clean lint. The linter is never installed or run on the + host, and Docker is the only prerequisite. Caching is waived for + linting: the lint stage is forced to execute on every run with + `--no-cache-filter`, because a cached build lints nothing. - `script/fmt` — format all code (gofmt -s, goimports) - `script/fmt-check` — check formatting (read-only) - `script/check` — run test, lint, and fmt-check @@ -403,7 +410,7 @@ them. We provide: ```sh make build # Build binary to bin/dnswatcher make test # Run tests with race detector -make lint # Run golangci-lint +make lint # Run golangci-lint in Docker (requires docker) make fmt # Format code make check # Run all checks (test, lint, fmt-check) make clean # Remove build artifacts diff --git a/TODO.md b/TODO.md index bc4c519..8e3909b 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,16 @@ confirm make check still passes. # Completed Steps +- 2026-08-10: all linting moved into Docker: new root `Dockerfile.lint` + on the digest-pinned `golangci/golangci-lint:v2.12.2` image, + `script/lint` reduced to a thin wrapper that builds it with + `--no-cache-filter=lint` so the linter actually executes every run, + golangci-lint install dropped from `script/bootstrap` (goimports + stays, `script/fmt` needs it on the host), and the root `Dockerfile` + given its own lint stage so its build no longer recurses through + `make check` into `script/lint`. `golangci-lint config verify` is + deliberately omitted: it fetches its schema over an unpinned live + HTTPS call - 2026-08-07: golangci-lint bumped to v2.12.2 (commit-pinned installs in `Dockerfile` and `script/bootstrap`); `.golangci.yml` set to the org-standard v2-schema config used across the org's repos diff --git a/script/bootstrap b/script/bootstrap index 129cc77..0901c87 100755 --- a/script/bootstrap +++ b/script/bootstrap @@ -3,15 +3,16 @@ # this repo. Idempotent: every install is guarded by a check so already # installed tools are skipped. Base tooling comes from nix, apt, brew, # or apk (detected in that order); assumes nothing is present. -# golangci-lint and goimports are installed via `go install` at the same -# pinned commits the Dockerfile uses (never "latest"). +# goimports is installed via `go install` at a pinned commit (never +# "latest") because script/fmt and script/fmt-check run it on the host. +# The linter is NOT installed here: golangci-lint runs via docker only +# (script/lint), pinned by image digest, so its only prerequisite is a +# working docker. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" -# Pinned versions, 2026-08-07 (same pins as the Dockerfile) -# golangci-lint v2.12.2 -GOLANGCI_LINT_REF="github.com/golangci/golangci-lint/v2/cmd/golangci-lint@c0d3ddc9cf3faa61a4e378e879ece580256d76e5" +# Pinned version, 2026-08-07 (same pin as the Dockerfile) # goimports v0.42.0 GOIMPORTS_REF="golang.org/x/tools/cmd/goimports@009367f5c17a8d4c45a961a3a509277190a9a6f0" @@ -69,11 +70,18 @@ main() { if missing make; then pkg_install gnumake make make make; fi if missing go; then pkg_install go golang go go; fi - # Lint/format tools, pinned via go install (installs into + # Format tools, pinned via go install (installs into # "$(go env GOPATH)/bin"; ensure that is on your PATH). - if missing golangci-lint; then go install "$GOLANGCI_LINT_REF"; fi if missing goimports; then go install "$GOIMPORTS_REF"; fi + # Linting runs via docker only (script/lint). Warn, don't fail: + # everything except `make lint` works without it. + if missing docker; then + echo "bootstrap: WARNING: docker not found; make lint and" >&2 + echo "bootstrap: make docker require it. Install docker to" >&2 + echo "bootstrap: run the linter." >&2 + fi + go mod download echo "bootstrap complete" diff --git a/script/lint b/script/lint index 8017180..84a1972 100755 --- a/script/lint +++ b/script/lint @@ -1,12 +1,28 @@ #!/bin/sh -# script/lint: run the linter. +# script/lint: run the linter. golangci-lint is never installed or run +# on the host: it runs via docker only, one way, everywhere. This +# builds Dockerfile.lint, which COPYs the repo into the digest-pinned +# golangci-lint image and lints as a build step, so a successful build +# means a clean lint. +# +# --no-cache-filter=lint forces the lint stage (source copy + linter +# run) to execute on every invocation. Without it an unchanged tree +# returns success in well under a second having linted nothing. The +# deps stage (base image + go mod download) stays cached, and no global +# cache invalidation is performed. --progress=plain keeps the linter's +# own output visible. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" main() { cd "$ROOT" - golangci-lint run --config .golangci.yml ./... + docker build \ + --progress=plain \ + --no-cache-filter=lint \ + --target lint \ + -f Dockerfile.lint \ + . } main "$@" -- 2.54.0 From 9cb2c2b7e00774488faaf8c7c2068f61fcb23937 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 10 Aug 2026 13:09:17 +0000 Subject: [PATCH 02/15] test: make live DNS tests robust instead of gated (closes #93) The resolver's live-DNS tests failed nondeterministically, a different subset each run. Three structural causes, all test-side: - Burst fan-out. Every test in the package is parallel and the build hosts have many cores, so all ~35 iterative resolutions started at the same instant and, because queryServers walks rootServerList() in fixed order, hit the same root server within milliseconds. Root servers rate-limit that. - No retry anywhere. One dropped UDP packet in a delegation chain failed a test outright. - Unanimity assertions. TestQueryAllNameservers_AllReturnOK and _NXDomainFromAllNS required every one of a domain's nameservers to answer, with no tolerance for one being slow. New internal/resolver/livedns_test.go addresses each: a package-wide gate bounds how many live resolutions are in flight at once, every live operation gets three attempts with exponential backoff and its own deadline, and multi-nameserver assertions now need a strict majority rather than unanimity. The retry predicate is deliberately transport-level -- "did a nameserver answer at all" -- never the assertion under test, so a resolver that answers incorrectly still fails on the first attempt. A nameserver that stays silent is tolerated; one that answers wrongly is not. livedns_harness_test.go tests that machinery directly: quorum arithmetic, status counting, the gate's concurrency bound, per-attempt deadlines, and recovery from a transient failure. It touches no DNS. Nothing is mocked, faked, stubbed, recorded, skipped or build-tagged, and production resolver behaviour is unchanged. Test caps move to the new org-wide values ruled at prompts issue 41: 60s hard cap, 20s target, 90s -timeout backstop. REPO_POLICIES.md is re-vendored byte-identical from sneak/prompts rather than hand-edited, which also picks up the golangci-lint paragraph this copy had drifted behind on. TESTING.md's stale 30-second target follows to 60. https://git.eeqj.de/sneak/dnswatcher/issues/93 --- REPO_POLICIES.md | 20 +- TESTING.md | 2 +- TODO.md | 10 + internal/resolver/livedns_harness_test.go | 162 ++++++++ internal/resolver/livedns_test.go | 437 ++++++++++++++++++++++ internal/resolver/resolver_test.go | 289 +++++--------- script/test | 2 +- 7 files changed, 709 insertions(+), 213 deletions(-) create mode 100644 internal/resolver/livedns_harness_test.go create mode 100644 internal/resolver/livedns_test.go diff --git a/REPO_POLICIES.md b/REPO_POLICIES.md index bc2f161..9aba6b0 100644 --- a/REPO_POLICIES.md +++ b/REPO_POLICIES.md @@ -1,6 +1,6 @@ --- title: Repository Policies -last_modified: 2026-07-06 +last_modified: 2026-08-07 --- This document covers repository structure, tooling, and workflow standards. Code @@ -189,8 +189,13 @@ style conventions are in separate documents: module under test to verify it compiles/parses. There is no excuse for `make test` to be a no-op. -- `make test` must complete in under 20 seconds. Add a 30-second timeout in the - Makefile. +- `make test` must complete in under 60 seconds. That is the hard cap, and a + suite that exceeds it fails. Under 20 seconds is the target. A suite between + 20 and 60 seconds is still green, but the overage must be filed as an + improvement bug against that repo. Add a 90-second timeout to the test + invocation in the Makefile (`go test -timeout 90s`). The backstop deliberately + sits above the hard cap so that it catches a genuinely hung test rather than a + merely slow one. - **`make test` should use the conditional verbose rerun pattern.** Run tests without `-v` (verbose) first. If tests fail, automatically rerun with `-v` to @@ -209,9 +214,9 @@ style conventions are in separate documents: ```makefile test: - @go test -timeout 30s -race -cover ./... || \ + @go test -timeout 90s -race -cover ./... || \ { echo "--- Rerunning with -v for details ---"; \ - go test -timeout 30s -race -v ./...; exit 1; } + go test -timeout 90s -race -v ./...; exit 1; } ``` Python example: @@ -260,7 +265,10 @@ style conventions are in separate documents: - `.golangci.yml` is standardized and must _NEVER_ be modified by an agent, only manually by the user. Fetch from - `https://git.eeqj.de/sneak/prompts/raw/branch/main/.golangci.yml`. + `https://git.eeqj.de/sneak/prompts/raw/branch/main/.golangci.yml`. The + canonical golangci-lint version is v2.12.2 (released 2026-05-06), installed + commit-pinned via + `go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@c0d3ddc9cf3faa61a4e378e879ece580256d76e5`. - When pinning images or packages by hash, add a comment above the reference with the version and date (YYYY-MM-DD). diff --git a/TESTING.md b/TESTING.md index 3dd34c4..0acae53 100644 --- a/TESTING.md +++ b/TESTING.md @@ -17,7 +17,7 @@ real servers ensures the resolver works correctly in production. - 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 + the suite within the 60-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 diff --git a/TODO.md b/TODO.md index 8e3909b..1b4f13f 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,16 @@ confirm make check still passes. # Completed Steps +- 2026-08-10: live-DNS test flakiness addressed by robustness rather + than gating, per the owner's ruling on #93: new + `internal/resolver/livedns_test.go` adds a package-wide concurrency + gate (so parallel tests stop bursting at the first root server), + retry with exponential backoff on transport failures only, and + quorum instead of unanimity for multi-nameserver assertions. The + `make test` cap moved to the new org-wide 60s hard cap / 20s target + with a 90s `-timeout` backstop; `REPO_POLICIES.md` re-vendored + byte-identical from `sneak/prompts`. No mocks, no `-short`, no build + tags, no skips, and no change to production resolver behaviour - 2026-08-10: all linting moved into Docker: new root `Dockerfile.lint` on the digest-pinned `golangci/golangci-lint:v2.12.2` image, `script/lint` reduced to a thin wrapper that builds it with diff --git a/internal/resolver/livedns_harness_test.go b/internal/resolver/livedns_harness_test.go new file mode 100644 index 0000000..066337d --- /dev/null +++ b/internal/resolver/livedns_harness_test.go @@ -0,0 +1,162 @@ +package resolver_test + +import ( + "context" + "sync" + "testing" + "time" + + "github.com/stretchr/testify/assert" + + "sneak.berlin/go/dnswatcher/internal/resolver" +) + +// Tests for the live-DNS harness in livedns_test.go itself. These +// exercise pure logic and the retry/concurrency plumbing; they +// perform no DNS resolution of any kind, so they neither mock DNS +// nor depend on it. + +func TestLiveQuorumIsStrictMajority(t *testing.T) { + t.Parallel() + + cases := map[int]int{ + 0: 1, + 1: 1, + 2: 2, + 3: 2, + 4: 3, + 5: 3, + 13: 7, + } + + for total, want := range cases { + assert.Equal( + t, want, liveQuorum(total), + "liveQuorum(%d)", total, + ) + } +} + +func TestStatusCountingIgnoresSilentNameservers(t *testing.T) { + t.Parallel() + + results := map[string]*resolver.NameserverResponse{ + "ns1.example.": { + Nameserver: "ns1.example.", + Status: resolver.StatusOK, + }, + "ns2.example.": { + Nameserver: "ns2.example.", + Status: resolver.StatusOK, + }, + "ns3.example.": { + Nameserver: "ns3.example.", + Status: resolver.StatusTimeout, + }, + "ns4.example.": { + Nameserver: "ns4.example.", + Status: resolver.StatusError, + }, + } + + assert.Equal( + t, 2, countStatus(results, resolver.StatusOK), + ) + assert.Equal( + t, 0, countStatus(results, resolver.StatusNXDomain), + ) + + // Two of four answered, which is short of the quorum of + // three: this is the state that triggers a retry rather + // than an assertion failure. + assert.Equal(t, 2, answeredCount(results)) + assert.Less(t, answeredCount(results), liveQuorum(len(results))) + + assert.Equal( + t, + "ns1.example.=ok ns2.example.=ok "+ + "ns3.example.=timeout ns4.example.=error", + describeStatuses(results), + ) +} + +func TestRetryLiveRecoversFromTransientFailure(t *testing.T) { + t.Parallel() + + const wantAttempts = 2 + + attempts := 0 + + retryLive(t, "transient", func(_ context.Context) error { + attempts++ + + if attempts < wantAttempts { + return errLiveNoAnswer + } + + return nil + }) + + assert.Equal(t, wantAttempts, attempts) +} + +func TestRetryLiveGivesEachAttemptADeadline(t *testing.T) { + t.Parallel() + + retryLive(t, "deadline", func(ctx context.Context) error { + deadline, ok := ctx.Deadline() + assert.True(t, ok, "attempt should carry a deadline") + assert.LessOrEqual( + t, time.Until(deadline), liveAttemptTimeout, + ) + + return nil + }) +} + +func TestRunLiveBoundsConcurrency(t *testing.T) { + t.Parallel() + + const workers = 24 + + var ( + mu sync.Mutex + wg sync.WaitGroup + inFlight int + maxSeen int + ) + + wg.Add(workers) + + for range workers { + go func() { + defer wg.Done() + + _ = runLive(func(_ context.Context) error { + mu.Lock() + inFlight++ + + if inFlight > maxSeen { + maxSeen = inFlight + } + mu.Unlock() + + time.Sleep(time.Millisecond) + + mu.Lock() + inFlight-- + mu.Unlock() + + return nil + }) + }() + } + + wg.Wait() + + assert.Positive(t, maxSeen) + assert.LessOrEqual( + t, maxSeen, liveConcurrency, + "live queries must stay under the package-wide gate", + ) +} diff --git a/internal/resolver/livedns_test.go b/internal/resolver/livedns_test.go new file mode 100644 index 0000000..55ea755 --- /dev/null +++ b/internal/resolver/livedns_test.go @@ -0,0 +1,437 @@ +package resolver_test + +import ( + "context" + "errors" + "fmt" + "sort" + "strings" + "testing" + "time" + + "sneak.berlin/go/dnswatcher/internal/resolver" +) + +// ---------------------------------------------------------------- +// Live DNS test support +// ---------------------------------------------------------------- +// +// Every test in this package resolves against the real, live DNS — +// see TESTING.md. Nothing here mocks, fakes, stubs, records or +// replays DNS, and nothing here skips or gates a test: the helpers +// below only change *how* the live queries are issued, so that a +// single dropped UDP packet or one slow authoritative server does +// not turn a correct resolver into a red build. +// +// Three mechanisms, all test-side: +// +// 1. Bounded concurrency. The package's tests are parallel and the +// build hosts have many cores, so without a limit every test +// starts its own iterative resolution at the same instant and +// they all hit the first root server in rootServerList() within +// a few milliseconds of each other. Root servers rate-limit +// that, which shows up as a different arbitrary subset of tests +// failing on each run. liveGate caps how many resolutions are +// in flight at once. +// +// 2. Retry with exponential backoff. Each live operation gets +// several attempts with its own timeout. The retry predicate is +// strictly transport-level — "did a nameserver answer at all" — +// never the assertion the test is making. A resolver that +// answers incorrectly still fails on the first attempt. +// +// 3. Quorum. Where an assertion spans several independent +// nameservers, a strict majority answering as expected is +// enough; a server that fails to answer is tolerated, while a +// server that answers *wrongly* still fails the test. + +const ( + // liveAttempts is how many times a live DNS operation is + // attempted before the test fails. + liveAttempts = 3 + + // liveAttemptTimeout bounds one attempt. Worst case for an + // operation is liveAttempts * liveAttemptTimeout plus the + // backoff — about 26 seconds, well inside the 90-second + // `go test -timeout` backstop even when several operations + // exhaust their attempts. + liveAttemptTimeout = 8 * time.Second + + // liveBackoffBase is the delay after the first failed + // attempt; it is multiplied by liveBackoffFactor each time. + liveBackoffBase = 500 * time.Millisecond + + // liveBackoffFactor is the exponential backoff multiplier. + liveBackoffFactor = 2 + + // liveConcurrency caps how many live resolutions may be in + // flight across the whole package at once. + liveConcurrency = 6 + + // minNameservers is the smallest nameserver count a + // well-run zone is expected to publish. + minNameservers = 2 +) + +// liveGate bounds concurrent live resolutions package-wide. It has +// to be package scoped: the whole point is that it is shared by +// every parallel test in the package. +// +//nolint:gochecknoglobals // package-wide live query rate limit +var liveGate = make(chan struct{}, liveConcurrency) + +var ( + // errLiveNoAnswer reports that a live operation produced no + // usable answer, which is retried rather than asserted on. + errLiveNoAnswer = errors.New("no answer from live DNS") + + // errLiveNoQuorum reports that too few of a domain's + // nameservers answered for a quorum assertion to be made. + errLiveNoQuorum = errors.New("no nameserver quorum") +) + +// runLive executes one attempt of a live operation, holding a slot +// in liveGate for its duration and bounding it with its own +// timeout. +func runLive(op func(ctx context.Context) error) error { + liveGate <- struct{}{} + defer func() { <-liveGate }() + + ctx, cancel := context.WithTimeout( + context.Background(), liveAttemptTimeout, + ) + defer cancel() + + return op(ctx) +} + +// retryLive runs op until it reports success, retrying transport +// failures with exponential backoff, and fails the test if every +// attempt fails. op returns an error only for a failure to obtain +// an answer — never for an answer the test disagrees with, which +// belongs in an assertion so that it fails immediately. op stores +// whatever it obtained where its caller can find it. +func retryLive( + t *testing.T, + what string, + op func(ctx context.Context) error, +) { + t.Helper() + + var last error + + backoff := liveBackoffBase + + for attempt := range liveAttempts { + if attempt > 0 { + t.Logf( + "%s: attempt %d of %d failed (%v), "+ + "retrying in %s", + what, attempt, liveAttempts, last, backoff, + ) + time.Sleep(backoff) + + backoff *= liveBackoffFactor + } + + last = runLive(op) + if last == nil { + return + } + } + + t.Fatalf( + "%s: no answer after %d live attempts: %v", + what, liveAttempts, last, + ) +} + +// liveQuorum is how many of total nameservers must agree for a +// multi-nameserver assertion to hold: a strict majority. +func liveQuorum(total int) int { + if total < 1 { + return 1 + } + + return total/2 + 1 +} + +// countStatus counts the responses carrying the given status. +func countStatus( + results map[string]*resolver.NameserverResponse, + status string, +) int { + n := 0 + + for _, resp := range results { + if resp.Status == status { + n++ + } + } + + return n +} + +// answeredCount counts the nameservers that produced an answer of +// any kind, as opposed to failing or timing out. +func answeredCount( + results map[string]*resolver.NameserverResponse, +) int { + return len(results) - + countStatus(results, resolver.StatusError) - + countStatus(results, resolver.StatusTimeout) +} + +// describeStatuses renders per-nameserver statuses for use in +// assertion failure messages. +func describeStatuses( + results map[string]*resolver.NameserverResponse, +) string { + parts := make([]string, 0, len(results)) + for ns, resp := range results { + parts = append( + parts, fmt.Sprintf("%s=%s", ns, resp.Status), + ) + } + + sort.Strings(parts) + + return strings.Join(parts, " ") +} + +// ---------------------------------------------------------------- +// Live operation wrappers +// ---------------------------------------------------------------- + +// liveFindAuthoritative resolves a domain's authoritative +// nameservers, retrying until the delegation chain can be walked. +func liveFindAuthoritative( + t *testing.T, + r *resolver.Resolver, + domain string, +) []string { + t.Helper() + + var out []string + + retryLive( + t, + "FindAuthoritativeNameservers("+domain+")", + func(ctx context.Context) error { + ns, err := r.FindAuthoritativeNameservers(ctx, domain) + if err != nil { + return err + } + + if len(ns) == 0 { + return fmt.Errorf( + "%w: %s has no nameservers", + errLiveNoAnswer, domain, + ) + } + + out = ns + + return nil + }, + ) + + return out +} + +// liveLookupNS is liveFindAuthoritative through the LookupNS entry +// point, so that both entry points stay independently exercised. +func liveLookupNS( + t *testing.T, + r *resolver.Resolver, + domain string, +) []string { + t.Helper() + + var out []string + + retryLive( + t, + "LookupNS("+domain+")", + func(ctx context.Context) error { + ns, err := r.LookupNS(ctx, domain) + if err != nil { + return err + } + + if len(ns) == 0 { + return fmt.Errorf( + "%w: %s has no nameservers", + errLiveNoAnswer, domain, + ) + } + + out = ns + + return nil + }, + ) + + return out +} + +// liveQueryNameserver queries one nameserver, retrying while that +// nameserver fails to answer. NXDOMAIN and NODATA are answers and +// are returned to the caller to assert on. +func liveQueryNameserver( + t *testing.T, + r *resolver.Resolver, + nameserver string, + hostname string, +) *resolver.NameserverResponse { + t.Helper() + + what := fmt.Sprintf( + "QueryNameserver(%s, %s)", nameserver, hostname, + ) + + var out *resolver.NameserverResponse + + retryLive( + t, + what, + func(ctx context.Context) error { + resp, err := r.QueryNameserver( + ctx, nameserver, hostname, + ) + if err != nil { + return err + } + + if resp.Status == resolver.StatusTimeout || + resp.Status == resolver.StatusError { + return fmt.Errorf( + "%w: %s returned %s: %s", + errLiveNoAnswer, nameserver, + resp.Status, resp.Error, + ) + } + + out = resp + + return nil + }, + ) + + return out +} + +// liveQueryAllNameservers queries every authoritative nameserver +// for a hostname, retrying until a quorum of them has answered. +// Individual nameservers that stay silent are left in the result +// for the caller to account for. +func liveQueryAllNameservers( + t *testing.T, + r *resolver.Resolver, + hostname string, +) map[string]*resolver.NameserverResponse { + t.Helper() + + var out map[string]*resolver.NameserverResponse + + retryLive( + t, + "QueryAllNameservers("+hostname+")", + func(ctx context.Context) error { + results, err := r.QueryAllNameservers(ctx, hostname) + if err != nil { + return err + } + + if len(results) == 0 { + return fmt.Errorf( + "%w: no nameservers queried for %s", + errLiveNoAnswer, hostname, + ) + } + + answered := answeredCount(results) + if answered < liveQuorum(len(results)) { + return fmt.Errorf( + "%w: %d of %d answered: %s", + errLiveNoQuorum, answered, + len(results), describeStatuses(results), + ) + } + + out = results + + return nil + }, + ) + + return out +} + +// liveResolveIPs resolves a hostname that is expected to have +// addresses, retrying until at least one is returned. +func liveResolveIPs( + t *testing.T, + r *resolver.Resolver, + hostname string, +) []string { + t.Helper() + + var out []string + + retryLive( + t, + "ResolveIPAddresses("+hostname+")", + func(ctx context.Context) error { + ips, err := r.ResolveIPAddresses(ctx, hostname) + if err != nil { + return err + } + + if len(ips) == 0 { + return fmt.Errorf( + "%w: no addresses for %s", + errLiveNoAnswer, hostname, + ) + } + + out = ips + + return nil + }, + ) + + return out +} + +// liveResolveIPsAllowingEmpty resolves a hostname that may legitimately +// have no addresses, so the empty result is returned rather than +// retried. Used for names that must not exist; the corresponding +// QueryAllNameservers test is what proves the nameservers actively +// said NXDOMAIN rather than merely staying silent. +func liveResolveIPsAllowingEmpty( + t *testing.T, + r *resolver.Resolver, + hostname string, +) []string { + t.Helper() + + var out []string + + retryLive( + t, + "ResolveIPAddresses("+hostname+")", + func(ctx context.Context) error { + ips, err := r.ResolveIPAddresses(ctx, hostname) + if err != nil { + return err + } + + out = ips + + return nil + }, + ) + + return out +} diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go index bcebfb9..721847e 100644 --- a/internal/resolver/resolver_test.go +++ b/internal/resolver/resolver_test.go @@ -32,32 +32,17 @@ func newTestResolver(t *testing.T) *resolver.Resolver { return resolver.NewFromLogger(log) } -func testContext(t *testing.T) context.Context { - t.Helper() - - ctx, cancel := context.WithTimeout( - context.Background(), 60*time.Second, - ) - t.Cleanup(cancel) - - return ctx -} - +// findOneNSForDomain picks one authoritative nameserver to aim a +// test at. Live-DNS retry, concurrency and quorum handling live in +// livedns_test.go. func findOneNSForDomain( t *testing.T, r *resolver.Resolver, - ctx context.Context, //nolint:revive // test helper domain string, ) string { t.Helper() - nameservers, err := r.FindAuthoritativeNameservers( - ctx, domain, - ) - require.NoError(t, err) - require.NotEmpty(t, nameservers) - - return nameservers[0] + return liveFindAuthoritative(t, r, domain)[0] } // ---------------------------------------------------------------- @@ -70,13 +55,7 @@ func TestFindAuthoritativeNameservers_ValidDomain( t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - - nameservers, err := r.FindAuthoritativeNameservers( - ctx, "google.com", - ) - require.NoError(t, err) - require.NotEmpty(t, nameservers) + nameservers := liveFindAuthoritative(t, r, "google.com") hasGoogleNS := false @@ -99,13 +78,9 @@ func TestFindAuthoritativeNameservers_Subdomain( t.Parallel() r := newTestResolver(t) - ctx := testContext(t) + nameservers := liveFindAuthoritative(t, r, "www.google.com") - nameservers, err := r.FindAuthoritativeNameservers( - ctx, "www.google.com", - ) - require.NoError(t, err) - require.NotEmpty(t, nameservers) + assert.NotEmpty(t, nameservers) } func TestFindAuthoritativeNameservers_ReturnsSorted( @@ -114,12 +89,7 @@ func TestFindAuthoritativeNameservers_ReturnsSorted( t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - - nameservers, err := r.FindAuthoritativeNameservers( - ctx, "google.com", - ) - require.NoError(t, err) + nameservers := liveFindAuthoritative(t, r, "google.com") assert.True( t, @@ -134,17 +104,8 @@ func TestFindAuthoritativeNameservers_Deterministic( t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - - first, err := r.FindAuthoritativeNameservers( - ctx, "google.com", - ) - require.NoError(t, err) - - second, err := r.FindAuthoritativeNameservers( - ctx, "google.com", - ) - require.NoError(t, err) + first := liveFindAuthoritative(t, r, "google.com") + second := liveFindAuthoritative(t, r, "google.com") assert.Equal(t, first, second) } @@ -155,17 +116,8 @@ func TestFindAuthoritativeNameservers_TrailingDot( t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - - ns1, err := r.FindAuthoritativeNameservers( - ctx, "google.com", - ) - require.NoError(t, err) - - ns2, err := r.FindAuthoritativeNameservers( - ctx, "google.com.", - ) - require.NoError(t, err) + ns1 := liveFindAuthoritative(t, r, "google.com") + ns2 := liveFindAuthoritative(t, r, "google.com.") assert.Equal(t, ns1, ns2) } @@ -176,13 +128,7 @@ func TestFindAuthoritativeNameservers_CloudflareDomain( t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - - nameservers, err := r.FindAuthoritativeNameservers( - ctx, "cloudflare.com", - ) - require.NoError(t, err) - require.NotEmpty(t, nameservers) + nameservers := liveFindAuthoritative(t, r, "cloudflare.com") for _, ns := range nameservers { assert.True(t, strings.HasSuffix(ns, "."), @@ -199,13 +145,9 @@ func TestQueryNameserver_BasicA(t *testing.T) { t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - ns := findOneNSForDomain(t, r, ctx, "google.com") + ns := findOneNSForDomain(t, r, "google.com") + resp := liveQueryNameserver(t, r, ns, "www.google.com") - resp, err := r.QueryNameserver( - ctx, ns, "www.google.com", - ) - require.NoError(t, err) require.NotNil(t, resp) assert.Equal(t, resolver.StatusOK, resp.Status) @@ -222,13 +164,8 @@ func TestQueryNameserver_AAAA(t *testing.T) { t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - ns := findOneNSForDomain(t, r, ctx, "cloudflare.com") - - resp, err := r.QueryNameserver( - ctx, ns, "cloudflare.com", - ) - require.NoError(t, err) + ns := findOneNSForDomain(t, r, "cloudflare.com") + resp := liveQueryNameserver(t, r, ns, "cloudflare.com") aaaaRecords := resp.Records["AAAA"] require.NotEmpty(t, aaaaRecords, @@ -247,13 +184,8 @@ func TestQueryNameserver_MX(t *testing.T) { t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - ns := findOneNSForDomain(t, r, ctx, "google.com") - - resp, err := r.QueryNameserver( - ctx, ns, "google.com", - ) - require.NoError(t, err) + ns := findOneNSForDomain(t, r, "google.com") + resp := liveQueryNameserver(t, r, ns, "google.com") mxRecords := resp.Records["MX"] require.NotEmpty(t, mxRecords, @@ -265,13 +197,8 @@ func TestQueryNameserver_TXT(t *testing.T) { t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - ns := findOneNSForDomain(t, r, ctx, "google.com") - - resp, err := r.QueryNameserver( - ctx, ns, "google.com", - ) - require.NoError(t, err) + ns := findOneNSForDomain(t, r, "google.com") + resp := liveQueryNameserver(t, r, ns, "google.com") txtRecords := resp.Records["TXT"] require.NotEmpty(t, txtRecords, @@ -297,14 +224,10 @@ func TestQueryNameserver_NXDomain(t *testing.T) { t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - ns := findOneNSForDomain(t, r, ctx, "google.com") - - resp, err := r.QueryNameserver( - ctx, ns, - "this-surely-does-not-exist-xyz.google.com", + ns := findOneNSForDomain(t, r, "google.com") + resp := liveQueryNameserver( + t, r, ns, "this-surely-does-not-exist-xyz.google.com", ) - require.NoError(t, err) assert.Equal(t, resolver.StatusNXDomain, resp.Status) } @@ -313,13 +236,8 @@ func TestQueryNameserver_RecordsSorted(t *testing.T) { t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - ns := findOneNSForDomain(t, r, ctx, "google.com") - - resp, err := r.QueryNameserver( - ctx, ns, "google.com", - ) - require.NoError(t, err) + ns := findOneNSForDomain(t, r, "google.com") + resp := liveQueryNameserver(t, r, ns, "google.com") for recordType, values := range resp.Records { assert.True( @@ -336,13 +254,8 @@ func TestQueryNameserver_ResponseIncludesNameserver( t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - ns := findOneNSForDomain(t, r, ctx, "cloudflare.com") - - resp, err := r.QueryNameserver( - ctx, ns, "cloudflare.com", - ) - require.NoError(t, err) + ns := findOneNSForDomain(t, r, "cloudflare.com") + resp := liveQueryNameserver(t, r, ns, "cloudflare.com") assert.Equal(t, ns, resp.Nameserver) } @@ -353,14 +266,10 @@ func TestQueryNameserver_EmptyRecordsOnNXDomain( t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - ns := findOneNSForDomain(t, r, ctx, "google.com") - - resp, err := r.QueryNameserver( - ctx, ns, - "this-surely-does-not-exist-xyz.google.com", + ns := findOneNSForDomain(t, r, "google.com") + resp := liveQueryNameserver( + t, r, ns, "this-surely-does-not-exist-xyz.google.com", ) - require.NoError(t, err) totalRecords := 0 for _, values := range resp.Records { @@ -374,18 +283,9 @@ func TestQueryNameserver_TrailingDotHandling(t *testing.T) { t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - ns := findOneNSForDomain(t, r, ctx, "google.com") - - resp1, err := r.QueryNameserver( - ctx, ns, "google.com", - ) - require.NoError(t, err) - - resp2, err := r.QueryNameserver( - ctx, ns, "google.com.", - ) - require.NoError(t, err) + ns := findOneNSForDomain(t, r, "google.com") + resp1 := liveQueryNameserver(t, r, ns, "google.com") + resp2 := liveQueryNameserver(t, r, ns, "google.com.") assert.Equal(t, resp1.Status, resp2.Status) } @@ -398,15 +298,9 @@ func TestQueryAllNameservers_ReturnsAllNS(t *testing.T) { t.Parallel() r := newTestResolver(t) - ctx := testContext(t) + results := liveQueryAllNameservers(t, r, "google.com") - results, err := r.QueryAllNameservers( - ctx, "google.com", - ) - require.NoError(t, err) - require.NotEmpty(t, results) - - assert.GreaterOrEqual(t, len(results), 2) + assert.GreaterOrEqual(t, len(results), minNameservers) for ns, resp := range results { assert.Equal(t, ns, resp.Nameserver) @@ -417,19 +311,27 @@ func TestQueryAllNameservers_AllReturnOK(t *testing.T) { t.Parallel() r := newTestResolver(t) - ctx := testContext(t) + results := liveQueryAllNameservers(t, r, "google.com") - results, err := r.QueryAllNameservers( - ctx, "google.com", + // A quorum, not unanimity: one authoritative server being + // slow or rate-limiting us is a property of the live + // internet, not a resolver defect. + assert.GreaterOrEqual( + t, + countStatus(results, resolver.StatusOK), + liveQuorum(len(results)), + "a quorum of nameservers should answer OK: %s", + describeStatuses(results), ) - require.NoError(t, err) - for ns, resp := range results { - assert.Equal( - t, resolver.StatusOK, resp.Status, - "NS %s should return OK", ns, - ) - } + // Any nameserver claiming google.com does not exist is a + // real failure and is never tolerated. + assert.Zero( + t, + countStatus(results, resolver.StatusNXDomain), + "no nameserver should report NXDOMAIN: %s", + describeStatuses(results), + ) } func TestQueryAllNameservers_NXDomainFromAllNS( @@ -438,20 +340,26 @@ func TestQueryAllNameservers_NXDomainFromAllNS( t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - - results, err := r.QueryAllNameservers( - ctx, - "this-surely-does-not-exist-xyz.google.com", + results := liveQueryAllNameservers( + t, r, "this-surely-does-not-exist-xyz.google.com", ) - require.NoError(t, err) - for ns, resp := range results { - assert.Equal( - t, resolver.StatusNXDomain, resp.Status, - "NS %s should return nxdomain", ns, - ) - } + assert.GreaterOrEqual( + t, + countStatus(results, resolver.StatusNXDomain), + liveQuorum(len(results)), + "a quorum of nameservers should report NXDOMAIN: %s", + describeStatuses(results), + ) + + // Silence is tolerated; a positive answer for a name that + // does not exist is not. + assert.Zero( + t, + countStatus(results, resolver.StatusOK), + "no nameserver should answer OK: %s", + describeStatuses(results), + ) } // ---------------------------------------------------------------- @@ -462,11 +370,7 @@ func TestLookupNS_ValidDomain(t *testing.T) { t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - - nameservers, err := r.LookupNS(ctx, "google.com") - require.NoError(t, err) - require.NotEmpty(t, nameservers) + nameservers := liveLookupNS(t, r, "google.com") for _, ns := range nameservers { assert.True(t, strings.HasSuffix(ns, "."), @@ -479,10 +383,7 @@ func TestLookupNS_Sorted(t *testing.T) { t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - - nameservers, err := r.LookupNS(ctx, "google.com") - require.NoError(t, err) + nameservers := liveLookupNS(t, r, "google.com") assert.True(t, sort.StringsAreSorted(nameservers)) } @@ -491,15 +392,8 @@ func TestLookupNS_MatchesFindAuthoritative(t *testing.T) { t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - - fromLookup, err := r.LookupNS(ctx, "google.com") - require.NoError(t, err) - - fromFind, err := r.FindAuthoritativeNameservers( - ctx, "google.com", - ) - require.NoError(t, err) + fromLookup := liveLookupNS(t, r, "google.com") + fromFind := liveFindAuthoritative(t, r, "google.com") assert.Equal(t, fromFind, fromLookup) } @@ -512,11 +406,7 @@ func TestResolveIPAddresses_ReturnsIPs(t *testing.T) { t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - - ips, err := r.ResolveIPAddresses(ctx, "google.com") - require.NoError(t, err) - require.NotEmpty(t, ips) + ips := liveResolveIPs(t, r, "google.com") for _, ip := range ips { parsed := net.ParseIP(ip) @@ -530,10 +420,7 @@ func TestResolveIPAddresses_Deduplicated(t *testing.T) { t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - - ips, err := r.ResolveIPAddresses(ctx, "google.com") - require.NoError(t, err) + ips := liveResolveIPs(t, r, "google.com") seen := make(map[string]bool) @@ -547,10 +434,7 @@ func TestResolveIPAddresses_Sorted(t *testing.T) { t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - - ips, err := r.ResolveIPAddresses(ctx, "google.com") - require.NoError(t, err) + ips := liveResolveIPs(t, r, "google.com") assert.True(t, sort.StringsAreSorted(ips)) } @@ -561,13 +445,10 @@ func TestResolveIPAddresses_NXDomainReturnsEmpty( t.Parallel() r := newTestResolver(t) - ctx := testContext(t) - - ips, err := r.ResolveIPAddresses( - ctx, - "this-surely-does-not-exist-xyz.google.com", + ips := liveResolveIPsAllowingEmpty( + t, r, "this-surely-does-not-exist-xyz.google.com", ) - require.NoError(t, err) + assert.Empty(t, ips) } @@ -575,11 +456,9 @@ func TestResolveIPAddresses_CloudflareDomain(t *testing.T) { t.Parallel() r := newTestResolver(t) - ctx := testContext(t) + ips := liveResolveIPs(t, r, "cloudflare.com") - ips, err := r.ResolveIPAddresses(ctx, "cloudflare.com") - require.NoError(t, err) - require.NotEmpty(t, ips) + assert.NotEmpty(t, ips) } // ---------------------------------------------------------------- diff --git a/script/test b/script/test index 50b1730..568e3f5 100755 --- a/script/test +++ b/script/test @@ -6,7 +6,7 @@ ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" main() { cd "$ROOT" - go test -v -race -timeout 30s -cover ./... + go test -v -race -timeout 90s -cover ./... } main "$@" -- 2.54.0 From 87bce43f8d7b67f50fa210dd89165f48bf506680 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 10 Aug 2026 13:33:26 +0000 Subject: [PATCH 03/15] =?UTF-8?q?test:=20rework=20live-DNS=20quorum=20unit?= =?UTF-8?q?=20=E2=80=94=20tolerate=20silence,=20never=20a=20wrong=20answer?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rework of the unit at https://git.eeqj.de/sneak/dnswatcher/issues/93 (commit 9cb2c2b), against the review at https://git.eeqj.de/sneak/dnswatcher/pulls/136#issuecomment-53869. Review of 9cb2c2b found the quorum assertions could not fail on a class of wrong answer. Each test banned exactly one bad status — _AllReturnOK banned only nxdomain, _NXDomainFromAllNS banned only ok — so resolver.StatusNoData passed both. nodata is a wrong answer, not silence, and answeredCount counted it as answered, so it did not even trigger a retry; with a quorum of 3 of 4 a single wrong nameserver slid through undetected. That is assertion-loosening beyond what the quorum change requires. Tolerance is now a closed allowlist rather than a blocklist of one status. unsanctionedStatuses() reports every per-nameserver result whose status the caller did not explicitly sanction: ok/timeout/error for the all-OK test, nxdomain/timeout/error for the NXDOMAIN test. Silence (timeout, error) is the only thing quorum exists to tolerate; any other status, including one added to the resolver later, fails by name. answeredCount is likewise an allowlist of ok/nxdomain/nodata, so an unknown status counts as silence and can only cause a retry and then a loud failure, never a quiet pass. Two harness tests cover the regression directly: three OK plus one nodata (quorum satisfied, no nxdomain present — the input that used to pass) is now reported as unsanctioned, and an unknown status is neither counted as answered nor tolerated. Verified by re-running the reviewer's probe: queryEachNS patched to force one of google.com's four nameservers to return StatusNoData turns both tests red, naming the offending nameserver and status — --- FAIL: TestQueryAllNameservers_AllReturnOK (1.12s) Should be empty, but was [ns1.google.com.=nodata] every nameserver must answer OK or not answer at all: ns1.google.com.=nodata ns2.google.com.=ok ns3.google.com.=ok ns4.google.com.=ok --- FAIL: TestQueryAllNameservers_NXDomainFromAllNS (1.34s) Should be empty, but was [ns1.google.com.=nodata] every nameserver must report NXDOMAIN or not answer at all: ns1.google.com.=nodata ns2.google.com.=nxdomain ns3.google.com.=nxdomain ns4.google.com.=nxdomain — and green with the probe reverted. Also fixes the review's nit: the per-attempt deadline assertion had no lower bound, so it passed for a deadline far shorter than intended. No production code changed; DNS is still never mocked. --- TODO.md | 7 +- internal/resolver/livedns_harness_test.go | 144 ++++++++++++++++++++-- internal/resolver/livedns_test.go | 64 +++++++++- internal/resolver/resolver_test.go | 37 ++++-- 4 files changed, 227 insertions(+), 25 deletions(-) diff --git a/TODO.md b/TODO.md index 1b4f13f..8905768 100644 --- a/TODO.md +++ b/TODO.md @@ -30,7 +30,12 @@ confirm make check still passes. `internal/resolver/livedns_test.go` adds a package-wide concurrency gate (so parallel tests stop bursting at the first root server), retry with exponential backoff on transport failures only, and - quorum instead of unanimity for multi-nameserver assertions. The + quorum instead of unanimity for multi-nameserver assertions. Quorum + tolerates silence only: every per-nameserver status must be in a + closed allowlist (`ok`/`timeout`/`error`, or + `nxdomain`/`timeout`/`error`), so a wrong answer from a minority — + `nodata` today, any status added later — fails the test instead of + sliding through under the majority. The `make test` cap moved to the new org-wide 60s hard cap / 20s target with a 90s `-timeout` backstop; `REPO_POLICIES.md` re-vendored byte-identical from `sneak/prompts`. No mocks, no `-short`, no build diff --git a/internal/resolver/livedns_harness_test.go b/internal/resolver/livedns_harness_test.go index 066337d..87ce704 100644 --- a/internal/resolver/livedns_harness_test.go +++ b/internal/resolver/livedns_harness_test.go @@ -16,6 +16,16 @@ import ( // perform no DNS resolution of any kind, so they neither mock DNS // nor depend on it. +// Names for the synthetic status maps below. Nothing is ever queried +// at them: they are map keys handed to the package's pure counting +// helpers, not a stand-in for a nameserver. +const ( + nsExample1 = "ns1.example." + nsExample2 = "ns2.example." + nsExample3 = "ns3.example." + nsExample4 = "ns4.example." +) + func TestLiveQuorumIsStrictMajority(t *testing.T) { t.Parallel() @@ -41,20 +51,20 @@ func TestStatusCountingIgnoresSilentNameservers(t *testing.T) { t.Parallel() results := map[string]*resolver.NameserverResponse{ - "ns1.example.": { - Nameserver: "ns1.example.", + nsExample1: { + Nameserver: nsExample1, Status: resolver.StatusOK, }, - "ns2.example.": { - Nameserver: "ns2.example.", + nsExample2: { + Nameserver: nsExample2, Status: resolver.StatusOK, }, - "ns3.example.": { - Nameserver: "ns3.example.", + nsExample3: { + Nameserver: nsExample3, Status: resolver.StatusTimeout, }, - "ns4.example.": { - Nameserver: "ns4.example.", + nsExample4: { + Nameserver: nsExample4, Status: resolver.StatusError, }, } @@ -106,14 +116,126 @@ func TestRetryLiveGivesEachAttemptADeadline(t *testing.T) { retryLive(t, "deadline", func(ctx context.Context) error { deadline, ok := ctx.Deadline() assert.True(t, ok, "attempt should carry a deadline") - assert.LessOrEqual( - t, time.Until(deadline), liveAttemptTimeout, - ) + + remaining := time.Until(deadline) + + assert.LessOrEqual(t, remaining, liveAttemptTimeout) + + // Lower bound too: without one this passes for a + // deadline far shorter than intended, which would + // silently turn every live attempt into an instant + // timeout. + assert.Greater(t, remaining, liveAttemptTimeout/2) return nil }) } +// TestUnsanctionedStatusesRejectsWrongAnswers is the regression test +// for the defect this allowlist exists to prevent: a minority of +// nameservers answering WRONGLY while quorum keeps the suite green. +// nodata is the case that motivated it — it is a wrong answer, not +// silence, and it was previously banned by neither test. +func TestUnsanctionedStatusesRejectsWrongAnswers(t *testing.T) { + t.Parallel() + + // Four nameservers, three OK and one answering nodata: a + // quorum of three is satisfied and no NXDOMAIN is present, so + // the old blocklist assertions both passed on this input. + results := map[string]*resolver.NameserverResponse{ + nsExample1: { + Nameserver: nsExample1, + Status: resolver.StatusOK, + }, + nsExample2: { + Nameserver: nsExample2, + Status: resolver.StatusOK, + }, + nsExample3: { + Nameserver: nsExample3, + Status: resolver.StatusOK, + }, + nsExample4: { + Nameserver: nsExample4, + Status: resolver.StatusNoData, + }, + } + + assert.GreaterOrEqual( + t, + countStatus(results, resolver.StatusOK), + liveQuorum(len(results)), + ) + assert.Zero(t, countStatus(results, resolver.StatusNXDomain)) + + // nodata is an ANSWER, so it never triggers a retry: nothing + // but the allowlist stands between it and a false green. + assert.Equal(t, len(results), answeredCount(results)) + + assert.Equal( + t, + []string{nsExample4 + "=nodata"}, + unsanctionedStatuses( + results, + resolver.StatusOK, + resolver.StatusTimeout, + resolver.StatusError, + ), + "nodata must be reported as an unsanctioned status", + ) +} + +func TestUnsanctionedStatusesToleratesSilenceOnly(t *testing.T) { + t.Parallel() + + results := map[string]*resolver.NameserverResponse{ + nsExample1: { + Nameserver: nsExample1, + Status: resolver.StatusNXDomain, + }, + nsExample2: { + Nameserver: nsExample2, + Status: resolver.StatusTimeout, + }, + nsExample3: { + Nameserver: nsExample3, + Status: resolver.StatusError, + }, + } + + allowed := []string{ + resolver.StatusNXDomain, + resolver.StatusTimeout, + resolver.StatusError, + } + + assert.Empty( + t, + unsanctionedStatuses(results, allowed...), + "timeout and error are non-answers and are tolerated", + ) + + // The same silent nameservers do not count towards a quorum. + assert.Equal(t, 1, answeredCount(results)) + + // An unknown status is treated as silence by answeredCount — + // so it retries and fails loudly — and is unsanctioned by the + // allowlist rather than quietly permitted. + const laterStatus = "some-status-added-later" + + results[nsExample4] = &resolver.NameserverResponse{ + Nameserver: nsExample4, + Status: laterStatus, + } + + assert.Equal(t, 1, answeredCount(results)) + assert.Equal( + t, + []string{nsExample4 + "=" + laterStatus}, + unsanctionedStatuses(results, allowed...), + ) +} + func TestRunLiveBoundsConcurrency(t *testing.T) { t.Parallel() diff --git a/internal/resolver/livedns_test.go b/internal/resolver/livedns_test.go index 55ea755..b864460 100644 --- a/internal/resolver/livedns_test.go +++ b/internal/resolver/livedns_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "slices" "sort" "strings" "testing" @@ -44,6 +45,15 @@ import ( // nameservers, a strict majority answering as expected is // enough; a server that fails to answer is tolerated, while a // server that answers *wrongly* still fails the test. +// +// The tolerance in (3) is expressed as an ALLOWLIST of sanctioned +// statuses, never as a blocklist of known-bad ones. A blocklist bans +// the one wrong answer its author thought of and silently admits +// every other status, including any added to the resolver later; an +// allowlist fails on anything nobody explicitly sanctioned. Silence +// (timeout, error) is the only thing quorum exists to tolerate. A +// *wrong answer* — nxdomain for a name that exists, ok for one that +// does not, nodata for either — is never tolerated at any count. const ( // liveAttempts is how many times a live DNS operation is @@ -172,14 +182,62 @@ func countStatus( return n } +// liveAnswerStatuses is the closed set of statuses that count as a +// nameserver having ANSWERED at all, whether or not the test agrees +// with the answer. It is deliberately an allowlist: a status added +// to the resolver later is treated as silence, so it can only ever +// cause a retry and then a loud failure, never a quiet pass. +func liveAnswerStatuses() []string { + return []string{ + resolver.StatusOK, + resolver.StatusNXDomain, + resolver.StatusNoData, + } +} + // answeredCount counts the nameservers that produced an answer of // any kind, as opposed to failing or timing out. func answeredCount( results map[string]*resolver.NameserverResponse, ) int { - return len(results) - - countStatus(results, resolver.StatusError) - - countStatus(results, resolver.StatusTimeout) + answers := liveAnswerStatuses() + + n := 0 + + for _, resp := range results { + if slices.Contains(answers, resp.Status) { + n++ + } + } + + return n +} + +// unsanctionedStatuses returns "nameserver=status" for every result +// whose status the caller did not explicitly sanction, sorted for a +// stable failure message. Callers pass the full closed set they will +// accept — the expected answer plus whichever non-answers (timeout, +// error) quorum is allowed to tolerate — so that any status outside +// it fails the test by name. +func unsanctionedStatuses( + results map[string]*resolver.NameserverResponse, + allowed ...string, +) []string { + offenders := make([]string, 0, len(results)) + + for ns, resp := range results { + if slices.Contains(allowed, resp.Status) { + continue + } + + offenders = append( + offenders, fmt.Sprintf("%s=%s", ns, resp.Status), + ) + } + + sort.Strings(offenders) + + return offenders } // describeStatuses renders per-nameserver statuses for use in diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go index 721847e..13bdcde 100644 --- a/internal/resolver/resolver_test.go +++ b/internal/resolver/resolver_test.go @@ -324,12 +324,21 @@ func TestQueryAllNameservers_AllReturnOK(t *testing.T) { describeStatuses(results), ) - // Any nameserver claiming google.com does not exist is a - // real failure and is never tolerated. - assert.Zero( + // Quorum tolerates SILENCE only. Every individual result must + // be either the expected answer or a non-answer: ok, timeout + // or error, and nothing else. Stated as a closed allowlist so + // that a wrong answer no one thought to ban — nxdomain and + // nodata today, any status added later — fails here rather + // than sliding through under the quorum. + assert.Empty( t, - countStatus(results, resolver.StatusNXDomain), - "no nameserver should report NXDOMAIN: %s", + unsanctionedStatuses( + results, + resolver.StatusOK, + resolver.StatusTimeout, + resolver.StatusError, + ), + "every nameserver must answer OK or not answer at all: %s", describeStatuses(results), ) } @@ -352,12 +361,20 @@ func TestQueryAllNameservers_NXDomainFromAllNS( describeStatuses(results), ) - // Silence is tolerated; a positive answer for a name that - // does not exist is not. - assert.Zero( + // Silence is tolerated; any actual answer other than NXDOMAIN + // is not. Closed allowlist for the same reason as above: a + // server answering `ok` or `nodata` for a name that must not + // exist is a wrong answer, not a slow one. + assert.Empty( t, - countStatus(results, resolver.StatusOK), - "no nameserver should answer OK: %s", + unsanctionedStatuses( + results, + resolver.StatusNXDomain, + resolver.StatusTimeout, + resolver.StatusError, + ), + "every nameserver must report NXDOMAIN or not answer "+ + "at all: %s", describeStatuses(results), ) } -- 2.54.0 From 6f6bf3a65bbe3cf0ea42dd256a62a0b7e6f27796 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 10 Aug 2026 13:48:15 +0000 Subject: [PATCH 04/15] test: disable Go's test cache so every run queries live DNS (closes #139) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `script/test` did not pass `-count=1`, so on an unchanged tree Go served the whole suite from its test cache: exit 0 in ~0.2s with every package marked `(cached)` and not one DNS query made. This repo's suite exists to exercise live resolution on every run (`TESTING.md`), so that green asserted nothing — and it is exactly the green used as evidence that a flakiness fix works, since "run it a few times" stops being runs after the first. `-count=1` now disables caching on every invocation. The conditional verbose rerun that `REPO_POLICIES.md` mandates was missing at the same spot and is added here rather than left broken: the primary run had been unconditionally `-v`, which is the failure mode the policy exists to prevent (unreadable CI and `docker build` logs on success). Tests now run quiet, and only a failure triggers the `-v` rerun. The rerun carries `-count=1` too, so it cannot replay a cached copy of the failure it is meant to diagnose, and its exit status is discarded in favour of a forced 1: the first failure already proved the suite broken, so a flake that passes the second time must not turn the build green. `-timeout 90s` is untouched. It is a deliberate backstop that must strictly exceed the 60s hard cap on suite duration. No special-casing for the Docker build, which also reaches this script via `RUN make test`: a fresh container's test cache is empty, so `-count=1` changes nothing there and carving out an exception would only create a second code path that could drift. Verified: three back-to-back `make test` runs on an unchanged tree, zero `(cached)` markers, ~4.0-4.5s wall each (was ~0.2s cached), comfortably inside the 20s target with `-race` and `-cover` both still working and coverage percentages unchanged. The rerun-and-still-fail path was exercised against a purpose-built flaky test that fails once then passes: quiet failure, verbose rerun that genuinely re-executed, exit 1 regardless. `make check` green. --- README.md | 6 +++++- TESTING.md | 3 +++ TODO.md | 12 ++++++++++++ script/test | 25 ++++++++++++++++++++++++- 4 files changed, 44 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 96141f9..8a5e580 100644 --- a/README.md +++ b/README.md @@ -387,7 +387,11 @@ them. We provide: plus the git pre-commit hook - `script/projectname` — print the project name (used for the Docker image tag) -- `script/test` — run the test suite (race detector, coverage) +- `script/test` — run the test suite (race detector, coverage). Caching + is waived for testing, exactly as it is for linting: `-count=1` + forces every invocation to execute, because the suite queries live + DNS and a cached pass queries nothing. Failures are rerun with `-v` + automatically, and the build fails even if that rerun passes. - `script/lint` — run golangci-lint, always inside Docker: it builds `Dockerfile.lint`, which COPYs the repo into the digest-pinned `golangci-lint` image and lints as a build step, so a successful diff --git a/TESTING.md b/TESTING.md index 0acae53..659db13 100644 --- a/TESTING.md +++ b/TESTING.md @@ -31,4 +31,7 @@ real servers ensures the resolver works correctly in production. 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 remove `-count=1` from `script/test`** — Go's test cache + replays a previous run's output without querying anything, so a + cached pass is not evidence that live resolution works - **Do not modify linter configuration** to suppress findings diff --git a/TODO.md b/TODO.md index 8905768..97286e7 100644 --- a/TODO.md +++ b/TODO.md @@ -25,6 +25,18 @@ confirm make check still passes. # Completed Steps +- 2026-08-10: Go's test cache disabled for `script/test` via `-count=1`, + so every invocation actually executes. A cached pass replays an + earlier run's output without querying DNS at all, which in this repo + means the suite's entire premise goes unexercised while the run + reports green in under a second. The conditional verbose rerun that + `REPO_POLICIES.md` mandates was added at the same time (the primary + run had been unconditionally `-v`): quiet first, `-v` only on + failure, `-count=1` on both, and exit 1 forced regardless of the + rerun's result so a flake passing the second time cannot turn the + build green. `-timeout 90s` left alone as the deliberate backstop + above the 60s hard cap. Uncached suite runs ~4s, well inside the 20s + target - 2026-08-10: live-DNS test flakiness addressed by robustness rather than gating, per the owner's ruling on #93: new `internal/resolver/livedns_test.go` adds a package-wide concurrency diff --git a/script/test b/script/test index 568e3f5..b4ef492 100755 --- a/script/test +++ b/script/test @@ -1,12 +1,35 @@ #!/bin/sh # script/test: run the test suite. +# +# -count=1 disables Go's test cache, and is load-bearing here. This +# suite queries live DNS on every run by policy (TESTING.md); a cached +# result is a replay of an earlier run's output with no query made at +# all. On an unchanged tree the whole suite would return success in +# under a second having resolved nothing, which makes the repeated-run +# green that is used as evidence for flakiness fixes worthless. Do not +# remove it. +# +# Conditional verbose rerun per REPO_POLICIES.md: run quiet first so +# CI and docker build logs stay readable, and rerun with -v only on +# failure. The rerun also carries -count=1 (a cached replay of the +# failure would show nothing new), and the exit status is forced to 1 +# no matter how the rerun ends: the first failure already proved the +# suite broken, so a flaky test that passes the second time must not +# turn the build green. +# +# -timeout 90s is a deliberate backstop above the 60s hard cap on +# suite duration. Do not lower it. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" main() { cd "$ROOT" - go test -v -race -timeout 90s -cover ./... + go test -count=1 -race -timeout 90s -cover ./... || { + echo "--- Rerunning with -v for details ---" >&2 + go test -count=1 -race -timeout 90s -v ./... || true + exit 1 + } } main "$@" -- 2.54.0 From 168281ad60d488ab65bfc671ecd3816195a728c8 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 10 Aug 2026 14:04:31 +0000 Subject: [PATCH 05/15] docs: add MIT LICENSE and state the licence in the README (closes #102) The repository had no licence file at all, which makes publicly readable code all-rights-reserved by default: nobody may legally use it. That is a 1.0 blocker rather than a nicety, and `LICENSE` was the only file from `REPO_POLICIES.md`'s required minimum still missing here. The choice is standing org policy rather than a per-repo call: any public repo lacking a licence gets MIT, while a private repo with no licence is already all-rights-reserved and needs nothing. `sneak/dnswatcher` is public, so MIT. `LICENSE` carries the canonical MIT text byte-for-byte with only the copyright line filled in; no clauses added, removed, reworded, or reflowed. `README.md`'s first line now names the licence, which the Description requirement in `REPO_POLICIES.md` calls for, and the License section states MIT and points at the file instead of recording the decision as pending. --- LICENSE | 21 +++++++++++++++++++++ README.md | 7 ++++--- TODO.md | 26 ++++++++++++++++++++------ 3 files changed, 45 insertions(+), 9 deletions(-) create mode 100644 LICENSE diff --git a/LICENSE b/LICENSE new file mode 100644 index 0000000..34edefe --- /dev/null +++ b/LICENSE @@ -0,0 +1,21 @@ +MIT License + +Copyright (c) 2026 sneak + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is +furnished to do so, subject to the following conditions: + +The above copyright notice and this permission notice shall be included in all +copies or substantial portions of the Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +SOFTWARE. diff --git a/README.md b/README.md index 8a5e580..3f5d5bf 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # dnswatcher -dnswatcher is a pre-1.0 Go daemon by [@sneak](https://sneak.berlin) that monitors DNS records, TCP port availability, and TLS certificates, delivering real-time change notifications via Slack, Mattermost, and ntfy webhooks. +dnswatcher is an MIT-licensed, pre-1.0 Go daemon by [@sneak](https://sneak.berlin) that monitors DNS records, TCP port availability, and TLS certificates, delivering real-time change notifications via Slack, Mattermost, and ntfy webhooks. > ⚠️ Pre-1.0 software. APIs, configuration, and behavior may change without notice. @@ -486,8 +486,9 @@ Viper for configuration. ## License -License has not yet been chosen for this project. Pending decision by the -author (MIT, GPL, or WTFPL). +dnswatcher is released under the MIT License, Copyright (c) 2026 +[@sneak](https://sneak.berlin). See the [`LICENSE`](./LICENSE) file in the +repository root for the full text. ## Author diff --git a/TODO.md b/TODO.md index 97286e7..b44e872 100644 --- a/TODO.md +++ b/TODO.md @@ -18,13 +18,29 @@ iterative resolver implementation with hermetic mocked tests. # Next Step -Policy scaffold commit: add LICENSE, REPO_POLICIES.md, .editorconfig, -.dockerignore, and .gitea/workflows/check.yml, and add the missing -fmt-check, docker, and hooks targets to the Makefile. One commit, then -confirm make check still passes. +Add the README sections required by policy (Description, Getting Started, +Rationale, Design, TODO, License, Author) if any are still missing. # Completed Steps +- 2026-08-10: MIT `LICENSE` added at the repository root, closing the + last gap in `REPO_POLICIES.md`'s required-minimum file list and + removing the all-rights-reserved default that would otherwise have + shipped with a 1.0 tag. The licence choice is the standing org policy + (any public repo lacking a licence gets MIT; a private repo with no + licence is already all-rights-reserved), and this repo is public. The + file holds the canonical MIT text byte-for-byte with only the + copyright line filled in (`Copyright (c) 2026 sneak`); no clauses were + added, removed, or reflowed. `README.md`'s first line now names the + licence, as the Description requirement demands, and the License + section states MIT and points at the file instead of saying the choice + is pending. `make fmt` covers only Go sources (`gofmt -s`, + `goimports`), so it cannot reflow `LICENSE` +- 2026-08-10: the policy scaffold (`REPO_POLICIES.md`, `.editorconfig`, + `.dockerignore`, `.gitea/workflows/check.yml`, and the `fmt-check`, + `docker`, and hooks Makefile targets) is present; it landed piecemeal + across the scripts-to-rule-them-all and policy commits rather than as + the single commit this file once planned - 2026-08-10: Go's test cache disabled for `script/test` via `-count=1`, so every invocation actually executes. A cached pass replays an earlier run's output without querying DNS at all, which in this repo @@ -90,8 +106,6 @@ confirm make check still passes. Compliance: -- Add README sections required by policy (Description, Getting Started, - Rationale, Design, TODO, License, Author) if any are missing - Pin Dockerfile base images by sha256 and ensure the Docker build runs make check -- 2.54.0 From b8662b8a9c03cb79ea7a5d68e2a3609086cede85 Mon Sep 17 00:00:00 2001 From: sneak Date: Mon, 10 Aug 2026 14:09:20 +0000 Subject: [PATCH 06/15] docs: correct stale script headers and record the config-verify cost (closes #137) script/bootstrap credited the goimports pin to script/fmt-check, which runs gofmt only; the header now credits script/fmt. script/cibuild still described the Dockerfile as running make check, which stopped being true once linting moved to its own stage. The docker-missing warning in script/bootstrap is one sentence instead of three fragments each carrying the bootstrap: prefix. Dockerfile.lint now states the residual risk of skipping golangci-lint config verify: unknown top-level keys in .golangci.yml are ignored silently, so a mistyped key lints clean and applies nothing. Comment and message text only; no behaviour changes. --- Dockerfile.lint | 4 +++- TODO.md | 14 ++++++++++++++ script/bootstrap | 8 ++++---- script/cibuild | 5 +++-- 4 files changed, 24 insertions(+), 7 deletions(-) diff --git a/Dockerfile.lint b/Dockerfile.lint index 5c236de..4dcb14e 100644 --- a/Dockerfile.lint +++ b/Dockerfile.lint @@ -6,7 +6,9 @@ # # `golangci-lint config verify` is deliberately NOT run here: it # fetches its JSON schema over a live, unpinned HTTPS call, which would -# make linting network-dependent and defeat hash-pinning. +# make linting network-dependent and defeat hash-pinning. The cost of +# that: unknown top-level keys in .golangci.yml are silently ignored, +# so a mistyped or wrong-schema key lints clean while applying nothing. # # golangci/golangci-lint:v2.12.2 (Debian-based), 2026-08-10 FROM golangci/golangci-lint:v2.12.2@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240 AS deps diff --git a/TODO.md b/TODO.md index b44e872..7bf50f3 100644 --- a/TODO.md +++ b/TODO.md @@ -23,6 +23,20 @@ Rationale, Design, TODO, License, Author) if any are still missing. # Completed Steps +- 2026-08-10: comment-only corrections to `script/bootstrap`, + `script/cibuild`, and `Dockerfile.lint`. The `goimports` pin in + `script/bootstrap` was justified by a claim that `script/fmt-check` + runs it on the host; it does not (it runs `gofmt -l .` only), so the + header now credits `script/fmt` alone. `script/cibuild` still claimed + the `Dockerfile` runs `make check`, which stopped being true when + linting moved to its own stage; it now describes the lint stage + (`make fmt-check` plus `golangci-lint`) and the builder stage + (`make test`, `make build`). The `docker`-missing warning in + `script/bootstrap` reads as one sentence instead of three fragments + each re-prefixed with `bootstrap:`. `Dockerfile.lint` now records the + residual risk of omitting `golangci-lint config verify`: unknown + top-level keys in `.golangci.yml` are silently ignored, so a mistyped + key lints clean while applying nothing. No behaviour changed - 2026-08-10: MIT `LICENSE` added at the repository root, closing the last gap in `REPO_POLICIES.md`'s required-minimum file list and removing the all-rights-reserved default that would otherwise have diff --git a/script/bootstrap b/script/bootstrap index 0901c87..06da5d0 100755 --- a/script/bootstrap +++ b/script/bootstrap @@ -4,7 +4,8 @@ # installed tools are skipped. Base tooling comes from nix, apt, brew, # or apk (detected in that order); assumes nothing is present. # goimports is installed via `go install` at a pinned commit (never -# "latest") because script/fmt and script/fmt-check run it on the host. +# "latest") because script/fmt runs it on the host; script/fmt-check +# does not (it runs gofmt only). # The linter is NOT installed here: golangci-lint runs via docker only # (script/lint), pinned by image digest, so its only prerequisite is a # working docker. @@ -77,9 +78,8 @@ main() { # Linting runs via docker only (script/lint). Warn, don't fail: # everything except `make lint` works without it. if missing docker; then - echo "bootstrap: WARNING: docker not found; make lint and" >&2 - echo "bootstrap: make docker require it. Install docker to" >&2 - echo "bootstrap: run the linter." >&2 + echo "bootstrap: WARNING: docker not found; install it to" \ + "run make lint and make docker." >&2 fi go mod download diff --git a/script/cibuild b/script/cibuild index 966f51d..1b9e57d 100755 --- a/script/cibuild +++ b/script/cibuild @@ -1,6 +1,7 @@ #!/bin/sh -# script/cibuild: run the CI build. The Dockerfile runs make check, so -# a successful build implies all checks pass. +# script/cibuild: run the CI build. The Dockerfile's lint stage runs +# make fmt-check and golangci-lint; its builder stage runs make test +# and make build. A successful build implies all of those passed. set -eu ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" -- 2.54.0 From ae06f7e3a1b2fafa1769b276834c3489b6e53183 Mon Sep 17 00:00:00 2001 From: clawbot Date: Wed, 9 Sep 2026 14:26:53 +0200 Subject: [PATCH 07/15] notify: drain in-flight deliveries at shutdown (closes #106) Shutdown waits for deliveries already in flight instead of dropping them. Reviewed and green; squashed to next by the dispatcher, dnswatcher having no manager. Model: opus-5 --- README.md | 13 +- TODO.md | 9 + internal/notify/export_test.go | 26 +- internal/notify/notify.go | 145 +++++---- internal/notify/retry.go | 9 + internal/notify/shutdown.go | 119 +++++++ internal/notify/shutdown_test.go | 531 +++++++++++++++++++++++++++++++ 7 files changed, 780 insertions(+), 72 deletions(-) create mode 100644 internal/notify/shutdown.go create mode 100644 internal/notify/shutdown_test.go diff --git a/README.md b/README.md index 3f5d5bf..9e854ff 100644 --- a/README.md +++ b/README.md @@ -218,7 +218,8 @@ internal/ - **Structured logging**: All logs use `log/slog` with JSON output in production (TTY detection for development). - **Graceful shutdown**: All background goroutines respect context - cancellation and the fx lifecycle. + cancellation and the fx lifecycle. In-flight notification deliveries + are drained on shutdown, bounded by the shutdown timeout. --- @@ -463,8 +464,14 @@ docker run -d \ from a previous cycle. 4. **On change detection**: Send notifications to all configured endpoints, update in-memory state, persist to disk. -5. **Shutdown**: Persist final state to disk, complete in-flight - notifications, stop gracefully. +5. **Shutdown**: Persist final state to disk, wait for in-flight + notification deliveries to complete, stop gracefully. The wait is + bounded by the fx shutdown timeout (15s by default): deliveries still + retrying against an unreachable endpoint when that expires are + abandoned, and the number abandoned is logged at warn level rather + than dropped silently. Notifications generated after shutdown has + begun are refused and logged, so a late burst cannot extend the + shutdown. --- diff --git a/TODO.md b/TODO.md index 7bf50f3..91ea4e5 100644 --- a/TODO.md +++ b/TODO.md @@ -92,6 +92,15 @@ Rationale, Design, TODO, License, Author) if any are still missing. `make check` into `script/lint`. `golangci-lint config verify` is deliberately omitted: it fetches its schema over an unpinned live HTTPS call +- 2026-08-09: in-flight notification deliveries are now drained at + shutdown (#106): `notify.New` registers an fx `OnStop` hook that waits + on a `sync.WaitGroup` of tracked delivery goroutines, bounded by the + `OnStop` context; on expiry the outstanding count is logged at warn + level and parked retry backoffs are released instead of being dropped + silently, and deliveries submitted after the drain begins are refused + so shutdown cannot be extended indefinitely; an `OnStop` context that + is already expired on entry with nothing outstanding drains quietly + rather than warning about deliveries that were never abandoned - 2026-08-07: golangci-lint bumped to v2.12.2 (commit-pinned installs in `Dockerfile` and `script/bootstrap`); `.golangci.yml` set to the org-standard v2-schema config used across the org's repos diff --git a/internal/notify/export_test.go b/internal/notify/export_test.go index ae6818d..723fee6 100644 --- a/internal/notify/export_test.go +++ b/internal/notify/export_test.go @@ -32,11 +32,27 @@ func NewRequestForTest( // NewTestService creates a Service suitable for unit testing. // It discards log output and uses the given transport. func NewTestService(transport http.RoundTripper) *Service { - return &Service{ - log: slog.New(slog.DiscardHandler), - transport: transport, - history: NewAlertHistory(), - } + return newService(slog.New(slog.DiscardHandler), transport) +} + +// NewTestServiceWithLogger creates a Service that writes to the +// given handler, so tests can assert on emitted log records. +func NewTestServiceWithLogger( + transport http.RoundTripper, + handler slog.Handler, +) *Service { + return newService(slog.New(handler), transport) +} + +// Drain exports drain for testing. +func (svc *Service) Drain(ctx context.Context) { + svc.drain(ctx) +} + +// OutstandingDeliveries reports how many delivery goroutines +// are currently tracked as in flight. +func (svc *Service) OutstandingDeliveries() int64 { + return svc.outstanding.Load() } // SetNtfyURL sets the ntfy URL on a Service for testing. diff --git a/internal/notify/notify.go b/internal/notify/notify.go index 878b912..1185ed6 100644 --- a/internal/notify/notify.go +++ b/internal/notify/notify.go @@ -12,6 +12,8 @@ import ( "log/slog" "net/http" "net/url" + "sync" + "sync/atomic" "time" "go.uber.org/fx" @@ -115,19 +117,41 @@ type Service struct { history *AlertHistory retryConfig RetryConfig sleepFn func(time.Duration) <-chan time.Time + + // Shutdown draining state. drainMu guards draining and + // serialises it against the counter increment in + // startDelivery; inFlight tracks the delivery goroutines + // themselves and outstanding mirrors its count so a timed + // out drain can report how many were abandoned. + drainMu sync.Mutex + draining bool + inFlight sync.WaitGroup + outstanding atomic.Int64 + abandon chan struct{} + abandonOnce sync.Once +} + +// newService builds a Service with the fields every Service +// needs regardless of how it was constructed. +func newService( + log *slog.Logger, + transport http.RoundTripper, +) *Service { + return &Service{ + log: log, + transport: transport, + history: NewAlertHistory(), + abandon: make(chan struct{}), + } } // New creates a new notify Service. func New( - _ fx.Lifecycle, + lifecycle fx.Lifecycle, params Params, ) (*Service, error) { - svc := &Service{ - log: params.Logger.Get(), - transport: http.DefaultTransport, - config: params.Config, - history: NewAlertHistory(), - } + svc := newService(params.Logger.Get(), http.DefaultTransport) + svc.config = params.Config if params.Config.NtfyTopic != "" { u, err := ValidateWebhookURL( @@ -168,6 +192,14 @@ func New( svc.mattermostWebhookURL = u } + lifecycle.Append(fx.Hook{ + OnStop: func(ctx context.Context) error { + svc.drain(ctx) + + return nil + }, + }) + return svc, nil } @@ -194,6 +226,32 @@ func (svc *Service) SendNotification( svc.dispatchMattermost(ctx, title, message, priority) } +// dispatch delivers a notification to one endpoint on a +// tracked background goroutine. +// +// The delivery context is detached from ctx with +// context.WithoutCancel so that a cancelled caller does not +// kill a delivery already under way; the shutdown drain, not +// the caller, decides how long deliveries may keep running. +func (svc *Service) dispatch( + ctx context.Context, + endpoint string, + send func(context.Context) error, +) { + notifyCtx := context.WithoutCancel(ctx) + + svc.startDelivery(endpoint, func() { + err := svc.deliverWithRetry(notifyCtx, endpoint, send) + if err != nil { + svc.log.Error( + "failed to send notification after retries", + "endpoint", endpoint, + "error", err, + ) + } + }) +} + func (svc *Service) dispatchNtfy( ctx context.Context, title, message, priority string, @@ -202,26 +260,11 @@ func (svc *Service) dispatchNtfy( return } - go func() { - notifyCtx := context.WithoutCancel(ctx) - - err := svc.deliverWithRetry( - notifyCtx, "ntfy", - func(c context.Context) error { - return svc.sendNtfy( - c, svc.ntfyURL, - title, message, priority, - ) - }, + svc.dispatch(ctx, "ntfy", func(c context.Context) error { + return svc.sendNtfy( + c, svc.ntfyURL, title, message, priority, ) - if err != nil { - svc.log.Error( - "failed to send ntfy notification "+ - "after retries", - "error", err, - ) - } - }() + }) } func (svc *Service) dispatchSlack( @@ -232,26 +275,11 @@ func (svc *Service) dispatchSlack( return } - go func() { - notifyCtx := context.WithoutCancel(ctx) - - err := svc.deliverWithRetry( - notifyCtx, "slack", - func(c context.Context) error { - return svc.sendSlack( - c, svc.slackWebhookURL, - title, message, priority, - ) - }, + svc.dispatch(ctx, "slack", func(c context.Context) error { + return svc.sendSlack( + c, svc.slackWebhookURL, title, message, priority, ) - if err != nil { - svc.log.Error( - "failed to send slack notification "+ - "after retries", - "error", err, - ) - } - }() + }) } func (svc *Service) dispatchMattermost( @@ -262,26 +290,15 @@ func (svc *Service) dispatchMattermost( return } - go func() { - notifyCtx := context.WithoutCancel(ctx) - - err := svc.deliverWithRetry( - notifyCtx, "mattermost", - func(c context.Context) error { - return svc.sendSlack( - c, svc.mattermostWebhookURL, - title, message, priority, - ) - }, - ) - if err != nil { - svc.log.Error( - "failed to send mattermost notification "+ - "after retries", - "error", err, + svc.dispatch( + ctx, "mattermost", + func(c context.Context) error { + return svc.sendSlack( + c, svc.mattermostWebhookURL, + title, message, priority, ) - } - }() + }, + ) } func (svc *Service) sendNtfy( diff --git a/internal/notify/retry.go b/internal/notify/retry.go index cbc49d3..6745085 100644 --- a/internal/notify/retry.go +++ b/internal/notify/retry.go @@ -2,6 +2,7 @@ package notify import ( "context" + "fmt" "math" "math/rand/v2" "time" @@ -121,6 +122,14 @@ func (svc *Service) deliverWithRetry( select { case <-ctx.Done(): return ctx.Err() + case <-svc.abandon: + // Shutdown drained past its deadline; stop + // sleeping rather than outlive the process. + // A nil channel (Service built without a + // constructor) simply never fires. + return fmt.Errorf( + "%w: %s", ErrDeliveryAbandoned, endpoint, + ) case <-svc.sleepFunc(delay): } } diff --git a/internal/notify/shutdown.go b/internal/notify/shutdown.go new file mode 100644 index 0000000..0269889 --- /dev/null +++ b/internal/notify/shutdown.go @@ -0,0 +1,119 @@ +package notify + +import ( + "context" + "errors" +) + +// ErrDeliveryAbandoned is returned by a retry loop that was +// cut short because shutdown drained past its deadline. +var ErrDeliveryAbandoned = errors.New( + "notification delivery abandoned at shutdown", +) + +// startDelivery runs fn on its own goroutine while tracking it, +// so that drain can wait for it during shutdown. +// +// The WaitGroup counter is incremented here, on the caller's +// goroutine, before the worker exists: incrementing it inside +// the worker would race with drain's Wait and could let +// shutdown sail past a delivery that had not started yet. +// +// Once draining has begun the delivery is refused outright +// rather than queued, so a steady stream of newly submitted +// notifications cannot keep extending the drain. +func (svc *Service) startDelivery(endpoint string, fn func()) { + svc.drainMu.Lock() + + if svc.draining { + svc.drainMu.Unlock() + + svc.log.Warn( + "notification not dispatched: shutdown in progress", + "endpoint", endpoint, + ) + + return + } + + svc.outstanding.Add(1) + + // WaitGroup.Go increments the counter synchronously, here, + // and only then starts the goroutine. + svc.inFlight.Go(func() { + // Runs before the WaitGroup counter is decremented, so + // a drain that times out reports an accurate count. + defer svc.outstanding.Add(-1) + + fn() + }) + + svc.drainMu.Unlock() +} + +// drain waits for in-flight notification deliveries to finish. +// +// It first stops accepting new deliveries, then waits until +// either every outstanding delivery has completed or ctx +// expires — whichever comes first. ctx is the context fx +// passes to the OnStop hook, so a permanently dead webhook +// cannot hang shutdown indefinitely. +// +// When the deadline arrives with deliveries still outstanding, +// the count is logged at warn level and the abandon channel is +// closed, which releases any retry loop sleeping in backoff. +// Deliveries already inside an HTTP round trip are bounded by +// the existing httpClientTimeout instead. +// +// A ctx that is already expired on entry is not by itself cause +// for alarm: if nothing is outstanding there is nothing to +// abandon, and the drain says so at debug level rather than +// warning about deliveries that do not exist. +func (svc *Service) drain(ctx context.Context) { + svc.drainMu.Lock() + svc.draining = true + svc.drainMu.Unlock() + + done := make(chan struct{}) + + go func() { + svc.inFlight.Wait() + close(done) + }() + + select { + case <-done: + svc.log.Debug( + "all in-flight notifications completed", + ) + case <-ctx.Done(): + // outstanding is decremented before the WaitGroup + // counter, and startDelivery can no longer add to it + // now that draining is set, so a zero here means every + // delivery really did finish. ctx expiring in that + // state (an OnStop context that was already cancelled + // on entry is the usual way) abandons nothing, so it + // must not close abandon or warn about it. + abandoned := svc.outstanding.Load() + if abandoned == 0 { + svc.log.Debug( + "all in-flight notifications completed", + ) + + return + } + + svc.abandonOnce.Do(func() { + if svc.abandon != nil { + close(svc.abandon) + } + }) + + svc.log.Warn( + "shutdown deadline reached with notifications "+ + "still in flight; abandoning them", + "abandoned", abandoned, + "error", ctx.Err(), + ) + } +} diff --git a/internal/notify/shutdown_test.go b/internal/notify/shutdown_test.go new file mode 100644 index 0000000..f938b64 --- /dev/null +++ b/internal/notify/shutdown_test.go @@ -0,0 +1,531 @@ +package notify_test + +import ( + "bytes" + "context" + "log/slog" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "go.uber.org/fx" + + "sneak.berlin/go/dnswatcher/internal/config" + "sneak.berlin/go/dnswatcher/internal/globals" + "sneak.berlin/go/dnswatcher/internal/logger" + "sneak.berlin/go/dnswatcher/internal/notify" +) + +// Timings used by the drain tests. They stay in the same +// 10-100ms band as the retry tests so the suite never waits on +// a real backoff delay. +const ( + // inFlightHold is how long a delivery is kept mid-request + // before the handler is released. + inFlightHold = 30 * time.Millisecond + + // drainDeadline bounds a drain that is expected to time + // out. + drainDeadline = 50 * time.Millisecond + + // drainSlack is the upper bound on how long a bounded + // drain may take; generous enough for a loaded CI box, + // still far below the 20s test ceiling. + drainSlack = 2 * time.Second + + // settleDelay is how long to wait before asserting that + // something did *not* happen. + settleDelay = 50 * time.Millisecond + + // idleDrainBound is the upper bound on a drain that has + // nothing in flight. It is deliberately far above the cost + // of the goroutine hop through inFlight.Wait() — which + // reached 57ms on a loaded box under -race with the package's + // parallel tests — and far below drainSlack, the deadline + // such a drain is given. A drain that blocked until its + // deadline instead of returning on the WaitGroup therefore + // still fails this bound, but scheduling delay alone cannot. + idleDrainBound = 500 * time.Millisecond +) + +// syncBuffer is an io.Writer safe for concurrent use, so log +// output written from delivery goroutines can be inspected. +type syncBuffer struct { + mu sync.Mutex + buf bytes.Buffer +} + +func (sb *syncBuffer) Write(p []byte) (int, error) { + sb.mu.Lock() + defer sb.mu.Unlock() + + return sb.buf.Write(p) //nolint:wrapcheck // test helper +} + +func (sb *syncBuffer) String() string { + sb.mu.Lock() + defer sb.mu.Unlock() + + return sb.buf.String() +} + +// newLoggingService returns a Service writing JSON logs into +// the returned buffer. +func newLoggingService( + transport http.RoundTripper, +) (*notify.Service, *syncBuffer) { + logs := &syncBuffer{} + handler := slog.NewJSONHandler(logs, nil) + + return notify.NewTestServiceWithLogger(transport, handler), + logs +} + +// blockingNtfyServer returns a server whose handler signals on +// entered, waits for release, and then responds 200. +func blockingNtfyServer( + entered chan<- struct{}, + release <-chan struct{}, + served *atomic.Bool, +) *httptest.Server { + var once sync.Once + + return httptest.NewServer( + http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + once.Do(func() { close(entered) }) + <-release + + served.Store(true) + + w.WriteHeader(http.StatusOK) + }), + ) +} + +// TestDrainWaitsForInFlightDelivery verifies that a delivery +// already under way when shutdown starts is allowed to finish. +func TestDrainWaitsForInFlightDelivery(t *testing.T) { + t.Parallel() + + var served atomic.Bool + + entered := make(chan struct{}) + release := make(chan struct{}) + + srv := blockingNtfyServer(entered, release, &served) + defer srv.Close() + + topicURL, _ := url.Parse(srv.URL) + + svc := notify.NewTestService(http.DefaultTransport) + svc.SetNtfyURL(topicURL) + + svc.SendNotification( + context.Background(), "t", "m", prioInfo, + ) + + // Make sure the delivery really is mid-request before the + // drain begins. + select { + case <-entered: + case <-time.After(drainSlack): + t.Fatal("delivery never reached the endpoint") + } + + // As in TestDrainBoundedByContextDeadline: start is captured + // before the clock it is compared against, here the timer + // holding the delivery open, so elapsed covers the whole hold + // and the lower bound cannot come out short from scheduling + // delay alone. + start := time.Now() + + timer := time.AfterFunc(inFlightHold, func() { + close(release) + }) + defer timer.Stop() + + ctx, cancel := context.WithTimeout( + context.Background(), drainSlack, + ) + defer cancel() + + svc.Drain(ctx) + + elapsed := time.Since(start) + + if !served.Load() { + t.Error( + "drain returned before the in-flight delivery " + + "completed", + ) + } + + if elapsed < inFlightHold { + t.Errorf( + "drain took %v, want at least %v", + elapsed, inFlightHold, + ) + } + + if got := svc.OutstandingDeliveries(); got != 0 { + t.Errorf("outstanding deliveries = %d, want 0", got) + } +} + +// neverFires returns a channel that never delivers, standing in +// for a long backoff sleep without actually sleeping. +func neverFires(_ time.Duration) <-chan time.Time { + return make(chan time.Time) +} + +// TestDrainBoundedByContextDeadline verifies that a delivery +// stuck retrying against a dead endpoint does not hold shutdown +// past the OnStop context deadline, and that the abandoned +// deliveries are logged at warn level rather than dropped +// silently. +func TestDrainBoundedByContextDeadline(t *testing.T) { + t.Parallel() + + var requests atomic.Int64 + + srv := httptest.NewServer( + http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + requests.Add(1) + + w.WriteHeader(http.StatusInternalServerError) + }), + ) + defer srv.Close() + + topicURL, _ := url.Parse(srv.URL) + + svc, logs := newLoggingService(http.DefaultTransport) + svc.SetNtfyURL(topicURL) + // Never let the backoff sleep complete: the delivery is + // parked in its retry wait until shutdown releases it. + svc.SetSleepFunc(neverFires) + svc.SetRetryConfig(notify.RetryConfig{ + MaxRetries: 5, + BaseDelay: time.Hour, + MaxDelay: time.Hour, + }) + + svc.SendNotification( + context.Background(), "t", "m", prioError, + ) + + waitForCondition(t, func() bool { + return requests.Load() >= 1 && + svc.OutstandingDeliveries() == 1 + }) + + // start must be captured *before* the deadline clock starts, + // so that the measured interval is a superset of the deadline + // interval. Capturing it after context.WithTimeout would + // make elapsed structurally smaller than drainDeadline and + // the lower bound below unfalsifiable-by-luck: it would fail + // whenever the two statements were separated by any + // scheduling delay, and pass otherwise, regardless of what + // the drain did. + start := time.Now() + + ctx, cancel := context.WithTimeout( + context.Background(), drainDeadline, + ) + defer cancel() + + // The upper bound is enforced by a watchdog rather than by + // measuring after the fact: a drain that is not bounded at + // all never returns here (the delivery is parked in a backoff + // that never fires), so an unbounded drain must fail this + // test promptly instead of hanging the package until the test + // binary's 30s timeout. + returned := make(chan struct{}) + + go func() { + defer close(returned) + + svc.Drain(ctx) + }() + + select { + case <-returned: + case <-time.After(drainSlack): + t.Fatalf( + "drain did not return within %v; its %v deadline "+ + "did not bound it", + drainSlack, drainDeadline, + ) + } + + // The lower bound is the real assertion: the drain must have + // waited for its whole deadline rather than giving up on the + // outstanding delivery early. With start captured above, an + // early return is the only thing that can make it fail. + if elapsed := time.Since(start); elapsed < drainDeadline { + t.Errorf( + "drain returned after %v, before its %v deadline", + elapsed, drainDeadline, + ) + } + + assertAbandonLogged(t, logs.String()) + + // The abandoned delivery must stop retrying rather than + // outlive the drain. + waitForCondition(t, func() bool { + return svc.OutstandingDeliveries() == 0 + }) +} + +// assertAbandonLogged checks that the drain logged the +// abandoned deliveries at warn level with a count. +func assertAbandonLogged(t *testing.T, output string) { + t.Helper() + + if !strings.Contains(output, `"level":"WARN"`) { + t.Errorf( + "abandoned deliveries not logged at warn level; "+ + "log output: %s", + output, + ) + } + + if !strings.Contains(output, `"abandoned":1`) { + t.Errorf( + "abandoned delivery count not logged; "+ + "log output: %s", + output, + ) + } +} + +// TestDrainRefusesNewDeliveries verifies that notifications +// submitted after the drain has begun are refused and logged, +// so a stream of new work cannot extend shutdown indefinitely. +func TestDrainRefusesNewDeliveries(t *testing.T) { + t.Parallel() + + var requests atomic.Int64 + + srv := httptest.NewServer( + http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + requests.Add(1) + + w.WriteHeader(http.StatusOK) + }), + ) + defer srv.Close() + + target, _ := url.Parse(srv.URL) + + svc, logs := newLoggingService(http.DefaultTransport) + svc.SetNtfyURL(target) + svc.SetSlackWebhookURL(target) + svc.SetMattermostWebhookURL(target) + + ctx, cancel := context.WithTimeout( + context.Background(), drainSlack, + ) + defer cancel() + + // Nothing is in flight, so this returns immediately and + // leaves the service refusing further deliveries. + svc.Drain(ctx) + + for range 3 { + svc.SendNotification( + context.Background(), "t", "m", prioInfo, + ) + } + + time.Sleep(settleDelay) + + if got := requests.Load(); got != 0 { + t.Errorf( + "%d requests reached the endpoint after drain, "+ + "want 0", + got, + ) + } + + if got := svc.OutstandingDeliveries(); got != 0 { + t.Errorf("outstanding deliveries = %d, want 0", got) + } + + output := logs.String() + if !strings.Contains(output, "shutdown in progress") { + t.Errorf( + "refused deliveries not logged; log output: %s", + output, + ) + } +} + +// recordingLifecycle is a minimal fx.Lifecycle that records the +// hooks appended to it, so the wiring done by notify.New can be +// inspected without standing up a whole fx application. +type recordingLifecycle struct { + hooks []fx.Hook +} + +func (l *recordingLifecycle) Append(hook fx.Hook) { + l.hooks = append(l.hooks, hook) +} + +// newNotifyService builds a Service through the real +// constructor, wired to the given lifecycle. +func newNotifyService( + t *testing.T, + lifecycle fx.Lifecycle, + ntfyTopic string, +) *notify.Service { + t.Helper() + + g, err := globals.New(nil) + if err != nil { + t.Fatalf("globals.New: %v", err) + } + + log, err := logger.New(nil, logger.Params{Globals: g}) + if err != nil { + t.Fatalf("logger.New: %v", err) + } + + svc, err := notify.New(lifecycle, notify.Params{ + Logger: log, + Config: &config.Config{NtfyTopic: ntfyTopic}, + }) + if err != nil { + t.Fatalf("notify.New: %v", err) + } + + return svc +} + +// TestNewRegistersDrainingStopHook verifies that notify.New +// wires an OnStop hook into the fx lifecycle and that the hook +// waits for in-flight deliveries. +func TestNewRegistersDrainingStopHook(t *testing.T) { + t.Parallel() + + var served atomic.Bool + + entered := make(chan struct{}) + release := make(chan struct{}) + + srv := blockingNtfyServer(entered, release, &served) + defer srv.Close() + + lifecycle := &recordingLifecycle{} + svc := newNotifyService(t, lifecycle, srv.URL) + + if len(lifecycle.hooks) != 1 { + t.Fatalf( + "appended %d lifecycle hooks, want 1", + len(lifecycle.hooks), + ) + } + + stop := lifecycle.hooks[0].OnStop + if stop == nil { + t.Fatal("lifecycle hook has no OnStop function") + } + + svc.SendNotification( + context.Background(), "t", "m", prioInfo, + ) + + select { + case <-entered: + case <-time.After(drainSlack): + t.Fatal("delivery never reached the endpoint") + } + + timer := time.AfterFunc(inFlightHold, func() { + close(release) + }) + defer timer.Stop() + + ctx, cancel := context.WithTimeout( + context.Background(), drainSlack, + ) + defer cancel() + + err := stop(ctx) + if err != nil { + t.Fatalf("OnStop returned error: %v", err) + } + + if !served.Load() { + t.Error( + "OnStop returned before the in-flight delivery " + + "completed", + ) + } +} + +// TestDrainWithoutDeliveriesReturnsImmediately verifies the +// common case: nothing in flight, shutdown is not delayed. +func TestDrainWithoutDeliveriesReturnsImmediately(t *testing.T) { + t.Parallel() + + svc := notify.NewTestService(http.DefaultTransport) + + // Captured before the deadline clock, as elsewhere in this + // file; for an upper bound that is the conservative + // direction, since the measured interval can then only be + // longer than the drain itself. + start := time.Now() + + ctx, cancel := context.WithTimeout( + context.Background(), drainSlack, + ) + defer cancel() + + svc.Drain(ctx) + + if elapsed := time.Since(start); elapsed > idleDrainBound { + t.Errorf( + "drain of an idle service took %v, want well "+ + "under its %v deadline", + elapsed, drainSlack, + ) + } +} + +// TestDrainWithCancelledContextDoesNotWarn verifies that an +// OnStop context that is already dead on entry does not produce +// an "abandoning them" warning when there was nothing in flight +// to abandon. The expired context wins the select immediately, +// so only the outstanding count can tell the difference between +// a genuine timeout and a shutdown that had simply already run +// out of time with no work left. +func TestDrainWithCancelledContextDoesNotWarn(t *testing.T) { + t.Parallel() + + svc, logs := newLoggingService(http.DefaultTransport) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + svc.Drain(ctx) + + if output := logs.String(); strings.Contains( + output, `"level":"WARN"`, + ) { + t.Errorf( + "drain with nothing in flight warned about "+ + "abandoned deliveries; log output: %s", + output, + ) + } +} -- 2.54.0 From fc43f893a54e4abe995ca6c0c17e873494317163 Mon Sep 17 00:00:00 2001 From: clawbot Date: Wed, 9 Sep 2026 15:17:43 +0200 Subject: [PATCH 08/15] server: set all four http.Server socket timeouts (#118) The http.Server was built with only ReadHeaderTimeout set; the other three timeouts were zero, which in net/http means no limit, so a peer could hold a connection open past the header phase, responses had no write deadline, and keep-alive connections were never reaped. ReadTimeout 15s, WriteTimeout 75s, and IdleTimeout 120s now join ReadHeaderTimeout 10s as named constants. WriteTimeout must stay above the 60s chimw.Timeout handler budget, because net/http arms the write deadline once request headers are read; a test fails the build if either number moves alone. The server literal moved into newHTTPServer so the configuration can be asserted without binding a socket (closes #99) Model: opus-5 --- README.md | 21 +++++++ TODO.md | 8 +++ internal/server/export_test.go | 19 ++++++ internal/server/server.go | 71 ++++++++++++++++++--- internal/server/server_test.go | 112 +++++++++++++++++++++++++++++++++ 5 files changed, 224 insertions(+), 7 deletions(-) create mode 100644 internal/server/export_test.go create mode 100644 internal/server/server_test.go diff --git a/README.md b/README.md index 9e854ff..c849041 100644 --- a/README.md +++ b/README.md @@ -182,6 +182,27 @@ dnswatcher exposes a lightweight HTTP API for operational visibility: | `GET /api/v1/status` | Current monitoring state | | `GET /metrics` | Prometheus metrics (optional) | +#### Server timeouts + +The HTTP server sets all four socket-level timeouts. These are compile-time +constants in `internal/server/server.go`, not configurable via environment +variables. + +| Timeout | Value | Purpose | +|---------------------|-------|-----------------------------------------------| +| `ReadHeaderTimeout` | 10s | Bounds the request header read (slowloris) | +| `ReadTimeout` | 15s | Bounds the whole request read, headers + body | +| `WriteTimeout` | 75s | Bounds handler execution plus response flush | +| `IdleTimeout` | 120s | Reaps idle keep-alive connections | + +These are distinct from the 60s per-request handler budget applied by +`chimw.Timeout` in `internal/server/routes.go`, which cancels the request +context but does not touch the socket. `WriteTimeout` is deliberately +larger than that budget: the write deadline is armed once request headers +are read, so a smaller value would sever the connection before a handler +using its full budget could respond. `IdleTimeout` exceeds common +Prometheus scrape intervals so the scraper reuses its connection. + --- ## Architecture diff --git a/TODO.md b/TODO.md index 91ea4e5..259fad2 100644 --- a/TODO.md +++ b/TODO.md @@ -101,6 +101,14 @@ Rationale, Design, TODO, License, Author) if any are still missing. so shutdown cannot be extended indefinitely; an `OnStop` context that is already expired on entry with nothing outstanding drains quietly rather than warning about deliveries that were never abandoned +- 2026-08-09: `http.Server` now sets all four socket-level timeouts + (`ReadTimeout` 15s, `ReadHeaderTimeout` 10s, `WriteTimeout` 75s, + `IdleTimeout` 120s) as named constants in `internal/server/server.go`, + closing the slowloris / unreaped-keep-alive exposure required by + `REPO_POLICIES.md` before 1.0; `WriteTimeout` is deliberately greater + than the 60s `chimw.Timeout` handler budget so that budget stays + reachable, and tests in `internal/server` pin both the non-zero + values and that relationship (#99) - 2026-08-07: golangci-lint bumped to v2.12.2 (commit-pinned installs in `Dockerfile` and `script/bootstrap`); `.golangci.yml` set to the org-standard v2-schema config used across the org's repos diff --git a/internal/server/export_test.go b/internal/server/export_test.go new file mode 100644 index 0000000..8df05f2 --- /dev/null +++ b/internal/server/export_test.go @@ -0,0 +1,19 @@ +package server + +import ( + "net/http" + "time" +) + +// NewHTTPServer exports newHTTPServer for testing. +func NewHTTPServer( + listenAddr string, + handler http.Handler, +) *http.Server { + return newHTTPServer(listenAddr, handler) +} + +// RequestTimeout exports the handler execution budget applied by +// chimw.Timeout in SetupRoutes, so tests can assert the relationship +// between it and the server's WriteTimeout. +const RequestTimeout time.Duration = requestTimeout diff --git a/internal/server/server.go b/internal/server/server.go index 4cb30a2..f9385eb 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -33,8 +33,52 @@ type Params struct { // shutdownTimeout is how long to wait for graceful shutdown. const shutdownTimeout = 30 * time.Second -// readHeaderTimeout is the max duration for reading request headers. -const readHeaderTimeout = 10 * time.Second +// Socket-level timeouts for the HTTP server. +// +// These bound time spent on the connection itself and are a distinct +// control from the per-request handler budget enforced by +// chimw.Timeout(requestTimeout) in routes.go: that one cancels the +// request context after requestTimeout but never touches the socket, +// so without the values below a peer can hold a connection open +// forever (slowloris, unreaped keep-alives). +// +// The one hard constraint between the two controls is +// writeTimeout > requestTimeout. net/http arms the write deadline +// once the request headers have been read, so on a plaintext +// connection it covers handler execution AND the response flush. If +// writeTimeout were <= requestTimeout the server would sever the +// connection before a handler that legitimately consumed its full +// budget could emit anything, making the 60s budget unreachable in +// practice. The margin between them is the response-flush allowance. +// +// The only clients of this service are browsers loading the dashboard +// and a Prometheus scraper; the values are sized for those. +const ( + // readHeaderTimeout is the max duration for reading request + // headers. + readHeaderTimeout = 10 * time.Second + + // readTimeout bounds reading the entire request, headers plus + // body. Every route here is a GET with no body, so this only + // ever needs to cover headers; the extra 5s over + // readHeaderTimeout is slack, not a real allowance, and keeps a + // body dribbled one byte at a time from holding the read side + // open indefinitely. + readTimeout = 15 * time.Second + + // writeTimeout must exceed the requestTimeout handler budget + // (60s) per the note above. The 15s difference is the allowance + // for flushing a completed response to a slow client. + writeTimeout = 75 * time.Second + + // idleTimeout reaps keep-alive connections between requests. It + // is deliberately longer than the common Prometheus scrape + // intervals (15s/30s/60s) so the scraper reuses its connection + // rather than reconnecting every cycle, while a browser tab + // left open on the dashboard stops occupying a connection + // within two minutes of going quiet. + idleTimeout = 120 * time.Second +) // Server is the HTTP server. type Server struct { @@ -76,16 +120,29 @@ func New( return srv, nil } +// newHTTPServer builds the listening http.Server with every +// socket-level timeout set. All four are set deliberately: a zero +// value in net/http means "no limit", not "some default". +func newHTTPServer( + listenAddr string, + handler http.Handler, +) *http.Server { + return &http.Server{ + Addr: listenAddr, + Handler: handler, + ReadTimeout: readTimeout, + ReadHeaderTimeout: readHeaderTimeout, + WriteTimeout: writeTimeout, + IdleTimeout: idleTimeout, + } +} + // Run starts the HTTP server. func (s *Server) Run() { s.SetupRoutes() listenAddr := fmt.Sprintf(":%d", s.port) - s.httpServer = &http.Server{ - Addr: listenAddr, - Handler: s, - ReadHeaderTimeout: readHeaderTimeout, - } + s.httpServer = newHTTPServer(listenAddr, s) s.log.Info("http server starting", "addr", listenAddr) diff --git a/internal/server/server_test.go b/internal/server/server_test.go new file mode 100644 index 0000000..9909f7d --- /dev/null +++ b/internal/server/server_test.go @@ -0,0 +1,112 @@ +package server_test + +import ( + "net/http" + "testing" + + "sneak.berlin/go/dnswatcher/internal/server" +) + +// noopHandler stands in for the router; newHTTPServer only stores it. +func noopHandler() http.Handler { + return http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }, + ) +} + +// TestHTTPServerTimeoutsAreSet asserts that every socket-level +// timeout is configured. A zero value in net/http means "no limit", +// so a refactor that silently drops one of these reintroduces the +// slowloris / unreaped-keep-alive exposure this guards against. +// +// The assertions are on the configured field values only; nothing +// here measures elapsed time, so the test cannot flake on timing. +func TestHTTPServerTimeoutsAreSet(t *testing.T) { + t.Parallel() + + srv := server.NewHTTPServer(":8080", noopHandler()) + + if srv.ReadTimeout <= 0 { + t.Errorf( + "ReadTimeout must be non-zero, got %v", + srv.ReadTimeout, + ) + } + + if srv.ReadHeaderTimeout <= 0 { + t.Errorf( + "ReadHeaderTimeout must be non-zero, got %v", + srv.ReadHeaderTimeout, + ) + } + + if srv.WriteTimeout <= 0 { + t.Errorf( + "WriteTimeout must be non-zero, got %v", + srv.WriteTimeout, + ) + } + + if srv.IdleTimeout <= 0 { + t.Errorf( + "IdleTimeout must be non-zero, got %v", + srv.IdleTimeout, + ) + } +} + +// TestWriteTimeoutExceedsHandlerBudget pins the one relationship the +// values must satisfy. net/http arms the write deadline once request +// headers are read, so it covers handler execution plus the response +// flush. If WriteTimeout were not greater than the chimw.Timeout +// handler budget, the connection would be severed before a handler +// that used its full budget could respond, making that budget +// unreachable. +func TestWriteTimeoutExceedsHandlerBudget(t *testing.T) { + t.Parallel() + + srv := server.NewHTTPServer(":8080", noopHandler()) + + if srv.WriteTimeout <= server.RequestTimeout { + t.Errorf( + "WriteTimeout (%v) must exceed handler budget (%v)", + srv.WriteTimeout, + server.RequestTimeout, + ) + } +} + +// TestReadTimeoutCoversHeaderTimeout asserts the read deadline for +// the whole request is at least as long as the header-only deadline; +// a smaller ReadTimeout would make ReadHeaderTimeout unreachable. +func TestReadTimeoutCoversHeaderTimeout(t *testing.T) { + t.Parallel() + + srv := server.NewHTTPServer(":8080", noopHandler()) + + if srv.ReadTimeout < srv.ReadHeaderTimeout { + t.Errorf( + "ReadTimeout (%v) must be >= ReadHeaderTimeout (%v)", + srv.ReadTimeout, + srv.ReadHeaderTimeout, + ) + } +} + +// TestHTTPServerAddrAndHandler covers the rest of the constructor so +// a future edit cannot drop the listen address or the handler. +func TestHTTPServerAddrAndHandler(t *testing.T) { + t.Parallel() + + srv := server.NewHTTPServer(":9999", noopHandler()) + + if srv.Addr != ":9999" { + t.Errorf("Addr = %q, want %q", srv.Addr, ":9999") + } + + if srv.Handler == nil { + t.Error("Handler must not be nil") + } +} -- 2.54.0 From b351a2350c1e2443a8a3b34229c56dde065ef0fe Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 21 Sep 2026 09:44:49 +0200 Subject: [PATCH 09/15] gomod: drop stale golang.org/x/sync indirect line (closes #132) go.mod listed golang.org/x/sync v0.19.0 twice: once in the direct require block and once as // indirect. The module is a genuine direct dependency (internal/portcheck/portcheck.go imports golang.org/x/sync/errgroup), so the indirect entry was redundant and stale. script/bootstrap ends with go mod download, which dropped that line as a side effect and left every fresh checkout with a dirty go.mod. Running go mod tidy removes the line for good; no dependency and no version changed, and go.sum is unaffected. Model: opus-4-8 --- TODO.md | 2 ++ go.mod | 1 - 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/TODO.md b/TODO.md index 259fad2..b00f161 100644 --- a/TODO.md +++ b/TODO.md @@ -23,6 +23,8 @@ Rationale, Design, TODO, License, Author) if any are still missing. # Completed Steps +- 2026-09-21: `go mod tidy` dropped the redundant `golang.org/x/sync` + `// indirect` line so `script/bootstrap` leaves a clean tree (#132) - 2026-08-10: comment-only corrections to `script/bootstrap`, `script/cibuild`, and `Dockerfile.lint`. The `goimports` pin in `script/bootstrap` was justified by a claim that `script/fmt-check` diff --git a/go.mod b/go.mod index 53638c9..0078dcf 100644 --- a/go.mod +++ b/go.mod @@ -40,7 +40,6 @@ require ( go.yaml.in/yaml/v2 v2.4.2 // indirect go.yaml.in/yaml/v3 v3.0.4 // indirect golang.org/x/mod v0.32.0 // indirect - golang.org/x/sync v0.19.0 // indirect golang.org/x/sys v0.41.0 // indirect golang.org/x/text v0.34.0 // indirect golang.org/x/tools v0.41.0 // indirect -- 2.54.0 From c2a07ce69069d5ceae26c985cbc88767a6d0bb3c Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 21 Sep 2026 10:05:38 +0200 Subject: [PATCH 10/15] test: add tests for globals, healthcheck and logger (closes #110) Tests for the three packages that had none, written from outside each package, each able to fail on a plausible break: - globals: values set are read back through New, and New returns an independent copy. One sequential test function with a disclosed paralleltest suppression, because it changes shared package variables. - healthcheck: Check returns status "ok", the documented JSON fields, an RFC3339Nano timestamp, the maintenance flag from config in both states, and version and appname from globals. - logger: New gives a usable *slog.Logger, debug output is off by default and EnableDebugLogging turns it on. No production code changed. The terminal output format is not asserted. Model: opus-4-8 --- TODO.md | 2 + internal/globals/globals_test.go | 50 +++++++++ internal/healthcheck/healthcheck_test.go | 134 +++++++++++++++++++++++ internal/logger/logger_test.go | 66 +++++++++++ 4 files changed, 252 insertions(+) create mode 100644 internal/globals/globals_test.go create mode 100644 internal/healthcheck/healthcheck_test.go create mode 100644 internal/logger/logger_test.go diff --git a/TODO.md b/TODO.md index b00f161..18cd4e6 100644 --- a/TODO.md +++ b/TODO.md @@ -23,6 +23,8 @@ Rationale, Design, TODO, License, Author) if any are still missing. # Completed Steps +- 2026-09-21: added behavioural tests for `internal/globals`, + `internal/healthcheck`, and `internal/logger` (closes #110). - 2026-09-21: `go mod tidy` dropped the redundant `golang.org/x/sync` `// indirect` line so `script/bootstrap` leaves a clean tree (#132) - 2026-08-10: comment-only corrections to `script/bootstrap`, diff --git a/internal/globals/globals_test.go b/internal/globals/globals_test.go new file mode 100644 index 0000000..f44174e --- /dev/null +++ b/internal/globals/globals_test.go @@ -0,0 +1,50 @@ +package globals_test + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sneak.berlin/go/dnswatcher/internal/globals" +) + +// TestGlobals exercises the package-level version and appname +// variables through their setters and read-back via New. These are +// shared package state, so the test mutates a global and must run +// sequentially; it cannot use t.Parallel(). +// +//nolint:paralleltest // mutates shared package-level globals, must run sequentially +func TestGlobals(t *testing.T) { + versions := []string{"v1.2.3", "dev", "", "v1.2.3-4-gabcdef"} + for _, want := range versions { + globals.SetVersion(want) + + g, err := globals.New(nil) + require.NoError(t, err) + assert.Equal(t, want, g.Version, + "New must surface the version set by SetVersion") + } + + names := []string{"dnswatcher", "other", ""} + for _, want := range names { + globals.SetAppname(want) + + g, err := globals.New(nil) + require.NoError(t, err) + assert.Equal(t, want, g.Appname, + "New must surface the appname set by SetAppname") + } + + // New returns a snapshot: a later SetVersion must not mutate a + // Globals handed out earlier. + globals.SetVersion("first") + + g, err := globals.New(nil) + require.NoError(t, err) + + globals.SetVersion("second") + assert.Equal(t, "first", g.Version, + "a Globals returned by New must not change when the "+ + "package variable is set again") +} diff --git a/internal/healthcheck/healthcheck_test.go b/internal/healthcheck/healthcheck_test.go new file mode 100644 index 0000000..b87c3d1 --- /dev/null +++ b/internal/healthcheck/healthcheck_test.go @@ -0,0 +1,134 @@ +package healthcheck_test + +import ( + "context" + "encoding/json" + "testing" + "time" + + "go.uber.org/fx" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sneak.berlin/go/dnswatcher/internal/config" + "sneak.berlin/go/dnswatcher/internal/globals" + "sneak.berlin/go/dnswatcher/internal/healthcheck" + "sneak.berlin/go/dnswatcher/internal/logger" +) + +// recordingLifecycle is a minimal fx.Lifecycle that records the hooks +// appended to it, so healthcheck.New can be exercised through its real +// constructor without standing up a whole fx application. +type recordingLifecycle struct { + hooks []fx.Hook +} + +func (l *recordingLifecycle) Append(hook fx.Hook) { + l.hooks = append(l.hooks, hook) +} + +// newHealthcheck builds a Healthcheck through the real constructor and +// runs the registered OnStart hook so StartupTime is set the same way +// the fx lifecycle would set it. +func newHealthcheck( + t *testing.T, + maintenance bool, + version string, +) *healthcheck.Healthcheck { + t.Helper() + + g := &globals.Globals{Appname: "dnswatcher", Version: version} + + log, err := logger.New(nil, logger.Params{Globals: g}) + require.NoError(t, err) + + lifecycle := &recordingLifecycle{} + + hc, err := healthcheck.New(lifecycle, healthcheck.Params{ + Globals: g, + Config: &config.Config{MaintenanceMode: maintenance}, + Logger: log, + }) + require.NoError(t, err) + + require.Len(t, lifecycle.hooks, 1, + "New must register exactly one lifecycle hook") + require.NotNil(t, lifecycle.hooks[0].OnStart) + require.NoError(t, lifecycle.hooks[0].OnStart(context.Background())) + + return hc +} + +func TestCheckStatusAndPayloadShape(t *testing.T) { + t.Parallel() + + hc := newHealthcheck(t, false, "v9.9.9") + resp := hc.Check() + + assert.Equal(t, "ok", resp.Status) + + // The JSON shape and field names are part of the contract for the + // /health and /.well-known/healthcheck routes, so assert on the + // exact set of keys the response marshals to. + raw, err := json.Marshal(resp) + require.NoError(t, err) + + var fields map[string]json.RawMessage + require.NoError(t, json.Unmarshal(raw, &fields)) + + wantKeys := []string{ + "status", + "now", + "uptimeSeconds", + "uptimeHuman", + "version", + "appname", + "maintenanceMode", + } + assert.Len(t, fields, len(wantKeys), + "response must marshal to exactly the documented fields") + + for _, key := range wantKeys { + assert.Contains(t, fields, key, "missing JSON field %q", key) + } + + // The Now field is documented as RFC3339Nano; a change to the + // format constant should turn this red. + _, err = time.Parse(time.RFC3339Nano, resp.Now) + assert.NoError(t, err, "Now must be RFC3339Nano") +} + +func TestCheckMaintenanceModeReflectsConfig(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + maintenance bool + }{ + {"maintenance off", false}, + {"maintenance on", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + hc := newHealthcheck(t, tt.maintenance, "test") + resp := hc.Check() + assert.Equal(t, tt.maintenance, resp.Maintenance, + "maintenanceMode must mirror Config.MaintenanceMode") + }) + } +} + +func TestCheckSurfacesVersionAndAppname(t *testing.T) { + t.Parallel() + + hc := newHealthcheck(t, false, "surfaced-version-123") + resp := hc.Check() + + assert.Equal(t, "surfaced-version-123", resp.Version, + "version from globals must appear in the payload") + assert.Equal(t, "dnswatcher", resp.Appname) +} diff --git a/internal/logger/logger_test.go b/internal/logger/logger_test.go new file mode 100644 index 0000000..80ec8c9 --- /dev/null +++ b/internal/logger/logger_test.go @@ -0,0 +1,66 @@ +package logger_test + +import ( + "context" + "log/slog" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "sneak.berlin/go/dnswatcher/internal/globals" + "sneak.berlin/go/dnswatcher/internal/logger" +) + +func newTestLogger(t *testing.T) *logger.Logger { + t.Helper() + + g := &globals.Globals{Appname: "dnswatcher", Version: "test"} + + l, err := logger.New(nil, logger.Params{Globals: g}) + require.NoError(t, err) + + return l +} + +// TestNewReturnsUsableLogger checks that the constructor yields a +// working *slog.Logger. +func TestNewReturnsUsableLogger(t *testing.T) { + t.Parallel() + + l := newTestLogger(t) + require.NotNil(t, l.Get(), "Get must return a non-nil logger") +} + +// TestDefaultLevelExcludesDebug verifies the default configuration +// logs at info: debug records are suppressed, info records pass. +func TestDefaultLevelExcludesDebug(t *testing.T) { + t.Parallel() + + log := newTestLogger(t).Get() + ctx := context.Background() + + assert.False(t, log.Enabled(ctx, slog.LevelDebug), + "debug must be suppressed at the default level") + assert.True(t, log.Enabled(ctx, slog.LevelInfo), + "info must be enabled at the default level") +} + +// TestEnableDebugLoggingChangesLevel verifies the debug and non-debug +// configurations differ as intended: enabling debug makes debug +// records pass where they previously did not. +func TestEnableDebugLoggingChangesLevel(t *testing.T) { + t.Parallel() + + l := newTestLogger(t) + log := l.Get() + ctx := context.Background() + + require.False(t, log.Enabled(ctx, slog.LevelDebug), + "debug must start disabled") + + l.EnableDebugLogging() + + assert.True(t, log.Enabled(ctx, slog.LevelDebug), + "debug must be enabled after EnableDebugLogging") +} -- 2.54.0 From 8aaa1039568d8c738c7b61c1979f1de1986cc516 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 22 Sep 2026 00:52:48 +0200 Subject: [PATCH 11/15] middleware: add security response headers (closes #98) Adds a SecurityHeaders middleware and registers it globally, right after the request ID middleware, so every route gets the headers, including static files, /metrics and error responses. It sets Strict-Transport-Security (one year, includeSubDomains), a Content-Security-Policy with default-src 'self', no scripts and frame-ancestors 'none', X-Frame-Options DENY, X-Content-Type-Options nosniff, Referrer-Policy no-referrer and a Permissions-Policy that turns every listed feature off. HSTS is sent on every response, not only over TLS: the service runs behind a TLS-terminating proxy and REPO_POLICIES.md requires the application to send it. Referrer-Policy is stricter than the policy baseline because dashboard URLs can name internal hosts. model: claude-opus-4-8 (implementation); claude-fable-5 (commit message) --- README.md | 43 +++- TODO.md | 10 + internal/middleware/middleware.go | 85 +++++++ internal/middleware/middleware_test.go | 334 +++++++++++++++++++++++++ internal/server/routes.go | 1 + 5 files changed, 472 insertions(+), 1 deletion(-) create mode 100644 internal/middleware/middleware_test.go diff --git a/README.md b/README.md index c849041..2dae62c 100644 --- a/README.md +++ b/README.md @@ -203,6 +203,46 @@ are read, so a smaller value would sever the connection before a handler using its full budget could respond. `IdleTimeout` exceeds common Prometheus scrape intervals so the scraper reuses its connection. +### Security Headers + +Every response — the dashboard, the static assets under `/s/...`, the +healthchecks, the JSON API, and `/metrics` — carries the following +headers, set by a global middleware: + +| Header | Value | +|-----------------------------|---------------------------------------| +| `Strict-Transport-Security` | `max-age=31536000; includeSubDomains` | +| `Content-Security-Policy` | see below | +| `X-Frame-Options` | `DENY` | +| `X-Content-Type-Options` | `nosniff` | +| `Referrer-Policy` | `no-referrer` | +| `Permissions-Policy` | all unused browser features denied | + +The content security policy is: + +``` +default-src 'self'; script-src 'none'; style-src 'self'; img-src 'self'; +font-src 'none'; connect-src 'none'; object-src 'none'; base-uri 'none'; +form-action 'none'; frame-ancestors 'none' +``` + +The dashboard ships no JavaScript (the 30-second refresh is a +``), no inline styles, no inline event +handlers, and no images; its only subresource is the embedded stylesheet +at `/s/css/tailwind.min.css`, which `style-src 'self'` permits. The +policy therefore needs neither `unsafe-inline` nor `unsafe-eval`. +`frame-ancestors 'none'` is the primary anti-framing control, with +`X-Frame-Options: DENY` retained as the legacy fallback. + +HSTS is emitted unconditionally, including over plain HTTP. dnswatcher is +expected to run behind a TLS-terminating reverse proxy, and the browser +must still be told to enforce HTTPS end to end, so the header is never +gated on whether the request itself arrived over TLS. + +`Referrer-Policy: no-referrer` is stricter than the +`strict-origin-when-cross-origin` baseline: the dashboard has no +cross-origin navigation needs, and its URL may name internal hosts. + --- ## Architecture @@ -215,7 +255,8 @@ internal/ globals/globals.go Build-time variables (version) logger/logger.go slog structured logging (TTY detection) healthcheck/healthcheck.go Health check service - middleware/middleware.go HTTP middleware (logging, CORS, metrics auth) + middleware/middleware.go HTTP middleware (logging, CORS, security + headers, metrics auth) handlers/handlers.go HTTP request handlers server/ server.go HTTP server lifecycle diff --git a/TODO.md b/TODO.md index 18cd4e6..a0596cc 100644 --- a/TODO.md +++ b/TODO.md @@ -113,6 +113,16 @@ Rationale, Design, TODO, License, Author) if any are still missing. than the 60s `chimw.Timeout` handler budget so that budget stays reachable, and tests in `internal/server` pin both the non-zero values and that relationship (#99) +- 2026-08-09: security response headers middleware + (`SecurityHeaders()` in `internal/middleware/middleware.go`) + registered globally in `internal/server/routes.go`, so HSTS, CSP, + `X-Frame-Options`, `X-Content-Type-Options`, `Referrer-Policy`, and + `Permissions-Policy` are set on every response including `/s/...` and + `/metrics`; the CSP needs no `unsafe-inline`/`unsafe-eval` because the + dashboard ships no JavaScript and no inline styles; HSTS is emitted + unconditionally per policy (TLS-terminating proxy in front). Remaining + 1.0 hardening items — `http.Server` timeouts, request body limits, + rate limiting, CORS scoping — are tracked separately - 2026-08-07: golangci-lint bumped to v2.12.2 (commit-pinned installs in `Dockerfile` and `script/bootstrap`); `.golangci.yml` set to the org-standard v2-schema config used across the org's repos diff --git a/internal/middleware/middleware.go b/internal/middleware/middleware.go index 0a05dd5..03f435e 100644 --- a/internal/middleware/middleware.go +++ b/internal/middleware/middleware.go @@ -21,6 +21,60 @@ import ( // corsMaxAge is the maximum age for CORS preflight responses. const corsMaxAge = 300 +// Security response header values applied to every response. +// +// The CSP is as strict as the dashboard allows: the template ships no +// JavaScript, no inline styles, no inline event handlers and no images, +// and its only subresource is the embedded stylesheet at +// /s/css/tailwind.min.css, which style-src 'self' permits. Neither +// unsafe-inline nor unsafe-eval is used. frame-ancestors 'none' is the +// primary anti-framing control; X-Frame-Options is the legacy fallback. +const ( + // hstsValue is emitted unconditionally, including over plain HTTP, + // because the service runs behind a TLS-terminating proxy and the + // browser must still enforce HTTPS end to end. + hstsValue = "max-age=31536000; includeSubDomains" + + cspValue = "default-src 'self'; " + + "script-src 'none'; " + + "style-src 'self'; " + + "img-src 'self'; " + + "font-src 'none'; " + + "connect-src 'none'; " + + "object-src 'none'; " + + "base-uri 'none'; " + + "form-action 'none'; " + + "frame-ancestors 'none'" + + frameOptionsValue = "DENY" + + contentTypeOptionsValue = "nosniff" + + // referrerPolicyValue is stricter than the policy minimum of + // strict-origin-when-cross-origin: the dashboard has no + // cross-origin navigation needs and its URL may name internal + // hosts. + referrerPolicyValue = "no-referrer" + + permissionsPolicyValue = "accelerometer=(), " + + "autoplay=(), " + + "camera=(), " + + "display-capture=(), " + + "encrypted-media=(), " + + "fullscreen=(), " + + "geolocation=(), " + + "gyroscope=(), " + + "magnetometer=(), " + + "microphone=(), " + + "midi=(), " + + "payment=(), " + + "picture-in-picture=(), " + + "publickey-credentials-get=(), " + + "screen-wake-lock=(), " + + "usb=(), " + + "xr-spatial-tracking=()" +) + // Params contains dependencies for Middleware. type Params struct { fx.In @@ -186,6 +240,37 @@ func (m *Middleware) CORS() func(http.Handler) http.Handler { }) } +// SecurityHeaders returns middleware that sets the security response +// headers required for production internet exposure on every response. +// +// The headers are set before the request reaches the next handler so +// that they are present on every response, including panics recovered +// by chi's Recoverer and timeouts produced by chi's Timeout. +func (m *Middleware) SecurityHeaders() func(http.Handler) http.Handler { + return func(next http.Handler) http.Handler { + return http.HandlerFunc(func( + writer http.ResponseWriter, + request *http.Request, + ) { + header := writer.Header() + header.Set("Strict-Transport-Security", hstsValue) + header.Set("Content-Security-Policy", cspValue) + header.Set("X-Frame-Options", frameOptionsValue) + header.Set( + "X-Content-Type-Options", + contentTypeOptionsValue, + ) + header.Set("Referrer-Policy", referrerPolicyValue) + header.Set( + "Permissions-Policy", + permissionsPolicyValue, + ) + + next.ServeHTTP(writer, request) + }) + } +} + // MetricsAuth returns basic auth middleware for /metrics. func (m *Middleware) MetricsAuth() func(http.Handler) http.Handler { if m.params.Config.MetricsUsername == "" { diff --git a/internal/middleware/middleware_test.go b/internal/middleware/middleware_test.go new file mode 100644 index 0000000..598f476 --- /dev/null +++ b/internal/middleware/middleware_test.go @@ -0,0 +1,334 @@ +package middleware_test + +import ( + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/go-chi/chi/v5" + "go.uber.org/fx/fxtest" + + "sneak.berlin/go/dnswatcher/internal/config" + "sneak.berlin/go/dnswatcher/internal/globals" + "sneak.berlin/go/dnswatcher/internal/handlers" + "sneak.berlin/go/dnswatcher/internal/logger" + "sneak.berlin/go/dnswatcher/internal/middleware" + "sneak.berlin/go/dnswatcher/internal/notify" + "sneak.berlin/go/dnswatcher/internal/state" +) + +// Expected security header values, spelled out literally so that any +// change to the middleware has to be made deliberately here as well. +const ( + wantHSTS = "max-age=31536000; includeSubDomains" + + wantCSP = "default-src 'self'; " + + "script-src 'none'; " + + "style-src 'self'; " + + "img-src 'self'; " + + "font-src 'none'; " + + "connect-src 'none'; " + + "object-src 'none'; " + + "base-uri 'none'; " + + "form-action 'none'; " + + "frame-ancestors 'none'" + + wantFrameOptions = "DENY" + + wantContentTypeOptions = "nosniff" + + wantReferrerPolicy = "no-referrer" + + wantPermissionsPolicy = "accelerometer=(), " + + "autoplay=(), " + + "camera=(), " + + "display-capture=(), " + + "encrypted-media=(), " + + "fullscreen=(), " + + "geolocation=(), " + + "gyroscope=(), " + + "magnetometer=(), " + + "microphone=(), " + + "midi=(), " + + "payment=(), " + + "picture-in-picture=(), " + + "publickey-credentials-get=(), " + + "screen-wake-lock=(), " + + "usb=(), " + + "xr-spatial-tracking=()" +) + +// stylesheetPath is the only subresource the dashboard loads. +const stylesheetPath = "/s/css/tailwind.min.css" + +// newTestLogger builds a logger for direct component construction. +func newTestLogger(t *testing.T) *logger.Logger { + t.Helper() + + glob, err := globals.New(nil) + if err != nil { + t.Fatalf("globals.New: %v", err) + } + + log, err := logger.New(nil, logger.Params{Globals: glob}) + if err != nil { + t.Fatalf("logger.New: %v", err) + } + + return log +} + +// newTestMiddleware builds a Middleware without an fx application. +func newTestMiddleware(t *testing.T) *middleware.Middleware { + t.Helper() + + glob, err := globals.New(nil) + if err != nil { + t.Fatalf("globals.New: %v", err) + } + + mw, err := middleware.New(nil, middleware.Params{ + Logger: newTestLogger(t), + Globals: glob, + Config: &config.Config{}, + }) + if err != nil { + t.Fatalf("middleware.New: %v", err) + } + + return mw +} + +// serveWithSecurityHeaders runs a GET through SecurityHeaders and +// returns the recorded response. +func serveWithSecurityHeaders( + t *testing.T, + target string, + handler http.Handler, +) *httptest.ResponseRecorder { + t.Helper() + + mw := newTestMiddleware(t) + rec := httptest.NewRecorder() + req := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, target, nil, + ) + + mw.SecurityHeaders()(handler).ServeHTTP(rec, req) + + return rec +} + +// okHandler writes a trivial 200 response. +func okHandler() http.Handler { + return http.HandlerFunc(func( + writer http.ResponseWriter, + _ *http.Request, + ) { + writer.WriteHeader(http.StatusOK) + }) +} + +func TestSecurityHeaders(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + header string + want string + }{ + { + "hsts", + "Strict-Transport-Security", + wantHSTS, + }, + { + "csp", + "Content-Security-Policy", + wantCSP, + }, + { + "frame options", + "X-Frame-Options", + wantFrameOptions, + }, + { + "content type options", + "X-Content-Type-Options", + wantContentTypeOptions, + }, + { + "referrer policy", + "Referrer-Policy", + wantReferrerPolicy, + }, + { + "permissions policy", + "Permissions-Policy", + wantPermissionsPolicy, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + rec := serveWithSecurityHeaders(t, "/", okHandler()) + + got := rec.Header().Get(tt.header) + if got != tt.want { + t.Errorf( + "%s = %q, want %q", + tt.header, got, tt.want, + ) + } + }) + } +} + +// TestSecurityHeadersCSPDirectives guards the properties the repo +// policy requires of the content security policy itself. +func TestSecurityHeadersCSPDirectives(t *testing.T) { + t.Parallel() + + rec := serveWithSecurityHeaders(t, "/", okHandler()) + csp := rec.Header().Get("Content-Security-Policy") + + forbidden := []string{"unsafe-inline", "unsafe-eval"} + for _, directive := range forbidden { + if strings.Contains(csp, directive) { + t.Errorf("CSP must not contain %q: %q", directive, csp) + } + } + + required := []string{ + "default-src 'self'", + "script-src 'none'", + "style-src 'self'", + "frame-ancestors 'none'", + } + for _, directive := range required { + if !strings.Contains(csp, directive) { + t.Errorf("CSP must contain %q: %q", directive, csp) + } + } +} + +// TestSecurityHeadersOnErrorResponse verifies the headers are emitted +// even when the wrapped handler fails, since they are set before the +// handler runs. +func TestSecurityHeadersOnErrorResponse(t *testing.T) { + t.Parallel() + + failing := http.HandlerFunc(func( + writer http.ResponseWriter, + _ *http.Request, + ) { + http.Error( + writer, + "boom", + http.StatusInternalServerError, + ) + }) + + rec := serveWithSecurityHeaders(t, "/api/v1/status", failing) + + if rec.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500", rec.Code) + } + + if got := rec.Header().Get( + "X-Content-Type-Options", + ); got != wantContentTypeOptions { + t.Errorf( + "X-Content-Type-Options = %q, want %q", + got, wantContentTypeOptions, + ) + } + + if got := rec.Header().Get( + "Strict-Transport-Security", + ); got != wantHSTS { + t.Errorf( + "Strict-Transport-Security = %q, want %q", + got, wantHSTS, + ) + } +} + +// newTestHandlers builds real Handlers with empty monitoring state. +func newTestHandlers(t *testing.T) *handlers.Handlers { + t.Helper() + + glob, err := globals.New(nil) + if err != nil { + t.Fatalf("globals.New: %v", err) + } + + log := newTestLogger(t) + + notifier, err := notify.New(fxtest.NewLifecycle(t), notify.Params{ + Logger: log, + Config: &config.Config{}, + }) + if err != nil { + t.Fatalf("notify.New: %v", err) + } + + hnd, err := handlers.New(nil, handlers.Params{ + Logger: log, + Globals: glob, + State: state.NewForTest(), + Notify: notifier, + }) + if err != nil { + t.Fatalf("handlers.New: %v", err) + } + + return hnd +} + +// TestDashboardRendersWithSecurityHeaders renders the real dashboard +// through the middleware and checks that the policy still permits the +// one stylesheet the page loads. +func TestDashboardRendersWithSecurityHeaders(t *testing.T) { + t.Parallel() + + mw := newTestMiddleware(t) + hnd := newTestHandlers(t) + + router := chi.NewRouter() + router.Use(mw.SecurityHeaders()) + router.Get("/", hnd.HandleDashboard()) + + rec := httptest.NewRecorder() + req := httptest.NewRequestWithContext( + t.Context(), http.MethodGet, "/", nil, + ) + + router.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + + body := rec.Body.String() + if !strings.Contains(body, stylesheetPath) { + t.Errorf("dashboard does not reference %q", stylesheetPath) + } + + if !strings.Contains(body, "dnswatcher") { + t.Errorf("dashboard body looks empty: %d bytes", len(body)) + } + + csp := rec.Header().Get("Content-Security-Policy") + if csp != wantCSP { + t.Errorf("CSP = %q, want %q", csp, wantCSP) + } + + // The stylesheet is same-origin, so style-src 'self' allows it. + if !strings.Contains(csp, "style-src 'self'") { + t.Errorf("CSP would block %q: %q", stylesheetPath, csp) + } +} diff --git a/internal/server/routes.go b/internal/server/routes.go index fa99177..5c71d84 100644 --- a/internal/server/routes.go +++ b/internal/server/routes.go @@ -21,6 +21,7 @@ func (s *Server) SetupRoutes() { // Global middleware s.router.Use(chimw.Recoverer) s.router.Use(chimw.RequestID) + s.router.Use(s.mw.SecurityHeaders()) s.router.Use(s.mw.Logging()) s.router.Use(s.mw.CORS()) s.router.Use(chimw.Timeout(requestTimeout)) -- 2.54.0 From 148e47d9c0fbefd3e4416c2d6d3e862b175c78bd Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 20:13:37 +0200 Subject: [PATCH 12/15] docker: run as non-root, add health check, document upaas (closes #147) The runtime image runs as uid 10001, which owns /var/lib/dnswatcher. The working directory is /, so config loading finds no .env or dnswatcher config file there; the binary lives in /usr/local/bin. A Docker HEALTHCHECK probes /.well-known/healthcheck every 10 seconds with busybox wget, well inside the 60 seconds upaas waits. Startup now fails with an error naming the data directory when it cannot be written, instead of running with every save failing. The check creates the directory if needed and writes and removes the temp file Save uses; tests cover the create and the write failing. README gains "Running under upaas": the prod branch, host directory setup, network and port, environment and health check. Model: opus-5-5 --- Dockerfile | 30 ++++++++-- README.md | 52 ++++++++++++++++- TODO.md | 3 + internal/state/state.go | 29 ++++++++++ internal/state/state_test.go | 107 +++++++++++++++++++++++++++++++++++ 5 files changed, 212 insertions(+), 9 deletions(-) diff --git a/Dockerfile b/Dockerfile index 94b5b57..48b467e 100644 --- a/Dockerfile +++ b/Dockerfile @@ -41,15 +41,33 @@ FROM alpine@sha256:c3f8e73fdb79deaebaa2037150150191b9dcbfba68b4a46d70103204c53f4 RUN apk add --no-cache ca-certificates tzdata -WORKDIR /app +COPY --from=builder /src/bin/dnswatcher /usr/local/bin/dnswatcher -COPY --from=builder /src/bin/dnswatcher /app/dnswatcher - -# Create data directory -RUN mkdir -p /var/lib/dnswatcher +# Run as an unprivileged user that owns the data directory. A fresh named +# volume inherits this ownership; a bind-mounted host directory must be +# owned by uid 10001 (see "Running under upaas" in README.md), or startup +# fails. +RUN addgroup -S -g 10001 dnswatcher \ + && adduser -S -G dnswatcher -u 10001 dnswatcher \ + && mkdir -p /var/lib/dnswatcher \ + && chown dnswatcher:dnswatcher /var/lib/dnswatcher ENV DNSWATCHER_DATA_DIR=/var/lib/dnswatcher +# Config loading also reads a `.env` file and a file named `dnswatcher` +# (any config extension, or none) from the working directory. `/` holds +# neither, so every setting comes from the environment. Do not make the +# data directory, or the binary's directory, the working directory. +WORKDIR / + +USER dnswatcher + EXPOSE 8080 -ENTRYPOINT ["/app/dnswatcher"] +# busybox wget (already in alpine) probes the health endpoint every 10 +# seconds, so the container is healthy well before upaas reads its health +# 60 seconds after a deploy and fails the deploy unless it is healthy. +HEALTHCHECK --interval=10s --timeout=5s --start-period=10s --retries=3 \ + CMD wget -q -O /dev/null "http://127.0.0.1:${PORT:-8080}/.well-known/healthcheck" || exit 1 + +ENTRYPOINT ["/usr/local/bin/dnswatcher"] diff --git a/README.md b/README.md index 2dae62c..08d7d55 100644 --- a/README.md +++ b/README.md @@ -508,11 +508,57 @@ docker run -d \ --- +## Running under upaas + +[upaas](https://git.eeqj.de/sneak/upaas) builds the image from this +repository's `Dockerfile` and runs it. The app needs: + +- **Branch:** `prod`. `prod` is cut from `main`, and merging a `main` to + `prod` pull request is a deploy. +- **Volume:** one host directory mounted at `/var/lib/dnswatcher`, where + the state file lives. upaas bind-mounts the host path it is given and + does not create it. The container runs as uid 10001 and does not start + unless it can write there. Create the directory before the first + deploy: + + ```sh + mkdir -p /path/to/data + chown 10001:10001 /path/to/data + chmod 700 /path/to/data + ``` + +- **Network and port:** the dashboard is unauthenticated and shows every + watched name and recent alert, and upaas publishes every mapped port on + all interfaces of the host + ([upaas issue 113](https://git.eeqj.de/sneak/upaas/issues/113)). Add a + port mapping to container port `8080` only if the dashboard should be + public. Otherwise add none: set the app's Docker network in upaas to + your reverse proxy's Docker network, and the proxy reaches the app at + `upaas-` followed by the app name, port `8080`. +- **Required environment:** `DNSWATCHER_TARGETS`, a comma-separated list + of the domains and hostnames to watch. dnswatcher refuses to start + without it. +- **Recommended environment:** at least one notification endpoint + (`DNSWATCHER_SLACK_WEBHOOK`, `DNSWATCHER_MATTERMOST_WEBHOOK`, + `DNSWATCHER_NTFY_TOPIC`); without one, changes show only on the + dashboard. `DNSWATCHER_METRICS_USERNAME` and + `DNSWATCHER_METRICS_PASSWORD` serve `/metrics` behind basic auth. +- **Leave unset:** `DNSWATCHER_DATA_DIR`, which the image sets to + `/var/lib/dnswatcher`, and `PORT`, which defaults to `8080`. Every + setting comes from the environment; the image holds no config file. +- **Health check:** the image's own, which requests + `/.well-known/healthcheck` every 10 seconds. upaas reads the + container's health 60 seconds after a deploy and marks the deploy + failed unless it is `healthy`. + +--- + ## Monitoring Lifecycle -1. **Startup**: Load state from disk. If no state file exists, start - with empty state (first check will establish baseline without - triggering change notifications). +1. **Startup**: Check that the data directory can be written, and exit + with an error naming it if not. Load state from disk. If no state + file exists, start with empty state (first check will establish + baseline without triggering change notifications). 2. **Initial check**: Immediately perform all DNS, port, and TLS checks on startup. 3. **Periodic checks** (DNS always runs first): diff --git a/TODO.md b/TODO.md index a0596cc..e3fee17 100644 --- a/TODO.md +++ b/TODO.md @@ -23,6 +23,9 @@ Rationale, Design, TODO, License, Author) if any are still missing. # Completed Steps +- 2026-09-28: upaas deploy readiness — runtime image runs as unprivileged + `dnswatcher`, Docker `HEALTHCHECK`, startup fails when the data directory is + not writable, README "Running under upaas" (closes #147). - 2026-09-21: added behavioural tests for `internal/globals`, `internal/healthcheck`, and `internal/logger` (closes #110). - 2026-09-21: `go mod tidy` dropped the redundant `golang.org/x/sync` diff --git a/internal/state/state.go b/internal/state/state.go index efe681c..417a4d7 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -148,6 +148,11 @@ func New( lifecycle.Append(fx.Hook{ OnStart: func(_ context.Context) error { + err := state.checkDataDirWritable() + if err != nil { + return err + } + return state.Load() }, OnStop: func(_ context.Context) error { @@ -345,3 +350,27 @@ func (s *State) GetCertificateState( return cs, ok } + +// checkDataDirWritable creates the data directory if needed, then writes +// and removes the temp file that Save uses. It runs at startup so that an +// unwritable directory stops the process, instead of the process running +// with every save failing and only logged. +func (s *State) checkDataDirWritable() error { + dir := s.config.DataDir + tmpPath := s.config.StatePath() + ".tmp" + + err := os.MkdirAll(dir, dirPermissions) + if err == nil { + err = os.WriteFile(tmpPath, nil, filePermissions) + } + + if err == nil { + err = os.Remove(tmpPath) + } + + if err != nil { + return fmt.Errorf("data directory %s is not writable: %w", dir, err) + } + + return nil +} diff --git a/internal/state/state_test.go b/internal/state/state_test.go index 699d17c..3fbca93 100644 --- a/internal/state/state_test.go +++ b/internal/state/state_test.go @@ -4,10 +4,16 @@ import ( "encoding/json" "os" "path/filepath" + "strings" "sync" "testing" "time" + "go.uber.org/fx/fxtest" + + "sneak.berlin/go/dnswatcher/internal/config" + "sneak.berlin/go/dnswatcher/internal/globals" + "sneak.berlin/go/dnswatcher/internal/logger" "sneak.berlin/go/dnswatcher/internal/state" ) @@ -493,6 +499,107 @@ func TestSaveWritePermissionError(t *testing.T) { } } +// startState builds a State through the real constructor and runs its +// startup hook against dataDir, returning the startup error. +func startState(t *testing.T, dataDir string) error { + t.Helper() + + g, err := globals.New(nil) + if err != nil { + t.Fatalf("globals.New: %v", err) + } + + log, err := logger.New(nil, logger.Params{Globals: g}) + if err != nil { + t.Fatalf("logger.New: %v", err) + } + + lifecycle := fxtest.NewLifecycle(t) + + _, err = state.New(lifecycle, state.Params{ + Logger: log, + Config: &config.Config{DataDir: dataDir}, + }) + if err != nil { + t.Fatalf("state.New: %v", err) + } + + return lifecycle.Start(t.Context()) +} + +// TestStartupFailsWhenDataDirNotWritable verifies that startup stops +// with an error naming the data directory when it cannot be written. +// The directory's parent is a regular file, which also fails as root. +func TestStartupFailsWhenDataDirNotWritable(t *testing.T) { + t.Parallel() + + parent := filepath.Join(t.TempDir(), "file") + + err := os.WriteFile(parent, nil, 0o600) + if err != nil { + t.Fatalf("writing file: %v", err) + } + + dataDir := filepath.Join(parent, "data") + + err = startState(t, dataDir) + if err == nil { + t.Fatal("startup should fail when the data directory is not writable") + } + + want := "data directory " + dataDir + " is not writable" + if !strings.Contains(err.Error(), want) { + t.Errorf("startup error %q does not contain %q", err, want) + } +} + +// TestStartupFailsWhenExistingDataDirNotWritable verifies that startup +// stops when the data directory exists but the temp file that saving uses +// cannot be written in it. A directory sitting at the temp file's path +// makes that write fail, which also holds as root. +func TestStartupFailsWhenExistingDataDirNotWritable(t *testing.T) { + t.Parallel() + + dataDir := t.TempDir() + + err := os.Mkdir(filepath.Join(dataDir, "state.json.tmp"), 0o700) + if err != nil { + t.Fatalf("creating directory: %v", err) + } + + err = startState(t, dataDir) + if err == nil { + t.Fatal("startup should fail when the data directory is not writable") + } + + want := "data directory " + dataDir + " is not writable" + if !strings.Contains(err.Error(), want) { + t.Errorf("startup error %q does not contain %q", err, want) + } +} + +// TestStartupCreatesDataDir verifies that startup creates a missing +// data directory and leaves nothing behind in it. +func TestStartupCreatesDataDir(t *testing.T) { + t.Parallel() + + dataDir := filepath.Join(t.TempDir(), "data") + + err := startState(t, dataDir) + if err != nil { + t.Fatalf("startup error: %v", err) + } + + entries, err := os.ReadDir(dataDir) + if err != nil { + t.Fatalf("reading data directory: %v", err) + } + + if len(entries) != 0 { + t.Errorf("startup left %d entries in the data directory", len(entries)) + } +} + // TestPortStateUnmarshalJSON_NewFormat verifies deserialization of the // current multi-hostname format. func TestPortStateUnmarshalJSON_NewFormat(t *testing.T) { -- 2.54.0 From 19f282c8b3122e916e6809c6890a8135c43d36ca Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 22:01:36 +0200 Subject: [PATCH 13/15] server: assert timeouts on the served http.Server, not just the constructor (closes #120) The timeout tests called newHTTPServer directly, so a Run that built its http.Server inline would drop every timeout with the suite still green. TestRunWiresSocketTimeouts wires a Server as cmd/dnswatcher does, minus the watcher and resolver so no live DNS is touched, drives Run with an unbindable port so it stores its http.Server and returns without listening, and checks that server carries all four timeouts and both required relationships. The addr/handler test, which could not fail, is dropped. The ReadTimeout note now says what net/http does: a request whose headers arrive after ReadTimeout but within ReadHeaderTimeout gets a read deadline that has already passed, so reading its body fails at once. Model: opus-4-8 (implementation); opus-5-5 (rework) --- TODO.md | 3 + internal/server/export_test.go | 21 ++-- internal/server/server_test.go | 178 ++++++++++++++++++--------------- 3 files changed, 114 insertions(+), 88 deletions(-) diff --git a/TODO.md b/TODO.md index e3fee17..10c50d2 100644 --- a/TODO.md +++ b/TODO.md @@ -23,6 +23,9 @@ Rationale, Design, TODO, License, Author) if any are still missing. # Completed Steps +- 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 `dnswatcher`, Docker `HEALTHCHECK`, startup fails when the data directory is not writable, README "Running under upaas" (closes #147). diff --git a/internal/server/export_test.go b/internal/server/export_test.go index 8df05f2..5a1bec7 100644 --- a/internal/server/export_test.go +++ b/internal/server/export_test.go @@ -5,15 +5,20 @@ import ( "time" ) -// NewHTTPServer exports newHTTPServer for testing. -func NewHTTPServer( - listenAddr string, - handler http.Handler, -) *http.Server { - return newHTTPServer(listenAddr, handler) -} - // RequestTimeout exports the handler execution budget applied by // chimw.Timeout in SetupRoutes, so tests can assert the relationship // between it and the server's WriteTimeout. const RequestTimeout time.Duration = requestTimeout + +// SetListenPort overrides the port Run binds. A test uses it to hand +// Run an unbindable port so ListenAndServe fails immediately and Run +// returns after storing its http.Server. +func SetListenPort(s *Server, port int) { + s.port = port +} + +// HTTPServerOf returns the http.Server that Run built and stored, so a +// test can inspect the timeouts the running server actually carries. +func HTTPServerOf(s *Server) *http.Server { + return s.httpServer +} diff --git a/internal/server/server_test.go b/internal/server/server_test.go index 9909f7d..6cd036c 100644 --- a/internal/server/server_test.go +++ b/internal/server/server_test.go @@ -1,112 +1,130 @@ package server_test import ( - "net/http" "testing" + "github.com/spf13/viper" + "go.uber.org/fx" + + "sneak.berlin/go/dnswatcher/internal/config" + "sneak.berlin/go/dnswatcher/internal/globals" + "sneak.berlin/go/dnswatcher/internal/handlers" + "sneak.berlin/go/dnswatcher/internal/healthcheck" + "sneak.berlin/go/dnswatcher/internal/logger" + "sneak.berlin/go/dnswatcher/internal/middleware" + "sneak.berlin/go/dnswatcher/internal/notify" "sneak.berlin/go/dnswatcher/internal/server" + "sneak.berlin/go/dnswatcher/internal/state" ) -// noopHandler stands in for the router; newHTTPServer only stores it. -func noopHandler() http.Handler { - return http.HandlerFunc( - func(w http.ResponseWriter, _ *http.Request) { - w.WriteHeader(http.StatusOK) - }, +// buildServer wires a *server.Server exactly as cmd/dnswatcher does, +// minus the watcher/resolver subtree that would touch live DNS. fx +// builds the object graph but the lifecycle is never started, so no +// OnStart hook runs and nothing listens or resolves. The caller must +// first configure viper (config.New reads it), which is also why the +// caller cannot run in parallel. +func buildServer(t *testing.T) *server.Server { + t.Helper() + + var srv *server.Server + + app := fx.New( + fx.NopLogger, + fx.Provide( + globals.New, + logger.New, + config.New, + state.New, + healthcheck.New, + notify.New, + middleware.New, + handlers.New, + server.New, + ), + fx.Populate(&srv), ) -} -// TestHTTPServerTimeoutsAreSet asserts that every socket-level -// timeout is configured. A zero value in net/http means "no limit", -// so a refactor that silently drops one of these reintroduces the -// slowloris / unreaped-keep-alive exposure this guards against. -// -// The assertions are on the configured field values only; nothing -// here measures elapsed time, so the test cannot flake on timing. -func TestHTTPServerTimeoutsAreSet(t *testing.T) { - t.Parallel() - - srv := server.NewHTTPServer(":8080", noopHandler()) - - if srv.ReadTimeout <= 0 { - t.Errorf( - "ReadTimeout must be non-zero, got %v", - srv.ReadTimeout, - ) + err := app.Err() + if err != nil { + t.Fatalf("building server graph: %v", err) } - if srv.ReadHeaderTimeout <= 0 { + return srv +} + +// TestRunWiresSocketTimeouts pins that the http.Server the running +// server actually serves — the one Run builds and hands to +// ListenAndServe — carries every socket-level timeout, plus the two +// relationships the values must satisfy. +// +// Run is driven to completion with an unbindable port: it builds and +// stores s.httpServer, then ListenAndServe fails at once and Run +// returns without ever listening. The assertions run in the same +// goroutine after Run returns, so reading s.httpServer is free of any +// data race. Nothing here measures elapsed time. +// +// ReadTimeout must be at least ReadHeaderTimeout. net/http reads the +// headers under ReadHeaderTimeout, then sets the read deadline for the +// rest of the request to ReadTimeout, counted from when it started +// reading the request. If ReadTimeout were smaller, a request whose +// headers arrived after ReadTimeout but within ReadHeaderTimeout would +// get a read deadline that had already passed, so reading its body +// would fail at once. +func TestRunWiresSocketTimeouts(t *testing.T) { + // Sets an env var and touches viper global state, so like the + // config tests it cannot use t.Parallel. + viper.Reset() + t.Setenv("DNSWATCHER_TARGETS", "example.com") + + srv := buildServer(t) + server.SetListenPort(srv, -1) + + srv.Run() + + hs := server.HTTPServerOf(srv) + if hs == nil { + t.Fatal("Run did not build an http.Server") + } + + if hs.ReadTimeout <= 0 { + t.Errorf("ReadTimeout must be non-zero, got %v", hs.ReadTimeout) + } + + if hs.ReadHeaderTimeout <= 0 { t.Errorf( "ReadHeaderTimeout must be non-zero, got %v", - srv.ReadHeaderTimeout, + hs.ReadHeaderTimeout, ) } - if srv.WriteTimeout <= 0 { - t.Errorf( - "WriteTimeout must be non-zero, got %v", - srv.WriteTimeout, - ) + if hs.WriteTimeout <= 0 { + t.Errorf("WriteTimeout must be non-zero, got %v", hs.WriteTimeout) } - if srv.IdleTimeout <= 0 { - t.Errorf( - "IdleTimeout must be non-zero, got %v", - srv.IdleTimeout, - ) + if hs.IdleTimeout <= 0 { + t.Errorf("IdleTimeout must be non-zero, got %v", hs.IdleTimeout) } -} -// TestWriteTimeoutExceedsHandlerBudget pins the one relationship the -// values must satisfy. net/http arms the write deadline once request -// headers are read, so it covers handler execution plus the response -// flush. If WriteTimeout were not greater than the chimw.Timeout -// handler budget, the connection would be severed before a handler -// that used its full budget could respond, making that budget -// unreachable. -func TestWriteTimeoutExceedsHandlerBudget(t *testing.T) { - t.Parallel() - - srv := server.NewHTTPServer(":8080", noopHandler()) - - if srv.WriteTimeout <= server.RequestTimeout { + if hs.WriteTimeout <= server.RequestTimeout { t.Errorf( "WriteTimeout (%v) must exceed handler budget (%v)", - srv.WriteTimeout, + hs.WriteTimeout, server.RequestTimeout, ) } -} -// TestReadTimeoutCoversHeaderTimeout asserts the read deadline for -// the whole request is at least as long as the header-only deadline; -// a smaller ReadTimeout would make ReadHeaderTimeout unreachable. -func TestReadTimeoutCoversHeaderTimeout(t *testing.T) { - t.Parallel() - - srv := server.NewHTTPServer(":8080", noopHandler()) - - if srv.ReadTimeout < srv.ReadHeaderTimeout { + if hs.ReadTimeout < hs.ReadHeaderTimeout { t.Errorf( "ReadTimeout (%v) must be >= ReadHeaderTimeout (%v)", - srv.ReadTimeout, - srv.ReadHeaderTimeout, + hs.ReadTimeout, + hs.ReadHeaderTimeout, + ) + } + + if hs.Handler != srv { + t.Errorf( + "Run wired handler %T, want the *server.Server", + hs.Handler, ) } } - -// TestHTTPServerAddrAndHandler covers the rest of the constructor so -// a future edit cannot drop the listen address or the handler. -func TestHTTPServerAddrAndHandler(t *testing.T) { - t.Parallel() - - srv := server.NewHTTPServer(":9999", noopHandler()) - - if srv.Addr != ":9999" { - t.Errorf("Addr = %q, want %q", srv.Addr, ":9999") - } - - if srv.Handler == nil { - t.Error("Handler must not be nil") - } -} -- 2.54.0 From 1ab0b9f61db060a565029b163129da87077849ab Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Mon, 28 Sep 2026 23:13:38 +0200 Subject: [PATCH 14/15] script: force lint and test to run in cibuild and docker (closes #115) script/cibuild and script/docker were plain docker build. On an unchanged tree the lint stage and the builder stage, which runs make test, came from the layer cache, so the build passed without linting or querying live DNS. Both scripts now pass --no-cache-filter=lint,builder so those stages run on every build, as script/lint already does for its own lint stage. Dependency downloads inside those stages re-run each build. Each of the two stages in the Dockerfile now notes that the scripts name it. README and TODO.md updated to match. Model: opus-4-8 (implementation); opus-5-5 (rework) --- Dockerfile | 2 ++ README.md | 10 +++++++--- TODO.md | 3 +++ script/cibuild | 8 ++++++-- script/docker | 8 ++++++-- 5 files changed, 24 insertions(+), 7 deletions(-) diff --git a/Dockerfile b/Dockerfile index 48b467e..b4f7273 100644 --- a/Dockerfile +++ b/Dockerfile @@ -2,6 +2,7 @@ # The linter is invoked directly rather than through `make lint`: that # target shells out to `docker build -f Dockerfile.lint`, and there is # 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 FROM golangci/golangci-lint:v2.12.2@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240 AS lint @@ -15,6 +16,7 @@ RUN make fmt-check RUN golangci-lint run --config .golangci.yml ./... # Build stage +# script/cibuild and script/docker name this stage in --no-cache-filter. # golang 1.25-alpine, 2026-02-28 FROM golang@sha256:f6751d823c26342f9506c03797d2527668d095b0a15f1862cddb4d927a7a4ced AS builder diff --git a/README.md b/README.md index 08d7d55..13c9ebb 100644 --- a/README.md +++ b/README.md @@ -465,9 +465,13 @@ them. We provide: - `script/fmt` — format all code (gofmt -s, goimports) - `script/fmt-check` — check formatting (read-only) - `script/check` — run test, lint, and fmt-check -- `script/docker` — build the Docker image tagged via - `script/projectname` -- `script/cibuild` — CI entrypoint: plain `docker build .` +- `script/docker` — build the Docker image tagged via `script/projectname`, with + `--no-cache-filter=lint,builder` so the lint stage and the builder stage, + which runs the tests, run on every invocation +- `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` guard, then `script/check` - `script/install-precommit` — install the git pre-commit hook diff --git a/TODO.md b/TODO.md index 10c50d2..575b959 100644 --- a/TODO.md +++ b/TODO.md @@ -23,6 +23,9 @@ Rationale, Design, TODO, License, Author) if any are still missing. # Completed Steps +- 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). diff --git a/script/cibuild b/script/cibuild index 1b9e57d..29bea03 100755 --- a/script/cibuild +++ b/script/cibuild @@ -1,14 +1,18 @@ #!/bin/sh # script/cibuild: run the CI build. The Dockerfile's lint stage runs # make fmt-check and golangci-lint; its builder stage runs make test -# and make build. A successful build implies all of those passed. +# and make build. +# +# --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 ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" main() { cd "$ROOT" - docker build . + docker build --no-cache-filter=lint,builder . } main "$@" diff --git a/script/docker b/script/docker index 9b9ea86..4f1fd14 100755 --- a/script/docker +++ b/script/docker @@ -1,6 +1,10 @@ #!/bin/sh # script/docker: build the Docker image tagged with the project name. -# Identical in all repos; the tag comes from script/projectname. +# 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 SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd -P)" @@ -8,7 +12,7 @@ ROOT="$(cd "$SCRIPT_DIR/.." && pwd -P)" main() { cd "$ROOT" - docker build -t "$("$SCRIPT_DIR/projectname")" . + docker build --no-cache-filter=lint,builder -t "$("$SCRIPT_DIR/projectname")" . } main "$@" -- 2.54.0 From 95b017eb3e1cf166f6dc17a12fb2e68f9be554f8 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Tue, 29 Sep 2026 00:30:29 +0200 Subject: [PATCH 15/15] resolver: lower-case DNS names in record values (closes #157) Nameservers may answer with DNS names in any letter case. For eeqj.de, y.ns.joker.com answers in upper case while its peers answer in lower case, so the inconsistency check fired on every cycle. extractRecordValue now lower-cases CNAME, MX, SRV and NS targets, so the inconsistency check and the record-change check both compare names regardless of case. A, AAAA, TXT and CAA values are formatted as before. State saved before this change can hold upper-case names, which report a one-time record change on the first check after upgrading. Model: opus-5-5 --- README.md | 4 ++ TODO.md | 3 ++ internal/resolver/export_test.go | 8 ++++ internal/resolver/iterative.go | 13 +++--- internal/resolver/iterative_test.go | 62 +++++++++++++++++++++++++++++ 5 files changed, 85 insertions(+), 5 deletions(-) create mode 100644 internal/resolver/export_test.go create mode 100644 internal/resolver/iterative_test.go diff --git a/README.md b/README.md index 13c9ebb..8e51883 100644 --- a/README.md +++ b/README.md @@ -61,6 +61,10 @@ rejected. record types: A, AAAA, CNAME, MX, TXT, SRV, CAA, NS. - Stores results **per nameserver**. The state for a hostname is not a 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 notification. This includes: - **Record change**: A nameserver returns different records than it diff --git a/TODO.md b/TODO.md index 575b959..36a1c9a 100644 --- a/TODO.md +++ b/TODO.md @@ -23,6 +23,9 @@ Rationale, Design, TODO, License, Author) if any are still missing. # Completed Steps +- 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). diff --git a/internal/resolver/export_test.go b/internal/resolver/export_test.go new file mode 100644 index 0000000..6f4f9f1 --- /dev/null +++ b/internal/resolver/export_test.go @@ -0,0 +1,8 @@ +package resolver + +import "github.com/miekg/dns" + +// ExtractRecordValue exports extractRecordValue for testing. +func ExtractRecordValue(rr dns.RR) string { + return extractRecordValue(rr) +} diff --git a/internal/resolver/iterative.go b/internal/resolver/iterative.go index eebab39..f89d73d 100644 --- a/internal/resolver/iterative.go +++ b/internal/resolver/iterative.go @@ -608,7 +608,10 @@ func classifyResponse(resp *NameserverResponse, state queryState) { } } -// extractRecordValue formats a DNS RR value as a string. +// extractRecordValue formats a DNS RR value as a string. DNS names +// 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 { switch r := rr.(type) { case *dns.A: @@ -616,22 +619,22 @@ func extractRecordValue(rr dns.RR) string { case *dns.AAAA: return r.AAAA.String() case *dns.CNAME: - return r.Target + return strings.ToLower(r.Target) case *dns.MX: - return fmt.Sprintf("%d %s", r.Preference, r.Mx) + return fmt.Sprintf("%d %s", r.Preference, strings.ToLower(r.Mx)) case *dns.TXT: return strings.Join(r.Txt, "") case *dns.SRV: return fmt.Sprintf( "%d %d %d %s", - r.Priority, r.Weight, r.Port, r.Target, + r.Priority, r.Weight, r.Port, strings.ToLower(r.Target), ) case *dns.CAA: return fmt.Sprintf( "%d %s \"%s\"", r.Flag, r.Tag, r.Value, ) case *dns.NS: - return r.Ns + return strings.ToLower(r.Ns) default: return "" } diff --git a/internal/resolver/iterative_test.go b/internal/resolver/iterative_test.go new file mode 100644 index 0000000..781e90b --- /dev/null +++ b/internal/resolver/iterative_test.go @@ -0,0 +1,62 @@ +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)) + }) + } +} -- 2.54.0