From 21509db121ff5f73f50e34ee5d8227e78d51080d Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 7 May 2024 21:37:33 -0700 Subject: [PATCH] ipn/ipnlocal, all: plumb health trackers in tests I saw some panics in CI, like: 2024-05-08T04:30:25.9553518Z ## WARNING: (non-fatal) nil health.Tracker (being strict in CI): 2024-05-08T04:30:25.9554043Z goroutine 801 [running]: 2024-05-08T04:30:25.9554489Z tailscale.com/health.(*Tracker).nil(0x0) 2024-05-08T04:30:25.9555086Z tailscale.com/health/health.go:185 +0x70 2024-05-08T04:30:25.9555688Z tailscale.com/health.(*Tracker).SetUDP4Unbound(0x0, 0x0) 2024-05-08T04:30:25.9556373Z tailscale.com/health/health.go:532 +0x2f 2024-05-08T04:30:25.9557296Z tailscale.com/wgengine/magicsock.(*Conn).bindSocket(0xc0003b4808, 0xc0003b4878, {0x1fbca53, 0x4}, 0x0) 2024-05-08T04:30:25.9558301Z tailscale.com/wgengine/magicsock/magicsock.go:2481 +0x12c5 2024-05-08T04:30:25.9559026Z tailscale.com/wgengine/magicsock.(*Conn).rebind(0xc0003b4808, 0x0) 2024-05-08T04:30:25.9559874Z tailscale.com/wgengine/magicsock/magicsock.go:2510 +0x16f 2024-05-08T04:30:25.9561038Z tailscale.com/wgengine/magicsock.NewConn({0xc000063c80, 0x0, 0xc000197930, 0xc000197950, 0xc000197960, {0x0, 0x0}, 0xc000197970, 0xc000198ee0, 0x0, ...}) 2024-05-08T04:30:25.9562402Z tailscale.com/wgengine/magicsock/magicsock.go:476 +0xd5f 2024-05-08T04:30:25.9563779Z tailscale.com/wgengine.NewUserspaceEngine(0xc000063c80, {{0x22c8750, 0xc0001976b0}, 0x0, {0x22c3210, 0xc000063c80}, {0x22c31d8, 0x2d3c900}, 0x0, 0x0, ...}) 2024-05-08T04:30:25.9564982Z tailscale.com/wgengine/userspace.go:389 +0x159d 2024-05-08T04:30:25.9565529Z tailscale.com/ipn/ipnlocal.newTestBackend(0xc000358b60) 2024-05-08T04:30:25.9566086Z tailscale.com/ipn/ipnlocal/serve_test.go:675 +0x2a5 2024-05-08T04:30:25.9566612Z ta Updates #11874 Change-Id: I3432ed52d670743e532be4642f38dbd6e3763b1b Signed-off-by: Brad Fitzpatrick --- ipn/ipnlocal/local_test.go | 2 +- ipn/ipnlocal/loglines_test.go | 2 +- ipn/ipnlocal/peerapi_test.go | 20 ++++++++++++-------- ipn/ipnlocal/serve_test.go | 5 ++++- ipn/ipnlocal/state_test.go | 4 ++-- ipn/localapi/localapi_test.go | 2 +- wgengine/bench/wg.go | 22 ++++++++++++---------- wgengine/netstack/netstack_test.go | 14 ++++++++------ wgengine/userspace.go | 5 +++++ wgengine/userspace_ext_test.go | 6 +++++- wgengine/userspace_test.go | 10 +++++++--- wgengine/watchdog_test.go | 5 ++++- 12 files changed, 62 insertions(+), 35 deletions(-) diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index b05f2950c..2bf6ade30 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -431,7 +431,7 @@ func newTestLocalBackend(t testing.TB) *LocalBackend { sys := new(tsd.System) store := new(mem.Store) sys.Set(store) - eng, err := wgengine.NewFakeUserspaceEngine(logf, sys.Set) + eng, err := wgengine.NewFakeUserspaceEngine(logf, sys.Set, sys.HealthTracker()) if err != nil { t.Fatalf("NewFakeUserspaceEngine: %v", err) } diff --git a/ipn/ipnlocal/loglines_test.go b/ipn/ipnlocal/loglines_test.go index 8334b7f57..d05436e6d 100644 --- a/ipn/ipnlocal/loglines_test.go +++ b/ipn/ipnlocal/loglines_test.go @@ -50,7 +50,7 @@ func TestLocalLogLines(t *testing.T) { sys := new(tsd.System) store := new(mem.Store) sys.Set(store) - e, err := wgengine.NewFakeUserspaceEngine(logf, sys.Set) + e, err := wgengine.NewFakeUserspaceEngine(logf, sys.Set, sys.HealthTracker()) if err != nil { t.Fatal(err) } diff --git a/ipn/ipnlocal/peerapi_test.go b/ipn/ipnlocal/peerapi_test.go index 1552cc210..8497b38e2 100644 --- a/ipn/ipnlocal/peerapi_test.go +++ b/ipn/ipnlocal/peerapi_test.go @@ -642,8 +642,9 @@ func TestPeerAPIReplyToDNSQueries(t *testing.T) { h.isSelf = false h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345") - eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0) - pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker))) + ht := new(health.Tracker) + eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0, ht) + pm := must.Get(newProfileManager(new(mem.Store), t.Logf, ht)) h.ps = &peerAPIServer{ b: &LocalBackend{ e: eng, @@ -692,8 +693,9 @@ func TestPeerAPIPrettyReplyCNAME(t *testing.T) { var h peerAPIHandler h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345") - eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0) - pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker))) + ht := new(health.Tracker) + eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0, ht) + pm := must.Get(newProfileManager(new(mem.Store), t.Logf, ht)) var a *appc.AppConnector if shouldStore { a = appc.NewAppConnector(t.Logf, &appctest.RouteCollector{}, &appc.RouteInfo{}, fakeStoreRoutes) @@ -764,8 +766,9 @@ func TestPeerAPIReplyToDNSQueriesAreObserved(t *testing.T) { h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345") rc := &appctest.RouteCollector{} - eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0) - pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker))) + ht := new(health.Tracker) + eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0, ht) + pm := must.Get(newProfileManager(new(mem.Store), t.Logf, ht)) var a *appc.AppConnector if shouldStore { a = appc.NewAppConnector(t.Logf, rc, &appc.RouteInfo{}, fakeStoreRoutes) @@ -826,9 +829,10 @@ func TestPeerAPIReplyToDNSQueriesAreObservedWithCNAMEFlattening(t *testing.T) { var h peerAPIHandler h.remoteAddr = netip.MustParseAddrPort("100.150.151.152:12345") + ht := new(health.Tracker) rc := &appctest.RouteCollector{} - eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0) - pm := must.Get(newProfileManager(new(mem.Store), t.Logf, new(health.Tracker))) + eng, _ := wgengine.NewFakeUserspaceEngine(logger.Discard, 0, ht) + pm := must.Get(newProfileManager(new(mem.Store), t.Logf, ht)) var a *appc.AppConnector if shouldStore { a = appc.NewAppConnector(t.Logf, rc, &appc.RouteInfo{}, fakeStoreRoutes) diff --git a/ipn/ipnlocal/serve_test.go b/ipn/ipnlocal/serve_test.go index 686f49db3..a09729d78 100644 --- a/ipn/ipnlocal/serve_test.go +++ b/ipn/ipnlocal/serve_test.go @@ -672,7 +672,10 @@ func newTestBackend(t *testing.T) *LocalBackend { } sys := &tsd.System{} - e, err := wgengine.NewUserspaceEngine(logf, wgengine.Config{SetSubsystem: sys.Set}) + e, err := wgengine.NewUserspaceEngine(logf, wgengine.Config{ + SetSubsystem: sys.Set, + HealthTracker: sys.HealthTracker(), + }) if err != nil { t.Fatal(err) } diff --git a/ipn/ipnlocal/state_test.go b/ipn/ipnlocal/state_test.go index 0c58241a6..ae8871619 100644 --- a/ipn/ipnlocal/state_test.go +++ b/ipn/ipnlocal/state_test.go @@ -298,7 +298,7 @@ func TestStateMachine(t *testing.T) { sys := new(tsd.System) store := new(testStateStorage) sys.Set(store) - e, err := wgengine.NewFakeUserspaceEngine(logf, sys.Set) + e, err := wgengine.NewFakeUserspaceEngine(logf, sys.Set, sys.HealthTracker()) if err != nil { t.Fatalf("NewFakeUserspaceEngine: %v", err) } @@ -903,7 +903,7 @@ func TestEditPrefsHasNoKeys(t *testing.T) { logf := tstest.WhileTestRunningLogger(t) sys := new(tsd.System) sys.Set(new(mem.Store)) - e, err := wgengine.NewFakeUserspaceEngine(logf, sys.Set) + e, err := wgengine.NewFakeUserspaceEngine(logf, sys.Set, sys.HealthTracker()) if err != nil { t.Fatalf("NewFakeUserspaceEngine: %v", err) } diff --git a/ipn/localapi/localapi_test.go b/ipn/localapi/localapi_test.go index 0e97fd7b1..2086023d7 100644 --- a/ipn/localapi/localapi_test.go +++ b/ipn/localapi/localapi_test.go @@ -314,7 +314,7 @@ func newTestLocalBackend(t testing.TB) *ipnlocal.LocalBackend { sys := new(tsd.System) store := new(mem.Store) sys.Set(store) - eng, err := wgengine.NewFakeUserspaceEngine(logf, sys.Set) + eng, err := wgengine.NewFakeUserspaceEngine(logf, sys.Set, sys.HealthTracker()) if err != nil { t.Fatalf("NewFakeUserspaceEngine: %v", err) } diff --git a/wgengine/bench/wg.go b/wgengine/bench/wg.go index 883b4a4eb..45823dd56 100644 --- a/wgengine/bench/wg.go +++ b/wgengine/bench/wg.go @@ -48,11 +48,12 @@ func setupWGTest(b *testing.B, logf logger.Logf, traf *TrafficGen, a1, a2 netip. } s1 := new(tsd.System) e1, err := wgengine.NewUserspaceEngine(l1, wgengine.Config{ - Router: router.NewFake(l1), - NetMon: nil, - ListenPort: 0, - Tun: t1, - SetSubsystem: s1.Set, + Router: router.NewFake(l1), + NetMon: nil, + ListenPort: 0, + Tun: t1, + SetSubsystem: s1.Set, + HealthTracker: s1.HealthTracker(), }) if err != nil { log.Fatalf("e1 init: %v", err) @@ -74,11 +75,12 @@ func setupWGTest(b *testing.B, logf logger.Logf, traf *TrafficGen, a1, a2 netip. } s2 := new(tsd.System) e2, err := wgengine.NewUserspaceEngine(l2, wgengine.Config{ - Router: router.NewFake(l2), - NetMon: nil, - ListenPort: 0, - Tun: t2, - SetSubsystem: s2.Set, + Router: router.NewFake(l2), + NetMon: nil, + ListenPort: 0, + Tun: t2, + SetSubsystem: s2.Set, + HealthTracker: s2.HealthTracker(), }) if err != nil { log.Fatalf("e2 init: %v", err) diff --git a/wgengine/netstack/netstack_test.go b/wgengine/netstack/netstack_test.go index 51292c649..a8b180f95 100644 --- a/wgengine/netstack/netstack_test.go +++ b/wgengine/netstack/netstack_test.go @@ -44,9 +44,10 @@ func TestInjectInboundLeak(t *testing.T) { } sys := new(tsd.System) eng, err := wgengine.NewUserspaceEngine(logf, wgengine.Config{ - Tun: tunDev, - Dialer: dialer, - SetSubsystem: sys.Set, + Tun: tunDev, + Dialer: dialer, + SetSubsystem: sys.Set, + HealthTracker: sys.HealthTracker(), }) if err != nil { t.Fatal(err) @@ -100,9 +101,10 @@ func makeNetstack(t *testing.T, config func(*Impl)) *Impl { dialer := new(tsdial.Dialer) logf := tstest.WhileTestRunningLogger(t) eng, err := wgengine.NewUserspaceEngine(logf, wgengine.Config{ - Tun: tunDev, - Dialer: dialer, - SetSubsystem: sys.Set, + Tun: tunDev, + Dialer: dialer, + SetSubsystem: sys.Set, + HealthTracker: sys.HealthTracker(), }) if err != nil { t.Fatal(err) diff --git a/wgengine/userspace.go b/wgengine/userspace.go index f1f0a08ba..aaa45ddda 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -47,6 +47,7 @@ "tailscale.com/util/deephash" "tailscale.com/util/mak" "tailscale.com/util/set" + "tailscale.com/util/testenv" "tailscale.com/version" "tailscale.com/wgengine/capture" "tailscale.com/wgengine/filter" @@ -260,6 +261,10 @@ func NewUserspaceEngine(logf logger.Logf, conf Config) (_ Engine, reterr error) var closePool closeOnErrorPool defer closePool.closeAllIfError(&reterr) + if testenv.InTest() && conf.HealthTracker == nil { + panic("NewUserspaceEngine called without HealthTracker (being strict in tests)") + } + if conf.Tun == nil { logf("[v1] using fake (no-op) tun device") conf.Tun = tstun.NewFake() diff --git a/wgengine/userspace_ext_test.go b/wgengine/userspace_ext_test.go index 5183d8f1b..6610f1e92 100644 --- a/wgengine/userspace_ext_test.go +++ b/wgengine/userspace_ext_test.go @@ -19,7 +19,10 @@ func TestIsNetstack(t *testing.T) { sys := new(tsd.System) e, err := wgengine.NewUserspaceEngine( tstest.WhileTestRunningLogger(t), - wgengine.Config{SetSubsystem: sys.Set}, + wgengine.Config{ + SetSubsystem: sys.Set, + HealthTracker: sys.HealthTracker(), + }, ) if err != nil { t.Fatal(err) @@ -68,6 +71,7 @@ func TestIsNetstackRouter(t *testing.T) { } conf := tt.conf conf.SetSubsystem = sys.Set + conf.HealthTracker = sys.HealthTracker() e, err := wgengine.NewUserspaceEngine(logger.Discard, conf) if err != nil { t.Fatal(err) diff --git a/wgengine/userspace_test.go b/wgengine/userspace_test.go index 6d086a448..8763a84a1 100644 --- a/wgengine/userspace_test.go +++ b/wgengine/userspace_test.go @@ -15,6 +15,7 @@ "tailscale.com/cmd/testwrapper/flakytest" "tailscale.com/control/controlknobs" "tailscale.com/envknob" + "tailscale.com/health" "tailscale.com/net/dns" "tailscale.com/net/netaddr" "tailscale.com/net/tstun" @@ -98,7 +99,8 @@ func nodeViews(v []*tailcfg.Node) []tailcfg.NodeView { } func TestUserspaceEngineReconfig(t *testing.T) { - e, err := NewFakeUserspaceEngine(t.Logf, 0) + ht := new(health.Tracker) + e, err := NewFakeUserspaceEngine(t.Logf, 0, ht) if err != nil { t.Fatal(err) } @@ -164,9 +166,10 @@ func TestUserspaceEnginePortReconfig(t *testing.T) { // Keep making a wgengine until we find an unused port var ue *userspaceEngine + ht := new(health.Tracker) for i := range 100 { attempt := uint16(defaultPort + i) - e, err := NewFakeUserspaceEngine(t.Logf, attempt, &knobs) + e, err := NewFakeUserspaceEngine(t.Logf, attempt, &knobs, ht) if err != nil { t.Fatal(err) } @@ -245,7 +248,8 @@ func TestUserspaceEnginePeerMTUReconfig(t *testing.T) { var knobs controlknobs.Knobs - e, err := NewFakeUserspaceEngine(t.Logf, 0, &knobs) + ht := new(health.Tracker) + e, err := NewFakeUserspaceEngine(t.Logf, 0, &knobs, ht) if err != nil { t.Fatal(err) } diff --git a/wgengine/watchdog_test.go b/wgengine/watchdog_test.go index da453606a..0d4fcd8c1 100644 --- a/wgengine/watchdog_test.go +++ b/wgengine/watchdog_test.go @@ -7,6 +7,8 @@ "runtime" "testing" "time" + + "tailscale.com/health" ) func TestWatchdog(t *testing.T) { @@ -21,7 +23,8 @@ func TestWatchdog(t *testing.T) { t.Run("default watchdog does not fire", func(t *testing.T) { t.Parallel() - e, err := NewFakeUserspaceEngine(t.Logf, 0) + ht := new(health.Tracker) + e, err := NewFakeUserspaceEngine(t.Logf, 0, ht) if err != nil { t.Fatal(err) }