Conversation
📝 WalkthroughWalkthroughAdded two new test files: extensive correctness tests validating network-map component outputs and a suite of benchmarks measuring network-map generation and related sub-operations across multiple peer/group scales. Changes are limited to test code and introduce no production API changes. (43 words) Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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_benchmark_test.go`:
- Around line 138-145: The benchmark currently reconstructs the
NetworkMapBuilder inside the timed loop; move the
types.NewNetworkMapBuilder(account, validatedPeers) call so it runs once before
b.ResetTimer() and reuse that builder within the for range b.N loop, calling
builder.GetPeerNetworkMap(ctx, peerID, nbdns.CustomZone{}, nil, validatedPeers,
nil) for each peerID; this makes the "builder/..." sub-benchmark measure only
generation cost (like the legacy branches) and leaves
BenchmarkNetworkMapGeneration_BuilderInit to measure initialization separately.
- Around line 243-258: The benchmark currently measures setup/teardown work
(creating NewNetworkMapBuilder, allocating/timestamping a Peer, and mutating
account/validatedPeers) rather than just OnPeerAddedIncremental; fix it by
moving all builder/peer/account setup and cleanup outside the timed section or
by bracketing only the call to builder.OnPeerAddedIncremental with
b.StopTimer()/b.StartTimer() (or ResetTimer) so the timer runs only for the
invocation of OnPeerAddedIncremental; reference NewNetworkMapBuilder,
OnPeerAddedIncremental, account, validatedPeers and builder to locate and adjust
the code.
In `@management/server/types/networkmap_components_correctness_test.go`:
- Around line 628-665: Tests TestComponents_PostureCheckFiltering_FailingPeer
and TestComponents_MultiplePostureChecks only assert NotNil, so add concrete
assertions comparing the network maps returned by componentsNetworkMap for a
passing peer ("peer-0") and a failing peer ("peer-1"): verify that nm0 (from
"peer-0") contains resource/router routes (e.g., nm0.Routes length > nm1.Routes,
presence of resource-specific entries in nm0.ResourceRoutes or route.TargetType
== "resource") and that nm1 lacks those same entries or has visibility flags
disabled (e.g., Router visibility false) to demonstrate posture gating removed
resource routes for the failing peer; update
TestComponents_PostureCheckFiltering_PassingPeer/TestComponents_PostureCheckFiltering_FailingPeer
to assert these concrete deltas between nm0 and nm1 after calling
componentsNetworkMap.
- Around line 83-90: The test fixture currently appends a blanket allow policy
(policies append of &types.Policy with ID "policy-all" containing PolicyRule ID
"rule-all" that references "group-all") which makes many targeted tests vacuous;
change the fixture to make this baseline policy optional by adding a disable
flag or option (e.g., disableDefaultPolicy/DisableBaselinePolicy) to the test
setup and avoid appending the &types.Policy{"policy-all"/"rule-all"/"group-all"}
when that option is set, then update the isolated tests (e.g.,
TestComponents_CrossGroupConnectivity, TestComponents_BidirectionalPolicy,
TestComponents_PeerAsDestinationResource,
TestComponents_MultipleRoutersPerNetwork, TestComponents_DomainNetworkResource)
to construct the fixture with the flag enabled so they run without the baseline
allow rule.
- Around line 548-573: The test TestComponents_HARouteDeduplication currently
only asserts at least one HA route exists, which won't catch duplicate-emission
regressions; update the test to assert the exact expected representation for the
haNetwork by verifying nm.Routes yields exactly one entry for haNetwork (e.g.
replace assert.GreaterOrEqual(t, haRoutes, 1) with an exact assertion like
assert.Equal(t, 1, haRoutes) or assert.Len for the filtered slice) and
optionally validate that the single emitted route's fields
(PeerID/Metric/PeerGroups) match the expected selection logic from
componentsNetworkMap so the deduplication behavior is enforced.
🪄 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: dc4febdf-5894-46ba-ad5e-80bb200a11eb
📒 Files selected for processing (2)
management/server/types/networkmap_benchmark_test.gomanagement/server/types/networkmap_components_correctness_test.go
management/server/types/networkmap_components_correctness_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (2)
management/server/types/networkmap_components_correctness_test.go (2)
545-555: Assertion is vacuous when all routes are disabled.If
GetPeerNetworkMapFromComponentscorrectly excludes disabled routes,nm.Routeswill be empty and the loop assertion never executes. Add an explicit emptiness check to verify the expected behavior.♻️ Suggested fix
func TestComponents_DisabledRouteExcluded(t *testing.T) { account, validatedPeers := scalableTestAccount(50, 2) for _, r := range account.Routes { r.Enabled = false } nm := componentsNetworkMap(account, "peer-0", validatedPeers) require.NotNil(t, nm) - for _, r := range nm.Routes { - assert.True(t, r.Enabled, "only enabled routes should appear") - } + assert.Empty(t, nm.Routes, "disabled routes should not appear in network map") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmap_components_correctness_test.go` around lines 545 - 555, The test TestComponents_DisabledRouteExcluded currently disables all routes and then loops over nm.Routes making assertions that only run if nm.Routes is non-empty, which is vacuous; update the test (which builds its input via scalableTestAccount and calls componentsNetworkMap/GetPeerNetworkMapFromComponents) to explicitly assert that nm.Routes is empty when all account.Routes are disabled (e.g., use require.Empty or assert.Empty on nm.Routes) and keep the existing loop/true assertion only as a secondary check if you change the setup to leave some routes enabled.
631-639: Test assertion does not verify disabled resources are ignored.
assert.NotNil(t, nm.Network)only confirms the network map was created, which will always succeed for a valid account. Consider asserting that resource-related routes or router peer visibility are absent when resources are disabled.♻️ Suggested improvement
func TestComponents_DisabledNetworkResourceIgnored(t *testing.T) { - account, validatedPeers := scalableTestAccount(50, 5) + // Use account without default policy so resource policies provide the only + // connectivity to router peers + account, validatedPeers := scalableTestAccountWithoutDefaultPolicy(50, 5) + + // Capture router peer IDs before disabling resources + routerPeerIDs := make(map[string]bool) + for _, nr := range account.NetworkRouters { + routerPeerIDs[nr.Peer] = true + } + for _, nr := range account.NetworkResources { nr.Enabled = false } nm := componentsNetworkMap(account, "peer-0", validatedPeers) require.NotNil(t, nm) - assert.NotNil(t, nm.Network) + + // With resources disabled, resource policies should not grant visibility + // to router peers (assuming no other policies connect them) + for _, p := range nm.Peers { + if routerPeerIDs[p.ID] { + // Router peer visible via resource policy would indicate disabled + // resources are not being properly ignored + t.Logf("Warning: router peer %s still visible despite disabled resources", p.ID) + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmap_components_correctness_test.go` around lines 631 - 639, The test TestComponents_DisabledNetworkResourceIgnored currently only checks nm.Network is non-nil; change it to verify that disabled resources produce no network artifacts by inspecting the generated map from componentsNetworkMap: iterate account.NetworkResources (which you set Enabled=false) and assert that nm.Network.Routes contains no routes referencing any resource IDs and that nm.Network.Routers (or equivalent router/peer visibility structures returned by componentsNetworkMap) do not include peers or entries derived from those resource IDs; use explicit checks like len(nm.Network.Routes)==0 or absence of resource.ID in route/peer lists to ensure disabled resources are ignored.
🤖 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_correctness_test.go`:
- Around line 545-555: The test TestComponents_DisabledRouteExcluded currently
disables all routes and then loops over nm.Routes making assertions that only
run if nm.Routes is non-empty, which is vacuous; update the test (which builds
its input via scalableTestAccount and calls
componentsNetworkMap/GetPeerNetworkMapFromComponents) to explicitly assert that
nm.Routes is empty when all account.Routes are disabled (e.g., use require.Empty
or assert.Empty on nm.Routes) and keep the existing loop/true assertion only as
a secondary check if you change the setup to leave some routes enabled.
- Around line 631-639: The test TestComponents_DisabledNetworkResourceIgnored
currently only checks nm.Network is non-nil; change it to verify that disabled
resources produce no network artifacts by inspecting the generated map from
componentsNetworkMap: iterate account.NetworkResources (which you set
Enabled=false) and assert that nm.Network.Routes contains no routes referencing
any resource IDs and that nm.Network.Routers (or equivalent router/peer
visibility structures returned by componentsNetworkMap) do not include peers or
entries derived from those resource IDs; use explicit checks like
len(nm.Network.Routes)==0 or absence of resource.ID in route/peer lists to
ensure disabled resources are ignored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e3373bba-9d6a-4fec-a934-39b4ef45984f
📒 Files selected for processing (2)
management/server/types/networkmap_benchmark_test.gomanagement/server/types/networkmap_components_correctness_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- management/server/types/networkmap_benchmark_test.go
…ased test coverage for clarity and scalability.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
management/server/types/networkmap_benchmark_test.go (2)
29-33: Harden CI benchmark skip detection.Line 30 only skips when
CI=="true". Some environments setCI=1or uppercase variants, so benchmarks may run unintentionally.♻️ Proposed tweak
import ( "context" "fmt" "os" + "strconv" "testing" @@ func skipCIBenchmark(b *testing.B) { - if os.Getenv("CI") == "true" { + if ci, err := strconv.ParseBool(os.Getenv("CI")); err == nil && ci { b.Skip("Skipping benchmark in CI") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmap_benchmark_test.go` around lines 29 - 33, The CI skip currently only checks for exact "true" in skipCIBenchmark, which misses values like "1" or different case; update skipCIBenchmark to read os.Getenv("CI"), normalize it (e.g., strings.ToLower or strings.EqualFold) and treat "1" and "true" (case-insensitive) as true — if the value matches either, call b.Skip("Skipping benchmark in CI"); reference the skipCIBenchmark function and the use of os.Getenv("CI") when making the change.
77-80: Make peer iteration deterministic to reduce benchmark noise.Line 77–80 collects peer IDs from map iteration, so order is randomized and can add variance across runs. Sorting once will make comparisons steadier.
♻️ Proposed tweak
import ( "context" "fmt" "os" + "sort" "testing" @@ peerIDs := make([]string, 0, len(account.Peers)) for peerID := range account.Peers { peerIDs = append(peerIDs, peerID) } + sort.Strings(peerIDs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@management/server/types/networkmap_benchmark_test.go` around lines 77 - 80, The benchmark collects peer IDs from the map account.Peers into peerIDs, which yields nondeterministic order and noise; after building peerIDs (the loop that appends peerID), sort the slice (e.g., with sort.Strings(peerIDs)) so iteration is deterministic and add the required import for sort; locate the peerIDs variable and the for peerID := range account.Peers loop to apply this change.
🤖 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_benchmark_test.go`:
- Around line 29-33: The CI skip currently only checks for exact "true" in
skipCIBenchmark, which misses values like "1" or different case; update
skipCIBenchmark to read os.Getenv("CI"), normalize it (e.g., strings.ToLower or
strings.EqualFold) and treat "1" and "true" (case-insensitive) as true — if the
value matches either, call b.Skip("Skipping benchmark in CI"); reference the
skipCIBenchmark function and the use of os.Getenv("CI") when making the change.
- Around line 77-80: The benchmark collects peer IDs from the map account.Peers
into peerIDs, which yields nondeterministic order and noise; after building
peerIDs (the loop that appends peerID), sort the slice (e.g., with
sort.Strings(peerIDs)) so iteration is deterministic and add the required import
for sort; locate the peerIDs variable and the for peerID := range account.Peers
loop to apply this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 702b73c1-b5d5-469d-9130-298b640ff7f3
📒 Files selected for processing (2)
management/server/types/networkmap_benchmark_test.gomanagement/server/types/networkmap_components_correctness_test.go
✅ Files skipped from review due to trivial changes (1)
- management/server/types/networkmap_components_correctness_test.go



Describe your changes
Network Map Test Inventory
Complete inventory of all tests under
management/that test network map functionality directly or indirectly.Last verified: all tests passing on branch
nm/tests.DIRECT TESTS
Tests that call
GetPeerNetworkMapFromComponents,GetPeerNetworkMapComponents,CalculateNetworkMapFromComponents,GetPeerNetworkMap,NewNetworkMapBuilder, or directly construct/validateNetworkMapstructs.management/server/types/networkmap_components_correctness_test.go(NEW — 46 tests)Tests use
scalableTestAccount()(with default allow policy) orscalableTestAccountWithoutDefaultPolicy()(isolated, feature-specific connectivity only).TestComponents_PeerVisibilityTestComponents_PeerDoesNotSeeItselfTestComponents_IntraGroupConnectivityTestComponents_CrossGroupConnectivityTestComponents_BidirectionalPolicyTestComponents_ExpiredPeerInOfflineListTestComponents_ExpirationDisabledSettingTestComponents_LoginExpiration_PeerLevelTestComponents_NetworkSerialTestComponents_NonValidatedPeerExcludedTestComponents_NonValidatedTargetPeerGetsEmptyMapTestComponents_NonExistentPeerGetsEmptyMapTestComponents_FirewallRulesGeneratedTestComponents_DropPolicyGeneratesDropRulesTestComponents_DisabledPolicyIgnoredTestComponents_PortPolicyTestComponents_PortRangePolicyTestComponents_FirewallRuleDirectionTestComponents_RoutesIncludedTestComponents_DisabledRouteExcludedTestComponents_RoutesFirewallRulesForACGTestComponents_HARouteDeduplicationTestComponents_NetworkResourceRoutes_RouterPeerTestComponents_NetworkResourceRoutes_SourcePeerSeesRouterPeerTestComponents_DisabledNetworkResourceIgnoredTestComponents_PostureCheckFiltering_PassingPeerTestComponents_PostureCheckFiltering_FailingPeerTestComponents_MultiplePostureChecksTestComponents_DNSConfigEnabledTestComponents_DNSDisabledByManagementGroupTestComponents_DNSNameServerGroupDistributionTestComponents_DNSCustomZoneTestComponents_SSHPolicyTestComponents_SSHNotEnabledWithoutPolicyTestComponents_AllPeersGetValidMapsTestComponents_LargeScaleMapGenerationTestComponents_PeerAsSourceResourceTestComponents_PeerAsDestinationResourceTestComponents_MultipleRulesPerPolicyTestComponents_SSHAuthorizedUsersContentTestComponents_SSHLegacyImpliedSSHTestComponents_RouteDefaultPermitTestComponents_MultipleRoutersPerNetworkTestComponents_PeerIsNameserverExcludedFromNSGroupTestComponents_DomainNetworkResourceTestComponents_DisabledRuleInEnabledPolicymanagement/server/types/networkmap_golden_test.go(existing — 6 tests + 4 benchmarks)TestGetPeerNetworkMap_GoldenTestGetPeerNetworkMap_Golden_WithNewPeerTestGetPeerNetworkMap_Golden_WithNewRoutingPeerTestGetPeerNetworkMap_Golden_WithDeletedPeerTestGetPeerNetworkMap_Golden_WithDeletedRouterPeerTestGetPeerNetworkMap_Golden_New_WithOnPeerAddedRouter_BatchedBenchmarkGetPeerNetworkMapBenchmarkGetPeerNetworkMap_AfterPeerAddedBenchmarkGetPeerNetworkMap_AfterRouterPeerAddedBenchmarkGetPeerNetworkMap_AfterPeerDeletedmanagement/server/types/networkmap_comparison_test.go(existing — 2 tests + 4 benchmarks)TestNetworkMapComponents_CompareWithLegacyTestNetworkMapComponents_GoldenFileComparisonBenchmarkLegacyNetworkMapBenchmarkComponentsNetworkMapBenchmarkComponentsCreationBenchmarkCalculationFromComponentsmanagement/server/types/account_test.go(existing — 17 tests)Test_GetResourceRoutersMapTest_GetResourcePoliciesMapTest_AddNetworksRoutingPeersAddsMissingPeersTest_AddNetworksRoutingPeersIgnoresExistingPeersTest_AddNetworksRoutingPeersAddsExpiredPeersTest_AddNetworksRoutingPeersExcludesSelfTest_AddNetworksRoutingPeersHandlesNoMissingPeersTest_NetworksNetMapGenWithNoPostureChecksTest_NetworksNetMapGenWithPostureChecksTest_NetworksNetMapGenWithNoMatchedPostureChecksTest_NetworksNetMapGenWithTwoPoliciesAndPostureChecksTest_NetworksNetMapGenWithTwoPostureChecksTest_NetworksNetMapGenShouldExcludeOtherRoutersTest_ExpandPortsAndRanges_SSHRuleExpansionTest_GetActiveGroupUsersTest_FilterZoneRecordsForPeersTest_filterPeerAppliedZonesmanagement/server/account_test.go(existing — 2 tests)TestAccount_GetPeerNetworkMapTestAccount_GetRoutesToSyncmanagement/server/route_test.go(existing — 3 tests)TestGetNetworkMap_RouteSyncPeerGroupsTestGetNetworkMap_RouteSyncTestAccount_GetPeerNetworkResourceFirewallRulesmanagement/server/peer_test.go(existing — 1 test)TestToSyncResponsemanagement/server/types/networkmap_benchmark_test.go(NEW — 7 benchmarks)All benchmarks are skipped in CI (
skipCIBenchmark). Run locally withgo test -bench=....BenchmarkNetworkMapGeneration_ComponentsBenchmarkNetworkMapGeneration_AllPeersBenchmarkNetworkMapGeneration_ComponentsCreationBenchmarkNetworkMapGeneration_ComponentsCalculationBenchmarkNetworkMapGeneration_PrecomputeMapsBenchmarkNetworkMapGeneration_GroupScalingBenchmarkNetworkMapGeneration_PeerScalingINDIRECT TESTS
Tests that trigger network map generation through higher-level operations (LoginPeer, SyncPeer, AddPeer, UpdateAccountPeers, etc.).
management/server/peer_test.goTestAccountManager_GetNetworkMapTestAccountManager_GetNetworkMap_ExperimentalTestAccountManager_GetNetworkMapWithPolicyTestAccountManager_GetPeerNetworkTestDefaultAccountManager_GetPeersTestPeerAccountPeersUpdateTestUpdateAccountPeersTestUpdateAccountPeers_ExperimentalTest_LoginPeerTest_RegisterPeerByUserTest_RegisterPeerBySetupKeyTest_AddPeerTestAddPeer_UserPendingApprovalBlockedTestAddPeer_ApprovedUserCanAddPeersTestLoginPeer_UserPendingApprovalBlockedTestLoginPeer_ApprovedUserCanLoginTestHandleUserAddedPeerTestHandleSetupKeyAddedPeerTestProcessPeerAddAuthBenchmarkGetPeersBenchmarkUpdateAccountPeersmanagement/server/account_test.goTestAccountManager_AddPeerTestAccountManager_AddPeerWithUserIDTestAccountManager_NetworkUpdates_DeletePeerTestAccountManager_NetworkUpdates_DeletePeer_ExperimentalTestDefaultAccountManager_UpdatePeer_PeerLoginExpirationTestDefaultAccountManager_UpdateAccountSettings_PeerLoginExpirationTestDefaultAccountManager_UpdateAccountSettings_PeerApprovalTestDefaultAccountManager_UpdatePeerIPBenchmarkSyncAndMarkPeerBenchmarkLoginPeer_ExistingPeerBenchmarkLoginPeer_NewPeermanagement/server/dns_test.goTestGetNetworkMap_DNSConfigSyncTestDNSAccountPeersUpdatemanagement/server/route_test.goTestRouteAccountPeersUpdatemanagement/server/group_test.goTestGroupAccountPeersUpdatemanagement/server/policy_test.goTestPolicyAccountPeersUpdatemanagement/server/nameserver_test.goTestNameServerAccountPeersUpdatemanagement/server/setupkey_test.goTestSetupKeyAccountPeersUpdatemanagement/server/posture_checks_test.goTestPostureCheckAccountPeersUpdatemanagement/server/user_test.goTestUserAccountPeersUpdatemanagement/server/management_test.go(gRPC integration)TestSyncNewPeerConfigurationTestSyncThreePeersTestSyncNewPeerUpdateTestSync10PeersGetUpdatesTestConcurrentPeersNoDuplicateIPsmanagement/server/management_proto_test.goTest_LoginPerformanceHTTP Handler Integration Tests (
management/server/http/testing/integration/)Test_Peers_GetAllTest_Peers_GetByIdTest_Peers_UpdateTest_Peers_DeleteTest_Peers_GetAccessiblePeersTest_Groups_GetAllTest_Groups_UpdateTest_Groups_DeleteTest_Routes_GetAllTest_Routes_CreateTest_Routes_UpdateTest_Routes_DeleteTest_Policies_GetAllTest_Policies_CreateTest_Policies_UpdateTest_Policies_DeleteTest_DnsSettings_UpdateTest_Nameservers_GetAllTest_Nameservers_CreateTest_Nameservers_UpdateTest_Nameservers_DeleteTest_NetworkResources_*(6 tests)Test_NetworkRouters_*(6 tests)HTTP Handler Benchmarks (
management/server/http/testing/benchmarks/) (require-tags=benchmark)BenchmarkPeersHandler_*(Create/Update/GetOne/GetAll/Delete)BenchmarkSetupKeysHandler_*BenchmarkUsersHandler_*Store Benchmarks (
management/server/store/)BenchmarkGetAccountBenchmarkGetAccountPeersBenchmarkTest_StoreWrite/BenchmarkTest_StoreReadFEATURE COVERAGE MATRIX
SUMMARY
Test isolation notes
Tests marked "(isolated)" use
scalableTestAccountWithoutDefaultPolicy()which omits the blanketgroup-all allow rule, ensuring the test validates feature-specific connectivity rather than passing
vacuously through the catch-all policy. All new benchmarks use
skipCIBenchmark()to avoid runningin CI environments.
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