net/netcheck, wgengine/magicsock: make netmon.Monitor required

This has been a TODO for ages. Time to do it.

The goal is to move more network state accessors to netmon.Monitor
where they can be cheaper/cached.

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

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

cmd/tailscale/cli/netcheck.go

@@ -53,6 +53,7 @@ func runNetcheck(ctx context.Context, args []string) error {
 		return err
 	}
 	c := &netcheck.Client{
+		NetMon:      netMon,
 		PortMapper:  portmapper.NewClient(logf, netMon, nil, nil, nil),
 		UseDNSCache: false, // always resolve, don't cache
 	}

net/netcheck/netcheck.go

@@ -159,6 +159,11 @@ func cloneDurationMap(m map[int]time.Duration) map[int]time.Duration {
 // active probes, and must receive STUN packet replies via ReceiveSTUNPacket.
 // Client can be used in a standalone fashion via the Standalone method.
 type Client struct {
+	// NetMon is the netmon.Monitor to use to get the current
+	// (cached) network interface.
+	// It must be non-nil.
+	NetMon *netmon.Monitor
+
 	// Verbose enables verbose logging.
 	Verbose bool
 
@@ -166,13 +171,6 @@ type Client struct {
 	// If nil, log.Printf is used.
 	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.
-	// TODO(bradfitz): make NetMon required. As of 2023-08-01, it basically always is
-	// present anyway.
-	NetMon *netmon.Monitor
-
 	// TimeNow, if non-nil, is used instead of time.Now.
 	TimeNow func() time.Time
 
@@ -781,6 +779,9 @@ func (c *Client) GetReport(ctx context.Context, dm *tailcfg.DERPMap, opts *GetRe
 	if dm == nil {
 		return nil, errors.New("netcheck: GetReport: DERP map is nil")
 	}
+	if c.NetMon == nil {
+		return nil, errors.New("netcheck: GetReport: Client.NetMon is nil")
+	}
 
 	c.mu.Lock()
 	if c.curState != nil {
@@ -844,18 +845,7 @@ func (c *Client) GetReport(ctx context.Context, dm *tailcfg.DERPMap, opts *GetRe
 		return c.finishAndStoreReport(rs, dm), nil
 	}
 
-	var ifState *interfaces.State
+	ifState := c.NetMon.InterfaceState()
-	if c.NetMon == nil {
-		directState, err := interfaces.GetState()
-		if err != nil {
-			c.logf("[v1] interfaces: %v", err)
-			return nil, err
-		} else {
-			ifState = directState
-		}
-	} else {
-		ifState = c.NetMon.InterfaceState()
-	}
 
 	// See if IPv6 works at all, or if it's been hard disabled at the
 	// OS level.

net/netcheck/netcheck_test.go

@@ -19,10 +19,12 @@ import (
 	"time"
 
 	"tailscale.com/net/interfaces"
+	"tailscale.com/net/netmon"
 	"tailscale.com/net/stun"
 	"tailscale.com/net/stun/stuntest"
 	"tailscale.com/tailcfg"
 	"tailscale.com/tstest"
+	"tailscale.com/types/logger"
 )
 
 func TestHairpinSTUN(t *testing.T) {
@@ -154,13 +156,25 @@ 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,
+		Logf:   t.Logf,
+	}
+	return c
+}
+
 func TestBasic(t *testing.T) {
 	stunAddr, cleanup := stuntest.Serve(t)
 	defer cleanup()
 
-	c := &Client{
+	c := newTestClient(t)
-		Logf: t.Logf,
-	}
 
 	ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
 	defer cancel()
@@ -202,9 +216,8 @@ func TestWorksWhenUDPBlocked(t *testing.T) {
 	dm := stuntest.DERPMapOf(stunAddr)
 	dm.Regions[1].Nodes[0].STUNOnly = true
 
-	c := &Client{
+	c := newTestClient(t)
-		Logf: t.Logf,
+
-	}
 	ctx, cancel := context.WithTimeout(context.Background(), 250*time.Millisecond)
 	defer cancel()
 
@@ -895,14 +908,11 @@ func TestNoCaptivePortalWhenUDP(t *testing.T) {
 	stunAddr, cleanup := stuntest.Serve(t)
 	defer cleanup()
 
-	c := &Client{
+	c := newTestClient(t)
-		Logf:              t.Logf,
+	c.testEnoughRegions = 1
-		testEnoughRegions: 1,
+	// Set the delay long enough that we have time to cancel it
-
+	// when our STUN probe succeeds.
-		// Set the delay long enough that we have time to cancel it
+	c.testCaptivePortalDelay = 10 * time.Second
-		// when our STUN probe succeeds.
-		testCaptivePortalDelay: 10 * time.Second,
-	}
 
 	ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second)
 	defer cancel()

wgengine/magicsock/magicsock.go

@@ -90,7 +90,7 @@ type Conn struct {
 	idleFunc               func() time.Duration // nil means unknown
 	testOnlyPacketListener nettype.PacketListener
 	noteRecvActivity       func(key.NodePublic) // or nil, see Options.NoteRecvActivity
-	netMon                 *netmon.Monitor      // or nil
+	netMon                 *netmon.Monitor      // must be non-nil
 	health                 *health.Tracker      // or nil
 	controlKnobs           *controlknobs.Knobs  // or nil
 
@@ -370,7 +370,7 @@ type Options struct {
 	NoteRecvActivity func(key.NodePublic)
 
 	// NetMon is the network monitor to use.
-	// If nil, the portmapper won't be used.
+	// It must be non-nil.
 	NetMon *netmon.Monitor
 
 	// HealthTracker optionally specifies the health tracker to
@@ -451,6 +451,10 @@ func newConn() *Conn {
 // As the set of possible endpoints for a Conn changes, the
 // callback opts.EndpointsFunc is called.
 func NewConn(opts Options) (*Conn, error) {
+	if opts.NetMon == nil {
+		return nil, errors.New("magicsock.Options.NetMon must be non-nil")
+	}
+
 	c := newConn()
 	c.port.Store(uint32(opts.Port))
 	c.controlKnobs = opts.ControlKnobs
@@ -464,9 +468,7 @@ func NewConn(opts Options) (*Conn, error) {
 		DisableAll: func() bool { return opts.DisablePortMapper || c.onlyTCP443.Load() },
 	}
 	c.portMapper = portmapper.NewClient(logger.WithPrefix(c.logf, "portmapper: "), opts.NetMon, portMapOpts, opts.ControlKnobs, c.onPortMapChanged)
-	if opts.NetMon != nil {
+	c.portMapper.SetGatewayLookupFunc(opts.NetMon.GatewayAndSelfIP)
-		c.portMapper.SetGatewayLookupFunc(opts.NetMon.GatewayAndSelfIP)
-	}
 	c.netMon = opts.NetMon
 	c.health = opts.HealthTracker
 	c.onPortUpdate = opts.OnPortUpdate

wgengine/magicsock/magicsock_test.go

@@ -47,6 +47,7 @@ import (
 	"tailscale.com/net/connstats"
 	"tailscale.com/net/netaddr"
 	"tailscale.com/net/netcheck"
+	"tailscale.com/net/netmon"
 	"tailscale.com/net/packet"
 	"tailscale.com/net/ping"
 	"tailscale.com/net/stun/stuntest"
@@ -155,6 +156,7 @@ type magicStack struct {
 	tsTun      *tstun.Wrapper          // wrapped tun that implements filtering and wgengine hooks
 	dev        *device.Device          // the wireguard-go Device that connects the previous things
 	wgLogger   *wglog.Logger           // wireguard-go log wrapper
+	netMon     *netmon.Monitor         // always non-nil
 }
 
 // newMagicStack builds and initializes an idle magicsock and
@@ -168,8 +170,14 @@ func newMagicStack(t testing.TB, logf logger.Logf, l nettype.PacketListener, der
 func newMagicStackWithKey(t testing.TB, logf logger.Logf, l nettype.PacketListener, derpMap *tailcfg.DERPMap, privateKey key.NodePrivate) *magicStack {
 	t.Helper()
 
+	netMon, err := netmon.New(logf)
+	if err != nil {
+		t.Fatalf("netmon.New: %v", err)
+	}
+
 	epCh := make(chan []tailcfg.Endpoint, 100) // arbitrary
 	conn, err := NewConn(Options{
+		NetMon:                 netMon,
 		Logf:                   logf,
 		DisablePortMapper:      true,
 		TestOnlyPacketListener: l,
@@ -211,6 +219,7 @@ func newMagicStackWithKey(t testing.TB, logf logger.Logf, l nettype.PacketListen
 		tsTun:      tsTun,
 		dev:        dev,
 		wgLogger:   wgLogger,
+		netMon:     netMon,
 	}
 }
 
@@ -228,6 +237,7 @@ func (s *magicStack) String() string {
 func (s *magicStack) Close() {
 	s.dev.Close()
 	s.conn.Close()
+	s.netMon.Close()
 }
 
 func (s *magicStack) Public() key.NodePublic {
@@ -372,6 +382,12 @@ func TestNewConn(t *testing.T) {
 		}
 	}
 
+	netMon, err := netmon.New(logger.WithPrefix(t.Logf, "... netmon: "))
+	if err != nil {
+		t.Fatalf("netmon.New: %v", err)
+	}
+	defer netMon.Close()
+
 	stunAddr, stunCleanupFn := stuntest.Serve(t)
 	defer stunCleanupFn()
 
@@ -381,6 +397,7 @@ func TestNewConn(t *testing.T) {
 		DisablePortMapper: true,
 		EndpointsFunc:     epFunc,
 		Logf:              t.Logf,
+		NetMon:            netMon,
 	})
 	if err != nil {
 		t.Fatal(err)
@@ -497,9 +514,16 @@ func TestDeviceStartStop(t *testing.T) {
 	tstest.PanicOnLog()
 	tstest.ResourceCheck(t)
 
+	netMon, err := netmon.New(logger.WithPrefix(t.Logf, "... netmon: "))
+	if err != nil {
+		t.Fatalf("netmon.New: %v", err)
+	}
+	defer netMon.Close()
+
 	conn, err := NewConn(Options{
 		EndpointsFunc: func(eps []tailcfg.Endpoint) {},
 		Logf:          t.Logf,
+		NetMon:        netMon,
 	})
 	if err != nil {
 		t.Fatal(err)
@@ -1243,7 +1267,15 @@ func Test32bitAlignment(t *testing.T) {
 func newTestConn(t testing.TB) *Conn {
 	t.Helper()
 	port := pickPort(t)
+
+	netMon, err := netmon.New(logger.WithPrefix(t.Logf, "... netmon: "))
+	if err != nil {
+		t.Fatalf("netmon.New: %v", err)
+	}
+	t.Cleanup(func() { netMon.Close() })
+
 	conn, err := NewConn(Options{
+		NetMon:                 netMon,
 		DisablePortMapper:      true,
 		Logf:                   t.Logf,
 		Port:                   port,
@@ -3145,48 +3177,24 @@ func TestMaybeRebindOnError(t *testing.T) {
 	tstest.PanicOnLog()
 	tstest.ResourceCheck(t)
 
-	t.Run("darwin should rebind", func(t *testing.T) {
+	conn := newTestConn(t)
-		conn, err := NewConn(Options{
+	defer conn.Close()
-			EndpointsFunc: func(eps []tailcfg.Endpoint) {},
-			Logf:          t.Logf,
-		})
-		if err != nil {
-			t.Fatal(err)
-		}
-		defer conn.Close()
 
+	t.Run("darwin-rebind", func(t *testing.T) {
 		rebound := conn.maybeRebindOnError("darwin", syscall.EPERM)
 		if !rebound {
 			t.Errorf("darwin should rebind on syscall.EPERM")
 		}
 	})
 
-	t.Run("linux should not rebind", func(t *testing.T) {
+	t.Run("linux-not-rebind", func(t *testing.T) {
-		conn, err := NewConn(Options{
-			EndpointsFunc: func(eps []tailcfg.Endpoint) {},
-			Logf:          t.Logf,
-		})
-		if err != nil {
-			t.Fatal(err)
-		}
-		defer conn.Close()
-
 		rebound := conn.maybeRebindOnError("linux", syscall.EPERM)
 		if rebound {
 			t.Errorf("linux should not rebind on syscall.EPERM")
 		}
 	})
 
-	t.Run("should not rebind if recently rebind recently performed", func(t *testing.T) {
+	t.Run("no-frequent-rebind", func(t *testing.T) {
-		conn, err := NewConn(Options{
-			EndpointsFunc: func(eps []tailcfg.Endpoint) {},
-			Logf:          t.Logf,
-		})
-		if err != nil {
-			t.Fatal(err)
-		}
-		defer conn.Close()
-
 		conn.lastEPERMRebind.Store(time.Now().Add(-1 * time.Second))
 		rebound := conn.maybeRebindOnError("darwin", syscall.EPERM)
 		if rebound {