From 7873f337df2ebc191c9f666a4bcb72577730f53d Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Tue, 23 Jun 2026 18:39:53 +0200 Subject: [PATCH 01/12] when collecting group and peer IDs from policies, do so directionally Signed-off-by: Dmitri Dolguikh --- management/server/affectedpeers/resolver.go | 54 ++++++++++++++----- .../server/affectedpeers/resolver_test.go | 52 +++++++++++++----- 2 files changed, 79 insertions(+), 27 deletions(-) diff --git a/management/server/affectedpeers/resolver.go b/management/server/affectedpeers/resolver.go index 4ef986345..ad278039a 100644 --- a/management/server/affectedpeers/resolver.go +++ b/management/server/affectedpeers/resolver.go @@ -447,15 +447,23 @@ func (r *resolver) collectFromPostureChecks(postureCheckIDs []string) { func (r *resolver) collectFromPolicies() { for _, policy := range r.policies() { - matchedByGroup := policyReferencesGroups(policy, r.changedGroupSet) - matchedByPeer := len(r.changedPeerSet) > 0 && policyReferencesDirectPeers(policy, r.changedPeerSet) - if !matchedByGroup && !matchedByPeer { + // changed peer IDs have been mapped to changedGroupSet on resolver creation (see seedChangedGroupsFromPeers) + // there's no change to the groupSet if the same policies have been changed directly + groupIds := groupsFromPolicyDirectionally(policy, r.changedGroupSet) + addAll(r.groupSet, groupIds) + + var peerIds []string + if len(r.changedPeerSet) > 0 { + peerIds = peersFromPolicyDirectionally(policy, r.changedPeerSet) + addAll(r.peerSet, peerIds) + } + + if len(groupIds) == 0 && len(peerIds) == 0 { continue } + log.WithContext(r.ctx).Tracef("collectFromPolicies: policy %s (%s) matched (byGroup=%t byPeer=%t) -> folding rule groups %v + direct peers", - policy.ID, policy.Name, matchedByGroup, matchedByPeer, policy.RuleGroups()) - addAll(r.groupSet, policy.RuleGroups()) - collectPolicyDirectPeers(policy, r.peerSet) + policy.ID, policy.Name, len(groupIds) > 0, len(peerIds) > 0, policy.RuleGroups()) r.matchedPolicies = append(r.matchedPolicies, policy) } } @@ -734,22 +742,40 @@ func collectPolicySources(policy *types.Policy, groupSet, peerSet map[string]str } } -func policyReferencesGroups(policy *types.Policy, groupSet map[string]struct{}) bool { +// returns group IDs of groups on the opposite side of the policy: +// i.e. if a group is present in the policy rule sources, use group IDs from the rule's destinations +// and vice-versa +func groupsFromPolicyDirectionally(policy *types.Policy, groupSet map[string]struct{}) []string { + groupIds := make([]string, 0) for _, rule := range policy.Rules { - if anyInSet(rule.Sources, groupSet) || anyInSet(rule.Destinations, groupSet) { - return true + // TODO (dmitri) can a group to be present on both sides of a policy? + if anyInSet(rule.Sources, groupSet) { + groupIds = append(groupIds, rule.Destinations...) + } else if anyInSet(rule.Destinations, groupSet) { + groupIds = append(groupIds, rule.Sources...) } } - return false + return groupIds } -func policyReferencesDirectPeers(policy *types.Policy, changedSet map[string]struct{}) bool { +// returns peer IDs of peers on the opposite side of the policy: +// i.e. if a peer is present in the policy rule sourceResources, use destinationResources of the policy +// and vice-versa +func peersFromPolicyDirectionally(policy *types.Policy, changedSet map[string]struct{}) []string { + peerIds := make([]string, 0) for _, rule := range policy.Rules { - if isDirectPeerInSet(rule.SourceResource, changedSet) || isDirectPeerInSet(rule.DestinationResource, changedSet) { - return true + // TODO (dmitri) can a peer to be present on both sides of a policy? + if isDirectPeerInSet(rule.SourceResource, changedSet) { + if rule.DestinationResource.Type == types.ResourceTypePeer && rule.DestinationResource.ID != "" { + peerIds = append(peerIds, rule.DestinationResource.ID) + } + } else if isDirectPeerInSet(rule.DestinationResource, changedSet) { + if rule.SourceResource.Type == types.ResourceTypePeer && rule.SourceResource.ID != "" { + peerIds = append(peerIds, rule.SourceResource.ID) + } } } - return false + return peerIds } func policyReferencesPostureChecks(policy *types.Policy, ids map[string]struct{}) bool { diff --git a/management/server/affectedpeers/resolver_test.go b/management/server/affectedpeers/resolver_test.go index dcd304a56..28b718162 100644 --- a/management/server/affectedpeers/resolver_test.go +++ b/management/server/affectedpeers/resolver_test.go @@ -80,24 +80,50 @@ func TestChangeIsEmpty(t *testing.T) { assert.False(t, Change{PostureCheckIDs: []string{"pc"}}.isEmpty()) } -func TestPolicyReferencesGroups(t *testing.T) { - policy := &types.Policy{Rules: []*types.PolicyRule{{Sources: []string{"g1", "g2"}, Destinations: []string{"g3"}}}} +func TestGroupsFromPolicyDirectionally(t *testing.T) { + policy := &types.Policy{Rules: []*types.PolicyRule{ + {Sources: []string{"g1", "g2"}, Destinations: []string{"g3"}}, + {Sources: []string{"g4"}, Destinations: []string{"g5", "g6"}}, + }} - assert.True(t, policyReferencesGroups(policy, map[string]struct{}{"g1": {}})) - assert.True(t, policyReferencesGroups(policy, map[string]struct{}{"g3": {}})) - assert.False(t, policyReferencesGroups(policy, map[string]struct{}{"g4": {}})) - assert.False(t, policyReferencesGroups(policy, map[string]struct{}{})) + assert.Equal(t, groupsFromPolicyDirectionally(policy, map[string]struct{}{"g1": {}, "g4": {}}), []string{"g3", "g5", "g6"}) + assert.Equal(t, groupsFromPolicyDirectionally(policy, map[string]struct{}{"g3": {}, "g6": {}}), []string{"g1", "g2", "g4"}) + assert.Equal(t, groupsFromPolicyDirectionally(policy, map[string]struct{}{"g33": {}}), []string{}) + assert.Equal(t, groupsFromPolicyDirectionally(policy, map[string]struct{}{}), []string{}) } func TestPolicyReferencesDirectPeers(t *testing.T) { - policy := &types.Policy{Rules: []*types.PolicyRule{{ - SourceResource: types.Resource{Type: types.ResourceTypePeer, ID: "p1"}, - DestinationResource: types.Resource{Type: types.ResourceTypeHost, ID: "r1"}, - }}} + policy := &types.Policy{Rules: []*types.PolicyRule{ + { + SourceResource: types.Resource{Type: types.ResourceTypePeer, ID: "p1"}, + DestinationResource: types.Resource{Type: types.ResourceTypePeer, ID: "r1"}, + }, + { + SourceResource: types.Resource{Type: types.ResourceTypePeer, ID: "p2"}, + DestinationResource: types.Resource{Type: types.ResourceTypePeer, ID: "r2"}, + }, + { + SourceResource: types.Resource{Type: types.ResourceTypePeer, ID: "p3"}, + DestinationResource: types.Resource{Type: types.ResourceTypeHost, ID: "r3"}, + }, + { + SourceResource: types.Resource{Type: types.ResourceTypeHost, ID: "p4"}, + DestinationResource: types.Resource{Type: types.ResourceTypePeer, ID: "r4"}, + }, + { + SourceResource: types.Resource{Type: types.ResourceTypeHost, ID: "p5"}, + DestinationResource: types.Resource{Type: types.ResourceTypePeer, ID: "r5"}, + }, + { + SourceResource: types.Resource{Type: types.ResourceTypePeer, ID: "p6"}, + DestinationResource: types.Resource{Type: types.ResourceTypeHost, ID: "r6"}, + }, + }} - assert.True(t, policyReferencesDirectPeers(policy, map[string]struct{}{"p1": {}})) - assert.False(t, policyReferencesDirectPeers(policy, map[string]struct{}{"r1": {}})) - assert.False(t, policyReferencesDirectPeers(policy, map[string]struct{}{"p2": {}})) + assert.Equal(t, []string{"r1", "r2"}, peersFromPolicyDirectionally(policy, map[string]struct{}{"p1": {}, "p2": {}})) + assert.Equal(t, []string{"p1", "p2"}, peersFromPolicyDirectionally(policy, map[string]struct{}{"r1": {}, "r2": {}})) + assert.Empty(t, peersFromPolicyDirectionally(policy, map[string]struct{}{"p3": {}, "r4": {}})) + assert.Empty(t, peersFromPolicyDirectionally(policy, map[string]struct{}{"p5": {}, "r6": {}})) } func TestPolicyReferencesPostureChecks(t *testing.T) { From 4c4434a8715c7d214fbbd37d57052c840fe475ad Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 24 Jun 2026 09:48:45 +0200 Subject: [PATCH 02/12] fixed a few tests Signed-off-by: Dmitri Dolguikh --- management/server/affected_peers_test.go | 48 ++++++++++++------------ 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/management/server/affected_peers_test.go b/management/server/affected_peers_test.go index e2dcd830b..a19f9e174 100644 --- a/management/server/affected_peers_test.go +++ b/management/server/affected_peers_test.go @@ -114,12 +114,10 @@ func TestCollectGroupChange_PolicyLinked(t *testing.T) { require.NoError(t, err) groups, _ := collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[0]}) - assert.Contains(t, groups, groupIDs[0]) - assert.Contains(t, groups, groupIDs[1]) + assert.ElementsMatch(t, groups, []string{groupIDs[1]}) groups, _ = collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[1]}) - assert.Contains(t, groups, groupIDs[0]) - assert.Contains(t, groups, groupIDs[1]) + assert.ElementsMatch(t, groups, []string{groupIDs[0]}) groups, _ = collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[2]}) assert.Empty(t, groups) @@ -133,20 +131,20 @@ func TestCollectGroupChange_PolicyWithDirectPeerResource(t *testing.T) { Enabled: true, Rules: []*types.PolicyRule{ { - Enabled: true, - Sources: []string{groupIDs[0]}, - SourceResource: types.Resource{ID: peerIDs[3], Type: types.ResourceTypePeer}, - Destinations: []string{groupIDs[1]}, - Action: types.PolicyTrafficActionAccept, + Enabled: true, + Sources: []string{groupIDs[0]}, + SourceResource: types.Resource{ID: peerIDs[3], Type: types.ResourceTypePeer}, + DestinationResource: types.Resource{ID: peerIDs[4], Type: types.ResourceTypePeer}, + Destinations: []string{groupIDs[1]}, + Action: types.PolicyTrafficActionAccept, }, }, }, true) require.NoError(t, err) groups, directPeers := collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[0]}) - assert.Contains(t, groups, groupIDs[0]) - assert.Contains(t, groups, groupIDs[1]) - assert.Contains(t, directPeers, peerIDs[3]) + assert.ElementsMatch(t, groups, []string{groupIDs[1]}) + assert.ElementsMatch(t, directPeers, []string{peerIDs[4]}) } func TestCollectGroupChange_PolicyWithNonPeerResource_NoDirectPeers(t *testing.T) { @@ -168,8 +166,7 @@ func TestCollectGroupChange_PolicyWithNonPeerResource_NoDirectPeers(t *testing.T require.NoError(t, err) groups, directPeers := collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[0]}) - assert.Contains(t, groups, groupIDs[0]) - assert.Contains(t, groups, groupIDs[1]) + assert.ElementsMatch(t, groups, []string{groupIDs[1]}) assert.Empty(t, directPeers, "non-peer resources should not produce direct peer IDs") } @@ -373,7 +370,7 @@ func TestCollectGroupChange_MultipleEntities(t *testing.T) { require.NoError(t, err) groups, directPeers := collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[0]}) - assert.Contains(t, groups, groupIDs[0]) + assert.NotContains(t, groups, groupIDs[0]) assert.Contains(t, groups, groupIDs[1]) assert.NotContains(t, groups, groupIDs[2]) assert.NotContains(t, groups, groupIDs[3]) @@ -447,10 +444,10 @@ func TestResolveAffectedPeers_PolicyBetweenTwoGroups(t *testing.T) { require.NoError(t, err) result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) - assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1]}, result) + assert.ElementsMatch(t, []string{peerIDs[1]}, result) result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[1]}) - assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1]}, result) + assert.ElementsMatch(t, []string{peerIDs[0]}, result) result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[2]}) assert.Empty(t, result) @@ -474,7 +471,7 @@ func TestResolveAffectedPeers_PolicyThreeGroups(t *testing.T) { require.NoError(t, err) result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) - assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1], peerIDs[2]}, result) + assert.ElementsMatch(t, []string{peerIDs[2]}, result) } func TestResolveAffectedPeers_RoutePeerGroups(t *testing.T) { @@ -661,7 +658,7 @@ func TestResolveAffectedPeers_PeerInMultipleGroups(t *testing.T) { // peer0 is in group0 AND group1, so both policies apply result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) - assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1], peerIDs[2], peerIDs[3]}, result) + assert.ElementsMatch(t, []string{peerIDs[2], peerIDs[3]}, result) } func TestResolveAffectedPeers_MultipleChangedPeers(t *testing.T) { @@ -697,7 +694,7 @@ func TestResolveAffectedPeers_MultipleChangedPeers(t *testing.T) { require.NoError(t, err) result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0], peerIDs[2]}) - assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1], peerIDs[2], peerIDs[3]}, result) + assert.ElementsMatch(t, []string{peerIDs[1], peerIDs[3]}, result) } func TestResolveAffectedPeers_SharedGroupAcrossPolicyAndRoute(t *testing.T) { @@ -845,12 +842,12 @@ func TestAffectedPeers_IsolatedPolicies(t *testing.T) { require.NoError(t, err) result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) - assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1]}, result) + assert.ElementsMatch(t, []string{peerIDs[1]}, result) assert.NotContains(t, result, peerIDs[2]) assert.NotContains(t, result, peerIDs[3]) result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[2]}) - assert.ElementsMatch(t, []string{peerIDs[2], peerIDs[3]}, result) + assert.ElementsMatch(t, []string{peerIDs[3]}, result) assert.NotContains(t, result, peerIDs[0]) assert.NotContains(t, result, peerIDs[1]) @@ -896,7 +893,7 @@ func TestAffectedPeers_IsolatedRouteAndPolicy(t *testing.T) { require.NoError(t, err) result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) - assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1]}, result) + assert.ElementsMatch(t, []string{peerIDs[1]}, result) assert.NotContains(t, result, peerIDs[2]) assert.NotContains(t, result, peerIDs[3]) @@ -951,13 +948,14 @@ func TestAffectedPeers_GroupUpdateOnlyAffectsLinkedPeers(t *testing.T) { }) result := manager.resolveAffectedPeersForPeerChanges(ctx, manager.Store, accountID, []string{peer1.ID}) - assert.ElementsMatch(t, []string{peer1.ID, peer2.ID}, result) + assert.ElementsMatch(t, []string{peer2.ID}, result) t.Run("group change updates all peers in policy groups", func(t *testing.T) { done := make(chan struct{}) go func() { - peerShouldReceiveUpdate(t, updMsg1) + peerShouldNotReceiveUpdate(t, updMsg1) peerShouldReceiveUpdate(t, updMsg2) + // TODO (dmitri) what's going on here? peerShouldReceiveUpdate(t, updMsg3) close(done) }() From 33954ea15e616b72499cf82ac450b8c07d8591c0 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 24 Jun 2026 13:07:53 +0200 Subject: [PATCH 03/12] fixing tests + adding tests Signed-off-by: Dmitri Dolguikh --- .../server/affected_peers_coverage_test.go | 10 +- management/server/affected_peers_test.go | 104 ++++++++++++------ management/server/affectedpeers/resolver.go | 84 +++++++++----- 3 files changed, 130 insertions(+), 68 deletions(-) diff --git a/management/server/affected_peers_coverage_test.go b/management/server/affected_peers_coverage_test.go index 56917905f..661e89b2e 100644 --- a/management/server/affected_peers_coverage_test.go +++ b/management/server/affected_peers_coverage_test.go @@ -32,7 +32,7 @@ func TestAffectedPeers_DependencyCoverageMatrix(t *testing.T) { _, err := s.manager.SavePolicy(ctx, s.accountID, userID, peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID), true) require.NoError(t, err) return affectedpeers.Change{ChangedGroupIDs: []string{s.sourceGroupID}}, - []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} + []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} // TODO (dmitri) routerPeer is missing }, }, { @@ -106,12 +106,8 @@ func TestAffectedPeers_DependencyCoverageMatrix(t *testing.T) { change, mustContain, mustExclude := r.build(t, s, ctx) affected := resolveAffected(t, s.manager.Store, s.accountID, change) - for _, id := range mustContain { - assert.Contains(t, affected, id, "expected peer to be affected") - } - for _, id := range mustExclude { - assert.NotContains(t, affected, id, "peer must not be affected") - } + assert.ElementsMatch(t, affected, mustContain, "expected peer to be affected") + assert.NotElementsMatch(t, affected, mustExclude, "peer must not be affected") }) } } diff --git a/management/server/affected_peers_test.go b/management/server/affected_peers_test.go index a19f9e174..ed2f558dc 100644 --- a/management/server/affected_peers_test.go +++ b/management/server/affected_peers_test.go @@ -96,31 +96,54 @@ func affectedGroupID(i int) string { return fmt.Sprintf("affected-grp-%d", i) func affectedGroupName(i int) string { return fmt.Sprintf("AffectedGroup%d", i) } func TestCollectGroupChange_PolicyLinked(t *testing.T) { - manager, s, accountID, _, groupIDs := setupAffectedPeersTest(t) + manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() _, err := manager.SavePolicy(ctx, accountID, userID, &types.Policy{ Enabled: true, Rules: []*types.PolicyRule{ { - Enabled: true, - Sources: []string{groupIDs[0]}, - Destinations: []string{groupIDs[1]}, - Bidirectional: true, - Action: types.PolicyTrafficActionAccept, + Enabled: true, + Sources: []string{groupIDs[0]}, + Destinations: []string{groupIDs[1]}, + SourceResource: types.Resource{ID: peerIDs[0], Type: types.ResourceTypePeer}, + DestinationResource: types.Resource{ID: peerIDs[1], Type: types.ResourceTypePeer}, + Bidirectional: true, + Action: types.PolicyTrafficActionAccept, + }, + { + Enabled: true, + Sources: []string{groupIDs[0]}, + Destinations: []string{groupIDs[1]}, + SourceResource: types.Resource{ID: peerIDs[2], Type: types.ResourceTypeHost}, + DestinationResource: types.Resource{ID: peerIDs[3], Type: types.ResourceTypeHost}, + Bidirectional: true, + Action: types.PolicyTrafficActionAccept, + }, + { + Enabled: true, + Sources: []string{groupIDs[0]}, + Destinations: []string{groupIDs[1]}, + SourceResource: types.Resource{ID: "", Type: types.ResourceTypePeer}, + DestinationResource: types.Resource{ID: "", Type: types.ResourceTypePeer}, + Bidirectional: true, + Action: types.PolicyTrafficActionAccept, }, }, }, true) require.NoError(t, err) - groups, _ := collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[0]}) - assert.ElementsMatch(t, groups, []string{groupIDs[1]}) + groups, directPeers := collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[0]}) + assert.ElementsMatch(t, groups, []string{groupIDs[0], groupIDs[1]}) + assert.ElementsMatch(t, directPeers, []string{peerIDs[1]}) - groups, _ = collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[1]}) - assert.ElementsMatch(t, groups, []string{groupIDs[0]}) + groups, directPeers = collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[1]}) + assert.ElementsMatch(t, groups, []string{groupIDs[0], groupIDs[1]}) + assert.ElementsMatch(t, directPeers, []string{peerIDs[0]}) - groups, _ = collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[2]}) + groups, directPeers = collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[2]}) assert.Empty(t, groups) + assert.Empty(t, directPeers) } func TestCollectGroupChange_PolicyWithDirectPeerResource(t *testing.T) { @@ -138,13 +161,37 @@ func TestCollectGroupChange_PolicyWithDirectPeerResource(t *testing.T) { Destinations: []string{groupIDs[1]}, Action: types.PolicyTrafficActionAccept, }, + { + Enabled: true, + Sources: []string{groupIDs[0]}, + SourceResource: types.Resource{ID: peerIDs[1], Type: types.ResourceTypeHost}, + DestinationResource: types.Resource{ID: peerIDs[2], Type: types.ResourceTypeHost}, + Destinations: []string{groupIDs[1]}, + Action: types.PolicyTrafficActionAccept, + }, + { + Enabled: true, + Sources: []string{groupIDs[0]}, + SourceResource: types.Resource{ID: "", Type: types.ResourceTypePeer}, + DestinationResource: types.Resource{ID: "", Type: types.ResourceTypePeer}, + Destinations: []string{groupIDs[1]}, + Action: types.PolicyTrafficActionAccept, + }, }, }, true) require.NoError(t, err) groups, directPeers := collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[0]}) - assert.ElementsMatch(t, groups, []string{groupIDs[1]}) + assert.ElementsMatch(t, groups, []string{groupIDs[0], groupIDs[1]}) assert.ElementsMatch(t, directPeers, []string{peerIDs[4]}) + + groups, directPeers = collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[1]}) + assert.ElementsMatch(t, groups, []string{groupIDs[0], groupIDs[1]}) + assert.ElementsMatch(t, directPeers, []string{peerIDs[3]}) + + groups, directPeers = collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[2]}) + assert.Empty(t, groups) + assert.Empty(t, directPeers) } func TestCollectGroupChange_PolicyWithNonPeerResource_NoDirectPeers(t *testing.T) { @@ -166,7 +213,7 @@ func TestCollectGroupChange_PolicyWithNonPeerResource_NoDirectPeers(t *testing.T require.NoError(t, err) groups, directPeers := collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[0]}) - assert.ElementsMatch(t, groups, []string{groupIDs[1]}) + assert.ElementsMatch(t, groups, []string{groupIDs[0], groupIDs[1]}) assert.Empty(t, directPeers, "non-peer resources should not produce direct peer IDs") } @@ -370,17 +417,11 @@ func TestCollectGroupChange_MultipleEntities(t *testing.T) { require.NoError(t, err) groups, directPeers := collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[0]}) - assert.NotContains(t, groups, groupIDs[0]) - assert.Contains(t, groups, groupIDs[1]) - assert.NotContains(t, groups, groupIDs[2]) - assert.NotContains(t, groups, groupIDs[3]) + assert.ElementsMatch(t, groups, []string{groupIDs[0], groupIDs[1]}) assert.Empty(t, directPeers) groups, directPeers = collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[3]}) - assert.Contains(t, groups, groupIDs[2]) - assert.Contains(t, groups, groupIDs[3]) - assert.NotContains(t, groups, groupIDs[0]) - assert.NotContains(t, groups, groupIDs[1]) + assert.ElementsMatch(t, groups, []string{groupIDs[2], groupIDs[3]}) assert.Empty(t, directPeers) } @@ -444,10 +485,10 @@ func TestResolveAffectedPeers_PolicyBetweenTwoGroups(t *testing.T) { require.NoError(t, err) result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) - assert.ElementsMatch(t, []string{peerIDs[1]}, result) + assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1]}, result) result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[1]}) - assert.ElementsMatch(t, []string{peerIDs[0]}, result) + assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1]}, result) result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[2]}) assert.Empty(t, result) @@ -471,7 +512,7 @@ func TestResolveAffectedPeers_PolicyThreeGroups(t *testing.T) { require.NoError(t, err) result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) - assert.ElementsMatch(t, []string{peerIDs[2]}, result) + assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[2]}, result) } func TestResolveAffectedPeers_RoutePeerGroups(t *testing.T) { @@ -658,7 +699,7 @@ func TestResolveAffectedPeers_PeerInMultipleGroups(t *testing.T) { // peer0 is in group0 AND group1, so both policies apply result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) - assert.ElementsMatch(t, []string{peerIDs[2], peerIDs[3]}, result) + assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1], peerIDs[2], peerIDs[3]}, result) } func TestResolveAffectedPeers_MultipleChangedPeers(t *testing.T) { @@ -694,7 +735,7 @@ func TestResolveAffectedPeers_MultipleChangedPeers(t *testing.T) { require.NoError(t, err) result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0], peerIDs[2]}) - assert.ElementsMatch(t, []string{peerIDs[1], peerIDs[3]}, result) + assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[2], peerIDs[1], peerIDs[3]}, result) } func TestResolveAffectedPeers_SharedGroupAcrossPolicyAndRoute(t *testing.T) { @@ -842,12 +883,12 @@ func TestAffectedPeers_IsolatedPolicies(t *testing.T) { require.NoError(t, err) result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) - assert.ElementsMatch(t, []string{peerIDs[1]}, result) + assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1]}, result) assert.NotContains(t, result, peerIDs[2]) assert.NotContains(t, result, peerIDs[3]) result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[2]}) - assert.ElementsMatch(t, []string{peerIDs[3]}, result) + assert.ElementsMatch(t, []string{peerIDs[2], peerIDs[3]}, result) assert.NotContains(t, result, peerIDs[0]) assert.NotContains(t, result, peerIDs[1]) @@ -893,7 +934,7 @@ func TestAffectedPeers_IsolatedRouteAndPolicy(t *testing.T) { require.NoError(t, err) result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) - assert.ElementsMatch(t, []string{peerIDs[1]}, result) + assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1]}, result) assert.NotContains(t, result, peerIDs[2]) assert.NotContains(t, result, peerIDs[3]) @@ -948,14 +989,13 @@ func TestAffectedPeers_GroupUpdateOnlyAffectsLinkedPeers(t *testing.T) { }) result := manager.resolveAffectedPeersForPeerChanges(ctx, manager.Store, accountID, []string{peer1.ID}) - assert.ElementsMatch(t, []string{peer2.ID}, result) + assert.ElementsMatch(t, []string{peer1.ID, peer2.ID}, result) t.Run("group change updates all peers in policy groups", func(t *testing.T) { done := make(chan struct{}) go func() { - peerShouldNotReceiveUpdate(t, updMsg1) + peerShouldReceiveUpdate(t, updMsg1) peerShouldReceiveUpdate(t, updMsg2) - // TODO (dmitri) what's going on here? peerShouldReceiveUpdate(t, updMsg3) close(done) }() diff --git a/management/server/affectedpeers/resolver.go b/management/server/affectedpeers/resolver.go index ad278039a..5e63524b5 100644 --- a/management/server/affectedpeers/resolver.go +++ b/management/server/affectedpeers/resolver.go @@ -449,14 +449,13 @@ func (r *resolver) collectFromPolicies() { for _, policy := range r.policies() { // changed peer IDs have been mapped to changedGroupSet on resolver creation (see seedChangedGroupsFromPeers) // there's no change to the groupSet if the same policies have been changed directly - groupIds := groupsFromPolicyDirectionally(policy, r.changedGroupSet) + peerIds, groupIds := getGroupsAndPeersFromPolicyViaGroups(policy, r.changedGroupSet) addAll(r.groupSet, groupIds) + addAll(r.peerSet, peerIds) - var peerIds []string - if len(r.changedPeerSet) > 0 { - peerIds = peersFromPolicyDirectionally(policy, r.changedPeerSet) - addAll(r.peerSet, peerIds) - } + peerIds, groupIds = getGroupsAndPeersFromPolicyViaPeers(policy, r.changedPeerSet) + addAll(r.groupSet, groupIds) + addAll(r.peerSet, peerIds) if len(groupIds) == 0 && len(peerIds) == 0 { continue @@ -742,40 +741,57 @@ func collectPolicySources(policy *types.Policy, groupSet, peerSet map[string]str } } -// returns group IDs of groups on the opposite side of the policy: -// i.e. if a group is present in the policy rule sources, use group IDs from the rule's destinations +// returns group and peer IDs on the opposite side of the policy: +// i.e. if a group is present in the policy rule sources, return destination group IDs and the destinationResource from the rule // and vice-versa -func groupsFromPolicyDirectionally(policy *types.Policy, groupSet map[string]struct{}) []string { - groupIds := make([]string, 0) - for _, rule := range policy.Rules { - // TODO (dmitri) can a group to be present on both sides of a policy? - if anyInSet(rule.Sources, groupSet) { - groupIds = append(groupIds, rule.Destinations...) - } else if anyInSet(rule.Destinations, groupSet) { - groupIds = append(groupIds, rule.Sources...) - } +func getGroupsAndPeersFromPolicyViaGroups(policy *types.Policy, groupSet map[string]struct{}) ([]string, []string) { + var groupIds, peerIds []string + if len(groupSet) == 0 { + return peerIds, groupIds } - return groupIds -} - -// returns peer IDs of peers on the opposite side of the policy: -// i.e. if a peer is present in the policy rule sourceResources, use destinationResources of the policy -// and vice-versa -func peersFromPolicyDirectionally(policy *types.Policy, changedSet map[string]struct{}) []string { - peerIds := make([]string, 0) for _, rule := range policy.Rules { - // TODO (dmitri) can a peer to be present on both sides of a policy? - if isDirectPeerInSet(rule.SourceResource, changedSet) { + if matchedIds, ok := allInSet(rule.Sources, groupSet); ok { + groupIds = append(groupIds, matchedIds...) + groupIds = append(groupIds, rule.Destinations...) if rule.DestinationResource.Type == types.ResourceTypePeer && rule.DestinationResource.ID != "" { peerIds = append(peerIds, rule.DestinationResource.ID) } - } else if isDirectPeerInSet(rule.DestinationResource, changedSet) { + } + if matchedIds, ok := allInSet(rule.Destinations, groupSet); ok { + groupIds = append(groupIds, matchedIds...) + groupIds = append(groupIds, rule.Sources...) if rule.SourceResource.Type == types.ResourceTypePeer && rule.SourceResource.ID != "" { peerIds = append(peerIds, rule.SourceResource.ID) } } } - return peerIds + return peerIds, groupIds +} + +// returns group and peer IDs on the opposite side of the policy: +// i.e. if a peer is present in the policy rule sourceResources, return destination group IDs and the destinationResource from the rule +// and vice-versa +func getGroupsAndPeersFromPolicyViaPeers(policy *types.Policy, changedSet map[string]struct{}) ([]string, []string) { + var groupIds, peerIds []string + if len(changedSet) == 0 { + return peerIds, groupIds + } + for _, rule := range policy.Rules { + if isDirectPeerInSet(rule.SourceResource, changedSet) { + groupIds = append(groupIds, rule.Destinations...) + peerIds = append(peerIds, rule.SourceResource.ID) + if rule.DestinationResource.Type == types.ResourceTypePeer && rule.DestinationResource.ID != "" { + peerIds = append(peerIds, rule.DestinationResource.ID) + } + } else if isDirectPeerInSet(rule.DestinationResource, changedSet) { + groupIds = append(groupIds, rule.Sources...) + peerIds = append(peerIds, rule.DestinationResource.ID) + if rule.SourceResource.Type == types.ResourceTypePeer && rule.SourceResource.ID != "" { + peerIds = append(peerIds, rule.SourceResource.ID) + } + } + } + return peerIds, groupIds } func policyReferencesPostureChecks(policy *types.Policy, ids map[string]struct{}) bool { @@ -821,6 +837,16 @@ func anyInSet(ids []string, set map[string]struct{}) bool { return false } +func allInSet(ids []string, set map[string]struct{}) ([]string, bool) { + var matchedIds []string + for _, id := range ids { + if _, ok := set[id]; ok { + matchedIds = append(matchedIds, id) + } + } + return matchedIds, len(matchedIds) > 0 +} + func isInSet(id string, set map[string]struct{}) bool { _, ok := set[id] return ok From 9b768d17734133fb4df18c6ff742762704b1e03f Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 24 Jun 2026 13:42:19 +0200 Subject: [PATCH 04/12] fixed a bug in collectFromPolicies Signed-off-by: Dmitri Dolguikh --- .../server/affected_peers_coverage_test.go | 120 ++++++++++-------- management/server/affectedpeers/resolver.go | 18 +-- 2 files changed, 74 insertions(+), 64 deletions(-) diff --git a/management/server/affected_peers_coverage_test.go b/management/server/affected_peers_coverage_test.go index 661e89b2e..b8e3327e1 100644 --- a/management/server/affected_peers_coverage_test.go +++ b/management/server/affected_peers_coverage_test.go @@ -8,10 +8,6 @@ import ( "github.com/stretchr/testify/require" "github.com/netbirdio/netbird/management/server/affectedpeers" - resourceTypes "github.com/netbirdio/netbird/management/server/networks/resources/types" - networkTypes "github.com/netbirdio/netbird/management/server/networks/types" - "github.com/netbirdio/netbird/management/server/posture" - "github.com/netbirdio/netbird/management/server/types" ) // TestAffectedPeers_DependencyCoverageMatrix enumerates each network-map @@ -26,6 +22,7 @@ func TestAffectedPeers_DependencyCoverageMatrix(t *testing.T) { } rows := []row{ +<<<<<<< Updated upstream { name: "policy-groups/source-group-change refreshes source+routing, excludes unrelated", build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { @@ -35,6 +32,17 @@ func TestAffectedPeers_DependencyCoverageMatrix(t *testing.T) { []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} // TODO (dmitri) routerPeer is missing }, }, +======= + // { + // name: "policy-groups/source-group-change refreshes source+routing, excludes unrelated", + // build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { + // _, err := s.manager.SavePolicy(ctx, s.accountID, userID, peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID), true) + // require.NoError(t, err) + // return affectedpeers.Change{ChangedGroupIDs: []string{s.sourceGroupID}}, + // []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} + // }, + // }, +>>>>>>> Stashed changes { name: "resource-routing-bridge/router-peer-change refreshes policy sources", build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { @@ -44,58 +52,58 @@ func TestAffectedPeers_DependencyCoverageMatrix(t *testing.T) { []string{s.sourcePeerID}, []string{s.unrelatedPeerID} }, }, - { - name: "policy-change/explicit-policy refreshes source+routing", - build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { - policy := peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID) - return affectedpeers.Change{Policies: []*types.Policy{policy}}, - []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} - }, - }, - { - name: "policy-destinationresource/explicit-policy bridges to routing peer", - build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { - policy := peerToResourcePolicyByResource(s.sourceGroupID, s.resourceID) - return affectedpeers.Change{Policies: []*types.Policy{policy}}, - []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} - }, - }, - { - name: "resource-change refreshes source+routing on its network", - build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { - _, err := s.manager.SavePolicy(ctx, s.accountID, userID, peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID), true) - require.NoError(t, err) - return affectedpeers.Change{Resources: []*resourceTypes.NetworkResource{ - {ID: s.resourceID, NetworkID: s.networkID, GroupIDs: []string{s.resourceGroupID}}, - }}, - []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} - }, - }, - { - name: "network-change refreshes source+routing on that network", - build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { - _, err := s.manager.SavePolicy(ctx, s.accountID, userID, peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID), true) - require.NoError(t, err) - return affectedpeers.Change{Networks: []*networkTypes.Network{{ID: s.networkID}}}, - []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} - }, - }, - { - name: "posture-check-change refreshes source+routing of gated policy", - build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { - check, err := s.manager.SavePostureChecks(ctx, s.accountID, userID, &posture.Checks{ - Name: "cov-min-version", - Checks: posture.ChecksDefinition{NBVersionCheck: &posture.NBVersionCheck{MinVersion: "0.30.0"}}, - }, true) - require.NoError(t, err) - policy := peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID) - policy.SourcePostureChecks = []string{check.ID} - _, err = s.manager.SavePolicy(ctx, s.accountID, userID, policy, true) - require.NoError(t, err) - return affectedpeers.Change{PostureCheckIDs: []string{check.ID}}, - []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} - }, - }, + // { + // name: "policy-change/explicit-policy refreshes source+routing", + // build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { + // policy := peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID) + // return affectedpeers.Change{Policies: []*types.Policy{policy}}, + // []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} + // }, + // }, + // { + // name: "policy-destinationresource/explicit-policy bridges to routing peer", + // build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { + // policy := peerToResourcePolicyByResource(s.sourceGroupID, s.resourceID) + // return affectedpeers.Change{Policies: []*types.Policy{policy}}, + // []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} + // }, + // }, + // { + // name: "resource-change refreshes source+routing on its network", + // build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { + // _, err := s.manager.SavePolicy(ctx, s.accountID, userID, peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID), true) + // require.NoError(t, err) + // return affectedpeers.Change{Resources: []*resourceTypes.NetworkResource{ + // {ID: s.resourceID, NetworkID: s.networkID, GroupIDs: []string{s.resourceGroupID}}, + // }}, + // []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} + // }, + // }, + // { + // name: "network-change refreshes source+routing on that network", + // build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { + // _, err := s.manager.SavePolicy(ctx, s.accountID, userID, peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID), true) + // require.NoError(t, err) + // return affectedpeers.Change{Networks: []*networkTypes.Network{{ID: s.networkID}}}, + // []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} + // }, + // }, + // { + // name: "posture-check-change refreshes source+routing of gated policy", + // build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { + // check, err := s.manager.SavePostureChecks(ctx, s.accountID, userID, &posture.Checks{ + // Name: "cov-min-version", + // Checks: posture.ChecksDefinition{NBVersionCheck: &posture.NBVersionCheck{MinVersion: "0.30.0"}}, + // }, true) + // require.NoError(t, err) + // policy := peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID) + // policy.SourcePostureChecks = []string{check.ID} + // _, err = s.manager.SavePolicy(ctx, s.accountID, userID, policy, true) + // require.NoError(t, err) + // return affectedpeers.Change{PostureCheckIDs: []string{check.ID}}, + // []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} + // }, + // }, } for _, r := range rows { diff --git a/management/server/affectedpeers/resolver.go b/management/server/affectedpeers/resolver.go index 5e63524b5..5ab8a96f7 100644 --- a/management/server/affectedpeers/resolver.go +++ b/management/server/affectedpeers/resolver.go @@ -449,20 +449,22 @@ func (r *resolver) collectFromPolicies() { for _, policy := range r.policies() { // changed peer IDs have been mapped to changedGroupSet on resolver creation (see seedChangedGroupsFromPeers) // there's no change to the groupSet if the same policies have been changed directly - peerIds, groupIds := getGroupsAndPeersFromPolicyViaGroups(policy, r.changedGroupSet) - addAll(r.groupSet, groupIds) - addAll(r.peerSet, peerIds) + peerIdsViaGroups, groupIdsViaGroups := getGroupsAndPeersFromPolicyViaGroups(policy, r.changedGroupSet) + addAll(r.groupSet, groupIdsViaGroups) + addAll(r.peerSet, peerIdsViaGroups) - peerIds, groupIds = getGroupsAndPeersFromPolicyViaPeers(policy, r.changedPeerSet) - addAll(r.groupSet, groupIds) - addAll(r.peerSet, peerIds) + peerIdsViaPeers, groupIdsViaPeers := getGroupsAndPeersFromPolicyViaPeers(policy, r.changedPeerSet) + addAll(r.groupSet, groupIdsViaPeers) + addAll(r.peerSet, peerIdsViaPeers) - if len(groupIds) == 0 && len(peerIds) == 0 { + hasGroupChanges := len(groupIdsViaPeers) > 0 || len(groupIdsViaGroups) > 0 + hasPeerChanges := len(peerIdsViaPeers) > 0 || len(peerIdsViaGroups) > 0 + if !hasGroupChanges && !hasPeerChanges { continue } log.WithContext(r.ctx).Tracef("collectFromPolicies: policy %s (%s) matched (byGroup=%t byPeer=%t) -> folding rule groups %v + direct peers", - policy.ID, policy.Name, len(groupIds) > 0, len(peerIds) > 0, policy.RuleGroups()) + policy.ID, policy.Name, hasGroupChanges, hasPeerChanges, policy.RuleGroups()) r.matchedPolicies = append(r.matchedPolicies, policy) } } From 56e8215ebe16ff354647fbb7c238a9e2de964dfa Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 24 Jun 2026 14:29:23 +0200 Subject: [PATCH 05/12] updated 'resource-routing-bridge/router-peer-change refreshes policy sources' test to expect router peer among changed peer ids Signed-off-by: Dmitri Dolguikh --- .../server/affected_peers_coverage_test.go | 124 ++++++++---------- 1 file changed, 58 insertions(+), 66 deletions(-) diff --git a/management/server/affected_peers_coverage_test.go b/management/server/affected_peers_coverage_test.go index b8e3327e1..998676c5e 100644 --- a/management/server/affected_peers_coverage_test.go +++ b/management/server/affected_peers_coverage_test.go @@ -8,6 +8,10 @@ import ( "github.com/stretchr/testify/require" "github.com/netbirdio/netbird/management/server/affectedpeers" + resourceTypes "github.com/netbirdio/netbird/management/server/networks/resources/types" + networkTypes "github.com/netbirdio/netbird/management/server/networks/types" + "github.com/netbirdio/netbird/management/server/posture" + "github.com/netbirdio/netbird/management/server/types" ) // TestAffectedPeers_DependencyCoverageMatrix enumerates each network-map @@ -22,88 +26,76 @@ func TestAffectedPeers_DependencyCoverageMatrix(t *testing.T) { } rows := []row{ -<<<<<<< Updated upstream { name: "policy-groups/source-group-change refreshes source+routing, excludes unrelated", build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { _, err := s.manager.SavePolicy(ctx, s.accountID, userID, peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID), true) require.NoError(t, err) return affectedpeers.Change{ChangedGroupIDs: []string{s.sourceGroupID}}, - []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} // TODO (dmitri) routerPeer is missing + []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} }, }, -======= - // { - // name: "policy-groups/source-group-change refreshes source+routing, excludes unrelated", - // build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { - // _, err := s.manager.SavePolicy(ctx, s.accountID, userID, peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID), true) - // require.NoError(t, err) - // return affectedpeers.Change{ChangedGroupIDs: []string{s.sourceGroupID}}, - // []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} - // }, - // }, ->>>>>>> Stashed changes { name: "resource-routing-bridge/router-peer-change refreshes policy sources", build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { _, err := s.manager.SavePolicy(ctx, s.accountID, userID, peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID), true) require.NoError(t, err) return affectedpeers.Change{ChangedPeerIDs: []string{s.routerPeerID}}, - []string{s.sourcePeerID}, []string{s.unrelatedPeerID} + []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} + }, + }, + { + name: "policy-change/explicit-policy refreshes source+routing", + build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { + policy := peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID) + return affectedpeers.Change{Policies: []*types.Policy{policy}}, + []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} + }, + }, + { + name: "policy-destinationresource/explicit-policy bridges to routing peer", + build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { + policy := peerToResourcePolicyByResource(s.sourceGroupID, s.resourceID) + return affectedpeers.Change{Policies: []*types.Policy{policy}}, + []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} + }, + }, + { + name: "resource-change refreshes source+routing on its network", + build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { + _, err := s.manager.SavePolicy(ctx, s.accountID, userID, peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID), true) + require.NoError(t, err) + return affectedpeers.Change{Resources: []*resourceTypes.NetworkResource{ + {ID: s.resourceID, NetworkID: s.networkID, GroupIDs: []string{s.resourceGroupID}}, + }}, + []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} + }, + }, + { + name: "network-change refreshes source+routing on that network", + build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { + _, err := s.manager.SavePolicy(ctx, s.accountID, userID, peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID), true) + require.NoError(t, err) + return affectedpeers.Change{Networks: []*networkTypes.Network{{ID: s.networkID}}}, + []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} + }, + }, + { + name: "posture-check-change refreshes source+routing of gated policy", + build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { + check, err := s.manager.SavePostureChecks(ctx, s.accountID, userID, &posture.Checks{ + Name: "cov-min-version", + Checks: posture.ChecksDefinition{NBVersionCheck: &posture.NBVersionCheck{MinVersion: "0.30.0"}}, + }, true) + require.NoError(t, err) + policy := peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID) + policy.SourcePostureChecks = []string{check.ID} + _, err = s.manager.SavePolicy(ctx, s.accountID, userID, policy, true) + require.NoError(t, err) + return affectedpeers.Change{PostureCheckIDs: []string{check.ID}}, + []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} }, }, - // { - // name: "policy-change/explicit-policy refreshes source+routing", - // build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { - // policy := peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID) - // return affectedpeers.Change{Policies: []*types.Policy{policy}}, - // []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} - // }, - // }, - // { - // name: "policy-destinationresource/explicit-policy bridges to routing peer", - // build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { - // policy := peerToResourcePolicyByResource(s.sourceGroupID, s.resourceID) - // return affectedpeers.Change{Policies: []*types.Policy{policy}}, - // []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} - // }, - // }, - // { - // name: "resource-change refreshes source+routing on its network", - // build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { - // _, err := s.manager.SavePolicy(ctx, s.accountID, userID, peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID), true) - // require.NoError(t, err) - // return affectedpeers.Change{Resources: []*resourceTypes.NetworkResource{ - // {ID: s.resourceID, NetworkID: s.networkID, GroupIDs: []string{s.resourceGroupID}}, - // }}, - // []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} - // }, - // }, - // { - // name: "network-change refreshes source+routing on that network", - // build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { - // _, err := s.manager.SavePolicy(ctx, s.accountID, userID, peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID), true) - // require.NoError(t, err) - // return affectedpeers.Change{Networks: []*networkTypes.Network{{ID: s.networkID}}}, - // []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} - // }, - // }, - // { - // name: "posture-check-change refreshes source+routing of gated policy", - // build: func(t *testing.T, s *routerScenario, ctx context.Context) (affectedpeers.Change, []string, []string) { - // check, err := s.manager.SavePostureChecks(ctx, s.accountID, userID, &posture.Checks{ - // Name: "cov-min-version", - // Checks: posture.ChecksDefinition{NBVersionCheck: &posture.NBVersionCheck{MinVersion: "0.30.0"}}, - // }, true) - // require.NoError(t, err) - // policy := peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID) - // policy.SourcePostureChecks = []string{check.ID} - // _, err = s.manager.SavePolicy(ctx, s.accountID, userID, policy, true) - // require.NoError(t, err) - // return affectedpeers.Change{PostureCheckIDs: []string{check.ID}}, - // []string{s.sourcePeerID, s.routerPeerID}, []string{s.unrelatedPeerID} - // }, - // }, } for _, r := range rows { From 1205641b44e795a4c8aef215d14244de37a04640 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 24 Jun 2026 15:17:41 +0200 Subject: [PATCH 06/12] fixed test Signed-off-by: Dmitri Dolguikh --- .../server/affectedpeers/resolver_test.go | 143 +++++++++++++++++- 1 file changed, 135 insertions(+), 8 deletions(-) diff --git a/management/server/affectedpeers/resolver_test.go b/management/server/affectedpeers/resolver_test.go index 28b718162..62f426549 100644 --- a/management/server/affectedpeers/resolver_test.go +++ b/management/server/affectedpeers/resolver_test.go @@ -84,12 +84,92 @@ func TestGroupsFromPolicyDirectionally(t *testing.T) { policy := &types.Policy{Rules: []*types.PolicyRule{ {Sources: []string{"g1", "g2"}, Destinations: []string{"g3"}}, {Sources: []string{"g4"}, Destinations: []string{"g5", "g6"}}, + {Sources: []string{"g7"}, Destinations: []string{"g8"}, + SourceResource: types.Resource{ID: "r7", Type: types.ResourceTypePeer}, + DestinationResource: types.Resource{ID: "r8", Type: types.ResourceTypePeer}}, + {Sources: []string{"g9"}, Destinations: []string{"g10"}, + SourceResource: types.Resource{ID: "", Type: types.ResourceTypePeer}, + DestinationResource: types.Resource{ID: "", Type: types.ResourceTypePeer}}, + {Sources: []string{"g11"}, Destinations: []string{"g12"}, + SourceResource: types.Resource{ID: "r11", Type: types.ResourceTypeHost}, + DestinationResource: types.Resource{ID: "r12", Type: types.ResourceTypeHost}}, }} - assert.Equal(t, groupsFromPolicyDirectionally(policy, map[string]struct{}{"g1": {}, "g4": {}}), []string{"g3", "g5", "g6"}) - assert.Equal(t, groupsFromPolicyDirectionally(policy, map[string]struct{}{"g3": {}, "g6": {}}), []string{"g1", "g2", "g4"}) - assert.Equal(t, groupsFromPolicyDirectionally(policy, map[string]struct{}{"g33": {}}), []string{}) - assert.Equal(t, groupsFromPolicyDirectionally(policy, map[string]struct{}{}), []string{}) + var tests = []struct { + name string + inGroups map[string]struct{} + expectedPeerIds []string + expectedGroupIds []string + }{ + { + name: "match sources", + inGroups: map[string]struct{}{"g1": {}, "g4": {}}, + expectedPeerIds: []string{}, + expectedGroupIds: []string{"g1", "g4", "g3", "g5", "g6"}, + }, + { + name: "match destinations", + inGroups: map[string]struct{}{"g3": {}, "g6": {}}, + expectedPeerIds: []string{}, + expectedGroupIds: []string{"g1", "g2", "g4", "g3", "g6"}, + }, + { + name: "should return destinations and destination resource", + inGroups: map[string]struct{}{"g7": {}}, + expectedPeerIds: []string{"r8"}, + expectedGroupIds: []string{"g7", "g8"}, + }, + { + name: "should return sources and source resource", + inGroups: map[string]struct{}{"g8": {}}, + expectedPeerIds: []string{"r7"}, + expectedGroupIds: []string{"g7", "g8"}, + }, + { + name: "should not return source resource (empty id)", + inGroups: map[string]struct{}{"g10": {}}, + expectedPeerIds: []string{}, + expectedGroupIds: []string{"g9", "g10"}, + }, + { + name: "should not return destination resource (empty id)", + inGroups: map[string]struct{}{"g9": {}}, + expectedPeerIds: []string{}, + expectedGroupIds: []string{"g9", "g10"}, + }, + { + name: "should not return source resource (non-peer type)", + inGroups: map[string]struct{}{"g12": {}}, + expectedPeerIds: []string{}, + expectedGroupIds: []string{"g11", "g12"}, + }, + { + name: "should not return destination resource (non-peer type)", + inGroups: map[string]struct{}{"g12": {}}, + expectedPeerIds: []string{}, + expectedGroupIds: []string{"g11", "g12"}, + }, + { + name: "non-existing group", + inGroups: map[string]struct{}{"g33": {}}, + expectedPeerIds: []string{}, + expectedGroupIds: []string{}, + }, + { + name: "empty groupset", + inGroups: map[string]struct{}{}, + expectedPeerIds: []string{}, + expectedGroupIds: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + peerIds, groupIds := getGroupsAndPeersFromPolicyViaGroups(policy, tt.inGroups) + assert.ElementsMatch(t, peerIds, tt.expectedPeerIds) + assert.ElementsMatch(t, groupIds, tt.expectedGroupIds) + }) + } } func TestPolicyReferencesDirectPeers(t *testing.T) { @@ -97,33 +177,80 @@ func TestPolicyReferencesDirectPeers(t *testing.T) { { SourceResource: types.Resource{Type: types.ResourceTypePeer, ID: "p1"}, DestinationResource: types.Resource{Type: types.ResourceTypePeer, ID: "r1"}, + Sources: []string{"sg1"}, + Destinations: []string{"dg1"}, }, { SourceResource: types.Resource{Type: types.ResourceTypePeer, ID: "p2"}, DestinationResource: types.Resource{Type: types.ResourceTypePeer, ID: "r2"}, + Sources: []string{"sg2"}, + Destinations: []string{"dg2"}, }, { SourceResource: types.Resource{Type: types.ResourceTypePeer, ID: "p3"}, DestinationResource: types.Resource{Type: types.ResourceTypeHost, ID: "r3"}, + Sources: []string{"sg3"}, + Destinations: []string{"dg3"}, }, { SourceResource: types.Resource{Type: types.ResourceTypeHost, ID: "p4"}, DestinationResource: types.Resource{Type: types.ResourceTypePeer, ID: "r4"}, + Sources: []string{"sg4"}, + Destinations: []string{"dg4"}, }, { SourceResource: types.Resource{Type: types.ResourceTypeHost, ID: "p5"}, DestinationResource: types.Resource{Type: types.ResourceTypePeer, ID: "r5"}, + Sources: []string{"sg5"}, + Destinations: []string{"dg5"}, }, { SourceResource: types.Resource{Type: types.ResourceTypePeer, ID: "p6"}, DestinationResource: types.Resource{Type: types.ResourceTypeHost, ID: "r6"}, + Sources: []string{"sg6"}, + Destinations: []string{"dg6"}, }, }} - assert.Equal(t, []string{"r1", "r2"}, peersFromPolicyDirectionally(policy, map[string]struct{}{"p1": {}, "p2": {}})) - assert.Equal(t, []string{"p1", "p2"}, peersFromPolicyDirectionally(policy, map[string]struct{}{"r1": {}, "r2": {}})) - assert.Empty(t, peersFromPolicyDirectionally(policy, map[string]struct{}{"p3": {}, "r4": {}})) - assert.Empty(t, peersFromPolicyDirectionally(policy, map[string]struct{}{"p5": {}, "r6": {}})) + var tests = []struct { + name string + inGroups map[string]struct{} + expectedPeerIds []string + expectedGroupIds []string + }{ + { + name: "match sources", + inGroups: map[string]struct{}{"p1": {}, "p2": {}}, + expectedPeerIds: []string{"p1", "p2", "r1", "r2"}, + expectedGroupIds: []string{"dg1", "dg2"}, + }, + { + name: "match destinations", + inGroups: map[string]struct{}{"r1": {}, "r2": {}}, + expectedPeerIds: []string{"r1", "r2", "p1", "p2"}, + expectedGroupIds: []string{"sg1", "sg2"}, + }, + { + name: "wrong opposing peer types, only changed peer ids and groups on the opposing end of the rule", + inGroups: map[string]struct{}{"p3": {}, "r4": {}}, + expectedPeerIds: []string{"p3", "r4"}, + expectedGroupIds: []string{"dg3", "sg4"}, + }, + { + name: "wrong peer type, no matching peer ids", + inGroups: map[string]struct{}{"p5": {}, "r6": {}}, + expectedPeerIds: []string{}, + expectedGroupIds: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + peerIds, groupIds := getGroupsAndPeersFromPolicyViaPeers(policy, tt.inGroups) + assert.ElementsMatch(t, peerIds, tt.expectedPeerIds) + assert.ElementsMatch(t, groupIds, tt.expectedGroupIds) + }) + } } func TestPolicyReferencesPostureChecks(t *testing.T) { From d8e7f2e9e67167175d9ad75c05d1ea38e0919946 Mon Sep 17 00:00:00 2001 From: Dmitri Dolguikh Date: Wed, 24 Jun 2026 15:40:44 +0200 Subject: [PATCH 07/12] a couple of fixes Signed-off-by: Dmitri Dolguikh --- .../server/affected_peers_coverage_test.go | 2 +- management/server/affectedpeers/resolver.go | 21 +++++++++------- .../server/affectedpeers/resolver_test.go | 24 ++++++++++++++----- 3 files changed, 32 insertions(+), 15 deletions(-) diff --git a/management/server/affected_peers_coverage_test.go b/management/server/affected_peers_coverage_test.go index 998676c5e..cf9b83617 100644 --- a/management/server/affected_peers_coverage_test.go +++ b/management/server/affected_peers_coverage_test.go @@ -107,7 +107,7 @@ func TestAffectedPeers_DependencyCoverageMatrix(t *testing.T) { affected := resolveAffected(t, s.manager.Store, s.accountID, change) assert.ElementsMatch(t, affected, mustContain, "expected peer to be affected") - assert.NotElementsMatch(t, affected, mustExclude, "peer must not be affected") + assert.NotContains(t, affected, mustExclude, "peer must not be affected") }) } } diff --git a/management/server/affectedpeers/resolver.go b/management/server/affectedpeers/resolver.go index 5ab8a96f7..42b99c461 100644 --- a/management/server/affectedpeers/resolver.go +++ b/management/server/affectedpeers/resolver.go @@ -11,6 +11,8 @@ package affectedpeers import ( "context" + "maps" + "slices" log "github.com/sirupsen/logrus" @@ -774,26 +776,29 @@ func getGroupsAndPeersFromPolicyViaGroups(policy *types.Policy, groupSet map[str // i.e. if a peer is present in the policy rule sourceResources, return destination group IDs and the destinationResource from the rule // and vice-versa func getGroupsAndPeersFromPolicyViaPeers(policy *types.Policy, changedSet map[string]struct{}) ([]string, []string) { - var groupIds, peerIds []string + peerIds := make(map[string]struct{}) + var groupIds []string if len(changedSet) == 0 { - return peerIds, groupIds + return []string{}, groupIds } for _, rule := range policy.Rules { if isDirectPeerInSet(rule.SourceResource, changedSet) { groupIds = append(groupIds, rule.Destinations...) - peerIds = append(peerIds, rule.SourceResource.ID) + peerIds[rule.SourceResource.ID] = struct{}{} if rule.DestinationResource.Type == types.ResourceTypePeer && rule.DestinationResource.ID != "" { - peerIds = append(peerIds, rule.DestinationResource.ID) + peerIds[rule.DestinationResource.ID] = struct{}{} } - } else if isDirectPeerInSet(rule.DestinationResource, changedSet) { + } + // it's possible that the changeSet contains peer ids of both source and destination resources + if isDirectPeerInSet(rule.DestinationResource, changedSet) { groupIds = append(groupIds, rule.Sources...) - peerIds = append(peerIds, rule.DestinationResource.ID) + peerIds[rule.DestinationResource.ID] = struct{}{} if rule.SourceResource.Type == types.ResourceTypePeer && rule.SourceResource.ID != "" { - peerIds = append(peerIds, rule.SourceResource.ID) + peerIds[rule.SourceResource.ID] = struct{}{} } } } - return peerIds, groupIds + return slices.Collect(maps.Keys(peerIds)), groupIds } func policyReferencesPostureChecks(policy *types.Policy, ids map[string]struct{}) bool { diff --git a/management/server/affectedpeers/resolver_test.go b/management/server/affectedpeers/resolver_test.go index 62f426549..08a73c264 100644 --- a/management/server/affectedpeers/resolver_test.go +++ b/management/server/affectedpeers/resolver_test.go @@ -210,43 +210,55 @@ func TestPolicyReferencesDirectPeers(t *testing.T) { Sources: []string{"sg6"}, Destinations: []string{"dg6"}, }, + { + SourceResource: types.Resource{Type: types.ResourceTypePeer, ID: "p7"}, + DestinationResource: types.Resource{Type: types.ResourceTypePeer, ID: "r7"}, + Sources: []string{"sg7"}, + Destinations: []string{"dg7"}, + }, }} var tests = []struct { name string - inGroups map[string]struct{} + changedPeerIds map[string]struct{} expectedPeerIds []string expectedGroupIds []string }{ { name: "match sources", - inGroups: map[string]struct{}{"p1": {}, "p2": {}}, + changedPeerIds: map[string]struct{}{"p1": {}, "p2": {}}, expectedPeerIds: []string{"p1", "p2", "r1", "r2"}, expectedGroupIds: []string{"dg1", "dg2"}, }, { name: "match destinations", - inGroups: map[string]struct{}{"r1": {}, "r2": {}}, + changedPeerIds: map[string]struct{}{"r1": {}, "r2": {}}, expectedPeerIds: []string{"r1", "r2", "p1", "p2"}, expectedGroupIds: []string{"sg1", "sg2"}, }, { name: "wrong opposing peer types, only changed peer ids and groups on the opposing end of the rule", - inGroups: map[string]struct{}{"p3": {}, "r4": {}}, + changedPeerIds: map[string]struct{}{"p3": {}, "r4": {}}, expectedPeerIds: []string{"p3", "r4"}, expectedGroupIds: []string{"dg3", "sg4"}, }, { name: "wrong peer type, no matching peer ids", - inGroups: map[string]struct{}{"p5": {}, "r6": {}}, + changedPeerIds: map[string]struct{}{"p5": {}, "r6": {}}, expectedPeerIds: []string{}, expectedGroupIds: []string{}, }, + { + name: "changed peers on both sides of the policy", + changedPeerIds: map[string]struct{}{"p7": {}, "r7": {}}, + expectedPeerIds: []string{"p7", "r7"}, + expectedGroupIds: []string{"sg7", "dg7"}, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - peerIds, groupIds := getGroupsAndPeersFromPolicyViaPeers(policy, tt.inGroups) + peerIds, groupIds := getGroupsAndPeersFromPolicyViaPeers(policy, tt.changedPeerIds) assert.ElementsMatch(t, peerIds, tt.expectedPeerIds) assert.ElementsMatch(t, groupIds, tt.expectedGroupIds) }) From 62ffa087447b954a16bcaa31a20531347220c3a5 Mon Sep 17 00:00:00 2001 From: pascal Date: Wed, 24 Jun 2026 22:39:35 +0200 Subject: [PATCH 08/12] split networkIDs to check --- management/server/affected_peers_test.go | 8 +- management/server/affectedpeers/resolver.go | 210 ++++++++++-------- .../server/affectedpeers/resolver_test.go | 185 --------------- 3 files changed, 121 insertions(+), 282 deletions(-) diff --git a/management/server/affected_peers_test.go b/management/server/affected_peers_test.go index ed2f558dc..d8966e6c6 100644 --- a/management/server/affected_peers_test.go +++ b/management/server/affected_peers_test.go @@ -697,9 +697,13 @@ func TestResolveAffectedPeers_PeerInMultipleGroups(t *testing.T) { }, true) require.NoError(t, err) - // peer0 is in group0 AND group1, so both policies apply + // peer0 is in group0 AND group1, so both policies apply. A peer change folds + // only the changed peer plus the opposite side of each rule: group2 (peer2) via + // the group0 policy and group3 (peer3) via the group1 policy. peer1, a co-member + // of group1, is a sibling of the changed peer and must NOT refresh. result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) - assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1], peerIDs[2], peerIDs[3]}, result) + assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[2], peerIDs[3]}, result) + assert.NotContains(t, result, peerIDs[1], "co-member of the changed peer's group must not refresh") } func TestResolveAffectedPeers_MultipleChangedPeers(t *testing.T) { diff --git a/management/server/affectedpeers/resolver.go b/management/server/affectedpeers/resolver.go index 42b99c461..9fa2f3b2a 100644 --- a/management/server/affectedpeers/resolver.go +++ b/management/server/affectedpeers/resolver.go @@ -11,8 +11,6 @@ package affectedpeers import ( "context" - "maps" - "slices" log "github.com/sirupsen/logrus" @@ -223,15 +221,17 @@ func Collect(ctx context.Context, s store.Store, accountID string, c Change) (gr func newResolver(ctx context.Context, snap *Snapshot, accountID string, c Change) *resolver { r := &resolver{ - ctx: ctx, - snap: snap, - accountID: accountID, - change: c, - changedGroupSet: toSet(c.ChangedGroupIDs), - changedPeerSet: toSet(c.ChangedPeerIDs), - groupSet: make(map[string]struct{}), - peerSet: make(map[string]struct{}), - networkIDs: make(map[string]struct{}), + ctx: ctx, + snap: snap, + accountID: accountID, + change: c, + changedGroupSet: toSet(c.ChangedGroupIDs), + changedPeerSet: toSet(c.ChangedPeerIDs), + groupSet: make(map[string]struct{}), + peerSet: make(map[string]struct{}), + networkIDs: make(map[string]struct{}), + sourceOriginatedNetworkIDs: make(map[string]struct{}), + changedGroupIDs: toSet(c.ChangedGroupIDs), } // Resolve each changed peer to its groups here so callers pass only ChangedPeerIDs. r.seedChangedGroupsFromPeers() @@ -241,6 +241,9 @@ func newResolver(ctx context.Context, snap *Snapshot, accountID string, c Change // seedChangedGroupsFromPeers adds each changed peer's groups to changedGroupSet so // the group-driven walkers fire for memberships, not just direct peer references. +// These seeded groups are for MATCHING only — folding the changed entity's own +// side is gated on changedGroupIDs (the caller-reported groups), so a seeded group +// never folds its whole membership; only the changed peer itself folds in. func (r *resolver) seedChangedGroupsFromPeers() { if len(r.changedPeerSet) == 0 { return @@ -294,6 +297,18 @@ type resolver struct { matchedPolicies []*types.Policy networkIDs map[string]struct{} + // sourceOriginatedNetworkIDs are networks marked affected only because a + // source-side change targets a resource on them (bridgeSourceToRouters). Their + // routers must refresh, but the policy sources must not be folded back: a + // changed source propagates only to the opposite (router) side, never to its + // co-sources. Networks marked by a router/resource/network change are absent + // here and do fold sources, since the destination side itself changed. + sourceOriginatedNetworkIDs map[string]struct{} + + // changedGroupIDs are the groups the caller reported as changed via + // Change.ChangedGroupIDs (NOT the peer-seeded ones in changedGroupSet). Only + // these fold their whole membership; a peer-seeded group folds the peer alone. + changedGroupIDs map[string]struct{} } func (r *resolver) policies() []*types.Policy { return r.snap.policies } @@ -447,30 +462,88 @@ func (r *resolver) collectFromPostureChecks(postureCheckIDs []string) { } } +// collectFromPolicies folds, for every policy a changed group or peer touches: +// the opposite side of the matching rule, the changed entity's own side (the +// changed group itself, or the changed peer alone — never the changed side's +// sibling groups or co-members), and records the policy for the resource<->router +// bridge. A changed peer is mapped to its groups in changedGroupSet up front (see +// seedChangedGroupsFromPeers); changedGroupIDs holds only the caller-reported +// groups, so a peer-seeded group does not fold its whole membership. func (r *resolver) collectFromPolicies() { for _, policy := range r.policies() { - // changed peer IDs have been mapped to changedGroupSet on resolver creation (see seedChangedGroupsFromPeers) - // there's no change to the groupSet if the same policies have been changed directly - peerIdsViaGroups, groupIdsViaGroups := getGroupsAndPeersFromPolicyViaGroups(policy, r.changedGroupSet) - addAll(r.groupSet, groupIdsViaGroups) - addAll(r.peerSet, peerIdsViaGroups) - - peerIdsViaPeers, groupIdsViaPeers := getGroupsAndPeersFromPolicyViaPeers(policy, r.changedPeerSet) - addAll(r.groupSet, groupIdsViaPeers) - addAll(r.peerSet, peerIdsViaPeers) - - hasGroupChanges := len(groupIdsViaPeers) > 0 || len(groupIdsViaGroups) > 0 - hasPeerChanges := len(peerIdsViaPeers) > 0 || len(peerIdsViaGroups) > 0 - if !hasGroupChanges && !hasPeerChanges { + if !r.collectPolicyDirectional(policy) { continue } - - log.WithContext(r.ctx).Tracef("collectFromPolicies: policy %s (%s) matched (byGroup=%t byPeer=%t) -> folding rule groups %v + direct peers", - policy.ID, policy.Name, hasGroupChanges, hasPeerChanges, policy.RuleGroups()) + log.WithContext(r.ctx).Tracef("collectFromPolicies: policy %s (%s) matched directionally", policy.ID, policy.Name) r.matchedPolicies = append(r.matchedPolicies, policy) } } +// collectPolicyDirectional folds one policy's affected groups/peers and reports +// whether it matched a changed group or peer at all (so the caller can record it +// for the bridge even when the opposite side is a resource, not a group). +func (r *resolver) collectPolicyDirectional(policy *types.Policy) bool { + matched := false + for _, rule := range policy.Rules { + matched = r.foldRuleSide(rule.Sources, rule.Destinations, rule.DestinationResource) || matched + matched = r.foldRuleSide(rule.Destinations, rule.Sources, rule.SourceResource) || matched + + if isDirectPeerInSet(rule.SourceResource, r.changedPeerSet) { + r.peerSet[rule.SourceResource.ID] = struct{}{} + addAll(r.groupSet, rule.Destinations) + if rule.DestinationResource.Type == types.ResourceTypePeer && rule.DestinationResource.ID != "" { + r.peerSet[rule.DestinationResource.ID] = struct{}{} + } + matched = true + } + if isDirectPeerInSet(rule.DestinationResource, r.changedPeerSet) { + r.peerSet[rule.DestinationResource.ID] = struct{}{} + addAll(r.groupSet, rule.Sources) + if rule.SourceResource.Type == types.ResourceTypePeer && rule.SourceResource.ID != "" { + r.peerSet[rule.SourceResource.ID] = struct{}{} + } + matched = true + } + } + return matched +} + +// foldRuleSide handles a changed group on `near` (Sources or Destinations): it +// folds the `far` (opposite) groups and far resource peer, the changed group(s) +// themselves (caller-reported groups only — not seeded ones, so a changed peer's +// group does not pull in its members), and the changed peers seeded from those +// groups (the peer alone). Returns whether the side matched. +func (r *resolver) foldRuleSide(near, far []string, farResource types.Resource) bool { + if !anyInSet(near, r.changedGroupSet) { + return false + } + addAll(r.groupSet, far) + if farResource.Type == types.ResourceTypePeer && farResource.ID != "" { + r.peerSet[farResource.ID] = struct{}{} + } + for _, gID := range near { + if _, ok := r.changedGroupIDs[gID]; ok { + r.groupSet[gID] = struct{}{} // changed group itself -> its members + } + r.foldChangedPeersInGroup(gID) // a changed peer in this group -> the peer alone + } + return true +} + +// foldChangedPeersInGroup folds changed peers that belong to groupID directly into +// peerSet (the peer only, never its co-members). +func (r *resolver) foldChangedPeersInGroup(groupID string) { + if len(r.changedPeerSet) == 0 { + return + } + members := r.snap.groupPeers[groupID] + for pID := range r.changedPeerSet { + if _, ok := members[pID]; ok { + r.peerSet[pID] = struct{}{} + } + } +} + func (r *resolver) collectFromRoutes() { for _, rt := range r.snap.routes { matchedByGroup := anyInSet(rt.Groups, r.changedGroupSet) || anyInSet(rt.PeerGroups, r.changedGroupSet) || anyInSet(rt.AccessControlGroups, r.changedGroupSet) @@ -599,6 +672,11 @@ func (r *resolver) bridgeSourceToRouters() { log.WithContext(r.ctx).Tracef("bridgeSourceToRouters: targeted resources %v -> networks %v (their routers become affected via the router->source pass)", setToSlice(resourceIDs), setToSlice(networkIDs)) for id := range networkIDs { + // Mark source-originated unless a router/resource/network change already + // marked this network directly (then it folds sources back). + if _, ok := r.networkIDs[id]; !ok { + r.sourceOriginatedNetworkIDs[id] = struct{}{} + } r.networkIDs[id] = struct{}{} } } @@ -613,11 +691,19 @@ func (r *resolver) bridgeRoutersToSources() { r.foldRoutersOnNetworks(r.networkIDs) + // Sources are folded back only for networks the destination side itself changed + // (router/resource/network change). Networks reached only because a source-side + // change targets their resource must not refresh the policy's sources — the + // changed source propagates to the router side, not back to its co-sources. resourceIDs := make(map[string]struct{}) for _, resource := range r.networkResources() { - if _, ok := r.networkIDs[resource.NetworkID]; ok { - resourceIDs[resource.ID] = struct{}{} + if _, ok := r.networkIDs[resource.NetworkID]; !ok { + continue } + if _, sourceOriginated := r.sourceOriginatedNetworkIDs[resource.NetworkID]; sourceOriginated { + continue + } + resourceIDs[resource.ID] = struct{}{} } if len(resourceIDs) == 0 { return @@ -745,62 +831,6 @@ func collectPolicySources(policy *types.Policy, groupSet, peerSet map[string]str } } -// returns group and peer IDs on the opposite side of the policy: -// i.e. if a group is present in the policy rule sources, return destination group IDs and the destinationResource from the rule -// and vice-versa -func getGroupsAndPeersFromPolicyViaGroups(policy *types.Policy, groupSet map[string]struct{}) ([]string, []string) { - var groupIds, peerIds []string - if len(groupSet) == 0 { - return peerIds, groupIds - } - for _, rule := range policy.Rules { - if matchedIds, ok := allInSet(rule.Sources, groupSet); ok { - groupIds = append(groupIds, matchedIds...) - groupIds = append(groupIds, rule.Destinations...) - if rule.DestinationResource.Type == types.ResourceTypePeer && rule.DestinationResource.ID != "" { - peerIds = append(peerIds, rule.DestinationResource.ID) - } - } - if matchedIds, ok := allInSet(rule.Destinations, groupSet); ok { - groupIds = append(groupIds, matchedIds...) - groupIds = append(groupIds, rule.Sources...) - if rule.SourceResource.Type == types.ResourceTypePeer && rule.SourceResource.ID != "" { - peerIds = append(peerIds, rule.SourceResource.ID) - } - } - } - return peerIds, groupIds -} - -// returns group and peer IDs on the opposite side of the policy: -// i.e. if a peer is present in the policy rule sourceResources, return destination group IDs and the destinationResource from the rule -// and vice-versa -func getGroupsAndPeersFromPolicyViaPeers(policy *types.Policy, changedSet map[string]struct{}) ([]string, []string) { - peerIds := make(map[string]struct{}) - var groupIds []string - if len(changedSet) == 0 { - return []string{}, groupIds - } - for _, rule := range policy.Rules { - if isDirectPeerInSet(rule.SourceResource, changedSet) { - groupIds = append(groupIds, rule.Destinations...) - peerIds[rule.SourceResource.ID] = struct{}{} - if rule.DestinationResource.Type == types.ResourceTypePeer && rule.DestinationResource.ID != "" { - peerIds[rule.DestinationResource.ID] = struct{}{} - } - } - // it's possible that the changeSet contains peer ids of both source and destination resources - if isDirectPeerInSet(rule.DestinationResource, changedSet) { - groupIds = append(groupIds, rule.Sources...) - peerIds[rule.DestinationResource.ID] = struct{}{} - if rule.SourceResource.Type == types.ResourceTypePeer && rule.SourceResource.ID != "" { - peerIds[rule.SourceResource.ID] = struct{}{} - } - } - } - return slices.Collect(maps.Keys(peerIds)), groupIds -} - func policyReferencesPostureChecks(policy *types.Policy, ids map[string]struct{}) bool { for _, id := range policy.SourcePostureChecks { if _, ok := ids[id]; ok { @@ -844,16 +874,6 @@ func anyInSet(ids []string, set map[string]struct{}) bool { return false } -func allInSet(ids []string, set map[string]struct{}) ([]string, bool) { - var matchedIds []string - for _, id := range ids { - if _, ok := set[id]; ok { - matchedIds = append(matchedIds, id) - } - } - return matchedIds, len(matchedIds) > 0 -} - func isInSet(id string, set map[string]struct{}) bool { _, ok := set[id] return ok diff --git a/management/server/affectedpeers/resolver_test.go b/management/server/affectedpeers/resolver_test.go index 08a73c264..775dccb86 100644 --- a/management/server/affectedpeers/resolver_test.go +++ b/management/server/affectedpeers/resolver_test.go @@ -80,191 +80,6 @@ func TestChangeIsEmpty(t *testing.T) { assert.False(t, Change{PostureCheckIDs: []string{"pc"}}.isEmpty()) } -func TestGroupsFromPolicyDirectionally(t *testing.T) { - policy := &types.Policy{Rules: []*types.PolicyRule{ - {Sources: []string{"g1", "g2"}, Destinations: []string{"g3"}}, - {Sources: []string{"g4"}, Destinations: []string{"g5", "g6"}}, - {Sources: []string{"g7"}, Destinations: []string{"g8"}, - SourceResource: types.Resource{ID: "r7", Type: types.ResourceTypePeer}, - DestinationResource: types.Resource{ID: "r8", Type: types.ResourceTypePeer}}, - {Sources: []string{"g9"}, Destinations: []string{"g10"}, - SourceResource: types.Resource{ID: "", Type: types.ResourceTypePeer}, - DestinationResource: types.Resource{ID: "", Type: types.ResourceTypePeer}}, - {Sources: []string{"g11"}, Destinations: []string{"g12"}, - SourceResource: types.Resource{ID: "r11", Type: types.ResourceTypeHost}, - DestinationResource: types.Resource{ID: "r12", Type: types.ResourceTypeHost}}, - }} - - var tests = []struct { - name string - inGroups map[string]struct{} - expectedPeerIds []string - expectedGroupIds []string - }{ - { - name: "match sources", - inGroups: map[string]struct{}{"g1": {}, "g4": {}}, - expectedPeerIds: []string{}, - expectedGroupIds: []string{"g1", "g4", "g3", "g5", "g6"}, - }, - { - name: "match destinations", - inGroups: map[string]struct{}{"g3": {}, "g6": {}}, - expectedPeerIds: []string{}, - expectedGroupIds: []string{"g1", "g2", "g4", "g3", "g6"}, - }, - { - name: "should return destinations and destination resource", - inGroups: map[string]struct{}{"g7": {}}, - expectedPeerIds: []string{"r8"}, - expectedGroupIds: []string{"g7", "g8"}, - }, - { - name: "should return sources and source resource", - inGroups: map[string]struct{}{"g8": {}}, - expectedPeerIds: []string{"r7"}, - expectedGroupIds: []string{"g7", "g8"}, - }, - { - name: "should not return source resource (empty id)", - inGroups: map[string]struct{}{"g10": {}}, - expectedPeerIds: []string{}, - expectedGroupIds: []string{"g9", "g10"}, - }, - { - name: "should not return destination resource (empty id)", - inGroups: map[string]struct{}{"g9": {}}, - expectedPeerIds: []string{}, - expectedGroupIds: []string{"g9", "g10"}, - }, - { - name: "should not return source resource (non-peer type)", - inGroups: map[string]struct{}{"g12": {}}, - expectedPeerIds: []string{}, - expectedGroupIds: []string{"g11", "g12"}, - }, - { - name: "should not return destination resource (non-peer type)", - inGroups: map[string]struct{}{"g12": {}}, - expectedPeerIds: []string{}, - expectedGroupIds: []string{"g11", "g12"}, - }, - { - name: "non-existing group", - inGroups: map[string]struct{}{"g33": {}}, - expectedPeerIds: []string{}, - expectedGroupIds: []string{}, - }, - { - name: "empty groupset", - inGroups: map[string]struct{}{}, - expectedPeerIds: []string{}, - expectedGroupIds: []string{}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - peerIds, groupIds := getGroupsAndPeersFromPolicyViaGroups(policy, tt.inGroups) - assert.ElementsMatch(t, peerIds, tt.expectedPeerIds) - assert.ElementsMatch(t, groupIds, tt.expectedGroupIds) - }) - } -} - -func TestPolicyReferencesDirectPeers(t *testing.T) { - policy := &types.Policy{Rules: []*types.PolicyRule{ - { - SourceResource: types.Resource{Type: types.ResourceTypePeer, ID: "p1"}, - DestinationResource: types.Resource{Type: types.ResourceTypePeer, ID: "r1"}, - Sources: []string{"sg1"}, - Destinations: []string{"dg1"}, - }, - { - SourceResource: types.Resource{Type: types.ResourceTypePeer, ID: "p2"}, - DestinationResource: types.Resource{Type: types.ResourceTypePeer, ID: "r2"}, - Sources: []string{"sg2"}, - Destinations: []string{"dg2"}, - }, - { - SourceResource: types.Resource{Type: types.ResourceTypePeer, ID: "p3"}, - DestinationResource: types.Resource{Type: types.ResourceTypeHost, ID: "r3"}, - Sources: []string{"sg3"}, - Destinations: []string{"dg3"}, - }, - { - SourceResource: types.Resource{Type: types.ResourceTypeHost, ID: "p4"}, - DestinationResource: types.Resource{Type: types.ResourceTypePeer, ID: "r4"}, - Sources: []string{"sg4"}, - Destinations: []string{"dg4"}, - }, - { - SourceResource: types.Resource{Type: types.ResourceTypeHost, ID: "p5"}, - DestinationResource: types.Resource{Type: types.ResourceTypePeer, ID: "r5"}, - Sources: []string{"sg5"}, - Destinations: []string{"dg5"}, - }, - { - SourceResource: types.Resource{Type: types.ResourceTypePeer, ID: "p6"}, - DestinationResource: types.Resource{Type: types.ResourceTypeHost, ID: "r6"}, - Sources: []string{"sg6"}, - Destinations: []string{"dg6"}, - }, - { - SourceResource: types.Resource{Type: types.ResourceTypePeer, ID: "p7"}, - DestinationResource: types.Resource{Type: types.ResourceTypePeer, ID: "r7"}, - Sources: []string{"sg7"}, - Destinations: []string{"dg7"}, - }, - }} - - var tests = []struct { - name string - changedPeerIds map[string]struct{} - expectedPeerIds []string - expectedGroupIds []string - }{ - { - name: "match sources", - changedPeerIds: map[string]struct{}{"p1": {}, "p2": {}}, - expectedPeerIds: []string{"p1", "p2", "r1", "r2"}, - expectedGroupIds: []string{"dg1", "dg2"}, - }, - { - name: "match destinations", - changedPeerIds: map[string]struct{}{"r1": {}, "r2": {}}, - expectedPeerIds: []string{"r1", "r2", "p1", "p2"}, - expectedGroupIds: []string{"sg1", "sg2"}, - }, - { - name: "wrong opposing peer types, only changed peer ids and groups on the opposing end of the rule", - changedPeerIds: map[string]struct{}{"p3": {}, "r4": {}}, - expectedPeerIds: []string{"p3", "r4"}, - expectedGroupIds: []string{"dg3", "sg4"}, - }, - { - name: "wrong peer type, no matching peer ids", - changedPeerIds: map[string]struct{}{"p5": {}, "r6": {}}, - expectedPeerIds: []string{}, - expectedGroupIds: []string{}, - }, - { - name: "changed peers on both sides of the policy", - changedPeerIds: map[string]struct{}{"p7": {}, "r7": {}}, - expectedPeerIds: []string{"p7", "r7"}, - expectedGroupIds: []string{"sg7", "dg7"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - peerIds, groupIds := getGroupsAndPeersFromPolicyViaPeers(policy, tt.changedPeerIds) - assert.ElementsMatch(t, peerIds, tt.expectedPeerIds) - assert.ElementsMatch(t, groupIds, tt.expectedGroupIds) - }) - } -} - func TestPolicyReferencesPostureChecks(t *testing.T) { policy := &types.Policy{SourcePostureChecks: []string{"pc1", "pc2"}} From c2db940a8cddae88299680e2c02793944cb8fba8 Mon Sep 17 00:00:00 2001 From: pascal Date: Thu, 25 Jun 2026 00:01:17 +0200 Subject: [PATCH 09/12] simplify affected peers walk --- management/server/affectedpeers/resolver.go | 636 +++++++++++--------- 1 file changed, 359 insertions(+), 277 deletions(-) diff --git a/management/server/affectedpeers/resolver.go b/management/server/affectedpeers/resolver.go index 9fa2f3b2a..19818ca3a 100644 --- a/management/server/affectedpeers/resolver.go +++ b/management/server/affectedpeers/resolver.go @@ -216,62 +216,81 @@ func Collect(ctx context.Context, s store.Store, accountID string, c Change) (gr } r := newResolver(ctx, snap, accountID, c) r.walk() - return setToSlice(r.groupSet), setToSlice(r.peerSet) + return setToSlice(r.affectedGroups), setToSlice(r.affectedPeers) } func newResolver(ctx context.Context, snap *Snapshot, accountID string, c Change) *resolver { r := &resolver{ - ctx: ctx, - snap: snap, - accountID: accountID, - change: c, - changedGroupSet: toSet(c.ChangedGroupIDs), - changedPeerSet: toSet(c.ChangedPeerIDs), - groupSet: make(map[string]struct{}), - peerSet: make(map[string]struct{}), - networkIDs: make(map[string]struct{}), - sourceOriginatedNetworkIDs: make(map[string]struct{}), - changedGroupIDs: toSet(c.ChangedGroupIDs), + ctx: ctx, + snap: snap, + accountID: accountID, + change: c, + linkGroups: toSet(c.ChangedGroupIDs), + outputGroups: toSet(c.ChangedGroupIDs), + changedPeers: toSet(c.ChangedPeerIDs), + affectedGroups: make(map[string]struct{}), + affectedPeers: make(map[string]struct{}), } // Resolve each changed peer to its groups here so callers pass only ChangedPeerIDs. r.seedChangedGroupsFromPeers() - r.matchedPolicies = append(r.matchedPolicies, c.Policies...) return r } -// seedChangedGroupsFromPeers adds each changed peer's groups to changedGroupSet so +// seedChangedGroupsFromPeers adds each changed peer's groups to linkGroups so // the group-driven walkers fire for memberships, not just direct peer references. // These seeded groups are for MATCHING only — folding the changed entity's own -// side is gated on changedGroupIDs (the caller-reported groups), so a seeded group +// side is gated on outputGroups (the caller-reported groups), so a seeded group // never folds its whole membership; only the changed peer itself folds in. func (r *resolver) seedChangedGroupsFromPeers() { - if len(r.changedPeerSet) == 0 { + if len(r.changedPeers) == 0 { return } for groupID, members := range r.snap.groupPeers { - for pID := range r.changedPeerSet { + for pID := range r.changedPeers { if _, ok := members[pID]; ok { - r.changedGroupSet[groupID] = struct{}{} + r.linkGroups[groupID] = struct{}{} break } } } } +// policySide selects which side of a policy rule to walk. +type policySide int + +const ( + sideSource policySide = iota + sideDestination +) + +func (s policySide) opposite() policySide { + if s == sideSource { + return sideDestination + } + return sideSource +} + +// walk resolves affected peers in two buckets, by how far each change propagates. +// +// BOTH-SIDES — the rule itself changed (an explicit policy edit, or a policy whose +// posture check changed). Source AND destination refresh, so each such policy is +// walked on both sides. +// +// OPPOSITE-SIDE — an endpoint moved but no rule changed. For each policy the change +// touches we fold only the side AWAY from the change: +// - a changed peer/group sits ON a policy side -> fold the opposite side; +// - a changed router/resource/network sits on a NETWORK -> fold the SOURCE side of +// the policies whose destination reaches it (and the routers it implies). +// +// Routes, nameserver groups, DNS and embedded-proxy services distribute to their own +// member peers, outside the policy graph, and are folded here too. func (r *resolver) walk() { - r.collectFromExplicitPolicies() - r.collectFromExplicitRoutes(r.change.Routes) - r.collectFromExplicitRouters(r.change.Routers) - r.collectFromExplicitResources(r.change.Resources) - r.collectFromExplicitNetworks(r.change.Networks) - r.collectFromPostureChecks(r.change.PostureCheckIDs) + for _, policy := range r.bothSidesPolicies() { + r.foldPolicySide(policy, sideSource) + r.foldPolicySide(policy, sideDestination) + } - // Distribution groups (nameserver/DNS) affect only their member peers: fold them - // straight into groupSet so expand() maps them to members, without the policy/ - // route walk that changedGroupSet would trigger. - addAll(r.groupSet, r.change.DistributionGroupIDs) - - if len(r.changedGroupSet) > 0 || len(r.changedPeerSet) > 0 { + if len(r.linkGroups) > 0 || len(r.changedPeers) > 0 { r.collectFromPolicies() r.collectFromRoutes() r.collectFromNameServers() @@ -280,7 +299,22 @@ func (r *resolver) walk() { r.collectFromProxyServices() } - r.collectResourceRouterBridge() + r.collectFromChangedRoutes(r.change.Routes) + r.collectFromChangedRouters(r.change.Routers) + r.collectFromChangedResources(r.change.Resources) + r.collectFromChangedNetworks(r.change.Networks) + + // 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. + addAll(r.affectedGroups, r.change.DistributionGroupIDs) +} + +// bothSidesPolicies are the policies whose rule changed: the explicitly edited ones +// plus those gated by a changed posture check. walk folds both their sides. +func (r *resolver) bothSidesPolicies() []*types.Policy { + policies := append([]*types.Policy(nil), r.change.Policies...) + return r.appendPoliciesForPostureChecks(policies, r.change.PostureCheckIDs) } type resolver struct { @@ -289,26 +323,25 @@ type resolver struct { accountID string change Change - changedGroupSet map[string]struct{} - changedPeerSet map[string]struct{} + // Inputs — what changed. Set once at construction, read-only during the walk + // (except linkGroups, which collectFromExplicitResources also seeds). + // + // linkGroups is the MATCH set: caller-changed groups ∪ the groups of changed + // peers ∪ changed-resource groups. A rule/route/router matches the change when + // one of its groups is here — used only to find the opposite side to fold. + // + // outputGroups is the FOLD-WHOLE-GROUP set: ONLY Change.ChangedGroupIDs. When a + // matched group is here, its whole membership is affected. A peer-seeded group + // is in linkGroups but NOT outputGroups, so it folds only the changed peer + // (changedPeers), never its siblings. + linkGroups map[string]struct{} + outputGroups map[string]struct{} + changedPeers map[string]struct{} - groupSet map[string]struct{} - peerSet map[string]struct{} - - matchedPolicies []*types.Policy - networkIDs map[string]struct{} - // sourceOriginatedNetworkIDs are networks marked affected only because a - // source-side change targets a resource on them (bridgeSourceToRouters). Their - // routers must refresh, but the policy sources must not be folded back: a - // changed source propagates only to the opposite (router) side, never to its - // co-sources. Networks marked by a router/resource/network change are absent - // here and do fold sources, since the destination side itself changed. - sourceOriginatedNetworkIDs map[string]struct{} - - // changedGroupIDs are the groups the caller reported as changed via - // Change.ChangedGroupIDs (NOT the peer-seeded ones in changedGroupSet). Only - // these fold their whole membership; a peer-seeded group folds the peer alone. - changedGroupIDs map[string]struct{} + // Outputs — the answer. The only sets the walk accumulates into. affectedGroups + // is expanded to its member peers in expand(). + affectedGroups map[string]struct{} + affectedPeers map[string]struct{} } func (r *resolver) policies() []*types.Policy { return r.snap.policies } @@ -318,10 +351,10 @@ func (r *resolver) networkResources() []*resourceTypes.NetworkResource { return func (r *resolver) networkRouters() []*routerTypes.NetworkRouter { return r.snap.routers } // peerIDsForGroups maps a group set to its member peer IDs via the preloaded index. -func (r *resolver) peerIDsForGroups(groupSet map[string]struct{}) []string { +func (r *resolver) peerIDsForGroups(groups map[string]struct{}) []string { seen := make(map[string]struct{}) var ids []string - for gID := range groupSet { + for gID := range groups { for pID := range r.snap.groupPeers[gID] { if _, ok := seen[pID]; ok { continue @@ -334,25 +367,25 @@ func (r *resolver) peerIDsForGroups(groupSet map[string]struct{}) []string { } func (r *resolver) expand() []string { - peerIDs := r.peerIDsForGroups(r.groupSet) + peerIDs := r.peerIDsForGroups(r.affectedGroups) log.WithContext(r.ctx).Tracef("affectedpeers expand: account=%s affectedGroups=%v -> %d group-member peers; direct peers=%v", - r.accountID, setToSlice(r.groupSet), len(peerIDs), setToSlice(r.peerSet)) + r.accountID, setToSlice(r.affectedGroups), len(peerIDs), setToSlice(r.affectedPeers)) seen := make(map[string]struct{}, len(peerIDs)) for _, id := range peerIDs { seen[id] = struct{}{} } - for id := range r.peerSet { + for id := range r.affectedPeers { if _, ok := seen[id]; !ok { peerIDs = append(peerIDs, id) seen[id] = struct{}{} } } - // Fold in removed peers only when their group is linked (in groupSet). + // Fold in removed peers only when their group is linked (in affectedGroups). for groupID, removed := range r.change.RemovedPeersByGroup { - if _, linked := r.groupSet[groupID]; !linked { + if _, linked := r.affectedGroups[groupID]; !linked { continue } for _, id := range removed { @@ -368,236 +401,316 @@ func (r *resolver) expand() []string { return peerIDs } -func (r *resolver) collectFromExplicitPolicies() { - for _, policy := range r.matchedPolicies { - if policy == nil { - continue - } - log.WithContext(r.ctx).Tracef("collectFromExplicitPolicies: changed policy %s (%s) -> folding rule groups %v + direct peers", - policy.ID, policy.Name, policy.RuleGroups()) - addAll(r.groupSet, policy.RuleGroups()) - collectPolicyDirectPeers(policy, r.peerSet) +// ruleSideGroups / ruleSideResource return the groups and the resource on the given +// side of a rule. +func ruleSideGroups(rule *types.PolicyRule, side policySide) []string { + if side == sideDestination { + return rule.Destinations } + return rule.Sources } -func (r *resolver) collectFromExplicitRoutes(routes []*route.Route) { - for _, rt := range routes { - if rt == nil { - continue - } - log.WithContext(r.ctx).Tracef("collectFromExplicitRoutes: changed route %s -> folding groups=%v peerGroups=%v accessControlGroups=%v peer=%q", - rt.ID, rt.Groups, rt.PeerGroups, rt.AccessControlGroups, rt.Peer) - addAll(r.groupSet, rt.Groups, rt.PeerGroups, rt.AccessControlGroups) - if rt.Peer != "" { - r.peerSet[rt.Peer] = struct{}{} - } +func ruleSideResource(rule *types.PolicyRule, side policySide) types.Resource { + if side == sideDestination { + return rule.DestinationResource } + return rule.SourceResource } -// collectFromExplicitRouters folds changed routers' peers and marks their networks -// for the bridge. Passing the old router keeps a repointed router's previous peers -// affected without a post-commit read. -func (r *resolver) collectFromExplicitRouters(routers []*routerTypes.NetworkRouter) { - for _, router := range routers { - if router == nil { - continue - } - log.WithContext(r.ctx).Tracef("collectFromExplicitRouters: changed router %s on network %s -> folding peerGroups=%v peer=%q and marking network for source bridge", - router.ID, router.NetworkID, router.PeerGroups, router.Peer) - addAll(r.groupSet, router.PeerGroups) - if router.Peer != "" { - r.peerSet[router.Peer] = struct{}{} - } - if router.NetworkID != "" { - r.networkIDs[router.NetworkID] = struct{}{} - } - } -} - -// collectFromExplicitResources marks changed resources' networks for the bridge and -// treats their group IDs as changed, so policies targeting the resource via a -// now-detached (old) group still refresh. -func (r *resolver) collectFromExplicitResources(resources []*resourceTypes.NetworkResource) { - for _, resource := range resources { - if resource == nil { - continue - } - log.WithContext(r.ctx).Tracef("collectFromExplicitResources: changed resource %s on network %s -> marking network for bridge and treating groups %v as changed", - resource.ID, resource.NetworkID, resource.GroupIDs) - addAll(r.changedGroupSet, resource.GroupIDs) - if resource.NetworkID != "" { - r.networkIDs[resource.NetworkID] = struct{}{} - } - } -} - -// collectFromExplicitNetworks marks changed networks for the bridge. A network has -// no groups/peers of its own. -func (r *resolver) collectFromExplicitNetworks(networks []*networkTypes.Network) { - for _, network := range networks { - if network == nil { - continue - } - log.WithContext(r.ctx).Tracef("collectFromExplicitNetworks: changed network %s -> marking for bridge", network.ID) - if network.ID != "" { - r.networkIDs[network.ID] = struct{}{} - } - } -} - -func (r *resolver) collectFromPostureChecks(postureCheckIDs []string) { - if len(postureCheckIDs) == 0 { +// foldPolicySide folds one side of a policy down to affected peers: its groups +// (resolved to members in expand) and its direct peer. When the side is the +// DESTINATION and references a network resource (directly or via a destination +// group's resources), it also folds the routers that serve that resource's network +// — a destination resource is reached through its routers. A resource on the SOURCE +// side routes to nobody (GetPoliciesForNetworkResource matches destinations only), +// so the router hop is destination-only. +func (r *resolver) foldPolicySide(policy *types.Policy, side policySide) { + if policy == nil { return } + for _, rule := range policy.Rules { + addAll(r.affectedGroups, ruleSideGroups(rule, side)) + res := ruleSideResource(rule, side) + if res.Type == types.ResourceTypePeer && res.ID != "" { + r.affectedPeers[res.ID] = struct{}{} + } + } + if side == sideDestination { + r.foldRoutersForResources(r.policyDestinationResourceIDs(policy)) + } +} + +// appendPoliciesForPostureChecks appends every policy that references a changed +// posture check (a rule change, so walk both sides). +func (r *resolver) appendPoliciesForPostureChecks(policies []*types.Policy, postureCheckIDs []string) []*types.Policy { + if len(postureCheckIDs) == 0 { + return policies + } ids := toSet(postureCheckIDs) for _, policy := range r.policies() { if !policyReferencesPostureChecks(policy, ids) { continue } - log.WithContext(r.ctx).Tracef("collectFromPostureChecks: policy %s (%s) references changed posture checks %v -> folding rule groups %v + direct peers", - policy.ID, policy.Name, postureCheckIDs, policy.RuleGroups()) - addAll(r.groupSet, policy.RuleGroups()) - collectPolicyDirectPeers(policy, r.peerSet) - r.matchedPolicies = append(r.matchedPolicies, policy) + log.WithContext(r.ctx).Tracef("appendPoliciesForPostureChecks: policy %s (%s) references changed posture checks %v -> both-sides policy", + policy.ID, policy.Name, postureCheckIDs) + policies = append(policies, policy) } + return policies } -// collectFromPolicies folds, for every policy a changed group or peer touches: -// the opposite side of the matching rule, the changed entity's own side (the -// changed group itself, or the changed peer alone — never the changed side's -// sibling groups or co-members), and records the policy for the resource<->router -// bridge. A changed peer is mapped to its groups in changedGroupSet up front (see -// seedChangedGroupsFromPeers); changedGroupIDs holds only the caller-reported -// groups, so a peer-seeded group does not fold its whole membership. +// collectFromPolicies folds, for every policy whose rule a changed group or peer +// touches, only the OPPOSITE side (down to peers, incl. destination routers), plus +// the changed entity's own side: the changed group's whole membership when the +// group itself changed (outputGroups), or the changed peer alone when matched via a +// peer-seeded group (never its co-members). func (r *resolver) collectFromPolicies() { for _, policy := range r.policies() { - if !r.collectPolicyDirectional(policy) { - continue + for _, rule := range policy.Rules { + r.foldRuleSideIfChanged(policy, rule, sideSource) + r.foldRuleSideIfChanged(policy, rule, sideDestination) } - log.WithContext(r.ctx).Tracef("collectFromPolicies: policy %s (%s) matched directionally", policy.ID, policy.Name) - r.matchedPolicies = append(r.matchedPolicies, policy) } } -// collectPolicyDirectional folds one policy's affected groups/peers and reports -// whether it matched a changed group or peer at all (so the caller can record it -// for the bridge even when the opposite side is a resource, not a group). -func (r *resolver) collectPolicyDirectional(policy *types.Policy) bool { - matched := false - for _, rule := range policy.Rules { - matched = r.foldRuleSide(rule.Sources, rule.Destinations, rule.DestinationResource) || matched - matched = r.foldRuleSide(rule.Destinations, rule.Sources, rule.SourceResource) || matched +// foldRuleSideIfChanged: when a changed group or direct peer sits on `side` of the +// rule, fold the opposite side fully (groups/peers + destination routers) and fold +// the changed entity's own side (the whole changed group, or the changed peer alone). +func (r *resolver) foldRuleSideIfChanged(policy *types.Policy, rule *types.PolicyRule, side policySide) { + nearGroups := ruleSideGroups(rule, side) + nearResource := ruleSideResource(rule, side) - if isDirectPeerInSet(rule.SourceResource, r.changedPeerSet) { - r.peerSet[rule.SourceResource.ID] = struct{}{} - addAll(r.groupSet, rule.Destinations) - if rule.DestinationResource.Type == types.ResourceTypePeer && rule.DestinationResource.ID != "" { - r.peerSet[rule.DestinationResource.ID] = struct{}{} - } - matched = true - } - if isDirectPeerInSet(rule.DestinationResource, r.changedPeerSet) { - r.peerSet[rule.DestinationResource.ID] = struct{}{} - addAll(r.groupSet, rule.Sources) - if rule.SourceResource.Type == types.ResourceTypePeer && rule.SourceResource.ID != "" { - r.peerSet[rule.SourceResource.ID] = struct{}{} - } - matched = true - } + matchedByGroup := anyInSet(nearGroups, r.linkGroups) + matchedByPeer := isDirectPeerInSet(nearResource, r.changedPeers) + if !matchedByGroup && !matchedByPeer { + return + } + + // 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. + 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 + // through their network's routers, so those routers refresh too (e.g. attaching a + // resource to a destination group, or a changed destination group/resource). + if side == sideDestination { + r.foldRoutersForResources(r.ruleDestinationResourceIDs(rule)) } - return matched } -// foldRuleSide handles a changed group on `near` (Sources or Destinations): it -// folds the `far` (opposite) groups and far resource peer, the changed group(s) -// themselves (caller-reported groups only — not seeded ones, so a changed peer's -// group does not pull in its members), and the changed peers seeded from those -// groups (the peer alone). Returns whether the side matched. -func (r *resolver) foldRuleSide(near, far []string, farResource types.Resource) bool { - if !anyInSet(near, r.changedGroupSet) { - return false +// foldPolicySideForRule folds one side of a single rule (groups + direct peer), and +// for a destination side the routers of that rule's destination resources. +func (r *resolver) foldPolicySideForRule(policy *types.Policy, rule *types.PolicyRule, side policySide) { + addAll(r.affectedGroups, ruleSideGroups(rule, side)) + res := ruleSideResource(rule, side) + if res.Type == types.ResourceTypePeer && res.ID != "" { + r.affectedPeers[res.ID] = struct{}{} } - addAll(r.groupSet, far) - if farResource.Type == types.ResourceTypePeer && farResource.ID != "" { - r.peerSet[farResource.ID] = struct{}{} + if side == sideDestination { + r.foldRoutersForResources(r.ruleDestinationResourceIDs(rule)) } - for _, gID := range near { - if _, ok := r.changedGroupIDs[gID]; ok { - r.groupSet[gID] = struct{}{} // changed group itself -> its members - } - r.foldChangedPeersInGroup(gID) // a changed peer in this group -> the peer alone - } - return true } // foldChangedPeersInGroup folds changed peers that belong to groupID directly into -// peerSet (the peer only, never its co-members). +// affectedPeers (the peer only, never its co-members). func (r *resolver) foldChangedPeersInGroup(groupID string) { - if len(r.changedPeerSet) == 0 { + if len(r.changedPeers) == 0 { return } members := r.snap.groupPeers[groupID] - for pID := range r.changedPeerSet { + for pID := range r.changedPeers { if _, ok := members[pID]; ok { - r.peerSet[pID] = struct{}{} + 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 { + if rt == nil { + continue + } + log.WithContext(r.ctx).Tracef("collectFromChangedRoutes: changed route %s -> folding groups=%v peerGroups=%v accessControlGroups=%v peer=%q", + rt.ID, rt.Groups, rt.PeerGroups, rt.AccessControlGroups, rt.Peer) + addAll(r.affectedGroups, rt.Groups, rt.PeerGroups, rt.AccessControlGroups) + if rt.Peer != "" { + r.affectedPeers[rt.Peer] = struct{}{} + } + } +} + +// collectFromChangedRouters: a changed router refreshes its OWN backing peer/groups +// (the changed entity) and the SOURCE side of every policy reaching a resource on +// its network (the router serves the whole network). Sibling routers on the network +// are independent and are NOT folded. Passing the old router state keeps a repointed +// router's previous backing affected without a post-commit read. +func (r *resolver) collectFromChangedRouters(routers []*routerTypes.NetworkRouter) { + for _, router := range routers { + if router == nil { + continue + } + log.WithContext(r.ctx).Tracef("collectFromChangedRouters: changed router %s on network %s -> folding its own peerGroups=%v peer=%q + sources reaching network resources", + router.ID, router.NetworkID, router.PeerGroups, router.Peer) + addAll(r.affectedGroups, router.PeerGroups) + if router.Peer != "" { + r.affectedPeers[router.Peer] = struct{}{} + } + if router.NetworkID != "" { + r.foldPolicySourcesForResources(r.networkResourceIDs(router.NetworkID)) + } + } +} + +// collectFromChangedResources: a changed resource refreshes the SOURCE side of the +// policies targeting EXACTLY that resource — directly, or via one of the resource's +// own groups (old∪new across the change, so a now-detached group's sources still +// refresh) — plus the routers serving its network (the resource is reached through +// them). It does not touch sibling resources on the same network. +func (r *resolver) collectFromChangedResources(resources []*resourceTypes.NetworkResource) { + for _, resource := range resources { + if resource == nil { + continue + } + log.WithContext(r.ctx).Tracef("collectFromChangedResources: changed resource %s on network %s (groups %v) -> folding sources of policies targeting it + its network's routers", + resource.ID, resource.NetworkID, resource.GroupIDs) + r.foldPolicySourcesForResource(resource.ID, resource.GroupIDs) + if resource.NetworkID != "" { + r.foldRoutersOnNetworks(map[string]struct{}{resource.NetworkID: {}}) + } + } +} + +// foldPolicySourcesForResource folds the source side of every policy whose +// destination is the given resource — referenced directly, or via any of the given +// groups (the resource's own old∪new groups, which captures a detached group). +func (r *resolver) foldPolicySourcesForResource(resourceID string, groupIDs []string) { + groups := toSet(groupIDs) + for _, policy := range r.policies() { + if !policyTargetsResourceOrGroups(policy, resourceID, groups) { + continue + } + log.WithContext(r.ctx).Tracef("foldPolicySourcesForResource: policy %s (%s) targets changed resource %s -> folding its source groups/peers", policy.ID, policy.Name, resourceID) + collectPolicySources(policy, r.affectedGroups, r.affectedPeers) + } +} + +// policyTargetsResourceOrGroups reports whether a policy's destination is the given +// resource directly, or one of the given destination groups. +func policyTargetsResourceOrGroups(policy *types.Policy, resourceID string, groups map[string]struct{}) bool { + if policy == nil { + return false + } + for _, rule := range policy.Rules { + if rule.DestinationResource.Type != types.ResourceTypePeer && rule.DestinationResource.ID == resourceID && resourceID != "" { + return true + } + if anyInSet(rule.Destinations, groups) { + return true + } + } + return false +} + +// collectFromChangedNetworks: a changed network refreshes the SOURCE side of the +// policies reaching any of its resources, plus its routers. A network has no +// groups/peers of its own. +func (r *resolver) collectFromChangedNetworks(networks []*networkTypes.Network) { + for _, network := range networks { + if network == nil || network.ID == "" { + continue + } + log.WithContext(r.ctx).Tracef("collectFromChangedNetworks: changed network %s -> folding sources reaching its resources + its routers", network.ID) + resourceIDs := r.networkResourceIDs(network.ID) + r.foldPolicySourcesForResources(resourceIDs) + r.foldRoutersOnNetworks(map[string]struct{}{network.ID: {}}) + } +} + +// foldPolicySourcesForResources folds the source groups/peers of every policy whose +// destination targets one of resourceIDs (directly or via a destination group). +func (r *resolver) foldPolicySourcesForResources(resourceIDs map[string]struct{}) { + if len(resourceIDs) == 0 { + return + } + for _, policy := range r.policies() { + if r.policyTargetsResources(policy, resourceIDs) { + log.WithContext(r.ctx).Tracef("foldPolicySourcesForResources: policy %s (%s) targets a changed resource -> folding its source groups/peers", policy.ID, policy.Name) + collectPolicySources(policy, r.affectedGroups, r.affectedPeers) } } } func (r *resolver) collectFromRoutes() { for _, rt := range r.snap.routes { - matchedByGroup := anyInSet(rt.Groups, r.changedGroupSet) || anyInSet(rt.PeerGroups, r.changedGroupSet) || anyInSet(rt.AccessControlGroups, r.changedGroupSet) - matchedByPeer := rt.Peer != "" && len(r.changedPeerSet) > 0 && isInSet(rt.Peer, r.changedPeerSet) + matchedByGroup := anyInSet(rt.Groups, r.linkGroups) || anyInSet(rt.PeerGroups, r.linkGroups) || anyInSet(rt.AccessControlGroups, r.linkGroups) + matchedByPeer := rt.Peer != "" && len(r.changedPeers) > 0 && isInSet(rt.Peer, r.changedPeers) if !matchedByGroup && !matchedByPeer { continue } log.WithContext(r.ctx).Tracef("collectFromRoutes: route %s matched (byGroup=%t byPeer=%t) -> folding groups=%v peerGroups=%v accessControlGroups=%v peer=%q", rt.ID, matchedByGroup, matchedByPeer, rt.Groups, rt.PeerGroups, rt.AccessControlGroups, rt.Peer) - addAll(r.groupSet, rt.Groups, rt.PeerGroups, rt.AccessControlGroups) + addAll(r.affectedGroups, rt.Groups, rt.PeerGroups, rt.AccessControlGroups) if rt.Peer != "" { - r.peerSet[rt.Peer] = struct{}{} + r.affectedPeers[rt.Peer] = struct{}{} } } } func (r *resolver) collectFromNameServers() { - if len(r.changedGroupSet) == 0 { + if len(r.linkGroups) == 0 { return } for _, ns := range r.snap.nsGroups { - if anyInSet(ns.Groups, r.changedGroupSet) { + if anyInSet(ns.Groups, r.linkGroups) { log.WithContext(r.ctx).Tracef("collectFromNameServers: nameserver group %s references a changed group -> folding its groups %v", ns.ID, ns.Groups) - addAll(r.groupSet, ns.Groups) + addAll(r.affectedGroups, ns.Groups) } } } func (r *resolver) collectFromDNSSettings() { - if len(r.changedGroupSet) == 0 || r.snap.dnsSettings == nil { + if len(r.linkGroups) == 0 || r.snap.dnsSettings == nil { return } for _, gID := range r.snap.dnsSettings.DisabledManagementGroups { - if _, ok := r.changedGroupSet[gID]; ok { + if _, ok := r.linkGroups[gID]; ok { log.WithContext(r.ctx).Tracef("collectFromDNSSettings: changed group %s is in DisabledManagementGroups -> folding it", gID) - r.groupSet[gID] = struct{}{} + r.affectedGroups[gID] = struct{}{} } } } +// collectFromNetworkRouters handles a changed group/peer that BACKS a router (the +// routing peer set moved): the router's own peers refresh and so do the sources of +// the policies reaching its network's resources. Sibling routers on the network are +// independent and are not folded. func (r *resolver) collectFromNetworkRouters() { for _, router := range r.networkRouters() { - matchedByGroup := anyInSet(router.PeerGroups, r.changedGroupSet) - matchedByPeer := router.Peer != "" && len(r.changedPeerSet) > 0 && isInSet(router.Peer, r.changedPeerSet) + matchedByGroup := anyInSet(router.PeerGroups, r.linkGroups) + matchedByPeer := router.Peer != "" && len(r.changedPeers) > 0 && isInSet(router.Peer, r.changedPeers) if !matchedByGroup && !matchedByPeer { continue } - log.WithContext(r.ctx).Tracef("collectFromNetworkRouters: router %s on network %s matched (byGroup=%t byPeer=%t) -> folding peerGroups=%v peer=%q and marking network for source bridge", + log.WithContext(r.ctx).Tracef("collectFromNetworkRouters: router %s on network %s matched (byGroup=%t byPeer=%t) -> folding its peerGroups=%v peer=%q + sources reaching network resources", router.ID, router.NetworkID, matchedByGroup, matchedByPeer, router.PeerGroups, router.Peer) - addAll(r.groupSet, router.PeerGroups) + addAll(r.affectedGroups, router.PeerGroups) if router.Peer != "" { - r.peerSet[router.Peer] = struct{}{} + r.affectedPeers[router.Peer] = struct{}{} + } + if router.NetworkID != "" { + r.foldPolicySourcesForResources(r.networkResourceIDs(router.NetworkID)) } - r.networkIDs[router.NetworkID] = struct{}{} } } @@ -618,34 +731,34 @@ func (r *resolver) collectFromProxyServices() { continue } matchedByPeer := serviceMatchesChangedPeers(svc, proxyPeers, expanded) - matchedByAccessGroup := anyInSet(svc.AccessGroups, r.changedGroupSet) + matchedByAccessGroup := anyInSet(svc.AccessGroups, r.linkGroups) if !matchedByPeer && !matchedByAccessGroup { continue } log.WithContext(r.ctx).Tracef("collectFromProxyServices: service %s (cluster=%s) matched (byProxyOrTargetPeer=%t byAccessGroup=%t) -> folding %d proxy peers, peer targets and access groups %v", svc.ID, svc.ProxyCluster, matchedByPeer, matchedByAccessGroup, len(proxyPeers), svc.AccessGroups) for _, pid := range proxyPeers { - r.peerSet[pid] = struct{}{} + r.affectedPeers[pid] = struct{}{} } for _, target := range svc.Targets { if target.TargetType == rpservice.TargetTypePeer && target.TargetId != "" { - r.peerSet[target.TargetId] = struct{}{} + r.affectedPeers[target.TargetId] = struct{}{} } } - addAll(r.groupSet, svc.AccessGroups) + addAll(r.affectedGroups, svc.AccessGroups) } } func (r *resolver) expandChangedPeersWithGroups() map[string]struct{} { - if len(r.changedGroupSet) == 0 { - return r.changedPeerSet + if len(r.linkGroups) == 0 { + return r.changedPeers } - ids := r.peerIDsForGroups(r.changedGroupSet) + ids := r.peerIDsForGroups(r.linkGroups) if len(ids) == 0 { - return r.changedPeerSet + return r.changedPeers } - merged := make(map[string]struct{}, len(r.changedPeerSet)+len(ids)) - for id := range r.changedPeerSet { + merged := make(map[string]struct{}, len(r.changedPeers)+len(ids)) + for id := range r.changedPeers { merged[id] = struct{}{} } for _, id := range ids { @@ -654,67 +767,36 @@ func (r *resolver) expandChangedPeersWithGroups() map[string]struct{} { return merged } -// collectResourceRouterBridge crosses between source peers and routing peers, which -// are reachable only via resource -> network -> router, not through the policy's own -// groups: source -> router (targeted resources' networks), then router -> source. -func (r *resolver) collectResourceRouterBridge() { - r.bridgeSourceToRouters() - r.bridgeRoutersToSources() -} - -func (r *resolver) bridgeSourceToRouters() { - resourceIDs := r.policyDestinationResourceIDs(r.matchedPolicies...) +// foldRoutersForResources folds the routers serving the networks of the given +// resources (a destination resource is reached through its network's routers). It is +// the resource -> network -> router hop used by foldPolicySide for a destination. +func (r *resolver) foldRoutersForResources(resourceIDs map[string]struct{}) { if len(resourceIDs) == 0 { return } - - networkIDs := r.resourceNetworkIDs(resourceIDs) - log.WithContext(r.ctx).Tracef("bridgeSourceToRouters: targeted resources %v -> networks %v (their routers become affected via the router->source pass)", - setToSlice(resourceIDs), setToSlice(networkIDs)) - for id := range networkIDs { - // Mark source-originated unless a router/resource/network change already - // marked this network directly (then it folds sources back). - if _, ok := r.networkIDs[id]; !ok { - r.sourceOriginatedNetworkIDs[id] = struct{}{} - } - r.networkIDs[id] = struct{}{} - } + r.foldRoutersOnNetworks(r.resourceNetworkIDs(resourceIDs)) } -func (r *resolver) bridgeRoutersToSources() { - if len(r.networkIDs) == 0 { - return +// ruleDestinationResourceIDs returns the destination resource IDs of a single rule: +// the direct DestinationResource plus the resources of its destination groups. +func (r *resolver) ruleDestinationResourceIDs(rule *types.PolicyRule) map[string]struct{} { + resourceIDs := make(map[string]struct{}) + if rule.DestinationResource.Type != types.ResourceTypePeer && rule.DestinationResource.ID != "" { + resourceIDs[rule.DestinationResource.ID] = struct{}{} } + r.addGroupResourceIDs(toSet(rule.Destinations), resourceIDs) + return resourceIDs +} - log.WithContext(r.ctx).Tracef("bridgeRoutersToSources: affected networks %v -> folding their routing peers and the source peers of policies targeting their resources", - setToSlice(r.networkIDs)) - - r.foldRoutersOnNetworks(r.networkIDs) - - // Sources are folded back only for networks the destination side itself changed - // (router/resource/network change). Networks reached only because a source-side - // change targets their resource must not refresh the policy's sources — the - // changed source propagates to the router side, not back to its co-sources. +// networkResourceIDs returns the IDs of all resources on the given network. +func (r *resolver) networkResourceIDs(networkID string) map[string]struct{} { resourceIDs := make(map[string]struct{}) for _, resource := range r.networkResources() { - if _, ok := r.networkIDs[resource.NetworkID]; !ok { - continue - } - if _, sourceOriginated := r.sourceOriginatedNetworkIDs[resource.NetworkID]; sourceOriginated { - continue - } - resourceIDs[resource.ID] = struct{}{} - } - if len(resourceIDs) == 0 { - return - } - - for _, policy := range r.policies() { - if r.policyTargetsResources(policy, resourceIDs) { - log.WithContext(r.ctx).Tracef("bridgeRoutersToSources: policy %s (%s) targets an affected-network resource -> folding its source groups/peers", policy.ID, policy.Name) - collectPolicySources(policy, r.groupSet, r.peerSet) + if resource.NetworkID == networkID { + resourceIDs[resource.ID] = struct{}{} } } + return resourceIDs } func (r *resolver) foldRoutersOnNetworks(networkIDs map[string]struct{}) { @@ -724,9 +806,9 @@ func (r *resolver) foldRoutersOnNetworks(networkIDs map[string]struct{}) { } log.WithContext(r.ctx).Tracef("bridgeRoutersToSources: router %s serves affected network %s -> folding peerGroups=%v peer=%q", router.ID, router.NetworkID, router.PeerGroups, router.Peer) - addAll(r.groupSet, router.PeerGroups) + addAll(r.affectedGroups, router.PeerGroups) if router.Peer != "" { - r.peerSet[router.Peer] = struct{}{} + r.affectedPeers[router.Peer] = struct{}{} } } } @@ -811,22 +893,22 @@ func (r *resolver) addGroupResourceIDs(groupIDs map[string]struct{}, resourceIDs } } -func collectPolicyDirectPeers(policy *types.Policy, peerSet map[string]struct{}) { +func collectPolicyDirectPeers(policy *types.Policy, peers map[string]struct{}) { for _, rule := range policy.Rules { if rule.SourceResource.Type == types.ResourceTypePeer && rule.SourceResource.ID != "" { - peerSet[rule.SourceResource.ID] = struct{}{} + peers[rule.SourceResource.ID] = struct{}{} } if rule.DestinationResource.Type == types.ResourceTypePeer && rule.DestinationResource.ID != "" { - peerSet[rule.DestinationResource.ID] = struct{}{} + peers[rule.DestinationResource.ID] = struct{}{} } } } -func collectPolicySources(policy *types.Policy, groupSet, peerSet map[string]struct{}) { +func collectPolicySources(policy *types.Policy, groups, peers map[string]struct{}) { for _, rule := range policy.Rules { - addAll(groupSet, rule.Sources) + addAll(groups, rule.Sources) if rule.SourceResource.Type == types.ResourceTypePeer && rule.SourceResource.ID != "" { - peerSet[rule.SourceResource.ID] = struct{}{} + peers[rule.SourceResource.ID] = struct{}{} } } } From d7740f98682a56958dea58f07a453dcb4e05dd00 Mon Sep 17 00:00:00 2001 From: pascal Date: Thu, 25 Jun 2026 00:48:33 +0200 Subject: [PATCH 10/12] respect disabled --- .../affected_peers_router_paths_test.go | 17 ++-- .../server/affected_peers_router_test.go | 17 ++-- management/server/affected_peers_test.go | 11 ++- management/server/affectedpeers/resolver.go | 83 +++++++++++++++---- .../server/affectedpeers/resolver_test.go | 30 +++---- 5 files changed, 107 insertions(+), 51 deletions(-) diff --git a/management/server/affected_peers_router_paths_test.go b/management/server/affected_peers_router_paths_test.go index 11313c387..5d83367fd 100644 --- a/management/server/affected_peers_router_paths_test.go +++ b/management/server/affected_peers_router_paths_test.go @@ -251,7 +251,9 @@ func TestAffectedPeers_E2E_UpdateResource_DestinationResourcePolicy_RefreshesSou } } -func TestAffectedPeers_E2E_UpdateResource_DisabledSiblingRouter_StillBridged(t *testing.T) { +// A disabled sibling router routes to nobody, so updating a resource on its network +// must NOT refresh its peer (the enabled router carries the bridge instead). +func TestAffectedPeers_E2E_UpdateResource_DisabledSiblingRouterNotBridged(t *testing.T) { s := setupRouterScenario(t, true) ctx := context.Background() @@ -274,13 +276,18 @@ func TestAffectedPeers_E2E_UpdateResource_DisabledSiblingRouter_StillBridged(t * require.NoError(t, err) disabledCh := s.updateManager.CreateChannel(ctx, disabledRouterPeer.ID) - t.Cleanup(func() { s.updateManager.CloseChannel(ctx, disabledRouterPeer.ID) }) + enabledCh := s.updateManager.CreateChannel(ctx, s.routerPeerID) + t.Cleanup(func() { + s.updateManager.CloseChannel(ctx, disabledRouterPeer.ID) + s.updateManager.CloseChannel(ctx, s.routerPeerID) + }) - settleAffectedUpdates(disabledCh) + settleAffectedUpdates(disabledCh, enabledCh) done := make(chan struct{}) go func() { - peerShouldReceiveUpdate(t, disabledCh) + peerShouldReceiveUpdate(t, enabledCh) + peerShouldNotReceiveUpdate(t, disabledCh) close(done) }() @@ -298,7 +305,7 @@ func TestAffectedPeers_E2E_UpdateResource_DisabledSiblingRouter_StillBridged(t * select { case <-done: case <-time.After(peerUpdateTimeout): - t.Error("timeout: resource update did not refresh the disabled sibling router's peer") + t.Error("timeout") } } diff --git a/management/server/affected_peers_router_test.go b/management/server/affected_peers_router_test.go index dc064e787..cc9df0a6a 100644 --- a/management/server/affected_peers_router_test.go +++ b/management/server/affected_peers_router_test.go @@ -682,6 +682,9 @@ func TestAffectedPeers_AllRoutingPeers_Network(t *testing.T) { assert.Contains(t, affected, secondRouterPeer.ID, "second routing peer on the same network must also be affected") } +// A disabled router in the snapshot routes to nobody, so it is skipped when the +// walk scans existing account data: a policy edit still folds the literal source +// group, but not the disabled router's peer. func TestAffectedPeers_DisabledRouter(t *testing.T) { s := setupRouterScenario(t, true) ctx := context.Background() @@ -694,11 +697,13 @@ func TestAffectedPeers_DisabledRouter(t *testing.T) { affected := s.resolvePolicyAffected(ctx, peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID)) - assert.Contains(t, affected, s.sourcePeerID, "source peer must be affected") - assert.Contains(t, affected, s.routerPeerID, - "disabled router's peer must still be affected: Enabled must not gate affected-peers") + assert.Contains(t, affected, s.sourcePeerID, "source peer (literal policy source group) must be affected") + assert.NotContains(t, affected, s.routerPeerID, + "a disabled router routes to nobody, so its peer must not be folded from snapshot data") } +// A disabled resource in the snapshot is skipped: the policy edit still folds the +// literal source group, but the resource no longer bridges to its network's router. func TestAffectedPeers_DisabledResource(t *testing.T) { s := setupRouterScenario(t, true) ctx := context.Background() @@ -710,9 +715,9 @@ func TestAffectedPeers_DisabledResource(t *testing.T) { affected := s.resolvePolicyAffected(ctx, peerToResourcePolicyByGroup(s.sourceGroupID, s.resourceGroupID)) - assert.Contains(t, affected, s.sourcePeerID, "source peer must be affected") - assert.Contains(t, affected, s.routerPeerID, - "disabled resource must still resolve the routing peer: Enabled must not gate affected-peers") + assert.Contains(t, affected, s.sourcePeerID, "source peer (literal policy source group) must be affected") + assert.NotContains(t, affected, s.routerPeerID, + "a disabled resource routes to nobody, so its network's router must not be folded from snapshot data") } func TestAffectedPeers_DisabledRule(t *testing.T) { diff --git a/management/server/affected_peers_test.go b/management/server/affected_peers_test.go index d8966e6c6..e3beff379 100644 --- a/management/server/affected_peers_test.go +++ b/management/server/affected_peers_test.go @@ -338,6 +338,7 @@ func TestCollectGroupChange_NetworkRouterLinked(t *testing.T) { AccountID: accountID, PeerGroups: []string{groupIDs[0]}, Peer: peerIDs[3], + Enabled: true, }) require.NoError(t, err) @@ -368,6 +369,7 @@ func TestCollectGroupChange_NetworkRouterPeerOnlyNoGroups(t *testing.T) { NetworkID: net1.ID, AccountID: accountID, Peer: peerIDs[4], + Enabled: true, }) require.NoError(t, err) @@ -625,6 +627,7 @@ func TestResolveAffectedPeers_NetworkRouter(t *testing.T) { AccountID: accountID, PeerGroups: []string{groupIDs[0]}, Peer: peerIDs[3], + Enabled: true, }) require.NoError(t, err) @@ -1374,6 +1377,7 @@ func TestAffectedPeers_NetworkRouterUnlinkedPeerNoUpdate(t *testing.T) { NetworkID: net1.ID, AccountID: accountID, PeerGroups: []string{"nr-grpA"}, + Enabled: true, }) require.NoError(t, err) @@ -1797,7 +1801,9 @@ func TestCollectAffectedFromProxyServices_GroupContainingTargetPeerChanged(t *te assert.Contains(t, directPeers, peerIDs[1], "target peer must be refreshed") } -func TestCollectAffectedFromProxyServices_DisabledServiceStillMatches(t *testing.T) { +// A disabled service in the snapshot proxies nothing, so it is skipped: a changed +// target peer does not pull in the service's proxy peer. +func TestCollectAffectedFromProxyServices_DisabledServiceSkipped(t *testing.T) { manager, s, accountID, peerIDs, _ := setupAffectedPeersTest(t) ctx := context.Background() @@ -1823,8 +1829,7 @@ func TestCollectAffectedFromProxyServices_DisabledServiceStillMatches(t *testing require.NoError(t, s.CreateService(ctx, svc)) _, directPeers := collectPeerChangeAffectedGroups(ctx, manager.Store, accountID, nil, []string{peerIDs[1]}) - assert.Contains(t, directPeers, peerIDs[0], "disabled service should still trigger a refresh so peers are ready when re-enabled") - assert.Contains(t, directPeers, peerIDs[1], "disabled target should still trigger a refresh") + assert.NotContains(t, directPeers, peerIDs[0], "a disabled service proxies nothing, so its proxy peer must not be folded") } func TestCollectAffectedFromProxyServices_NonPeerTargetType(t *testing.T) { diff --git a/management/server/affectedpeers/resolver.go b/management/server/affectedpeers/resolver.go index 19818ca3a..089266266 100644 --- a/management/server/affectedpeers/resolver.go +++ b/management/server/affectedpeers/resolver.go @@ -6,7 +6,12 @@ // and before a delete/removal severs the old state). // - Snapshot.Expand: in-memory walk, no store access. Run AFTER the tx commits. // -// Enabled is never consulted: toggling it is itself an observable change. +// Enabled handling differs by source. Disabled objects in the SNAPSHOT (existing +// account policies/resources/routers/routes/proxy services and their rules/targets) +// route to nobody and are skipped — they cannot affect any peer's map. Objects in +// the CHANGE itself are processed regardless of Enabled, so disabling one still +// refreshes the peers that lose access (the toggle is the observable change, and the +// update carries the old∪new state). package affectedpeers import ( @@ -344,11 +349,44 @@ type resolver struct { affectedPeers map[string]struct{} } -func (r *resolver) policies() []*types.Policy { return r.snap.policies } +// policies returns the account's ENABLED policies from the snapshot. Disabled +// policies grant no access, so the walk skips them when scanning existing account +// data. Explicitly changed policies (Change.Policies, via bothSidesPolicies) are +// processed regardless of Enabled, so disabling one still refreshes its peers. +func (r *resolver) policies() []*types.Policy { + enabled := make([]*types.Policy, 0, len(r.snap.policies)) + for _, policy := range r.snap.policies { + if policy != nil && policy.Enabled { + enabled = append(enabled, policy) + } + } + return enabled +} -func (r *resolver) networkResources() []*resourceTypes.NetworkResource { return r.snap.resources } +// networkResources / networkRouters return the account's ENABLED resources/routers +// from the snapshot. Disabled objects route to nobody, so the walk skips them when +// it scans existing account data. The explicitly changed objects in the Change are +// processed regardless of Enabled (collectFromChanged*), so disabling one still +// refreshes the peers that lose access. +func (r *resolver) networkResources() []*resourceTypes.NetworkResource { + enabled := make([]*resourceTypes.NetworkResource, 0, len(r.snap.resources)) + for _, resource := range r.snap.resources { + if resource.Enabled { + enabled = append(enabled, resource) + } + } + return enabled +} -func (r *resolver) networkRouters() []*routerTypes.NetworkRouter { return r.snap.routers } +func (r *resolver) networkRouters() []*routerTypes.NetworkRouter { + enabled := make([]*routerTypes.NetworkRouter, 0, len(r.snap.routers)) + for _, router := range r.snap.routers { + if router.Enabled { + enabled = append(enabled, router) + } + } + return enabled +} // peerIDsForGroups maps a group set to its member peer IDs via the preloaded index. func (r *resolver) peerIDsForGroups(groups map[string]struct{}) []string { @@ -466,6 +504,9 @@ func (r *resolver) appendPoliciesForPostureChecks(policies []*types.Policy, post func (r *resolver) collectFromPolicies() { for _, policy := range r.policies() { for _, rule := range policy.Rules { + if !rule.Enabled { + continue // a disabled rule grants no access + } r.foldRuleSideIfChanged(policy, rule, sideSource) r.foldRuleSideIfChanged(policy, rule, sideDestination) } @@ -612,6 +653,9 @@ func policyTargetsResourceOrGroups(policy *types.Policy, resourceID string, grou return false } for _, rule := range policy.Rules { + if !rule.Enabled { + continue + } if rule.DestinationResource.Type != types.ResourceTypePeer && rule.DestinationResource.ID == resourceID && resourceID != "" { return true } @@ -653,6 +697,9 @@ func (r *resolver) foldPolicySourcesForResources(resourceIDs map[string]struct{} func (r *resolver) collectFromRoutes() { for _, rt := range r.snap.routes { + if !rt.Enabled { + continue // disabled routes route to nobody; skip existing account data + } matchedByGroup := anyInSet(rt.Groups, r.linkGroups) || anyInSet(rt.PeerGroups, r.linkGroups) || anyInSet(rt.AccessControlGroups, r.linkGroups) matchedByPeer := rt.Peer != "" && len(r.changedPeers) > 0 && isInSet(rt.Peer, r.changedPeers) if !matchedByGroup && !matchedByPeer { @@ -723,8 +770,8 @@ func (r *resolver) collectFromProxyServices() { expanded := r.expandChangedPeersWithGroups() for _, svc := range services { - if svc == nil { - continue + if svc == nil || !svc.Enabled { + continue // a disabled service proxies nothing; skip existing account data } proxyPeers := proxyByCluster[svc.ProxyCluster] if len(proxyPeers) == 0 { @@ -741,6 +788,9 @@ func (r *resolver) collectFromProxyServices() { r.affectedPeers[pid] = struct{}{} } for _, target := range svc.Targets { + if !target.Enabled { + continue // a disabled target forwards nothing + } if target.TargetType == rpservice.TargetTypePeer && target.TargetId != "" { r.affectedPeers[target.TargetId] = struct{}{} } @@ -829,6 +879,9 @@ func (r *resolver) policyTargetsResources(policy *types.Policy, resourceIDs map[ } destGroupSet := make(map[string]struct{}) for _, rule := range policy.Rules { + if !rule.Enabled { + continue + } if rule.DestinationResource.Type != types.ResourceTypePeer && isInSet(rule.DestinationResource.ID, resourceIDs) { return true } @@ -893,19 +946,13 @@ func (r *resolver) addGroupResourceIDs(groupIDs map[string]struct{}, resourceIDs } } -func collectPolicyDirectPeers(policy *types.Policy, peers map[string]struct{}) { - for _, rule := range policy.Rules { - if rule.SourceResource.Type == types.ResourceTypePeer && rule.SourceResource.ID != "" { - peers[rule.SourceResource.ID] = struct{}{} - } - if rule.DestinationResource.Type == types.ResourceTypePeer && rule.DestinationResource.ID != "" { - peers[rule.DestinationResource.ID] = struct{}{} - } - } -} - +// collectPolicySources folds the source groups/peers of a snapshot policy's enabled +// rules (a disabled rule grants no access). func collectPolicySources(policy *types.Policy, groups, peers map[string]struct{}) { for _, rule := range policy.Rules { + if !rule.Enabled { + continue + } addAll(groups, rule.Sources) if rule.SourceResource.Type == types.ResourceTypePeer && rule.SourceResource.ID != "" { peers[rule.SourceResource.ID] = struct{}{} @@ -937,7 +984,7 @@ func serviceMatchesChangedPeers(svc *rpservice.Service, proxyPeers []string, cha } } for _, target := range svc.Targets { - if target.TargetType != rpservice.TargetTypePeer || target.TargetId == "" { + if !target.Enabled || target.TargetType != rpservice.TargetTypePeer || target.TargetId == "" { continue } if _, ok := changedPeers[target.TargetId]; ok { diff --git a/management/server/affectedpeers/resolver_test.go b/management/server/affectedpeers/resolver_test.go index 775dccb86..fe6ada347 100644 --- a/management/server/affectedpeers/resolver_test.go +++ b/management/server/affectedpeers/resolver_test.go @@ -10,8 +10,8 @@ import ( "github.com/netbirdio/netbird/management/server/types" ) -// policyGroupsAndPeers mirrors the explicit-policy extraction (RuleGroups + -// direct peers) the resolver folds in, for asserting the pure logic. +// policyGroupsAndPeers mirrors the both-sides extraction (RuleGroups + direct peers) +// the resolver folds in for a changed policy, for asserting the pure logic. func policyGroupsAndPeers(policies ...*types.Policy) (groups []string, peers []string) { peerSet := map[string]struct{}{} for _, p := range policies { @@ -19,7 +19,14 @@ func policyGroupsAndPeers(policies ...*types.Policy) (groups []string, peers []s continue } groups = append(groups, p.RuleGroups()...) - collectPolicyDirectPeers(p, peerSet) + for _, rule := range p.Rules { + if rule.SourceResource.Type == types.ResourceTypePeer && rule.SourceResource.ID != "" { + peerSet[rule.SourceResource.ID] = struct{}{} + } + if rule.DestinationResource.Type == types.ResourceTypePeer && rule.DestinationResource.ID != "" { + peerSet[rule.DestinationResource.ID] = struct{}{} + } + } } for id := range peerSet { peers = append(peers, id) @@ -87,24 +94,9 @@ func TestPolicyReferencesPostureChecks(t *testing.T) { assert.False(t, policyReferencesPostureChecks(policy, map[string]struct{}{"pc3": {}})) } -func TestCollectPolicyDirectPeers(t *testing.T) { - policy := &types.Policy{Rules: []*types.PolicyRule{{ - SourceResource: types.Resource{Type: types.ResourceTypePeer, ID: "p1"}, - DestinationResource: types.Resource{Type: types.ResourceTypePeer, ID: "p2"}, - }, { - DestinationResource: types.Resource{Type: types.ResourceTypeHost, ID: "r1"}, - }}} - - peerSet := map[string]struct{}{} - collectPolicyDirectPeers(policy, peerSet) - - assert.Contains(t, peerSet, "p1") - assert.Contains(t, peerSet, "p2") - assert.NotContains(t, peerSet, "r1") -} - func TestCollectPolicySources(t *testing.T) { policy := &types.Policy{Rules: []*types.PolicyRule{{ + Enabled: true, Sources: []string{"g1"}, SourceResource: types.Resource{Type: types.ResourceTypePeer, ID: "p1"}, Destinations: []string{"g2"}, From 42867c7a59fde64ad6771feabc71daa48c20cbe0 Mon Sep 17 00:00:00 2001 From: pascal Date: Thu, 25 Jun 2026 01:48:39 +0200 Subject: [PATCH 11/12] 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 From c5d26106f2eaf691b69878a58fcdd8eec0a2188b Mon Sep 17 00:00:00 2001 From: pascal Date: Thu, 25 Jun 2026 09:51:20 +0200 Subject: [PATCH 12/12] make test policies enabled to not fali tests --- management/server/posture_checks_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/management/server/posture_checks_test.go b/management/server/posture_checks_test.go index 14bc2c45a..abf0b3237 100644 --- a/management/server/posture_checks_test.go +++ b/management/server/posture_checks_test.go @@ -489,6 +489,7 @@ func TestArePostureCheckChangesAffectPeers(t *testing.T) { policy := &types.Policy{ AccountID: account.Id, + Enabled: true, Rules: []*types.PolicyRule{ { Enabled: true,