diff --git a/hscontrol/servertest/via_compat_test.go b/hscontrol/servertest/via_compat_test.go index df698e8cc..694d1f1eb 100644 --- a/hscontrol/servertest/via_compat_test.go +++ b/hscontrol/servertest/via_compat_test.go @@ -240,6 +240,9 @@ func runViaMapCompat(t *testing.T, c *testcapture.Capture) { saasAddrs := saasAddrsByNode(c, clients) // Compare each viewer's [tailcfg.MapResponse] against the golden [netmap.NetworkMap]. + // Route approvals reach clients asynchronously, so the peer-count wait + // above says nothing about routes, primaries or filters: retry the + // whole comparison until the viewer's netmap has caught up. for viewerName, cl := range clients { capture := c.Captures[viewerName] if capture.Netmap == nil { @@ -247,17 +250,69 @@ func runViaMapCompat(t *testing.T, c *testcapture.Capture) { } t.Run(viewerName, func(t *testing.T) { - nm := cl.Netmap() - require.NotNil(t, nm, "netmap is nil") + requireNetmapHolds(t, func(tt require.TestingT) { + nm := cl.Netmap() + require.NotNil(tt, nm, "netmap is nil") - require.Equal(t, selfOffersExit(capture.Netmap), selfOffersExit(nm), - "SelfNode exit routes should match SaaS") + require.Equal(tt, selfOffersExit(capture.Netmap), selfOffersExit(nm), + "SelfNode exit routes should match SaaS") - compareNetmap(t, nm, capture, clients, saasAddrs) + compareNetmap(tt, nm, capture, clients, saasAddrs) + }) }) } } +// requireNetmapHolds retries check until it passes on several consecutive +// ticks. One passing tick is not enough: a netmap that matches and then +// drifts, such as a primary flipping away, is the bug these oracles guard. +func requireNetmapHolds(t *testing.T, check func(require.TestingT)) { + t.Helper() + + // Long enough for a batch still in flight to land. + const holdTicks = 5 + + held := 0 + + require.EventuallyWithT(t, func(collect *assert.CollectT) { + tick := &failRecorder{CollectT: collect} + + // Deferred because FailNow exits the goroutine. + defer func() { + if tick.failed { + held = 0 + + return + } + + held++ + if held < holdTicks { + collect.Errorf("netmap held for %d of %d ticks", held, holdTicks) + } + }() + + check(tick) + }, 30*time.Second, 100*time.Millisecond) +} + +// failRecorder notes whether a tick failed, which [assert.CollectT] does +// not expose. +type failRecorder struct { + *assert.CollectT + + failed bool +} + +func (r *failRecorder) Errorf(format string, args ...any) { + r.failed = true + r.CollectT.Errorf(format, args...) +} + +func (r *failRecorder) FailNow() { + r.failed = true + r.CollectT.FailNow() +} + // compareNetmap compares the headscale [tailcfg.MapResponse] against the // captured [netmap.NetworkMap] data in an IP-independent way. It validates: // - Peer visibility (which peers are present, by hostname) @@ -265,14 +320,18 @@ func runViaMapCompat(t *testing.T, c *testcapture.Capture) { // - Number of Tailscale IPs per peer (should be 2: one v4 + one v6) // - PrimaryRoutes per peer // - PacketFilter (source, destination, ports) triples +// +// It takes [require.TestingT] so it can run inside [requireNetmapHolds]. func compareNetmap( - t *testing.T, + t require.TestingT, got *netmap.NetworkMap, want testcapture.Node, clients map[string]*servertest.TestClient, saasAddrs map[netip.Addr]string, ) { - t.Helper() + if h, ok := t.(interface{ Helper() }); ok { + h.Helper() + } require.NotNil(t, want.Netmap, "golden Netmap is nil") @@ -412,13 +471,15 @@ func compareNetmap( // Tailscale IPs differ between SaaS and headscale allocation, so both // sides are re-keyed by peer identity. func comparePacketFilter( - t *testing.T, + t require.TestingT, got *netmap.NetworkMap, want testcapture.Node, clients map[string]*servertest.TestClient, saasAddrs map[netip.Addr]string, ) { - t.Helper() + if h, ok := t.(interface{ Helper() }); ok { + h.Helper() + } hsAddrs := hsAddrsByPeer(clients) @@ -486,12 +547,14 @@ func capTriple(srcs, dst string, c tailcfg.PeerCapability) string { // peerIdents resolves a prefix into sorted canonical identity tokens, // see [addIdentsForSrc]. func peerIdents( - t *testing.T, + t require.TestingT, pfx netip.Prefix, addrToPeer map[netip.Addr]string, ruleIndex int, ) []string { - t.Helper() + if h, ok := t.(interface{ Helper() }); ok { + h.Helper() + } seen := map[string]struct{}{} addIdentsForSrc(t, pfx, addrToPeer, ruleIndex, seen) @@ -561,12 +624,14 @@ func hsAddrsByPeer(clients map[string]*servertest.TestClient) map[netip.Addr]str // or /128 (IPv6) expands to the union of its contained peers. // Unresolvable Tailscale-range sources fail the test. func canonicaliseSrcStrings( - t *testing.T, + t require.TestingT, srcs []string, addrToPeer map[netip.Addr]string, ruleIndex int, ) []string { - t.Helper() + if h, ok := t.(interface{ Helper() }); ok { + h.Helper() + } seen := map[string]struct{}{} @@ -591,12 +656,14 @@ func canonicaliseSrcStrings( // [canonicaliseSrcStrings], reading already-parsed [netip.Prefix] values // from [tailcfg.Match.Srcs]. func canonicaliseSrcPrefixes( - t *testing.T, + t require.TestingT, srcs []netip.Prefix, addrToPeer map[netip.Addr]string, ruleIndex int, ) []string { - t.Helper() + if h, ok := t.(interface{ Helper() }); ok { + h.Helper() + } seen := map[string]struct{}{} @@ -618,13 +685,15 @@ func canonicaliseSrcPrefixes( // through literally; a Tailscale-range prefix expands to the union // of peer names whose addresses fall within it. func addIdentsForSrc( - t *testing.T, + t require.TestingT, pfx netip.Prefix, addrToPeer map[netip.Addr]string, ruleIndex int, seen map[string]struct{}, ) { - t.Helper() + if h, ok := t.(interface{ Helper() }); ok { + h.Helper() + } if !prefixInTailscaleRange(pfx) { seen[pfx.String()] = struct{}{} diff --git a/hscontrol/servertest/via_ha_compat_test.go b/hscontrol/servertest/via_ha_compat_test.go index b9e95ec74..357335564 100644 --- a/hscontrol/servertest/via_ha_compat_test.go +++ b/hscontrol/servertest/via_ha_compat_test.go @@ -186,6 +186,9 @@ func runViaHACompat(t *testing.T, c *testcapture.Capture) { } // Compare each viewer's [tailcfg.MapResponse] against golden [netmap.NetworkMap]. + // Route approvals reach clients asynchronously, so the peer-count wait + // above says nothing about routes, primaries or filters: retry the + // whole comparison until the viewer's netmap has caught up. for viewerName, cl := range clients { capture := c.Captures[viewerName] if capture.Netmap == nil { @@ -193,7 +196,9 @@ func runViaHACompat(t *testing.T, c *testcapture.Capture) { } t.Run(viewerName, func(t *testing.T) { - compareCaptureNetmap(t, cl, capture, clients) + requireNetmapHolds(t, func(tt require.TestingT) { + compareCaptureNetmap(tt, cl, capture, clients) + }) }) } } @@ -201,13 +206,16 @@ func runViaHACompat(t *testing.T, c *testcapture.Capture) { // compareCaptureNetmap compares headscale's [tailcfg.MapResponse] against a // [testcapture.Node]'s [netmap.NetworkMap] data. Same logic as [compareNetmap] but // reads from typed [testcapture] fields instead of goldenFile strings. +// It takes [require.TestingT] so it can run inside [requireNetmapHolds]. func compareCaptureNetmap( - t *testing.T, + t require.TestingT, viewer *servertest.TestClient, want testcapture.Node, clients map[string]*servertest.TestClient, ) { - t.Helper() + if h, ok := t.(interface{ Helper() }); ok { + h.Helper() + } nm := viewer.Netmap() require.NotNil(t, nm, "viewer has no netmap")