From d7740f98682a56958dea58f07a453dcb4e05dd00 Mon Sep 17 00:00:00 2001 From: pascal Date: Thu, 25 Jun 2026 00:48:33 +0200 Subject: [PATCH] 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"},