all: remove some Debug fields, NetworkMap.Debug, Reconfig Debug arg

Updates #8923

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>

control/controlclient/map.go

@@ -14,7 +14,6 @@ import (
 	"tailscale.com/types/key"
 	"tailscale.com/types/logger"
 	"tailscale.com/types/netmap"
-	"tailscale.com/types/opt"
 	"tailscale.com/types/views"
 	"tailscale.com/wgengine/filter"
 )
@@ -145,16 +144,6 @@ func (ms *mapSession) netmapForResponse(resp *tailcfg.MapResponse) *netmap.Netwo
 		ms.lastTKAInfo = resp.TKAInfo
 	}
 
-	debug := resp.Debug
-	if debug != nil {
-		copyDebugOptBools(&ms.stickyDebug, debug)
-	} else if ms.stickyDebug != (tailcfg.Debug{}) {
-		debug = new(tailcfg.Debug)
-	}
-	if debug != nil {
-		copyDebugOptBools(debug, &ms.stickyDebug)
-	}
-
 	nm := &netmap.NetworkMap{
 		NodeKey:           ms.privateNodeKey.Public(),
 		PrivateKey:        ms.privateNodeKey,
@@ -169,7 +158,6 @@ func (ms *mapSession) netmapForResponse(resp *tailcfg.MapResponse) *netmap.Netwo
 		SSHPolicy:         ms.lastSSHPolicy,
 		CollectServices:   ms.collectServices,
 		DERPMap:           ms.lastDERPMap,
-		Debug:             debug,
 		ControlHealth:     ms.lastHealth,
 		TKAEnabled:        ms.lastTKAInfo != nil && !ms.lastTKAInfo.Disabled,
 	}
@@ -401,12 +389,3 @@ func filterSelfAddresses(in []netip.Prefix) (ret []netip.Prefix) {
 		return ret
 	}
 }
-
-func copyDebugOptBools(dst, src *tailcfg.Debug) {
-	copy := func(v *opt.Bool, s opt.Bool) {
-		if s != "" {
-			*v = s
-		}
-	}
-	copy(&dst.OneCGNATRoute, src.OneCGNATRoute)
-}

control/controlclient/map_test.go

@@ -17,7 +17,6 @@ import (
 	"tailscale.com/tstime"
 	"tailscale.com/types/key"
 	"tailscale.com/types/netmap"
-	"tailscale.com/types/opt"
 	"tailscale.com/types/ptr"
 	"tailscale.com/util/must"
 )
@@ -470,85 +469,6 @@ 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 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
-// also accept the new form, keeping the old form in sync.
-func TestDeltaDebug(t *testing.T) {
-	type step struct {
-		got  *tailcfg.Debug
-		want *tailcfg.Debug
-	}
-	tests := []struct {
-		name  string
-		steps []step
-	}{
-		{
-			name: "nothing-to-nothing",
-			steps: []step{
-				{nil, nil},
-				{nil, nil},
-			},
-		},
-		{
-			name: "opt-bool-sticky-changing-over-time",
-			steps: []step{
-				{nil, nil},
-				{nil, nil},
-				{
-					&tailcfg.Debug{OneCGNATRoute: "true"},
-					&tailcfg.Debug{OneCGNATRoute: "true"},
-				},
-				{
-					nil,
-					&tailcfg.Debug{OneCGNATRoute: "true"},
-				},
-				{
-					&tailcfg.Debug{OneCGNATRoute: "false"},
-					&tailcfg.Debug{OneCGNATRoute: "false"},
-				},
-				{
-					nil,
-					&tailcfg.Debug{OneCGNATRoute: "false"},
-				},
-			},
-		},
-	}
-	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{Debug: s.got})
-				if !reflect.DeepEqual(nm.Debug, s.want) {
-					t.Errorf("unexpected result at step index %v; got: %s", stepi, must.Get(json.Marshal(nm.Debug)))
-				}
-			}
-		})
-	}
-}
-
-// Verifies that copyDebugOptBools doesn't missing any opt.Bools.
-func TestCopyDebugOptBools(t *testing.T) {
-	rt := reflect.TypeOf(tailcfg.Debug{})
-	for i := 0; i < rt.NumField(); i++ {
-		sf := rt.Field(i)
-		if sf.Type != reflect.TypeOf(opt.Bool("")) {
-			continue
-		}
-		var src, dst tailcfg.Debug
-		reflect.ValueOf(&src).Elem().Field(i).Set(reflect.ValueOf(opt.Bool("true")))
-		if src == (tailcfg.Debug{}) {
-			t.Fatalf("failed to set field %v", sf.Name)
-		}
-		copyDebugOptBools(&dst, &src)
-		if src != dst {
-			t.Fatalf("copyDebugOptBools didn't copy field %v", sf.Name)
-		}
-	}
-}
-
 func TestDeltaDERPMap(t *testing.T) {
 	regions1 := map[int]*tailcfg.DERPRegion{
 		1: {

ipn/ipnlocal/local.go

@@ -3036,7 +3036,7 @@ func (b *LocalBackend) authReconfig() {
 	rcfg := b.routerConfig(cfg, prefs, oneCGNATRoute)
 	dcfg := dnsConfigForNetmap(nm, prefs, b.logf, version.OS())
 
-	err = b.e.Reconfig(cfg, rcfg, dcfg, nm.Debug)
+	err = b.e.Reconfig(cfg, rcfg, dcfg)
 	if err == wgengine.ErrNoChanges {
 		return
 	}
@@ -3052,16 +3052,6 @@ func (b *LocalBackend) authReconfig() {
 // a runtime.GOOS.
 func shouldUseOneCGNATRoute(nm *netmap.NetworkMap, logf logger.Logf, versionOS string) bool {
 	// Explicit enabling or disabling always take precedence.
-
-	// Old way from control plane, pre capver 71:
-	// TODO(bradfitz): delete this path, once the control server starts
-	// sending it in nodeAttr.
-	if nm.Debug != nil {
-		if v, ok := nm.Debug.OneCGNATRoute.Get(); ok {
-			logf("[v1] shouldUseOneCGNATRoute: explicit=%v", v)
-			return v
-		}
-	}
 	if v, ok := controlclient.ControlOneCGNATSetting().Get(); ok {
 		logf("[v1] shouldUseOneCGNATRoute: explicit=%v", v)
 		return v
@@ -3663,7 +3653,7 @@ func (b *LocalBackend) enterStateLockedOnEntry(newState ipn.State) {
 		b.blockEngineUpdates(true)
 		fallthrough
 	case ipn.Stopped:
-		err := b.e.Reconfig(&wgcfg.Config{}, &router.Config{}, &dns.Config{}, nil)
+		err := b.e.Reconfig(&wgcfg.Config{}, &router.Config{}, &dns.Config{})
 		if err != nil {
 			b.logf("Reconfig(down): %v", err)
 		}
@@ -3805,7 +3795,7 @@ func (b *LocalBackend) stateMachine() {
 // a status update that predates the "I've shut down" update.
 func (b *LocalBackend) stopEngineAndWait() {
 	b.logf("stopEngineAndWait...")
-	b.e.Reconfig(&wgcfg.Config{}, &router.Config{}, &dns.Config{}, nil)
+	b.e.Reconfig(&wgcfg.Config{}, &router.Config{}, &dns.Config{})
 	b.requestEngineStatusAndWait()
 	b.logf("stopEngineAndWait: done.")
 }

tailcfg/tailcfg.go

@@ -1742,15 +1742,10 @@ type ControlIPCandidate struct {
 	Priority int `json:",omitempty"`
 }
 
-// Debug were instructions from the control server to the client to adjust debug
+// Debug used to be a miscellaneous set of declarative debug config changes and
-// settings.
+// imperative debug commands. They've since been mostly migrated to node
-//
+// attributes (MapResponse.Node.Capabilities) for the declarative things and c2n
-// Deprecated: these should no longer be used. Most have been deleted except for some They're a weird mix of declartive
+// requests for the imperative things. Not much remains here. Don't add more.
-// and imperative. The imperative ones should be c2n requests instead, and the
-// declarative ones (at least the bools) should generally be self
-// Node.Capabilities.
-//
-// TODO(bradfitz): start migrating the imperative ones to c2n requests.
 type Debug struct {
 	// SleepSeconds requests that the client sleep for the
 	// provided number of seconds.
@@ -1760,21 +1755,6 @@ type Debug struct {
 	// state machine.
 	SleepSeconds float64 `json:",omitempty"`
 
-	// RandomizeClientPort is whether magicsock should UDP bind to :0 to get a
-	// random local port, ignoring any configured fixed port.
-	//
-	// Deprecated: use NodeAttrRandomizeClientPort instead. This is kept in code
-	// only so the control plane can use it to send to old clients.
-	RandomizeClientPort bool `json:",omitempty"`
-
-	// OneCGNATRoute controls whether the client should prefer to make one big
-	// CGNAT /10 route rather than a /32 per peer.
-	//
-	// Deprecated: use NodeAttrOneCGNATEnable or NodeAttrOneCGNATDisable
-	// instead. This is kept in code only so the control plane can use it to
-	// send to old clients.
-	OneCGNATRoute opt.Bool `json:",omitempty"`
-
 	// DisableLogTail disables the logtail package. Once disabled it can't be
 	// re-enabled for the lifetime of the process.
 	//

types/netmap/netmap.go

@@ -8,7 +8,6 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/netip"
-	"reflect"
 	"strings"
 	"time"
 
@@ -53,9 +52,6 @@ type NetworkMap struct {
 	// between updates and should not be modified.
 	DERPMap *tailcfg.DERPMap
 
-	// Debug knobs from control server for debug or feature gating.
-	Debug *tailcfg.Debug
-
 	// ControlHealth are the list of health check problems for this
 	// node from the perspective of the control plane.
 	// If empty, there are no known problems from the control plane's
@@ -182,10 +178,6 @@ func (nm *NetworkMap) printConciseHeader(buf *strings.Builder) {
 		}
 	}
 	fmt.Fprintf(buf, " u=%s", login)
-	if nm.Debug != nil {
-		j, _ := json.Marshal(nm.Debug)
-		fmt.Fprintf(buf, " debug=%s", j)
-	}
 	fmt.Fprintf(buf, " %v", nm.Addresses)
 	buf.WriteByte('\n')
 }
@@ -204,7 +196,7 @@ func (a *NetworkMap) equalConciseHeader(b *NetworkMap) bool {
 			return false
 		}
 	}
-	return (a.Debug == nil && b.Debug == nil) || reflect.DeepEqual(a.Debug, b.Debug)
+	return true
 }
 
 // printPeerConcise appends to buf a line representing the peer p.

types/netmap/netmap_test.go

@@ -59,22 +59,6 @@ func TestNetworkMapConcise(t *testing.T) {
 			},
 			want: "netmap: self: [AQEBA] auth=machine-unknown u=? []\n [AgICA] D2                 :    192.168.0.100:12     192.168.0.100:12354\n [AwMDA] D4                 :       10.2.0.100:12        10.1.0.100:12345\n",
 		},
-		{
-			name: "debug_non_nil",
-			nm: &NetworkMap{
-				NodeKey: testNodeKey(1),
-				Debug:   &tailcfg.Debug{},
-			},
-			want: "netmap: self: [AQEBA] auth=machine-unknown u=? debug={} []\n",
-		},
-		{
-			name: "debug_values",
-			nm: &NetworkMap{
-				NodeKey: testNodeKey(1),
-				Debug:   &tailcfg.Debug{SleepSeconds: 1.5},
-			},
-			want: "netmap: self: [AQEBA] auth=machine-unknown u=? debug={\"SleepSeconds\":1.5} []\n",
-		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
 			var got string

wgengine/bench/wg.go

@@ -114,7 +114,7 @@ func setupWGTest(b *testing.B, logf logger.Logf, traf *TrafficGen, a1, a2 netip.
 			AllowedIPs: []netip.Prefix{a1},
 		}
 		c2.Peers = []wgcfg.Peer{p}
-		e2.Reconfig(&c2, &router.Config{}, new(dns.Config), nil)
+		e2.Reconfig(&c2, &router.Config{}, new(dns.Config))
 		e1waitDoneOnce.Do(wait.Done)
 	})
 
@@ -151,7 +151,7 @@ func setupWGTest(b *testing.B, logf logger.Logf, traf *TrafficGen, a1, a2 netip.
 			AllowedIPs: []netip.Prefix{a2},
 		}
 		c1.Peers = []wgcfg.Peer{p}
-		e1.Reconfig(&c1, &router.Config{}, new(dns.Config), nil)
+		e1.Reconfig(&c1, &router.Config{}, new(dns.Config))
 		e2waitDoneOnce.Do(wait.Done)
 	})
 

wgengine/magicsock/magicsock.go

@@ -13,7 +13,6 @@ import (
 	"io"
 	"net"
 	"net/netip"
-	"reflect"
 	"runtime"
 	"slices"
 	"strconv"
@@ -1756,17 +1755,12 @@ func (c *Conn) SetNetworkMap(nm *netmap.NetworkMap) {
 	}
 
 	priorNetmap := c.netMap
-	var priorDebug *tailcfg.Debug
-	if priorNetmap != nil {
-		priorDebug = priorNetmap.Debug
-	}
-	debugChanged := !reflect.DeepEqual(priorDebug, nm.Debug)
 	metricNumPeers.Set(int64(len(nm.Peers)))
 
 	// Update c.netMap regardless, before the following early return.
 	c.netMap = nm
 
-	if priorNetmap != nil && nodesEqual(priorNetmap.Peers, nm.Peers) && !debugChanged {
+	if priorNetmap != nil && nodesEqual(priorNetmap.Peers, nm.Peers) {
 		// The rest of this function is all adjusting state for peers that have
 		// changed. But if the set of peers is equal and the debug flags (for
 		// silent disco) haven't changed, no need to do anything else.

wgengine/userspace.go

@@ -766,7 +766,9 @@ func hasOverlap(aips, rips []netip.Prefix) bool {
 	return false
 }
 
-func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, dnsCfg *dns.Config, debug *tailcfg.Debug) error {
+var randomizeClientPort = controlclient.RandomizeClientPort
+
+func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, dnsCfg *dns.Config) error {
 	if routerCfg == nil {
 		panic("routerCfg must not be nil")
 	}
@@ -792,8 +794,7 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config,
 	e.mu.Unlock()
 
 	listenPort := e.confListenPort
-	if controlclient.RandomizeClientPort() || // new way (capver 70)
+	if randomizeClientPort() {
-		debug != nil && debug.RandomizeClientPort { // old way which server might still send (as of 2023-08-16)
 		listenPort = 0
 	}
 

wgengine/userspace_test.go

@@ -121,7 +121,7 @@ func TestUserspaceEngineReconfig(t *testing.T) {
 		}
 
 		e.SetNetworkMap(nm)
-		err = e.Reconfig(cfg, routerCfg, &dns.Config{}, nil)
+		err = e.Reconfig(cfg, routerCfg, &dns.Config{})
 		if err != nil {
 			t.Fatal(err)
 		}
@@ -143,6 +143,8 @@ func TestUserspaceEngineReconfig(t *testing.T) {
 }
 
 func TestUserspaceEnginePortReconfig(t *testing.T) {
+	tstest.Replace(t, &randomizeClientPort, func() bool { return false })
+
 	flakytest.Mark(t, "https://github.com/tailscale/tailscale/issues/2855")
 	const defaultPort = 49983
 	// Keep making a wgengine until we find an unused port
@@ -181,13 +183,15 @@ func TestUserspaceEnginePortReconfig(t *testing.T) {
 		},
 	}
 	routerCfg := &router.Config{}
-	if err := ue.Reconfig(cfg, routerCfg, &dns.Config{}, nil); err != nil {
+	if err := ue.Reconfig(cfg, routerCfg, &dns.Config{}); err != nil {
 		t.Fatal(err)
 	}
 	if got := ue.magicConn.LocalPort(); got != startingPort {
 		t.Errorf("no debug setting changed local port to %d from %d", got, startingPort)
 	}
-	if err := ue.Reconfig(cfg, routerCfg, &dns.Config{}, &tailcfg.Debug{RandomizeClientPort: true}); err != nil {
+
+	randomizeClientPort = func() bool { return true }
+	if err := ue.Reconfig(cfg, routerCfg, &dns.Config{}); err != nil {
 		t.Fatal(err)
 	}
 	if got := ue.magicConn.LocalPort(); got == startingPort {
@@ -195,7 +199,8 @@ func TestUserspaceEnginePortReconfig(t *testing.T) {
 	}
 
 	lastPort := ue.magicConn.LocalPort()
-	if err := ue.Reconfig(cfg, routerCfg, &dns.Config{}, nil); err != nil {
+	randomizeClientPort = func() bool { return false }
+	if err := ue.Reconfig(cfg, routerCfg, &dns.Config{}); err != nil {
 		t.Fatal(err)
 	}
 	if startingPort == defaultPort {

wgengine/watchdog.go

@@ -119,8 +119,8 @@ func (e *watchdogEngine) watchdog(name string, fn func()) {
 	})
 }
 
-func (e *watchdogEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, dnsCfg *dns.Config, debug *tailcfg.Debug) error {
+func (e *watchdogEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, dnsCfg *dns.Config) error {
-	return e.watchdogErr("Reconfig", func() error { return e.wrap.Reconfig(cfg, routerCfg, dnsCfg, debug) })
+	return e.watchdogErr("Reconfig", func() error { return e.wrap.Reconfig(cfg, routerCfg, dnsCfg) })
 }
 func (e *watchdogEngine) GetFilter() *filter.Filter {
 	return e.wrap.GetFilter()

wgengine/wgengine.go

@@ -72,10 +72,8 @@ type Engine interface {
 	// This is called whenever tailcontrol (the control plane)
 	// sends an updated network map.
 	//
-	// The *tailcfg.Debug parameter can be nil.
-	//
 	// The returned error is ErrNoChanges if no changes were made.
-	Reconfig(*wgcfg.Config, *router.Config, *dns.Config, *tailcfg.Debug) error
+	Reconfig(*wgcfg.Config, *router.Config, *dns.Config) error
 
 	// PeerForIP returns the node to which the provided IP routes,
 	// if any. If none is found, (nil, false) is returned.