From e9ae398199ae12125d089e626329d6d1ac854c8c Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 23 Jun 2026 19:14:48 +0000 Subject: [PATCH] wgengine: drop userspaceEngine.peerSequence Another baby step toward removing slices of peers from the engine. getStatus iterated peerSequence (a key snapshot built in Reconfig from cfg.Peers) and then asked wgdev for each peer's stats; peers that weren't active in wgdev silently fell out. Iterate active wgdev peers directly via RemoveMatchingPeers(returnFalse) instead. Updates #12542 Signed-off-by: Brad Fitzpatrick Change-Id: I3abd348abc30db706db29b3a785179259e48abda --- tsnet/tsnet_test.go | 82 +++++++++++++++++++++++++++++++++---------- wgengine/userspace.go | 25 +++++++------ 2 files changed, 79 insertions(+), 28 deletions(-) diff --git a/tsnet/tsnet_test.go b/tsnet/tsnet_test.go index ce1162fc6..69b8531e4 100644 --- a/tsnet/tsnet_test.go +++ b/tsnet/tsnet_test.go @@ -1819,12 +1819,9 @@ func testPingPeerLearnedViaDelta(t *testing.T, pt tailcfg.PingType) { // Wait for the delta to land in s1's nodeBackend. if err := waitFor(t, ctx, s1, func(nm *netmap.NetworkMap) bool { - for _, p := range nm.Peers { - if p.Key() == s2Key { - return true - } - } - return false + return slices.ContainsFunc(nm.Peers, func(p tailcfg.NodeView) bool { + return p.Key() == s2Key + }) }); err != nil { t.Fatalf("waitFor s2 in s1 netmap: %v", err) } @@ -1915,12 +1912,9 @@ func TestPingSubnetRouteOfDeltaPeer(t *testing.T) { // Wait for the delta to land in s1's nodeBackend. if err := waitFor(t, ctx, s1, func(nm *netmap.NetworkMap) bool { - for _, p := range nm.Peers { - if p.Key() == s2Key { - return true - } - } - return false + return slices.ContainsFunc(nm.Peers, func(p tailcfg.NodeView) bool { + return p.Key() == s2Key + }) }); err != nil { t.Fatalf("waitFor s2 in s1 netmap: %v", err) } @@ -1978,6 +1972,61 @@ func TestPingSelfReturnsIsLocalIP(t *testing.T) { } } +// TestStatusReportsPeerInEngine verifies that a peer with an active +// wireguard session is reported as InEngine=true in the local +// [ipnstate.Status]. This exercises [wgengine.Engine.UpdateStatus] -> +// userspaceEngine.getStatus -> the active-wgdev-peer iteration -> +// [ipnstate.StatusBuilder.AddPeer] with InEngine=true. It's the only +// signal in the tree that exercises getStatus's peer-list path; the +// wgengine and ipnlocal unit tests don't assert on it. +func TestStatusReportsPeerInEngine(t *testing.T) { + tstest.ResourceCheck(t) + ctx, cancel := context.WithTimeout(t.Context(), 120*time.Second) + defer cancel() + + controlURL, _ := startControl(t) + s1, _, _ := startServer(t, ctx, controlURL, "s1") + _, s2ip, s2Key := startServer(t, ctx, controlURL, "s2") + + lc1, err := s1.LocalClient() + if err != nil { + t.Fatal(err) + } + + if err := waitFor(t, ctx, s1, func(nm *netmap.NetworkMap) bool { + return slices.ContainsFunc(nm.Peers, func(p tailcfg.NodeView) bool { + return p.Key() == s2Key + }) + }); err != nil { + t.Fatalf("waitFor s2 in s1 netmap: %v", err) + } + + // Ping via ICMP so a real packet flows through wireguard-go and + // instantiates s2 in s1's wgdev peer map. PingDisco wouldn't + // suffice; it goes directly to magicsock and bypasses wgdev. + pingCtx, cancelPing := pingTimeout(ctx) + defer cancelPing() + pr, err := lc1.Ping(pingCtx, s2ip, tailcfg.PingICMP) + if err != nil { + t.Fatalf("Ping: %v", err) + } + if pr.Err != "" { + t.Fatalf("Ping s1->s2 failed: %s", pr.Err) + } + + status, err := lc1.Status(ctx) + if err != nil { + t.Fatal(err) + } + peer, ok := status.Peer[s2Key] + if !ok { + t.Fatalf("status.Peer missing s2 (%v); peers=%v", s2Key, status.Peers()) + } + if !peer.InEngine { + t.Errorf("peer.InEngine = false, want true (peer=%+v)", peer) + } +} + func TestCapturePcap(t *testing.T) { const timeLimit = 120 ctx, cancel := context.WithTimeout(context.Background(), timeLimit*time.Second) @@ -3238,12 +3287,9 @@ func TestDialUDPInjectedReadRecordsFlowState(t *testing.T) { ctx, cancel := context.WithTimeout(t.Context(), 30*time.Second) defer cancel() if err := waitFor(t, ctx, lt.s2, func(nm *netmap.NetworkMap) bool { - for _, p := range nm.Peers { - if p.Key() == s1Key && p.IsJailed() { - return true - } - } - return false + return slices.ContainsFunc(nm.Peers, func(p tailcfg.NodeView) bool { + return p.Key() == s1Key && p.IsJailed() + }) }); err != nil { t.Fatalf("waiting for s1 to appear jailed in s2's netmap: %v", err) } diff --git a/wgengine/userspace.go b/wgengine/userspace.go index 6175bfc90..b27a60895 100644 --- a/wgengine/userspace.go +++ b/wgengine/userspace.go @@ -142,7 +142,6 @@ type userspaceEngine struct { netMap *netmap.NetworkMap // or nil closing bool // Close was called (even if we're still closing) statusCallback StatusCallback - peerSequence views.Slice[key.NodePublic] endpoints []tailcfg.Endpoint pendOpen map[flowtrackTuple]*pendingOpenFlow // see pendopen.go @@ -829,15 +828,11 @@ func (e *userspaceEngine) Reconfig(cfg *wgcfg.Config, routerCfg *router.Config, e.tundev.SetWGConfig(cfg) peerSet := make(set.Set[key.NodePublic], len(cfg.Peers)) - - e.mu.Lock() - seq := make([]key.NodePublic, 0, len(cfg.Peers)) for _, p := range cfg.Peers { - seq = append(seq, p.PublicKey) peerSet.Add(p.PublicKey) } - e.peerSequence = views.SliceOf(seq) + e.mu.Lock() nm := e.netMap e.mu.Unlock() @@ -1155,7 +1150,6 @@ func (e *userspaceEngine) getStatus() (*Status, error) { e.mu.Lock() closing := e.closing - peerKeys := e.peerSequence localAddrs := slices.Clone(e.endpoints) e.mu.Unlock() @@ -1163,9 +1157,20 @@ func (e *userspaceEngine) getStatus() (*Status, error) { return nil, ErrEngineClosing } - peers := make([]ipnstate.PeerStatusLite, 0, peerKeys.Len()) - for _, key := range peerKeys.All() { - if status, ok := e.getPeerStatusLite(key); ok { + // Snapshot the set of active wgdev peers. wireguard-go has no + // read-only iterator over its peer map; RemoveMatchingPeers with + // a callback that always returns false is the cheap equivalent + // (the callback can't itself call LookupActivePeer, though, as + // RemoveMatchingPeers holds the wireguard device's mutex). + var peerKeys []key.NodePublic + e.wgdev.RemoveMatchingPeers(func(pk device.NoisePublicKey) bool { + peerKeys = append(peerKeys, key.NodePublicFromRaw32(mem.B(pk[:]))) + return false + }) + + peers := make([]ipnstate.PeerStatusLite, 0, len(peerKeys)) + for _, k := range peerKeys { + if status, ok := e.getPeerStatusLite(k); ok { peers = append(peers, status) } }