From 6ecc25b26a8edf191cfbebe2f16254468b1f1695 Mon Sep 17 00:00:00 2001 From: Nick Khyl Date: Thu, 3 Jul 2025 11:50:27 -0500 Subject: [PATCH] ipn/ipnlocal: skip TestUpdateNetmapDeltaAutoExitNode suggestExitNode never checks whether an exit node candidate is online. It also accepts a full netmap, which doesn't include changes from delta updates. The test can't work correctly until both issues are fixed. Previously, it passed only because the test itself is flawed. It doesn't succeed because the currently selected node goes offline and a new one is chosen. Instead, it succeeds because lastSuggestedExitNode is incorrect, and suggestExitNode picks the correct node the first time it runs, based on the DERP map and the netcheck report. The node in exitNodeIDWant just happens to be the optimal choice. Fixing SuggestExitNode requires refactoring its callers first, which in turn reveals the flawed test, as suggestExitNode ends up being called slightly earlier. In this PR, we update the test to correctly fail due to existing bugs in SuggestExitNode, and temporarily skip it until those issues are addressed in a future commit. Updates #16455 Updates tailscale/corp#29969 Signed-off-by: Nick Khyl --- ipn/ipnlocal/local_test.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 47e5fa37d..06acd85ce 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -1918,8 +1918,10 @@ func TestSetExitNodeIDPolicy(t *testing.T) { } func TestUpdateNetmapDeltaAutoExitNode(t *testing.T) { - peer1 := makePeer(1, withCap(26), withSuggest(), withExitRoutes()) - peer2 := makePeer(2, withCap(26), withSuggest(), withExitRoutes()) + t.Skip("TODO(tailscale/tailscale#16455): suggestExitNode does not check for online status of exit nodes") + + peer1 := makePeer(1, withCap(26), withSuggest(), withOnline(true), withExitRoutes()) + peer2 := makePeer(2, withCap(26), withSuggest(), withOnline(true), withExitRoutes()) derpMap := &tailcfg.DERPMap{ Regions: map[int]*tailcfg.DERPRegion{ 1: { @@ -1958,8 +1960,10 @@ func TestUpdateNetmapDeltaAutoExitNode(t *testing.T) { }{ { // selected auto exit node goes offline - name: "exit-node-goes-offline", - lastSuggestedExitNode: peer1.StableID(), + name: "exit-node-goes-offline", + // PreferredDERP is 2, and it's also the region with the lowest latency. + // So, peer2 should be selected as the exit node. + lastSuggestedExitNode: peer2.StableID(), netmap: &netmap.NetworkMap{ Peers: []tailcfg.NodeView{ peer1, @@ -1970,14 +1974,14 @@ func TestUpdateNetmapDeltaAutoExitNode(t *testing.T) { muts: []*tailcfg.PeerChange{ { NodeID: 1, - Online: ptr.To(false), + Online: ptr.To(true), }, { NodeID: 2, - Online: ptr.To(true), + Online: ptr.To(false), // the selected exit node goes offline }, }, - exitNodeIDWant: peer2.StableID(), + exitNodeIDWant: peer1.StableID(), report: report, }, { @@ -1994,7 +1998,7 @@ func TestUpdateNetmapDeltaAutoExitNode(t *testing.T) { muts: []*tailcfg.PeerChange{ { NodeID: 1, - Online: ptr.To(false), + Online: ptr.To(false), // a different exit node goes offline }, { NodeID: 2,