diff --git a/control/controlclient/map.go b/control/controlclient/map.go index 07e29956f..eed5694e8 100644 --- a/control/controlclient/map.go +++ b/control/controlclient/map.go @@ -14,7 +14,6 @@ "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) -} diff --git a/control/controlclient/map_test.go b/control/controlclient/map_test.go index bc7c681ad..2880f276c 100644 --- a/control/controlclient/map_test.go +++ b/control/controlclient/map_test.go @@ -17,7 +17,6 @@ "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: { diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index f2bf47eb6..b62326fbb 100644 --- a/ipn/ipnlocal/local.go +++ b/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.") } diff --git a/tailcfg/tailcfg.go b/tailcfg/tailcfg.go index 1fe2a6ebe..09bd37cea 100644 --- a/tailcfg/tailcfg.go +++ b/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 -// settings. -// -// Deprecated: these should no longer be used. Most have been deleted except for some They're a weird mix of declartive -// 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. +// Debug used to be a miscellaneous set of declarative debug config changes and +// imperative debug commands. They've since been mostly migrated to node +// attributes (MapResponse.Node.Capabilities) for the declarative things and c2n +// requests for the imperative things. Not much remains here. Don't add more. 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. // diff --git a/types/netmap/netmap.go b/types/netmap/netmap.go index b3a9df8ba..e521e0064 100644 --- a/types/netmap/netmap.go +++ b/types/netmap/netmap.go @@ -8,7 +8,6 @@ "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. diff --git a/types/netmap/netmap_test.go b/types/netmap/netmap_test.go index 5383ecad5..a8eba7286 100644 --- a/types/netmap/netmap_test.go +++ b/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 diff --git a/wgengine/bench/wg.go b/wgengine/bench/wg.go index dd73d560f..5bd722225 100644 --- a/wgengine/bench/wg.go +++ b/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) }) diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index b9e4c797d..81975fac8 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -13,7 +13,6 @@ "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. diff --git a/wgengine/userspace.go b/wgengine/userspace.go index 56a429ae1..84a3a5b1f 100644 --- a/wgengine/userspace.go +++ b/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) - debug != nil && debug.RandomizeClientPort { // old way which server might still send (as of 2023-08-16) + if randomizeClientPort() { listenPort = 0 } diff --git a/wgengine/userspace_test.go b/wgengine/userspace_test.go index de0555c5a..07bea455b 100644 --- a/wgengine/userspace_test.go +++ b/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 { diff --git a/wgengine/watchdog.go b/wgengine/watchdog.go index 94dcef810..5209566cd 100644 --- a/wgengine/watchdog.go +++ b/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 { - return e.watchdogErr("Reconfig", func() error { return e.wrap.Reconfig(cfg, routerCfg, dnsCfg, debug) }) +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) }) } func (e *watchdogEngine) GetFilter() *filter.Filter { return e.wrap.GetFilter() diff --git a/wgengine/wgengine.go b/wgengine/wgengine.go index b288e658a..3bbb7a895 100644 --- a/wgengine/wgengine.go +++ b/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.