From 42867c7a59fde64ad6771feabc71daa48c20cbe0 Mon Sep 17 00:00:00 2001 From: pascal Date: Thu, 25 Jun 2026 01:48:39 +0200 Subject: [PATCH] ignore siblings when add/remove peer from group --- management/server/affected_peers_test.go | 18 +++--- management/server/affectedpeers/resolver.go | 67 +++++++++++++-------- management/server/group.go | 18 +++--- 3 files changed, 64 insertions(+), 39 deletions(-) diff --git a/management/server/affected_peers_test.go b/management/server/affected_peers_test.go index d8966e6c6..79aae926f 100644 --- a/management/server/affected_peers_test.go +++ b/management/server/affected_peers_test.go @@ -490,8 +490,9 @@ func TestResolveAffectedPeers_PolicyBetweenTwoGroups(t *testing.T) { result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[1]}) assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1]}, result) + // peerIDs[2] is unrelated to the route; only its own map can change. result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[2]}) - assert.Empty(t, result) + assert.ElementsMatch(t, []string{peerIDs[2]}, result) } func TestResolveAffectedPeers_PolicyThreeGroups(t *testing.T) { @@ -544,8 +545,9 @@ func TestResolveAffectedPeers_RoutePeerGroups(t *testing.T) { result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[1]}) assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1]}, result) + // peerIDs[2] is in no policy; only its own map can change, so it refreshes itself. result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[2]}) - assert.Empty(t, result) + assert.ElementsMatch(t, []string{peerIDs[2]}, result) } func TestResolveAffectedPeers_RouteWithDirectPeer(t *testing.T) { @@ -602,9 +604,9 @@ func TestResolveAffectedPeers_RouteWithAccessControlGroups(t *testing.T) { result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[2]}) assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1], peerIDs[2]}, result) - // peer3 is unrelated + // peer3 is unrelated to the route; only its own map can change. result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[3]}) - assert.Empty(t, result) + assert.ElementsMatch(t, []string{peerIDs[3]}, result) } func TestResolveAffectedPeers_NetworkRouter(t *testing.T) { @@ -896,8 +898,9 @@ func TestAffectedPeers_IsolatedPolicies(t *testing.T) { assert.NotContains(t, result, peerIDs[0]) assert.NotContains(t, result, peerIDs[1]) + // peerIDs[4] is in neither isolated policy; only its own map can change. result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[4]}) - assert.Empty(t, result) + assert.ElementsMatch(t, []string{peerIDs[4]}, result) } func TestAffectedPeers_IsolatedRouteAndPolicy(t *testing.T) { @@ -1019,12 +1022,13 @@ func TestAffectedPeers_GroupUpdateOnlyAffectsLinkedPeers(t *testing.T) { }) } -func TestAffectedPeers_UnlinkedGroupChange_NoUpdates(t *testing.T) { +// A peer in no policy/route refreshes only itself — no other peer is affected. +func TestAffectedPeers_UnlinkedPeerChange_RefreshesSelfOnly(t *testing.T) { manager, s, accountID, peerIDs, _ := setupAffectedPeersTest(t) ctx := context.Background() result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) - assert.Empty(t, result) + assert.ElementsMatch(t, []string{peerIDs[0]}, result) } // TestAffectedPeers_PolicyChange_UnrelatedPeerNoUpdate verifies that creating/deleting a diff --git a/management/server/affectedpeers/resolver.go b/management/server/affectedpeers/resolver.go index 19818ca3a..6769450c9 100644 --- a/management/server/affectedpeers/resolver.go +++ b/management/server/affectedpeers/resolver.go @@ -61,7 +61,8 @@ func Load(ctx context.Context, s store.Store, accountID string, c Change) (*Snap // loadCollections reads the policy/route/nameserver/dns/router/resource/proxy // collections a Change can touch, gated to what the walk needs. func (snap *Snapshot) loadCollections(ctx context.Context, s store.Store, accountID string, c Change) error { - hasGroupOrPeerChange := len(c.ChangedGroupIDs) > 0 || len(c.ChangedPeerIDs) > 0 || len(c.Resources) > 0 + // LinkGroups drive the same policy/route/dns walk as a changed group or peer. + hasGroupOrPeerChange := len(c.ChangedGroupIDs) > 0 || len(c.ChangedPeerIDs) > 0 || len(c.LinkGroups) > 0 || len(c.Resources) > 0 hasNetworkObject := len(c.Routers) > 0 || len(c.Resources) > 0 || len(c.Networks) > 0 // the resource<->router bridge can fire for any of these needsRoutersResources := hasGroupOrPeerChange || len(c.PostureCheckIDs) > 0 || len(c.Policies) > 0 || hasNetworkObject @@ -76,7 +77,7 @@ func (snap *Snapshot) loadCollections(ctx context.Context, s store.Store, accoun return err } } - if len(c.ChangedGroupIDs) > 0 || len(c.ChangedPeerIDs) > 0 { + if len(c.ChangedGroupIDs) > 0 || len(c.ChangedPeerIDs) > 0 || len(c.LinkGroups) > 0 { if err := snap.loadDNS(ctx, s, accountID); err != nil { return err } @@ -174,6 +175,24 @@ type Change struct { // folded in — but only when the group is linked (an unlinked group has no map // impact), matching how current members are handled. RemovedPeersByGroup map[string][]string + + // OutputPeerIDs are peers folded straight into the result without seeding their + // group memberships into the walk. Use for the peer whose group membership changed: + // the peer itself must refresh, but its OTHER groups did not change, so they must + // not be walked. Contrast ChangedPeerIDs, which seeds ALL of the peer's groups + // (correct when the peer's own attributes changed, e.g. IP/status). + OutputPeerIDs []string + + // LinkGroups are groups used ONLY to match policies/routes/routers and walk to the + // OPPOSITE side — they are never expanded to their own members. Use this when a + // peer's group membership changed: pass the peer in ChangedPeerIDs and its + // group(s) here. The opposite side of the policies the group participates in + // refreshes, but the group's other members (siblings) do not — nothing changed for + // them. For an intra-group policy (A→A) the opposite side IS the group, so its + // members still refresh via the opposite-side fold, exactly when they genuinely + // gain/lose the changed peer. Unlike ChangedGroupIDs, a LinkGroup is not added to + // the output, so a one-sided membership change never wakes the whole group. + LinkGroups []string } func (c Change) isEmpty() bool { @@ -186,7 +205,9 @@ func (c Change) isEmpty() bool { len(c.Networks) == 0 && len(c.PostureCheckIDs) == 0 && len(c.DistributionGroupIDs) == 0 && - len(c.RemovedPeersByGroup) == 0 + len(c.RemovedPeersByGroup) == 0 && + len(c.LinkGroups) == 0 && + len(c.OutputPeerIDs) == 0 } // Expand returns the deduplicated affected peer IDs from the preloaded Snapshot, @@ -197,8 +218,8 @@ func (snap *Snapshot) Expand(ctx context.Context, accountID string, c Change) [] return nil } r := newResolver(ctx, snap, accountID, c) - log.WithContext(ctx).Tracef("affectedpeers expand start: account=%s changedGroups=%v changedPeers=%v policies=%d routes=%d routers=%d resources=%d networks=%d postureChecks=%v distributionGroups=%v", - accountID, c.ChangedGroupIDs, c.ChangedPeerIDs, len(c.Policies), len(c.Routes), len(c.Routers), len(c.Resources), len(c.Networks), c.PostureCheckIDs, c.DistributionGroupIDs) + log.WithContext(ctx).Tracef("affectedpeers expand start: account=%s changedGroups=%v changedPeers=%v linkGroups=%v policies=%d routes=%d routers=%d resources=%d networks=%d postureChecks=%v distributionGroups=%v", + accountID, c.ChangedGroupIDs, c.ChangedPeerIDs, c.LinkGroups, len(c.Policies), len(c.Routes), len(c.Routers), len(c.Resources), len(c.Networks), c.PostureCheckIDs, c.DistributionGroupIDs) r.walk() return r.expand() } @@ -231,6 +252,9 @@ func newResolver(ctx context.Context, snap *Snapshot, accountID string, c Change affectedGroups: make(map[string]struct{}), affectedPeers: make(map[string]struct{}), } + // LinkGroups match policies/routes to find the opposite side but are NOT output: + // they go into linkGroups only, never outputGroups, so their members never fold in. + addAll(r.linkGroups, c.LinkGroups) // Resolve each changed peer to its groups here so callers pass only ChangedPeerIDs. r.seedChangedGroupsFromPeers() return r @@ -304,6 +328,15 @@ func (r *resolver) walk() { r.collectFromChangedResources(r.change.Resources) r.collectFromChangedNetworks(r.change.Networks) + // The explicitly changed peers always refresh their own maps. OnPeersUpdated only + // refreshes the resolver's output (it ignores the separately-passed changed peers), + // so the changed peer reaches its own new map only via here. An offline/deleted + // peer in the set is filtered downstream (filterConnectedAffectedPeers). + addAll(r.affectedPeers, setToSlice(r.changedPeers)) + // OutputPeerIDs refresh themselves too, but unlike changedPeers their group + // memberships were not seeded into the walk (only the changed group was). + addAll(r.affectedPeers, r.change.OutputPeerIDs) + // Distribution groups (nameserver/DNS) affect only their member peers: fold them // straight into affectedGroups so expand() maps them to members, without the // policy/route walk that linkGroups would trigger. @@ -488,16 +521,14 @@ func (r *resolver) foldRuleSideIfChanged(policy *types.Policy, rule *types.Polic // Opposite side, fully down to peers (a destination opposite also folds routers). r.foldPolicySideForRule(policy, rule, side.opposite()) - // Own side: the changed group's members (only if the group itself changed), and - // the changed direct peer / changed peers in a matched group — never siblings. + // Own side: fold the whole changed group's members only when the group itself + // changed (outputGroups). A peer-seeded or link-only group is not folded here — + // its siblings never refresh. The changed peers themselves are folded once, after + // the walk (see walk()). for _, gID := range nearGroups { if _, ok := r.outputGroups[gID]; ok { r.affectedGroups[gID] = struct{}{} } - r.foldChangedPeersInGroup(gID) - } - if matchedByPeer { - r.affectedPeers[nearResource.ID] = struct{}{} } // When the changed side IS a destination, the resources it targets are reached @@ -521,20 +552,6 @@ func (r *resolver) foldPolicySideForRule(policy *types.Policy, rule *types.Polic } } -// foldChangedPeersInGroup folds changed peers that belong to groupID directly into -// affectedPeers (the peer only, never its co-members). -func (r *resolver) foldChangedPeersInGroup(groupID string) { - if len(r.changedPeers) == 0 { - return - } - members := r.snap.groupPeers[groupID] - for pID := range r.changedPeers { - if _, ok := members[pID]; ok { - r.affectedPeers[pID] = struct{}{} - } - } -} - // collectFromChangedRoutes folds an explicitly changed route's own groups and peer. func (r *resolver) collectFromChangedRoutes(routes []*route.Route) { for _, rt := range routes { diff --git a/management/server/group.go b/management/server/group.go index 070344c61..460b51274 100644 --- a/management/server/group.go +++ b/management/server/group.go @@ -520,7 +520,12 @@ func collectDeletableGroups(ctx context.Context, transaction store.Store, accoun // GroupAddPeer appends peer to the group func (am *DefaultAccountManager) GroupAddPeer(ctx context.Context, accountID, groupID, peerID string) error { var snap *affectedpeers.Snapshot - change := affectedpeers.Change{ChangedGroupIDs: []string{groupID}} + // A membership change affects only the peer itself and the opposite side of THIS + // group's policies — not the group's other members, and not the peer's other + // groups. LinkGroups walks only this group (matched, not expanded); OutputPeerIDs + // refreshes the peer without seeding its other group memberships. For an + // intra-group policy the opposite side is the group, so its members still refresh. + change := affectedpeers.Change{OutputPeerIDs: []string{peerID}, LinkGroups: []string{groupID}} err := am.Store.ExecuteInTransaction(ctx, func(transaction store.Store) error { if err := transaction.AddPeerToGroup(ctx, accountID, peerID, groupID); err != nil { @@ -586,10 +591,11 @@ func (am *DefaultAccountManager) GroupAddResource(ctx context.Context, accountID // GroupDeletePeer removes peer from the group func (am *DefaultAccountManager) GroupDeletePeer(ctx context.Context, accountID, groupID, peerID string) error { var snap *affectedpeers.Snapshot - change := affectedpeers.Change{ - ChangedGroupIDs: []string{groupID}, - RemovedPeersByGroup: map[string][]string{groupID: {peerID}}, - } + // Same as GroupAddPeer: the removed peer and the opposite side of THIS group's + // policies refresh, not the group's other members or the peer's other groups. The + // peer is no longer in the group's index, but LinkGroups still drives the + // opposite-side walk, and OutputPeerIDs refreshes the removed peer itself. + change := affectedpeers.Change{OutputPeerIDs: []string{peerID}, LinkGroups: []string{groupID}} err := am.Store.ExecuteInTransaction(ctx, func(transaction store.Store) error { if err := transaction.RemovePeerFromGroup(ctx, peerID, groupID); err != nil { @@ -600,8 +606,6 @@ func (am *DefaultAccountManager) GroupDeletePeer(ctx context.Context, accountID, return err } - // The removed peer is carried in change.RemovedPeersByGroup and folded in - // only when the group is linked, so loading post-removal is correct. var err error if snap, err = affectedpeers.Load(ctx, transaction, accountID, change); err != nil { return err