From eb3313e825c2d2e20f4c11bb7168ad72397e3d20 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Fri, 7 Mar 2025 17:12:07 -0700 Subject: [PATCH] tailcfg: add DERPRegion.NoMeasureNoHome, deprecate+document Avoid [cap 115] Fixes tailscale/corp#24697 Change-Id: Ib81994b5ded3dc87a1eef079eb268906a2acb3f8 Signed-off-by: Brad Fitzpatrick --- net/captivedetection/endpoints.go | 2 +- net/netcheck/netcheck.go | 5 ++++- net/netcheck/netcheck_test.go | 7 ++++--- tailcfg/derpmap.go | 28 ++++++++++++++++++++++++---- tailcfg/tailcfg.go | 3 ++- tailcfg/tailcfg_clone.go | 15 ++++++++------- tailcfg/tailcfg_view.go | 28 +++++++++++++++------------- 7 files changed, 58 insertions(+), 30 deletions(-) diff --git a/net/captivedetection/endpoints.go b/net/captivedetection/endpoints.go index 450ed4a1c..57b3e5335 100644 --- a/net/captivedetection/endpoints.go +++ b/net/captivedetection/endpoints.go @@ -89,7 +89,7 @@ func availableEndpoints(derpMap *tailcfg.DERPMap, preferredDERPRegionID int, log // Use the DERP IPs as captive portal detection endpoints. Using IPs is better than hostnames // because they do not depend on DNS resolution. for _, region := range derpMap.Regions { - if region.Avoid { + if region.Avoid || region.NoMeasureNoHome { continue } for _, node := range region.Nodes { diff --git a/net/netcheck/netcheck.go b/net/netcheck/netcheck.go index 107573e5d..a33ca2209 100644 --- a/net/netcheck/netcheck.go +++ b/net/netcheck/netcheck.go @@ -387,6 +387,9 @@ type probe struct { func sortRegions(dm *tailcfg.DERPMap, last *Report, preferredDERP int) (prev []*tailcfg.DERPRegion) { prev = make([]*tailcfg.DERPRegion, 0, len(dm.Regions)) for _, reg := range dm.Regions { + if reg.NoMeasureNoHome { + continue + } // include an otherwise avoid region if it is the current preferred region if reg.Avoid && reg.RegionID != preferredDERP { continue @@ -533,7 +536,7 @@ func makeProbePlanInitial(dm *tailcfg.DERPMap, ifState *netmon.State) (plan prob plan = make(probePlan) for _, reg := range dm.Regions { - if len(reg.Nodes) == 0 { + if reg.NoMeasureNoHome || len(reg.Nodes) == 0 { continue } diff --git a/net/netcheck/netcheck_test.go b/net/netcheck/netcheck_test.go index 88c19623d..3affa614d 100644 --- a/net/netcheck/netcheck_test.go +++ b/net/netcheck/netcheck_test.go @@ -455,7 +455,7 @@ func TestMakeProbePlan(t *testing.T) { basicMap := &tailcfg.DERPMap{ Regions: map[int]*tailcfg.DERPRegion{}, } - for rid := 1; rid <= 5; rid++ { + for rid := 1; rid <= 6; rid++ { var nodes []*tailcfg.DERPNode for nid := 0; nid < rid; nid++ { nodes = append(nodes, &tailcfg.DERPNode{ @@ -467,8 +467,9 @@ func TestMakeProbePlan(t *testing.T) { }) } basicMap.Regions[rid] = &tailcfg.DERPRegion{ - RegionID: rid, - Nodes: nodes, + RegionID: rid, + Nodes: nodes, + NoMeasureNoHome: rid == 6, } } diff --git a/tailcfg/derpmap.go b/tailcfg/derpmap.go index b3e54983f..e05559f3e 100644 --- a/tailcfg/derpmap.go +++ b/tailcfg/derpmap.go @@ -96,12 +96,32 @@ type DERPRegion struct { Latitude float64 `json:",omitempty"` Longitude float64 `json:",omitempty"` - // Avoid is whether the client should avoid picking this as its home - // region. The region should only be used if a peer is there. - // Clients already using this region as their home should migrate - // away to a new region without Avoid set. + // Avoid is whether the client should avoid picking this as its home region. + // The region should only be used if a peer is there. Clients already using + // this region as their home should migrate away to a new region without + // Avoid set. + // + // Deprecated: because of bugs in past implementations combined with unclear + // docs that caused people to think the bugs were intentional, this field is + // deprecated. It was never supposed to cause STUN/DERP measurement probes, + // but due to bugs, it sometimes did. And then some parts of the code began + // to rely on that property. But then we were unable to use this field for + // its original purpose, nor its later imagined purpose, because various + // parts of the codebase thought it meant one thing and others thought it + // meant another. But it did something in the middle instead. So we're retiring + // it. Use NoMeasureNoHome instead. Avoid bool `json:",omitempty"` + // NoMeasureNoHome says that this regions should not be measured for its + // latency distance (STUN, HTTPS, etc) or availability (e.g. captive portal + // checks) and should never be selected as the node's home region. However, + // if a peer declares this region as its home, then this client is allowed + // to connect to it for the purpose of communicating with that peer. + // + // This is what the now deprecated Avoid bool was supposed to mean + // originally but had implementation bugs and documentation omissions. + NoMeasureNoHome bool `json:",omitempty"` + // Nodes are the DERP nodes running in this region, in // priority order for the current client. Client TLS // connections should ideally only go to the first entry diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index b5f49c614..7556ba3d0 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -159,7 +159,8 @@ // - 112: 2025-01-14: Client interprets AllowedIPs of nil as meaning same as Addresses // - 113: 2025-01-20: Client communicates to control whether funnel is enabled by sending Hostinfo.IngressEnabled (#14688) // - 114: 2025-01-30: NodeAttrMaxKeyDuration CapMap defined, clients might use it (no tailscaled code change) (#14829) -const CurrentCapabilityVersion CapabilityVersion = 114 +// - 115: 2025-03-07: Client understands DERPRegion.NoMeasureNoHome. +const CurrentCapabilityVersion CapabilityVersion = 115 // ID is an integer ID for a user, node, or login allocated by the // control plane. diff --git a/tailcfg/tailcfg_clone.go b/tailcfg/tailcfg_clone.go index aeeacebec..da1f4f374 100644 --- a/tailcfg/tailcfg_clone.go +++ b/tailcfg/tailcfg_clone.go @@ -416,13 +416,14 @@ func (src *DERPRegion) Clone() *DERPRegion { // A compilation failure here means this code must be regenerated, with the command at the top of this file. var _DERPRegionCloneNeedsRegeneration = DERPRegion(struct { - RegionID int - RegionCode string - RegionName string - Latitude float64 - Longitude float64 - Avoid bool - Nodes []*DERPNode + RegionID int + RegionCode string + RegionName string + Latitude float64 + Longitude float64 + Avoid bool + NoMeasureNoHome bool + Nodes []*DERPNode }{}) // Clone makes a deep copy of DERPMap. diff --git a/tailcfg/tailcfg_view.go b/tailcfg/tailcfg_view.go index 4b56b8c09..b1aacab23 100644 --- a/tailcfg/tailcfg_view.go +++ b/tailcfg/tailcfg_view.go @@ -880,25 +880,27 @@ func (v *DERPRegionView) UnmarshalJSON(b []byte) error { return nil } -func (v DERPRegionView) RegionID() int { return v.ж.RegionID } -func (v DERPRegionView) RegionCode() string { return v.ж.RegionCode } -func (v DERPRegionView) RegionName() string { return v.ж.RegionName } -func (v DERPRegionView) Latitude() float64 { return v.ж.Latitude } -func (v DERPRegionView) Longitude() float64 { return v.ж.Longitude } -func (v DERPRegionView) Avoid() bool { return v.ж.Avoid } +func (v DERPRegionView) RegionID() int { return v.ж.RegionID } +func (v DERPRegionView) RegionCode() string { return v.ж.RegionCode } +func (v DERPRegionView) RegionName() string { return v.ж.RegionName } +func (v DERPRegionView) Latitude() float64 { return v.ж.Latitude } +func (v DERPRegionView) Longitude() float64 { return v.ж.Longitude } +func (v DERPRegionView) Avoid() bool { return v.ж.Avoid } +func (v DERPRegionView) NoMeasureNoHome() bool { return v.ж.NoMeasureNoHome } func (v DERPRegionView) Nodes() views.SliceView[*DERPNode, DERPNodeView] { return views.SliceOfViews[*DERPNode, DERPNodeView](v.ж.Nodes) } // A compilation failure here means this code must be regenerated, with the command at the top of this file. var _DERPRegionViewNeedsRegeneration = DERPRegion(struct { - RegionID int - RegionCode string - RegionName string - Latitude float64 - Longitude float64 - Avoid bool - Nodes []*DERPNode + RegionID int + RegionCode string + RegionName string + Latitude float64 + Longitude float64 + Avoid bool + NoMeasureNoHome bool + Nodes []*DERPNode }{}) // View returns a read-only view of DERPMap.