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 +}