diff --git a/control/controlclient/map.go b/control/controlclient/map.go index e1161c34a..9dcdc1c2d 100644 --- a/control/controlclient/map.go +++ b/control/controlclient/map.go @@ -90,9 +90,28 @@ func (ms *mapSession) netmapForResponse(resp *tailcfg.MapResponse) *netmap.Netwo ms.lastUserProfile[up.ID] = up } - if resp.DERPMap != nil { + if dm := resp.DERPMap; dm != nil { ms.vlogf("netmap: new map contains DERP map") - ms.lastDERPMap = resp.DERPMap + + // Zero-valued fields in a DERPMap mean that we're not changing + // anything and are using the previous value(s). + if ldm := ms.lastDERPMap; ldm != nil { + if dm.Regions == nil { + dm.Regions = ldm.Regions + dm.OmitDefaultRegions = ldm.OmitDefaultRegions + } + if dm.HomeParams == nil { + dm.HomeParams = ldm.HomeParams + } else if oldhh := ldm.HomeParams; oldhh != nil { + // Propagate sub-fields of HomeParams + hh := dm.HomeParams + if hh.RegionScore == nil { + hh.RegionScore = oldhh.RegionScore + } + } + } + + ms.lastDERPMap = dm } if pf := resp.PacketFilter; pf != nil { diff --git a/control/controlclient/map_test.go b/control/controlclient/map_test.go index fa130c91b..241053b40 100644 --- a/control/controlclient/map_test.go +++ b/control/controlclient/map_test.go @@ -619,3 +619,108 @@ func TestCopyDebugOptBools(t *testing.T) { } } } + +func TestDeltaDERPMap(t *testing.T) { + regions1 := map[int]*tailcfg.DERPRegion{ + 1: { + RegionID: 1, + Nodes: []*tailcfg.DERPNode{{ + Name: "derp1a", + RegionID: 1, + HostName: "derp1a" + tailcfg.DotInvalid, + IPv4: "169.254.169.254", + IPv6: "none", + }}, + }, + } + + // As above, but with a changed IPv4 addr + regions2 := map[int]*tailcfg.DERPRegion{1: regions1[1].Clone()} + regions2[1].Nodes[0].IPv4 = "127.0.0.1" + + type step struct { + got *tailcfg.DERPMap + want *tailcfg.DERPMap + } + tests := []struct { + name string + steps []step + }{ + { + name: "nothing-to-nothing", + steps: []step{ + {nil, nil}, + {nil, nil}, + }, + }, + { + name: "regions-sticky", + steps: []step{ + {&tailcfg.DERPMap{Regions: regions1}, &tailcfg.DERPMap{Regions: regions1}}, + {&tailcfg.DERPMap{}, &tailcfg.DERPMap{Regions: regions1}}, + }, + }, + { + name: "regions-change", + steps: []step{ + {&tailcfg.DERPMap{Regions: regions1}, &tailcfg.DERPMap{Regions: regions1}}, + {&tailcfg.DERPMap{Regions: regions2}, &tailcfg.DERPMap{Regions: regions2}}, + }, + }, + { + name: "home-params", + steps: []step{ + // Send a DERP map + {&tailcfg.DERPMap{Regions: regions1}, &tailcfg.DERPMap{Regions: regions1}}, + // Send home params, want to still have the same regions + { + &tailcfg.DERPMap{HomeParams: &tailcfg.DERPHomeParams{ + RegionScore: map[int]float64{1: 0.5}, + }}, + &tailcfg.DERPMap{Regions: regions1, HomeParams: &tailcfg.DERPHomeParams{ + RegionScore: map[int]float64{1: 0.5}, + }}, + }, + }, + }, + { + name: "home-params-sub-fields", + steps: []step{ + // Send a DERP map with home params + { + &tailcfg.DERPMap{Regions: regions1, HomeParams: &tailcfg.DERPHomeParams{ + RegionScore: map[int]float64{1: 0.5}, + }}, + &tailcfg.DERPMap{Regions: regions1, HomeParams: &tailcfg.DERPHomeParams{ + RegionScore: map[int]float64{1: 0.5}, + }}, + }, + // Sending a struct with a 'HomeParams' field but nil RegionScore doesn't change home params... + { + &tailcfg.DERPMap{HomeParams: &tailcfg.DERPHomeParams{RegionScore: nil}}, + &tailcfg.DERPMap{Regions: regions1, HomeParams: &tailcfg.DERPHomeParams{ + RegionScore: map[int]float64{1: 0.5}, + }}, + }, + // ... but sending one with a non-nil and empty RegionScore field zeroes that out. + { + &tailcfg.DERPMap{HomeParams: &tailcfg.DERPHomeParams{RegionScore: map[int]float64{}}}, + &tailcfg.DERPMap{Regions: regions1, HomeParams: &tailcfg.DERPHomeParams{ + RegionScore: map[int]float64{}, + }}, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ms := newTestMapSession(t) + for stepi, s := range tt.steps { + nm := ms.netmapForResponse(&tailcfg.MapResponse{DERPMap: s.got}) + if !reflect.DeepEqual(nm.DERPMap, s.want) { + t.Errorf("unexpected result at step index %v; got: %s", stepi, must.Get(json.Marshal(nm.DERPMap))) + } + } + }) + } +} diff --git a/net/netcheck/netcheck.go b/net/netcheck/netcheck.go index b0a93454a..207c925c7 100644 --- a/net/netcheck/netcheck.go +++ b/net/netcheck/netcheck.go @@ -41,6 +41,7 @@ import ( "tailscale.com/types/nettype" "tailscale.com/types/opt" "tailscale.com/types/ptr" + "tailscale.com/types/views" "tailscale.com/util/clientmetric" "tailscale.com/util/cmpx" "tailscale.com/util/mak" @@ -1110,7 +1111,7 @@ func (c *Client) finishAndStoreReport(rs *reportState, dm *tailcfg.DERPMap) *Rep report := rs.report.Clone() rs.mu.Unlock() - c.addReportHistoryAndSetPreferredDERP(report) + c.addReportHistoryAndSetPreferredDERP(report, dm.View()) c.logConciseReport(report, dm) return report @@ -1444,7 +1445,7 @@ func (c *Client) timeNow() time.Time { // addReportHistoryAndSetPreferredDERP adds r to the set of recent Reports // and mutates r.PreferredDERP to contain the best recent one. -func (c *Client) addReportHistoryAndSetPreferredDERP(r *Report) { +func (c *Client) addReportHistoryAndSetPreferredDERP(r *Report, dm tailcfg.DERPMapView) { c.mu.Lock() defer c.mu.Unlock() @@ -1476,11 +1477,33 @@ func (c *Client) addReportHistoryAndSetPreferredDERP(r *Report) { } } + // Scale each region's best latency by any provided scores from the + // DERPMap, for use in comparison below. + var scores views.Map[int, float64] + if hp := dm.HomeParams(); hp.Valid() { + scores = hp.RegionScore() + } + for regionID, d := range bestRecent { + if score := scores.Get(regionID); score > 0 { + bestRecent[regionID] = time.Duration(float64(d) * score) + } + } + // Then, pick which currently-alive DERP server from the // current report has the best latency over the past maxAge. - var bestAny time.Duration - var oldRegionCurLatency time.Duration + var ( + bestAny time.Duration // global minimum + oldRegionCurLatency time.Duration // latency of old PreferredDERP + ) for regionID, d := range r.RegionLatency { + // Scale this report's latency by any scores provided by the + // server; we did this for the bestRecent map above, but we + // don't mutate the actual reports in-place (in case scores + // change), so we need to do it here as well. + if score := scores.Get(regionID); score > 0 { + d = time.Duration(float64(d) * score) + } + if regionID == prevDERP { oldRegionCurLatency = d } @@ -1491,13 +1514,11 @@ func (c *Client) addReportHistoryAndSetPreferredDERP(r *Report) { } } - // If we're changing our preferred DERP but the old one's still - // accessible and the new one's not much better, just stick with - // where we are. - if prevDERP != 0 && - r.PreferredDERP != prevDERP && - oldRegionCurLatency != 0 && - bestAny > oldRegionCurLatency/3*2 { + // If we're changing our preferred DERP, the old one's still + // accessible, and the new one's not much better, just stick + // with where we are. + changingPreferred := prevDERP != 0 && r.PreferredDERP != prevDERP + if changingPreferred && oldRegionCurLatency != 0 && bestAny > oldRegionCurLatency/3*2 { r.PreferredDERP = prevDERP } } diff --git a/net/netcheck/netcheck_test.go b/net/netcheck/netcheck_test.go index 1de3693ce..86d435235 100644 --- a/net/netcheck/netcheck_test.go +++ b/net/netcheck/netcheck_test.go @@ -264,6 +264,7 @@ func TestAddReportHistoryAndSetPreferredDERP(t *testing.T) { tests := []struct { name string steps []step + homeParams *tailcfg.DERPHomeParams wantDERP int // want PreferredDERP on final step wantPrevLen int // wanted len(c.prev) }{ @@ -335,6 +336,52 @@ func TestAddReportHistoryAndSetPreferredDERP(t *testing.T) { wantPrevLen: 2, wantDERP: 2, // 2 got fast enough }, + { + name: "derp_home_params", + homeParams: &tailcfg.DERPHomeParams{ + RegionScore: map[int]float64{ + 1: 2.0 / 3, // 66% + }, + }, + steps: []step{ + // We only use a single step here to avoid + // conflating DERP selection as a result of + // weight hints with the "stickiness" check + // that tries to not change the home DERP + // between steps. + {1 * time.Second, report("d1", 10, "d2", 8)}, + }, + wantPrevLen: 1, + wantDERP: 1, // 2 was faster, but not by 50%+ + }, + { + name: "derp_home_params_high_latency", + homeParams: &tailcfg.DERPHomeParams{ + RegionScore: map[int]float64{ + 1: 2.0 / 3, // 66% + }, + }, + steps: []step{ + // See derp_home_params for why this is a single step. + {1 * time.Second, report("d1", 100, "d2", 10)}, + }, + wantPrevLen: 1, + wantDERP: 2, // 2 was faster by more than 50% + }, + { + name: "derp_home_params_invalid", + homeParams: &tailcfg.DERPHomeParams{ + RegionScore: map[int]float64{ + 1: 0.0, + 2: -1.0, + }, + }, + steps: []step{ + {1 * time.Second, report("d1", 4, "d2", 5)}, + }, + wantPrevLen: 1, + wantDERP: 1, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -342,9 +389,10 @@ func TestAddReportHistoryAndSetPreferredDERP(t *testing.T) { c := &Client{ TimeNow: func() time.Time { return fakeTime }, } + dm := &tailcfg.DERPMap{HomeParams: tt.homeParams} for _, s := range tt.steps { fakeTime = fakeTime.Add(s.after) - c.addReportHistoryAndSetPreferredDERP(s.r) + c.addReportHistoryAndSetPreferredDERP(s.r, dm.View()) } lastReport := tt.steps[len(tt.steps)-1].r if got, want := len(c.prev), tt.wantPrevLen; got != want { diff --git a/tailcfg/derpmap.go b/tailcfg/derpmap.go index abc763e47..d95d26d57 100644 --- a/tailcfg/derpmap.go +++ b/tailcfg/derpmap.go @@ -7,6 +7,11 @@ import "sort" // DERPMap describes the set of DERP packet relay servers that are available. type DERPMap struct { + // HomeParams, if non-nil, is a change in home parameters. + // + // The rest of the DEPRMap fields, if zero, means unchanged. + HomeParams *DERPHomeParams `json:",omitempty"` + // Regions is the set of geographic regions running DERP node(s). // // It's keyed by the DERPRegion.RegionID. @@ -16,6 +21,8 @@ type DERPMap struct { // OmitDefaultRegions specifies to not use Tailscale's DERP servers, and only use those // specified in this DERPMap. If there are none set outside of the defaults, this is a noop. + // + // This field is only meaningful if the Regions map is non-nil (indicating a change). OmitDefaultRegions bool `json:"omitDefaultRegions,omitempty"` } @@ -29,6 +36,25 @@ func (m *DERPMap) RegionIDs() []int { return ret } +// DERPHomeParams contains parameters from the server related to selecting a +// DERP home region (sometimes referred to as the "preferred DERP"). +type DERPHomeParams struct { + // RegionScore scales latencies of DERP regions by a given scaling + // factor when determining which region to use as the home + // ("preferred") DERP. Scores in the range (0, 1) will cause this + // region to be proportionally more preferred, and scores in the range + // (1, ∞) will penalize a region. + // + // If a region is not present in this map, it is treated as having a + // score of 1.0. + // + // Scores should not be 0 or negative; such scores will be ignored. + // + // A nil map means no change from the previous value (if any); an empty + // non-nil map can be sent to reset all scores back to 1.0. + RegionScore map[int]float64 `json:",omitempty"` +} + // DERPRegion is a geographic region running DERP relay node(s). // // Client nodes discover which region they're closest to, advertise diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index 3859a3564..441b0d9ab 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -3,7 +3,7 @@ package tailcfg -//go:generate go run tailscale.com/cmd/viewer --type=User,Node,Hostinfo,NetInfo,Login,DNSConfig,RegisterResponse,DERPRegion,DERPMap,DERPNode,SSHRule,SSHAction,SSHPrincipal,ControlDialPlan,Location --clonefunc +//go:generate go run tailscale.com/cmd/viewer --type=User,Node,Hostinfo,NetInfo,Login,DNSConfig,RegisterResponse,DERPHomeParams,DERPRegion,DERPMap,DERPNode,SSHRule,SSHAction,SSHPrincipal,ControlDialPlan,Location --clonefunc import ( "bytes" @@ -101,7 +101,8 @@ type CapabilityVersion int // - 62: 2023-05-05: Client can notify control over noise for SSHEventNotificationRequest recording failure events // - 63: 2023-06-08: Client understands SSHAction.AllowRemotePortForwarding. // - 64: 2023-07-11: Client understands s/CapabilityTailnetLockAlpha/CapabilityTailnetLock -const CurrentCapabilityVersion CapabilityVersion = 64 +// - 65: 2023-07-12: Client understands DERPMap.HomeParams + incremental DERPMap updates with params +const CurrentCapabilityVersion CapabilityVersion = 65 type StableID string diff --git a/tailcfg/tailcfg_clone.go b/tailcfg/tailcfg_clone.go index 24670a5f3..8ab367733 100644 --- a/tailcfg/tailcfg_clone.go +++ b/tailcfg/tailcfg_clone.go @@ -286,6 +286,28 @@ var _RegisterResponseCloneNeedsRegeneration = RegisterResponse(struct { Error string }{}) +// Clone makes a deep copy of DERPHomeParams. +// The result aliases no memory with the original. +func (src *DERPHomeParams) Clone() *DERPHomeParams { + if src == nil { + return nil + } + dst := new(DERPHomeParams) + *dst = *src + if dst.RegionScore != nil { + dst.RegionScore = map[int]float64{} + for k, v := range src.RegionScore { + dst.RegionScore[k] = v + } + } + return dst +} + +// A compilation failure here means this code must be regenerated, with the command at the top of this file. +var _DERPHomeParamsCloneNeedsRegeneration = DERPHomeParams(struct { + RegionScore map[int]float64 +}{}) + // Clone makes a deep copy of DERPRegion. // The result aliases no memory with the original. func (src *DERPRegion) Clone() *DERPRegion { @@ -318,6 +340,7 @@ func (src *DERPMap) Clone() *DERPMap { } dst := new(DERPMap) *dst = *src + dst.HomeParams = src.HomeParams.Clone() if dst.Regions != nil { dst.Regions = map[int]*DERPRegion{} for k, v := range src.Regions { @@ -329,6 +352,7 @@ func (src *DERPMap) Clone() *DERPMap { // A compilation failure here means this code must be regenerated, with the command at the top of this file. var _DERPMapCloneNeedsRegeneration = DERPMap(struct { + HomeParams *DERPHomeParams Regions map[int]*DERPRegion OmitDefaultRegions bool }{}) @@ -484,7 +508,7 @@ var _LocationCloneNeedsRegeneration = Location(struct { // Clone duplicates src into dst and reports whether it succeeded. // To succeed, must be of types <*T, *T> or <*T, **T>, -// where T is one of User,Node,Hostinfo,NetInfo,Login,DNSConfig,RegisterResponse,DERPRegion,DERPMap,DERPNode,SSHRule,SSHAction,SSHPrincipal,ControlDialPlan,Location. +// where T is one of User,Node,Hostinfo,NetInfo,Login,DNSConfig,RegisterResponse,DERPHomeParams,DERPRegion,DERPMap,DERPNode,SSHRule,SSHAction,SSHPrincipal,ControlDialPlan,Location. func Clone(dst, src any) bool { switch src := src.(type) { case *User: @@ -550,6 +574,15 @@ func Clone(dst, src any) bool { *dst = src.Clone() return true } + case *DERPHomeParams: + switch dst := dst.(type) { + case *DERPHomeParams: + *dst = *src.Clone() + return true + case **DERPHomeParams: + *dst = src.Clone() + return true + } case *DERPRegion: switch dst := dst.(type) { case *DERPRegion: diff --git a/tailcfg/tailcfg_view.go b/tailcfg/tailcfg_view.go index 906229d7d..3236ece3a 100644 --- a/tailcfg/tailcfg_view.go +++ b/tailcfg/tailcfg_view.go @@ -20,7 +20,7 @@ import ( "tailscale.com/types/views" ) -//go:generate go run tailscale.com/cmd/cloner -clonefunc=true -type=User,Node,Hostinfo,NetInfo,Login,DNSConfig,RegisterResponse,DERPRegion,DERPMap,DERPNode,SSHRule,SSHAction,SSHPrincipal,ControlDialPlan,Location +//go:generate go run tailscale.com/cmd/cloner -clonefunc=true -type=User,Node,Hostinfo,NetInfo,Login,DNSConfig,RegisterResponse,DERPHomeParams,DERPRegion,DERPMap,DERPNode,SSHRule,SSHAction,SSHPrincipal,ControlDialPlan,Location // View returns a readonly view of User. func (p *User) View() UserView { @@ -633,6 +633,60 @@ var _RegisterResponseViewNeedsRegeneration = RegisterResponse(struct { Error string }{}) +// View returns a readonly view of DERPHomeParams. +func (p *DERPHomeParams) View() DERPHomeParamsView { + return DERPHomeParamsView{ж: p} +} + +// DERPHomeParamsView provides a read-only view over DERPHomeParams. +// +// Its methods should only be called if `Valid()` returns true. +type DERPHomeParamsView struct { + // ж is the underlying mutable value, named with a hard-to-type + // character that looks pointy like a pointer. + // It is named distinctively to make you think of how dangerous it is to escape + // to callers. You must not let callers be able to mutate it. + ж *DERPHomeParams +} + +// Valid reports whether underlying value is non-nil. +func (v DERPHomeParamsView) Valid() bool { return v.ж != nil } + +// AsStruct returns a clone of the underlying value which aliases no memory with +// the original. +func (v DERPHomeParamsView) AsStruct() *DERPHomeParams { + if v.ж == nil { + return nil + } + return v.ж.Clone() +} + +func (v DERPHomeParamsView) MarshalJSON() ([]byte, error) { return json.Marshal(v.ж) } + +func (v *DERPHomeParamsView) UnmarshalJSON(b []byte) error { + if v.ж != nil { + return errors.New("already initialized") + } + if len(b) == 0 { + return nil + } + var x DERPHomeParams + if err := json.Unmarshal(b, &x); err != nil { + return err + } + v.ж = &x + return nil +} + +func (v DERPHomeParamsView) RegionScore() views.Map[int, float64] { + return views.MapOf(v.ж.RegionScore) +} + +// A compilation failure here means this code must be regenerated, with the command at the top of this file. +var _DERPHomeParamsViewNeedsRegeneration = DERPHomeParams(struct { + RegionScore map[int]float64 +}{}) + // View returns a readonly view of DERPRegion. func (p *DERPRegion) View() DERPRegionView { return DERPRegionView{ж: p} @@ -740,6 +794,8 @@ func (v *DERPMapView) UnmarshalJSON(b []byte) error { return nil } +func (v DERPMapView) HomeParams() DERPHomeParamsView { return v.ж.HomeParams.View() } + func (v DERPMapView) Regions() views.MapFn[int, *DERPRegion, DERPRegionView] { return views.MapFnOf(v.ж.Regions, func(t *DERPRegion) DERPRegionView { return t.View() @@ -749,6 +805,7 @@ func (v DERPMapView) OmitDefaultRegions() bool { return v.ж.OmitDefaultRegions // A compilation failure here means this code must be regenerated, with the command at the top of this file. var _DERPMapViewNeedsRegeneration = DERPMap(struct { + HomeParams *DERPHomeParams Regions map[int]*DERPRegion OmitDefaultRegions bool }{})