Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR removes legacy network map generation paths and related comparison tests, standardizing network map calculation on the “components” approach while keeping the experimental cached builder path intact.
Changes:
- Replaced remaining call sites of the legacy
Account.GetPeerNetworkMapwithAccount.GetPeerNetworkMapFromComponents. - Removed legacy network map calculation methods and associated tests/benchmarks that compared legacy vs components.
- Dropped the unused compacted-network-map env flag path and constant.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| management/server/types/networkmap_golden_test.go | Switches golden tests/benchmarks from legacy network map generation to components-based generation. |
| management/server/types/networkmap_components.go | Removes unused env constant related to compacted network maps. |
| management/server/types/networkmap_comparison_test.go | Deletes legacy-vs-components comparison tests/benchmarks tied to removed legacy code. |
| management/server/types/account_test.go | Removes tests that depended on legacy network map/resource-route helper APIs that were deleted. |
| management/server/types/account.go | Removes legacy network map generation and related helper methods no longer referenced in-repo. |
| management/server/route_test.go | Removes tests that depended on deleted legacy route firewall rule helper APIs. |
| management/server/http/handlers/peers/peers_handler.go | Updates accessible-peers handler to use components-based network map generation. |
| management/server/account_test.go | Updates network map test to use components-based network map generation; removes routes-to-sync test tied to deleted API. |
| management/internals/controllers/network_map/controller/controller.go | Removes compacted flag branching and defaults non-experimental path to components-based network map generation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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:
📝 WalkthroughWalkthroughThis PR removes the legacy NetworkMapBuilder, holder/cache and experimental/compacted flags, consolidating all network-map generation to the components-based path; it deletes related types, helpers, and many legacy tests/benchmarks and adds a comprehensive components-focused test suite. Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant Client
end
rect rgba(200,255,200,0.5)
participant Controller
end
rect rgba(255,200,200,0.5)
participant RequestBuffer
participant Account
participant Components
end
Client->>Controller: request/update peers
Controller->>RequestBuffer: GetAccountWithBackpressure(accountID)
RequestBuffer-->>Controller: Account
Controller->>Account: GetPeerNetworkMapFromComponents(peerID, params...)
Account->>Components: build/derive network map from components
Components-->>Account: NetworkMap
Account-->>Controller: NetworkMap
Controller-->>Client: propagate update
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ 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/server/types/networkmap_golden_test.go (1)
115-123: Precomputeaccount.GetActiveGroupUsers()outside the benchmarked loop.Rebuilding the group-user map on every inner-loop call inflates the “components builder” timing with setup work that the “new builder” branch doesn’t pay, so this benchmark stops being apples-to-apples. The same cleanup applies to the later components-builder benchmarks in this file.
♻️ Suggested cleanup
resourcePolicies := account.GetResourcePoliciesMap() routers := account.GetResourceRoutersMap() + groupIDToUserIDs := account.GetActiveGroupUsers() b.ResetTimer() b.Run("components builder", func(b *testing.B) { for range b.N { for _, peerID := range peerIDs { - _ = account.GetPeerNetworkMapFromComponents(ctx, peerID, dns.CustomZone{}, []*zones.Zone{}, validatedPeersMap, resourcePolicies, routers, nil, account.GetActiveGroupUsers()) + _ = account.GetPeerNetworkMapFromComponents(ctx, peerID, dns.CustomZone{}, []*zones.Zone{}, validatedPeersMap, resourcePolicies, routers, nil, groupIDToUserIDs) } } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmap_golden_test.go` around lines 115 - 123, Precompute the group-user map once before the benchmark loop instead of calling account.GetActiveGroupUsers() inside the inner loop: call account.GetActiveGroupUsers() (store result in a variable like activeGroupUsers) prior to b.Run("components builder", ...) and then pass that variable into account.GetPeerNetworkMapFromComponents(...) in place of the inline call; apply the same change to the other "components builder" benchmark blocks in this file so the benchmarked work excludes repeated setup.
🤖 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/http/handlers/peers/peers_handler.go`:
- Line 420: The code calls account.GetPeerNetworkMapFromComponents(...) without
first injecting proxy-generated policies, causing missing peers for
proxy-reliant accounts; update the handler to call
account.InjectProxyPolicies(...) (using the same context and account) to produce
the injected resourcePolicies/resourceRouters before constructing validPeers and
calling GetPeerNetworkMapFromComponents, and then pass the resulting
account.GetResourcePoliciesMap() / account.GetResourceRoutersMap() (or the
injected maps returned by InjectProxyPolicies) into
GetPeerNetworkMapFromComponents so the network map includes proxy-generated
policies/resources.
---
Nitpick comments:
In `@management/server/types/networkmap_golden_test.go`:
- Around line 115-123: Precompute the group-user map once before the benchmark
loop instead of calling account.GetActiveGroupUsers() inside the inner loop:
call account.GetActiveGroupUsers() (store result in a variable like
activeGroupUsers) prior to b.Run("components builder", ...) and then pass that
variable into account.GetPeerNetworkMapFromComponents(...) in place of the
inline call; apply the same change to the other "components builder" benchmark
blocks in this file so the benchmarked work excludes repeated setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b9ed676b-8bbe-4cef-9b23-a207f0a34f0a
📒 Files selected for processing (9)
management/internals/controllers/network_map/controller/controller.gomanagement/server/account_test.gomanagement/server/http/handlers/peers/peers_handler.gomanagement/server/route_test.gomanagement/server/types/account.gomanagement/server/types/account_test.gomanagement/server/types/networkmap_comparison_test.gomanagement/server/types/networkmap_components.gomanagement/server/types/networkmap_golden_test.go
💤 Files with no reviewable changes (5)
- management/server/route_test.go
- management/server/types/networkmap_components.go
- management/server/types/account_test.go
- management/server/types/account.go
- management/server/types/networkmap_comparison_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
management/server/types/networkmap_components_test.go (1)
302-310: Consider explicitly settingGoOSin this subtest for clarity.The "fails OS posture check" subtest relies on
GoOSbeing"linux"from the account creation defaults (line 686). While this works correctly, explicitly setting it here would make the test self-documenting and resilient to future changes in the test fixture.💡 Suggested clarification
t.Run("fails OS posture check", func(t *testing.T) { account.Peers["peer-src-1"].Meta.WtVersion = "0.35.0" + account.Peers["peer-src-1"].Meta.GoOS = "linux" account.Peers["peer-src-1"].Meta.KernelVersion = "4.0.0" nm := networkMapFromComponents(t, account, "peer-src-1", validated)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmap_components_test.go` around lines 302 - 310, The subtest "fails OS posture check" relies on account defaults for OS; make it explicit by setting account.Peers["peer-src-1"].Meta.GoOS = "linux" inside that t.Run block so the test is self-documenting and robust to fixture changes; locate the subtest where Meta.WtVersion and Meta.KernelVersion are set (the failing OS posture check) and add the explicit GoOS assignment before calling networkMapFromComponents.
🤖 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_test.go`:
- Around line 302-310: The subtest "fails OS posture check" relies on account
defaults for OS; make it explicit by setting
account.Peers["peer-src-1"].Meta.GoOS = "linux" inside that t.Run block so the
test is self-documenting and robust to fixture changes; locate the subtest where
Meta.WtVersion and Meta.KernelVersion are set (the failing OS posture check) and
add the explicit GoOS assignment before calling networkMapFromComponents.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f01f8865-3ff7-4f65-8c24-6b2c2f03a8f1
📒 Files selected for processing (1)
management/server/types/networkmap_components_test.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/internals/controllers/network_map/controller/controller.go (1)
199-202:⚠️ Potential issue | 🟡 MinorUse the goroutine parameter
p.IDinstead of outer loop variablepeer.ID.Inside the goroutine, the peer is passed as parameter
p, but line 199 referencespeer.IDfrom the outer loop scope. While Go 1.22+ creates per-iteration loop variables (making this safe), usingp.IDis consistent with the function signature and the intent of passing the peer as a parameter.Proposed fix
- proxyNetworkMap, ok := proxyNetworkMaps[peer.ID] + proxyNetworkMap, ok := proxyNetworkMaps[p.ID]🤖 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 199 - 202, The goroutine is using the outer loop variable peer.ID instead of the goroutine parameter p.ID; update the lookup to use p.ID when accessing proxyNetworkMaps and any related references (e.g., change proxyNetworkMaps[peer.ID] to proxyNetworkMaps[p.ID]) so that the goroutine consistently uses its parameter and then call remotePeerNetworkMap.Merge(proxyNetworkMap) as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@management/internals/controllers/network_map/controller/controller.go`:
- Around line 199-202: The goroutine is using the outer loop variable peer.ID
instead of the goroutine parameter p.ID; update the lookup to use p.ID when
accessing proxyNetworkMaps and any related references (e.g., change
proxyNetworkMaps[peer.ID] to proxyNetworkMaps[p.ID]) so that the goroutine
consistently uses its parameter and then call
remotePeerNetworkMap.Merge(proxyNetworkMap) as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec0dc43b-918a-4496-9658-4f9c1f0932c5
📒 Files selected for processing (11)
management/internals/controllers/network_map/controller/controller.gomanagement/internals/controllers/network_map/interface.gomanagement/server/account_test.gomanagement/server/peer_test.gomanagement/server/store/sql_store.gomanagement/server/types/account.gomanagement/server/types/holder.gomanagement/server/types/networkmap.gomanagement/server/types/networkmap_components.gomanagement/server/types/networkmap_golden_test.gomanagement/server/types/networkmapbuilder.go
💤 Files with no reviewable changes (9)
- management/internals/controllers/network_map/interface.go
- management/server/types/networkmap_components.go
- management/server/types/holder.go
- management/server/store/sql_store.go
- management/server/peer_test.go
- management/server/types/networkmap_golden_test.go
- management/server/types/networkmapbuilder.go
- management/server/types/account.go
- management/server/types/networkmap.go
pascal-fischer
previously approved these changes
Apr 14, 2026
|
pascal-fischer
approved these changes
Apr 27, 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
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
removal of an old unused code of legacy network map calculation
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
removal of an unused code
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
Refactor
Tests