[management] set components network map by default and optimize memory usage#5575
Merged
[management] set components network map by default and optimize memory usage#5575
Conversation
Contributor
📝 WalkthroughWalkthroughController env parsing for Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Controller
participant Generator as NetworkMapGenerator
participant Groups as PeerGroupsStore
participant PeerStore as PeerInfoStore
participant Firewall as FirewallRuleBuilder
Controller->>Generator: request network map (compacted? default true)
Generator->>Groups: fetch peerGroups for peers
Groups-->>Generator: peerGroups
Generator->>PeerStore: fetch target peer info (by ID/IP)
PeerStore-->>Generator: peerInfo
Generator->>Generator: determine DNS/NS groups using peerGroups + peerIP
Generator->>Firewall: build firewall rules (use precomputed strings)
Firewall-->>Generator: firewall rule entries
Generator-->>Controller: return composed network map
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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
🧹 Nitpick comments (1)
management/internals/controllers/network_map/controller/controller.go (1)
84-94: Consider aligning default handling for both experimental feature flags.There's now an inconsistency:
EnvNewNetworkMapBuilderdefaults tofalseon parse error (line 87), whileEnvNewNetworkMapCompacteddefaults totrue(line 93). If this is intentional (i.e., compacted is production-ready while the builder is still experimental), the current state is fine. Otherwise, consider aligning them for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/internals/controllers/network_map/controller/controller.go` around lines 84 - 94, The two experimental feature flags are handled inconsistently: when strconv.ParseBool fails for EnvNewNetworkMapBuilder the code sets newNetworkMapBuilder = false, but for EnvNewNetworkMapCompacted it sets compactedNetworkMap = true; update the error handling so both flags use the same default policy (e.g., set compactedNetworkMap = false on parse error to match newNetworkMapBuilder or vice versa), and adjust the Warnf messages accordingly; preferably extract a clear default (constant or inline) and apply it for both EnvNewNetworkMapBuilder and EnvNewNetworkMapCompacted when ParseBool returns an error to ensure consistent behavior.
🤖 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/internals/controllers/network_map/controller/controller.go`:
- Around line 90-94: Add unit tests for the EnvNewNetworkMapCompacted parsing
and default behavior: write tests in controller_test.go that set/unset
NB_NETWORK_MAP_COMPACTED and assert compactedNetworkMap results for unset
(should be true), invalid value (should be true), and valid "true"/"false"
values; use os.Setenv/Unset around invoking the parsing logic (the code that
sets compactedNetworkMap in controller.go) and restore env afterwards. Also
either document NB_NETWORK_MAP_COMPACTED and its default in the management
README/deployment guide, or align behavior with EnvNewNetworkMapBuilder (which
currently defaults to false on parse error) by updating the parsing/default
logic in controller.go to match the intended default and update/add tests
accordingly.
---
Nitpick comments:
In `@management/internals/controllers/network_map/controller/controller.go`:
- Around line 84-94: The two experimental feature flags are handled
inconsistently: when strconv.ParseBool fails for EnvNewNetworkMapBuilder the
code sets newNetworkMapBuilder = false, but for EnvNewNetworkMapCompacted it
sets compactedNetworkMap = true; update the error handling so both flags use the
same default policy (e.g., set compactedNetworkMap = false on parse error to
match newNetworkMapBuilder or vice versa), and adjust the Warnf messages
accordingly; preferably extract a clear default (constant or inline) and apply
it for both EnvNewNetworkMapBuilder and EnvNewNetworkMapCompacted when ParseBool
returns an error to ensure consistent behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 16af4dce-ef55-45c5-ae28-b7e79e399143
📒 Files selected for processing (1)
management/internals/controllers/network_map/controller/controller.go
Contributor
There was a problem hiding this comment.
Pull request overview
Updates the default behavior of the network map controller when NB_NETWORK_MAP_COMPACTED cannot be parsed, effectively changing which network map generation path is used by default in that scenario.
Changes:
- Switch default
compactedNetworkMapfallback fromfalsetotruewhenNB_NETWORK_MAP_COMPACTEDis not a valid boolean.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
mlsmaycon
previously approved these changes
Mar 11, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
management/server/types/networkmap_components.go (1)
311-312: Delimiter the firewall-rule dedupe key.This key is built as a raw concatenation, so different field combinations can theoretically collapse to the same string and suppress a distinct rule. A fixed separator keeps the allocation win and makes the key unambiguous.
♻️ Proposed tweak
- ruleID := rule.ID + peerIP + dirStr + - protocolStr + actionStr + portsJoined + ruleID := strings.Join([]string{ + rule.ID, + peerIP, + dirStr, + protocolStr, + actionStr, + portsJoined, + }, "|")🤖 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 311 - 312, The dedupe key for firewall rules is built by raw concatenation (ruleID := rule.ID + peerIP + dirStr + protocolStr + actionStr + portsJoined) which can create collisions; change the construction to join each component with a fixed separator (e.g. "|") so use the same fields (rule.ID, peerIP, dirStr, protocolStr, actionStr, portsJoined) but concatenate them with a delimiter between each component to make the key unambiguous and stable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@management/server/types/networkmap_components.go`:
- Around line 311-312: The dedupe key for firewall rules is built by raw
concatenation (ruleID := rule.ID + peerIP + dirStr + protocolStr + actionStr +
portsJoined) which can create collisions; change the construction to join each
component with a fixed separator (e.g. "|") so use the same fields (rule.ID,
peerIP, dirStr, protocolStr, actionStr, portsJoined) but concatenate them with a
delimiter between each component to make the key unambiguous and stable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52e85ef5-9459-4b71-aa95-862286cc3b33
📒 Files selected for processing (1)
management/server/types/networkmap_components.go
|
mlsmaycon
approved these changes
Mar 11, 2026
mlsmaycon
changed the title
7 tasks
This was referenced Mar 26, 2026
7 tasks
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
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:
we just set a new components network map as default
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
Bug Fixes
Refactor
Performance