From 14449d9e3683428f4c1961b6bfebb80f627336b9 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 8 Sep 2026 15:08:10 -0700 Subject: [PATCH] [release-branch] fix various netmap delta bugs (#21121) * ipn/ipnlocal: fix cache update for peers deleted by netmap deltas (#20851) After a netmap delta is applied, we scan the mutations for affected peers and update the cache (if enabled) for those peers. For removals in particular, we were relying on the node backend to resolve node IDs (provided by the delta mutation) to stable IDs. Prior to 65fd320a this happened to work because the node backend would hold on to all the peers mentioned by the previous full netmap, even after applying deltas. But that was essentially accidental, and once we fixed it not to do that, these lookups no longer worked. We need the stable ID, since that is how the cache is keyed, and now that they're no longer pinned, we were not properly evicting removed peers from the cache. To fix this, capture removed peer stable IDs while applying mutations to the node backend, instead of trying to look them up afterward. Updates #20796 Change-Id: I14ded78eaf9657645f0869a52460fd3cd86edba6 Signed-off-by: M. J. Fromberger (cherry picked from commit f3552c29c04e5ec4d0d38637e67e1f8a40d8ca09) * ipn/ipnlocal: don't evict another node's index entries on netmap deltas When applying netmap deltas, nodeBackend evicted its index entries (nodeByAddr, nodeByKey, nodeByWGString, nodeByStableID, nodeByName) derived from a node's last-known value without checking that the entry still pointed at that node. Control can reassign a churning ephemeral peer's Tailscale IP (or MagicDNS name) to a newer peer and deliver the new peer's upsert before the old peer's removal, either in an earlier MapResponse or reordered within one batch by the NodeID sort in netmap.MutationsFromMapResponse. The removal then wiped the new owner's entry. The peers map itself stayed correct in every ordering, so WireGuard kept the peer and handshakes succeeded, but WhoIs lookups by IP failed until the next full netmap rebuilt the indexes. On App Connectors that surfaced as "peerapi: unknown peer" and refused DNS connections from affected clients, with a toggle of Tailscale (forcing a full netmap) as the only recovery. Make every index eviction conditional on the entry still mapping to the node being removed or replaced, and add a regression test covering the cross-batch, intra-batch, and upsert-eviction orderings. Also add an end-to-end test in tstest/integration showing that a MapResponse reusing an address is handled incrementally rather than as a full netmap, and that LocalBackend.WhoIs still resolves the reused address afterwards, which is the lookup PeerAPI makes before it accepts a connection. Updates tailscale/corp#47435 Co-authored-by: Brendan Creane Signed-off-by: Brendan Creane Signed-off-by: Brad Fitzpatrick Change-Id: I3f8c2a9d41e07b6a5cd2e94f78b013c6ad2f5e91 (cherry picked from commit 2ae2808b64622a74cad39fd658dd2a276c4d5beb) * ipn/ipnlocal: preserve peer deltas on expiry Refresh the expiry timer netmap from the live peer state before reinstalling it, preventing delta updates from being rolled back. Updates tailscale/corp#47686 Change-Id: Idc738acea82bab5a8ba772084a41e55b38a06bcc Signed-off-by: Brad Fitzpatrick (cherry picked from commit 0640312e510e091907b4cb386e9f658b50949889) * control/controlclient: replay user profiles on delta peer upserts A full netmap carries only the profiles of users with a currently visible peer (netmapForResponse), and nodeBackend replaces its live profile set wholesale on every full netmap install. A full netmap that arrives while a user has no visible peer therefore drops that user's profile downstream. When a peer of that user later returns as an incremental upsert, control does not resend the profile, because MapResponse.UserProfiles has carried only new or updated profiles since mapver 5. The upsert indexes the node by address and key, so WireGuard admits its traffic, but WhoIs then fails one step later at the user profile lookup, surfacing as "peerapi: unknown peer" until the next full netmap. It is a second, independent cause of the symptom fixed by the recent index eviction change. mapSession.lastUserProfile holds the profile the whole time, so when handling a response incrementally, also deliver the profiles of upserted peers' users (and sharers) from that store, before the mutations that reference them. The backport adapts the new tests to this branch's extra viaTSMP parameter on handleNonKeepAliveMapResponse. Updates tailscale/corp#47435 Signed-off-by: Brad Fitzpatrick Change-Id: I9c4b2f6a8e0d47d3b1a5c2e7f4098d61b23a7c50 (cherry picked from commit 5201273aec737d6372ab7423c31c04ca3ca2a0c2) --------- Signed-off-by: M. J. Fromberger Signed-off-by: Brendan Creane Signed-off-by: Brad Fitzpatrick Co-authored-by: M. J. Fromberger Co-authored-by: Brendan Creane --- control/controlclient/map.go | 49 ++++-- control/controlclient/map_test.go | 99 ++++++++++++ ipn/ipnlocal/local.go | 45 +++--- ipn/ipnlocal/local_test.go | 94 +++++++++++ ipn/ipnlocal/node_backend.go | 52 ++++-- ipn/ipnlocal/node_backend_test.go | 96 +++++++++++ tstest/integration/netmapdelta_test.go | 215 +++++++++++++++++++++++++ 7 files changed, 605 insertions(+), 45 deletions(-) create mode 100644 tstest/integration/netmapdelta_test.go diff --git a/control/controlclient/map.go b/control/controlclient/map.go index 77ca0e68d..16877a9b1 100644 --- a/control/controlclient/map.go +++ b/control/controlclient/map.go @@ -437,28 +437,55 @@ func (ms *mapSession) tryHandleIncrementally(res *tailcfg.MapResponse) bool { return false } } - // Same shape for UserProfiles: deliver any new/updated profiles before - // the peer mutations that may reference them, so bus consumers never - // see a UserID for which a profile hasn't been published. The values - // are read from ms.lastUserProfile (just populated by + mutations, mutationsOK := netmap.MutationsFromMapResponse(res, time.Now()) + + // Same shape for UserProfiles: deliver profiles before the peer + // mutations that may reference them, so bus consumers never see a + // UserID for which a profile hasn't been published. + // + // Besides the new/updated profiles carried by the response, also + // replay the profiles of upserted peers' users from + // ms.lastUserProfile. A full netmap keeps only the profiles of users + // with a currently visible peer (see [mapSession.addUserProfile]), so + // a peer returning via delta upsert can reference a user the updater + // has since dropped, and control (mapver 5+) does not resend + // unchanged profiles. Without the replay, WhoIs on the returned peer + // fails at the user profile lookup until the next full netmap. + // + // The values are read from ms.lastUserProfile (just populated by // updateStateFromResponse) so views are shared with mapSession's // store; downstream consumers can use [UserProfileView.Equal] for // dedup without copying. - if len(res.UserProfiles) > 0 { + var profiles map[tailcfg.UserID]tailcfg.UserProfileView + addProfile := func(id tailcfg.UserID) { + if id == 0 { + return + } + if up, ok := ms.lastUserProfile[id]; ok { + mak.Set(&profiles, id, up) + } + } + for _, up := range res.UserProfiles { + addProfile(up.ID) + } + if mutationsOK { + for _, m := range mutations { + if up, ok := m.(netmap.NodeMutationUpsert); ok { + addProfile(up.Node.User()) + addProfile(up.Node.Sharer()) + } + } + } + if len(profiles) > 0 { upu, ok := ms.netmapUpdater.(UserProfileUpdater) if !ok { return false } - profiles := make(map[tailcfg.UserID]tailcfg.UserProfileView, len(res.UserProfiles)) - for _, up := range res.UserProfiles { - profiles[up.ID] = ms.lastUserProfile[up.ID] - } if !upu.UpdateUserProfiles(profiles) { return false } } - mutations, ok := netmap.MutationsFromMapResponse(res, time.Now()) - if !ok { + if !mutationsOK { return false } if len(mutations) > 0 { diff --git a/control/controlclient/map_test.go b/control/controlclient/map_test.go index fb4307c39..377fdc0f5 100644 --- a/control/controlclient/map_test.go +++ b/control/controlclient/map_test.go @@ -1376,6 +1376,105 @@ func TestExistingPeerReplacementHandledIncrementally(t *testing.T) { } } +type profileRecordingUpdater struct { + countingDeltaNetmapUpdater + profiles []map[tailcfg.UserID]tailcfg.UserProfileView + profilesAtDelta int +} + +func (nu *profileRecordingUpdater) UpdateUserProfiles(profiles map[tailcfg.UserID]tailcfg.UserProfileView) bool { + nu.profiles = append(nu.profiles, profiles) + return true +} + +func (nu *profileRecordingUpdater) UpdateNetmapDelta(muts []netmap.NodeMutation) bool { + nu.profilesAtDelta = len(nu.profiles) + return nu.countingDeltaNetmapUpdater.UpdateNetmapDelta(muts) +} + +// TestUpsertReplaysUserProfiles verifies that a peer upsert delivered as a +// delta also replays the peer's user and sharer profiles from the map +// session's profile store, even when the MapResponse carries no UserProfiles +// (control only resends changed profiles). A full netmap installed while the +// user had no visible peers drops the profile downstream, and without the +// replay a WhoIs on the returned peer fails at the user profile lookup. +func TestUpsertReplaysUserProfiles(t *testing.T) { + nu := &profileRecordingUpdater{} + ms := newTestMapSession(t, nu) + ctx := t.Context() + + peer := &tailcfg.Node{ + ID: 1, + StableID: "peer", + Name: "peer.example.ts.net.", + User: 100, + Sharer: 200, + Key: key.NewNode().Public(), + DiscoKey: key.NewDisco().Public(), + Addresses: []netip.Prefix{netip.MustParsePrefix("100.64.0.1/32")}, + AllowedIPs: []netip.Prefix{netip.MustParsePrefix("100.64.0.1/32")}, + Hostinfo: (&tailcfg.Hostinfo{}).View(), + } + if err := ms.handleNonKeepAliveMapResponse(ctx, &tailcfg.MapResponse{ + Node: &tailcfg.Node{Name: "self.example.ts.net."}, + Peers: []*tailcfg.Node{peer}, + UserProfiles: []tailcfg.UserProfile{ + {ID: 0, LoginName: "invalid@example.com"}, + {ID: 100, LoginName: "user@example.com"}, + {ID: 200, LoginName: "sharer@example.com"}, + }, + }, false); err != nil { + t.Fatal(err) + } + if got := nu.full.Load(); got != 1 { + t.Fatalf("full updates after initial response = %d; want 1", got) + } + + // An upsert with no UserProfiles in the response must still deliver + // both profiles, before the delta lands. + replacement := peer.Clone() + replacement.AllowedIPs = append(replacement.AllowedIPs, netip.MustParsePrefix("100.64.0.2/32")) + if err := ms.handleNonKeepAliveMapResponse(ctx, &tailcfg.MapResponse{ + PeersChanged: []*tailcfg.Node{replacement}, + }, false); err != nil { + t.Fatal(err) + } + if got := nu.full.Load(); got != 1 { + t.Fatalf("full updates after peer upsert = %d; want 1", got) + } + if got := nu.delta.Load(); got != 1 { + t.Fatalf("delta updates after peer upsert = %d; want 1", got) + } + if got := len(nu.profiles); got != 2 { + t.Fatalf("UpdateUserProfiles calls = %d; want 2 (one initial, one replayed)", got) + } + if got := nu.profilesAtDelta; got != 2 { + t.Errorf("profiles delivered before delta = %d; want 2", got) + } + replayed := nu.profiles[1] + if _, ok := replayed[0]; ok { + t.Error("replayed profiles contains zero user ID") + } + for _, id := range []tailcfg.UserID{100, 200} { + up, ok := replayed[id] + if !ok || !up.Valid() { + t.Errorf("replayed profiles missing valid profile for user %d", id) + } + } + + // A patch-only change (no upsert) must not replay any profiles. + patched := replacement.Clone() + patched.Endpoints = eps("10.0.0.1:1111") + if err := ms.handleNonKeepAliveMapResponse(ctx, &tailcfg.MapResponse{ + PeersChanged: []*tailcfg.Node{patched}, + }, false); err != nil { + t.Fatal(err) + } + if got := len(nu.profiles); got != 2 { + t.Errorf("UpdateUserProfiles calls after patch-only change = %d; want still 2", got) + } +} + // tests (*mapSession).patchifyPeersChanged; smaller tests are in TestPeerChangeDiff func TestPatchifyPeersChanged(t *testing.T) { hi := (&tailcfg.Hostinfo{}).View() diff --git a/ipn/ipnlocal/local.go b/ipn/ipnlocal/local.go index d5d8d8fbc..834c4f073 100644 --- a/ipn/ipnlocal/local.go +++ b/ipn/ipnlocal/local.go @@ -1857,19 +1857,7 @@ func (b *LocalBackend) setControlClientStatusLocked(c controlclient.Client, st c if !nextExpiry.IsZero() { tmrDuration := nextExpiry.Sub(now) + 10*time.Second b.nmExpiryTimer = b.clock.AfterFunc(tmrDuration, func() { - // Skip if the world has moved on past the - // saved call (e.g. if we race stopping this - // timer). - if b.numClientStatusCalls.Load() != currCall { - return - } - - b.logf("setClientStatus: netmap expiry timer triggered after %v", tmrDuration) - - // Call ourselves with the current status again; the logic in - // setClientStatus will take care of updating the expired field - // of peers in the netmap. - b.SetControlClientStatus(c, st) + b.handleNetmapExpiry(c, st, currCall, tmrDuration) }) } } @@ -2120,6 +2108,29 @@ func (b *LocalBackend) PatchDiscoKey(pub key.NodePublic, disco key.DiscoPublic) b.currentNode().recordTSMPLearnedDisco(pub, disco) } +// handleNetmapExpiry reruns netmap status handling when a node may have +// expired. The status captured when the timer was created has a Peers slice +// that delta updates do not change, so replace it with the live peers first. +// Hold b.mu across the generation check, snapshot, and status handling so a +// concurrent delta cannot be overwritten. +func (b *LocalBackend) handleNetmapExpiry(c controlclient.Client, st controlclient.Status, call uint32, after time.Duration) { + defer b.CheckDeadlocks()() + + if b.ignoreControlClientUpdates.Load() { + b.logf("ignoring netmap expiry during controlclient shutdown") + return + } + b.mu.Lock() + defer b.mu.Unlock() + if b.numClientStatusCalls.Load() != call { + return + } + + b.logf("setClientStatus: netmap expiry timer triggered after %v", after) + st.NetMap = b.currentNode().netMapWithPeers() + b.setControlClientStatusLocked(c, st) +} + type preferencePolicyInfo struct { key pkey.Key get func(ipn.PrefsView) bool @@ -2553,13 +2564,7 @@ func (b *LocalBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (handled bo peersToUpdate = append(peersToUpdate, n) } } - var peersToRemove []tailcfg.StableNodeID - for id := range removeIDs { - if n, ok := cn.NodeByID(id); ok { - peersToRemove = append(peersToRemove, n.StableID()) - } - } - if err := b.writePeerDeltaToDiskLocked(peersToUpdate, peersToRemove); err != nil { + if err := b.writePeerDeltaToDiskLocked(peersToUpdate, deltaRes.RemovedPeers); err != nil { b.logf("update netmap cache for peer deltas: %v", err) } } diff --git a/ipn/ipnlocal/local_test.go b/ipn/ipnlocal/local_test.go index d72d59c43..351d5def0 100644 --- a/ipn/ipnlocal/local_test.go +++ b/ipn/ipnlocal/local_test.go @@ -53,6 +53,7 @@ "tailscale.com/tstest" "tailscale.com/tstest/deptest" "tailscale.com/tstest/typewalk" + "tailscale.com/tstime" "tailscale.com/types/appctype" "tailscale.com/types/dnstype" "tailscale.com/types/ipproto" @@ -619,6 +620,15 @@ func TestUpdateNetMapCache(t *testing.T) { netip.MustParsePrefix("100.2.3.5/32"), }, }).View(), + (&tailcfg.Node{ + ID: 602, + StableID: "n602FAKE", + User: tailcfg.UserID(1), + Key: makeNodeKeyFromID(602), + Addresses: []netip.Prefix{ + netip.MustParsePrefix("100.3.4.6/32"), + }, + }).View(), }, } @@ -686,6 +696,19 @@ func TestUpdateNetMapCache(t *testing.T) { t.Logf("Cache directory has %d entries (OK)", len(des)) } + // Apply a delta update that removes a node, and verify that this gets + // reflected in the cache. + clb.UpdateNetmapDelta([]netmap.NodeMutation{ + netmap.MakeNodeMutationRemove(602), + }) + if got, err := netmapcache.NewCache(netmapcache.FileStore(cacheDir)).Load(t.Context()); err != nil { + t.Errorf("Load cached netmap: %v", err) + } else if i := slices.IndexFunc(got.Peers, func(n tailcfg.NodeView) bool { + return n.ID() == 602 + }); i >= 0 { + t.Errorf("Cache did not get updated, %d (%v) still present", got.Peers[i].ID(), got.Peers[i].StableID()) + } + // Now disable the node attribute again, send another update, and verify // that the cache got cleaned up. testMap.AllCaps = nil @@ -2485,6 +2508,77 @@ func TestSetControlClientStatusSendsFullNetmapAsPeerChanges(t *testing.T) { nw.check() } +type expiryCallbackClock struct { + tstime.StdClock + now time.Time + afterFunc func() +} + +type expiryCallbackTimer struct{} + +func (*expiryCallbackTimer) Reset(time.Duration) bool { return false } +func (*expiryCallbackTimer) Stop() bool { return true } + +func (c *expiryCallbackClock) Now() time.Time { return c.now } + +func (c *expiryCallbackClock) AfterFunc(_ time.Duration, f func()) tstime.TimerController { + c.afterFunc = f + return new(expiryCallbackTimer) +} + +func TestNetmapExpiryTimerPreservesPeerDeltas(t *testing.T) { + b := newTestLocalBackend(t) + now := time.Unix(1770000000, 0) + clock := &expiryCallbackClock{now: now} + b.ForTest().SetClock(clock) + + oldPeer := makePeer(1, func(n *tailcfg.Node) { + n.KeyExpiry = now.Add(time.Minute) + }) + nm := &netmap.NetworkMap{ + SelfNode: makePeer(2), + Peers: []tailcfg.NodeView{oldPeer}, + } + b.SetControlClientStatus(b.cc, controlclient.Status{NetMap: nm}) + if clock.afterFunc == nil { + t.Fatal("expiry timer was not scheduled") + } + expiryFunc := clock.afterFunc + + newPeer := oldPeer.AsStruct() + newPeer.Key = makeNodeKeyFromID(3) + newPeer.KeyExpiry = now.Add(time.Hour) + b.UpdateNetmapDelta([]netmap.NodeMutation{ + netmap.NodeMutationUpsert{Node: newPeer.View()}, + }) + + clock.now = now.Add(time.Minute + 10*time.Second) + expiryFunc() + + got, ok := b.PeerByID(oldPeer.ID()) + if !ok { + t.Fatal("peer disappeared when expiry timer fired") + } + if got.Key() != newPeer.Key { + t.Errorf("peer key reverted when expiry timer fired: got %v, want %v", got.Key(), newPeer.Key) + } + if got.Expired() { + t.Error("re-authenticated peer was marked expired when expiry timer fired") + } +} + +func TestNetmapExpiryIgnoredDuringControlClientShutdown(t *testing.T) { + b := newTestLocalBackend(t) + call := b.numClientStatusCalls.Load() + b.ignoreControlClientUpdates.Store(true) + + b.handleNetmapExpiry(b.cc, controlclient.Status{}, call, 0) + + if got := b.numClientStatusCalls.Load(); got != call { + t.Errorf("status calls = %d, want %d", got, call) + } +} + // TestNotifyForSessionUserProfilesGating verifies that // [Notify.UserProfiles] is only delivered to sessions opted in to // NotifyPeerChanges/NotifyPeerPatches, and is deduped per-UserID diff --git a/ipn/ipnlocal/node_backend.go b/ipn/ipnlocal/node_backend.go index 7511669fa..66a3d4ba6 100644 --- a/ipn/ipnlocal/node_backend.go +++ b/ipn/ipnlocal/node_backend.go @@ -794,16 +794,17 @@ func (nb *nodeBackend) addNodeNameLocked(name string, nid tailcfg.NodeID) { } // removeNodeNameLocked removes both the FQDN and short-name keys for the -// given node from nb.nodeByName. nb.mu must be held. -func (nb *nodeBackend) removeNodeNameLocked(name string) { +// given node from nb.nodeByName, unless another node has since claimed +// them (see [deleteIfOwned]). nb.mu must be held. +func (nb *nodeBackend) removeNodeNameLocked(name string, nid tailcfg.NodeID) { if name == "" { // We might support name-less nodes in the future; tailscale/corp#43949 return } canon := strings.ToLower(strings.TrimSuffix(name, ".")) - delete(nb.nodeByName, canon) + deleteIfOwned(nb.nodeByName, canon, nid) if suffix := nb.netMap.MagicDNSSuffix(); dnsname.HasSuffix(canon, suffix) { - delete(nb.nodeByName, dnsname.TrimSuffix(canon, suffix)) + deleteIfOwned(nb.nodeByName, dnsname.TrimSuffix(canon, suffix), nid) } } @@ -1066,6 +1067,22 @@ func (nb *nodeBackend) mergeUserProfiles(profiles map[tailcfg.UserID]tailcfg.Use } } +// deleteIfOwned deletes m[k] only if the entry still maps to nid. +// +// It exists because a node index entry derived from a node's last-known +// value may have since been claimed by another node. For example, control +// can reassign a churning ephemeral peer's Tailscale IP to a newer peer +// and deliver the new peer's upsert before the old peer's removal, either +// in an earlier MapResponse or reordered within one batch by the NodeID +// sort in [netmap.MutationsFromMapResponse]. Deleting unconditionally +// would then evict the new owner's entry, breaking lookups by IP (WhoIs, +// and thus PeerAPI and App Connector DNS) until the next full netmap. +func deleteIfOwned[K comparable](m map[K]tailcfg.NodeID, k K, nid tailcfg.NodeID) { + if m[k] == nid { + delete(m, k) + } +} + // netmapDeltaResult describes the side effects of applying netmap // delta mutations that the caller must propagate. type netmapDeltaResult struct { @@ -1077,6 +1094,10 @@ type netmapDeltaResult struct { // way that requires a WireGuard session reset (see // [nodeBackend.discoChangedLocked]). DiscoChanged set.Set[key.NodePublic] + + // RemovedPeers is a slice of peer stable node IDs (if any) that were + // removed by applying the delta. + RemovedPeers []tailcfg.StableNodeID } // UpdateNetmapDelta applies the given netmap mutations to the live @@ -1124,15 +1145,17 @@ func (nb *nodeBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (res netmap // console arrives as an upsert with a new Name, and a // stale nodeByName entry would keep serving MagicDNS // answers for the old name (tailscale/corp#45631). + // Evictions are conditional (see [deleteIfOwned]) so + // entries already claimed by another node are kept. for _, ipp := range old.Addresses().All() { if ipp.IsSingleIP() { - delete(nb.nodeByAddr, ipp.Addr()) + deleteIfOwned(nb.nodeByAddr, ipp.Addr(), nid) } } - delete(nb.nodeByKey, old.Key()) - delete(nb.nodeByWGString, old.Key().WireGuardGoString()) - delete(nb.nodeByStableID, old.StableID()) - nb.removeNodeNameLocked(old.Name()) + deleteIfOwned(nb.nodeByKey, old.Key(), nid) + deleteIfOwned(nb.nodeByWGString, old.Key().WireGuardGoString(), nid) + deleteIfOwned(nb.nodeByStableID, old.StableID(), nid) + nb.removeNodeNameLocked(old.Name(), nid) } mak.Set(&nb.peers, nid, m.Node) for _, ipp := range m.Node.Addresses().All() { @@ -1151,16 +1174,17 @@ func (nb *nodeBackend) UpdateNetmapDelta(muts []netmap.NodeMutation) (res netmap if old, ok := nb.peers[nid]; ok { for _, ipp := range old.Addresses().All() { if ipp.IsSingleIP() { - delete(nb.nodeByAddr, ipp.Addr()) + deleteIfOwned(nb.nodeByAddr, ipp.Addr(), nid) } } - delete(nb.nodeByKey, old.Key()) - delete(nb.nodeByWGString, old.Key().WireGuardGoString()) - delete(nb.nodeByStableID, old.StableID()) + deleteIfOwned(nb.nodeByKey, old.Key(), nid) + deleteIfOwned(nb.nodeByWGString, old.Key().WireGuardGoString(), nid) + deleteIfOwned(nb.nodeByStableID, old.StableID(), nid) delete(nb.tsmpLearnedDisco, old.Key()) - nb.removeNodeNameLocked(old.Name()) + nb.removeNodeNameLocked(old.Name(), nid) delete(nb.peers, nid) rt.RemovePeer(nid) + res.RemovedPeers = append(res.RemovedPeers, old.StableID()) } continue } diff --git a/ipn/ipnlocal/node_backend_test.go b/ipn/ipnlocal/node_backend_test.go index fa55fa6c6..c49c2f4d9 100644 --- a/ipn/ipnlocal/node_backend_test.go +++ b/ipn/ipnlocal/node_backend_test.go @@ -6,6 +6,7 @@ import ( "context" "errors" + "fmt" "iter" "maps" "net/netip" @@ -670,3 +671,98 @@ func testNodeBackendMagicDNSHosts(t *testing.T, magicDNSEnabled bool) { t.Errorf("magicDNSPTR(100.64.0.3) after rename = %q, %v; want p3's new name", fqdn, ok) } } + +// TestNodeBackendIndexReuseEviction exercises netmap delta orderings in +// which one peer's address, name, or key index entry is claimed by a +// second peer before the first peer's entries are evicted. The eviction +// must keep the second peer's entries, or lookups by IP (WhoIs, and thus +// PeerAPI and App Connector DNS) would fail until the next full netmap, +// even though the peers map and the WireGuard config remain correct. +func TestNodeBackendIndexReuseEviction(t *testing.T) { + addr := netip.MustParseAddr("100.64.0.1") + mkPeer := func(id tailcfg.NodeID, a netip.Addr) tailcfg.NodeView { + return (&tailcfg.Node{ + ID: id, + StableID: tailcfg.StableNodeID(fmt.Sprintf("stable%d", id)), + Key: makeNodeKeyFromID(id), + Name: "runner.example.ts.net.", + HomeDERP: 1, + Addresses: []netip.Prefix{netip.PrefixFrom(a, a.BitLen())}, + }).View() + } + newBackend := func(t *testing.T, initial ...tailcfg.NodeView) *nodeBackend { + nb := newNodeBackend(t.Context(), tstest.WhileTestRunningLogger(t), eventbus.New()) + nb.SetNetMap(&netmap.NetworkMap{Peers: initial}) + return nb + } + apply := func(t *testing.T, nb *nodeBackend, muts ...netmap.NodeMutation) { + t.Helper() + if _, handled := nb.UpdateNetmapDelta(muts); !handled { + t.Fatal("UpdateNetmapDelta not handled") + } + } + wantAddr := func(t *testing.T, nb *nodeBackend, a netip.Addr, want tailcfg.NodeID) { + t.Helper() + got, ok := nb.NodeByAddr(a) + if want == 0 { + if ok { + t.Errorf("NodeByAddr(%v) = %v; want no match", a, got) + } + return + } + if !ok || got != want { + t.Errorf("NodeByAddr(%v) = %v, %v; want %v", a, got, ok, want) + } + } + + t.Run("remove-after-reuse", func(t *testing.T) { + // Peer 1 owns the address. Control reassigns it (and the + // MagicDNS name) to new peer 2 in one delta batch and removes + // peer 1 in a later batch, as happens with churning ephemeral + // peers. The removal of peer 1 must not evict peer 2's claims. + nb := newBackend(t, mkPeer(1, addr)) + apply(t, nb, netmap.NodeMutationUpsert{Node: mkPeer(2, addr)}) + apply(t, nb, netmap.MakeNodeMutationRemove(1)) + wantAddr(t, nb, addr, 2) + if nid, ok := nb.NodeByName("runner.example.ts.net"); !ok || nid != 2 { + t.Errorf("NodeByName = %v, %v; want 2", nid, ok) + } + if nid, ok := nb.NodeByKey(makeNodeKeyFromID(2)); !ok || nid != 2 { + t.Errorf("NodeByKey(peer 2) = %v, %v; want 2", nid, ok) + } + if nid, ok := nb.NodeByKey(makeNodeKeyFromID(1)); ok { + t.Errorf("NodeByKey(peer 1) = %v; want no match after removal", nid) + } + // Removing the current owner still evicts. + apply(t, nb, netmap.MakeNodeMutationRemove(2)) + wantAddr(t, nb, addr, 0) + }) + + t.Run("single-response-sort-order", func(t *testing.T) { + // Within one MapResponse, MutationsFromMapResponse sorts by + // NodeID, which can order the upsert of the address's new + // owner before the removal of its old owner. + nb := newBackend(t, mkPeer(10, addr)) + muts, ok := netmap.MutationsFromMapResponse(&tailcfg.MapResponse{ + PeersRemoved: []tailcfg.NodeID{10}, + PeersChanged: []*tailcfg.Node{mkPeer(2, addr).AsStruct()}, + }, time.Unix(123, 0)) + if !ok { + t.Fatal("MutationsFromMapResponse failed") + } + apply(t, nb, muts...) + wantAddr(t, nb, addr, 2) + }) + + t.Run("upsert-eviction", func(t *testing.T) { + // Peer 2 claims peer 1's address. A later upsert of peer 1 + // with a new address evicts entries derived from peer 1's old + // value, which must not include peer 2's claim. + nb := newBackend(t, mkPeer(1, addr)) + apply(t, nb, netmap.NodeMutationUpsert{Node: mkPeer(2, addr)}) + addr2 := netip.MustParseAddr("100.64.0.9") + apply(t, nb, netmap.NodeMutationUpsert{Node: mkPeer(1, addr2)}) + wantAddr(t, nb, addr, 2) + wantAddr(t, nb, addr2, 1) + }) +} diff --git a/tstest/integration/netmapdelta_test.go b/tstest/integration/netmapdelta_test.go new file mode 100644 index 000000000..936c4d2a0 --- /dev/null +++ b/tstest/integration/netmapdelta_test.go @@ -0,0 +1,215 @@ +// Copyright (c) Tailscale Inc & contributors +// SPDX-License-Identifier: BSD-3-Clause + +package integration + +import ( + "encoding/json" + "errors" + "fmt" + "net/netip" + "testing" + "time" + + "tailscale.com/tailcfg" + "tailscale.com/tstest" + "tailscale.com/types/key" +) + +// TestWhoIsAfterPeerAddressReuse drives the whole client stack through the +// netmap delta ordering that broke WhoIs on App Connectors +// (tailscale/corp#47435). +// +// Control reassigns a churning ephemeral peer's Tailscale IP to a newer peer. +// The newer peer's upsert can reach the client before the older peer's +// removal, either in an earlier MapResponse or, as here, reordered within one +// MapResponse by the NodeID sort in netmap.MutationsFromMapResponse. Evicting +// the older peer's index entries unconditionally then wiped the address entry +// the newer peer had just claimed. +// +// n1 plays the App Connector. n2 plays the peer that ends up owning the +// address. n2 stays up throughout, so its WireGuard session with n1 keeps +// working while n1 can no longer say who owns its address. +// +// nodeBackend's own test covers the index bookkeeping directly. This test adds +// the two layers above it: that a MapResponse reusing an address is handled +// incrementally rather than as a full netmap, and that LocalBackend.WhoIs, the +// call PeerAPI makes before it accepts a connection, still resolves the +// address afterwards. +func TestWhoIsAfterPeerAddressReuse(t *testing.T) { + tstest.Parallel(t) + env := NewTestEnv(t) + + newNode := func() (*TestNode, key.NodePublic) { + n := NewTestNode(t, env) + n.StartDaemon() + n.AwaitListening() + n.MustUp() + n.AwaitRunning() + return n, n.MustStatus().Self.PublicKey + } + n1, k1 := newNode() + n2, k2 := newNode() + + n1IP := n1.AwaitIP4() + n2IP := n2.AwaitIP4() + t.Logf("connector n1 = %v %v", k1.ShortString(), n1IP) + t.Logf("peer n2 = %v %v", k2.ShortString(), n2IP) + + // whoIsOwner reports which node key n1 believes owns n2's address. It runs + // the same lookup peerAPIListener.ServeConn makes before accepting a + // connection. + whoIsOwner := func() (key.NodePublic, error) { + who, err := n1.LocalClient().WhoIs(t.Context(), n2IP.String()) + if err != nil { + return key.NodePublic{}, err + } + if who.Node == nil { + return key.NodePublic{}, errors.New("nil Node") + } + if who.UserProfile == nil || who.UserProfile.ID == 0 { + return key.NodePublic{}, fmt.Errorf("no user profile: %+v", who.UserProfile) + } + return who.Node.Key, nil + } + wantOwner := func(t *testing.T, want key.NodePublic, whose string) { + t.Helper() + got, err := whoIsOwner() + if err != nil { + t.Fatalf("n1 WhoIs(%v): %v; want %s", n2IP, err, whose) + } + if got != want { + t.Fatalf("n1 WhoIs(%v) = %v; want %s %v", n2IP, got.ShortString(), whose, want.ShortString()) + } + } + awaitPeer := func(t *testing.T, k key.NodePublic, want bool) { + t.Helper() + if err := tstest.WaitFor(30*time.Second, func() error { + _, ok := n1.MustStatus().Peer[k] + if ok != want { + return fmt.Errorf("n1 has peer %v = %v; want %v", k.ShortString(), ok, want) + } + return nil + }); err != nil { + t.Fatal(err) + } + } + + if err := tstest.WaitFor(30*time.Second, func() error { + _, err := whoIsOwner() + return err + }); err != nil { + t.Fatalf("baseline WhoIs(%v) on n1: %v", n2IP, err) + } + wantOwner(t, k2, "n2") + t.Logf("baseline ok: n1 says %v owns %v", k2.ShortString(), n2IP) + + // oldOwner stands in for the ephemeral peer that held n2's address before + // control reassigned it. Its user is n2's user, so the full netmap below + // keeps carrying that user's profile. Without that, the netmap would also + // drop the profile and WhoIs would fail for an unrelated reason. + n2User := n2.MustStatus().Self.UserID + oldOwner := &tailcfg.Node{ + ID: 12345, + StableID: "TESTOLDOWNER", + Name: "old-owner.fake-control.example.net.", + User: n2User, + Key: key.NewNode().Public(), + Machine: key.NewMachine().Public(), + DiscoKey: key.NewDisco().Public(), + MachineAuthorized: true, + Addresses: []netip.Prefix{netip.PrefixFrom(n2IP, n2IP.BitLen())}, + AllowedIPs: []netip.Prefix{netip.PrefixFrom(n2IP, n2IP.BitLen())}, + } + + // Step 1: a full netmap in which the address belongs to oldOwner. Setting + // Peers makes the client treat this as a complete peer set, so it drops + // n2. AddRawMapResponse also stops every later automatic map response to + // n1, which is what keeps a full netmap from rebuilding n1's indexes + // behind the test's back. + if !env.Control.AddRawMapResponse(k1, &tailcfg.MapResponse{ + Peers: []*tailcfg.Node{oldOwner}, + }) { + t.Fatal("AddRawMapResponse(full netmap) not delivered") + } + awaitPeer(t, k2, false) + wantOwner(t, oldOwner.Key, "oldOwner") + t.Logf("step 1: n1 says %v owns %v", oldOwner.Key.ShortString(), n2IP) + + // The node control hands back has to carry the disco key n2 is actually + // using. A peer that arrives with a stale one makes n1 learn the current + // key over TSMP, and that path rebuilds n1's whole netmap, which would + // rebuild the indexes and hide the bug. n2 pushes its disco key on a + // separate non-streaming poll, so wait for control to catch up. + var n2node *tailcfg.Node + if err := tstest.WaitFor(30*time.Second, func() error { + want := selfDiscoKey(t, n2) + n2node = env.Control.Node(k2) + if n2node == nil { + return errors.New("control has no node for n2") + } + if n2node.DiscoKey != want { + return fmt.Errorf("control has disco key %v for n2; n2 is using %v", + n2node.DiscoKey.ShortString(), want.ShortString()) + } + return nil + }); err != nil { + t.Fatalf("waiting for control to learn n2's disco key: %v", err) + } + + // Real control marks a connected node online, and that matters here. + // removeUnwantedDiscoUpdates drops a TSMP-learned disco key for a peer + // that is already online with that same key, and dropping it is what stops + // the next WireGuard handshake from making n1 rebuild its full netmap. + n2node.Online = new(true) + n2node.LastSeen = new(time.Now()) + + // Step 2: one MapResponse that hands the address to n2 and removes + // oldOwner. MutationsFromMapResponse sorts by NodeID, and n2's ID is far + // below oldOwner's, so the client applies n2's upsert first and then + // oldOwner's removal. The removal must leave n2's claim on the address + // alone. + if !env.Control.AddRawMapResponse(k1, &tailcfg.MapResponse{ + PeersChanged: []*tailcfg.Node{n2node}, + PeersRemoved: []tailcfg.NodeID{oldOwner.ID}, + }) { + t.Fatal("AddRawMapResponse(delta) not delivered") + } + awaitPeer(t, k2, true) + t.Logf("step 2: n1 re-added n2 and removed oldOwner") + + // n1 must now name n2 as the owner of the address. + wantOwner(t, k2, "n2") + + // And the data path has to still work, so a failure above is a failure to + // identify a peer n1 is actively exchanging traffic with, not a peer it + // has genuinely lost. + if err := tstest.WaitFor(30*time.Second, func() error { + out, err := n2.TailscaleForOutput("ping", "-c", "1", "--timeout=5s", "--tsmp", n1IP.String()).CombinedOutput() + if err != nil { + return fmt.Errorf("%v: %s", err, out) + } + return nil + }); err != nil { + t.Errorf("tsmp ping n2->n1: %v", err) + } + wantOwner(t, k2, "n2") +} + +// selfDiscoKey returns the disco key n is currently advertising for itself. +func selfDiscoKey(t *testing.T, n *TestNode) key.DiscoPublic { + t.Helper() + out, err := n.TailscaleForOutput("debug", "netmap").Output() + if err != nil { + t.Fatalf("debug netmap: %v", err) + } + var nm struct { + SelfNode struct { + DiscoKey key.DiscoPublic + } + } + if err := json.Unmarshal(out, &nm); err != nil { + t.Fatalf("unmarshal netmap: %v", err) + } + return nm.SelfNode.DiscoKey +}