net/netns, net/dns/resolver, etc: make netmon required in most places

The goal is to move more network state accessors to netmon.Monitor
where they can be cheaper/cached. But first (this change and others)
we need to make sure the one netmon.Monitor is plumbed everywhere.

Some notable bits:

* tsdial.NewDialer is added, taking a now-required netmon

* because a tsdial.Dialer always has a netmon, anything taking both
  a Dialer and a NetMon is now redundant; take only the Dialer and
  get the NetMon from that if/when needed.

* netmon.NewStatic is added, primarily for tests

Updates tailscale/corp#10910
Updates tailscale/corp#18960
Updates #7967
Updates #3299

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

net/dns/manager.go

@@ -55,15 +55,17 @@ type Manager struct {
 }
 
 // NewManagers created a new manager from the given config.
-// The netMon parameter is optional; if non-nil it's used to do faster interface lookups.
-func NewManager(logf logger.Logf, oscfg OSConfigurator, netMon *netmon.Monitor, health *health.Tracker, dialer *tsdial.Dialer, linkSel resolver.ForwardLinkSelector, knobs *controlknobs.Knobs) *Manager {
+func NewManager(logf logger.Logf, oscfg OSConfigurator, health *health.Tracker, dialer *tsdial.Dialer, linkSel resolver.ForwardLinkSelector, knobs *controlknobs.Knobs) *Manager {
 	if dialer == nil {
 		panic("nil Dialer")
 	}
+	if dialer.NetMon() == nil {
+		panic("Dialer has nil NetMon")
+	}
 	logf = logger.WithPrefix(logf, "dns: ")
 	m := &Manager{
 		logf:     logf,
-		resolver: resolver.New(logf, netMon, linkSel, dialer, knobs),
+		resolver: resolver.New(logf, linkSel, dialer, knobs),
 		os:       oscfg,
 		health:   health,
 	}
@@ -454,13 +456,15 @@ func (m *Manager) FlushCaches() error {
 // CleanUp restores the system DNS configuration to its original state
 // in case the Tailscale daemon terminated without closing the router.
 // No other state needs to be instantiated before this runs.
-func CleanUp(logf logger.Logf, interfaceName string) {
+func CleanUp(logf logger.Logf, netMon *netmon.Monitor, interfaceName string) {
 	oscfg, err := NewOSConfigurator(logf, nil, interfaceName)
 	if err != nil {
 		logf("creating dns cleanup: %v", err)
 		return
 	}
-	dns := NewManager(logf, oscfg, nil, nil, &tsdial.Dialer{Logf: logf}, nil, nil)
+	d := &tsdial.Dialer{Logf: logf}
+	d.SetNetMon(netMon)
+	dns := NewManager(logf, oscfg, nil, d, nil, nil)
 	if err := dns.Down(); err != nil {
 		logf("dns down: %v", err)
 	}

net/dns/manager_tcp_test.go

@@ -15,6 +15,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	dns "golang.org/x/net/dns/dnsmessage"
+	"tailscale.com/net/netmon"
 	"tailscale.com/net/tsdial"
 	"tailscale.com/tstest"
 	"tailscale.com/util/dnsname"
@@ -87,7 +88,7 @@ func TestDNSOverTCP(t *testing.T) {
 			SearchDomains: fqdns("coffee.shop"),
 		},
 	}
-	m := NewManager(t.Logf, &f, nil, nil, new(tsdial.Dialer), nil, nil)
+	m := NewManager(t.Logf, &f, nil, tsdial.NewDialer(netmon.NewStatic()), nil, nil)
 	m.resolver.TestOnlySetHook(f.SetResolver)
 	m.Set(Config{
 		Hosts: hosts(
@@ -172,7 +173,7 @@ func TestDNSOverTCP_TooLarge(t *testing.T) {
 			SearchDomains: fqdns("coffee.shop"),
 		},
 	}
-	m := NewManager(log, &f, nil, nil, new(tsdial.Dialer), nil, nil)
+	m := NewManager(log, &f, nil, tsdial.NewDialer(netmon.NewStatic()), nil, nil)
 	m.resolver.TestOnlySetHook(f.SetResolver)
 	m.Set(Config{
 		Hosts:         hosts("andrew.ts.com.", "1.2.3.4"),

net/dns/manager_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"tailscale.com/net/dns/resolver"
+	"tailscale.com/net/netmon"
 	"tailscale.com/net/tsdial"
 	"tailscale.com/types/dnstype"
 	"tailscale.com/util/dnsname"
@@ -613,7 +614,7 @@ func TestManager(t *testing.T) {
 				SplitDNS:   test.split,
 				BaseConfig: test.bs,
 			}
-			m := NewManager(t.Logf, &f, nil, nil, new(tsdial.Dialer), nil, nil)
+			m := NewManager(t.Logf, &f, nil, tsdial.NewDialer(netmon.NewStatic()), nil, nil)
 			m.resolver.TestOnlySetHook(f.SetResolver)
 
 			if err := m.Set(test.in); err != nil {

net/dns/resolver/forwarder.go

@@ -10,7 +10,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"math/rand"
 	"net"
 	"net/http"
 	"net/netip"
@@ -186,7 +185,7 @@ type resolverAndDelay struct {
 // forwarder forwards DNS packets to a number of upstream nameservers.
 type forwarder struct {
 	logf    logger.Logf
-	netMon  *netmon.Monitor
+	netMon  *netmon.Monitor     // always non-nil
 	linkSel ForwardLinkSelector // TODO(bradfitz): remove this when tsdial.Dialer absorbs it
 	dialer  *tsdial.Dialer
 
@@ -214,11 +213,10 @@ type forwarder struct {
 	cloudHostFallback []resolverAndDelay
 }
 
-func init() {
-	rand.Seed(time.Now().UnixNano())
-}
-
 func newForwarder(logf logger.Logf, netMon *netmon.Monitor, linkSel ForwardLinkSelector, dialer *tsdial.Dialer, knobs *controlknobs.Knobs) *forwarder {
+	if netMon == nil {
+		panic("nil netMon")
+	}
 	f := &forwarder{
 		logf:         logger.WithPrefix(logf, "forward: "),
 		netMon:       netMon,
@@ -410,7 +408,6 @@ func (f *forwarder) getKnownDoHClientForProvider(urlBase string) (c *http.Client
 		SingleHost:             dohURL.Hostname(),
 		SingleHostStaticResult: allIPs,
 		Logf:                   f.logf,
-		NetMon:                 f.netMon,
 	})
 	c = &http.Client{
 		Transport: &http.Transport{
@@ -584,7 +581,7 @@ func (f *forwarder) send(ctx context.Context, fq *forwardQuery, rr resolverAndDe
 	}
 
 	// Kick off the race between the UDP and TCP queries.
-	rh := race.New[[]byte](timeout, firstUDP, thenTCP)
+	rh := race.New(timeout, firstUDP, thenTCP)
 	resp, err := rh.Start(ctx)
 	if err == nil {
 		return resp, nil

net/dns/resolver/tsdns.go

@@ -181,7 +181,7 @@ func WriteRoutes(w *bufio.Writer, routes map[dnsname.FQDN][]*dnstype.Resolver) {
 // it delegates to upstream nameservers if any are set.
 type Resolver struct {
 	logf               logger.Logf
-	netMon             *netmon.Monitor  // or nil
+	netMon             *netmon.Monitor  // non-nil
 	dialer             *tsdial.Dialer   // non-nil
 	saveConfigForTests func(cfg Config) // used in tests to capture resolver config
 	// forwarder forwards requests to upstream nameservers.
@@ -205,11 +205,14 @@ type ForwardLinkSelector interface {
 }
 
 // New returns a new resolver.
-// netMon optionally specifies a network monitor to use for socket rebinding.
-func New(logf logger.Logf, netMon *netmon.Monitor, linkSel ForwardLinkSelector, dialer *tsdial.Dialer, knobs *controlknobs.Knobs) *Resolver {
+func New(logf logger.Logf, linkSel ForwardLinkSelector, dialer *tsdial.Dialer, knobs *controlknobs.Knobs) *Resolver {
 	if dialer == nil {
 		panic("nil Dialer")
 	}
+	netMon := dialer.NetMon()
+	if netMon == nil {
+		logf("nil netMon")
+	}
 	r := &Resolver{
 		logf:     logger.WithPrefix(logf, "resolver: "),
 		netMon:   netMon,

net/dns/resolver/tsdns_test.go

@@ -28,6 +28,7 @@ import (
 	"tailscale.com/net/tsdial"
 	"tailscale.com/tstest"
 	"tailscale.com/types/dnstype"
+	"tailscale.com/types/logger"
 	"tailscale.com/util/dnsname"
 )
 
@@ -313,7 +314,11 @@ func TestRDNSNameToIPv6(t *testing.T) {
 }
 
 func newResolver(t testing.TB) *Resolver {
-	return New(t.Logf, nil /* no network monitor */, nil /* no link selector */, new(tsdial.Dialer), nil /* no control knobs */)
+	return New(t.Logf,
+		nil, // no link selector
+		tsdial.NewDialer(netmon.NewStatic()),
+		nil, // no control knobs
+	)
 }
 
 func TestResolveLocal(t *testing.T) {
@@ -1009,7 +1014,13 @@ func TestForwardLinkSelection(t *testing.T) {
 	// routes differently.
 	specialIP := netaddr.IPv4(1, 2, 3, 4)
 
-	fwd := newForwarder(t.Logf, nil, linkSelFunc(func(ip netip.Addr) string {
+	netMon, err := netmon.New(logger.WithPrefix(t.Logf, ".... netmon: "))
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { netMon.Close() })
+
+	fwd := newForwarder(t.Logf, netMon, linkSelFunc(func(ip netip.Addr) string {
 		if ip == netaddr.IPv4(1, 2, 3, 4) {
 			return "special"
 		}

net/dnscache/dnscache.go

@@ -19,7 +19,6 @@ import (
 	"time"
 
 	"tailscale.com/envknob"
-	"tailscale.com/net/netmon"
 	"tailscale.com/types/logger"
 	"tailscale.com/util/cloudenv"
 	"tailscale.com/util/singleflight"
@@ -90,11 +89,6 @@ type Resolver struct {
 	// be added to all log messages printed with this logger.
 	Logf logger.Logf
 
-	// NetMon optionally provides a netmon.Monitor to use to get the current
-	// (cached) network interface.
-	// If nil, the interface will be looked up dynamically.
-	NetMon *netmon.Monitor
-
 	sf singleflight.Group[string, ipRes]
 
 	mu      sync.Mutex

net/netcheck/netcheck.go

@@ -1657,7 +1657,6 @@ func (c *Client) nodeAddr(ctx context.Context, n *tailcfg.DERPNode, proto probeP
 				Forward:     net.DefaultResolver,
 				UseLastGood: true,
 				Logf:        c.logf,
-				NetMon:      c.NetMon,
 			}
 		}
 		resolver := c.resolver

net/netcheck/netcheck_test.go

@@ -24,7 +24,6 @@ import (
 	"tailscale.com/net/stun/stuntest"
 	"tailscale.com/tailcfg"
 	"tailscale.com/tstest"
-	"tailscale.com/types/logger"
 )
 
 func TestHairpinSTUN(t *testing.T) {
@@ -157,14 +156,8 @@ func TestHairpinWait(t *testing.T) {
 }
 
 func newTestClient(t testing.TB) *Client {
-	netMon, err := netmon.New(logger.WithPrefix(t.Logf, "... netmon: "))
-	if err != nil {
-		t.Fatalf("netmon.New: %v", err)
-	}
-	t.Cleanup(func() { netMon.Close() })
-
 	c := &Client{
-		NetMon: netMon,
+		NetMon: netmon.NewStatic(),
 		Logf:   t.Logf,
 	}
 	return c

net/netcheck/standalone.go

@@ -24,12 +24,15 @@ import (
 // to bind, errors will be returned, if one or both protocols can bind no error
 // is returned.
 func (c *Client) Standalone(ctx context.Context, bindAddr string) error {
+	if c.NetMon == nil {
+		panic("netcheck.Client.NetMon must be set")
+	}
 	if bindAddr == "" {
 		bindAddr = ":0"
 	}
 	var errs []error
 
-	u4, err := nettype.MakePacketListenerWithNetIP(netns.Listener(c.logf, nil)).ListenPacket(ctx, "udp4", bindAddr)
+	u4, err := nettype.MakePacketListenerWithNetIP(netns.Listener(c.logf, c.NetMon)).ListenPacket(ctx, "udp4", bindAddr)
 	if err != nil {
 		c.logf("udp4: %v", err)
 		errs = append(errs, err)
@@ -37,7 +40,7 @@ func (c *Client) Standalone(ctx context.Context, bindAddr string) error {
 		go readPackets(ctx, c.logf, u4, c.ReceiveSTUNPacket)
 	}
 
-	u6, err := nettype.MakePacketListenerWithNetIP(netns.Listener(c.logf, nil)).ListenPacket(ctx, "udp6", bindAddr)
+	u6, err := nettype.MakePacketListenerWithNetIP(netns.Listener(c.logf, c.NetMon)).ListenPacket(ctx, "udp6", bindAddr)
 	if err != nil {
 		c.logf("udp6: %v", err)
 		errs = append(errs, err)

net/netmon/netmon.go

@@ -55,6 +55,7 @@ type Monitor struct {
 	om     osMon         // nil means not supported on this platform
 	change chan bool     // send false to wake poller, true to also force ChangeDeltas be sent
 	stop   chan struct{} // closed on Stop
+	static bool          // static Monitor that doesn't actually monitor
 
 	// Things that must be set early, before use,
 	// and not change at runtime.
@@ -139,6 +140,17 @@ func New(logf logger.Logf) (*Monitor, error) {
 	return m, nil
 }
 
+// NewStatic returns a Monitor that's a one-time snapshot of the network state
+// but doesn't actually monitor for changes. It should only be used in tests
+// and situations like cleanups or short-lived CLI programs.
+func NewStatic() *Monitor {
+	m := &Monitor{static: true}
+	if st, err := m.interfaceStateUncached(); err == nil {
+		m.ifState = st
+	}
+	return m
+}
+
 // InterfaceState returns the latest snapshot of the machine's network
 // interfaces.
 //
@@ -168,6 +180,10 @@ func (m *Monitor) SetTailscaleInterfaceName(ifName string) {
 // It's the same as interfaces.LikelyHomeRouterIP, but it caches the
 // result until the monitor detects a network change.
 func (m *Monitor) GatewayAndSelfIP() (gw, myIP netip.Addr, ok bool) {
+	if m.static {
+		return
+	}
+
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	if m.gwValid {
@@ -190,6 +206,9 @@ func (m *Monitor) GatewayAndSelfIP() (gw, myIP netip.Addr, ok bool) {
 // notified (in their own goroutine) when the network state changes.
 // To remove this callback, call unregister (or close the monitor).
 func (m *Monitor) RegisterChangeCallback(callback ChangeFunc) (unregister func()) {
+	if m.static {
+		return func() {}
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	handle := m.cbs.Add(callback)
@@ -210,6 +229,9 @@ type RuleDeleteCallback func(table uint8, priority uint32)
 // notified (in their own goroutine) when a Linux ip rule is deleted.
 // To remove this callback, call unregister (or close the monitor).
 func (m *Monitor) RegisterRuleDeleteCallback(callback RuleDeleteCallback) (unregister func()) {
+	if m.static {
+		return func() {}
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	handle := m.ruleDelCB.Add(callback)
@@ -223,6 +245,9 @@ func (m *Monitor) RegisterRuleDeleteCallback(callback RuleDeleteCallback) (unreg
 // Start starts the monitor.
 // A monitor can only be started & closed once.
 func (m *Monitor) Start() {
+	if m.static {
+		return
+	}
 	m.mu.Lock()
 	defer m.mu.Unlock()
 	if m.started || m.closed {
@@ -244,6 +269,9 @@ func (m *Monitor) Start() {
 
 // Close closes the monitor.
 func (m *Monitor) Close() error {
+	if m.static {
+		return nil
+	}
 	m.mu.Lock()
 	if m.closed {
 		m.mu.Unlock()
@@ -275,6 +303,9 @@ func (m *Monitor) Close() error {
 // ChangeFunc callbacks will be called within the event coalescing
 // period (under a fraction of a second).
 func (m *Monitor) InjectEvent() {
+	if m.static {
+		return
+	}
 	select {
 	case m.change <- true:
 	default:
@@ -290,6 +321,9 @@ func (m *Monitor) InjectEvent() {
 // This is like InjectEvent but only fires ChangeFunc callbacks
 // if the network state differed at all.
 func (m *Monitor) Poll() {
+	if m.static {
+		return
+	}
 	select {
 	case m.change <- false:
 	default:

net/netns/netns.go

@@ -56,8 +56,10 @@ func SetDisableBindConnToInterface(v bool) {
 // Listener returns a new net.Listener with its Control hook func
 // initialized as necessary to run in logical network namespace that
 // doesn't route back into Tailscale.
-// The netMon parameter is optional; if non-nil it's used to do faster interface lookups.
 func Listener(logf logger.Logf, netMon *netmon.Monitor) *net.ListenConfig {
+	if netMon == nil {
+		panic("netns.Listener called with nil netMon")
+	}
 	if disabled.Load() {
 		return new(net.ListenConfig)
 	}
@@ -68,8 +70,10 @@ func Listener(logf logger.Logf, netMon *netmon.Monitor) *net.ListenConfig {
 // hook func initialized as necessary to run in a logical network
 // namespace that doesn't route back into Tailscale. It also handles
 // using a SOCKS if configured in the environment with ALL_PROXY.
-// The netMon parameter is optional; if non-nil it's used to do faster interface lookups.
 func NewDialer(logf logger.Logf, netMon *netmon.Monitor) Dialer {
+	if netMon == nil {
+		panic("netns.NewDialer called with nil netMon")
+	}
 	return FromDialer(logf, netMon, &net.Dialer{
 		KeepAlive: netknob.PlatformTCPKeepAlive(),
 	})
@@ -79,8 +83,10 @@ func NewDialer(logf logger.Logf, netMon *netmon.Monitor) Dialer {
 // network namespace that doesn't route back into Tailscale. It also
 // handles using a SOCKS if configured in the environment with
 // ALL_PROXY.
-// The netMon parameter is optional; if non-nil it's used to do faster interface lookups.
 func FromDialer(logf logger.Logf, netMon *netmon.Monitor, d *net.Dialer) Dialer {
+	if netMon == nil {
+		panic("netns.FromDialer called with nil netMon")
+	}
 	if disabled.Load() {
 		return d
 	}

net/portmapper/igd_test.go

@@ -16,6 +16,7 @@ import (
 
 	"tailscale.com/control/controlknobs"
 	"tailscale.com/net/netaddr"
+	"tailscale.com/net/netmon"
 	"tailscale.com/syncs"
 	"tailscale.com/types/logger"
 )
@@ -259,12 +260,13 @@ 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, new(controlknobs.Knobs), func() {
+	c = NewClient(t.Logf, netmon.NewStatic(), nil, new(controlknobs.Knobs), func() {
 		t.Logf("port map changed")
 		t.Logf("have mapping: %v", c.HaveMapping())
 	})
 	c.testPxPPort = igd.TestPxPPort()
 	c.testUPnPPort = igd.TestUPnPPort()
+	c.netMon = netmon.NewStatic()
 	c.SetGatewayLookupFunc(testIPAndGateway)
 	return c
 }

net/portmapper/portmapper.go

@@ -198,8 +198,7 @@ func (m *pmpMapping) Release(ctx context.Context) {
 
 // NewClient returns a new portmapping client.
 //
-// The netMon parameter is optional; if non-nil it's used to do faster interface
-// lookups.
+// The netMon parameter is required.
 //
 // The debug argument allows configuring the behaviour of the portmapper for
 // debugging; if nil, a sensible set of defaults will be used.
@@ -211,10 +210,13 @@ func (m *pmpMapping) Release(ctx context.Context) {
 // 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 {
+	if netMon == nil {
+		panic("nil netMon")
+	}
 	ret := &Client{
 		logf:         logf,
 		netMon:       netMon,
-		ipAndGateway: interfaces.LikelyHomeRouterIP,
+		ipAndGateway: interfaces.LikelyHomeRouterIP, // TODO(bradfitz): move this to netMon
 		onChange:     onChange,
 		controlKnobs: controlKnobs,
 	}

net/tsdial/tsdial.go

@@ -26,13 +26,27 @@ import (
 	"tailscale.com/types/netmap"
 	"tailscale.com/util/clientmetric"
 	"tailscale.com/util/mak"
+	"tailscale.com/util/testenv"
 	"tailscale.com/version"
 )
 
+// NewDialer returns a new Dialer that can dial out of tailscaled.
+// Its exported fields should be set before use, if any.
+func NewDialer(netMon *netmon.Monitor) *Dialer {
+	if netMon == nil {
+		panic("NewDialer: netMon is nil")
+	}
+	d := &Dialer{}
+	d.SetNetMon(netMon)
+	return d
+}
+
 // Dialer dials out of tailscaled, while taking care of details while
 // handling the dozens of edge cases depending on the server mode
 // (TUN, netstack), the OS network sandboxing style (macOS/iOS
 // Extension, none), user-selected route acceptance prefs, etc.
+//
+// Before use, SetNetMon should be called with a netmon.Monitor.
 type Dialer struct {
 	Logf logger.Logf
 	// UseNetstackForIP if non-nil is whether NetstackDialTCP (if
@@ -130,9 +144,14 @@ func (d *Dialer) Close() error {
 	return nil
 }
 
+// SetNetMon sets d's network monitor to netMon.
+// It is a no-op to call SetNetMon with the same netMon as the current one.
 func (d *Dialer) SetNetMon(netMon *netmon.Monitor) {
 	d.mu.Lock()
 	defer d.mu.Unlock()
+	if d.netMon == netMon {
+		return
+	}
 	if d.netMonUnregister != nil {
 		go d.netMonUnregister()
 		d.netMonUnregister = nil
@@ -141,6 +160,14 @@ func (d *Dialer) SetNetMon(netMon *netmon.Monitor) {
 	d.netMonUnregister = d.netMon.RegisterChangeCallback(d.linkChanged)
 }
 
+// NetMon returns the Dialer's network monitor.
+// It returns nil if SetNetMon has not been called.
+func (d *Dialer) NetMon() *netmon.Monitor {
+	d.mu.Lock()
+	defer d.mu.Unlock()
+	return d.netMon
+}
+
 var (
 	metricLinkChangeConnClosed      = clientmetric.NewCounter("tsdial_linkchange_closes")
 	metricChangeDeltaNoDefaultRoute = clientmetric.NewCounter("tsdial_changedelta_no_default_route")
@@ -314,6 +341,13 @@ func (d *Dialer) logf(format string, args ...any) {
 // Control and (in the future, as of 2022-04-27) DERPs..
 func (d *Dialer) SystemDial(ctx context.Context, network, addr string) (net.Conn, error) {
 	d.mu.Lock()
+	if d.netMon == nil {
+		d.mu.Unlock()
+		if testenv.InTest() {
+			panic("SystemDial requires a netmon.Monitor; call SetNetMon first")
+		}
+		return nil, errors.New("SystemDial requires a netmon.Monitor; call SetNetMon first")
+	}
 	closed := d.closed
 	d.mu.Unlock()
 	if closed {