diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 7ad2b22417785..37bf195b6cf43 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -6373,11 +6373,11 @@ 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] var minDistance float64 = math.MaxFloat64 - type candidateDistance struct { + type nodeDistance struct { nv tailcfg.NodeView distance float64 // in meters, approximately } - candidateDistances := make([]candidateDistance, len(candidates)) + distances := make([]nodeDistance, len(candidates)) for _, c := range candidates { if !c.Valid() { continue @@ -6397,9 +6397,12 @@ func suggestExitNode(report *netcheck.Report, netMap *netmap.NetworkMap, r *rand } continue } + if len(candidatesByRegion) > 0 { + // Since a candidate exists that does have a DERP home, skip this candidate. We never select + // a candidate without a DERP home if there is a candidate available with a DERP home. + continue + } // This candidate does not have a DERP home. - // Since a candidate exists that does have a DERP home, skip this candidate. We never select - // a candidate without a DERP home if there is a candidate available with a DERP home. // Use geographic distance from our DERP home to estimate how good this candidate is. hi := c.Hostinfo() if !hi.Valid() { @@ -6409,11 +6412,11 @@ func suggestExitNode(report *netcheck.Report, netMap *netmap.NetworkMap, r *rand if loc == nil { continue } - distance := longLatDistance(preferredDERP.Latitude, preferredDERP.Longitude, c.Hostinfo().Location().Latitude, c.Hostinfo().Location().Longitude) + distance := longLatDistance(preferredDERP.Latitude, preferredDERP.Longitude, loc.Latitude, loc.Longitude) if distance < minDistance { minDistance = distance } - candidateDistances = append(candidateDistances, candidateDistance{nv: c, distance: distance}) + distances = append(distances, nodeDistance{nv: c, distance: distance}) } // First, try to select an exit node that has the closest DERP home, based on lastReport's DERP latency. if len(candidatesByRegion) > 0 { @@ -6433,10 +6436,17 @@ func suggestExitNode(report *netcheck.Report, netMap *netmap.NetworkMap, r *rand return res, nil } // None of the candidates have a DERP home, so proceed to select based on geographical distance from our preferred DERP region. - const minDistanceEpsilon = 100000 // consider peers at most 100km further away than the minimum distance found + // allowanceMeters is the extra distance that will be permitted when considering peers. By this point, there + // are multiple approximations taking place (DERP location standing in for this device's location, the peer's + // location may only be city granularity, the distance algorithm assumes a spherical planet, etc.) so it is + // reasonable to consider peers that are similar distances. Those peers are good enough to be within + // measurement error. 100km corresponds to approximately 1ms of additional round trip light + // propagation delay in a fiber optic cable and seems like a reasonable heuristic. It may be adjusted in + // future. + const allowanceMeters = 100000 // consider peers at most 100km further away than the minimum distance found var pickFrom []tailcfg.NodeView - for _, candidate := range candidateDistances { - if candidate.nv.Valid() && candidate.distance <= minDistance+minDistanceEpsilon { + for _, candidate := range distances { + if candidate.nv.Valid() && candidate.distance <= minDistance+allowanceMeters { pickFrom = append(pickFrom, candidate.nv) } } @@ -6482,7 +6492,10 @@ func pickDERPNode(nodes []tailcfg.NodeView, r *rand.Rand) tailcfg.NodeView { // If there are no latency values, it returns 0. func minLatencyDERPRegion(regions []int, report *netcheck.Report) int { return slices.MinFunc(regions, func(i, j int) int { - return cmp.Compare(report.RegionLatency[i], report.RegionLatency[j]) + if c := cmp.Compare(report.RegionLatency[i], report.RegionLatency[j]); c != 0 { + return c + } + return cmp.Compare(i, j) }) } diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index 2f9f3cf4ee2de..aa07b22c5a83e 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -3197,8 +3197,20 @@ func TestSuggestExitNode(t *testing.T) { t.Run(tt.name, func(t *testing.T) { r := rand.New(rand.NewSource(100)) got, err := suggestExitNode(&tt.lastReport, &tt.netMap, r) - if !reflect.DeepEqual(got.Name, tt.wantName) || !reflect.DeepEqual(got.ID, tt.wantID) || !errors.Is(err, tt.wantError) || !reflect.DeepEqual(got.Location, tt.wantLocation) { - t.Errorf("got name %v id %v location %v error %v want name %v id %v location %v error %v", got.Name, got.ID, got.Location, err, tt.wantName, tt.wantID, tt.wantLocation, tt.wantError) + if got.Name != tt.wantName { + t.Errorf("name=%v, want %v", got.Name, tt.wantName) + } + if got.ID != tt.wantID { + t.Errorf("ID=%v, want %v", got.ID, tt.wantID) + } + if tt.wantError == nil && err != nil { + t.Errorf("err=%v, want no error", err) + } + if tt.wantError != nil && !errors.Is(err, tt.wantError) { + t.Errorf("err=%v, want %v", err, tt.wantError) + } + if !reflect.DeepEqual(got.Location, tt.wantLocation) { + t.Errorf("location=%v, want %v", got.Location, tt.wantLocation) } }) } @@ -3331,7 +3343,7 @@ func TestSuggestExitNodeLongLatDistance(t *testing.T) { fromLong: -73.935242, toLat: 37.7749, toLong: -122.4194, - distanceValue: 4132000, + distanceValue: 4134926, }, { name: "valid values, locations in north and south of equator", @@ -3343,11 +3355,11 @@ func TestSuggestExitNodeLongLatDistance(t *testing.T) { }, } // Check if the absolute value of the difference between the calculated distance value - // and the hardcoded distance value is greater than 100km. + // and the hardcoded distance value is greater than 1km. for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { got := longLatDistance(tt.fromLat, tt.fromLong, tt.toLat, tt.toLong) - if math.Abs(got-tt.distanceValue) > 100000 { + if math.Abs(got-tt.distanceValue) > 1000 { t.Errorf("got value %v distance value %v absolute value difference %v", got, tt.distanceValue, math.Abs(got-tt.distanceValue)) } })