From 009882298135672522e0fa9dac1b9fe32a71581a Mon Sep 17 00:00:00 2001
From: Nick Khyl <nickk@tailscale.com>
Date: Thu, 3 Jul 2025 11:51:27 -0500
Subject: [PATCH] ipn/ipnlocal: update suggestExitNode to skip offline
 candidates and fix TestSetControlClientStatusAutoExitNode

TestSetControlClientStatusAutoExitNode is broken similarly to TestUpdateNetmapDeltaAutoExitNode
as suggestExitNode didn't previously check the online status of exit nodes, and similarly to the other test
it succeeded because the test itself is also broken.

However, it is easier to fix as it sends out a full netmap update rather than a delta peer update,
so it doesn't depend on the same refactoring as TestSetControlClientStatusAutoExitNode.

Updates #16455
Updates tailscale/corp#29969

Signed-off-by: Nick Khyl <nickk@tailscale.com>
---
 ipn/ipnlocal/local.go      |  2 +-
 ipn/ipnlocal/local_test.go | 24 ++++++++++++++----------
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go
index adc0af5cd..8889fa90b 100644
--- a/ipn/ipnlocal/local.go
+++ b/ipn/ipnlocal/local.go
@@ -7433,7 +7433,7 @@ func suggestExitNode(report *netcheck.Report, netMap *netmap.NetworkMap, prevSug
 	}
 	candidates := make([]tailcfg.NodeView, 0, len(netMap.Peers))
 	for _, peer := range netMap.Peers {
-		if !peer.Valid() {
+		if !peer.Valid() || !peer.Online().Get() {
 			continue
 		}
 		if allowList != nil && !allowList.Contains(peer.StableID()) {
diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go
index 06acd85ce..ca968ccd7 100644
--- a/ipn/ipnlocal/local_test.go
+++ b/ipn/ipnlocal/local_test.go
@@ -2166,8 +2166,8 @@ func TestAutoExitNodeSetNetInfoCallback(t *testing.T) {
 }
 
 func TestSetControlClientStatusAutoExitNode(t *testing.T) {
-	peer1 := makePeer(1, withCap(26), withSuggest(), withExitRoutes(), withNodeKey())
-	peer2 := makePeer(2, withCap(26), withSuggest(), withExitRoutes(), withNodeKey())
+	peer1 := makePeer(1, withCap(26), withSuggest(), withExitRoutes(), withOnline(true), withNodeKey())
+	peer2 := makePeer(2, withCap(26), withSuggest(), withExitRoutes(), withOnline(true), withNodeKey())
 	derpMap := &tailcfg.DERPMap{
 		Regions: map[int]*tailcfg.DERPRegion{
 			1: {
@@ -2210,22 +2210,25 @@ func TestSetControlClientStatusAutoExitNode(t *testing.T) {
 	))
 	syspolicy.MustRegisterStoreForTest(t, "TestStore", setting.DeviceScope, policyStore)
 	b.currentNode().SetNetMap(nm)
-	b.lastSuggestedExitNode = peer1.StableID()
+	// Peer 2 should be the initial exit node, as it's better than peer 1
+	// in terms of latency and DERP region.
+	b.lastSuggestedExitNode = peer2.StableID()
 	b.sys.MagicSock.Get().SetLastNetcheckReportForTest(b.ctx, report)
 	b.SetPrefsForTest(b.pm.CurrentPrefs().AsStruct())
-	firstExitNode := b.Prefs().ExitNodeID()
-	newPeer1 := makePeer(1, withCap(26), withSuggest(), withExitRoutes(), withOnline(false), withNodeKey())
+	offlinePeer2 := makePeer(2, withCap(26), withSuggest(), withExitRoutes(), withOnline(false), withNodeKey())
 	updatedNetmap := &netmap.NetworkMap{
 		Peers: []tailcfg.NodeView{
-			newPeer1,
-			peer2,
+			peer1,
+			offlinePeer2,
 		},
 		DERPMap: derpMap,
 	}
 	b.SetControlClientStatus(b.cc, controlclient.Status{NetMap: updatedNetmap})
-	lastExitNode := b.Prefs().ExitNodeID()
-	if firstExitNode == lastExitNode {
-		t.Errorf("did not switch exit nodes despite auto exit node going offline")
+	// But now that peer 2 is offline, we should switch to peer 1.
+	wantExitNode := peer1.StableID()
+	gotExitNode := b.Prefs().ExitNodeID()
+	if gotExitNode != wantExitNode {
+		t.Errorf("did not switch exit nodes despite auto exit node going offline: got %q; want %q", gotExitNode, wantExitNode)
 	}
 }
 
@@ -3289,6 +3292,7 @@ func makePeer(id tailcfg.NodeID, opts ...peerOptFunc) tailcfg.NodeView {
 		Key:      makeNodeKeyFromID(id),
 		StableID: tailcfg.StableNodeID(fmt.Sprintf("stable%d", id)),
 		Name:     fmt.Sprintf("peer%d", id),
+		Online:   ptr.To(true),
 		HomeDERP: int(id),
 	}
 	for _, opt := range opts {