From 384610f81ac7b41b9bd80ce260d8493250436f2a Mon Sep 17 00:00:00 2001 From: Nate Brown Date: Thu, 9 Jul 2026 09:34:30 -0500 Subject: [PATCH] hostmap: replace the shared next/prev hostinfo chain with independent per-address lists so divergent or overlapping vpnAddr sets cannot corrupt the map (#1790) --- handshake_manager.go | 7 +- hostmap.go | 242 +++++++++++++++---------- hostmap_test.go | 418 ++++++++++++++++++++++++------------------- 3 files changed, 386 insertions(+), 281 deletions(-) diff --git a/handshake_manager.go b/handshake_manager.go index 0d25305f..913918c2 100644 --- a/handshake_manager.go +++ b/handshake_manager.go @@ -430,14 +430,11 @@ func (hm *HandshakeManager) CheckAndComplete(hostinfo *HostInfo, handshakePacket // Check if we already have a tunnel with this vpn ip existingHostInfo, found := hm.mainHostMap.Hosts[hostinfo.vpnAddrs[0]] if found && existingHostInfo != nil { - testHostInfo := existingHostInfo - for testHostInfo != nil { - // Is it just a delayed handshake packet? + // Is it just a delayed handshake packet? Check every hostinfo we hold for this address. + for _, testHostInfo := range hm.mainHostMap.unlockedGetHostList(hostinfo.vpnAddrs[0]) { if bytes.Equal(hostinfo.HandshakePacket[handshakePacket], testHostInfo.HandshakePacket[handshakePacket]) { return testHostInfo, ErrAlreadySeen } - - testHostInfo = testHostInfo.next } // Is this a newer handshake? diff --git a/hostmap.go b/hostmap.go index e7399034..2f2db101 100644 --- a/hostmap.go +++ b/hostmap.go @@ -56,11 +56,20 @@ type Relay struct { } type HostMap struct { - sync.RWMutex //Because we concurrently read and write to our maps - Indexes map[uint32]*HostInfo - Relays map[uint32]*HostInfo // Maps a Relay IDX to a Relay HostInfo object - RemoteIndexes map[uint32]*HostInfo + sync.RWMutex //Because we concurrently read and write to our maps + Indexes map[uint32]*HostInfo + Relays map[uint32]*HostInfo // Maps a Relay IDX to a Relay HostInfo object + RemoteIndexes map[uint32]*HostInfo + // Hosts maps a vpn address to its primary hostinfo, one entry per address we hold a tunnel + // for. moreHosts only has an entry while an address is held by 2 or more hostinfos and stores + // the full most-recent-first list; moreHosts[a][0] is always the same hostinfo as Hosts[a]. + // Each address gets its own independent list, so a hostinfo owning multiple addresses can + // never corrupt another address's ordering the way the old shared next/prev chain could. + // Entries in moreHosts are only ever written by unlockedSetHostsForAddr; Hosts is written + // directly only in the single-hostinfo fast paths where moreHosts is known to have no entry, + // and unlockedDeleteHostInfo swaps either map for a fresh one when it fully drains. Hosts map[netip.Addr]*HostInfo + moreHosts map[netip.Addr][]*HostInfo preferredRanges atomic.Pointer[[]netip.Prefix] l *slog.Logger } @@ -266,10 +275,6 @@ type HostInfo struct { lastRoam time.Time lastRoamRemote netip.AddrPort - // Used to track other hostinfos for this vpn ip since only 1 can be primary - // Synchronised via hostmap lock and not the hostinfo lock. - next, prev *HostInfo - //TODO: in, out, and others might benefit from being an atomic.Int32. We could collapse connectionManager pendingDeletion, relayUsed, and in/out into this 1 thing in, out, pendingDeletion atomic.Bool @@ -334,6 +339,7 @@ func newHostMap(l *slog.Logger) *HostMap { Relays: map[uint32]*HostInfo{}, RemoteIndexes: map[uint32]*HostInfo{}, Hosts: map[netip.Addr]*HostInfo{}, + moreHosts: map[netip.Addr][]*HostInfo{}, l: l, } } @@ -382,13 +388,55 @@ func (hm *HostMap) EmitStats() { metrics.GetOrRegisterGauge("hostmap.main.relayIndexes", nil).Update(int64(relaysLen)) } -// DeleteHostInfo will fully unlink the hostinfo and return true if it was the final hostinfo for this vpn ip +// unlockedSetHostsForAddr stores the per-address hostinfo list (list[0] is the primary). An empty +// list removes the address. This is the one place Hosts and moreHosts are written together, keep +// it that way. Callers must hold the write lock. +func (hm *HostMap) unlockedSetHostsForAddr(addr netip.Addr, list []*HostInfo) { + if len(list) == 0 { + delete(hm.Hosts, addr) + delete(hm.moreHosts, addr) + return + } + hm.Hosts[addr] = list[0] + if len(list) > 1 { + hm.moreHosts[addr] = list + } else { + delete(hm.moreHosts, addr) + } +} + +// unlockedGetHostList returns every hostinfo holding addr, primary first, or nil if we have no +// tunnel for addr. The common single-hostinfo case builds a fresh one element list, so keep this +// off the packet hot path; the primary is a direct Hosts read. Callers must hold the lock (read +// or write). +func (hm *HostMap) unlockedGetHostList(addr netip.Addr) []*HostInfo { + if list, ok := hm.moreHosts[addr]; ok { + return list + } + if h, ok := hm.Hosts[addr]; ok { + return []*HostInfo{h} + } + return nil +} + +// removeHostInfo returns list with hi removed (order preserved), or list unchanged if hi is +// absent. It deletes in place: every mutator holds the hostmap write lock and no reader ever +// retains a slice across a mutation (readers iterate under RLock), so there is no snapshot to +// invalidate. +func removeHostInfo(list []*HostInfo, hi *HostInfo) []*HostInfo { + idx := slices.Index(list, hi) + if idx < 0 { + return list + } + return slices.Delete(list, idx, idx+1) +} + +// DeleteHostInfo will fully unlink the hostinfo and return true if no other hostinfo still holds +// any of its vpn addrs, meaning we no longer have a tunnel to the peer func (hm *HostMap) DeleteHostInfo(hostinfo *HostInfo) bool { // Delete the host itself, ensuring it's not modified anymore hm.Lock() - // If we have a previous or next hostinfo then we are not the last one for this vpn ip - final := (hostinfo.next == nil && hostinfo.prev == nil) - hm.unlockedDeleteHostInfo(hostinfo) + final := hm.unlockedDeleteHostInfo(hostinfo) hm.Unlock() return final @@ -401,70 +449,62 @@ func (hm *HostMap) MakePrimary(hostinfo *HostInfo) { } func (hm *HostMap) unlockedMakePrimary(hostinfo *HostInfo) { - // Get the current primary, if it exists - oldHostinfo := hm.Hosts[hostinfo.vpnAddrs[0]] - - // Every address in the hostinfo gets elevated to primary - for _, vpnAddr := range hostinfo.vpnAddrs { - //NOTE: It is possible that we leave a dangling hostinfo here but connection manager works on - // indexes so it should be fine. - hm.Hosts[vpnAddr] = hostinfo - } - - // If we are already primary then we won't bother re-linking - if oldHostinfo == hostinfo { + // A hostinfo that is no longer in the hostmap must not be re-inserted here. Callers can race + // tunnel teardown, deciding to promote under the read lock and only taking the write lock + // after a delete fully unlinked the hostinfo (connection manager swapPrimary, AddRelay). Every + // live hostinfo is registered in Indexes by unlockedAddHostInfo, so this is a membership test. + if hm.Indexes[hostinfo.localIndexId] != hostinfo { return } - // Unlink this hostinfo - if hostinfo.prev != nil { - hostinfo.prev.next = hostinfo.next - } - if hostinfo.next != nil { - hostinfo.next.prev = hostinfo.prev - } - - // If there wasn't a previous primary then clear out any links - if oldHostinfo == nil { - hostinfo.next = nil - hostinfo.prev = nil - return - } - - // Relink the hostinfo as primary - hostinfo.next = oldHostinfo - oldHostinfo.prev = hostinfo - hostinfo.prev = nil -} - -func (hm *HostMap) unlockedDeleteHostInfo(hostinfo *HostInfo) { - isLastHostinfo := hostinfo.next == nil && hostinfo.prev == nil - + // Move hostinfo to the front (primary) of each of its address lists. The lists are + // independent per address, so this can never leave a dangling entry the way promoting + // against a single shared chain could. for _, addr := range hostinfo.vpnAddrs { - if hm.Hosts[addr] != hostinfo { + if hm.Hosts[addr] == hostinfo { + // Already primary for this address, the list is already in the right order continue } - if hostinfo.next != nil { - // Promote the next hostinfo in the shared chain to primary for this address - hm.Hosts[addr] = hostinfo.next - } else { - delete(hm.Hosts, addr) + list := removeHostInfo(hm.unlockedGetHostList(addr), hostinfo) + list = append([]*HostInfo{hostinfo}, list...) + hm.unlockedSetHostsForAddr(addr, list) + } +} + +// unlockedDeleteHostInfo removes hostinfo from every one of its address lists and from the index +// maps. It returns true if this was the last hostinfo for all of its addresses (we no longer have +// any tunnel to the peer), which the caller uses to decide whether to clear learned lighthouse +// state and disestablish relays. +func (hm *HostMap) unlockedDeleteHostInfo(hostinfo *HostInfo) bool { + // Remove this hostinfo from each of its address lists. The lists are independent, so a + // sibling is never promoted to an address it does not own and no other list is touched. + final := true + for _, addr := range hostinfo.vpnAddrs { + if list, ok := hm.moreHosts[addr]; ok { + list = removeHostInfo(list, hostinfo) + hm.unlockedSetHostsForAddr(addr, list) + if len(list) > 0 { + final = false + } + } else if existing, ok := hm.Hosts[addr]; ok { + if existing == hostinfo { + // Common case, the only hostinfo for this address. moreHosts has no entry to clean up. + delete(hm.Hosts, addr) + } else { + // We don't hold this address but another hostinfo does, we still have a tunnel to the peer + final = false + } } } + + // Go maps never shrink their buckets, replace fully drained maps so a node that churned + // through a large peer count gives the memory back. Same idiom as the index maps below. if len(hm.Hosts) == 0 { hm.Hosts = map[netip.Addr]*HostInfo{} } - - // Splice this hostinfo out of the shared chain exactly once - if hostinfo.prev != nil { - hostinfo.prev.next = hostinfo.next + if len(hm.moreHosts) == 0 { + hm.moreHosts = map[netip.Addr][]*HostInfo{} } - if hostinfo.next != nil { - hostinfo.next.prev = hostinfo.prev - } - - hostinfo.next = nil - hostinfo.prev = nil // The remote index uses index ids outside our control so lets make sure we are only removing // the remote index pointer here if it points to the hostinfo we are deleting @@ -488,7 +528,7 @@ func (hm *HostMap) unlockedDeleteHostInfo(hostinfo *HostInfo) { ) } - if isLastHostinfo { + if final { // I have lost connectivity to my peers. My relay tunnel is likely broken. Mark the next // hops as 'Requested' so that new relay tunnels are created in the future. hm.unlockedDisestablishVpnAddrRelayFor(hostinfo) @@ -497,6 +537,8 @@ func (hm *HostMap) unlockedDeleteHostInfo(hostinfo *HostInfo) { for _, localRelayIdx := range hostinfo.relayState.CopyRelayForIdxs() { delete(hm.Relays, localRelayIdx) } + + return final } func (hm *HostMap) QueryIndex(index uint32) *HostInfo { @@ -540,19 +582,30 @@ func (hm *HostMap) QueryVpnAddrsRelayFor(targetIps []netip.Addr, relayHostIp net hm.RLock() defer hm.RUnlock() + // This runs per relayed packet, so check the primary with a single map probe and only consult + // moreHosts when the primary can't relay for us. h, ok := hm.Hosts[relayHostIp] if !ok { return nil, nil, errors.New("unable to find host") } - for h != nil { - for _, targetIp := range targetIps { - r, ok := h.relayState.QueryRelayForByIp(targetIp) - if ok && r.State == Established { - return h, r, nil + for _, targetIp := range targetIps { + r, ok := h.relayState.QueryRelayForByIp(targetIp) + if ok && r.State == Established { + return h, r, nil + } + } + + if list, ok := hm.moreHosts[relayHostIp]; ok { + // list[0] is the primary we already checked + for _, h := range list[1:] { + for _, targetIp := range targetIps { + r, ok := h.relayState.QueryRelayForByIp(targetIp) + if ok && r.State == Established { + return h, r, nil + } } } - h = h.next } return nil, nil, errors.New("unable to find host with relay") @@ -560,20 +613,14 @@ func (hm *HostMap) QueryVpnAddrsRelayFor(targetIps []netip.Addr, relayHostIp net func (hm *HostMap) unlockedDisestablishVpnAddrRelayFor(hi *HostInfo) { for _, relayHostIp := range hi.relayState.CopyRelayIps() { - if h, ok := hm.Hosts[relayHostIp]; ok { - for h != nil { - h.relayState.UpdateRelayForByIpState(hi.vpnAddrs[0], Disestablished) - h = h.next - } + for _, h := range hm.unlockedGetHostList(relayHostIp) { + h.relayState.UpdateRelayForByIpState(hi.vpnAddrs[0], Disestablished) } } for _, rs := range hi.relayState.CopyAllRelayFor() { if rs.Type == ForwardingType { - if h, ok := hm.Hosts[rs.PeerAddr]; ok { - for h != nil { - h.relayState.UpdateRelayForByIpState(hi.vpnAddrs[0], Disestablished) - h = h.next - } + for _, h := range hm.unlockedGetHostList(rs.PeerAddr) { + h.relayState.UpdateRelayForByIpState(hi.vpnAddrs[0], Disestablished) } } } @@ -623,22 +670,27 @@ func (hm *HostMap) unlockedAddHostInfo(hostinfo *HostInfo, f *Interface) { } func (hm *HostMap) unlockedInnerAddHostInfo(vpnAddr netip.Addr, hostinfo *HostInfo, f *Interface) { - existing := hm.Hosts[vpnAddr] - hm.Hosts[vpnAddr] = hostinfo - - if existing != nil && existing != hostinfo { - hostinfo.next = existing - existing.prev = hostinfo + existing, ok := hm.Hosts[vpnAddr] + if !ok { + // Common case, the first hostinfo for this address. moreHosts stays empty. + hm.Hosts[vpnAddr] = hostinfo + return } - i := 1 - check := hostinfo - for check != nil { - if i > MaxHostInfosPerVpnIp { - hm.unlockedDeleteHostInfo(check) - } - check = check.next - i++ + // The new hostinfo becomes the primary for this address. Remove any stale copy of it first so + // we never hold a duplicate, then prepend. + list, ok := hm.moreHosts[vpnAddr] + if !ok { + list = []*HostInfo{existing} + } + list = removeHostInfo(list, hostinfo) + list = append([]*HostInfo{hostinfo}, list...) + hm.unlockedSetHostsForAddr(vpnAddr, list) + + // Enforce the per-address cap by fully retiring the oldest hostinfo once we exceed it. + // Deleting it removes it from all of its addresses and the index maps, matching prior behavior. + if len(list) > MaxHostInfosPerVpnIp { + hm.unlockedDeleteHostInfo(list[len(list)-1]) } } diff --git a/hostmap_test.go b/hostmap_test.go index 156444a3..9cfebe17 100644 --- a/hostmap_test.go +++ b/hostmap_test.go @@ -2,6 +2,7 @@ package nebula import ( "net/netip" + "slices" "testing" "github.com/slackhq/nebula/config" @@ -10,78 +11,84 @@ import ( "github.com/stretchr/testify/require" ) +// chainIds returns the localIndexIds of the hostinfos holding addr, primary (index 0) first. It +// also validates the Hosts/moreHosts sync contract on every call so a mutation that broke it +// fails fast. +func chainIds(t *testing.T, hm *HostMap, addr netip.Addr) []uint32 { + t.Helper() + assertHostMapInvariants(t, hm) + list := hm.unlockedGetHostList(addr) + ids := make([]uint32, len(list)) + for i, h := range list { + ids[i] = h.localIndexId + } + return ids +} + +// assertHostMapInvariants checks the Hosts/moreHosts contract: moreHosts only holds addresses +// with 2 or more hostinfos, its first entry is always the primary in Hosts, lists never hold +// duplicates, every hostinfo in a list owns the address and is registered in Indexes, and every +// indexed hostinfo is reachable through each of its addresses. +func assertHostMapInvariants(t *testing.T, hm *HostMap) { + t.Helper() + for addr, list := range hm.moreHosts { + require.GreaterOrEqualf(t, len(list), 2, "moreHosts[%s] must hold at least 2 hostinfos", addr) + require.Samef(t, hm.Hosts[addr], list[0], "moreHosts[%s][0] must match the primary in Hosts", addr) + seen := map[*HostInfo]bool{} + for _, h := range list { + require.NotNilf(t, h, "moreHosts[%s] must never hold a nil hostinfo", addr) + require.Falsef(t, seen[h], "moreHosts[%s] holds hostinfo %d twice", addr, h.localIndexId) + seen[h] = true + require.Samef(t, hm.Indexes[h.localIndexId], h, "moreHosts[%s] member %d is not registered in Indexes", addr, h.localIndexId) + require.Truef(t, slices.Contains(h.vpnAddrs, addr), "moreHosts[%s] member %d does not own the address", addr, h.localIndexId) + } + } + for addr, h := range hm.Hosts { + require.NotNilf(t, h, "Hosts[%s] must never be nil", addr) + require.Samef(t, hm.Indexes[h.localIndexId], h, "Hosts[%s] primary %d is not registered in Indexes", addr, h.localIndexId) + require.Truef(t, slices.Contains(h.vpnAddrs, addr), "Hosts[%s] primary (index %d) does not own the address", addr, h.localIndexId) + } + for idx, h := range hm.Indexes { + require.Equalf(t, idx, h.localIndexId, "Indexes[%d] holds hostinfo with localIndexId %d", idx, h.localIndexId) + for _, va := range h.vpnAddrs { + require.Truef(t, slices.Contains(hm.unlockedGetHostList(va), h), "indexed hostinfo %d is missing from the list for %s", idx, va) + } + } +} + func TestHostMap_MakePrimary(t *testing.T) { l := test.NewLogger() hm := newHostMap(l) f := &Interface{} + a := netip.MustParseAddr("0.0.0.1") - h1 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 1} - h2 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 2} - h3 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 3} - h4 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 4} + h1 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 1} + h2 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 2} + h3 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 3} + h4 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 4} hm.unlockedAddHostInfo(h4, f) hm.unlockedAddHostInfo(h3, f) hm.unlockedAddHostInfo(h2, f) hm.unlockedAddHostInfo(h1, f) - // Make sure we go h1 -> h2 -> h3 -> h4 - prim := hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1")) - assert.Equal(t, h1.localIndexId, prim.localIndexId) - assert.Equal(t, h2.localIndexId, prim.next.localIndexId) - assert.Nil(t, prim.prev) - assert.Equal(t, h1.localIndexId, h2.prev.localIndexId) - assert.Equal(t, h3.localIndexId, h2.next.localIndexId) - assert.Equal(t, h2.localIndexId, h3.prev.localIndexId) - assert.Equal(t, h4.localIndexId, h3.next.localIndexId) - assert.Equal(t, h3.localIndexId, h4.prev.localIndexId) - assert.Nil(t, h4.next) + // Most-recently-added is primary: h1, h2, h3, h4 + assert.Equal(t, []uint32{1, 2, 3, 4}, chainIds(t, hm, a)) + assert.Equal(t, h1, hm.QueryVpnAddr(a)) - // Swap h3/middle to primary + // Swap the middle to primary: h3, h1, h2, h4 hm.MakePrimary(h3) + assert.Equal(t, []uint32{3, 1, 2, 4}, chainIds(t, hm, a)) + assert.Equal(t, h3, hm.QueryVpnAddr(a)) - // Make sure we go h3 -> h1 -> h2 -> h4 - prim = hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1")) - assert.Equal(t, h3.localIndexId, prim.localIndexId) - assert.Equal(t, h1.localIndexId, prim.next.localIndexId) - assert.Nil(t, prim.prev) - assert.Equal(t, h2.localIndexId, h1.next.localIndexId) - assert.Equal(t, h3.localIndexId, h1.prev.localIndexId) - assert.Equal(t, h4.localIndexId, h2.next.localIndexId) - assert.Equal(t, h1.localIndexId, h2.prev.localIndexId) - assert.Equal(t, h2.localIndexId, h4.prev.localIndexId) - assert.Nil(t, h4.next) - - // Swap h4/tail to primary + // Swap the tail to primary: h4, h3, h1, h2 hm.MakePrimary(h4) + assert.Equal(t, []uint32{4, 3, 1, 2}, chainIds(t, hm, a)) - // Make sure we go h4 -> h3 -> h1 -> h2 - prim = hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1")) - assert.Equal(t, h4.localIndexId, prim.localIndexId) - assert.Equal(t, h3.localIndexId, prim.next.localIndexId) - assert.Nil(t, prim.prev) - assert.Equal(t, h1.localIndexId, h3.next.localIndexId) - assert.Equal(t, h4.localIndexId, h3.prev.localIndexId) - assert.Equal(t, h2.localIndexId, h1.next.localIndexId) - assert.Equal(t, h3.localIndexId, h1.prev.localIndexId) - assert.Equal(t, h1.localIndexId, h2.prev.localIndexId) - assert.Nil(t, h2.next) - - // Swap h4 again should be no-op + // Swapping the current primary again is a no-op hm.MakePrimary(h4) - - // Make sure we go h4 -> h3 -> h1 -> h2 - prim = hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1")) - assert.Equal(t, h4.localIndexId, prim.localIndexId) - assert.Equal(t, h3.localIndexId, prim.next.localIndexId) - assert.Nil(t, prim.prev) - assert.Equal(t, h1.localIndexId, h3.next.localIndexId) - assert.Equal(t, h4.localIndexId, h3.prev.localIndexId) - assert.Equal(t, h2.localIndexId, h1.next.localIndexId) - assert.Equal(t, h3.localIndexId, h1.prev.localIndexId) - assert.Equal(t, h1.localIndexId, h2.prev.localIndexId) - assert.Nil(t, h2.next) + assert.Equal(t, []uint32{4, 3, 1, 2}, chainIds(t, hm, a)) } func TestHostMap_DeleteHostInfo(t *testing.T) { @@ -89,13 +96,14 @@ func TestHostMap_DeleteHostInfo(t *testing.T) { hm := newHostMap(l) f := &Interface{} + a := netip.MustParseAddr("0.0.0.1") - h1 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 1} - h2 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 2} - h3 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 3} - h4 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 4} - h5 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 5} - h6 := &HostInfo{vpnAddrs: []netip.Addr{netip.MustParseAddr("0.0.0.1")}, localIndexId: 6} + h1 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 1} + h2 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 2} + h3 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 3} + h4 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 4} + h5 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 5} + h6 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 6} hm.unlockedAddHostInfo(h6, f) hm.unlockedAddHostInfo(h5, f) @@ -104,94 +112,110 @@ func TestHostMap_DeleteHostInfo(t *testing.T) { hm.unlockedAddHostInfo(h2, f) hm.unlockedAddHostInfo(h1, f) - // h6 should be deleted - assert.Nil(t, h6.next) - assert.Nil(t, h6.prev) - h := hm.QueryIndex(h6.localIndexId) - assert.Nil(t, h) + // h6 is evicted by the MaxHostInfosPerVpnIp cap; the rest are newest-first. + assert.Nil(t, hm.QueryIndex(h6.localIndexId)) + assert.Equal(t, []uint32{1, 2, 3, 4, 5}, chainIds(t, hm, a)) - // Make sure we go h1 -> h2 -> h3 -> h4 -> h5 - prim := hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1")) - assert.Equal(t, h1.localIndexId, prim.localIndexId) - assert.Equal(t, h2.localIndexId, prim.next.localIndexId) - assert.Nil(t, prim.prev) - assert.Equal(t, h1.localIndexId, h2.prev.localIndexId) - assert.Equal(t, h3.localIndexId, h2.next.localIndexId) - assert.Equal(t, h2.localIndexId, h3.prev.localIndexId) - assert.Equal(t, h4.localIndexId, h3.next.localIndexId) - assert.Equal(t, h3.localIndexId, h4.prev.localIndexId) - assert.Equal(t, h5.localIndexId, h4.next.localIndexId) - assert.Equal(t, h4.localIndexId, h5.prev.localIndexId) - assert.Nil(t, h5.next) + // Delete primary; not final since siblings remain. + assert.False(t, hm.DeleteHostInfo(h1)) + assert.Equal(t, []uint32{2, 3, 4, 5}, chainIds(t, hm, a)) - // Delete primary - hm.DeleteHostInfo(h1) - assert.Nil(t, h1.prev) - assert.Nil(t, h1.next) + // Deleting the same hostinfo again must not report final while siblings remain and must not + // disturb the list. The old chain code got this wrong: the first delete nil'd next/prev, so a + // second delete looked final and wiped lighthouse state out from under the live sibling. + assert.False(t, hm.DeleteHostInfo(h1)) + assert.Equal(t, []uint32{2, 3, 4, 5}, chainIds(t, hm, a)) - // Make sure we go h2 -> h3 -> h4 -> h5 - prim = hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1")) - assert.Equal(t, h2.localIndexId, prim.localIndexId) - assert.Equal(t, h3.localIndexId, prim.next.localIndexId) - assert.Nil(t, prim.prev) - assert.Equal(t, h3.localIndexId, h2.next.localIndexId) - assert.Equal(t, h2.localIndexId, h3.prev.localIndexId) - assert.Equal(t, h4.localIndexId, h3.next.localIndexId) - assert.Equal(t, h3.localIndexId, h4.prev.localIndexId) - assert.Equal(t, h5.localIndexId, h4.next.localIndexId) - assert.Equal(t, h4.localIndexId, h5.prev.localIndexId) - assert.Nil(t, h5.next) + // Delete a middle node. + assert.False(t, hm.DeleteHostInfo(h3)) + assert.Equal(t, []uint32{2, 4, 5}, chainIds(t, hm, a)) - // Delete in the middle - hm.DeleteHostInfo(h3) - assert.Nil(t, h3.prev) - assert.Nil(t, h3.next) + // Delete the tail. + assert.False(t, hm.DeleteHostInfo(h5)) + assert.Equal(t, []uint32{2, 4}, chainIds(t, hm, a)) - // Make sure we go h2 -> h4 -> h5 - prim = hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1")) - assert.Equal(t, h2.localIndexId, prim.localIndexId) - assert.Equal(t, h4.localIndexId, prim.next.localIndexId) - assert.Nil(t, prim.prev) - assert.Equal(t, h4.localIndexId, h2.next.localIndexId) - assert.Equal(t, h2.localIndexId, h4.prev.localIndexId) - assert.Equal(t, h5.localIndexId, h4.next.localIndexId) - assert.Equal(t, h4.localIndexId, h5.prev.localIndexId) - assert.Nil(t, h5.next) + // Delete the head; h4 remains and becomes primary. + assert.False(t, hm.DeleteHostInfo(h2)) + assert.Equal(t, []uint32{4}, chainIds(t, hm, a)) + assert.Equal(t, h4, hm.QueryVpnAddr(a)) - // Delete the tail - hm.DeleteHostInfo(h5) - assert.Nil(t, h5.prev) - assert.Nil(t, h5.next) + // Delete the only remaining item; final is true and the address is gone. + assert.True(t, hm.DeleteHostInfo(h4)) + assert.Empty(t, chainIds(t, hm, a)) + assert.Nil(t, hm.QueryVpnAddr(a)) - // Make sure we go h2 -> h4 - prim = hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1")) - assert.Equal(t, h2.localIndexId, prim.localIndexId) - assert.Equal(t, h4.localIndexId, prim.next.localIndexId) - assert.Nil(t, prim.prev) - assert.Equal(t, h4.localIndexId, h2.next.localIndexId) - assert.Equal(t, h2.localIndexId, h4.prev.localIndexId) - assert.Nil(t, h4.next) + // Deleting an already-gone hostinfo is still final; nothing holds the address anymore. + assert.True(t, hm.DeleteHostInfo(h4)) + assert.Empty(t, chainIds(t, hm, a)) +} - // Delete the head - hm.DeleteHostInfo(h2) - assert.Nil(t, h2.prev) - assert.Nil(t, h2.next) +// TestHostMap_MakePrimary_DeletedHostInfo covers promoting a hostinfo that lost a race with +// tunnel teardown: swapPrimary and AddRelay decide to promote while holding a stale pointer and +// only take the write lock after a delete fully unlinked the hostinfo. MakePrimary must be a +// no-op, not a resurrection that installs an unmanaged primary. +func TestHostMap_MakePrimary_DeletedHostInfo(t *testing.T) { + l := test.NewLogger() + hm := newHostMap(l) + f := &Interface{} + a := netip.MustParseAddr("0.0.0.1") - // Make sure we only have h4 - prim = hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1")) - assert.Equal(t, h4.localIndexId, prim.localIndexId) - assert.Nil(t, prim.prev) - assert.Nil(t, prim.next) - assert.Nil(t, h4.next) + h1 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 1} + h2 := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 2} + hm.unlockedAddHostInfo(h1, f) + hm.unlockedAddHostInfo(h2, f) - // Delete the only item - hm.DeleteHostInfo(h4) - assert.Nil(t, h4.prev) - assert.Nil(t, h4.next) + // h1 is fully deleted while another goroutine still holds a pointer to it. + assert.False(t, hm.DeleteHostInfo(h1)) + assert.Equal(t, []uint32{2}, chainIds(t, hm, a)) - // Make sure we have nil - prim = hm.QueryVpnAddr(netip.MustParseAddr("0.0.0.1")) - assert.Nil(t, prim) + // The stale promote must not bring it back. + hm.MakePrimary(h1) + assert.Equal(t, []uint32{2}, chainIds(t, hm, a)) + assert.Equal(t, h2, hm.QueryVpnAddr(a)) + assert.Nil(t, hm.QueryIndex(h1.localIndexId)) +} + +// TestHostMap_QueryVpnAddrsRelayFor_NonPrimary makes sure a relay established on an older +// hostinfo is still found after a newer tunnel without relay state takes primary for the same +// address. The lookup checks the primary first and falls back to the rest of the list. +func TestHostMap_QueryVpnAddrsRelayFor_NonPrimary(t *testing.T) { + l := test.NewLogger() + hm := newHostMap(l) + f := &Interface{} + relayAddr := netip.MustParseAddr("0.0.0.9") + target := netip.MustParseAddr("0.0.0.1") + + older := &HostInfo{ + vpnAddrs: []netip.Addr{relayAddr}, + localIndexId: 1, + relayState: RelayState{ + relayForByAddr: map[netip.Addr]*Relay{}, + relayForByIdx: map[uint32]*Relay{}, + }, + } + older.relayState.InsertRelay(target, 100, &Relay{Type: ForwardingType, State: Established, LocalIndex: 100, PeerAddr: target}) + hm.unlockedAddHostInfo(older, f) + + // The relay is found on the primary. + h, r, err := hm.QueryVpnAddrsRelayFor([]netip.Addr{target}, relayAddr) + require.NoError(t, err) + assert.Equal(t, older, h) + assert.Equal(t, uint32(100), r.LocalIndex) + + // A re-handshake with no relay state takes primary; the established relay on the older + // hostinfo must still be found through the fallback. + newer := &HostInfo{vpnAddrs: []netip.Addr{relayAddr}, localIndexId: 2} + hm.unlockedAddHostInfo(newer, f) + assert.Equal(t, []uint32{2, 1}, chainIds(t, hm, relayAddr)) + + h, r, err = hm.QueryVpnAddrsRelayFor([]netip.Addr{target}, relayAddr) + require.NoError(t, err) + assert.Equal(t, older, h) + assert.Equal(t, uint32(100), r.LocalIndex) + + // No hostinfo at all is a plain miss. + _, _, err = hm.QueryVpnAddrsRelayFor([]netip.Addr{target}, netip.MustParseAddr("0.0.0.42")) + require.Error(t, err) } // TestHostMap_DeleteHostInfo_MultipleVpnAddrs exercises the case where a hostinfo carries more than one @@ -216,32 +240,82 @@ func TestHostMap_DeleteHostInfo_MultipleVpnAddrs(t *testing.T) { hm.unlockedAddHostInfo(other, f) hm.unlockedAddHostInfo(head, f) - // head is primary for both addresses, other is next in the shared chain - assert.Equal(t, head.localIndexId, hm.QueryVpnAddr(a).localIndexId) - assert.Equal(t, head.localIndexId, hm.QueryVpnAddr(b).localIndexId) - assert.Equal(t, other.localIndexId, head.next.localIndexId) - assert.Equal(t, head.localIndexId, other.prev.localIndexId) + // head is primary for both addresses, other is next in each address's list. + assert.Equal(t, head, hm.QueryVpnAddr(a)) + assert.Equal(t, head, hm.QueryVpnAddr(b)) + assert.Equal(t, []uint32{2, 1}, chainIds(t, hm, a)) + assert.Equal(t, []uint32{2, 1}, chainIds(t, hm, b)) // Delete the head. other is still live, so it must become primary for BOTH addresses. - hm.DeleteHostInfo(head) + assert.False(t, hm.DeleteHostInfo(head)) + assert.Equal(t, other, hm.QueryVpnAddr(a)) + assert.Equal(t, other, hm.QueryVpnAddr(b)) + assert.Equal(t, []uint32{1}, chainIds(t, hm, a)) + assert.Equal(t, []uint32{1}, chainIds(t, hm, b)) - // Pre-fix: QueryVpnAddr(b) came back nil here because the second address was deleted rather than - // promoted, leaving other unreachable at b. - require.NotNil(t, hm.QueryVpnAddr(a)) - require.NotNil(t, hm.QueryVpnAddr(b)) - assert.Equal(t, other.localIndexId, hm.QueryVpnAddr(a).localIndexId) - assert.Equal(t, other.localIndexId, hm.QueryVpnAddr(b).localIndexId) - - // other is now the only hostinfo in the chain - assert.Nil(t, other.prev) - assert.Nil(t, other.next) - - // head is fully detached - assert.Nil(t, head.prev) - assert.Nil(t, head.next) + // head is fully removed from the index map. assert.Nil(t, hm.QueryIndex(head.localIndexId)) } +// TestHostMap_DeleteHostInfo_DivergentVpnAddrs covers chained hostinfos for the same peer whose +// vpnAddrs sets differ (a re-handshake cert added a second address). Deleting the superset node +// must not promote a sibling to an address it does not own. +func TestHostMap_DeleteHostInfo_DivergentVpnAddrs(t *testing.T) { + l := test.NewLogger() + hm := newHostMap(l) + f := &Interface{} + a := netip.MustParseAddr("0.0.0.1") + b := netip.MustParseAddr("0.0.0.2") + + // sub owns only a; super (a newer handshake) owns a and b. + sub := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 1} + super := &HostInfo{vpnAddrs: []netip.Addr{a, b}, localIndexId: 2} + hm.unlockedAddHostInfo(sub, f) + hm.unlockedAddHostInfo(super, f) + + assert.Equal(t, []uint32{2, 1}, chainIds(t, hm, a)) + assert.Equal(t, []uint32{2}, chainIds(t, hm, b)) + + // Delete super: a promotes to sub (which owns it); b has no remaining owner and must be + // removed, not dangled at sub (which does not own b). + assert.False(t, hm.DeleteHostInfo(super)) + assert.Equal(t, []uint32{1}, chainIds(t, hm, a)) + assert.Empty(t, chainIds(t, hm, b)) + assert.Equal(t, sub, hm.QueryVpnAddr(a)) + assert.Nil(t, hm.QueryVpnAddr(b)) + assert.Nil(t, hm.QueryIndex(super.localIndexId)) + + // Deleting sub cleans up fully. + assert.True(t, hm.DeleteHostInfo(sub)) + assert.Nil(t, hm.QueryVpnAddr(a)) + assertHostMapInvariants(t, hm) +} + +// TestHostMap_AddDivergentOverlap covers a new hostinfo claiming addresses currently owned by two +// DIFFERENT hostinfos. The old single shared next/prev chain overwrote a pointer and orphaned one +// of them (in Indexes but unreachable via its address); independent per-address lists cannot. +func TestHostMap_AddDivergentOverlap(t *testing.T) { + l := test.NewLogger() + hm := newHostMap(l) + f := &Interface{} + a := netip.MustParseAddr("0.0.0.1") + b := netip.MustParseAddr("0.0.0.2") + + hiA := &HostInfo{vpnAddrs: []netip.Addr{a}, localIndexId: 1} + hiP := &HostInfo{vpnAddrs: []netip.Addr{b}, localIndexId: 2} + hm.unlockedAddHostInfo(hiA, f) + hm.unlockedAddHostInfo(hiP, f) + + hiB := &HostInfo{vpnAddrs: []netip.Addr{a, b}, localIndexId: 3} + hm.unlockedAddHostInfo(hiB, f) + + assert.Equal(t, []uint32{3, 1}, chainIds(t, hm, a)) + assert.Equal(t, []uint32{3, 2}, chainIds(t, hm, b)) + // hiA is still reachable via its address (not orphaned) and still indexed. + assert.Contains(t, chainIds(t, hm, a), hiA.localIndexId) + assert.NotNil(t, hm.QueryIndex(hiA.localIndexId)) +} + // TestHostMap_MaxHostInfosPerVpnIp_MultipleVpnAddrs verifies the MaxHostInfosPerVpnIp overflow prune // (unlockedInnerAddHostInfo calls unlockedDeleteHostInfo on the oldest node once the chain is too long) // still behaves when hostinfos carry more than one vpnAddr. The pruned node is always the tail, so it is @@ -267,32 +341,14 @@ func TestHostMap_MaxHostInfosPerVpnIp_MultipleVpnAddrs(t *testing.T) { oldest := hostinfos[len(hostinfos)-1] - // The oldest hostinfo should have been pruned and fully detached - assert.Nil(t, oldest.next) - assert.Nil(t, oldest.prev) + // The oldest hostinfo was pruned from both lists and the index map. assert.Nil(t, hm.QueryIndex(oldest.localIndexId)) - // Both addresses resolve to the same head, and that head is one of the survivors (not the pruned one) - primA := hm.QueryVpnAddr(a) - primB := hm.QueryVpnAddr(b) - require.NotNil(t, primA) - require.NotNil(t, primB) - assert.Equal(t, primA.localIndexId, primB.localIndexId) - assert.NotEqual(t, oldest.localIndexId, primA.localIndexId) - - // Walk the shared chain: exactly MaxHostInfosPerVpnIp survivors, no cycles, oldest absent - seen := map[uint32]struct{}{} - for h := primA; h != nil; h = h.next { - _, dup := seen[h.localIndexId] - require.False(t, dup, "cycle detected in hostinfo chain") - seen[h.localIndexId] = struct{}{} - if h.next != nil { - assert.Equal(t, h.localIndexId, h.next.prev.localIndexId, "prev pointer must mirror next") - } - } - assert.Len(t, seen, MaxHostInfosPerVpnIp) - _, prunedStillPresent := seen[oldest.localIndexId] - assert.False(t, prunedStillPresent) + // Both addresses hold exactly MaxHostInfosPerVpnIp survivors in the same order; oldest is absent. + require.Len(t, chainIds(t, hm, a), MaxHostInfosPerVpnIp) + assert.Equal(t, chainIds(t, hm, a), chainIds(t, hm, b), "both addresses must list the same survivors in the same order") + assert.NotContains(t, chainIds(t, hm, a), oldest.localIndexId) + assert.Equal(t, hm.QueryVpnAddr(a), hm.QueryVpnAddr(b)) } func TestHostMap_reload(t *testing.T) {