ipn/ipnlocal: improve sticky last suggestion

The last suggested exit node needs to be incorporated in the decision
making process when a new suggestion is requested, but currently it is
not quite right: it'll be used if the suggestion code has an error or a
netmap is unavailable, but it won't be used otherwise.

Instead, this makes the last suggestion into a tiebreaker when making a
random selection between equally-good options. If the last suggestion
does not make it to the final selection pool, then a different
suggestion will be made.

Since LocalBackend.SuggestExitNode is back to being a thin shim that
sets up the parameters to suggestExitNode, it no longer needs a test.
Its test was unable to be comprehensive anyway as the code being tested
contains an uncontrolled random number generator.

Updates tailscale/corp#19681

Change-Id: I94ecc9a0d1b622de3df4ef90523f1d3e67b4bfba
Signed-off-by: Adrian Dewhurst <adrian@tailscale.com>

ipn/ipnlocal/local_test.go

@@ -24,7 +24,6 @@ import (
 	"golang.org/x/net/dns/dnsmessage"
 	"tailscale.com/appc"
 	"tailscale.com/appc/appctest"
-	"tailscale.com/client/tailscale/apitype"
 	"tailscale.com/clientupdate"
 	"tailscale.com/control/controlclient"
 	"tailscale.com/drive"
@@ -41,7 +40,6 @@ import (
 	"tailscale.com/tstest"
 	"tailscale.com/types/dnstype"
 	"tailscale.com/types/key"
-	"tailscale.com/types/lazy"
 	"tailscale.com/types/logger"
 	"tailscale.com/types/logid"
 	"tailscale.com/types/netmap"
@@ -2813,14 +2811,14 @@ func deterministicRegionForTest(t testing.TB, want views.Slice[int], use int) se
 	}
 }
 
-func deterministicNodeForTest(t testing.TB, want views.Slice[tailcfg.StableNodeID], use tailcfg.StableNodeID) selectNodeFunc {
+func deterministicNodeForTest(t testing.TB, want views.Slice[tailcfg.StableNodeID], wantLast 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 {
+	return func(got views.Slice[tailcfg.NodeView], last tailcfg.StableNodeID) tailcfg.NodeView {
 		var ret tailcfg.NodeView
 
 		gotIDs := make([]tailcfg.StableNodeID, got.Len())
@@ -2838,6 +2836,9 @@ func deterministicNodeForTest(t testing.TB, want views.Slice[tailcfg.StableNodeI
 		if !views.SliceEqualAnyOrder(views.SliceOf(gotIDs), want) {
 			t.Errorf("candidate nodes = %v, want %v", gotIDs, want)
 		}
+		if last != wantLast {
+			t.Errorf("last node = %v, want %v", last, wantLast)
+		}
 		if !ret.Valid() {
 			t.Fatalf("did not find matching node in %v, want %v", gotIDs, use)
 		}
@@ -3264,14 +3265,14 @@ func TestSuggestExitNode(t *testing.T) {
 			if wantNodes == nil {
 				wantNodes = []tailcfg.StableNodeID{tt.wantID}
 			}
-			selectNode := deterministicNodeForTest(t, views.SliceOf(wantNodes), tt.wantID)
+			selectNode := deterministicNodeForTest(t, views.SliceOf(wantNodes), tt.lastSuggestion, tt.wantID)
 
 			var allowList set.Set[tailcfg.StableNodeID]
 			if tt.allowPolicy != nil {
 				allowList = set.SetOf(tt.allowPolicy)
 			}
 
-			got, err := suggestExitNode(tt.lastReport, tt.netMap, selectRegion, selectNode, allowList)
+			got, err := suggestExitNode(tt.lastReport, tt.netMap, tt.lastSuggestion, selectRegion, selectNode, allowList)
 			if got.Name != tt.wantName {
 				t.Errorf("name=%v, want %v", got.Name, tt.wantName)
 			}
@@ -3448,488 +3449,6 @@ func TestMinLatencyDERPregion(t *testing.T) {
 	}
 }
 
-func TestLastSuggestedExitNodeAsAPIType(t *testing.T) {
-	tests := []struct {
-		name                      string
-		lastSuggestedExitNode     lastSuggestedExitNode
-		wantRes                   apitype.ExitNodeSuggestionResponse
-		wantLastSuggestedExitNode lastSuggestedExitNode
-		wantErr                   error
-	}{
-		{
-			name:                      "last suggested exit node is populated",
-			lastSuggestedExitNode:     lastSuggestedExitNode{id: "test", name: "test"},
-			wantRes:                   apitype.ExitNodeSuggestionResponse{ID: "test", Name: "test"},
-			wantLastSuggestedExitNode: lastSuggestedExitNode{id: "test", name: "test"},
-		},
-		{
-			name:    "last suggested exit node is not populated",
-			wantErr: ErrUnableToSuggestLastExitNode,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			got, err := tt.lastSuggestedExitNode.asAPIType()
-			if got != tt.wantRes || err != tt.wantErr {
-				t.Errorf("got %v error %v, want %v error %v", got, err, tt.wantRes, tt.wantErr)
-			}
-		})
-	}
-}
-
-func TestLocalBackendSuggestExitNode(t *testing.T) {
-	tests := []struct {
-		name                      string
-		lastSuggestedExitNode     lastSuggestedExitNode
-		report                    *netcheck.Report
-		netMap                    netmap.NetworkMap
-		allowedSuggestedExitNodes []string
-		wantID                    tailcfg.StableNodeID
-		wantName                  string
-		wantErr                   error
-		wantLastSuggestedExitNode lastSuggestedExitNode
-	}{
-		{
-			name:                  "nil netmap, returns last suggested exit node",
-			lastSuggestedExitNode: lastSuggestedExitNode{name: "test", id: "test"},
-			report: &netcheck.Report{
-				RegionLatency: map[int]time.Duration{
-					1: 0,
-					2: -1,
-					3: 0,
-				},
-			},
-			wantID:                    "test",
-			wantName:                  "test",
-			wantLastSuggestedExitNode: lastSuggestedExitNode{name: "test", id: "test"},
-		},
-		{
-			name:                  "nil report, returns last suggested exit node",
-			lastSuggestedExitNode: lastSuggestedExitNode{name: "test", id: "test"},
-			netMap: netmap.NetworkMap{
-				SelfNode: (&tailcfg.Node{
-					Addresses: []netip.Prefix{
-						netip.MustParsePrefix("100.64.1.1/32"),
-						netip.MustParsePrefix("fe70::1/128"),
-					},
-				}).View(),
-				DERPMap: &tailcfg.DERPMap{
-					Regions: map[int]*tailcfg.DERPRegion{
-						1: {},
-						2: {},
-						3: {},
-					},
-				},
-			},
-			wantID:                    "test",
-			wantName:                  "test",
-			wantLastSuggestedExitNode: lastSuggestedExitNode{name: "test", id: "test"},
-		},
-		{
-			name:                  "found better derp node, last suggested exit node updates",
-			lastSuggestedExitNode: lastSuggestedExitNode{name: "test", id: "test"},
-			report: &netcheck.Report{
-				RegionLatency: map[int]time.Duration{
-					1: 10,
-					2: 10,
-					3: 5,
-				},
-				PreferredDERP: 1,
-			},
-			netMap: netmap.NetworkMap{
-				SelfNode: (&tailcfg.Node{
-					Addresses: []netip.Prefix{
-						netip.MustParsePrefix("100.64.1.1/32"),
-						netip.MustParsePrefix("fe70::1/128"),
-					},
-				}).View(),
-				DERPMap: &tailcfg.DERPMap{
-					Regions: map[int]*tailcfg.DERPRegion{
-						1: {},
-						2: {},
-						3: {},
-					},
-				},
-				Peers: []tailcfg.NodeView{
-					(&tailcfg.Node{
-						ID:       2,
-						StableID: "test",
-						Name:     "test",
-						DERP:     "127.3.3.40:1",
-						AllowedIPs: []netip.Prefix{
-							netip.MustParsePrefix("0.0.0.0/0"), netip.MustParsePrefix("::/0"),
-						},
-						CapMap: (tailcfg.NodeCapMap)(map[tailcfg.NodeCapability][]tailcfg.RawMessage{
-							tailcfg.NodeAttrSuggestExitNode: {},
-						}),
-					}).View(),
-					(&tailcfg.Node{
-						ID:       3,
-						StableID: "foo",
-						Name:     "foo",
-						DERP:     "127.3.3.40:3",
-						AllowedIPs: []netip.Prefix{
-							netip.MustParsePrefix("0.0.0.0/0"), netip.MustParsePrefix("::/0"),
-						},
-						CapMap: (tailcfg.NodeCapMap)(map[tailcfg.NodeCapability][]tailcfg.RawMessage{
-							tailcfg.NodeAttrSuggestExitNode: {},
-						}),
-					}).View(),
-				},
-			},
-			wantID:                    "foo",
-			wantName:                  "foo",
-			wantLastSuggestedExitNode: lastSuggestedExitNode{name: "foo", id: "foo"},
-		},
-		{
-			name:                  "found better mullvad node, last suggested exit node updates",
-			lastSuggestedExitNode: lastSuggestedExitNode{name: "San Jose", id: "3"},
-			report: &netcheck.Report{
-				RegionLatency: map[int]time.Duration{
-					1: 0,
-					2: 0,
-					3: 0,
-				},
-				PreferredDERP: 1,
-			},
-			netMap: netmap.NetworkMap{
-				SelfNode: (&tailcfg.Node{
-					Addresses: []netip.Prefix{
-						netip.MustParsePrefix("100.64.1.1/32"),
-						netip.MustParsePrefix("fe70::1/128"),
-					},
-				}).View(),
-				DERPMap: &tailcfg.DERPMap{
-					Regions: map[int]*tailcfg.DERPRegion{
-						1: {
-							Latitude:  40.73061,
-							Longitude: -73.935242,
-						},
-						2: {},
-						3: {},
-					},
-				},
-				Peers: []tailcfg.NodeView{
-					(&tailcfg.Node{
-						ID:       2,
-						StableID: "2",
-						AllowedIPs: []netip.Prefix{
-							netip.MustParsePrefix("0.0.0.0/0"), netip.MustParsePrefix("::/0"),
-						},
-						Name: "Dallas",
-						Hostinfo: (&tailcfg.Hostinfo{
-							Location: &tailcfg.Location{
-								Latitude:  32.89748,
-								Longitude: -97.040443,
-								Priority:  100,
-							},
-						}).View(),
-						CapMap: (tailcfg.NodeCapMap)(map[tailcfg.NodeCapability][]tailcfg.RawMessage{
-							tailcfg.NodeAttrSuggestExitNode: {},
-						}),
-					}).View(),
-					(&tailcfg.Node{
-						ID:       3,
-						StableID: "3",
-						AllowedIPs: []netip.Prefix{
-							netip.MustParsePrefix("0.0.0.0/0"), netip.MustParsePrefix("::/0"),
-						},
-						Name: "San Jose",
-						Hostinfo: (&tailcfg.Hostinfo{
-							Location: &tailcfg.Location{
-								Latitude:  37.3382082,
-								Longitude: -121.8863286,
-								Priority:  20,
-							},
-						}).View(),
-						CapMap: (tailcfg.NodeCapMap)(map[tailcfg.NodeCapability][]tailcfg.RawMessage{
-							tailcfg.NodeAttrSuggestExitNode: {},
-						}),
-					}).View(),
-				},
-			},
-			wantID:                    "2",
-			wantName:                  "Dallas",
-			wantLastSuggestedExitNode: lastSuggestedExitNode{name: "Dallas", id: "2"},
-		},
-		{
-			name:                  "ErrNoPreferredDERP, use last suggested exit node",
-			lastSuggestedExitNode: lastSuggestedExitNode{name: "test", id: "test"},
-			report: &netcheck.Report{
-				RegionLatency: map[int]time.Duration{
-					1: 10,
-					2: 10,
-					3: 5,
-				},
-				PreferredDERP: 0,
-			},
-			netMap: netmap.NetworkMap{
-				SelfNode: (&tailcfg.Node{
-					Addresses: []netip.Prefix{
-						netip.MustParsePrefix("100.64.1.1/32"),
-						netip.MustParsePrefix("fe70::1/128"),
-					},
-				}).View(),
-				DERPMap: &tailcfg.DERPMap{
-					Regions: map[int]*tailcfg.DERPRegion{
-						1: {},
-						2: {},
-						3: {},
-					},
-				},
-				Peers: []tailcfg.NodeView{
-					(&tailcfg.Node{
-						ID:       2,
-						StableID: "test",
-						Name:     "test",
-						DERP:     "127.3.3.40:1",
-						AllowedIPs: []netip.Prefix{
-							netip.MustParsePrefix("0.0.0.0/0"), netip.MustParsePrefix("::/0"),
-						},
-						CapMap: (tailcfg.NodeCapMap)(map[tailcfg.NodeCapability][]tailcfg.RawMessage{
-							tailcfg.NodeAttrSuggestExitNode: {},
-						}),
-					}).View(),
-					(&tailcfg.Node{
-						ID:       3,
-						StableID: "foo",
-						Name:     "foo",
-						DERP:     "127.3.3.40:3",
-						AllowedIPs: []netip.Prefix{
-							netip.MustParsePrefix("0.0.0.0/0"), netip.MustParsePrefix("::/0"),
-						},
-						CapMap: (tailcfg.NodeCapMap)(map[tailcfg.NodeCapability][]tailcfg.RawMessage{
-							tailcfg.NodeAttrSuggestExitNode: {},
-						}),
-					}).View(),
-				},
-			},
-			wantID:                    "test",
-			wantName:                  "test",
-			wantLastSuggestedExitNode: lastSuggestedExitNode{name: "test", id: "test"},
-		},
-		{
-			name:                  "ErrNoPreferredDERP, use last suggested exit node",
-			lastSuggestedExitNode: lastSuggestedExitNode{name: "test", id: "test"},
-			report: &netcheck.Report{
-				RegionLatency: map[int]time.Duration{
-					1: 10,
-					2: 10,
-					3: 5,
-				},
-				PreferredDERP: 0,
-			},
-			netMap: netmap.NetworkMap{
-				SelfNode: (&tailcfg.Node{
-					Addresses: []netip.Prefix{
-						netip.MustParsePrefix("100.64.1.1/32"),
-						netip.MustParsePrefix("fe70::1/128"),
-					},
-				}).View(),
-				DERPMap: &tailcfg.DERPMap{
-					Regions: map[int]*tailcfg.DERPRegion{
-						1: {},
-						2: {},
-						3: {},
-					},
-				},
-				Peers: []tailcfg.NodeView{
-					(&tailcfg.Node{
-						ID:       2,
-						StableID: "test",
-						Name:     "test",
-						DERP:     "127.3.3.40:1",
-						AllowedIPs: []netip.Prefix{
-							netip.MustParsePrefix("0.0.0.0/0"), netip.MustParsePrefix("::/0"),
-						},
-						CapMap: (tailcfg.NodeCapMap)(map[tailcfg.NodeCapability][]tailcfg.RawMessage{
-							tailcfg.NodeAttrSuggestExitNode: {},
-						}),
-					}).View(),
-					(&tailcfg.Node{
-						ID:       3,
-						StableID: "foo",
-						Name:     "foo",
-						DERP:     "127.3.3.40:3",
-						AllowedIPs: []netip.Prefix{
-							netip.MustParsePrefix("0.0.0.0/0"), netip.MustParsePrefix("::/0"),
-						},
-						CapMap: (tailcfg.NodeCapMap)(map[tailcfg.NodeCapability][]tailcfg.RawMessage{
-							tailcfg.NodeAttrSuggestExitNode: {},
-						}),
-					}).View(),
-				},
-			},
-			wantID:                    "test",
-			wantName:                  "test",
-			wantLastSuggestedExitNode: lastSuggestedExitNode{name: "test", id: "test"},
-		},
-		{
-			name: "unable to use last suggested exit node",
-			report: &netcheck.Report{
-				RegionLatency: map[int]time.Duration{
-					1: 10,
-					2: 10,
-					3: 5,
-				},
-				PreferredDERP: 0,
-			},
-			wantErr: ErrCannotSuggestExitNode,
-		},
-		{
-			name:                  "only pick from allowed suggested exit nodes",
-			lastSuggestedExitNode: lastSuggestedExitNode{name: "test", id: "test"},
-			report: &netcheck.Report{
-				RegionLatency: map[int]time.Duration{
-					1: 10,
-					2: 10,
-					3: 5,
-				},
-				PreferredDERP: 1,
-			},
-			netMap: netmap.NetworkMap{
-				SelfNode: (&tailcfg.Node{
-					Addresses: []netip.Prefix{
-						netip.MustParsePrefix("100.64.1.1/32"),
-						netip.MustParsePrefix("fe70::1/128"),
-					},
-				}).View(),
-				DERPMap: &tailcfg.DERPMap{
-					Regions: map[int]*tailcfg.DERPRegion{
-						1: {},
-						2: {},
-						3: {},
-					},
-				},
-				Peers: []tailcfg.NodeView{
-					(&tailcfg.Node{
-						ID:       2,
-						StableID: "test",
-						Name:     "test",
-						DERP:     "127.3.3.40:1",
-						AllowedIPs: []netip.Prefix{
-							netip.MustParsePrefix("0.0.0.0/0"), netip.MustParsePrefix("::/0"),
-						},
-						CapMap: (tailcfg.NodeCapMap)(map[tailcfg.NodeCapability][]tailcfg.RawMessage{
-							tailcfg.NodeAttrSuggestExitNode: {},
-							tailcfg.NodeAttrAutoExitNode:    {},
-						}),
-					}).View(),
-					(&tailcfg.Node{
-						ID:       3,
-						StableID: "foo",
-						Name:     "foo",
-						DERP:     "127.3.3.40:3",
-						AllowedIPs: []netip.Prefix{
-							netip.MustParsePrefix("0.0.0.0/0"), netip.MustParsePrefix("::/0"),
-						},
-						CapMap: (tailcfg.NodeCapMap)(map[tailcfg.NodeCapability][]tailcfg.RawMessage{
-							tailcfg.NodeAttrSuggestExitNode: {},
-							tailcfg.NodeAttrAutoExitNode:    {},
-						}),
-					}).View(),
-				},
-			},
-			allowedSuggestedExitNodes: []string{"test"},
-			wantID:                    "test",
-			wantName:                  "test",
-			wantLastSuggestedExitNode: lastSuggestedExitNode{name: "test", id: "test"},
-		},
-		{
-			name:                  "allowed suggested exit nodes not nil but length 0",
-			lastSuggestedExitNode: lastSuggestedExitNode{name: "test", id: "test"},
-			report: &netcheck.Report{
-				RegionLatency: map[int]time.Duration{
-					1: 10,
-					2: 10,
-					3: 5,
-				},
-				PreferredDERP: 1,
-			},
-			netMap: netmap.NetworkMap{
-				SelfNode: (&tailcfg.Node{
-					Addresses: []netip.Prefix{
-						netip.MustParsePrefix("100.64.1.1/32"),
-						netip.MustParsePrefix("fe70::1/128"),
-					},
-				}).View(),
-				DERPMap: &tailcfg.DERPMap{
-					Regions: map[int]*tailcfg.DERPRegion{
-						1: {},
-						2: {},
-						3: {},
-					},
-				},
-				Peers: []tailcfg.NodeView{
-					(&tailcfg.Node{
-						ID:       2,
-						StableID: "test",
-						Name:     "test",
-						DERP:     "127.3.3.40:1",
-						AllowedIPs: []netip.Prefix{
-							netip.MustParsePrefix("0.0.0.0/0"), netip.MustParsePrefix("::/0"),
-						},
-						CapMap: (tailcfg.NodeCapMap)(map[tailcfg.NodeCapability][]tailcfg.RawMessage{
-							tailcfg.NodeAttrSuggestExitNode: {},
-							tailcfg.NodeAttrAutoExitNode:    {},
-						}),
-					}).View(),
-					(&tailcfg.Node{
-						ID:       3,
-						StableID: "foo",
-						Name:     "foo",
-						DERP:     "127.3.3.40:3",
-						AllowedIPs: []netip.Prefix{
-							netip.MustParsePrefix("0.0.0.0/0"), netip.MustParsePrefix("::/0"),
-						},
-						CapMap: (tailcfg.NodeCapMap)(map[tailcfg.NodeCapability][]tailcfg.RawMessage{
-							tailcfg.NodeAttrSuggestExitNode: {},
-							tailcfg.NodeAttrAutoExitNode:    {},
-						}),
-					}).View(),
-				},
-			},
-			allowedSuggestedExitNodes: []string{},
-			wantID:                    "foo",
-			wantName:                  "foo",
-			wantLastSuggestedExitNode: lastSuggestedExitNode{name: "foo", id: "foo"},
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			lb := newTestLocalBackend(t)
-			msh := &mockSyspolicyHandler{
-				t: t,
-				stringArrayPolicies: map[syspolicy.Key][]string{
-					syspolicy.AllowedSuggestedExitNodes: nil,
-				},
-			}
-			if len(tt.allowedSuggestedExitNodes) != 0 {
-				msh.stringArrayPolicies[syspolicy.AllowedSuggestedExitNodes] = tt.allowedSuggestedExitNodes
-			}
-			syspolicy.SetHandlerForTest(t, msh)
-			getAllowedSuggestions = lazy.SyncFunc(fillAllowedSuggestions) // clear cache
-			lb.lastSuggestedExitNode = tt.lastSuggestedExitNode
-			lb.netMap = &tt.netMap
-			lb.sys.MagicSock.Get().SetLastNetcheckReportForTest(context.Background(), tt.report)
-			got, err := lb.SuggestExitNode()
-			if got.ID != tt.wantID {
-				t.Errorf("ID=%v, want=%v", got.ID, tt.wantID)
-			}
-			if got.Name != tt.wantName {
-				t.Errorf("Name=%v, want=%v", got.Name, tt.wantName)
-			}
-			if lb.lastSuggestedExitNode != tt.wantLastSuggestedExitNode {
-				t.Errorf("lastSuggestedExitNode=%v, want=%v", lb.lastSuggestedExitNode, tt.wantLastSuggestedExitNode)
-			}
-			if err != tt.wantErr {
-				t.Errorf("Error=%v, want=%v", err, tt.wantErr)
-			}
-		})
-	}
-}
 func TestEnableAutoUpdates(t *testing.T) {
 	lb := newTestLocalBackend(t)