[management] allow local routing peer resource#5814
Open
pascal-fischer wants to merge 11 commits intomainfrom
Open
[management] allow local routing peer resource#5814pascal-fischer wants to merge 11 commits intomainfrom
pascal-fischer wants to merge 11 commits intomainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded an optional OnRoutingPeer boolean to network resources, persisted and exposed via API; resource construction and SQL scanning pass this flag through, and network-map logic generates additional local inbound firewall rules when a node is a routing peer with OnRoutingPeer enabled. Changes
Sequence Diagram(s)sequenceDiagram
participant API as API / Manager
participant Store as Store (SQL)
participant Map as NetworkMapComponents
participant FW as FirewallRuleBuilder
API->>Store: fetch network resources (includes on_routing_peer)
Store-->>API: resources (OnRoutingPeer set)
API->>Map: Calculate(network, resources, isRouter/isRoutingPeer)
Map->>FW: getNetworkResourcesRoutesToSync(resources, isRoutingPeer)
FW-->>Map: routes + localResourceFwRules (when isRoutingPeer && OnRoutingPeer)
Map-->>API: RoutesFirewallRules (includes local inbound rules)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
management/server/store/sql_store.go (1)
2262-2279:⚠️ Potential issue | 🔴 CriticalAdd schema migration for
on_routing_peercolumnThe query on line 2262 selects
on_routing_peerfrom the database, but there is no corresponding migration inmanagement/server/store/store.goto add this column to existing databases. Databases that haven't been migrated will fail at runtime whengetNetworkResources()is called.🔧 Proposed fix
--- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -1,7 +1,10 @@ func(db *gorm.DB) error { return migration.MigrateNewField[resourceTypes.NetworkResource](ctx, db, "enabled", true) }, + func(db *gorm.DB) error { + return migration.MigrateNewField[resourceTypes.NetworkResource](ctx, db, "on_routing_peer", false) + }, func(db *gorm.DB) error { return migration.MigrateNewField[routerTypes.NetworkRouter](ctx, db, "enabled", true) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/store/sql_store.go` around lines 2262 - 2279, The query in getNetworkResources reads the on_routing_peer column but no migration adds that column; add a schema migration that alters network_resources to include on_routing_peer boolean NOT NULL DEFAULT false (or NULLABLE if intended) and apply it in the migrations list used by store initialization (update the migrations slice in store.go where migrations are registered/defined) so existing DBs get the new column before getNetworkResources() is used; ensure the migration has a unique name/version consistent with the project's migration convention.management/server/types/networkmap_components.go (1)
708-720:⚠️ Potential issue | 🔴 CriticalKeep the return-level
isRoutingPeerseparate from the current resource check.Line 718 keys local peer rules off
isRoutingPeer, but that flag staystrueafter the first matching resource. A peer that routes network A but not network B will still pick up B'sOnRoutingPeerFirewallRules later in the loop, which can open the router itself for the wrong resource because these rules are not destination-scoped.🐛 Suggested fix
- var addSourcePeers bool + var ( + addSourcePeers bool + isCurrentRoutingPeer bool + ) networkRoutingPeers, exists := c.RoutersMap[resource.NetworkID] if exists { if router, ok := networkRoutingPeers[peerID]; ok { - isRoutingPeer, addSourcePeers = true, true + isRoutingPeer = true + addSourcePeers = true + isCurrentRoutingPeer = true routes = append(routes, c.getNetworkResourcesRoutes(resource, peerID, router)...) } } addedResourceRoute := false for _, policy := range c.ResourcePoliciesMap[resource.ID] { - if isRoutingPeer && resource.OnRoutingPeer { + if isCurrentRoutingPeer && resource.OnRoutingPeer { localResourceFwRule = append(localResourceFwRule, c.getLocalResourceFirewallRules(policy)...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmap_components.go` around lines 708 - 720, The loop reuses the outer isRoutingPeer flag across resources causing later resources to incorrectly get OnRoutingPeer firewall rules; change to compute a per-resource boolean (e.g., isRoutingPeerForResource) by checking c.RoutersMap[resource.NetworkID] and networkRoutingPeers[peerID] for the current resource, use that per-resource flag when deciding to append getLocalResourceFirewallRules(policy) and when calling getNetworkResourcesRoutes, but still set the outer/addSourcePeers (addSourcePeers or other summary flags) only when a match is first found so cumulative state is preserved.
🧹 Nitpick comments (1)
management/server/networks/resources/types/resource.go (1)
46-46: Avoid adjacent boolean parameters inNewNetworkResource.The
onRoutingPeer, enabled booltail is easy to swap at call sites and reduces readability. Prefer a params/options struct to make calls self-documenting and safer.♻️ Proposed refactor
+type NewNetworkResourceParams struct { + AccountID string + NetworkID string + Name string + Description string + Address string + GroupIDs []string + OnRoutingPeer bool + Enabled bool +} + -func NewNetworkResource(accountID, networkID, name, description, address string, groupIDs []string, onRoutingPeer, enabled bool) (*NetworkResource, error) { - resourceType, domain, prefix, err := GetResourceType(address) +func NewNetworkResource(p NewNetworkResourceParams) (*NetworkResource, error) { + resourceType, domain, prefix, err := GetResourceType(p.Address) if err != nil { return nil, fmt.Errorf("invalid address: %w", err) } return &NetworkResource{ ID: xid.New().String(), - AccountID: accountID, - NetworkID: networkID, - Name: name, - Description: description, + AccountID: p.AccountID, + NetworkID: p.NetworkID, + Name: p.Name, + Description: p.Description, Type: resourceType, - Address: address, + Address: p.Address, Domain: domain, Prefix: prefix, - GroupIDs: groupIDs, - Enabled: enabled, - OnRoutingPeer: onRoutingPeer, + GroupIDs: p.GroupIDs, + Enabled: p.Enabled, + OnRoutingPeer: p.OnRoutingPeer, }, nil }#!/bin/bash # Verify all current call sites and inspect bool argument ordering. # Expected: every invocation clearly passes onRoutingPeer first, enabled second. rg -nP --type=go '\bNewNetworkResource\s*\(' -C 3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/networks/resources/types/resource.go` at line 46, Replace the adjacent boolean parameters on NewNetworkResource by introducing an options struct (e.g., type NetworkResourceOptions with fields OnRoutingPeer bool and Enabled bool), change NewNetworkResource signature to accept opts *NetworkResourceOptions (or opts NetworkResourceOptions), update the function body to read opts.OnRoutingPeer and opts.Enabled, and then update every call site of NewNetworkResource to pass a named options struct (so callers use NetworkResourceOptions{OnRoutingPeer: true, Enabled: false} etc.); also update any tests and docs referring to the old parameter order and run a project-wide search for NewNetworkResource to fix all invocations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/server/types/networkmap_components.go`:
- Around line 746-799: getLocalResourceFirewallRules currently uses
getPoliciesSourcePeers(policy) to build a union of all policy sources, causing
peers from one rule to inherit other rules' ports/protocols and ignoring
per-rule SourcePostureChecks; change the function to build rules from each
Rule's own sources (e.g., call a per-rule source resolver instead of
getPoliciesSourcePeers) so you iterate rule.SourcePeers/SourcePostureChecks for
each Policy.Rules entry, consult GetPeerInfo(peerID) for each of those peerIDs,
and emit firewall rules only for that rule's peers; update the caller (as
suggested) to pass peerID into getLocalResourceFirewallRules if needed so the
function can validate posture checks per-peer.
---
Outside diff comments:
In `@management/server/store/sql_store.go`:
- Around line 2262-2279: The query in getNetworkResources reads the
on_routing_peer column but no migration adds that column; add a schema migration
that alters network_resources to include on_routing_peer boolean NOT NULL
DEFAULT false (or NULLABLE if intended) and apply it in the migrations list used
by store initialization (update the migrations slice in store.go where
migrations are registered/defined) so existing DBs get the new column before
getNetworkResources() is used; ensure the migration has a unique name/version
consistent with the project's migration convention.
In `@management/server/types/networkmap_components.go`:
- Around line 708-720: The loop reuses the outer isRoutingPeer flag across
resources causing later resources to incorrectly get OnRoutingPeer firewall
rules; change to compute a per-resource boolean (e.g., isRoutingPeerForResource)
by checking c.RoutersMap[resource.NetworkID] and networkRoutingPeers[peerID] for
the current resource, use that per-resource flag when deciding to append
getLocalResourceFirewallRules(policy) and when calling
getNetworkResourcesRoutes, but still set the outer/addSourcePeers
(addSourcePeers or other summary flags) only when a match is first found so
cumulative state is preserved.
---
Nitpick comments:
In `@management/server/networks/resources/types/resource.go`:
- Line 46: Replace the adjacent boolean parameters on NewNetworkResource by
introducing an options struct (e.g., type NetworkResourceOptions with fields
OnRoutingPeer bool and Enabled bool), change NewNetworkResource signature to
accept opts *NetworkResourceOptions (or opts NetworkResourceOptions), update the
function body to read opts.OnRoutingPeer and opts.Enabled, and then update every
call site of NewNetworkResource to pass a named options struct (so callers use
NetworkResourceOptions{OnRoutingPeer: true, Enabled: false} etc.); also update
any tests and docs referring to the old parameter order and run a project-wide
search for NewNetworkResource to fix all invocations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 56bdbac6-ebe7-4153-94d5-305478b0f208
📒 Files selected for processing (6)
management/server/networks/resources/manager.gomanagement/server/networks/resources/types/resource.gomanagement/server/store/sql_store.gomanagement/server/types/networkmap_components.goshared/management/http/api/openapi.ymlshared/management/http/api/types.gen.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/server/store/sql_store_test.go (1)
2466-2481:⚠️ Potential issue | 🟡 MinorAssert
OnRoutingPeerandEnabledto actually validate the new field round-tripAt Line 2466, the constructor now sets
onRoutingPeer=falseandenabled=true, but the test never asserts those persisted values after reload. This can miss regressions in SQL mapping for the new feature.✅ Suggested test assertions
savedNetResource, err := store.GetNetworkResourceByID(context.Background(), LockingStrengthNone, accountID, netResource.ID) require.NoError(t, err) require.Equal(t, netResource.ID, savedNetResource.ID) require.Equal(t, netResource.Name, savedNetResource.Name) require.Equal(t, netResource.NetworkID, savedNetResource.NetworkID) require.Equal(t, netResource.Type, resourceTypes.NetworkResourceType("domain")) require.Equal(t, netResource.Domain, "example.com") require.Equal(t, netResource.AccountID, savedNetResource.AccountID) require.Equal(t, netResource.Prefix, netip.Prefix{}) + require.Equal(t, netResource.OnRoutingPeer, savedNetResource.OnRoutingPeer) + require.Equal(t, netResource.Enabled, savedNetResource.Enabled)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/store/sql_store_test.go` around lines 2466 - 2481, The test creates a NetworkResource via resourceTypes.NewNetworkResource with onRoutingPeer=false and enabled=true but never asserts those fields after reload; after calling store.GetNetworkResourceByID (and alongside the existing require.Equal checks), add assertions that savedNetResource.OnRoutingPeer equals false and savedNetResource.Enabled equals true to validate the SQL round-trip for those new fields (references: NewNetworkResource, netResource, SaveNetworkResource, GetNetworkResourceByID, savedNetResource.OnRoutingPeer, savedNetResource.Enabled).
♻️ Duplicate comments (1)
management/server/types/networkmap_components.go (1)
747-752:⚠️ Potential issue | 🔴 CriticalResolve local-resource sources per rule, not per policy.
Line 747 builds a policy-wide source union once, and Lines 755-800 then emit every enabled rule for every peer in that union. If sibling rules have different sources, a peer that only matches one rule can inherit the others' ports/protocols here. Build the peer set inside the rule loop instead of reusing a policy-wide list. The call at Line 719 will also need
peerIDthreaded through.🔒 Suggested direction
-func (c *NetworkMapComponents) getLocalResourceFirewallRules(policy *Policy) []*FirewallRule { - sourcePeerIDs := c.getPoliciesSourcePeers([]*Policy{policy}) - - postureValidatedPeerIDs := make([]string, 0) - for _, pID := range c.getPostureValidPeers(maps.Keys(sourcePeerIDs), policy.SourcePostureChecks) { - postureValidatedPeerIDs = append(postureValidatedPeerIDs, pID) - } - +func (c *NetworkMapComponents) getLocalResourceFirewallRules(targetPeerID string, policy *Policy) []*FirewallRule { + distributionPeers := c.getPoliciesSourcePeers([]*Policy{policy}) rules := make([]*FirewallRule, 0) for _, rule := range policy.Rules { if !rule.Enabled { continue } @@ - for _, peerID := range postureValidatedPeerIDs { - peer := c.GetPeerInfo(peerID) - if peer == nil { - continue - } + for _, peer := range c.getRulePeers(rule, policy.SourcePostureChecks, targetPeerID, distributionPeers) { peerIP := peer.IP.String() fr := FirewallRule{Also applies to: 755-800
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmap_components.go` around lines 747 - 752, The code currently computes sourcePeerIDs once per policy (variable sourcePeerIDs from c.getPoliciesSourcePeers) and then emits every enabled rule for every peer in that union, allowing peers that match only one sibling rule to inherit others' ports/protocols; fix by moving the source resolution into the per-rule loop so each Rule computes its own sourcePeerIDs (call c.getPoliciesSourcePeers or equivalent with the rule context) and then compute postureValidatedPeerIDs from c.getPostureValidPeers(...) for that rule only; also thread the peerID parameter through the earlier helper call site (the existing getPoliciesSourcePeers/getPostureValidPeers usages) so the per-rule source resolution can filter correctly and avoid reusing the policy-wide list.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@management/server/types/networkmap_components.go`:
- Around line 718-720: The branch that appends local firewall rules is
incorrectly gated by the outer isRoutingPeer flag so once a peer routes any
network all subsequent resources with resource.OnRoutingPeer from other networks
emit rules; change the condition to check the per-resource routing flag instead
(use addSourcePeers or introduce an isResourceRoutingPeer boolean tied to the
specific resource) so only resources for which this peer actually routes will
call c.getLocalResourceFirewallRules and append to localResourceFwRule; update
the conditional that surrounds c.getLocalResourceFirewallRules(policy) to
reference that per-resource flag rather than isRoutingPeer.
---
Outside diff comments:
In `@management/server/store/sql_store_test.go`:
- Around line 2466-2481: The test creates a NetworkResource via
resourceTypes.NewNetworkResource with onRoutingPeer=false and enabled=true but
never asserts those fields after reload; after calling
store.GetNetworkResourceByID (and alongside the existing require.Equal checks),
add assertions that savedNetResource.OnRoutingPeer equals false and
savedNetResource.Enabled equals true to validate the SQL round-trip for those
new fields (references: NewNetworkResource, netResource, SaveNetworkResource,
GetNetworkResourceByID, savedNetResource.OnRoutingPeer,
savedNetResource.Enabled).
---
Duplicate comments:
In `@management/server/types/networkmap_components.go`:
- Around line 747-752: The code currently computes sourcePeerIDs once per policy
(variable sourcePeerIDs from c.getPoliciesSourcePeers) and then emits every
enabled rule for every peer in that union, allowing peers that match only one
sibling rule to inherit others' ports/protocols; fix by moving the source
resolution into the per-rule loop so each Rule computes its own sourcePeerIDs
(call c.getPoliciesSourcePeers or equivalent with the rule context) and then
compute postureValidatedPeerIDs from c.getPostureValidPeers(...) for that rule
only; also thread the peerID parameter through the earlier helper call site (the
existing getPoliciesSourcePeers/getPostureValidPeers usages) so the per-rule
source resolution can filter correctly and avoid reusing the policy-wide list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3e8bf97-c62b-4c9c-8bf8-0324eeed7ae9
📒 Files selected for processing (2)
management/server/store/sql_store_test.gomanagement/server/types/networkmap_components.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
management/server/types/networkmap_components.go (2)
718-720:⚠️ Potential issue | 🔴 CriticalUse
addSourcePeersto scope local firewall rules to the current resource.
isRoutingPeeris set once and persists across all resource iterations. After routing one network, any subsequent resource withOnRoutingPeer=truefrom a different network will incorrectly trigger firewall rule generation even though this peer is not a router for that resource's network.🔧 Proposed fix
- if isRoutingPeer && resource.OnRoutingPeer { + if addSourcePeers && resource.OnRoutingPeer { localResourceFwRule = append(localResourceFwRule, c.getLocalResourceFirewallRules(policy)...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmap_components.go` around lines 718 - 720, isRoutingPeer is being reused across resource iterations, causing rules to be generated for resources that aren't routers; instead, scope rule generation to the current resource by using addSourcePeers for that resource. Update the logic around getLocalResourceFirewallRules so you only append rules when addSourcePeers(resource) indicates this resource's peers are routing (or pass the result of addSourcePeers(resource) into getLocalResourceFirewallRules to produce scoped rules), and remove reliance on the persistent isRoutingPeer flag so each resource decision is evaluated per-resource.
746-804:⚠️ Potential issue | 🟠 MajorBuild firewall rules from each rule's own sources, not a policy-wide union.
getPoliciesSourcePeers(line 747) collects sources from all rules in the policy. The loop then emits firewall rules for every peer against every enabled rule's ports/protocols. This allows a peer from one rule to inherit the ports/protocols of another rule.Additionally, consider extracting parts of this logic (e.g., rule-to-firewall conversion) into helper functions to reduce cognitive complexity (currently 25, limit is 20).
🔧 Suggested direction
func (c *NetworkMapComponents) getLocalResourceFirewallRules(policy *Policy) []*FirewallRule { - sourcePeerIDs := c.getPoliciesSourcePeers([]*Policy{policy}) - - postureValidatedPeerIDs := make([]string, 0) - for _, pID := range c.getPostureValidPeers(maps.Keys(sourcePeerIDs), policy.SourcePostureChecks) { - postureValidatedPeerIDs = append(postureValidatedPeerIDs, pID) - } - rules := make([]*FirewallRule, 0) for _, rule := range policy.Rules { if !rule.Enabled { continue } protocol := rule.Protocol if protocol == PolicyRuleProtocolNetbirdSSH { continue } - for _, peerID := range postureValidatedPeerIDs { + // Get sources for THIS rule only + ruleSources := c.getRuleSourcePeerIDs(rule) + validPeers := c.getPostureValidPeers(ruleSources, policy.SourcePostureChecks) + + for _, peerID := range validPeers { peer := c.GetPeerInfo(peerID)You'd need a helper like
getRuleSourcePeerIDs(rule)that extracts peer IDs fromrule.Sourcesgroups andrule.SourceResource, similar to howgetRulePeersworks but returning just IDs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmap_components.go` around lines 746 - 804, The current getLocalResourceFirewallRules uses getPoliciesSourcePeers (policy-wide union) so peers from one rule get applied to other rules; instead, for each rule call a new helper getRuleSourcePeerIDs(rule) that extracts peer IDs from rule.Sources and rule.SourceResource (similar to getRulePeers but returning IDs), pass those IDs into c.getPostureValidPeers(...) to get postureValidatedPeerIDs per rule, and then convert that single rule+peer set into FirewallRule entries; also extract the rule->firewall conversion into a small helper (e.g., buildFirewallRulesForRuleAndPeer(rule, peerIP)) to lower cognitive complexity and reuse within getLocalResourceFirewallRules.
🧹 Nitpick comments (2)
management/server/types/networkmap_components.go (2)
695-744: Consider extracting sub-loops to reduce cognitive complexity.SonarCloud flags this method at complexity 35 (limit 20). The nested loops and conditionals for route generation, source peer collection, and local firewall rules contribute to this. While functional, extracting discrete concerns (e.g., a helper for iterating resources and building local rules) would improve maintainability.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmap_components.go` around lines 695 - 744, The function getNetworkResourcesRoutesToSync is too complex (nested loops/conditionals) — extract responsibilities into small helpers: create a processResource(resource *NetworkResource, peerID string) that handles per-resource logic, a handlePolicy(policy *ResourcePolicy, resource *NetworkResource, peerID string) to encapsulate the inner policy loop (including addedResourceRoute logic), a collectPeers(peers []string, policy *ResourcePolicy, addSourcePeers bool) to populate allSourcePeers and call getPostureValidPeers/ValidatePostureChecksOnPeer, and a buildLocalFirewallRules(policy *ResourcePolicy) to return localResourceFwRule entries; then have getNetworkResourcesRoutesToSync iterate NetworkResources and call these helpers, updating isRoutingPeer, routes (via getNetworkResourcesRoutes), allSourcePeers and localResourceFwRule accordingly to reduce cognitive complexity and keep original behavior.
749-752: Simplify loop per staticcheck.The loop can be replaced with a single append statement.
♻️ Proposed simplification
- postureValidatedPeerIDs := make([]string, 0) - for _, pID := range c.getPostureValidPeers(maps.Keys(sourcePeerIDs), policy.SourcePostureChecks) { - postureValidatedPeerIDs = append(postureValidatedPeerIDs, pID) - } + postureValidatedPeerIDs := c.getPostureValidPeers(maps.Keys(sourcePeerIDs), policy.SourcePostureChecks)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmap_components.go` around lines 749 - 752, The manual loop that builds postureValidatedPeerIDs from c.getPostureValidPeers can be simplified to a single append call: replace the loop that iterates over c.getPostureValidPeers(maps.Keys(sourcePeerIDs), policy.SourcePostureChecks) and appends each pID into postureValidatedPeerIDs with a single append that expands the returned slice (use append(..., returnedSlice...)) so postureValidatedPeerIDs is created from the result of c.getPostureValidPeers in one statement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@management/server/types/networkmap_components.go`:
- Around line 718-720: isRoutingPeer is being reused across resource iterations,
causing rules to be generated for resources that aren't routers; instead, scope
rule generation to the current resource by using addSourcePeers for that
resource. Update the logic around getLocalResourceFirewallRules so you only
append rules when addSourcePeers(resource) indicates this resource's peers are
routing (or pass the result of addSourcePeers(resource) into
getLocalResourceFirewallRules to produce scoped rules), and remove reliance on
the persistent isRoutingPeer flag so each resource decision is evaluated
per-resource.
- Around line 746-804: The current getLocalResourceFirewallRules uses
getPoliciesSourcePeers (policy-wide union) so peers from one rule get applied to
other rules; instead, for each rule call a new helper getRuleSourcePeerIDs(rule)
that extracts peer IDs from rule.Sources and rule.SourceResource (similar to
getRulePeers but returning IDs), pass those IDs into c.getPostureValidPeers(...)
to get postureValidatedPeerIDs per rule, and then convert that single rule+peer
set into FirewallRule entries; also extract the rule->firewall conversion into a
small helper (e.g., buildFirewallRulesForRuleAndPeer(rule, peerIP)) to lower
cognitive complexity and reuse within getLocalResourceFirewallRules.
---
Nitpick comments:
In `@management/server/types/networkmap_components.go`:
- Around line 695-744: The function getNetworkResourcesRoutesToSync is too
complex (nested loops/conditionals) — extract responsibilities into small
helpers: create a processResource(resource *NetworkResource, peerID string) that
handles per-resource logic, a handlePolicy(policy *ResourcePolicy, resource
*NetworkResource, peerID string) to encapsulate the inner policy loop (including
addedResourceRoute logic), a collectPeers(peers []string, policy
*ResourcePolicy, addSourcePeers bool) to populate allSourcePeers and call
getPostureValidPeers/ValidatePostureChecksOnPeer, and a
buildLocalFirewallRules(policy *ResourcePolicy) to return localResourceFwRule
entries; then have getNetworkResourcesRoutesToSync iterate NetworkResources and
call these helpers, updating isRoutingPeer, routes (via
getNetworkResourcesRoutes), allSourcePeers and localResourceFwRule accordingly
to reduce cognitive complexity and keep original behavior.
- Around line 749-752: The manual loop that builds postureValidatedPeerIDs from
c.getPostureValidPeers can be simplified to a single append call: replace the
loop that iterates over c.getPostureValidPeers(maps.Keys(sourcePeerIDs),
policy.SourcePostureChecks) and appends each pID into postureValidatedPeerIDs
with a single append that expands the returned slice (use append(...,
returnedSlice...)) so postureValidatedPeerIDs is created from the result of
c.getPostureValidPeers in one statement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11478b43-1e4b-4937-8227-470d1ca28118
📒 Files selected for processing (1)
management/server/types/networkmap_components.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
management/server/types/networkmap_components.go (2)
718-720:⚠️ Potential issue | 🔴 CriticalScope
OnRoutingPeerrules to the current resource.Line 718 still keys this branch off the function-level
isRoutingPeer. Once an earlier resource sets that flag, laterresource.OnRoutingPeerresources will append local rules even whenpeerIDis not a router for that network, which over-broadens inbound access.🔧 Suggested fix
- if isRoutingPeer && resource.OnRoutingPeer { + if addSourcePeers && resource.OnRoutingPeer { localResourceFwRule = append(localResourceFwRule, c.getLocalResourceFirewallRules(policy)...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmap_components.go` around lines 718 - 720, The branch uses the function-level isRoutingPeer flag which can remain true after an earlier resource sets it, causing later resources with resource.OnRoutingPeer to incorrectly append rules; change the condition so it checks routing status for the current resource only (e.g., compute a per-resource boolean like isRoutingPeerForResource by evaluating the peerID against this resource's routing/network attributes, then use if isRoutingPeerForResource && resource.OnRoutingPeer { localResourceFwRule = append(localResourceFwRule, c.getLocalResourceFirewallRules(policy)...) }), ensuring getLocalResourceFirewallRules is only called when the peer actually routes for that specific resource.
746-799:⚠️ Potential issue | 🔴 CriticalBuild local resource rules from each rule’s own sources.
Lines 747-748 compute one source-peer union for the whole policy, and Lines 761-796 then emit every enabled rule for every peer in that union. That lets a peer from one sibling rule — or from a disabled sibling, since the union is built before the
rule.Enabledcheck — inherit another rule's ports/protocols.🔧 Suggested direction
-func (c *NetworkMapComponents) getLocalResourceFirewallRules(policy *Policy) []*FirewallRule { - sourcePeerIDs := c.getPoliciesSourcePeers([]*Policy{policy}) - postureValidatedPeerIDs := c.getPostureValidPeers(maps.Keys(sourcePeerIDs), policy.SourcePostureChecks) +func (c *NetworkMapComponents) getLocalResourceFirewallRules(targetPeerID string, policy *Policy) []*FirewallRule { + distributionPeers := c.getPoliciesSourcePeers([]*Policy{policy}) rules := make([]*FirewallRule, 0) for _, rule := range policy.Rules { if !rule.Enabled { continue @@ - for _, peerID := range postureValidatedPeerIDs { - peer := c.GetPeerInfo(peerID) - if peer == nil { - continue - } + for _, peer := range c.getRulePeers(rule, policy.SourcePostureChecks, targetPeerID, distributionPeers) { peerIP := peer.IP.String()- localResourceFwRule = append(localResourceFwRule, c.getLocalResourceFirewallRules(policy)...) + localResourceFwRule = append(localResourceFwRule, c.getLocalResourceFirewallRules(peerID, policy)...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmap_components.go` around lines 746 - 799, The code currently computes sourcePeerIDs once for the whole policy (getPoliciesSourcePeers on the policy) then emits every enabled rule for that union, letting peers from other rules (or disabled siblings) inherit ports/protocols; fix by computing sourcePeerIDs and postureValidatedPeerIDs per individual rule inside getLocalResourceFirewallRules: move the source-peer and posture validation calls into the loop over policy.Rules (i.e., call c.getPoliciesSourcePeers for the single rule or add a helper to get a rule's sources), perform the rule.Enabled check before using those values, and then use the per-rule postureValidatedPeerIDs when creating FirewallRule entries so each rule only applies to its own sources.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@management/server/types/networkmap_components.go`:
- Around line 718-720: The branch uses the function-level isRoutingPeer flag
which can remain true after an earlier resource sets it, causing later resources
with resource.OnRoutingPeer to incorrectly append rules; change the condition so
it checks routing status for the current resource only (e.g., compute a
per-resource boolean like isRoutingPeerForResource by evaluating the peerID
against this resource's routing/network attributes, then use if
isRoutingPeerForResource && resource.OnRoutingPeer { localResourceFwRule =
append(localResourceFwRule, c.getLocalResourceFirewallRules(policy)...) }),
ensuring getLocalResourceFirewallRules is only called when the peer actually
routes for that specific resource.
- Around line 746-799: The code currently computes sourcePeerIDs once for the
whole policy (getPoliciesSourcePeers on the policy) then emits every enabled
rule for that union, letting peers from other rules (or disabled siblings)
inherit ports/protocols; fix by computing sourcePeerIDs and
postureValidatedPeerIDs per individual rule inside
getLocalResourceFirewallRules: move the source-peer and posture validation calls
into the loop over policy.Rules (i.e., call c.getPoliciesSourcePeers for the
single rule or add a helper to get a rule's sources), perform the rule.Enabled
check before using those values, and then use the per-rule
postureValidatedPeerIDs when creating FirewallRule entries so each rule only
applies to its own sources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5dccafdb-61bc-46ea-9e3f-734729ad3ed5
📒 Files selected for processing (1)
management/server/types/networkmap_components.go
|
crn4
approved these changes
Apr 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit