diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index d0ac41858..7dc8d770e 100644 --- a/ipn/ipnlocal/local.go +++ b/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(), - DomainName: st.NetMap.DomainName(), + MagicDNSName: curNetMap.MagicDNSSuffix(), + 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 { - diff := st.NetMap.ConciseDiffFrom(netMap) + if oldNetMap != nil { + 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. diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index df8cc1d94..b5c22e54b 100644 --- a/ipn/ipnlocal/local_test.go +++ b/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()