cmd/derper: fix TestLookupMetric to pass when run alone

TestLookupMetric was added in e8d140654 (2023-08-17) without
initializing the dnsCache and dnsCacheBytes globals. When run in
isolation, handleBootstrapDNS writes a nil body (from the
uninitialized dnsCacheBytes), causing getBootstrapDNS to fail
decoding an empty response with EOF.

Add a setDNSCache test helper that stores the dnsEntryMap, marshals
dnsCacheBytes, and registers a t.Cleanup to nil both out, so tests
that forget to call it will hit the dnsCache-nil fatal in
getBootstrapDNS rather than silently depending on prior test state.

Also add AssertNotParallel and a dnsCache-nil fatal check to
getBootstrapDNS, the central helper all bootstrap DNS tests flow
through, to prevent future tests from running in parallel (they
all mutate package-level DNS caches and metrics) and to give a
clear error if a test forgets to initialize the DNS caches.

Fixes #19388

Change-Id: I8ad454ec6026c71f13ecfa14d25925df5478b908
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
Co-authored-by: Avery Pennarun <apenwarr@tailscale.com>

1 files changed, 28 insertions, 6 deletions

diff --git a/cmd/derper/bootstrap_dns_test.go b/cmd/derper/bootstrap_dns_test.go
index 5b765f6d3..2055b9751 100644
--- a/cmd/derper/bootstrap_dns_test.go
+++ b/cmd/derper/bootstrap_dns_test.go
@@ -41,8 +41,28 @@ func (b *bitbucketResponseWriter) Write(p []byte) (int, error) { return len(p),
 
 func (b *bitbucketResponseWriter) WriteHeader(statusCode int) {}
 
+// setDNSCache sets the published DNS cache for tests.
+func setDNSCache(tb testing.TB, m *dnsEntryMap) {
+	tb.Helper()
+	j, err := json.Marshal(m.IPs)
+	if err != nil {
+		tb.Fatal(err)
+	}
+	tstest.AssertNotParallel(tb)
+	dnsCache.Store(m)
+	dnsCacheBytes.Store(j)
+	tb.Cleanup(func() {
+		dnsCache.Store(nil)
+		dnsCacheBytes.Store(nil)
+	})
+}
+
 func getBootstrapDNS(t *testing.T, q string) map[string][]net.IP {
 	t.Helper()
+	tstest.AssertNotParallel(t)
+	if dnsCache.Load() == nil {
+		t.Fatal("dnsCache not initialized; call setDNSCache before getBootstrapDNS")
+	}
 	req, _ := http.NewRequest("GET", "https://localhost/bootstrap-dns?q="+url.QueryEscape(q), nil)
 	w := httptest.NewRecorder()
 	handleBootstrapDNS(w, req)
@@ -100,7 +120,8 @@ func TestUnpublishedDNS(t *testing.T) {
 	}
 }
 
-func resetMetrics() {
+func resetMetrics(tb testing.TB) {
+	tstest.AssertNotParallel(tb)
 	publishedDNSHits.Set(0)
 	publishedDNSMisses.Set(0)
 	unpublishedDNSHits.Set(0)
@@ -114,8 +135,7 @@ func TestUnpublishedDNSEmptyList(t *testing.T) {
 	pub := &dnsEntryMap{
 		IPs: map[string][]net.IP{"tailscale.com": {net.IPv4(10, 10, 10, 10)}},
 	}
-	dnsCache.Store(pub)
-	dnsCacheBytes.Store([]byte(`{"tailscale.com":["10.10.10.10"]}`))
+	setDNSCache(t, pub)
 
 	unpublishedDNSCache.Store(&dnsEntryMap{
 		IPs: map[string][]net.IP{
@@ -131,7 +151,7 @@ func TestUnpublishedDNSEmptyList(t *testing.T) {
 	t.Run("CacheMiss", func(t *testing.T) {
 		// One domain in map but empty, one not in map at all
 		for _, q := range []string{"log.tailscale.com", "login.tailscale.com"} {
-			resetMetrics()
+			resetMetrics(t)
 			ips := getBootstrapDNS(t, q)
 
 			// Expected our public map to be returned on a cache miss
@@ -149,7 +169,7 @@ func TestUnpublishedDNSEmptyList(t *testing.T) {
 
 	// Verify that we do get a valid response and metric.
 	t.Run("CacheHit", func(t *testing.T) {
-		resetMetrics()
+		resetMetrics(t)
 		ips := getBootstrapDNS(t, "controlplane.tailscale.com")
 		want := map[string][]net.IP{"controlplane.tailscale.com": {net.IPv4(1, 2, 3, 4)}}
 		if !reflect.DeepEqual(ips, want) {
@@ -166,8 +186,10 @@ func TestUnpublishedDNSEmptyList(t *testing.T) {
 }
 
 func TestLookupMetric(t *testing.T) {
+	setDNSCache(t, &dnsEntryMap{})
+
 	d := []string{"a.io", "b.io", "c.io", "d.io", "e.io", "e.io", "e.io", "a.io"}
-	resetMetrics()
+	resetMetrics(t)
 	for _, q := range d {
 		_ = getBootstrapDNS(t, q)
 	}