From 7a62dddeac6c9fc0ffd06f9fb3d5ac66ce99a808 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Fri, 26 Apr 2024 18:28:01 -0700 Subject: [PATCH] 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 --- cmd/tailscale/cli/netcheck.go | 1 + net/netcheck/netcheck.go | 28 ++++-------- net/netcheck/netcheck_test.go | 38 ++++++++++------ wgengine/magicsock/magicsock.go | 12 ++--- wgengine/magicsock/magicsock_test.go | 66 ++++++++++++++++------------ 5 files changed, 78 insertions(+), 67 deletions(-) diff --git a/cmd/tailscale/cli/netcheck.go b/cmd/tailscale/cli/netcheck.go index a33ee067f..e642baec8 100644 --- a/cmd/tailscale/cli/netcheck.go +++ b/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 } diff --git a/net/netcheck/netcheck.go b/net/netcheck/netcheck.go index 3181119f2..c3dfb5be8 100644 --- a/net/netcheck/netcheck.go +++ b/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 - 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() - } + ifState := c.NetMon.InterfaceState() // See if IPv6 works at all, or if it's been hard disabled at the // OS level. diff --git a/net/netcheck/netcheck_test.go b/net/netcheck/netcheck_test.go index aaff4e86b..41c55cff5 100644 --- a/net/netcheck/netcheck_test.go +++ b/net/netcheck/netcheck_test.go @@ -19,10 +19,12 @@ "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{ - Logf: t.Logf, - } + c := newTestClient(t) 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{ - Logf: t.Logf, - } + c := newTestClient(t) + 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{ - Logf: t.Logf, - testEnoughRegions: 1, - - // Set the delay long enough that we have time to cancel it - // when our STUN probe succeeds. - testCaptivePortalDelay: 10 * time.Second, - } + c := newTestClient(t) + c.testEnoughRegions = 1 + // Set the delay long enough that we have time to cancel it + // when our STUN probe succeeds. + c.testCaptivePortalDelay = 10 * time.Second ctx, cancel := context.WithTimeout(context.Background(), 1*time.Second) defer cancel() diff --git a/wgengine/magicsock/magicsock.go b/wgengine/magicsock/magicsock.go index 4182eb478..c5693e28a 100644 --- a/wgengine/magicsock/magicsock.go +++ b/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 diff --git a/wgengine/magicsock/magicsock_test.go b/wgengine/magicsock/magicsock_test.go index 8202e2c2c..b4477cce6 100644 --- a/wgengine/magicsock/magicsock_test.go +++ b/wgengine/magicsock/magicsock_test.go @@ -47,6 +47,7 @@ "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, err := NewConn(Options{ - EndpointsFunc: func(eps []tailcfg.Endpoint) {}, - Logf: t.Logf, - }) - if err != nil { - t.Fatal(err) - } - defer conn.Close() + conn := newTestConn(t) + 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) { - conn, err := NewConn(Options{ - EndpointsFunc: func(eps []tailcfg.Endpoint) {}, - Logf: t.Logf, - }) - if err != nil { - t.Fatal(err) - } - defer conn.Close() - + t.Run("linux-not-rebind", func(t *testing.T) { 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) { - conn, err := NewConn(Options{ - EndpointsFunc: func(eps []tailcfg.Endpoint) {}, - Logf: t.Logf, - }) - if err != nil { - t.Fatal(err) - } - defer conn.Close() - + t.Run("no-frequent-rebind", func(t *testing.T) { conn.lastEPERMRebind.Store(time.Now().Add(-1 * time.Second)) rebound := conn.maybeRebindOnError("darwin", syscall.EPERM) if rebound {