From bb498a83931dd7ec7a1b5c8d532724e08e8969dd Mon Sep 17 00:00:00 2001 From: Simon Law Date: Wed, 29 Jul 2026 16:51:55 -0400 Subject: [PATCH] net/netcheck: extract Compare function for region latencies (#20658) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit There are two places in the code where we need to compare the latencies of a netcheck report. In both cases, the comparison wasn’t well tested. This PR extracts that logic into a Compare method of the new RegionLatency type. This type wraps the map of latency measurements keyed by region ID. Updates #cleanup Change-Id: I7f248988973007c2f452283c1b82f84b03068f77 Signed-off-by: Simon Law --- cmd/k8s-operator/depaware.txt | 2 +- cmd/tailscale/cli/netcheck.go | 19 +---- cmd/tailscale/depaware.txt | 1 + cmd/tailscaled/depaware-min.txt | 3 +- cmd/tailscaled/depaware-minbox.txt | 1 + cmd/tailscaled/depaware.txt | 2 +- cmd/tsidp/depaware.txt | 2 +- ipn/ipnlocal/local.go | 19 +---- net/netcheck/netcheck.go | 37 +++++++++- net/netcheck/netcheck_test.go | 115 +++++++++++++++++++++++++++++ tsnet/depaware.txt | 2 +- 11 files changed, 162 insertions(+), 41 deletions(-) diff --git a/cmd/k8s-operator/depaware.txt b/cmd/k8s-operator/depaware.txt index 51222b6b0..99b08763f 100644 --- a/cmd/k8s-operator/depaware.txt +++ b/cmd/k8s-operator/depaware.txt @@ -797,7 +797,7 @@ tailscale.com/cmd/k8s-operator dependencies: (generated by github.com/tailscale/ tailscale.com/net/ipset from tailscale.com/ipn/ipnlocal+ tailscale.com/net/memnet from tailscale.com/tsnet tailscale.com/net/netaddr from tailscale.com/ipn+ - tailscale.com/net/netcheck from tailscale.com/wgengine/magicsock + tailscale.com/net/netcheck from tailscale.com/wgengine/magicsock+ tailscale.com/net/neterror from tailscale.com/net/dns/resolver+ tailscale.com/net/netkernelconf from tailscale.com/ipn/ipnlocal tailscale.com/net/netknob from tailscale.com/logpolicy+ diff --git a/cmd/tailscale/cli/netcheck.go b/cmd/tailscale/cli/netcheck.go index 52d66d516..20332e191 100644 --- a/cmd/tailscale/cli/netcheck.go +++ b/cmd/tailscale/cli/netcheck.go @@ -10,10 +10,11 @@ "fmt" "io" "log" + "maps" "math" "net/http" "net/netip" - "sort" + "slices" "strings" "time" @@ -214,21 +215,7 @@ func printNetCheckReport(dm *tailcfg.DERPMap, report *netcheck.Report) error { printf("\t* Nearest DERP: [none]\n") } printf("\t* DERP latency:\n") - var rids []int - for rid := range dm.Regions { - rids = append(rids, rid) - } - sort.Slice(rids, func(i, j int) bool { - l1, ok1 := report.RegionLatency[rids[i]] - l2, ok2 := report.RegionLatency[rids[j]] - if ok1 != ok2 { - return ok1 // defined things sort first - } - if !ok1 { - return rids[i] < rids[j] - } - return l1 < l2 - }) + rids := slices.SortedFunc(maps.Keys(dm.Regions), report.RegionLatency.Compare) for _, rid := range rids { d, ok := report.RegionLatency[rid] var latency string diff --git a/cmd/tailscale/depaware.txt b/cmd/tailscale/depaware.txt index c240581e7..bdca200ba 100644 --- a/cmd/tailscale/depaware.txt +++ b/cmd/tailscale/depaware.txt @@ -267,6 +267,7 @@ tailscale.com/cmd/tailscale dependencies: (generated by github.com/tailscale/dep tailscale.com/tsweb from tailscale.com/util/eventbus+ tailscale.com/tsweb/varz from tailscale.com/util/usermetric+ tailscale.com/types/appctype from tailscale.com/client/local+ + tailscale.com/types/bools from tailscale.com/net/netcheck tailscale.com/types/dnstype from tailscale.com/tailcfg+ tailscale.com/types/empty from tailscale.com/ipn tailscale.com/types/ipproto from tailscale.com/ipn+ diff --git a/cmd/tailscaled/depaware-min.txt b/cmd/tailscaled/depaware-min.txt index e57c3bbb6..395dcd40f 100644 --- a/cmd/tailscaled/depaware-min.txt +++ b/cmd/tailscaled/depaware-min.txt @@ -97,7 +97,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de tailscale.com/net/flowtrack from tailscale.com/wgengine/filter tailscale.com/net/ipset from tailscale.com/ipn/ipnlocal+ tailscale.com/net/netaddr from tailscale.com/ipn+ - tailscale.com/net/netcheck from tailscale.com/wgengine/magicsock + tailscale.com/net/netcheck from tailscale.com/wgengine/magicsock+ tailscale.com/net/neterror from tailscale.com/net/batching+ tailscale.com/net/netkernelconf from tailscale.com/ipn/ipnlocal tailscale.com/net/netknob from tailscale.com/logpolicy+ @@ -135,6 +135,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de tailscale.com/tstime/mono from tailscale.com/net/tstun+ tailscale.com/tstime/rate from tailscale.com/wgengine/filter tailscale.com/types/appctype from tailscale.com/ipn/ipnlocal+ + tailscale.com/types/bools from tailscale.com/net/netcheck tailscale.com/types/dnstype from tailscale.com/client/tailscale/apitype+ tailscale.com/types/empty from tailscale.com/ipn+ tailscale.com/types/events from tailscale.com/control/controlclient+ diff --git a/cmd/tailscaled/depaware-minbox.txt b/cmd/tailscaled/depaware-minbox.txt index cf34705e6..8edf656b1 100644 --- a/cmd/tailscaled/depaware-minbox.txt +++ b/cmd/tailscaled/depaware-minbox.txt @@ -154,6 +154,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de tailscale.com/tstime/mono from tailscale.com/net/tstun+ tailscale.com/tstime/rate from tailscale.com/wgengine/filter tailscale.com/types/appctype from tailscale.com/ipn/ipnlocal+ + tailscale.com/types/bools from tailscale.com/net/netcheck tailscale.com/types/dnstype from tailscale.com/client/tailscale/apitype+ tailscale.com/types/empty from tailscale.com/ipn+ tailscale.com/types/events from tailscale.com/control/controlclient+ diff --git a/cmd/tailscaled/depaware.txt b/cmd/tailscaled/depaware.txt index 27fe06551..8b1b17cac 100644 --- a/cmd/tailscaled/depaware.txt +++ b/cmd/tailscaled/depaware.txt @@ -431,7 +431,7 @@ tailscale.com/cmd/tailscaled dependencies: (generated by github.com/tailscale/de tailscale.com/tsweb from tailscale.com/util/eventbus+ tailscale.com/tsweb/varz from tailscale.com/cmd/tailscaled+ tailscale.com/types/appctype from tailscale.com/ipn/ipnlocal+ - tailscale.com/types/bools from tailscale.com/wgengine/netlog + tailscale.com/types/bools from tailscale.com/wgengine/netlog+ tailscale.com/types/dnstype from tailscale.com/ipn/ipnlocal+ tailscale.com/types/empty from tailscale.com/ipn+ tailscale.com/types/events from tailscale.com/control/controlclient+ diff --git a/cmd/tsidp/depaware.txt b/cmd/tsidp/depaware.txt index 0e107b4f0..af0ed1218 100644 --- a/cmd/tsidp/depaware.txt +++ b/cmd/tsidp/depaware.txt @@ -194,7 +194,7 @@ tailscale.com/cmd/tsidp dependencies: (generated by github.com/tailscale/depawar tailscale.com/net/ipset from tailscale.com/ipn/ipnlocal+ tailscale.com/net/memnet from tailscale.com/tsnet tailscale.com/net/netaddr from tailscale.com/ipn+ - tailscale.com/net/netcheck from tailscale.com/wgengine/magicsock + tailscale.com/net/netcheck from tailscale.com/wgengine/magicsock+ tailscale.com/net/neterror from tailscale.com/net/dns/resolver+ tailscale.com/net/netkernelconf from tailscale.com/ipn/ipnlocal tailscale.com/net/netknob from tailscale.com/logpolicy+ diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 5170b9ee7..ba3a89241 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -58,6 +58,7 @@ "tailscale.com/net/dnscache" "tailscale.com/net/dnsfallback" "tailscale.com/net/ipset" + "tailscale.com/net/netcheck" "tailscale.com/net/netkernelconf" "tailscale.com/net/netmon" "tailscale.com/net/netns" @@ -9059,22 +9060,8 @@ func randomNode(nodes views.Slice[tailcfg.NodeView], prefer tailcfg.StableNodeID // minLatencyDERPRegion returns the region with the lowest latency value given // the per-region latency map. If there are no latency values, it returns 0. -func minLatencyDERPRegion(regions []int, regionLatency map[int]time.Duration) int { - min := slices.MinFunc(regions, func(i, j int) int { - const largeDuration time.Duration = math.MaxInt64 - iLatency, ok := regionLatency[i] - if !ok { - iLatency = largeDuration - } - jLatency, ok := regionLatency[j] - if !ok { - jLatency = largeDuration - } - if c := cmp.Compare(iLatency, jLatency); c != 0 { - return c - } - return cmp.Compare(i, j) - }) +func minLatencyDERPRegion(regions []int, regionLatency netcheck.RegionLatency) int { + min := slices.MinFunc(regions, regionLatency.Compare) latency, ok := regionLatency[min] if !ok || latency == 0 { return 0 diff --git a/net/netcheck/netcheck.go b/net/netcheck/netcheck.go index a3f7fe31a..a39bdd881 100644 --- a/net/netcheck/netcheck.go +++ b/net/netcheck/netcheck.go @@ -39,6 +39,7 @@ "tailscale.com/net/stun" "tailscale.com/syncs" "tailscale.com/tailcfg" + "tailscale.com/types/bools" "tailscale.com/types/logger" "tailscale.com/types/nettype" "tailscale.com/types/opt" @@ -111,10 +112,10 @@ type Report struct { // Empty means not checked. PCP opt.Bool - PreferredDERP int // or 0 for unknown - RegionLatency map[int]time.Duration // keyed by DERP Region ID - RegionV4Latency map[int]time.Duration // keyed by DERP Region ID - RegionV6Latency map[int]time.Duration // keyed by DERP Region ID + PreferredDERP int // or 0 for unknown + RegionLatency RegionLatency // keyed by DERP Region ID + RegionV4Latency RegionLatency // keyed by DERP Region ID + RegionV6Latency RegionLatency // keyed by DERP Region ID GlobalV4Counters map[netip.AddrPort]int // number of times the endpoint was observed GlobalV6Counters map[netip.AddrPort]int // number of times the endpoint was observed @@ -184,6 +185,34 @@ func (r *Report) Clone() *Report { return &r2 } +// RegionLatency is a map of DERP region IDs associated with their measured latencies. +type RegionLatency map[int]time.Duration + +// Compare compares the latency of the regions i and j. It returns +// +// -1 if region i is contained in lat, but region j is missing +// +1 if region j is contained in lat, but region i is missing +// +// Otherwise, it returns +// +// -1 if the latency of region i is less than the latency of region j +// +1 if the latency of region i is greater than the latency of region j +// +// If both latency values are equal, then it breaks ties by region ID and returns +// +// -1 if i is less than j +// 0 if i equals j +// +1 if i is greater than j +func (lat RegionLatency) Compare(i, j int) int { + iLat, iOK := lat[i] + jLat, jOK := lat[j] + return cmp.Or( + bools.Compare(!iOK, !jOK), // defined things sort first + cmp.Compare(iLat, jLat), // sort by latency + cmp.Compare(i, j), // break ties by region ID + ) +} + // Client generates Reports describing the result of both passive and active // network configuration probing. It provides two different modes of report, a // full report (see MakeNextReportFull) and a more lightweight incremental diff --git a/net/netcheck/netcheck_test.go b/net/netcheck/netcheck_test.go index 70c07aacd..dad71cdee 100644 --- a/net/netcheck/netcheck_test.go +++ b/net/netcheck/netcheck_test.go @@ -1101,3 +1101,118 @@ func TestNoUDPNilGetReportOpts(t *testing.T) { t.Fatal("unexpected working UDP") } } + +func TestRegionLatencyCompare(t *testing.T) { + for _, tc := range []struct { + name string + lat RegionLatency + i int + j int + want int + }{ + { + name: "no-lat-ids-equal", + lat: RegionLatency{}, + i: 1, + j: 1, + want: 0, + }, + { + name: "no-lat-ids-increasing", + lat: RegionLatency{}, + i: 1, + j: 2, + want: -1, + }, + { + name: "no-lat-ids-decreasing", + lat: RegionLatency{}, + i: 2, + j: 1, + want: +1, + }, + { + name: "i-lat-only-ids-increasing", + lat: RegionLatency{1: 10}, + i: 1, + j: 2, + want: -1, + }, + { + name: "i-lat-only-ids-decreasing", + lat: RegionLatency{2: 20}, + i: 2, + j: 1, + want: -1, + }, + { + name: "j-lat-only-ids-increasing", + lat: RegionLatency{2: 20}, + i: 1, + j: 2, + want: +1, + }, + { + name: "j-lat-only-ids-decreasing", + lat: RegionLatency{1: 10}, + i: 2, + j: 1, + want: +1, + }, + { + name: "i-lower-lat-ids-increasing", + lat: RegionLatency{1: 10, 2: 20}, + i: 1, + j: 2, + want: -1, + }, + { + name: "i-lower-lat-ids-decreasing", + lat: RegionLatency{2: 20, 1: 100}, + i: 2, + j: 1, + want: -1, + }, + { + name: "j-lower-lat-ids-increasing", + lat: RegionLatency{1: 100, 2: 20}, + i: 1, + j: 2, + want: +1, + }, + { + name: "j-lower-lat-ids-decreasing", + lat: RegionLatency{2: 20, 1: 10}, + i: 2, + j: 1, + want: +1, + }, + { + name: "equal-lat-ids-increasing", + lat: RegionLatency{1: 10, 2: 10}, + i: 1, + j: 2, + want: -1, + }, + { + name: "equal-lat-ids-decreasing", + lat: RegionLatency{2: 20, 1: 20}, + i: 2, + j: 1, + want: +1, + }, + { + name: "equal-lat-ids-equal", + lat: RegionLatency{1: 10}, + i: 1, + j: 1, + want: 0, + }, + } { + t.Run(tc.name, func(t *testing.T) { + if got := tc.lat.Compare(tc.i, tc.j); got != tc.want { + t.Errorf("got %d, want %d", got, tc.want) + } + }) + } +} diff --git a/tsnet/depaware.txt b/tsnet/depaware.txt index 4f80f04d0..302f5e0d8 100644 --- a/tsnet/depaware.txt +++ b/tsnet/depaware.txt @@ -190,7 +190,7 @@ tailscale.com/tsnet dependencies: (generated by github.com/tailscale/depaware) tailscale.com/net/ipset from tailscale.com/ipn/ipnlocal+ tailscale.com/net/memnet from tailscale.com/tsnet tailscale.com/net/netaddr from tailscale.com/ipn+ - tailscale.com/net/netcheck from tailscale.com/wgengine/magicsock + tailscale.com/net/netcheck from tailscale.com/wgengine/magicsock+ tailscale.com/net/neterror from tailscale.com/net/dns/resolver+ tailscale.com/net/netkernelconf from tailscale.com/ipn/ipnlocal tailscale.com/net/netknob from tailscale.com/logpolicy+