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 <bradfitz@tailscale.com>

control/controlclient/direct.go

@@ -22,8 +22,10 @@ import (
 	"os"
 	"reflect"
 	"runtime"
+	"slices"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	"go4.org/mem"
@@ -41,14 +43,12 @@ import (
 	"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]
+	controlDisableDRPO         atomic.Bool
-	controlTrimWGConfig syncs.AtomicValue[opt.Bool]
+	controlKeepFullWGConfig    atomic.Bool
+	controlRandomizeClientPort atomic.Bool
 )
 
-// DERPRouteFlag reports the last reported value from control for whether
+// DisableDRPO reports whether control says to disable the
-// DERP route optimization (Issue 150) should be enabled.
+// DERP route optimization (Issue 150).
-func DERPRouteFlag() opt.Bool {
+func DisableDRPO() bool {
-	return controlUseDERPRoute.Load()
+	return controlDisableDRPO.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()
 }
 
-// TrimWGConfig reports the last reported value from control for whether
+// RandomizeClientPort reports whether control says we should randomize
-// we should do lazy wireguard configuration.
+// the client port.
-func TrimWGConfig() opt.Bool {
+func RandomizeClientPort() bool {
-	return controlTrimWGConfig.Load()
+	return controlRandomizeClientPort.Load()
 }
 
 // ipForwardingBroken reports whether the system's IP forwarding is disabled

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)
 }

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
+// session and that should mean no change. (as of capver 37). But one Debug
-// fields existed prior to capver 37 that weren't opt.Bool; we test that we both
+// 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
+// also accept the new form, keeping the old form in sync.
-// sync.
 func TestDeltaDebug(t *testing.T) {
 	type step struct {
 		got  *tailcfg.Debug
@@ -493,44 +492,6 @@ func TestDeltaDebug(t *testing.T) {
 				{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 @@ func TestDeltaDebug(t *testing.T) {
 				},
 			},
 		},
-		{
-			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) {

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 {

tailcfg/tailcfg.go

@@ -107,7 +107,8 @@ type CapabilityVersion int
 //   - 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
+	// Deprecated: use NodeAttrRandomizeClientPort instead.
-	// the wire and RandomizeClientPort is only used within the code as the
+	RandomizeClientPort bool `json:",omitempty"`
-	// current map session value. But RandomizeClientPort can still be used too.
-	SetRandomizeClientPort opt.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 @@ const (
 	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.

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{
+		DNSConfig:       dns,
-			DisableUPnP: "true",
+		ControlTime:     &t,
-		},
-		DNSConfig:   dns,
-		ControlTime: &t,
 	}
 
 	s.mu.Lock()

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) {

wgengine/magicsock/derp.go

@@ -35,15 +35,14 @@ import (
 
 // 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()
+	return !controlclient.DisableDRPO()
-	if v, ok := ob.Get(); ok {
-		return v
-	}
-	return true // as of 1.21.x
 }
 
 // derpRoute is a route entry for a public key, saying that a certain

wgengine/magicsock/magicsock.go

@@ -15,6 +15,7 @@ import (
 	"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
 			}

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 controlclient.KeepFullWGConfig()
-		return !opt.EqualBool(true)
-	}
-	return false
 }
 
 // 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
 	}