tailcfg, control/controlclient: make Debug settings sticky in a map session [capver 37]

Fixes #4843

Change-Id: I3accfd91be474ac745cb47f5d6e866c37d5c5d2d
Signed-off-by: Brad Fitzpatrick <[email protected]>

control/controlclient/direct.go

@@ -710,7 +710,7 @@ func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, readOnly bool
 	c.logf("[v1] PollNetMap: stream=%v ep=%v", allowStream, epStrs)
 
 	vlogf := logger.Discard
-	if Debug.NetMap {
+	if DevKnob.DumpNetMaps {
 		// TODO(bradfitz): update this to use "[v2]" prefix perhaps? but we don't
 		// want to upload it always.
 		vlogf = c.logf
@@ -939,8 +939,6 @@ func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, readOnly bool
 			if resp.Debug.GoroutineDumpURL != "" {
 				go dumpGoroutinesToURL(c.httpc, resp.Debug.GoroutineDumpURL)
 			}
-			controlUseDERPRoute.Store(resp.Debug.DERPRoute)
-			controlTrimWGConfig.Store(resp.Debug.TrimWGConfig)
 			if sleep := time.Duration(resp.Debug.SleepSeconds * float64(time.Second)); sleep > 0 {
 				if err := sleepAsRequested(ctx, c.logf, timeoutReset, sleep); err != nil {
 					return err
@@ -954,12 +952,17 @@ func (c *Direct) sendMapRequest(ctx context.Context, maxPolls int, readOnly bool
 			return errors.New("MapResponse lacked node")
 		}
 
-		if Debug.StripEndpoints {
+		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
 			}
 		}
-		if Debug.StripCaps {
+		if DevKnob.StripCaps {
 			nm.SelfNode.Capabilities = nil
 		}
 
@@ -1125,25 +1128,23 @@ func loadServerPubKeys(ctx context.Context, httpc *http.Client, serverURL string
 	return &out, nil
 }
 
-// Debug contains temporary internal-only debug knobs.
+// DevKnob contains temporary internal-only debug knobs.
 // They're unexported to not draw attention to them.
-var Debug = initDebug()
+var DevKnob = initDevKnob()
 
-type debug struct {
-	NetMap         bool
-	ProxyDNS       bool
-	Disco          bool
+type devKnobs struct {
+	DumpNetMaps    bool
+	ForceProxyDNS  bool
 	StripEndpoints bool // strip endpoints from control (only use disco messages)
 	StripCaps      bool // strip all local node's control-provided capabilities
 }
 
-func initDebug() debug {
-	return debug{
-		NetMap:         envknob.Bool("TS_DEBUG_NETMAP"),
-		ProxyDNS:       envknob.Bool("TS_DEBUG_PROXY_DNS"),
+func initDevKnob() devKnobs {
+	return devKnobs{
+		DumpNetMaps:    envknob.Bool("TS_DEBUG_NETMAP"),
+		ForceProxyDNS:  envknob.Bool("TS_DEBUG_PROXY_DNS"),
 		StripEndpoints: envknob.Bool("TS_DEBUG_STRIP_ENDPOINTS"),
 		StripCaps:      envknob.Bool("TS_DEBUG_STRIP_CAPS"),
-		Disco:          envknob.BoolDefaultTrue("TS_DEBUG_USE_DISCO"),
 	}
 }
 

control/controlclient/map.go

@@ -15,6 +15,7 @@ import (
 	"tailscale.com/types/key"
 	"tailscale.com/types/logger"
 	"tailscale.com/types/netmap"
+	"tailscale.com/types/opt"
 	"tailscale.com/wgengine/filter"
 )
 
@@ -46,6 +47,7 @@ type mapSession struct {
 	lastDomain             string
 	lastHealth             []string
 	lastPopBrowserURL      string
+	stickyDebug            tailcfg.Debug // accumulated opt.Bool values
 
 	// netMapBuilding is non-nil during a netmapForResponse call,
 	// containing the value to be returned, once fully populated.
@@ -114,6 +116,28 @@ func (ms *mapSession) netmapForResponse(resp *tailcfg.MapResponse) *netmap.Netwo
 		ms.lastHealth = resp.Health
 	}
 
+	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{
 		NodeKey:         ms.privateNodeKey.Public(),
 		PrivateKey:      ms.privateNodeKey,
@@ -126,7 +150,7 @@ func (ms *mapSession) netmapForResponse(resp *tailcfg.MapResponse) *netmap.Netwo
 		SSHPolicy:       ms.lastSSHPolicy,
 		CollectServices: ms.collectServices,
 		DERPMap:         ms.lastDERPMap,
-		Debug:           resp.Debug,
+		Debug:           debug,
 		ControlHealth:   ms.lastHealth,
 	}
 	ms.netMapBuilding = nm
@@ -166,7 +190,7 @@ func (ms *mapSession) netmapForResponse(resp *tailcfg.MapResponse) *netmap.Netwo
 		}
 		ms.addUserProfile(peer.User)
 	}
-	if Debug.ProxyDNS {
+	if DevKnob.ForceProxyDNS {
 		nm.DNS.Proxied = true
 	}
 	ms.netMapBuilding = nil
@@ -344,3 +368,18 @@ 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.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

@@ -16,6 +16,8 @@ import (
 	"tailscale.com/tailcfg"
 	"tailscale.com/types/key"
 	"tailscale.com/types/netmap"
+	"tailscale.com/types/opt"
+	"tailscale.com/util/must"
 )
 
 func TestUndeltaPeers(t *testing.T) {
@@ -449,3 +451,152 @@ func TestNetmapForResponse(t *testing.T) {
 		}
 	})
 }
+
+// TestDeltaDebug tests that tailcfg.Debug values can be omitted in MapResposnes
+// 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
+// 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.
+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: "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{
+				{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"},
+				},
+			},
+		},
+		{
+			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) {
+			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)
+		}
+	}
+}

ipn/ipnlocal/local.go

@@ -1039,10 +1039,7 @@ func (b *LocalBackend) Start(opts ipn.Options) error {
 		})
 	}
 
-	var discoPublic key.DiscoPublic
-	if controlclient.Debug.Disco {
-		discoPublic = b.e.DiscoPublicKey()
-	}
+	discoPublic := b.e.DiscoPublicKey()
 
 	var err error
 	if persistv == nil {

tailcfg/tailcfg.go

@@ -71,6 +71,7 @@ type CapabilityVersion int
 //	33: 2022-07-20: added MapResponse.PeersChangedPatch (DERPRegion + Endpoints)
 //	34: 2022-08-02: client understands CapabilityFileSharingTarget
 //	36: 2022-08-02: added PeersChangedPatch.{Key,DiscoKey,Online,LastSeen,KeyExpiry,Capabilities}
+//	37: 2022-08-09: added Debug.{SetForceBackgroundSTUN,SetRandomizeClientPort}; Debug are sticky
 const CurrentCapabilityVersion CapabilityVersion = 36
 
 type StableID string
@@ -1335,6 +1336,15 @@ type Debug struct {
 	// 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
@@ -1365,6 +1375,14 @@ type Debug struct {
 	// 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"`
+
 	// 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"`