From 4e91cf20a854d19b4c3602a54297fcd23e88e351 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 11 Sep 2023 12:03:39 -0700 Subject: [PATCH] control/controlknobs, all: add plumbed Knobs type, not global variables Previously two tsnet nodes in the same process couldn't have disjoint sets of controlknob settings from control as both would overwrite each other's global variables. This plumbs a new controlknobs.Knobs type around everywhere and hangs the knobs sent by control on that instead. Updates #9351 Change-Id: I75338646d36813ed971b4ffad6f9a8b41ec91560 Signed-off-by: Brad Fitzpatrick --- cmd/tailscale/cli/netcheck.go | 2 +- cmd/tailscaled/tailscaled.go | 1 + cmd/tsconnect/wasm/wasm_js.go | 1 + control/controlclient/direct.go | 59 +++++++++------------------- control/controlclient/map.go | 9 +++-- control/controlclient/map_test.go | 3 +- control/controlknobs/controlknobs.go | 36 ++++++++++------- ipn/ipnlocal/local.go | 20 +++++++--- ipn/localapi/localapi.go | 2 +- net/portmapper/igd_test.go | 3 +- net/portmapper/portmapper.go | 14 +++++-- net/portmapper/portmapper_test.go | 8 ++-- net/portmapper/upnp.go | 10 +++-- tsd/tsd.go | 7 ++++ tsnet/tsnet.go | 1 + wgengine/magicsock/derp.go | 7 ++-- wgengine/magicsock/magicsock.go | 9 ++++- wgengine/userspace.go | 32 +++++++++------ wgengine/userspace_test.go | 12 +++--- 19 files changed, 136 insertions(+), 100 deletions(-) diff --git a/cmd/tailscale/cli/netcheck.go b/cmd/tailscale/cli/netcheck.go index a61505087..dd7d6a9af 100644 --- a/cmd/tailscale/cli/netcheck.go +++ b/cmd/tailscale/cli/netcheck.go @@ -53,7 +53,7 @@ func runNetcheck(ctx context.Context, args []string) error { return err } c := &netcheck.Client{ - PortMapper: portmapper.NewClient(logf, netMon, nil, nil), + PortMapper: portmapper.NewClient(logf, netMon, nil, nil, nil), UseDNSCache: false, // always resolve, don't cache } if netcheckArgs.verbose { diff --git a/cmd/tailscaled/tailscaled.go b/cmd/tailscaled/tailscaled.go index 9fbbe4869..15611928d 100644 --- a/cmd/tailscaled/tailscaled.go +++ b/cmd/tailscaled/tailscaled.go @@ -607,6 +607,7 @@ func tryEngine(logf logger.Logf, sys *tsd.System, name string) (onlyNetstack boo NetMon: sys.NetMon.Get(), Dialer: sys.Dialer.Get(), SetSubsystem: sys.Set, + ControlKnobs: sys.ControlKnobs(), } onlyNetstack = name == "userspace-networking" diff --git a/cmd/tsconnect/wasm/wasm_js.go b/cmd/tsconnect/wasm/wasm_js.go index 6066612fc..b49b43883 100644 --- a/cmd/tsconnect/wasm/wasm_js.go +++ b/cmd/tsconnect/wasm/wasm_js.go @@ -102,6 +102,7 @@ func newIPN(jsConfig js.Value) map[string]any { eng, err := wgengine.NewUserspaceEngine(logf, wgengine.Config{ Dialer: dialer, SetSubsystem: sys.Set, + ControlKnobs: sys.ControlKnobs(), }) if err != nil { log.Fatal(err) diff --git a/control/controlclient/direct.go b/control/controlclient/direct.go index dfd8efb12..30a81c5de 100644 --- a/control/controlclient/direct.go +++ b/control/controlclient/direct.go @@ -25,7 +25,6 @@ "slices" "strings" "sync" - "sync/atomic" "time" "go4.org/mem" @@ -44,7 +43,6 @@ "tailscale.com/net/tsdial" "tailscale.com/net/tshttpproxy" "tailscale.com/smallzstd" - "tailscale.com/syncs" "tailscale.com/tailcfg" "tailscale.com/tka" "tailscale.com/tstime" @@ -66,7 +64,8 @@ type Direct struct { httpc *http.Client // HTTP client used to talk to tailcontrol dialer *tsdial.Dialer dnsCache *dnscache.Resolver - serverURL string // URL of the tailcontrol server + controlKnobs *controlknobs.Knobs // always non-nil + serverURL string // URL of the tailcontrol server clock tstime.Clock lastPrintMap time.Time logf logger.Logf @@ -129,6 +128,7 @@ type Options struct { OnControlTime func(time.Time) // optional func to notify callers of new time from control Dialer *tsdial.Dialer // non-nil C2NHandler http.Handler // or nil + ControlKnobs *controlknobs.Knobs // or nil to ignore // Observer is called when there's a change in status to report // from the control client. @@ -202,6 +202,9 @@ func NewDirect(opts Options) (*Direct, error) { if opts.GetMachinePrivateKey == nil { return nil, errors.New("controlclient.New: no GetMachinePrivateKey specified") } + if opts.ControlKnobs == nil { + opts.ControlKnobs = &controlknobs.Knobs{} + } opts.ServerURL = strings.TrimRight(opts.ServerURL, "/") serverURL, err := url.Parse(opts.ServerURL) if err != nil { @@ -249,6 +252,7 @@ func NewDirect(opts Options) (*Direct, error) { c := &Direct{ httpc: httpc, + controlKnobs: opts.ControlKnobs, getMachinePrivKey: opts.GetMachinePrivateKey, serverURL: opts.ServerURL, clock: opts.Clock, @@ -946,7 +950,7 @@ func (c *Direct) sendMapRequest(ctx context.Context, isStreaming bool, nu Netmap var mapResIdx int // 0 for first message, then 1+ for deltas - sess := newMapSession(persist.PrivateNodeKey(), nu) + sess := newMapSession(persist.PrivateNodeKey(), nu, c.controlKnobs) defer sess.Close() sess.cancel = cancel sess.logf = c.logf @@ -1287,38 +1291,11 @@ func initDevKnob() devKnobs { var clock tstime.Clock = tstime.StdClock{} -// config from control. -var ( - controlDisableDRPO atomic.Bool - controlKeepFullWGConfig atomic.Bool - controlRandomizeClientPort atomic.Bool - controlOneCGNAT syncs.AtomicValue[opt.Bool] -) - -// DisableDRPO reports whether control says to disable the -// DERP route optimization (Issue 150). -func DisableDRPO() bool { - 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() -} - -// RandomizeClientPort reports whether control says we should randomize -// the client port. -func RandomizeClientPort() bool { - return controlRandomizeClientPort.Load() -} - -// ControlOneCGNATSetting returns control's OneCGNAT setting, if any. -func ControlOneCGNATSetting() opt.Bool { - return controlOneCGNAT.Load() -} - -func setControlKnobsFromNodeAttrs(selfNodeAttrs []string) { +func (ms *mapSession) setControlKnobsFromNodeAttrs(selfNodeAttrs []string) { + k := ms.controlKnobs + if k == nil { + return + } var ( keepFullWG bool disableDRPO bool @@ -1342,11 +1319,11 @@ func setControlKnobsFromNodeAttrs(selfNodeAttrs []string) { oneCGNAT.Set(false) } } - controlKeepFullWGConfig.Store(keepFullWG) - controlDisableDRPO.Store(disableDRPO) - controlknobs.SetDisableUPnP(disableUPnP) - controlRandomizeClientPort.Store(randomizeClientPort) - controlOneCGNAT.Store(oneCGNAT) + k.KeepFullWGConfig.Store(keepFullWG) + k.DisableDRPO.Store(disableDRPO) + k.DisableUPnP.Store(disableUPnP) + k.RandomizeClientPort.Store(randomizeClientPort) + k.OneCGNAT.Store(oneCGNAT) } // ipForwardingBroken reports whether the system's IP forwarding is disabled diff --git a/control/controlclient/map.go b/control/controlclient/map.go index 176f89a53..55eb20b04 100644 --- a/control/controlclient/map.go +++ b/control/controlclient/map.go @@ -15,6 +15,7 @@ "strconv" "sync" + "tailscale.com/control/controlknobs" "tailscale.com/envknob" "tailscale.com/tailcfg" "tailscale.com/tstime" @@ -38,7 +39,8 @@ // one MapRequest). type mapSession struct { // Immutable fields. - nu NetmapUpdater // called on changes (in addition to the optional hooks below) + nu NetmapUpdater // called on changes (in addition to the optional hooks below) + controlKnobs *controlknobs.Knobs // or nil privateNodeKey key.NodePrivate publicNodeKey key.NodePublic logf logger.Logf @@ -94,9 +96,10 @@ type mapSession struct { // Modify its optional fields on the returned value before use. // // It must have its Close method called to release resources. -func newMapSession(privateNodeKey key.NodePrivate, nu NetmapUpdater) *mapSession { +func newMapSession(privateNodeKey key.NodePrivate, nu NetmapUpdater, controlKnobs *controlknobs.Knobs) *mapSession { ms := &mapSession{ nu: nu, + controlKnobs: controlKnobs, privateNodeKey: privateNodeKey, publicNodeKey: privateNodeKey.Public(), lastDNSConfig: new(tailcfg.DNSConfig), @@ -184,7 +187,7 @@ func (ms *mapSession) HandleNonKeepAliveMapResponse(ctx context.Context, resp *t if DevKnob.StripCaps() { resp.Node.Capabilities = nil } - setControlKnobsFromNodeAttrs(resp.Node.Capabilities) + ms.setControlKnobsFromNodeAttrs(resp.Node.Capabilities) } // Call Node.InitDisplayNames on any changed nodes. diff --git a/control/controlclient/map_test.go b/control/controlclient/map_test.go index 1cfd511ff..bddec375f 100644 --- a/control/controlclient/map_test.go +++ b/control/controlclient/map_test.go @@ -16,6 +16,7 @@ "github.com/google/go-cmp/cmp" "go4.org/mem" + "tailscale.com/control/controlknobs" "tailscale.com/tailcfg" "tailscale.com/tstest" "tailscale.com/tstime" @@ -392,7 +393,7 @@ func formatNodes(nodes []*tailcfg.Node) string { } func newTestMapSession(t testing.TB, nu NetmapUpdater) *mapSession { - ms := newMapSession(key.NewNode(), nu) + ms := newMapSession(key.NewNode(), nu, new(controlknobs.Knobs)) t.Cleanup(ms.Close) ms.logf = t.Logf return ms diff --git a/control/controlknobs/controlknobs.go b/control/controlknobs/controlknobs.go index 65492b39e..ead726e90 100644 --- a/control/controlknobs/controlknobs.go +++ b/control/controlknobs/controlknobs.go @@ -8,22 +8,30 @@ import ( "sync/atomic" - "tailscale.com/envknob" + "tailscale.com/syncs" + "tailscale.com/types/opt" ) -// disableUPnP indicates whether to attempt UPnP mapping. -var disableUPnPControl atomic.Bool +// Knobs is the set of knobs that the control plane's coordination server can +// adjust at runtime. +type Knobs struct { + // DisableUPnP indicates whether to attempt UPnP mapping. + DisableUPnP atomic.Bool -var disableUPnpEnv = envknob.RegisterBool("TS_DISABLE_UPNP") + // DisableDRPO is whether control says to disable the + // DERP route optimization (Issue 150). + DisableDRPO atomic.Bool -// DisableUPnP reports the last reported value from control -// whether UPnP portmapping should be disabled. -func DisableUPnP() bool { - return disableUPnPControl.Load() || disableUPnpEnv() -} - -// SetDisableUPnP sets whether control says that UPnP should be -// disabled. -func SetDisableUPnP(v bool) { - disableUPnPControl.Store(v) + // KeepFullWGConfig is whether we should disable the lazy wireguard + // programming and instead give WireGuard the full netmap always, even for + // idle peers. + KeepFullWGConfig atomic.Bool + + // RandomizeClientPort is whether control says we should randomize + // the client port. + RandomizeClientPort atomic.Bool + + // OneCGNAT is whether the the node should make one big CGNAT route + // in the OS rather than one /32 per peer. + OneCGNAT syncs.AtomicValue[opt.Bool] } diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index d3024af81..57d1e66aa 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -34,6 +34,7 @@ "gvisor.dev/gvisor/pkg/tcpip" "tailscale.com/client/tailscale/apitype" "tailscale.com/control/controlclient" + "tailscale.com/control/controlknobs" "tailscale.com/doctor" "tailscale.com/doctor/permissions" "tailscale.com/doctor/routetable" @@ -3084,7 +3085,7 @@ func (b *LocalBackend) authReconfig() { return } - oneCGNATRoute := shouldUseOneCGNATRoute(b.logf, version.OS()) + oneCGNATRoute := shouldUseOneCGNATRoute(b.logf, b.sys.ControlKnobs(), version.OS()) rcfg := b.routerConfig(cfg, prefs, oneCGNATRoute) dcfg := dnsConfigForNetmap(nm, prefs, b.logf, version.OS()) @@ -3102,11 +3103,13 @@ func (b *LocalBackend) authReconfig() { // // The versionOS is a Tailscale-style version ("iOS", "macOS") and not // a runtime.GOOS. -func shouldUseOneCGNATRoute(logf logger.Logf, versionOS string) bool { - // Explicit enabling or disabling always take precedence. - if v, ok := controlclient.ControlOneCGNATSetting().Get(); ok { - logf("[v1] shouldUseOneCGNATRoute: explicit=%v", v) - return v +func shouldUseOneCGNATRoute(logf logger.Logf, controlKnobs *controlknobs.Knobs, versionOS string) bool { + if controlKnobs != nil { + // Explicit enabling or disabling always take precedence. + if v, ok := controlKnobs.OneCGNAT.Load().Get(); ok { + logf("[v1] shouldUseOneCGNATRoute: explicit=%v", v) + return v + } } // Also prefer to do this on the Mac, so that we don't need to constantly @@ -4663,6 +4666,11 @@ func (b *LocalBackend) DebugReSTUN() error { return nil } +// ControlKnobs returns the node's control knobs. +func (b *LocalBackend) ControlKnobs() *controlknobs.Knobs { + return b.sys.ControlKnobs() +} + func (b *LocalBackend) magicConn() (*magicsock.Conn, error) { mc, ok := b.sys.MagicSock.GetOK() if !ok { diff --git a/ipn/localapi/localapi.go b/ipn/localapi/localapi.go index 678e91458..82bb28797 100644 --- a/ipn/localapi/localapi.go +++ b/ipn/localapi/localapi.go @@ -694,7 +694,7 @@ func (h *Handler) serveDebugPortmap(w http.ResponseWriter, r *http.Request) { done := make(chan bool, 1) var c *portmapper.Client - c = portmapper.NewClient(logger.WithPrefix(logf, "portmapper: "), h.netMon, debugKnobs, func() { + c = portmapper.NewClient(logger.WithPrefix(logf, "portmapper: "), h.netMon, debugKnobs, h.b.ControlKnobs(), func() { logf("portmapping changed.") logf("have mapping: %v", c.HaveMapping()) diff --git a/net/portmapper/igd_test.go b/net/portmapper/igd_test.go index 57c0d7aa6..3551a7fa8 100644 --- a/net/portmapper/igd_test.go +++ b/net/portmapper/igd_test.go @@ -14,6 +14,7 @@ "sync/atomic" "testing" + "tailscale.com/control/controlknobs" "tailscale.com/net/netaddr" "tailscale.com/types/logger" ) @@ -249,7 +250,7 @@ func (d *TestIGD) handlePCPQuery(pkt []byte, src netip.AddrPort) { func newTestClient(t *testing.T, igd *TestIGD) *Client { var c *Client - c = NewClient(t.Logf, nil, nil, func() { + c = NewClient(t.Logf, nil, nil, new(controlknobs.Knobs), func() { t.Logf("port map changed") t.Logf("have mapping: %v", c.HaveMapping()) }) diff --git a/net/portmapper/portmapper.go b/net/portmapper/portmapper.go index d934e202b..ee16bfe61 100644 --- a/net/portmapper/portmapper.go +++ b/net/portmapper/portmapper.go @@ -18,6 +18,7 @@ "time" "go4.org/mem" + "tailscale.com/control/controlknobs" "tailscale.com/net/interfaces" "tailscale.com/net/netaddr" "tailscale.com/net/neterror" @@ -66,6 +67,7 @@ type DebugKnobs struct { type Client struct { logf logger.Logf netMon *netmon.Monitor // optional; nil means interfaces will be looked up on-demand + controlKnobs *controlknobs.Knobs ipAndGateway func() (gw, ip netip.Addr, ok bool) onChange func() // or nil debug DebugKnobs @@ -166,15 +168,19 @@ func (m *pmpMapping) Release(ctx context.Context) { // The debug argument allows configuring the behaviour of the portmapper for // debugging; if nil, a sensible set of defaults will be used. // -// The optional onChange argument specifies a func to run in a new -// goroutine whenever the port mapping status has changed. If nil, -// it doesn't make a callback. -func NewClient(logf logger.Logf, netMon *netmon.Monitor, debug *DebugKnobs, onChange func()) *Client { +// The controlKnobs, if non-nil, specifies the control knobs from the control +// plane that might disable portmapping. +// +// The optional onChange argument specifies a func to run in a new goroutine +// whenever the port mapping status has changed. If nil, it doesn't make a +// callback. +func NewClient(logf logger.Logf, netMon *netmon.Monitor, debug *DebugKnobs, controlKnobs *controlknobs.Knobs, onChange func()) *Client { ret := &Client{ logf: logf, netMon: netMon, ipAndGateway: interfaces.LikelyHomeRouterIP, onChange: onChange, + controlKnobs: controlKnobs, } if debug != nil { ret.debug = *debug diff --git a/net/portmapper/portmapper_test.go b/net/portmapper/portmapper_test.go index 15f2849fc..83a2afe66 100644 --- a/net/portmapper/portmapper_test.go +++ b/net/portmapper/portmapper_test.go @@ -10,13 +10,15 @@ "strconv" "testing" "time" + + "tailscale.com/control/controlknobs" ) func TestCreateOrGetMapping(t *testing.T) { if v, _ := strconv.ParseBool(os.Getenv("HIT_NETWORK")); !v { t.Skip("skipping test without HIT_NETWORK=1") } - c := NewClient(t.Logf, nil, nil, nil) + c := NewClient(t.Logf, nil, nil, new(controlknobs.Knobs), nil) defer c.Close() c.SetLocalPort(1234) for i := 0; i < 2; i++ { @@ -32,7 +34,7 @@ func TestClientProbe(t *testing.T) { if v, _ := strconv.ParseBool(os.Getenv("HIT_NETWORK")); !v { t.Skip("skipping test without HIT_NETWORK=1") } - c := NewClient(t.Logf, nil, nil, nil) + c := NewClient(t.Logf, nil, nil, new(controlknobs.Knobs), nil) defer c.Close() for i := 0; i < 3; i++ { if i > 0 { @@ -47,7 +49,7 @@ func TestClientProbeThenMap(t *testing.T) { if v, _ := strconv.ParseBool(os.Getenv("HIT_NETWORK")); !v { t.Skip("skipping test without HIT_NETWORK=1") } - c := NewClient(t.Logf, nil, nil, nil) + c := NewClient(t.Logf, nil, nil, new(controlknobs.Knobs), nil) defer c.Close() c.SetLocalPort(1234) res, err := c.Probe(context.Background()) diff --git a/net/portmapper/upnp.go b/net/portmapper/upnp.go index b85eb488d..54c1c2f1a 100644 --- a/net/portmapper/upnp.go +++ b/net/portmapper/upnp.go @@ -24,7 +24,7 @@ "github.com/tailscale/goupnp" "github.com/tailscale/goupnp/dcps/internetgateway2" - "tailscale.com/control/controlknobs" + "tailscale.com/envknob" "tailscale.com/net/netns" "tailscale.com/types/logger" ) @@ -192,7 +192,7 @@ func addAnyPortMapping( // The provided ctx is not retained in the returned upnpClient, but // its associated HTTP client is (if set via goupnp.WithHTTPClient). func getUPnPClient(ctx context.Context, logf logger.Logf, debug DebugKnobs, gw netip.Addr, meta uPnPDiscoResponse) (client upnpClient, err error) { - if controlknobs.DisableUPnP() || debug.DisableUPnP { + if debug.DisableUPnP { return nil, nil } @@ -270,6 +270,10 @@ func (c *Client) upnpHTTPClientLocked() *http.Client { return c.uPnPHTTPClient } +var ( + disableUPnpEnv = envknob.RegisterBool("TS_DISABLE_UPNP") +) + // getUPnPPortMapping attempts to create a port-mapping over the UPnP protocol. On success, // it will return the externally exposed IP and port. Otherwise, it will return a zeroed IP and // port and an error. @@ -279,7 +283,7 @@ func (c *Client) getUPnPPortMapping( internal netip.AddrPort, prevPort uint16, ) (external netip.AddrPort, ok bool) { - if controlknobs.DisableUPnP() || c.debug.DisableUPnP { + if disableUPnpEnv() || c.debug.DisableUPnP || (c.controlKnobs != nil && c.controlKnobs.DisableUPnP.Load()) { return netip.AddrPort{}, false } diff --git a/tsd/tsd.go b/tsd/tsd.go index ffa245bc9..f0f624f3a 100644 --- a/tsd/tsd.go +++ b/tsd/tsd.go @@ -21,6 +21,7 @@ "fmt" "reflect" + "tailscale.com/control/controlknobs" "tailscale.com/ipn" "tailscale.com/net/dns" "tailscale.com/net/netmon" @@ -42,6 +43,7 @@ type System struct { Router SubSystem[router.Router] Tun SubSystem[*tstun.Wrapper] StateStore SubSystem[ipn.StateStore] + controlKnobs controlknobs.Knobs } // Set is a convenience method to set a subsystem value. @@ -85,6 +87,11 @@ func (s *System) IsNetstack() bool { return name == tstun.FakeTUNName } +// ControlKnobs returns the control knobs for this node. +func (s *System) ControlKnobs() *controlknobs.Knobs { + return &s.controlKnobs +} + // SubSystem represents some subsystem of the Tailscale node daemon. // // A subsystem can be set to a value, and then later retrieved. A subsystem diff --git a/tsnet/tsnet.go b/tsnet/tsnet.go index 94ae47911..fb92b7139 100644 --- a/tsnet/tsnet.go +++ b/tsnet/tsnet.go @@ -501,6 +501,7 @@ func (s *Server) start() (reterr error) { NetMon: s.netMon, Dialer: s.dialer, SetSubsystem: sys.Set, + ControlKnobs: sys.ControlKnobs(), }) if err != nil { return err diff --git a/wgengine/magicsock/derp.go b/wgengine/magicsock/derp.go index 8a0328cc9..fa23d1d55 100644 --- a/wgengine/magicsock/derp.go +++ b/wgengine/magicsock/derp.go @@ -18,7 +18,6 @@ "time" "github.com/tailscale/wireguard-go/conn" - "tailscale.com/control/controlclient" "tailscale.com/derp" "tailscale.com/derp/derphttp" "tailscale.com/health" @@ -38,11 +37,11 @@ // // By default it's enabled, unless an environment variable // or control says to disable it. -func useDerpRoute() bool { +func (c *Conn) useDerpRoute() bool { if b, ok := debugUseDerpRoute().Get(); ok { return b } - return !controlclient.DisableDRPO() + return c.controlKnobs == nil || !c.controlKnobs.DisableDRPO.Load() } // derpRoute is a route entry for a public key, saying that a certain @@ -294,7 +293,7 @@ func (c *Conn) derpWriteChanOfAddr(addr netip.AddrPort, peer key.NodePublic) cha // perhaps peer's home is Frankfurt, but they dialed our home DERP // node in SF to reach us, so we can reply to them using our // SF connection rather than dialing Frankfurt. (Issue 150) - if !peer.IsZero() && useDerpRoute() { + if !peer.IsZero() && c.useDerpRoute() { if r, ok := c.derpRoute[peer]; ok { if ad, ok := c.activeDerp[r.derpID]; ok && ad.c == r.dc { c.setPeerLastDerpLocked(peer, r.derpID, regionID) diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index b82196c9a..f9244e0fb 100644 --- a/wgengine/magicsock/magicsock.go +++ b/wgengine/magicsock/magicsock.go @@ -25,6 +25,7 @@ "golang.org/x/net/ipv4" "golang.org/x/net/ipv6" + "tailscale.com/control/controlknobs" "tailscale.com/disco" "tailscale.com/envknob" "tailscale.com/health" @@ -87,6 +88,7 @@ type Conn struct { testOnlyPacketListener nettype.PacketListener noteRecvActivity func(key.NodePublic) // or nil, see Options.NoteRecvActivity netMon *netmon.Monitor // or nil + controlKnobs *controlknobs.Knobs // or nil // ================================================================ // No locking required to access these fields, either because @@ -340,6 +342,10 @@ type Options struct { // NetMon is the network monitor to use. // With one, the portmapper won't be used. NetMon *netmon.Monitor + + // ControlKnobs are the set of control knobs to use. + // If nil, they're ignored and not updated. + ControlKnobs *controlknobs.Knobs } func (o *Options) logf() logger.Logf { @@ -400,13 +406,14 @@ func newConn() *Conn { func NewConn(opts Options) (*Conn, error) { c := newConn() c.port.Store(uint32(opts.Port)) + c.controlKnobs = opts.ControlKnobs c.logf = opts.logf() c.epFunc = opts.endpointsFunc() c.derpActiveFunc = opts.derpActiveFunc() c.idleFunc = opts.IdleFunc c.testOnlyPacketListener = opts.TestOnlyPacketListener c.noteRecvActivity = opts.NoteRecvActivity - c.portMapper = portmapper.NewClient(logger.WithPrefix(c.logf, "portmapper: "), opts.NetMon, nil, c.onPortMapChanged) + c.portMapper = portmapper.NewClient(logger.WithPrefix(c.logf, "portmapper: "), opts.NetMon, nil, opts.ControlKnobs, c.onPortMapChanged) if opts.NetMon != nil { c.portMapper.SetGatewayLookupFunc(opts.NetMon.GatewayAndSelfIP) } diff --git a/wgengine/userspace.go b/wgengine/userspace.go index 967c6ea3d..62d40d336 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -19,7 +19,7 @@ "github.com/tailscale/wireguard-go/device" "github.com/tailscale/wireguard-go/tun" - "tailscale.com/control/controlclient" + "tailscale.com/control/controlknobs" "tailscale.com/envknob" "tailscale.com/health" "tailscale.com/ipn/ipnstate" @@ -95,9 +95,10 @@ type userspaceEngine struct { dns *dns.Manager magicConn *magicsock.Conn netMon *netmon.Monitor - netMonOwned bool // whether we created netMon (and thus need to close it) - netMonUnregister func() // unsubscribes from changes; used regardless of netMonOwned - birdClient BIRDClient // or nil + netMonOwned bool // whether we created netMon (and thus need to close it) + netMonUnregister func() // unsubscribes from changes; used regardless of netMonOwned + birdClient BIRDClient // or nil + controlKnobs *controlknobs.Knobs // or nil testMaybeReconfigHook func() // for tests; if non-nil, fires if maybeReconfigWireguardLocked called @@ -183,6 +184,11 @@ type Config struct { // If nil, a new Dialer is created Dialer *tsdial.Dialer + // ControlKnobs is the set of control plane-provied knobs + // to use. + // If nil, defaults are used. + ControlKnobs *controlknobs.Knobs + // ListenPort is the port on which the engine will listen. // If zero, a port is automatically selected. ListenPort uint16 @@ -220,6 +226,8 @@ func NewFakeUserspaceEngine(logf logger.Logf, opts ...any) (Engine, error) { conf.ListenPort = uint16(v) case func(any): conf.SetSubsystem = v + case *controlknobs.Knobs: + conf.ControlKnobs = v default: return nil, fmt.Errorf("unknown option type %T", v) } @@ -271,6 +279,7 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error) router: conf.Router, confListenPort: conf.ListenPort, birdClient: conf.BIRDClient, + controlKnobs: conf.ControlKnobs, } if e.birdClient != nil { @@ -326,6 +335,7 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error) IdleFunc: e.tundev.IdleDuration, NoteRecvActivity: e.noteRecvActivity, NetMon: e.netMon, + ControlKnobs: conf.ControlKnobs, } var err error @@ -493,12 +503,12 @@ func (e *userspaceEngine) handleLocalPackets(p *packet.Parsed, t *tstun.Wrapper) // That's sad too. Or we get rid of these knobs (lazy wireguard config has been // stable!) but I'm worried that a future regression would be easier to debug // with these knobs in place. -func forceFullWireguardConfig(numPeers int) bool { +func (e *userspaceEngine) forceFullWireguardConfig(numPeers int) bool { // Did the user explicitly enable trimming via the environment variable knob? if b, ok := debugTrimWireguard().Get(); ok { return !b } - return controlclient.KeepFullWGConfig() + return e.controlKnobs != nil && e.controlKnobs.KeepFullWGConfig.Load() } // isTrimmablePeer reports whether p is a peer that we can trim out of the @@ -508,8 +518,8 @@ func forceFullWireguardConfig(numPeers int) bool { // only non-subnet AllowedIPs (an IPv4 /32 or IPv6 /128), which is the // common case for most peers. Subnet router nodes will just always be // created in the wireguard-go config. -func isTrimmablePeer(p *wgcfg.Peer, numPeers int) bool { - if forceFullWireguardConfig(numPeers) { +func (e *userspaceEngine) isTrimmablePeer(p *wgcfg.Peer, numPeers int) bool { + if e.forceFullWireguardConfig(numPeers) { return false } @@ -625,7 +635,7 @@ func (e *userspaceEngine) maybeReconfigWireguardLocked(discoChanged map[key.Node for i := range full.Peers { p := &full.Peers[i] nk := p.PublicKey - if !isTrimmablePeer(p, len(full.Peers)) { + if !e.isTrimmablePeer(p, len(full.Peers)) { min.Peers = append(min.Peers, *p) if discoChanged[nk] { needRemoveStep = true @@ -766,8 +776,6 @@ func hasOverlap(aips, rips views.Slice[netip.Prefix]) bool { return false } -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") @@ -794,7 +802,7 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, e.mu.Unlock() listenPort := e.confListenPort - if randomizeClientPort() { + if e.controlKnobs != nil && e.controlKnobs.RandomizeClientPort.Load() { listenPort = 0 } diff --git a/wgengine/userspace_test.go b/wgengine/userspace_test.go index 2afbf7a93..c59bc8253 100644 --- a/wgengine/userspace_test.go +++ b/wgengine/userspace_test.go @@ -11,6 +11,7 @@ "go4.org/mem" "tailscale.com/cmd/testwrapper/flakytest" + "tailscale.com/control/controlknobs" "tailscale.com/net/dns" "tailscale.com/net/netaddr" "tailscale.com/net/tstun" @@ -152,15 +153,16 @@ 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 + + var knobs controlknobs.Knobs + // Keep making a wgengine until we find an unused port var ue *userspaceEngine for i := 0; i < 100; i++ { attempt := uint16(defaultPort + i) - e, err := NewFakeUserspaceEngine(t.Logf, attempt) + e, err := NewFakeUserspaceEngine(t.Logf, attempt, &knobs) if err != nil { t.Fatal(err) } @@ -199,7 +201,7 @@ func TestUserspaceEnginePortReconfig(t *testing.T) { t.Errorf("no debug setting changed local port to %d from %d", got, startingPort) } - randomizeClientPort = func() bool { return true } + knobs.RandomizeClientPort.Store(true) if err := ue.Reconfig(cfg, routerCfg, &dns.Config{}); err != nil { t.Fatal(err) } @@ -208,7 +210,7 @@ func TestUserspaceEnginePortReconfig(t *testing.T) { } lastPort := ue.magicConn.LocalPort() - randomizeClientPort = func() bool { return false } + knobs.RandomizeClientPort.Store(false) if err := ue.Reconfig(cfg, routerCfg, &dns.Config{}); err != nil { t.Fatal(err) }