From 3bf2bddbb58c30d332d78fb64115138d00ba6c09 Mon Sep 17 00:00:00 2001 From: Adrian Dewhurst Date: Mon, 3 Jun 2024 16:12:12 -0400 Subject: [PATCH] ipn/ipnlocal: improve testability of random node selection In order to test the sticky last suggestion code, a test was written for LocalBackend.SuggestExitNode but it contains a random number generator which makes writing comprehensive tests very difficult. This doesn't change how the last suggestion works, but it adds some infrastructure to make that easier in a later PR. This adds func parameters for the two randomized parts: breaking ties between DERP regions and breaking ties between nodes. This way tests can validate the entire list of tied options, rather than expecting a particular outcome given a particular random seed. As a result of this, the global random number generator can be used rather than seeding a local one each time. In order to see the tied nodes for the location based (i.e. Mullvad) case, pickWeighted needed to return a slice instead of a single arbitrary option, so there is a small change in how that works. Updates tailscale/corp#19681 Change-Id: I83c48a752abdec0f59c58ccfd8bfb3f3f17d0ea8 Signed-off-by: Adrian Dewhurst --- ipn/ipnlocal/local.go | 61 ++++++++++------- ipn/ipnlocal/local_test.go | 136 +++++++++++++++++++++++++++++-------- 2 files changed, 143 insertions(+), 54 deletions(-) diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index feda6e7e1..c711d7234 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -6420,9 +6420,8 @@ func (b *LocalBackend) SuggestExitNode() (response apitype.ExitNodeSuggestionRes } return last, err } - seed := time.Now().UnixNano() - r := rand.New(rand.NewSource(seed)) - res, err := suggestExitNode(lastReport, netMap, r) + + res, err := suggestExitNode(lastReport, netMap, randomRegion, randomNode) if err != nil { last, err := lastSuggestedExitNode.asAPIType() if err != nil { @@ -6437,6 +6436,13 @@ func (b *LocalBackend) SuggestExitNode() (response apitype.ExitNodeSuggestionRes return res, err } +// selectRegionFunc returns a DERP region from the slice of candidate regions. +// The value is returned, not the slice index. +type selectRegionFunc func(views.Slice[int]) int + +// selectNodeFunc returns a node from the slice of candidate nodes. +type selectNodeFunc func(nodes views.Slice[tailcfg.NodeView]) tailcfg.NodeView + // asAPIType formats a response with the last suggested exit node's ID and name. // Returns error if there is no id or name. // Used as a fallback before returning a nil response and error. @@ -6449,8 +6455,8 @@ func (n lastSuggestedExitNode) asAPIType() (res apitype.ExitNodeSuggestionRespon return res, ErrUnableToSuggestLastExitNode } -func suggestExitNode(report *netcheck.Report, netMap *netmap.NetworkMap, r *rand.Rand) (res apitype.ExitNodeSuggestionResponse, err error) { - if report.PreferredDERP == 0 { +func suggestExitNode(report *netcheck.Report, netMap *netmap.NetworkMap, selectRegion selectRegionFunc, selectNode selectNodeFunc) (res apitype.ExitNodeSuggestionResponse, err error) { + if report.PreferredDERP == 0 || netMap == nil || netMap.DERPMap == nil { return res, ErrNoPreferredDERP } var allowedCandidates set.Set[string] @@ -6461,6 +6467,9 @@ func suggestExitNode(report *netcheck.Report, netMap *netmap.NetworkMap, r *rand } candidates := make([]tailcfg.NodeView, 0, len(netMap.Peers)) for _, peer := range netMap.Peers { + if !peer.Valid() { + continue + } if allowedCandidates != nil && !allowedCandidates.Contains(string(peer.StableID())) { continue } @@ -6484,7 +6493,10 @@ func suggestExitNode(report *netcheck.Report, netMap *netmap.NetworkMap, r *rand } candidatesByRegion := make(map[int][]tailcfg.NodeView, len(netMap.DERPMap.Regions)) - var preferredDERP *tailcfg.DERPRegion = netMap.DERPMap.Regions[report.PreferredDERP] + preferredDERP, ok := netMap.DERPMap.Regions[report.PreferredDERP] + if !ok { + return res, ErrNoPreferredDERP + } var minDistance float64 = math.MaxFloat64 type nodeDistance struct { nv tailcfg.NodeView @@ -6492,9 +6504,6 @@ func suggestExitNode(report *netcheck.Report, netMap *netmap.NetworkMap, r *rand } distances := make([]nodeDistance, 0, len(candidates)) for _, c := range candidates { - if !c.Valid() { - continue - } if c.DERP() != "" { ipp, err := netip.ParseAddrPort(c.DERP()) if err != nil { @@ -6533,13 +6542,13 @@ func suggestExitNode(report *netcheck.Report, netMap *netmap.NetworkMap, r *rand if len(candidatesByRegion) > 0 { minRegion := minLatencyDERPRegion(xmaps.Keys(candidatesByRegion), report) if minRegion == 0 { - minRegion = randomRegion(xmaps.Keys(candidatesByRegion), r) + minRegion = selectRegion(views.SliceOf(xmaps.Keys(candidatesByRegion))) } regionCandidates, ok := candidatesByRegion[minRegion] if !ok { return res, errors.New("no candidates in expected region: this is a bug") } - chosen := randomNode(regionCandidates, r) + chosen := selectNode(views.SliceOf(regionCandidates)) res.ID = chosen.StableID() res.Name = chosen.Name() if hi := chosen.Hostinfo(); hi.Valid() { @@ -6565,7 +6574,8 @@ func suggestExitNode(report *netcheck.Report, netMap *netmap.NetworkMap, r *rand pickFrom = append(pickFrom, candidate.nv) } } - chosen := pickWeighted(pickFrom) + bestCandidates := pickWeighted(pickFrom) + chosen := selectNode(views.SliceOf(bestCandidates)) if !chosen.Valid() { return res, errors.New("chosen candidate invalid: this is a bug") } @@ -6580,36 +6590,35 @@ func suggestExitNode(report *netcheck.Report, netMap *netmap.NetworkMap, r *rand } // pickWeighted chooses the node with highest priority given a list of mullvad nodes. -func pickWeighted(candidates []tailcfg.NodeView) tailcfg.NodeView { +func pickWeighted(candidates []tailcfg.NodeView) []tailcfg.NodeView { maxWeight := 0 - var best tailcfg.NodeView + best := make([]tailcfg.NodeView, 0, 1) for _, c := range candidates { hi := c.Hostinfo() if !hi.Valid() { continue } loc := hi.Location() - if loc == nil || loc.Priority <= maxWeight { + if loc == nil || loc.Priority < maxWeight { continue } + if maxWeight != loc.Priority { + best = best[:0] + } maxWeight = loc.Priority - best = c + best = append(best, c) } return best } -// randomNode chooses a node randomly given a list of nodes and a *rand.Rand. -func randomNode(nodes []tailcfg.NodeView, r *rand.Rand) tailcfg.NodeView { - return nodes[r.Intn(len(nodes))] +// randomRegion is a selectRegionFunc that selects a uniformly random region. +func randomRegion(regions views.Slice[int]) int { + return regions.At(rand.Intn(regions.Len())) } -// randomRegion chooses a region randomly given a list of ints and a *rand.Rand -func randomRegion(regions []int, r *rand.Rand) int { - if testenv.InTest() { - regions = slices.Clone(regions) - slices.Sort(regions) - } - return regions[r.Intn(len(regions))] +// randomNode is a selectNodeFunc that returns a uniformly random node. +func randomNode(nodes views.Slice[tailcfg.NodeView]) tailcfg.NodeView { + return nodes.At(rand.Intn(nodes.Len())) } // minLatencyDERPRegion returns the region with the lowest latency value given the last netcheck report. diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index a1de50ec5..70da1130e 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -9,7 +9,6 @@ import ( "errors" "fmt" "math" - "math/rand" "net" "net/http" "net/netip" @@ -2775,6 +2774,54 @@ func withSuggest() peerOptFunc { } } +func deterministicRegionForTest(t testing.TB, want views.Slice[int], use int) selectRegionFunc { + t.Helper() + + if !views.SliceContains(want, use) { + t.Errorf("invalid test: use %v is not in want %v", use, want) + } + + return func(got views.Slice[int]) int { + if !views.SliceEqualAnyOrder(got, want) { + t.Errorf("candidate regions = %v, want %v", got, want) + } + return use + } +} + +func deterministicNodeForTest(t testing.TB, want views.Slice[tailcfg.StableNodeID], use tailcfg.StableNodeID) selectNodeFunc { + t.Helper() + + if !views.SliceContains(want, use) { + t.Errorf("invalid test: use %v is not in want %v", use, want) + } + + return func(got views.Slice[tailcfg.NodeView]) tailcfg.NodeView { + var ret tailcfg.NodeView + + gotIDs := make([]tailcfg.StableNodeID, got.Len()) + for i := range got.Len() { + nv := got.At(i) + if !nv.Valid() { + t.Fatalf("invalid node at index %v", i) + } + + gotIDs[i] = nv.StableID() + if nv.StableID() == use { + ret = nv + } + } + if !views.SliceEqualAnyOrder(views.SliceOf(gotIDs), want) { + t.Errorf("candidate nodes = %v, want %v", gotIDs, want) + } + if !ret.Valid() { + t.Fatalf("did not find matching node in %v, want %v", gotIDs, use) + } + + return ret + } +} + func TestSuggestExitNode(t *testing.T) { defaultDERPMap := &tailcfg.DERPMap{ Regions: map[int]*tailcfg.DERPRegion{ @@ -2911,7 +2958,6 @@ func TestSuggestExitNode(t *testing.T) { tests := []struct { name string - seed int64 lastReport *netcheck.Report netMap *netmap.NetworkMap @@ -2919,6 +2965,11 @@ func TestSuggestExitNode(t *testing.T) { allowPolicy []string + wantRegions []int + useRegion int + + wantNodes []tailcfg.StableNodeID + wantID tailcfg.StableNodeID wantName string wantLocation tailcfg.LocationView @@ -2936,15 +2987,21 @@ func TestSuggestExitNode(t *testing.T) { peer2DERP1, }, }, + wantNodes: []tailcfg.StableNodeID{ + "stable1", + "stable2", + }, wantName: "peer1", wantID: "stable1", }, { - name: "2 exit nodes different regions unknown latency", - lastReport: noLatency1Report, - netMap: defaultNetmap, - wantName: "peer2", - wantID: "stable2", + name: "2 exit nodes different regions unknown latency", + lastReport: noLatency1Report, + netMap: defaultNetmap, + wantRegions: []int{1, 3}, // the only regions with peers + useRegion: 1, + wantName: "peer2", + wantID: "stable2", }, { name: "2 derp based exit nodes, different regions, equal latency", @@ -2964,8 +3021,10 @@ func TestSuggestExitNode(t *testing.T) { peer3, }, }, - wantName: "peer1", - wantID: "stable1", + wantRegions: []int{1, 2}, + useRegion: 1, + wantName: "peer1", + wantID: "stable1", }, { name: "mullvad nodes, no derp based exit nodes", @@ -3003,6 +3062,7 @@ func TestSuggestExitNode(t *testing.T) { fortWorthPeer8LowPriority, }, }, + wantNodes: []tailcfg.StableNodeID{"stable5", "stable8"}, wantID: "stable5", wantLocation: dallas.View(), wantName: "Dallas", @@ -3019,8 +3079,9 @@ func TestSuggestExitNode(t *testing.T) { peer4DERP3, }, }, - wantID: "stable4", - wantName: "peer4", + useRegion: 3, + wantID: "stable4", + wantName: "peer4", }, { name: "no peers", @@ -3053,7 +3114,6 @@ func TestSuggestExitNode(t *testing.T) { }, { name: "prefer last node", - seed: 1, lastReport: preferred1Report, netMap: &netmap.NetworkMap{ SelfNode: selfNode.View(), @@ -3064,8 +3124,12 @@ func TestSuggestExitNode(t *testing.T) { }, }, lastSuggestion: "stable2", - wantName: "peer2", - wantID: "stable2", + wantNodes: []tailcfg.StableNodeID{ + "stable1", + "stable2", + }, + wantName: "peer2", + wantID: "stable2", }, { name: "found better derp node", @@ -3088,6 +3152,7 @@ func TestSuggestExitNode(t *testing.T) { fortWorthPeer8LowPriority, }, }, + wantNodes: []tailcfg.StableNodeID{"stable5", "stable8"}, wantID: "stable5", wantName: "Dallas", wantLocation: dallas.View(), @@ -3105,15 +3170,16 @@ func TestSuggestExitNode(t *testing.T) { fortWorthPeer7, }, }, + wantNodes: []tailcfg.StableNodeID{"stable7"}, wantID: "stable7", wantName: "Fort Worth", wantLocation: fortWorth.View(), }, { name: "large netmap", - seed: 1, lastReport: preferred1Report, netMap: largeNetmap, + wantNodes: []tailcfg.StableNodeID{"stable1", "stable2"}, wantID: "stable2", wantName: "peer2", }, @@ -3125,10 +3191,10 @@ func TestSuggestExitNode(t *testing.T) { }, { name: "only derp suggestions", - seed: 1, lastReport: preferred1Report, netMap: largeNetmap, allowPolicy: []string{"stable1", "stable2", "stable3"}, + wantNodes: []tailcfg.StableNodeID{"stable1", "stable2"}, wantID: "stable2", wantName: "peer2", }, @@ -3172,8 +3238,19 @@ func TestSuggestExitNode(t *testing.T) { } syspolicy.SetHandlerForTest(t, &mh) - r := rand.New(rand.NewSource(tt.seed)) - got, err := suggestExitNode(tt.lastReport, tt.netMap, r) + wantRegions := tt.wantRegions + if wantRegions == nil { + wantRegions = []int{tt.useRegion} + } + selectRegion := deterministicRegionForTest(t, views.SliceOf(wantRegions), tt.useRegion) + + wantNodes := tt.wantNodes + if wantNodes == nil { + wantNodes = []tailcfg.StableNodeID{tt.wantID} + } + selectNode := deterministicNodeForTest(t, views.SliceOf(wantNodes), tt.wantID) + + got, err := suggestExitNode(tt.lastReport, tt.netMap, selectRegion, selectNode) if got.Name != tt.wantName { t.Errorf("name=%v, want %v", got.Name, tt.wantName) } @@ -3204,7 +3281,7 @@ func TestSuggestExitNodePickWeighted(t *testing.T) { tests := []struct { name string candidates []tailcfg.NodeView - wantID tailcfg.StableNodeID + wantIDs []tailcfg.StableNodeID }{ { name: "different priorities", @@ -3212,7 +3289,7 @@ func TestSuggestExitNodePickWeighted(t *testing.T) { makePeer(2, withExitRoutes(), withLocation(location20.View())), makePeer(3, withExitRoutes(), withLocation(location10.View())), }, - wantID: "stable2", + wantIDs: []tailcfg.StableNodeID{"stable2"}, }, { name: "same priorities", @@ -3220,31 +3297,34 @@ func TestSuggestExitNodePickWeighted(t *testing.T) { makePeer(2, withExitRoutes(), withLocation(location10.View())), makePeer(3, withExitRoutes(), withLocation(location10.View())), }, - wantID: "stable2", + wantIDs: []tailcfg.StableNodeID{"stable2", "stable3"}, }, { name: "<1 candidates", candidates: []tailcfg.NodeView{}, - wantID: "", }, { name: "1 candidate", candidates: []tailcfg.NodeView{ makePeer(2, withExitRoutes(), withLocation(location20.View())), }, - wantID: "stable2", + wantIDs: []tailcfg.StableNodeID{"stable2"}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got := pickWeighted(tt.candidates) - gotID := tailcfg.StableNodeID("") - if got.Valid() { - gotID = got.StableID() + gotIDs := make([]tailcfg.StableNodeID, 0, len(got)) + for _, n := range got { + if !n.Valid() { + gotIDs = append(gotIDs, "") + continue + } + gotIDs = append(gotIDs, n.StableID()) } - if gotID != tt.wantID { - t.Errorf("node IDs = %v, want %v", gotID, tt.wantID) + if !views.SliceEqualAnyOrder(views.SliceOf(gotIDs), views.SliceOf(tt.wantIDs)) { + t.Errorf("node IDs = %v, want %v", gotIDs, tt.wantIDs) } }) }