ipn/ipnlocal: check for offline auto exit node in SetControlClientStatus (#12772)

Updates tailscale/corp#19681

Signed-off-by: Claire Wang <claire@tailscale.com>

ipn/ipnlocal/local.go

@@ -1191,7 +1191,13 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control
 
 	prefsChanged := false
 	prefs := b.pm.CurrentPrefs().AsStruct()
-	netMap := b.netMap
+	oldNetMap := b.netMap
+	curNetMap := st.NetMap
+	if curNetMap == nil {
+		// The status didn't include a netmap update, so the old one is still
+		// current.
+		curNetMap = oldNetMap
+	}
 
 	if prefs.ControlURL == "" {
 		// Once we get a message from the control plane, set
@@ -1222,7 +1228,14 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control
 		prefs.WantRunning = true
 		prefs.LoggedOut = false
 	}
-	if setExitNodeID(prefs, st.NetMap, b.lastSuggestedExitNode) {
+	if shouldAutoExitNode() {
+		// Re-evaluate exit node suggestion in case circumstances have changed.
+		_, err := b.suggestExitNodeLocked(curNetMap)
+		if err != nil && !errors.Is(err, ErrNoPreferredDERP) {
+			b.logf("SetControlClientStatus failed to select auto exit node: %v", err)
+		}
+	}
+	if setExitNodeID(prefs, curNetMap, b.lastSuggestedExitNode) {
 		prefsChanged = true
 	}
 	if applySysPolicy(prefs) {
@@ -1239,8 +1252,8 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control
 	if prefsChanged {
 		// Prefs will be written out if stale; this is not safe unless locked or cloned.
 		if err := b.pm.SetPrefs(prefs.View(), ipn.NetworkProfile{
-			MagicDNSName: st.NetMap.MagicDNSSuffix(),
+			MagicDNSName: curNetMap.MagicDNSSuffix(),
-			DomainName:   st.NetMap.DomainName(),
+			DomainName:   curNetMap.DomainName(),
 		}); err != nil {
 			b.logf("Failed to save new controlclient state: %v", err)
 		}
@@ -1307,8 +1320,8 @@ func (b *LocalBackend) SetControlClientStatus(c controlclient.Client, st control
 			b.send(ipn.Notify{ErrMessage: &msg, Prefs: &p})
 			return
 		}
-		if netMap != nil {
+		if oldNetMap != nil {
-			diff := st.NetMap.ConciseDiffFrom(netMap)
+			diff := st.NetMap.ConciseDiffFrom(oldNetMap)
 			if strings.TrimSpace(diff) == "" {
 				b.logf("[v1] netmap diff: (none)")
 			} else {
@@ -4891,7 +4904,7 @@ func (b *LocalBackend) setAutoExitNodeIDLockedOnEntry(unlock unlockOnce) {
 		return
 	}
 	prefsClone := prefs.AsStruct()
-	newSuggestion, err := b.suggestExitNodeLocked()
+	newSuggestion, err := b.suggestExitNodeLocked(nil)
 	if err != nil {
 		b.logf("setAutoExitNodeID: %v", err)
 		return
@@ -6573,7 +6586,6 @@ func mayDeref[T any](p *T) (v T) {
 }
 
 var ErrNoPreferredDERP = errors.New("no preferred DERP, try again later")
-var ErrCannotSuggestExitNode = errors.New("unable to suggest an exit node, try again later")
 
 // suggestExitNodeLocked computes a suggestion based on the current netmap and last netcheck report. If
 // there are multiple equally good options, one is selected at random, so the result is not stable. To be
@@ -6582,10 +6594,17 @@ var ErrCannotSuggestExitNode = errors.New("unable to suggest an exit node, try a
 // Currently, peers with a DERP home are preferred over those without (typically this means Mullvad).
 // Peers are selected based on having a DERP home that is the lowest latency to this device. For peers
 // without a DERP home, we look for geographic proximity to this device's DERP home.
+//
+// netMap is an optional netmap to use that overrides b.netMap (needed for SetControlClientStatus before b.netMap is updated).
+// If netMap is nil, then b.netMap is used.
+//
 // b.mu.lock() must be held.
-func (b *LocalBackend) suggestExitNodeLocked() (response apitype.ExitNodeSuggestionResponse, err error) {
+func (b *LocalBackend) suggestExitNodeLocked(netMap *netmap.NetworkMap) (response apitype.ExitNodeSuggestionResponse, err error) {
+	// netMap is an optional netmap to use that overrides b.netMap (needed for SetControlClientStatus before b.netMap is updated). If netMap is nil, then b.netMap is used.
+	if netMap == nil {
+		netMap = b.netMap
+	}
 	lastReport := b.MagicConn().GetLastNetcheckReport(b.ctx)
-	netMap := b.netMap
 	prevSuggestion := b.lastSuggestedExitNode
 
 	res, err := suggestExitNode(lastReport, netMap, prevSuggestion, randomRegion, randomNode, getAllowedSuggestions())
@@ -6599,7 +6618,7 @@ func (b *LocalBackend) suggestExitNodeLocked() (response apitype.ExitNodeSuggest
 func (b *LocalBackend) SuggestExitNode() (response apitype.ExitNodeSuggestionResponse, err error) {
 	b.mu.Lock()
 	defer b.mu.Unlock()
-	return b.suggestExitNodeLocked()
+	return b.suggestExitNodeLocked(nil)
 }
 
 // selectRegionFunc returns a DERP region from the slice of candidate regions.

ipn/ipnlocal/local_test.go

@@ -2119,6 +2119,72 @@ 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())
+	derpMap := &tailcfg.DERPMap{
+		Regions: map[int]*tailcfg.DERPRegion{
+			1: {
+				Nodes: []*tailcfg.DERPNode{
+					{
+						Name:     "t1",
+						RegionID: 1,
+					},
+				},
+			},
+			2: {
+				Nodes: []*tailcfg.DERPNode{
+					{
+						Name:     "t2",
+						RegionID: 2,
+					},
+				},
+			},
+		},
+	}
+	report := &netcheck.Report{
+		RegionLatency: map[int]time.Duration{
+			1: 10 * time.Millisecond,
+			2: 5 * time.Millisecond,
+			3: 30 * time.Millisecond,
+		},
+		PreferredDERP: 1,
+	}
+	nm := &netmap.NetworkMap{
+		Peers: []tailcfg.NodeView{
+			peer1,
+			peer2,
+		},
+		DERPMap: derpMap,
+	}
+	b := newTestLocalBackend(t)
+	msh := &mockSyspolicyHandler{
+		t: t,
+		stringPolicies: map[syspolicy.Key]*string{
+			syspolicy.ExitNodeID: ptr.To("auto:any"),
+		},
+	}
+	syspolicy.SetHandlerForTest(t, msh)
+	b.netMap = nm
+	b.lastSuggestedExitNode = peer1.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())
+	updatedNetmap := &netmap.NetworkMap{
+		Peers: []tailcfg.NodeView{
+			newPeer1,
+			peer2,
+		},
+		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")
+	}
+}
+
 func TestApplySysPolicy(t *testing.T) {
 	tests := []struct {
 		name           string
@@ -3036,6 +3102,18 @@ func withCap(version tailcfg.CapabilityVersion) peerOptFunc {
 	}
 }
 
+func withOnline(isOnline bool) peerOptFunc {
+	return func(n *tailcfg.Node) {
+		n.Online = &isOnline
+	}
+}
+
+func withNodeKey() peerOptFunc {
+	return func(n *tailcfg.Node) {
+		n.Key = key.NewNode().Public()
+	}
+}
+
 func deterministicRegionForTest(t testing.TB, want views.Slice[int], use int) selectRegionFunc {
 	t.Helper()