half broken

internal/engine/engine.go

@@ -13,6 +13,7 @@ package engine
 
 import (
 	"context"
+	"encoding/hex"
 	"net/netip"
 	"strconv"
 	"strings"
@@ -32,6 +33,12 @@ import (
 	"git.k8s-git-k6n.net/mats/fancy-dns/internal/resolver"
 )
 
+// upstreamNSID is the nameserver identifier we advertise to upstreams when a
+// client asks for one. It is hex-encoded on the way into the EDNS0 option:
+// dns.EDNS0_NSID.Nsid is a hex string in this miekg/dns version, not the
+// opaque bytes RFC 5001 describes.
+const upstreamNSID = "fancy-dns"
+
 // Request describes one inbound query.
 type Request struct {
 	// Msg is the parsed query. The engine may modify it (to attach an OPT or
@@ -696,10 +703,14 @@ func (e *Engine) prepareUpstream(q *dns.Msg, req Request) *dns.Msg {
 		if opt := o.Option; opt != nil {
 			for _, oo := range opt {
 				if oo.Option() == dns.EDNS0NSID {
-					// Our own NSID, not the client's.
+					// Our own NSID, not the client's — but only when the
+					// operator has not asked us to hide. dns.EDNS0_NSID.Nsid
+					// is hex on the wire (the library hex-decodes it in pack
+					// and hex-encodes it in unpack), so a plain string here
+					// does not pack and every NSID query gets SERVFAIL.
 					if e.cfgStore == nil || !e.cfgStore.Get().DNS.HideIdentity {
 						nsid := new(dns.EDNS0_NSID)
-						nsid.Nsid = "fancy-dns"
+						nsid.Nsid = hex.EncodeToString([]byte(upstreamNSID))
 						if m.IsEdns0() == nil {
 							m.SetEdns0(maxSize, o.Do())
 						}

internal/engine/engine_test.go

@@ -718,11 +718,10 @@ func TestForwardedQueryShape(t *testing.T) {
 		}
 	})
 
-	// The NSID branch of prepareUpstream is deliberately not pinned here:
-	// miekg/dns v1.1.73 reports an EDNS0_NSID option as zero length, so any
-	// message carrying one fails to pack ("dns: overflow packing opt") and a
-	// client that asks for NSID gets SERVFAIL. That is a library defect, not
-	// a decision this package owns; see STATUS.md.
+	// The NSID branch of prepareUpstream is covered in pins_test.go: the
+	// option is hex on the wire (dns.EDNS0_NSID.Nsid is a hex string), a
+	// plain string there does not pack, and a client that asks for NSID
+	// used to get SERVFAIL for exactly that reason.
 }
 
 func findOption(m *dns.Msg, code uint16) dns.EDNS0 {

internal/engine/harness_test.go

@@ -241,6 +241,7 @@ type hopt struct {
 	blockCfg func(*config.Block)
 	server   func(*config.Server)
 	cacheCfg func(*config.Cache)
+	dnsCfg   func(*config.DNS)
 	// rules are compiled into the blocklist set the engine holds.
 	rules []blocklist.Rule
 	// localZone is a master file; empty means no local zone at all.
@@ -302,6 +303,9 @@ func newHarness(t *testing.T, o hopt) *harness {
 	if o.cacheCfg != nil {
 		o.cacheCfg(&cfg.Cache)
 	}
+	if o.dnsCfg != nil {
+		o.dnsCfg(&cfg.DNS)
+	}
 
 	// The default pool is the fake upstream; a dead address is what the
 	// upstream-failure test needs, and noPool needs no pool at all.

internal/engine/pins_test.go

@@ -0,0 +1,170 @@
+package engine
+
+import (
+	"encoding/hex"
+	"net/http"
+	"net/http/httptest"
+	"strconv"
+	"strings"
+	"testing"
+
+	"git.k8s-git-k6n.net/mats/fancy-dns/internal/config"
+	"git.k8s-git-k6n.net/mats/fancy-dns/internal/metrics"
+	"git.k8s-git-k6n.net/mats/fancy-dns/internal/qlog"
+
+	"github.com/miekg/dns"
+)
+
+// The tests in this file PIN behaviour we know is wrong or known to be
+// incomplete. They are not assertions of intent — they are tripwires, so that
+// when the behaviour changes (a library bump, a metrics change) the suite says
+// so out loud instead of the drift reaching the dashboards or the clients.
+// Each one names in its failure message what to do when it goes off.
+
+// outcomeCount sums the _count of fancydns_query_duration_seconds over every
+// transport for one outcome label. Zero is a legitimate answer: a histogram
+// with no observations publishes no series at all.
+func outcomeCount(t *testing.T, outcome string) uint64 {
+	t.Helper()
+	rec := httptest.NewRecorder()
+	metrics.Handler().ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/metrics", nil))
+	prefix := metrics.Namespace + "_query_duration_seconds_count{"
+	want := `outcome="` + outcome + `"`
+	var total uint64
+	for _, line := range strings.Split(rec.Body.String(), "\n") {
+		if !strings.HasPrefix(line, prefix) || !strings.Contains(line, want) {
+			continue
+		}
+		fields := strings.Fields(line)
+		n, err := strconv.ParseUint(fields[len(fields)-1], 10, 64)
+		if err != nil {
+			t.Fatalf("unparseable exposition line %q: %v", line, err)
+		}
+		total += n
+	}
+	return total
+}
+
+// TestPinnedLocalAnswersAreCountedAsUpstream pins the gap STATUS.md calls
+// "metrics.OutcomeLocal has no producer": a query answered from the local
+// zone reports qlog.ActionAllow, and outcomeFor maps ActionAllow to
+// OutcomeUpstream, so every locally answered query is counted as an upstream
+// one. The outcome="local" series is therefore never published and the
+// dashboard's local column is permanently empty.
+//
+// If this test fails, something started producing OutcomeLocal (or the
+// mapping changed): that moves published counters, so it is a deliberate
+// change — update the Grafana panels and the ServiceMonitor queries, and
+// remove this pin. Do not "fix" it silently.
+func TestPinnedLocalAnswersAreCountedAsUpstream(t *testing.T) {
+	h := newHarness(t, hopt{localZone: testZone})
+
+	before := outcomeCount(t, metrics.OutcomeUpstream)
+	res := h.ask("host.local.", dns.TypeA)
+	requireAction(t, res, qlog.ActionAllow)
+	if got := answerA(res.Msg); got != "192.168.1.5" {
+		t.Fatalf("answer = %q, want the local zone's 192.168.1.5", got)
+	}
+	requireNoUpstreamHit(t, h, "host.local.")
+
+	if got := outcomeCount(t, metrics.OutcomeUpstream) - before; got != 1 {
+		t.Fatalf("a locally answered query moved the upstream outcome by %d, want 1", got)
+	}
+	if got := outcomeCount(t, metrics.OutcomeLocal); got != 0 {
+		t.Fatalf("metrics.OutcomeLocal now has %d observations; it is no longer "+
+			"dead. Publishing published counters is a decision: update the "+
+			"dashboards and STATUS.md, then delete this pin.", got)
+	}
+}
+
+// TestPinnedEDNS0NSIDIsHex pins a contract of the pinned miekg/dns version
+// (v1.1.73) that is easy to get wrong and fails silently: EDNS0_NSID.Nsid is
+// a HEX STRING, not the opaque bytes RFC 5001 describes. pack() hex-decodes
+// it and unpack() hex-encodes it, so:
+//
+//   - a hex string packs, and the bytes on the wire are the decoded value;
+//   - a plain string such as "fancy-dns" does NOT pack — hex.DecodeString
+//     rejects the '-' — and the caller gets an error instead of a message.
+//
+// This is the trap behind the SERVFAIL bug: the engine used to put the literal
+// "fancy-dns" in the option, so every query from a client that asked for NSID
+// failed to build. If a future miekg/dns changes Nsid to raw bytes, this pin
+// fails and the hex.EncodeToString in prepareUpstream has to go with it.
+func TestPinnedEDNS0NSIDIsHex(t *testing.T) {
+	pack := func(nsid string) error {
+		m := new(dns.Msg)
+		m.SetQuestion("nsid.example.test.", dns.TypeA)
+		m.SetEdns0(1232, false)
+		m.IsEdns0().Option = append(m.IsEdns0().Option, &dns.EDNS0_NSID{Nsid: nsid})
+		_, err := m.Pack()
+		return err
+	}
+
+	if err := pack(hex.EncodeToString([]byte(upstreamNSID))); err != nil {
+		t.Fatalf("a hex-encoded NSID must pack: %v", err)
+	}
+	if err := pack(upstreamNSID); err == nil {
+		t.Fatalf("%q packed as an NSID; dns.EDNS0_NSID.Nsid is no longer hex-encoded, "+
+			"so drop the hex.EncodeToString in prepareUpstream and update this pin", upstreamNSID)
+	}
+}
+
+// TestClientNSIDIsAnswered pins the user-visible half of that fix: a client
+// that asks for NSID gets a real answer, whatever the size of its NSID, and
+// the option we forward upstream is our own identifier rather than the
+// client's. With dns.hide_identity the option is dropped entirely, which is
+// the whole point of the setting.
+func TestClientNSIDIsAnswered(t *testing.T) {
+	t.Run("short NSID", func(t *testing.T) {
+		h := newHarness(t, hopt{})
+		req := h.request("nsid.example.test.", dns.TypeA)
+		req.Msg.SetEdns0(1232, false)
+		req.Msg.IsEdns0().Option = append(req.Msg.IsEdns0().Option,
+			&dns.EDNS0_NSID{Nsid: "abc"})
+		res := h.askWith(req)
+		requireRcode(t, res, dns.RcodeSuccess)
+
+		got := h.up.received()
+		if len(got) != 1 {
+			t.Fatalf("upstream saw %d queries, want 1", len(got))
+		}
+		opt, ok := findOption(got[0], dns.EDNS0NSID).(*dns.EDNS0_NSID)
+		if !ok {
+			t.Fatal("the forwarded query carries no NSID; we do not advertise ourselves upstream")
+		}
+		raw, err := hex.DecodeString(opt.Nsid)
+		if err != nil {
+			t.Fatalf("forwarded NSID %q is not hex: %v", opt.Nsid, err)
+		}
+		if string(raw) != upstreamNSID {
+			t.Fatalf("forwarded NSID = %q, want our own %q (we forwarded the client's)",
+				raw, upstreamNSID)
+		}
+	})
+
+	t.Run("NSID larger than 255 bytes", func(t *testing.T) {
+		// A character-string over 255 bytes does not pack for TXT, but NSID
+		// is hex-decoded into a byte slice, so a long NSID is fine here.
+		// Pinning it keeps the distinction between the two documented.
+		h := newHarness(t, hopt{})
+		req := h.request("big-nsid.example.test.", dns.TypeA)
+		req.Msg.SetEdns0(1232, false)
+		req.Msg.IsEdns0().Option = append(req.Msg.IsEdns0().Option,
+			&dns.EDNS0_NSID{Nsid: hex.EncodeToString([]byte(strings.Repeat("a", 300)))})
+		requireRcode(t, h.askWith(req), dns.RcodeSuccess)
+	})
+
+	t.Run("hide_identity drops the option", func(t *testing.T) {
+		h := newHarness(t, hopt{
+			dnsCfg: func(d *config.DNS) { d.HideIdentity = true },
+		})
+		req := h.request("hidden-nsid.example.test.", dns.TypeA)
+		req.Msg.SetEdns0(1232, false)
+		req.Msg.IsEdns0().Option = append(req.Msg.IsEdns0().Option,
+			&dns.EDNS0_NSID{Nsid: "abc"})
+		requireRcode(t, h.askWith(req), dns.RcodeSuccess)
+		if opt := findOption(h.up.received()[0], dns.EDNS0NSID); opt != nil {
+			t.Fatal("dns.hide_identity is set but the query still advertises an NSID")
+		}
+	})
+}