diff --git a/management/internals/controllers/network_map/controller/controller.go b/management/internals/controllers/network_map/controller/controller.go index b14d0d81a..8389fcdb8 100644 --- a/management/internals/controllers/network_map/controller/controller.go +++ b/management/internals/controllers/network_map/controller/controller.go @@ -270,8 +270,6 @@ func (c *Controller) UpdateAccountPeers(ctx context.Context, accountID string) e } // UpdateAffectedPeers updates only the specified peers that belong to an account. -// Should be called when a change is known to affect only a subset of peers. -// If peerIDs is empty, this is a no-op. func (c *Controller) UpdateAffectedPeers(ctx context.Context, accountID string, peerIDs []string) error { if len(peerIDs) == 0 { return nil @@ -287,7 +285,6 @@ func (c *Controller) sendUpdateForAffectedPeers(ctx context.Context, accountID s affected[id] = struct{}{} } - // Fast check: any of the affected peers actually connected? hasConnected := false for _, id := range peerIDs { if c.peersUpdateManager.HasChannel(id) { @@ -306,7 +303,6 @@ func (c *Controller) sendUpdateForAffectedPeers(ctx context.Context, accountID s globalStart := time.Now() - // Collect the subset of account peers that are both affected and connected. var peersToUpdate []*nbpeer.Peer for _, peer := range account.Peers { if _, ok := affected[peer.ID]; ok && c.peersUpdateManager.HasChannel(peer.ID) { @@ -504,8 +500,7 @@ func (c *Controller) BufferUpdateAccountPeers(ctx context.Context, accountID str return nil } -// BufferUpdateAffectedPeers accumulates peer IDs across rapid successive calls -// and flushes them in a single sendUpdateForAffectedPeers call after the buffer interval. +// BufferUpdateAffectedPeers accumulates peer IDs and flushes them after the buffer interval. func (c *Controller) BufferUpdateAffectedPeers(ctx context.Context, accountID string, peerIDs []string) error { if len(peerIDs) == 0 { return nil @@ -518,7 +513,6 @@ func (c *Controller) BufferUpdateAffectedPeers(ctx context.Context, accountID st }) b := bufUpd.(*bufferAffectedUpdate) - // Always accumulate incoming peer IDs (non-blocking). b.addPeerIDs(peerIDs) if !b.sendMu.TryLock() { diff --git a/management/server/affected_peers_test.go b/management/server/affected_peers_test.go index cf0cb033d..bf48b0e4a 100644 --- a/management/server/affected_peers_test.go +++ b/management/server/affected_peers_test.go @@ -20,15 +20,8 @@ import ( "github.com/netbirdio/netbird/route" ) -// setupAffectedPeersTest creates a manager with a clean account and 5 peers, each in its own group. -// Returns the manager, store, account ID, peer IDs, and group IDs. -// Peer layout: -// -// peer0 -> group0 -// peer1 -> group1 -// peer2 -> group2 -// peer3 -> group3 -// peer4 -> group4 +// setupAffectedPeersTest creates a manager with a clean account (default policy deleted) +// and 5 peers, each in its own group: peer0->group0, peer1->group1, ..., peer4->group4. func setupAffectedPeersTest(t *testing.T) (*DefaultAccountManager, store.Store, string, []string, []string) { t.Helper() @@ -41,7 +34,6 @@ func setupAffectedPeersTest(t *testing.T) (*DefaultAccountManager, store.Store, ctx := context.Background() accountID := account.Id - // Delete the default "All <-> All" policy so tests start with a clean slate policies, err := manager.Store.GetAccountPolicies(ctx, store.LockingStrengthNone, accountID) require.NoError(t, err) for _, p := range policies { @@ -76,16 +68,13 @@ func setupAffectedPeersTest(t *testing.T) (*DefaultAccountManager, store.Store, func affectedGroupID(i int) string { return fmt.Sprintf("affected-grp-%d", i) } func affectedGroupName(i int) string { return fmt.Sprintf("AffectedGroup%d", i) } -// ---------- collectGroupChangeAffectedGroups ---------- - func TestCollectGroupChange_NoEntities(t *testing.T) { _, s, accountID, _, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // group0 is not referenced by any entity groups, directPeers := collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[0]}) - assert.Empty(t, groups, "no entities reference group0, should return no affected groups") - assert.Empty(t, directPeers, "no entities reference group0, should return no direct peers") + assert.Empty(t, groups) + assert.Empty(t, directPeers) } func TestCollectGroupChange_EmptyInput(t *testing.T) { @@ -105,7 +94,6 @@ func TestCollectGroupChange_PolicyLinked(t *testing.T) { manager, s, accountID, _, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Create policy: group0 (src) <-> group1 (dst) _, err := manager.SavePolicy(ctx, accountID, userID, &types.Policy{ Enabled: true, Rules: []*types.PolicyRule{ @@ -120,17 +108,14 @@ func TestCollectGroupChange_PolicyLinked(t *testing.T) { }, true) require.NoError(t, err) - // Changing group0 should include both group0 and group1 (from the policy rule) groups, _ := collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[0]}) assert.Contains(t, groups, groupIDs[0]) assert.Contains(t, groups, groupIDs[1]) - // Changing group1 should also include both groups groups, _ = collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[1]}) assert.Contains(t, groups, groupIDs[0]) assert.Contains(t, groups, groupIDs[1]) - // Changing group2 (not in policy) should return nothing groups, _ = collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[2]}) assert.Empty(t, groups) } @@ -139,7 +124,6 @@ func TestCollectGroupChange_PolicyWithDirectPeerResource(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Create policy with direct peer resource _, err := manager.SavePolicy(ctx, accountID, userID, &types.Policy{ Enabled: true, Rules: []*types.PolicyRule{ @@ -157,46 +141,42 @@ func TestCollectGroupChange_PolicyWithDirectPeerResource(t *testing.T) { 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], "direct peer resource should be in directPeers") + assert.Contains(t, directPeers, peerIDs[3]) } func TestCollectGroupChange_RouteLinked(t *testing.T) { manager, s, accountID, _, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Create route: PeerGroups=[group0], Groups(distribution)=[group1] _, err := manager.CreateRoute(ctx, accountID, - netip.MustParsePrefix("10.0.0.0/24"), // network - route.IPv4Network, // type - nil, // domains - "", // peer - []string{groupIDs[0]}, // peerGroups - "test route", // description - "testnet", // netID - false, // masquerade - 9999, // metric - []string{groupIDs[1]}, // groups (distribution) - []string{groupIDs[2]}, // accessControlGroups - true, // enabled + netip.MustParsePrefix("10.0.0.0/24"), + route.IPv4Network, + nil, + "", + []string{groupIDs[0]}, + "test route", + "testnet", + false, + 9999, + []string{groupIDs[1]}, + []string{groupIDs[2]}, + true, userID, - false, // keepRoute - false, // skipAutoApply + false, + false, ) require.NoError(t, err) - // Changing group0 (peerGroups) should include group0, group1, group2 groups, _ := collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[0]}) assert.Contains(t, groups, groupIDs[0]) assert.Contains(t, groups, groupIDs[1]) assert.Contains(t, groups, groupIDs[2]) - // Changing group1 (distribution) should include all three groups, _ = collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[1]}) assert.Contains(t, groups, groupIDs[0]) assert.Contains(t, groups, groupIDs[1]) assert.Contains(t, groups, groupIDs[2]) - // Changing group3 (not in route) should return nothing groups, _ = collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[3]}) assert.Empty(t, groups) } @@ -205,18 +185,17 @@ func TestCollectGroupChange_RouteWithDirectPeer(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Create route with direct peer _, err := manager.CreateRoute(ctx, accountID, netip.MustParsePrefix("10.1.0.0/24"), route.IPv4Network, nil, - peerIDs[4], // direct peer - nil, // no peerGroups + peerIDs[4], + nil, "test route peer", "testnet2", false, 9999, - []string{groupIDs[1]}, // distribution groups + []string{groupIDs[1]}, nil, true, userID, @@ -225,7 +204,6 @@ func TestCollectGroupChange_RouteWithDirectPeer(t *testing.T) { ) require.NoError(t, err) - // Changing group1 should include group1 and direct peer4 groups, directPeers := collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[1]}) assert.Contains(t, groups, groupIDs[1]) assert.Contains(t, directPeers, peerIDs[4]) @@ -235,7 +213,6 @@ func TestCollectGroupChange_NameServerGroupLinked(t *testing.T) { manager, s, accountID, _, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Create nameserver group with group0 _, err := manager.CreateNameServerGroup(ctx, accountID, "ns1", "NS Group 1", []nbdns.NameServer{{ IP: netip.MustParseAddr("1.1.1.1"), @@ -247,11 +224,9 @@ func TestCollectGroupChange_NameServerGroupLinked(t *testing.T) { ) require.NoError(t, err) - // Changing group0 should include group0 (the NS group references it) groups, _ := collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[0]}) assert.Contains(t, groups, groupIDs[0]) - // Changing group1 (not in NS group) should not include anything from NS groups, _ = collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[1]}) assert.Empty(t, groups) } @@ -265,11 +240,9 @@ func TestCollectGroupChange_DNSSettingsLinked(t *testing.T) { }) require.NoError(t, err) - // Changing group2 should include group2 (from DNS disabled management) groups, _ := collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[2]}) assert.Contains(t, groups, groupIDs[2]) - // Changing group0 (not in DNS settings) should return nothing groups, _ = collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[0]}) assert.Empty(t, groups) } @@ -278,7 +251,6 @@ func TestCollectGroupChange_NetworkRouterLinked(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Create network and router net1 := &networkTypes.Network{ ID: "net-test-1", AccountID: accountID, @@ -296,12 +268,10 @@ func TestCollectGroupChange_NetworkRouterLinked(t *testing.T) { }) require.NoError(t, err) - // Changing group0 should include group0 and direct peer3 groups, directPeers := collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[0]}) assert.Contains(t, groups, groupIDs[0]) assert.Contains(t, directPeers, peerIDs[3]) - // Changing group1 (not in router) should return nothing groups, directPeers = collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[1]}) assert.Empty(t, groups) assert.Empty(t, directPeers) @@ -311,7 +281,6 @@ func TestCollectGroupChange_MultipleEntities(t *testing.T) { manager, s, accountID, _, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Policy: group0 <-> group1 _, err := manager.SavePolicy(ctx, accountID, userID, &types.Policy{ Enabled: true, Rules: []*types.PolicyRule{ @@ -326,7 +295,6 @@ func TestCollectGroupChange_MultipleEntities(t *testing.T) { }, true) require.NoError(t, err) - // Route: PeerGroups=[group2], distribution=[group3] _, err = manager.CreateRoute(ctx, accountID, netip.MustParsePrefix("10.2.0.0/24"), route.IPv4Network, @@ -346,25 +314,21 @@ func TestCollectGroupChange_MultipleEntities(t *testing.T) { ) require.NoError(t, err) - // Changing group0 should only pick up policy groups (group0, group1), not route groups groups, directPeers := collectGroupChangeAffectedGroups(ctx, s, accountID, []string{groupIDs[0]}) assert.Contains(t, groups, groupIDs[0]) assert.Contains(t, groups, groupIDs[1]) - assert.NotContains(t, groups, groupIDs[2], "route groups should not be included for group0 change") - assert.NotContains(t, groups, groupIDs[3], "route groups should not be included for group0 change") + assert.NotContains(t, groups, groupIDs[2]) + assert.NotContains(t, groups, groupIDs[3]) assert.Empty(t, directPeers) - // Changing group3 should only pick up route groups (group2, group3) 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], "policy groups should not be included for group3 change") - assert.NotContains(t, groups, groupIDs[1], "policy groups should not be included for group3 change") + assert.NotContains(t, groups, groupIDs[0]) + assert.NotContains(t, groups, groupIDs[1]) assert.Empty(t, directPeers) } -// ---------- collectPolicyAffectedGroupsAndPeers ---------- - func TestCollectPolicyAffectedGroups_Basic(t *testing.T) { policy := &types.Policy{ Rules: []*types.PolicyRule{ @@ -412,8 +376,6 @@ func TestCollectPolicyAffectedGroups_MultipleRules(t *testing.T) { assert.ElementsMatch(t, []string{"g1", "g2", "g3", "g4"}, groups) } -// ---------- collectRouteAffectedGroupsAndPeers ---------- - func TestCollectRouteAffectedGroups_Basic(t *testing.T) { r := &route.Route{ Groups: []string{"g1"}, @@ -441,13 +403,10 @@ func TestCollectRouteAffectedGroups_NilRoute(t *testing.T) { assert.Nil(t, directPeers) } -// ---------- resolvePeerIDs ---------- - func TestResolvePeerIDs_GroupsOnly(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // group0 has peer0, group1 has peer1 result := manager.resolvePeerIDs(ctx, s, accountID, []string{groupIDs[0], groupIDs[1]}, nil) assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1]}, result) } @@ -456,7 +415,6 @@ func TestResolvePeerIDs_WithDirectPeers(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // group0 has peer0, plus direct peer2 result := manager.resolvePeerIDs(ctx, s, accountID, []string{groupIDs[0]}, []string{peerIDs[2]}) assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[2]}, result) } @@ -465,7 +423,6 @@ func TestResolvePeerIDs_Deduplication(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // peer0 is in group0 and also passed as direct peer -> should appear once result := manager.resolvePeerIDs(ctx, s, accountID, []string{groupIDs[0]}, []string{peerIDs[0]}) assert.Len(t, result, 1) assert.Equal(t, peerIDs[0], result[0]) @@ -479,22 +436,18 @@ func TestResolvePeerIDs_EmptyInputs(t *testing.T) { assert.Empty(t, result) } -// ---------- resolveAffectedPeersForPeerChanges (end-to-end) ---------- - func TestResolveAffectedPeers_NoPoliciesOrRoutes(t *testing.T) { manager, s, accountID, peerIDs, _ := setupAffectedPeersTest(t) ctx := context.Background() - // No entity references any group, so changing peer0 should yield 0 affected peers result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) - assert.Empty(t, result, "no entities reference any group, should return no affected peers") + assert.Empty(t, result) } func TestResolveAffectedPeers_PolicyBetweenTwoGroups(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Policy: group0 (src) <-> group1 (dst) _, err := manager.SavePolicy(ctx, accountID, userID, &types.Policy{ Enabled: true, Rules: []*types.PolicyRule{ @@ -509,15 +462,12 @@ func TestResolveAffectedPeers_PolicyBetweenTwoGroups(t *testing.T) { }, true) require.NoError(t, err) - // Changing peer0 (in group0) should affect peer0 + peer1 result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1]}, result) - // Changing peer1 (in group1) should also affect peer0 + peer1 result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[1]}) assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1]}, result) - // Changing peer2 (in group2, not in policy) should return empty result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[2]}) assert.Empty(t, result) } @@ -526,7 +476,6 @@ func TestResolveAffectedPeers_PolicyThreeGroups(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Policy with multiple sources/destinations: group0,group1 -> group2 _, err := manager.SavePolicy(ctx, accountID, userID, &types.Policy{ Enabled: true, Rules: []*types.PolicyRule{ @@ -540,7 +489,6 @@ func TestResolveAffectedPeers_PolicyThreeGroups(t *testing.T) { }, true) require.NoError(t, err) - // Changing peer0 (in group0) should affect peer0 + peer1 + peer2 result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1], peerIDs[2]}, result) } @@ -549,7 +497,6 @@ func TestResolveAffectedPeers_RoutePeerGroups(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Route: peerGroups=[group0], distribution=[group1] _, err := manager.CreateRoute(ctx, accountID, netip.MustParsePrefix("10.3.0.0/24"), route.IPv4Network, @@ -569,15 +516,12 @@ func TestResolveAffectedPeers_RoutePeerGroups(t *testing.T) { ) require.NoError(t, err) - // Changing peer0 (in group0/peerGroups) should affect peer0 + peer1 result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1]}, result) - // Changing peer1 (in group1/distribution) should also affect peer0 + peer1 result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[1]}) assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1]}, result) - // Changing peer2 (unrelated) should return empty result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[2]}) assert.Empty(t, result) } @@ -586,7 +530,6 @@ func TestResolveAffectedPeers_RouteWithDirectPeer(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Route with direct peer: peer=peer4, distribution=[group1] _, err := manager.CreateRoute(ctx, accountID, netip.MustParsePrefix("10.4.0.0/24"), route.IPv4Network, @@ -606,7 +549,6 @@ func TestResolveAffectedPeers_RouteWithDirectPeer(t *testing.T) { ) require.NoError(t, err) - // Changing peer1 (in distribution group1) should affect peer1 + direct peer4 result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[1]}) assert.ElementsMatch(t, []string{peerIDs[1], peerIDs[4]}, result) } @@ -615,7 +557,6 @@ func TestResolveAffectedPeers_NetworkRouter(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Create network + router with peerGroups=[group0], direct peer=peer3 net1 := &networkTypes.Network{ ID: "net-test-2", AccountID: accountID, @@ -633,7 +574,6 @@ func TestResolveAffectedPeers_NetworkRouter(t *testing.T) { }) require.NoError(t, err) - // Changing peer0 (in group0) should affect peer0 + peer3 (direct) result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[3]}, result) } @@ -642,7 +582,6 @@ func TestResolveAffectedPeers_NameServerGroup(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // NS group with group0 _, err := manager.CreateNameServerGroup(ctx, accountID, "ns-test", "NS Test", []nbdns.NameServer{{ IP: netip.MustParseAddr("8.8.8.8"), @@ -654,7 +593,6 @@ func TestResolveAffectedPeers_NameServerGroup(t *testing.T) { ) require.NoError(t, err) - // Changing peer0 (in group0) should affect peer0 result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) assert.Contains(t, result, peerIDs[0]) } @@ -663,13 +601,11 @@ func TestResolveAffectedPeers_DNSSettings(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // DNS disabled management on group0 err := manager.SaveDNSSettings(ctx, accountID, userID, &types.DNSSettings{ DisabledManagementGroups: []string{groupIDs[0]}, }) require.NoError(t, err) - // Changing peer0 (in group0) should affect peer0 result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) assert.Contains(t, result, peerIDs[0]) } @@ -678,11 +614,9 @@ func TestResolveAffectedPeers_PeerInMultipleGroups(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Add peer0 to group1 as well err := manager.GroupAddPeer(ctx, accountID, groupIDs[1], peerIDs[0]) require.NoError(t, err) - // Policy: group0 -> group2 _, err = manager.SavePolicy(ctx, accountID, userID, &types.Policy{ Enabled: true, Rules: []*types.PolicyRule{ @@ -696,7 +630,6 @@ func TestResolveAffectedPeers_PeerInMultipleGroups(t *testing.T) { }, true) require.NoError(t, err) - // Another policy: group1 -> group3 _, err = manager.SavePolicy(ctx, accountID, userID, &types.Policy{ Enabled: true, Rules: []*types.PolicyRule{ @@ -710,10 +643,7 @@ func TestResolveAffectedPeers_PeerInMultipleGroups(t *testing.T) { }, true) require.NoError(t, err) - // Changing peer0 (in group0 AND group1) should affect: - // From policy1: group0+group2 -> peer0, peer2 - // From policy2: group1+group3 -> peer0, peer1, peer3 - // Total: peer0, peer1, peer2, peer3 + // 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) } @@ -722,7 +652,6 @@ func TestResolveAffectedPeers_MultipleChangedPeers(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Policy: group0 <-> group1 _, err := manager.SavePolicy(ctx, accountID, userID, &types.Policy{ Enabled: true, Rules: []*types.PolicyRule{ @@ -737,7 +666,6 @@ func TestResolveAffectedPeers_MultipleChangedPeers(t *testing.T) { }, true) require.NoError(t, err) - // Another policy: group2 <-> group3 _, err = manager.SavePolicy(ctx, accountID, userID, &types.Policy{ Enabled: true, Rules: []*types.PolicyRule{ @@ -752,10 +680,7 @@ func TestResolveAffectedPeers_MultipleChangedPeers(t *testing.T) { }, true) require.NoError(t, err) - // Changing peer0 AND peer2 at once result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0], peerIDs[2]}) - // peer0 -> policy1 -> peer0, peer1 - // peer2 -> policy2 -> peer2, peer3 assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1], peerIDs[2], peerIDs[3]}, result) } @@ -763,7 +688,6 @@ func TestResolveAffectedPeers_SharedGroupAcrossPolicyAndRoute(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Policy: group0 <-> group1 _, err := manager.SavePolicy(ctx, accountID, userID, &types.Policy{ Enabled: true, Rules: []*types.PolicyRule{ @@ -778,7 +702,6 @@ func TestResolveAffectedPeers_SharedGroupAcrossPolicyAndRoute(t *testing.T) { }, true) require.NoError(t, err) - // Route: distribution=[group0], peerGroups=[group2] _, err = manager.CreateRoute(ctx, accountID, netip.MustParsePrefix("10.5.0.0/24"), route.IPv4Network, @@ -798,10 +721,7 @@ func TestResolveAffectedPeers_SharedGroupAcrossPolicyAndRoute(t *testing.T) { ) require.NoError(t, err) - // Changing peer0 (in group0) should affect: - // From policy: group0+group1 -> peer0, peer1 - // From route: group0+group2 -> peer0, peer2 - // Total: peer0, peer1, peer2 + // group0 is shared: policy gives peer0+peer1, route gives peer0+peer2 result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1], peerIDs[2]}, result) } @@ -817,14 +737,11 @@ func TestResolveAffectedPeers_EmptyChangedPeers(t *testing.T) { assert.Empty(t, result) } -// ---------- Integration: peer changes with full update flow ---------- - func TestAffectedPeers_GroupUpdateOnlyAffectsLinkedPeers(t *testing.T) { manager, updateManager, account, peer1, peer2, peer3 := setupNetworkMapTest(t) ctx := context.Background() accountID := account.Id - // Delete the default "All <-> All" policy so only our explicit policy matters policies, err := manager.Store.GetAccountPolicies(ctx, store.LockingStrengthNone, accountID) require.NoError(t, err) for _, p := range policies { @@ -832,7 +749,6 @@ func TestAffectedPeers_GroupUpdateOnlyAffectsLinkedPeers(t *testing.T) { require.NoError(t, err) } - // Create groups for _, g := range []*types.Group{ {ID: "ap-grpA", Name: "AP-A", Peers: []string{peer1.ID}}, {ID: "ap-grpB", Name: "AP-B", Peers: []string{peer2.ID}}, @@ -842,7 +758,6 @@ func TestAffectedPeers_GroupUpdateOnlyAffectsLinkedPeers(t *testing.T) { require.NoError(t, err) } - // Policy: grpA <-> grpB (peer1 <-> peer2). peer3 is NOT in this policy. _, err = manager.SavePolicy(ctx, accountID, userID, &types.Policy{ Enabled: true, Rules: []*types.PolicyRule{ @@ -857,7 +772,6 @@ func TestAffectedPeers_GroupUpdateOnlyAffectsLinkedPeers(t *testing.T) { }, true) require.NoError(t, err) - // Open update channels for all peers updMsg1 := updateManager.CreateChannel(ctx, peer1.ID) updMsg2 := updateManager.CreateChannel(ctx, peer2.ID) updMsg3 := updateManager.CreateChannel(ctx, peer3.ID) @@ -867,25 +781,23 @@ func TestAffectedPeers_GroupUpdateOnlyAffectsLinkedPeers(t *testing.T) { updateManager.CloseChannel(ctx, peer3.ID) }) - // Verify resolution: changing peer1 should only affect peer1 and peer2 result := manager.resolveAffectedPeersForPeerChanges(ctx, manager.Store, accountID, []string{peer1.ID}) assert.ElementsMatch(t, []string{peer1.ID, peer2.ID}, result) - // Updating grpA to include peer3 should update all 3 peers because after the update - // grpA={peer1,peer3} which is in the policy, plus grpB={peer2} + // Adding peer3 to grpA makes it part of the policy, so all 3 peers get updated t.Run("group change updates all peers in policy groups", func(t *testing.T) { done := make(chan struct{}) go func() { peerShouldReceiveUpdate(t, updMsg1) peerShouldReceiveUpdate(t, updMsg2) - peerShouldReceiveUpdate(t, updMsg3) // peer3 is now in grpA which is in the policy + peerShouldReceiveUpdate(t, updMsg3) close(done) }() err := manager.UpdateGroup(ctx, accountID, userID, &types.Group{ ID: "ap-grpA", Name: "AP-A", - Peers: []string{peer1.ID, peer3.ID}, // add peer3 to group + Peers: []string{peer1.ID, peer3.ID}, }) assert.NoError(t, err) @@ -905,13 +817,10 @@ func TestAffectedPeers_UnlinkedGroupChange_NoUpdates(t *testing.T) { manager, s, accountID, peerIDs, _ := setupAffectedPeersTest(t) ctx := context.Background() - // No entities reference any group result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) - assert.Empty(t, result, "unlinked peer change should produce no affected peers") + assert.Empty(t, result) } -// ---------- collectPostureCheckAffectedGroupsAndPeers ---------- - func TestCollectPostureCheckAffected_NoMatch(t *testing.T) { _, s, accountID, _, _ := setupAffectedPeersTest(t) ctx := context.Background() @@ -921,13 +830,10 @@ func TestCollectPostureCheckAffected_NoMatch(t *testing.T) { assert.Empty(t, directPeers) } -// ---------- Isolation: unrelated entities don't bleed ---------- - func TestAffectedPeers_IsolatedPolicies(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Policy A: group0 <-> group1 _, err := manager.SavePolicy(ctx, accountID, userID, &types.Policy{ Enabled: true, Rules: []*types.PolicyRule{ @@ -942,7 +848,6 @@ func TestAffectedPeers_IsolatedPolicies(t *testing.T) { }, true) require.NoError(t, err) - // Policy B: group2 <-> group3 _, err = manager.SavePolicy(ctx, accountID, userID, &types.Policy{ Enabled: true, Rules: []*types.PolicyRule{ @@ -957,19 +862,16 @@ func TestAffectedPeers_IsolatedPolicies(t *testing.T) { }, true) require.NoError(t, err) - // Changing peer0 should ONLY affect peer0, peer1 (Policy A), NOT peer2, peer3 (Policy B) result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1]}, result) assert.NotContains(t, result, peerIDs[2]) assert.NotContains(t, result, peerIDs[3]) - // Changing peer2 should ONLY affect peer2, peer3 (Policy B) result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[2]}) assert.ElementsMatch(t, []string{peerIDs[2], peerIDs[3]}, result) assert.NotContains(t, result, peerIDs[0]) assert.NotContains(t, result, peerIDs[1]) - // Changing peer4 (not in any policy) should return empty result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[4]}) assert.Empty(t, result) } @@ -978,7 +880,6 @@ func TestAffectedPeers_IsolatedRouteAndPolicy(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Policy: group0 <-> group1 _, err := manager.SavePolicy(ctx, accountID, userID, &types.Policy{ Enabled: true, Rules: []*types.PolicyRule{ @@ -993,7 +894,6 @@ func TestAffectedPeers_IsolatedRouteAndPolicy(t *testing.T) { }, true) require.NoError(t, err) - // Route: peerGroups=[group2], distribution=[group3] _, err = manager.CreateRoute(ctx, accountID, netip.MustParsePrefix("10.6.0.0/24"), route.IPv4Network, @@ -1013,32 +913,26 @@ func TestAffectedPeers_IsolatedRouteAndPolicy(t *testing.T) { ) require.NoError(t, err) - // Changing peer0 (policy only) -> peer0, peer1 result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) assert.ElementsMatch(t, []string{peerIDs[0], peerIDs[1]}, result) assert.NotContains(t, result, peerIDs[2]) assert.NotContains(t, result, peerIDs[3]) - // Changing peer2 (route only) -> peer2, peer3 result = manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[2]}) assert.ElementsMatch(t, []string{peerIDs[2], peerIDs[3]}, result) assert.NotContains(t, result, peerIDs[0]) assert.NotContains(t, result, peerIDs[1]) } -// ---------- Helper: verify no duplicates in resolution ---------- - func TestResolveAffectedPeers_NoDuplicates(t *testing.T) { manager, s, accountID, peerIDs, groupIDs := setupAffectedPeersTest(t) ctx := context.Background() - // Add peer0 to multiple groups err := manager.GroupAddPeer(ctx, accountID, groupIDs[1], peerIDs[0]) require.NoError(t, err) err = manager.GroupAddPeer(ctx, accountID, groupIDs[2], peerIDs[0]) require.NoError(t, err) - // Policy that references all three groups _, err = manager.SavePolicy(ctx, accountID, userID, &types.Policy{ Enabled: true, Rules: []*types.PolicyRule{ @@ -1053,18 +947,15 @@ func TestResolveAffectedPeers_NoDuplicates(t *testing.T) { require.NoError(t, err) result := manager.resolveAffectedPeersForPeerChanges(ctx, s, accountID, []string{peerIDs[0]}) - // peer0 is in group0, group1, group2 - should only appear once count := 0 for _, id := range result { if id == peerIDs[0] { count++ } } - assert.Equal(t, 1, count, "peer0 should appear exactly once in results") + assert.Equal(t, 1, count, "peer0 should appear exactly once") } -// ---------- policyReferencesGroups ---------- - func TestPolicyReferencesGroups(t *testing.T) { policy := &types.Policy{ Rules: []*types.PolicyRule{ @@ -1142,8 +1033,6 @@ func TestRouterReferencesGroups(t *testing.T) { } } -// ---------- helpers ---------- - func addPeerToAccount(t *testing.T, manager *DefaultAccountManager, accountID, setupKeyKey string) *nbpeer.Peer { t.Helper() diff --git a/management/server/group.go b/management/server/group.go index cc19cf1a4..cf22e5221 100644 --- a/management/server/group.go +++ b/management/server/group.go @@ -821,10 +821,8 @@ func areGroupChangesAffectPeers(ctx context.Context, transaction store.Store, ac return false, nil } -// collectGroupChangeAffectedGroups walks all entities that reference the changed groups -// and collects the full set of affected group IDs and direct peer IDs. -// This ensures that when a group changes, we update not just the peers in that group -// but also peers in other groups that share policies, routes, DNS, or nameserver configs. +// collectGroupChangeAffectedGroups walks policies, routes, nameservers, DNS settings, +// and network routers to collect all group IDs and direct peer IDs affected by the changed groups. func collectGroupChangeAffectedGroups(ctx context.Context, transaction store.Store, accountID string, changedGroupIDs []string) (allGroupIDs []string, directPeerIDs []string) { if len(changedGroupIDs) == 0 { return nil, nil @@ -841,7 +839,6 @@ func collectGroupChangeAffectedGroups(ctx context.Context, transaction store.Sto peerSet := make(map[string]struct{}) - // Policies: collect all rule groups + direct peer resources from policies that reference any changed group policies, err := transaction.GetAccountPolicies(ctx, store.LockingStrengthNone, accountID) if err != nil { log.WithContext(ctx).Errorf("failed to get policies for group change resolution: %v", err) @@ -867,7 +864,6 @@ func collectGroupChangeAffectedGroups(ctx context.Context, transaction store.Sto } } - // Routes: collect all groups + direct peer from routes that reference any changed group routes, err := transaction.GetAccountRoutes(ctx, store.LockingStrengthNone, accountID) if err != nil { log.WithContext(ctx).Errorf("failed to get routes for group change resolution: %v", err) @@ -893,7 +889,6 @@ func collectGroupChangeAffectedGroups(ctx context.Context, transaction store.Sto } } - // Nameserver groups: collect groups from NS groups that reference any changed group nsGroups, err := transaction.GetAccountNameServerGroups(ctx, store.LockingStrengthNone, accountID) if err != nil { log.WithContext(ctx).Errorf("failed to get nameserver groups for group change resolution: %v", err) @@ -911,7 +906,6 @@ func collectGroupChangeAffectedGroups(ctx context.Context, transaction store.Sto } } - // DNS settings: if any changed group is in DisabledManagementGroups, include those groups dnsSettings, err := transaction.GetAccountDNSSettings(ctx, store.LockingStrengthNone, accountID) if err != nil { log.WithContext(ctx).Errorf("failed to get DNS settings for group change resolution: %v", err) @@ -924,7 +918,6 @@ func collectGroupChangeAffectedGroups(ctx context.Context, transaction store.Sto } } - // Network routers: collect peer groups + direct peer from routers that reference any changed group routers, err := transaction.GetNetworkRoutersByAccountID(ctx, store.LockingStrengthNone, accountID) if err != nil { log.WithContext(ctx).Errorf("failed to get network routers for group change resolution: %v", err) diff --git a/management/server/peer.go b/management/server/peer.go index ee3a6369f..0d7689619 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -1308,14 +1308,12 @@ func (am *DefaultAccountManager) UpdateAccountPeers(ctx context.Context, account } // UpdateAffectedPeers updates only the specified peers that belong to an account. -// Should be called when a change is known to affect only a subset of peers. func (am *DefaultAccountManager) UpdateAffectedPeers(ctx context.Context, accountID string, peerIDs []string) { log.WithContext(ctx).Tracef("UpdateAffectedPeers: %d peers for account %s", len(peerIDs), accountID) _ = am.networkMapController.UpdateAffectedPeers(ctx, accountID, peerIDs) } -// resolvePeerIDs resolves a set of group IDs and direct peer IDs into a -// deduplicated list of peer IDs suitable for UpdateAffectedPeers. +// resolvePeerIDs resolves group IDs and direct peer IDs into a deduplicated peer ID list. func (am *DefaultAccountManager) resolvePeerIDs(ctx context.Context, s store.Store, accountID string, groupIDs []string, directPeerIDs []string) []string { peerIDs, err := s.GetPeerIDsByGroups(ctx, accountID, groupIDs) if err != nil { @@ -1343,15 +1341,12 @@ func (am *DefaultAccountManager) resolvePeerIDs(ctx context.Context, s store.Sto return peerIDs } -// BufferUpdateAffectedPeers accumulates peer IDs across rapid successive calls -// and flushes them in a single update after the buffer interval. +// BufferUpdateAffectedPeers accumulates peer IDs and flushes them after the buffer interval. func (am *DefaultAccountManager) BufferUpdateAffectedPeers(ctx context.Context, accountID string, peerIDs []string) { _ = am.networkMapController.BufferUpdateAffectedPeers(ctx, accountID, peerIDs) } -// resolveAffectedPeersForPeerChanges resolves changed peer IDs into the full set of -// affected peers: finds groups containing the changed peers, walks all entity linkages, -// and resolves back to peer IDs. +// resolveAffectedPeersForPeerChanges resolves changed peer IDs into the full set of affected peer IDs. func (am *DefaultAccountManager) resolveAffectedPeersForPeerChanges(ctx context.Context, s store.Store, accountID string, changedPeerIDs []string) []string { groupIDs, err := s.GetGroupIDsByPeerIDs(ctx, accountID, changedPeerIDs) if err != nil { diff --git a/management/server/policy.go b/management/server/policy.go index 9ba4f98dd..0404a9208 100644 --- a/management/server/policy.go +++ b/management/server/policy.go @@ -150,8 +150,7 @@ func (am *DefaultAccountManager) ListPolicies(ctx context.Context, accountID, us return am.Store.GetAccountPolicies(ctx, store.LockingStrengthNone, accountID) } -// collectPolicyAffectedGroupsAndPeers returns the group IDs and direct peer IDs -// referenced by the given policies' rules. +// collectPolicyAffectedGroupsAndPeers returns group IDs and direct peer IDs from the given policies. func collectPolicyAffectedGroupsAndPeers(policies ...*types.Policy) (groupIDs []string, directPeerIDs []string) { for _, policy := range policies { if policy == nil { diff --git a/management/server/posture_checks.go b/management/server/posture_checks.go index bbf4ed198..cc6abc943 100644 --- a/management/server/posture_checks.go +++ b/management/server/posture_checks.go @@ -130,8 +130,7 @@ func (am *DefaultAccountManager) ListPostureChecks(ctx context.Context, accountI return am.Store.GetAccountPostureChecks(ctx, store.LockingStrengthNone, accountID) } -// collectPostureCheckAffectedGroupsAndPeers finds all policies referencing the given posture check -// and collects their affected group IDs and direct peer IDs. +// collectPostureCheckAffectedGroupsAndPeers returns group IDs and peer IDs from policies referencing the posture check. func collectPostureCheckAffectedGroupsAndPeers(ctx context.Context, transaction store.Store, accountID, postureCheckID string) (groupIDs []string, directPeerIDs []string) { policies, err := transaction.GetAccountPolicies(ctx, store.LockingStrengthNone, accountID) if err != nil {