From 25663b130748c70bb60b652bd4b1fc867b5980fc Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Wed, 16 Aug 2023 21:43:56 -0700 Subject: [PATCH] tailcfg: remove most Debug fields, move bulk to nodeAttrs [capver 70] Now a nodeAttr: ForceBackgroundSTUN, DERPRoute, TrimWGConfig, DisableSubnetsIfPAC, DisableUPnP. Kept support for, but also now a NodeAttr: RandomizeClientPort. Removed: SetForceBackgroundSTUN, SetRandomizeClientPort (both never used, sadly... never got around to them. But nodeAttrs are better anyway), EnableSilentDisco (will be a nodeAttr later when that effort resumes). Updates #8923 Signed-off-by: Brad Fitzpatrick --- control/controlclient/direct.go | 49 ++++++----- control/controlclient/map.go | 18 ----- control/controlclient/map_test.go | 76 +---------------- ipn/ipnlocal/local.go | 2 +- tailcfg/tailcfg.go | 81 ++++++++----------- tstest/integration/testcontrol/testcontrol.go | 9 +-- types/netmap/netmap_test.go | 4 +- wgengine/magicsock/derp.go | 9 +-- wgengine/magicsock/magicsock.go | 6 +- wgengine/userspace.go | 8 +- 10 files changed, 82 insertions(+), 180 deletions(-) diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go index 6aa395d7d..d564bcd8b 100644 --- a/control/controlclient/direct.go +++ b/control/controlclient/direct.go @@ -22,8 +22,10 @@ "os" "reflect" "runtime" + "slices" "strings" "sync" + "sync/atomic" "time" "go4.org/mem" @@ -41,14 +43,12 @@ "tailscale.com/net/tlsdial" "tailscale.com/net/tsdial" "tailscale.com/net/tshttpproxy" - "tailscale.com/syncs" "tailscale.com/tailcfg" "tailscale.com/tka" "tailscale.com/tstime" "tailscale.com/types/key" "tailscale.com/types/logger" "tailscale.com/types/netmap" - "tailscale.com/types/opt" "tailscale.com/types/persist" "tailscale.com/types/ptr" "tailscale.com/types/tkatype" @@ -1084,8 +1084,6 @@ func (c *Direct) sendMapRequest(ctx context.Context, isStreaming bool, nu Netmap } hasDebug := resp.Debug != nil - // being conservative here, if Debug not present set to False - controlknobs.SetDisableUPnP(hasDebug && resp.Debug.DisableUPnP.EqualBool(true)) if hasDebug { if code := resp.Debug.Exit; code != nil { c.logf("exiting process with status %v per controlplane", *code) @@ -1102,17 +1100,21 @@ func (c *Direct) sendMapRequest(ctx context.Context, isStreaming bool, nu Netmap } } + // For responses that mutate the self node, check for updated nodeAttrs. + if resp.Node != nil { + caps := resp.Node.Capabilities + controlknobs.SetDisableUPnP(slices.Contains(caps, tailcfg.NodeAttrDisableUPnP)) + controlDisableDRPO.Store(slices.Contains(caps, tailcfg.NodeAttrDebugDisableDRPO)) + controlKeepFullWGConfig.Store(slices.Contains(caps, tailcfg.NodeAttrDebugDisableWGTrim)) + controlRandomizeClientPort.Store(slices.Contains(caps, tailcfg.NodeAttrRandomizeClientPort)) + } + nm := sess.netmapForResponse(&resp) if nm.SelfNode == nil { c.logf("MapResponse lacked node") return errors.New("MapResponse lacked node") } - if d := nm.Debug; d != nil { - controlUseDERPRoute.Store(d.DERPRoute) - controlTrimWGConfig.Store(d.TrimWGConfig) - } - if DevKnob.StripEndpoints() { for _, p := range resp.Peers { p.Endpoints = nil @@ -1315,22 +1317,29 @@ func initDevKnob() devKnobs { var clock tstime.Clock = tstime.StdClock{} -// opt.Bool configs from control. +// config from control. var ( - controlUseDERPRoute syncs.AtomicValue[opt.Bool] - controlTrimWGConfig syncs.AtomicValue[opt.Bool] + controlDisableDRPO atomic.Bool + controlKeepFullWGConfig atomic.Bool + controlRandomizeClientPort atomic.Bool ) -// DERPRouteFlag reports the last reported value from control for whether -// DERP route optimization (Issue 150) should be enabled. -func DERPRouteFlag() opt.Bool { - return controlUseDERPRoute.Load() +// DisableDRPO reports whether control says to disable the +// DERP route optimization (Issue 150). +func DisableDRPO() bool { + return controlDisableDRPO.Load() } -// TrimWGConfig reports the last reported value from control for whether -// we should do lazy wireguard configuration. -func TrimWGConfig() opt.Bool { - return controlTrimWGConfig.Load() +// KeepFullWGConfig reports whether control says we should disable the lazy +// wireguard programming and instead give it the full netmap always. +func KeepFullWGConfig() bool { + return controlKeepFullWGConfig.Load() +} + +// RandomizeClientPort reports whether control says we should randomize +// the client port. +func RandomizeClientPort() bool { + return controlRandomizeClientPort.Load() } // ipForwardingBroken reports whether the system's IP forwarding is disabled diff --git a/control/controlclient/map.go b/control/controlclient/map.go index 714c57149..07e29956f 100644 --- a/control/controlclient/map.go +++ b/control/controlclient/map.go @@ -147,24 +147,12 @@ func (ms *mapSession) netmapForResponse(resp *tailcfg.MapResponse) *netmap.Netwo debug := resp.Debug if debug != nil { - if debug.RandomizeClientPort { - debug.SetRandomizeClientPort.Set(true) - } - if debug.ForceBackgroundSTUN { - debug.SetForceBackgroundSTUN.Set(true) - } copyDebugOptBools(&ms.stickyDebug, debug) } else if ms.stickyDebug != (tailcfg.Debug{}) { debug = new(tailcfg.Debug) } if debug != nil { copyDebugOptBools(debug, &ms.stickyDebug) - if !debug.ForceBackgroundSTUN { - debug.ForceBackgroundSTUN, _ = ms.stickyDebug.SetForceBackgroundSTUN.Get() - } - if !debug.RandomizeClientPort { - debug.RandomizeClientPort, _ = ms.stickyDebug.SetRandomizeClientPort.Get() - } } nm := &netmap.NetworkMap{ @@ -420,11 +408,5 @@ func copyDebugOptBools(dst, src *tailcfg.Debug) { *v = s } } - copy(&dst.DERPRoute, src.DERPRoute) - copy(&dst.DisableSubnetsIfPAC, src.DisableSubnetsIfPAC) - copy(&dst.DisableUPnP, src.DisableUPnP) copy(&dst.OneCGNATRoute, src.OneCGNATRoute) - copy(&dst.SetForceBackgroundSTUN, src.SetForceBackgroundSTUN) - copy(&dst.SetRandomizeClientPort, src.SetRandomizeClientPort) - copy(&dst.TrimWGConfig, src.TrimWGConfig) } diff --git a/control/controlclient/map_test.go b/control/controlclient/map_test.go index cfd243b62..bc7c681ad 100644 --- a/control/controlclient/map_test.go +++ b/control/controlclient/map_test.go @@ -472,11 +472,10 @@ func TestNetmapForResponse(t *testing.T) { // TestDeltaDebug tests that tailcfg.Debug values can be omitted in MapResponses // entirely or have their opt.Bool values unspecified between MapResponses in a -// session and that should mean no change. (as of capver 37). But two Debug -// fields existed prior to capver 37 that weren't opt.Bool; we test that we both +// session and that should mean no change. (as of capver 37). But one Debug +// field existed prior to capver 37 that wasn't opt.Bool; we test that we both // still accept the non-opt.Bool form from control for RandomizeClientPort and -// ForceBackgroundSTUN and also accept the new form, keeping the old form in -// sync. +// also accept the new form, keeping the old form in sync. func TestDeltaDebug(t *testing.T) { type step struct { got *tailcfg.Debug @@ -493,44 +492,6 @@ type step struct { {nil, nil}, }, }, - { - name: "sticky-with-old-style-randomize-client-port", - steps: []step{ - { - &tailcfg.Debug{RandomizeClientPort: true}, - &tailcfg.Debug{ - RandomizeClientPort: true, - SetRandomizeClientPort: "true", - }, - }, - { - nil, // not sent by server - &tailcfg.Debug{ - RandomizeClientPort: true, - SetRandomizeClientPort: "true", - }, - }, - }, - }, - { - name: "sticky-with-new-style-randomize-client-port", - steps: []step{ - { - &tailcfg.Debug{SetRandomizeClientPort: "true"}, - &tailcfg.Debug{ - RandomizeClientPort: true, - SetRandomizeClientPort: "true", - }, - }, - { - nil, // not sent by server - &tailcfg.Debug{ - RandomizeClientPort: true, - SetRandomizeClientPort: "true", - }, - }, - }, - }, { name: "opt-bool-sticky-changing-over-time", steps: []step{ @@ -554,37 +515,6 @@ type step struct { }, }, }, - { - name: "legacy-ForceBackgroundSTUN", - steps: []step{ - { - &tailcfg.Debug{ForceBackgroundSTUN: true}, - &tailcfg.Debug{ForceBackgroundSTUN: true, SetForceBackgroundSTUN: "true"}, - }, - }, - }, - { - name: "opt-bool-SetForceBackgroundSTUN", - steps: []step{ - { - &tailcfg.Debug{SetForceBackgroundSTUN: "true"}, - &tailcfg.Debug{ForceBackgroundSTUN: true, SetForceBackgroundSTUN: "true"}, - }, - }, - }, - { - name: "server-reset-to-default", - steps: []step{ - { - &tailcfg.Debug{SetForceBackgroundSTUN: "true"}, - &tailcfg.Debug{ForceBackgroundSTUN: true, SetForceBackgroundSTUN: "true"}, - }, - { - &tailcfg.Debug{SetForceBackgroundSTUN: "unset"}, - &tailcfg.Debug{ForceBackgroundSTUN: false, SetForceBackgroundSTUN: "unset"}, - }, - }, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index 5dc021ab0..545bb5c48 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -2987,7 +2987,7 @@ func (b *LocalBackend) authReconfig() { prefs := b.pm.CurrentPrefs() nm := b.netMap hasPAC := b.prevIfState.HasPAC() - disableSubnetsIfPAC := nm != nil && nm.Debug != nil && nm.Debug.DisableSubnetsIfPAC.EqualBool(true) + disableSubnetsIfPAC := hasCapability(nm, tailcfg.NodeAttrDisableSubnetsIfPAC) b.mu.Unlock() if blocked { diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index 0c34cb8cf..0d26f5d1e 100644 --- a/tailcfg/tailcfg.go +++ b/tailcfg/tailcfg.go @@ -107,7 +107,8 @@ // - 67: 2023-07-25: Client understands PeerCapMap // - 68: 2023-08-09: Client has dedicated updateRoutine; MapRequest.Stream true means ignore Hostinfo+Endpoints // - 69: 2023-08-16: removed Debug.LogHeap* + GoroutineDumpURL; added c2n /debug/logheap -const CurrentCapabilityVersion CapabilityVersion = 69 +// - 70: 2023-08-16: removed most Debug fields; added NodeAttrDisable*, NodeAttrDebug* instead +const CurrentCapabilityVersion CapabilityVersion = 70 type StableID string @@ -1750,35 +1751,6 @@ type ControlIPCandidate struct { // // TODO(bradfitz): start migrating the imperative ones to c2n requests. type Debug struct { - // ForceBackgroundSTUN controls whether magicsock should - // always do its background STUN queries (see magicsock's - // periodicReSTUN), regardless of inactivity. - ForceBackgroundSTUN bool `json:",omitempty"` - - // SetForceBackgroundSTUN controls whether magicsock should always do its - // background STUN queries (see magicsock's periodicReSTUN), regardless of - // inactivity. - // - // As of capver 37, this field is the preferred field for control to set on - // the wire and ForceBackgroundSTUN is only used within the code as the - // current map session value. But ForceBackgroundSTUN can still be used too. - SetForceBackgroundSTUN opt.Bool `json:",omitempty"` - - // DERPRoute controls whether the DERP reverse path - // optimization (see Issue 150) should be enabled or - // disabled. The environment variable in magicsock is the - // highest priority (if set), then this (if set), then the - // binary default value. - DERPRoute opt.Bool `json:",omitempty"` - - // TrimWGConfig controls whether Tailscale does lazy, on-demand - // wireguard configuration of peers. - TrimWGConfig opt.Bool `json:",omitempty"` - - // DisableSubnetsIfPAC controls whether subnet routers should be - // disabled if WPAD is present on the network. - DisableSubnetsIfPAC opt.Bool `json:",omitempty"` - // SleepSeconds requests that the client sleep for the // provided number of seconds. // The client can (and should) limit the value (such as 5 @@ -1788,35 +1760,18 @@ type Debug struct { // RandomizeClientPort is whether magicsock should UDP bind to // :0 to get a random local port, ignoring any configured // fixed port. - RandomizeClientPort bool `json:",omitempty"` - - // SetRandomizeClientPort is whether magicsock should UDP bind to :0 to get - // a random local port, ignoring any configured fixed port. // - // As of capver 37, this field is the preferred field for control to set on - // the wire and RandomizeClientPort is only used within the code as the - // current map session value. But RandomizeClientPort can still be used too. - SetRandomizeClientPort opt.Bool `json:",omitempty"` + // Deprecated: use NodeAttrRandomizeClientPort instead. + RandomizeClientPort bool `json:",omitempty"` // OneCGNATRoute controls whether the client should prefer to make one // big CGNAT /10 route rather than a /32 per peer. OneCGNATRoute opt.Bool `json:",omitempty"` - // DisableUPnP is whether the client will attempt to perform a UPnP portmapping. - // By default, we want to enable it to see if it works on more clients. - // - // If UPnP catastrophically fails for people, this should be set to True to kill - // new attempts at UPnP connections. - DisableUPnP opt.Bool `json:",omitempty"` - // DisableLogTail disables the logtail package. Once disabled it can't be // re-enabled for the lifetime of the process. DisableLogTail bool `json:",omitempty"` - // EnableSilentDisco disables the use of heartBeatTimer in magicsock and attempts to - // handle disco silently. See issue #540 for details. - EnableSilentDisco bool `json:",omitempty"` - // Exit optionally specifies that the client should os.Exit // with this code. Exit *int `json:",omitempty"` @@ -2003,6 +1958,34 @@ type Oauth2Token struct { NodeAttrFunnel = "funnel" // NodeAttrSSHAggregator grants the ability for a node to collect SSH sessions. NodeAttrSSHAggregator = "ssh-aggregator" + + // NodeAttrDebugForceBackgroundSTUN forces a node to always do background + // STUN queries regardless of inactivity. + NodeAttrDebugForceBackgroundSTUN = "debug-always-stun" + + // NodeAttrDebugDisableWGTrim disables the lazy WireGuard configuration, + // always giving WireGuard the full netmap, even for idle peers. + NodeAttrDebugDisableWGTrim = "debug-no-wg-trim" + + // NodeAttrDebugDisableDRPO disables the DERP Return Path Optimization. + // See Issue 150. + NodeAttrDebugDisableDRPO = "debug-disable-drpo" + + // NodeAttrDisableSubnetsIfPAC controls whether subnet routers should be + // disabled if WPAD is present on the network. + NodeAttrDisableSubnetsIfPAC = "debug-disable-subnets-if-pac" + + // NodeAttrDisableUPnP makes the client not perform a UPnP portmapping. + // By default, we want to enable it to see if it works on more clients. + // + // If UPnP catastrophically fails for people, this should be set kill + // new attempts at UPnP connections. + NodeAttrDisableUPnP = "debug-disable-upnp" + + // NodeAttrRandomizeClientPort makes magicsock UDP bind to + // :0 to get a random local port, ignoring any configured + // fixed port. + NodeAttrRandomizeClientPort = "randomize-client-port" ) // SetDNSRequest is a request to add a DNS record. diff --git a/tstest/integration/testcontrol/testcontrol.go b/tstest/integration/testcontrol/testcontrol.go index d86ceb5c9..96a9d47e9 100644 --- a/tstest/integration/testcontrol/testcontrol.go +++ b/tstest/integration/testcontrol/testcontrol.go @@ -814,6 +814,8 @@ func (s *Server) MapResponse(req *tailcfg.MapRequest) (res *tailcfg.MapResponse, // node key rotated away (once test server supports that) return nil, nil } + node.Capabilities = append(node.Capabilities, tailcfg.NodeAttrDisableUPnP) + user, _ := s.getUser(nk) t := time.Date(2020, 8, 3, 0, 0, 0, 1, time.UTC) dns := s.DNSConfig @@ -830,11 +832,8 @@ func (s *Server) MapResponse(req *tailcfg.MapRequest) (res *tailcfg.MapResponse, Domain: string(user.Domain), CollectServices: "true", PacketFilter: packetFilterWithIngressCaps(), - Debug: &tailcfg.Debug{ - DisableUPnP: "true", - }, - DNSConfig: dns, - ControlTime: &t, + DNSConfig: dns, + ControlTime: &t, } s.mu.Lock() diff --git a/types/netmap/netmap_test.go b/types/netmap/netmap_test.go index 9d2a97e70..5383ecad5 100644 --- a/types/netmap/netmap_test.go +++ b/types/netmap/netmap_test.go @@ -71,9 +71,9 @@ func TestNetworkMapConcise(t *testing.T) { name: "debug_values", nm: &NetworkMap{ NodeKey: testNodeKey(1), - Debug: &tailcfg.Debug{SetForceBackgroundSTUN: "true"}, + Debug: &tailcfg.Debug{SleepSeconds: 1.5}, }, - want: "netmap: self: [AQEBA] auth=machine-unknown u=? debug={\"SetForceBackgroundSTUN\":true} []\n", + want: "netmap: self: [AQEBA] auth=machine-unknown u=? debug={\"SleepSeconds\":1.5} []\n", }, } { t.Run(tt.name, func(t *testing.T) { diff --git a/wgengine/magicsock/derp.go b/wgengine/magicsock/derp.go index 23ae90add..592a56876 100644 --- a/wgengine/magicsock/derp.go +++ b/wgengine/magicsock/derp.go @@ -35,15 +35,14 @@ // useDerpRoute reports whether magicsock should enable the DERP // return path optimization (Issue 150). +// +// By default it's enabled, unless an environment variable +// or control says to disable it. func useDerpRoute() bool { if b, ok := debugUseDerpRoute().Get(); ok { return b } - ob := controlclient.DERPRouteFlag() - if v, ok := ob.Get(); ok { - return v - } - return true // as of 1.21.x + return !controlclient.DisableDRPO() } // derpRoute is a route entry for a public key, saying that a certain diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 33f20154d..b9e4c797d 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -15,6 +15,7 @@ "net/netip" "reflect" "runtime" + "slices" "strconv" "strings" "sync" @@ -1773,7 +1774,7 @@ func (c *Conn) SetNetworkMap(nm *netmap.NetworkMap) { } c.logf("[v1] magicsock: got updated network map; %d peers", len(nm.Peers)) - heartbeatDisabled := debugEnableSilentDisco() || (c.netMap != nil && c.netMap.Debug != nil && c.netMap.Debug.EnableSilentDisco) + heartbeatDisabled := debugEnableSilentDisco() // Set a maximum size for our set of endpoint ring buffers by assuming // that a single large update is ~500 bytes, and that we want to not @@ -2096,7 +2097,8 @@ func (c *Conn) shouldDoPeriodicReSTUNLocked() bool { c.logf("magicsock: periodicReSTUN: idle for %v", idleFor.Round(time.Second)) } if idleFor > sessionActiveTimeout { - if c.netMap != nil && c.netMap.Debug != nil && c.netMap.Debug.ForceBackgroundSTUN { + if c.netMap != nil && c.netMap.SelfNode != nil && + slices.Contains(c.netMap.SelfNode.Capabilities, tailcfg.NodeAttrDebugForceBackgroundSTUN) { // Overridden by control. return true } diff --git a/wgengine/userspace.go b/wgengine/userspace.go index 83f571b98..56a429ae1 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -497,10 +497,7 @@ func forceFullWireguardConfig(numPeers int) bool { if b, ok := debugTrimWireguard().Get(); ok { return !b } - if opt := controlclient.TrimWGConfig(); opt != "" { - return !opt.EqualBool(true) - } - return false + return controlclient.KeepFullWGConfig() } // isTrimmablePeer reports whether p is a peer that we can trim out of the @@ -795,7 +792,8 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, e.mu.Unlock() listenPort := e.confListenPort - if debug != nil && debug.RandomizeClientPort { + if controlclient.RandomizeClientPort() || // new way (capver 70) + debug != nil && debug.RandomizeClientPort { // old way which server might still send (as of 2023-08-16) listenPort = 0 }