offload client config generation to the client#6711
Open
dmitri-netbird wants to merge 36 commits into
Open
Conversation
# Conflicts: # management/server/posture/checks.go
# Conflicts: # client/internal/engine.go # management/server/store/sql_store.go # shared/management/proto/management.pb.go
Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io>
Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io>
Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io>
Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io>
Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io>
Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io>
Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds component-based NetworkMap synchronization with server encoding and client decoding, introduces PublicID persistence across management entities, updates shared conversion helpers and generated mocks, and adjusts lint timeout and Dex SQLite build handling. ChangesComponent NetworkMap Format
PublicID sequence-ID migration
Generated store mock regeneration
CI timeout and Dex SQLite build tags
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
…indexes-use-xids Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io>
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
![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: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
management/server/policy.go (1)
61-76: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUnchanged-policy early return skips
PublicIDassignment.When
policy.Equal(existingPolicy)short-circuits the update,policy.PublicIDnever gets set fromexistingPolicy.PublicID, soSavePolicyreturns aPolicywith an emptyPublicIDfor no-op updates, while an actual update correctly returns the preserved one.🐛 Proposed fix
if isUpdate { if policy.Equal(existingPolicy) { log.WithContext(ctx).Tracef("policy update skipped because equal to stored one - policy id %s", policy.ID) unchanged = true + policy.PublicID = existingPolicy.PublicID return nil }Also applies to: 99-101
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@management/server/policy.go` around lines 61 - 76, The no-op update path in policy save skips copying the stored PublicID, so the returned policy can have an empty identifier even when nothing changed. In the policy update flow inside the save logic for policy persistence, move the PublicID assignment from the non-equal update branch so it runs before the policy.Equal(existingPolicy) early return, ensuring the unchanged policy result preserves existingPolicy.PublicID just like the normal update path.dns/nameserver.go (1)
51-73: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
NameServerGroup.Copy()dropsPublicID.Unlike
Group.Copy()andPolicy.Copy()in this same PR,NameServerGroup.Copy()does not copy the newPublicIDfield. SinceCreateNameServerGroupreturnsnewNSGroup.Copy()to its caller, the freshly-generatedPublicIDwill be silently lost from the response even though it was correctly persisted.🐛 Proposed fix
func (g *NameServerGroup) Copy() *NameServerGroup { nsGroup := &NameServerGroup{ ID: g.ID, + AccountID: g.AccountID, + PublicID: g.PublicID, Name: g.Name, Description: g.Description, NameServers: make([]NameServer, len(g.NameServers)), Groups: make([]string, len(g.Groups)), Enabled: g.Enabled, Primary: g.Primary, Domains: make([]string, len(g.Domains)), SearchDomainsEnabled: g.SearchDomainsEnabled, }Also applies to: 142-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dns/nameserver.go` around lines 51 - 73, NameServerGroup.Copy() is not preserving the new PublicID field, so the response from CreateNameServerGroup loses the generated public identifier even though it was saved. Update the NameServerGroup.Copy method to include PublicID when cloning the struct, matching the behavior of Group.Copy and Policy.Copy, and ensure CreateNameServerGroup still returns the copied object with PublicID intact.management/server/store/sql_store.go (2)
2394-2409: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMap
account_seq_idto a struct field or stop usingRowToStructByNameheregetNetworkswill fail on theaccount_seq_idcolumn becausenetworkTypes.Networkhas no matching field/tag (PublicIDis untagged). Use explicitScan, or add a pgxdb:"account_seq_id"tag onPublicID.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@management/server/store/sql_store.go` around lines 2394 - 2409, The getNetworks function is using pgx.RowToStructByName against the networks query, but networkTypes.Network has no matching field/tag for account_seq_id, so row mapping will fail. Fix this either by adding the appropriate db tag to the PublicID field in networkTypes.Network or by replacing RowToStructByName in getNetworks with explicit row scanning so the account_seq_id column is handled correctly.
2206-2225: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMap
PublicIDtoaccount_seq_idin these models
getPostureChecks,getNetworkRouters, andgetNetworkResourcesscanaccount_seq_idinto.PublicID, butmanagement/server/posture/checks.go,management/server/networks/routers/types/router.go, andmanagement/server/networks/resources/types/resource.goleavePublicIDunmapped (json:"-"only). With the currentSave/Create/Updatescalls, GORM will targetpublic_id, so the reads and writes are using different columns.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@management/server/store/sql_store.go` around lines 2206 - 2225, The reads and writes for PublicID are mismatched because getPostureChecks, getNetworkRouters, and getNetworkResources scan account_seq_id into PublicID while the related GORM models leave PublicID unmapped. Update the affected model structs (posture.Checks, router types, and resource types) so PublicID is bound to account_seq_id instead of the default public_id, and keep the SQL scan logic aligned with that field. Verify the Save/Create/Updates paths now target the same column that the getters populate.management/server/networks/resources/types/resource.go (1)
31-44: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftMap
NetworkResource.PublicIDtoaccount_seq_idgetNetworkResourcesreadsaccount_seq_idintoPublicID, butSaveNetworkResourcewill persist the field topublic_idby default. That leaves the value out of sync on round-trip; add the same column tag asNetwork.PublicID.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@management/server/networks/resources/types/resource.go` around lines 31 - 44, `NetworkResource.PublicID` is being read from account_seq_id but saved to the default public_id column, causing round-trip mismatch. Update the `NetworkResource` struct to use the same GORM column mapping as `Network.PublicID`, and verify `SaveNetworkResource` and `getNetworkResources` both target the same database field via `NetworkResource.PublicID`.management/server/posture/checks.go (1)
39-56: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMap
PublicIDtoaccount_seq_id
SavePostureChecksuses GORMSave, whilegetPostureChecksscansaccount_seq_idintoPublicID. Without an explicitgorm:"column:account_seq_id"tag, GORM will targetpublic_idinstead, so this value won’t round-trip.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@management/server/posture/checks.go` around lines 39 - 56, The Checks model’s PublicID field is not mapped to the database column that getPostureChecks reads from, so SavePostureChecks will persist to the wrong column. Update the PublicID field in Checks to explicitly use gorm:"column:account_seq_id" (keeping the existing JSON behavior if needed) so GORM round-trips the value consistently between SavePostureChecks and getPostureChecks.management/server/networks/routers/types/router.go (1)
12-22: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMap
NetworkRouter.PublicIDtoaccount_seq_id
CreateNetworkRouterandUpdateNetworkRouteruse plain GORM, while the router read paths loadaccount_seq_id. Without a column tag,PublicIDis written topublic_idinstead.Suggested change
- PublicID string `json:"-"` + PublicID string `gorm:"column:account_seq_id" db:"account_seq_id" json:"-"`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@management/server/networks/routers/types/router.go` around lines 12 - 22, The NetworkRouter.PublicID field is currently being persisted to the default public_id column instead of account_seq_id. Update the NetworkRouter struct so the PublicID field in router.go is explicitly mapped with the correct GORM column tag, and verify CreateNetworkRouter and UpdateNetworkRouter continue using that struct so reads and writes stay consistent with account_seq_id.
🧹 Nitpick comments (2)
shared/management/proto/management.proto (1)
939-943: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDon’t reserve future delta field numbers here.
reserved 1 to 100permanently blocks those numbers from being added later; keep the message empty until the delta schema is defined, or reserve only deleted field numbers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/management/proto/management.proto` around lines 939 - 943, The NetworkMapComponentsDelta message is unnecessarily reserving future field numbers, which prevents those delta fields from being added later. Update the message definition in management.proto so NetworkMapComponentsDelta remains empty until the schema is defined, or only use reserved numbers for fields that have been removed; keep the change focused on the message itself and its reserved field declaration.management/server/store/sql_store.go (1)
645-710: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
CreateGroupsandUpdateGroupsare now identical — consider consolidating.Both functions perform the exact same
OnConflict/DoUpdates(groupUpsertColumns())/Omit(clause.Associations)/Createsequence. Extracting a shared private helper (e.g.upsertGroups) would remove the duplication without changing behavior.♻️ Proposed refactor
+func (s *SqlStore) upsertGroups(ctx context.Context, accountID string, groups []*types.Group) error { + if len(groups) == 0 { + return nil + } + return s.db.Transaction(func(tx *gorm.DB) error { + result := tx. + Clauses( + clause.OnConflict{ + Columns: []clause.Column{{Name: "id"}}, + Where: clause.Where{Exprs: []clause.Expression{clause.Eq{Column: "groups.account_id", Value: accountID}}}, + DoUpdates: groupUpsertColumns(), + }, + ). + Omit(clause.Associations). + Create(&groups) + if result.Error != nil { + log.WithContext(ctx).Errorf("failed to save groups to store: %v", result.Error) + return status.Errorf(status.Internal, "failed to save groups to store") + } + return nil + }) +} + +func (s *SqlStore) CreateGroups(ctx context.Context, accountID string, groups []*types.Group) error { + return s.upsertGroups(ctx, accountID, groups) +} + +func (s *SqlStore) UpdateGroups(ctx context.Context, accountID string, groups []*types.Group) error { + return s.upsertGroups(ctx, accountID, groups) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@management/server/store/sql_store.go` around lines 645 - 710, CreateGroups and UpdateGroups contain the same upsert flow, so consolidate the duplicated logic into a shared private helper such as upsertGroups in SqlStore. Move the common transaction, OnConflict with groupUpsertColumns, Omit(clause.Associations), and Create call into that helper, then have both CreateGroups and UpdateGroups delegate to it while preserving the existing error logging and status return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@management/internals/controllers/network_map/controller/controller.go`:
- Around line 603-607: The initial sync path in `GetPeerNetworkMapResult` can
return nil components when `account.GetPeerNetworkMapComponents` yields nil,
which causes `ToComponentSyncResponse` to fall back to a zero network instead of
the account network identifier. Mirror the nil-components fallback used in the
update dispatch path by replacing nil `components` with a component set that
preserves the account network before returning from `GetPeerNetworkMapResult`,
so `peer`, `components`, and the resulting sync response always carry the
correct account network context.
- Line 570: The initial-sync path in the network map controller is bypassing the
same proxy-policy injection flow used elsewhere, so update the component setup
to follow the existing controller-level path. In the controller logic around
account setup, replace the direct account.InjectProxyPolicies call with the same
injectAllProxyPolicies path used by streaming updates and legacy initial sync,
so controller-level proxy policy injection happens before building components.
In `@management/internals/shared/grpc/components_envelope_response.go`:
- Around line 66-70: The component sync response in SyncResponse is missing
SessionExpiresAt, unlike the legacy path, so peers can miss login-expiration
updates. Update the response construction in the component envelope flow to
populate SessionExpiresAt whenever settings are available, matching the behavior
used in the legacy response path. Use the existing SyncResponse assembly and the
shared settings data source so the component-capable path preserves the same
session expiry information.
In `@management/internals/shared/grpc/conversion_test.go`:
- Around line 110-114: The “WithoutCache” benchmark is still exercising cache
allocation and population on every iteration instead of the true no-cache path.
Update the benchmark case in the `b.Run("WithoutCache...")` block that calls
`networkmap.ToProtocolDNSConfig` so it passes a nil cache rather than
constructing `cache.DNSConfigCache` inside the loop, keeping the benchmark
focused on the actual no-cache behavior.
In `@management/internals/shared/grpc/server.go`:
- Around line 1019-1043: sendInitialSync is rebuilding the response with data
from GetValidatedPeerWithComponents but still passes the stale postureChecks
snapshot into ToComponentSyncResponse. Update the initial-sync branch in
sendInitialSync to use the checks returned by GetValidatedPeerWithComponents
when constructing the component response, and keep the fallback ToSyncResponse
path unchanged so both components and checks come from the same validated read.
In `@management/server/networks/types/network.go`:
- Around line 9-17: The Network.PublicID field is missing the explicit column
mapping needed for both GORM persistence and pgx row scanning, so it currently
resolves to public_id instead of account_seq_id. Update the Network struct to
add the appropriate db/gorm tag on PublicID so sql_store.go’s getNetworks and
pgx.RowToStructByName[networkTypes.Network] can match the selected
account_seq_id column, and ensure SaveNetwork round-trips the same field name
used by the database schema.
In `@management/server/types/account_components.go`:
- Around line 330-383: The route advertiser lookup in the relevant-route
assembly logic is using the WireGuard key field instead of the peer ID, so the
peer map lookup can miss valid advertisers. Update the `Route` handling in
`account_components.go` to use `Route.PeerID` consistently when checking
`validatedPeersMap` and calling `a.GetPeer`, while keeping the existing
relevance filtering and `relevantPeerIDs` population behavior intact.
In `@route/route.go`:
- Line 98: The Route.PublicID field is currently not mapped to the
account_seq_id column, so SaveRoute will persist it to the default public_id
field instead. Update the Route model in route.Route to add the proper GORM
column override for PublicID, or adjust SaveRoute to avoid saving that field
through s.db.Save(route) if it should be excluded. Use the Route struct and
SaveRoute method to locate the write path and ensure PublicID consistently maps
to account_seq_id.
In `@shared/management/client/client_test.go`:
- Around line 371-382: The helper is reading NetworkMapEnvelope.Full.Peers
directly, which tests the component peer universe instead of the actual remote
peer list. Update the test/helper around the existing envelope handling to
decode the envelope with EnvelopeToNetworkMap and assert against
result.NetworkMap.RemotePeers, using the current peer/key filtering logic in the
same function to keep the comparison aligned with the real NetworkMap view.
In `@shared/management/networkmap/decode.go`:
- Around line 110-112: The peer index handling in decode.go is appending empty
peer IDs whenever `peerIDByIndex[idx]` was never resolved, so update the lookup
logic used by `GroupCodec.Decode`, `PeerIndexes`, and the later map/resource
builders to skip both out-of-bounds indexes and empty IDs. Add a small shared
resolver/helper around `peerIDByIndex` access, then use it at the affected
append sites so groups, posture failures, router maps, routes, and resources
never store unresolved peer IDs.
- Around line 486-496: The decode helpers are currently unused, causing lint
failures: either remove the dead helpers or make them part of the decode flow.
Update the `synthID` function in `decode.go` and the unused `lookupAgentVersion`
symbol so they are either referenced from the relevant decode path or deleted if
no longer needed, keeping the naming and call sites consistent with the existing
network map decoding logic.
In `@shared/management/networkmap/envelope.go`:
- Around line 51-54: Handle the empty decoded peer set in the envelope decoding
flow before looking up localPeer in the NetworkMap/Envelope handling path.
Update the logic around components.Peers and localPeer so that when no peers are
present, full.PeerConfig still returns a minimal proto.NetworkMap populated from
the envelope’s DNS/proxy fields instead of erroring, and keep the current error
only for non-empty snapshots where the receiver is genuinely missing.
In `@shared/management/proto/management.proto`:
- Around line 1160-1170: The wire-type comment on NetworkResourceRaw is stale
and does not match the actual schema. Update the generated contract comment
above the NetworkResourceRaw message so it describes field 2 as string
network_seq, keeping the rest of the compatibility note intact and aligned with
the current proto definition.
In `@shared/management/types/firewall_helpers.go`:
- Around line 113-116: Replace the string-substring check in
peerSupportedFirewallFeatures with version.IsDevelopmentVersion() so development
builds are detected consistently instead of matching arbitrary “dev” substrings.
Update the peerVer handling in that function to use the version helper and keep
the existing supportedFeatures return behavior unchanged for development
versions.
---
Outside diff comments:
In `@dns/nameserver.go`:
- Around line 51-73: NameServerGroup.Copy() is not preserving the new PublicID
field, so the response from CreateNameServerGroup loses the generated public
identifier even though it was saved. Update the NameServerGroup.Copy method to
include PublicID when cloning the struct, matching the behavior of Group.Copy
and Policy.Copy, and ensure CreateNameServerGroup still returns the copied
object with PublicID intact.
In `@management/server/networks/resources/types/resource.go`:
- Around line 31-44: `NetworkResource.PublicID` is being read from
account_seq_id but saved to the default public_id column, causing round-trip
mismatch. Update the `NetworkResource` struct to use the same GORM column
mapping as `Network.PublicID`, and verify `SaveNetworkResource` and
`getNetworkResources` both target the same database field via
`NetworkResource.PublicID`.
In `@management/server/networks/routers/types/router.go`:
- Around line 12-22: The NetworkRouter.PublicID field is currently being
persisted to the default public_id column instead of account_seq_id. Update the
NetworkRouter struct so the PublicID field in router.go is explicitly mapped
with the correct GORM column tag, and verify CreateNetworkRouter and
UpdateNetworkRouter continue using that struct so reads and writes stay
consistent with account_seq_id.
In `@management/server/policy.go`:
- Around line 61-76: The no-op update path in policy save skips copying the
stored PublicID, so the returned policy can have an empty identifier even when
nothing changed. In the policy update flow inside the save logic for policy
persistence, move the PublicID assignment from the non-equal update branch so it
runs before the policy.Equal(existingPolicy) early return, ensuring the
unchanged policy result preserves existingPolicy.PublicID just like the normal
update path.
In `@management/server/posture/checks.go`:
- Around line 39-56: The Checks model’s PublicID field is not mapped to the
database column that getPostureChecks reads from, so SavePostureChecks will
persist to the wrong column. Update the PublicID field in Checks to explicitly
use gorm:"column:account_seq_id" (keeping the existing JSON behavior if needed)
so GORM round-trips the value consistently between SavePostureChecks and
getPostureChecks.
In `@management/server/store/sql_store.go`:
- Around line 2394-2409: The getNetworks function is using pgx.RowToStructByName
against the networks query, but networkTypes.Network has no matching field/tag
for account_seq_id, so row mapping will fail. Fix this either by adding the
appropriate db tag to the PublicID field in networkTypes.Network or by replacing
RowToStructByName in getNetworks with explicit row scanning so the
account_seq_id column is handled correctly.
- Around line 2206-2225: The reads and writes for PublicID are mismatched
because getPostureChecks, getNetworkRouters, and getNetworkResources scan
account_seq_id into PublicID while the related GORM models leave PublicID
unmapped. Update the affected model structs (posture.Checks, router types, and
resource types) so PublicID is bound to account_seq_id instead of the default
public_id, and keep the SQL scan logic aligned with that field. Verify the
Save/Create/Updates paths now target the same column that the getters populate.
---
Nitpick comments:
In `@management/server/store/sql_store.go`:
- Around line 645-710: CreateGroups and UpdateGroups contain the same upsert
flow, so consolidate the duplicated logic into a shared private helper such as
upsertGroups in SqlStore. Move the common transaction, OnConflict with
groupUpsertColumns, Omit(clause.Associations), and Create call into that helper,
then have both CreateGroups and UpdateGroups delegate to it while preserving the
existing error logging and status return behavior.
In `@shared/management/proto/management.proto`:
- Around line 939-943: The NetworkMapComponentsDelta message is unnecessarily
reserving future field numbers, which prevents those delta fields from being
added later. Update the message definition in management.proto so
NetworkMapComponentsDelta remains empty until the schema is defined, or only use
reserved numbers for fields that have been removed; keep the change focused on
the message itself and its reserved field declaration.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 161524be-8ea6-4562-8f7e-e22e045958f5
⛔ Files ignored due to path filters (1)
shared/management/proto/management.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (67)
.github/workflows/golangci-lint.ymlclient/internal/engine.godns/nameserver.goidp/dex/config.goidp/dex/provider.goidp/dex/sqlite_cgo.goidp/dex/sqlite_nocgo.gomanagement/internals/controllers/network_map/controller/controller.gomanagement/internals/controllers/network_map/interface.gomanagement/internals/controllers/network_map/interface_mock.gomanagement/internals/shared/grpc/components_encoder.gomanagement/internals/shared/grpc/components_encoder_test.gomanagement/internals/shared/grpc/components_envelope_response.gomanagement/internals/shared/grpc/components_envelope_response_test.gomanagement/internals/shared/grpc/conversion.gomanagement/internals/shared/grpc/conversion_test.gomanagement/internals/shared/grpc/server.gomanagement/server/account.gomanagement/server/account_test.gomanagement/server/group.gomanagement/server/nameserver.gomanagement/server/networks/manager.gomanagement/server/networks/manager_test.gomanagement/server/networks/resources/manager.gomanagement/server/networks/resources/types/resource.gomanagement/server/networks/routers/manager.gomanagement/server/networks/routers/types/router.gomanagement/server/networks/types/network.gomanagement/server/peer/peer.gomanagement/server/policy.gomanagement/server/posture/checks.gomanagement/server/posture_checks.gomanagement/server/posture_checks_test.gomanagement/server/route.gomanagement/server/store/sql_store.gomanagement/server/store/store_mock.gomanagement/server/store/store_mock_agentnetwork.gomanagement/server/types/account.gomanagement/server/types/account_components.gomanagement/server/types/account_test.gomanagement/server/types/aliases.gomanagement/server/types/networkmap_components_correctness_test.gomanagement/server/types/networkmap_wire_benchmark_test.gomanagement/server/types/networkmap_wire_breakdown_test.gomanagement/server/types/peer_networkmap_result.gomanagement/server/types/peer_networkmap_result_test.goroute/route.goshared/management/client/client_test.goshared/management/client/grpc.goshared/management/networkmap/decode.goshared/management/networkmap/encode.goshared/management/networkmap/envelope.goshared/management/networkmap/envelope_test.goshared/management/proto/management.protoshared/management/types/dns_settings.goshared/management/types/firewall_helpers.goshared/management/types/firewall_rule.goshared/management/types/firewall_rule_test.goshared/management/types/group.goshared/management/types/network.goshared/management/types/network_test.goshared/management/types/networkmap_components.goshared/management/types/networkmap_components_compact.goshared/management/types/policy.goshared/management/types/policyrule.goshared/management/types/resource.goshared/management/types/route_firewall_rule.go
💤 Files with no reviewable changes (1)
- management/server/store/store_mock_agentnetwork.go
| return nil, nil, nil, nil, 0, err | ||
| } | ||
|
|
||
| account.InjectProxyPolicies(ctx) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match the existing proxy-policy injection path.
Streaming updates and legacy initial sync call c.injectAllProxyPolicies; this component initial-sync path only calls account.InjectProxyPolicies, which can omit controller-level proxy policy injection before building components.
Proposed fix
- account.InjectProxyPolicies(ctx)
+ c.injectAllProxyPolicies(ctx, account)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| account.InjectProxyPolicies(ctx) | |
| c.injectAllProxyPolicies(ctx, account) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@management/internals/controllers/network_map/controller/controller.go` at
line 570, The initial-sync path in the network map controller is bypassing the
same proxy-policy injection flow used elsewhere, so update the component setup
to follow the existing controller-level path. In the controller logic around
account setup, replace the direct account.InjectProxyPolicies call with the same
injectAllProxyPolicies path used by streaming updates and legacy initial sync,
so controller-level proxy policy injection happens before building components.
Comment on lines
+603
to
+607
| components := account.GetPeerNetworkMapComponents(ctx, peer.ID, peersCustomZone, accountZones, approvedPeersMap, resourcePolicies, routers, groupIDToUserIDs) | ||
|
|
||
| dnsFwdPort := computeForwarderPort(maps.Values(account.Peers), network_map.DnsForwarderPortMinVersion) | ||
|
|
||
| return peer, components, proxyNetworkMaps[peer.ID], postureChecks, dnsFwdPort, nil |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Apply the same nil-components fallback used by update dispatch.
If GetPeerNetworkMapComponents returns nil, this path returns nil components and ToComponentSyncResponse falls back to a zero network. Mirror GetPeerNetworkMapResult so initial sync still carries the account network identifier.
Proposed fix
components := account.GetPeerNetworkMapComponents(ctx, peer.ID, peersCustomZone, accountZones, approvedPeersMap, resourcePolicies, routers, groupIDToUserIDs)
+ if components == nil {
+ components = &types.NetworkMapComponents{
+ PeerID: peer.ID,
+ Network: account.Network.Copy(),
+ }
+ }
dnsFwdPort := computeForwarderPort(maps.Values(account.Peers), network_map.DnsForwarderPortMinVersion)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| components := account.GetPeerNetworkMapComponents(ctx, peer.ID, peersCustomZone, accountZones, approvedPeersMap, resourcePolicies, routers, groupIDToUserIDs) | |
| dnsFwdPort := computeForwarderPort(maps.Values(account.Peers), network_map.DnsForwarderPortMinVersion) | |
| return peer, components, proxyNetworkMaps[peer.ID], postureChecks, dnsFwdPort, nil | |
| components := account.GetPeerNetworkMapComponents(ctx, peer.ID, peersCustomZone, accountZones, approvedPeersMap, resourcePolicies, routers, groupIDToUserIDs) | |
| if components == nil { | |
| components = &types.NetworkMapComponents{ | |
| PeerID: peer.ID, | |
| Network: account.Network.Copy(), | |
| } | |
| } | |
| dnsFwdPort := computeForwarderPort(maps.Values(account.Peers), network_map.DnsForwarderPortMinVersion) | |
| return peer, components, proxyNetworkMaps[peer.ID], postureChecks, dnsFwdPort, nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@management/internals/controllers/network_map/controller/controller.go` around
lines 603 - 607, The initial sync path in `GetPeerNetworkMapResult` can return
nil components when `account.GetPeerNetworkMapComponents` yields nil, which
causes `ToComponentSyncResponse` to fall back to a zero network instead of the
account network identifier. Mirror the nil-components fallback used in the
update dispatch path by replacing nil `components` with a component set that
preserves the account network before returning from `GetPeerNetworkMapResult`,
so `peer`, `components`, and the resulting sync response always carry the
correct account network context.
Comment on lines
110
to
+114
| b.Run(fmt.Sprintf("WithoutCache-Size%d", size), func(b *testing.B) { | ||
| b.ResetTimer() | ||
| for i := 0; i < b.N; i++ { | ||
| cache := &cache.DNSConfigCache{} | ||
| toProtocolDNSConfig(testData, cache, int64(network_map.DnsForwarderPort)) | ||
| networkmap.ToProtocolDNSConfig(testData, cache, int64(network_map.DnsForwarderPort)) |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Benchmark the actual no-cache path.
ToProtocolDNSConfig supports nil for no caching, but this “WithoutCache” case allocates and populates a cache on every iteration.
Suggested fix
b.Run(fmt.Sprintf("WithoutCache-Size%d", size), func(b *testing.B) {
b.ResetTimer()
for i := 0; i < b.N; i++ {
- cache := &cache.DNSConfigCache{}
- networkmap.ToProtocolDNSConfig(testData, cache, int64(network_map.DnsForwarderPort))
+ networkmap.ToProtocolDNSConfig(testData, nil, int64(network_map.DnsForwarderPort))
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| b.Run(fmt.Sprintf("WithoutCache-Size%d", size), func(b *testing.B) { | |
| b.ResetTimer() | |
| for i := 0; i < b.N; i++ { | |
| cache := &cache.DNSConfigCache{} | |
| toProtocolDNSConfig(testData, cache, int64(network_map.DnsForwarderPort)) | |
| networkmap.ToProtocolDNSConfig(testData, cache, int64(network_map.DnsForwarderPort)) | |
| b.Run(fmt.Sprintf("WithoutCache-Size%d", size), func(b *testing.B) { | |
| b.ResetTimer() | |
| for i := 0; i < b.N; i++ { | |
| networkmap.ToProtocolDNSConfig(testData, nil, int64(network_map.DnsForwarderPort)) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@management/internals/shared/grpc/conversion_test.go` around lines 110 - 114,
The “WithoutCache” benchmark is still exercising cache allocation and population
on every iteration instead of the true no-cache path. Update the benchmark case
in the `b.Run("WithoutCache...")` block that calls
`networkmap.ToProtocolDNSConfig` so it passes a nil cache rather than
constructing `cache.DNSConfigCache` inside the loop, keeping the benchmark
focused on the actual no-cache behavior.
Comment on lines
+110
to
+112
| for _, idx := range gc.PeerIndexes { | ||
| if int(idx) < len(peerIDByIndex) { | ||
| peerIDs = append(peerIDs, peerIDByIndex[idx]) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Skip unresolved peer indexes instead of emitting empty peer IDs.
Malformed peers are skipped by leaving peerIDByIndex[idx] == "", but later phases still append/store that empty ID when the index is in range. This can leak empty peer IDs into groups, posture failures, router maps, routes, and resources. Add a small resolver that checks both bounds and non-empty IDs before use.
Suggested direction
+func peerIDAt(peerIDByIndex []string, idx uint32) (string, bool) {
+ if uint64(idx) >= uint64(len(peerIDByIndex)) {
+ return "", false
+ }
+ peerID := peerIDByIndex[int(idx)]
+ return peerID, peerID != ""
+}Also applies to: 169-174, 220-223, 235-238, 362-364, 408-410
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shared/management/networkmap/decode.go` around lines 110 - 112, The peer
index handling in decode.go is appending empty peer IDs whenever
`peerIDByIndex[idx]` was never resolved, so update the lookup logic used by
`GroupCodec.Decode`, `PeerIndexes`, and the later map/resource builders to skip
both out-of-bounds indexes and empty IDs. Add a small shared resolver/helper
around `peerIDByIndex` access, then use it at the affected append sites so
groups, posture failures, router maps, routes, and resources never store
unresolved peer IDs.
Comment on lines
+51
to
+54
| localPeer := components.Peers[canonicalKey] | ||
| if localPeer == nil { | ||
| return nil, fmt.Errorf("receiving peer (wg_key prefix %q) not found among %d decoded peers — components have no PeerID, Calculate would return empty", trimKey(localPeerKey), len(components.Peers)) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle minimal envelopes before requiring a local peer.
The server encoder can intentionally send a Full envelope with no components for missing/unvalidated peers, but this path returns an error when components.Peers is empty. That makes the graceful-degrade envelope fail sync instead of yielding the minimal NetworkMap.
Return a minimal proto.NetworkMap from full.PeerConfig/DNS/proxy fields when the decoded component set has no peers, and reserve this error for non-empty snapshots that genuinely omit the receiver.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shared/management/networkmap/envelope.go` around lines 51 - 54, Handle the
empty decoded peer set in the envelope decoding flow before looking up localPeer
in the NetworkMap/Envelope handling path. Update the logic around
components.Peers and localPeer so that when no peers are present,
full.PeerConfig still returns a minimal proto.NetworkMap populated from the
envelope’s DNS/proxy fields instead of erroring, and keep the current error only
for non-empty snapshots where the receiver is genuinely missing.
Comment on lines
+1160
to
+1170
| // NetworkResourceRaw mirrors *resourceTypes.NetworkResource. | ||
| // | ||
| // INCOMPATIBLE WIRE CHANGE: field 2 changed from `string network_id` (xid) | ||
| // to `uint32 network_seq` without a `reserved` entry. Safe only because | ||
| // capability=3 has never been released — every cap=3 producer and consumer | ||
| // carries the same regenerated descriptor. Do NOT reuse this pattern once | ||
| // cap=3 ships. | ||
| message NetworkResourceRaw { | ||
| string id = 1; // network_resources.account_seq_id | ||
| string network_seq = 2; // networks.account_seq_id (replaces xid) | ||
| string name = 3; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Correct the stale wire-type comment.
The comment says field 2 changed to uint32 network_seq, but the schema defines string network_seq = 2. This is a generated contract comment, so it should match the actual wire type.
Suggested fix
-// INCOMPATIBLE WIRE CHANGE: field 2 changed from `string network_id` (xid)
-// to `uint32 network_seq` without a `reserved` entry. Safe only because
+// INCOMPATIBLE SEMANTIC CHANGE: field 2 changed from `string network_id` (xid)
+// to `string network_seq` without a `reserved` entry. Safe only because📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // NetworkResourceRaw mirrors *resourceTypes.NetworkResource. | |
| // | |
| // INCOMPATIBLE WIRE CHANGE: field 2 changed from `string network_id` (xid) | |
| // to `uint32 network_seq` without a `reserved` entry. Safe only because | |
| // capability=3 has never been released — every cap=3 producer and consumer | |
| // carries the same regenerated descriptor. Do NOT reuse this pattern once | |
| // cap=3 ships. | |
| message NetworkResourceRaw { | |
| string id = 1; // network_resources.account_seq_id | |
| string network_seq = 2; // networks.account_seq_id (replaces xid) | |
| string name = 3; | |
| // NetworkResourceRaw mirrors *resourceTypes.NetworkResource. | |
| // | |
| // INCOMPATIBLE SEMANTIC CHANGE: field 2 changed from `string network_id` (xid) | |
| // to `string network_seq` without a `reserved` entry. Safe only because | |
| // capability=3 has never been released — every cap=3 producer and consumer | |
| // carries the same regenerated descriptor. Do NOT reuse this pattern once | |
| // cap=3 ships. | |
| message NetworkResourceRaw { | |
| string id = 1; // network_resources.account_seq_id | |
| string network_seq = 2; // networks.account_seq_id (replaces xid) | |
| string name = 3; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shared/management/proto/management.proto` around lines 1160 - 1170, The
wire-type comment on NetworkResourceRaw is stale and does not match the actual
schema. Update the generated contract comment above the NetworkResourceRaw
message so it describes field 2 as string network_seq, keeping the rest of the
compatibility note intact and aligned with the current proto definition.
Comment on lines
+113
to
+116
| func peerSupportedFirewallFeatures(peerVer string) supportedFeatures { | ||
| if strings.Contains(peerVer, "dev") { | ||
| return supportedFeatures{true, true} | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the version package and check for potential circular imports
fd -t f "version" --max-depth 2 | head -20
rg -n "func IsDevelopmentVersion" --type=go | head -5
# Check if shared/management/types already imports anything that would cycle with version
rg -n "github.com/netbirdio/netbird/version" shared/management/types/ | head -10Repository: netbirdio/netbird
Length of output: 215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== version/version.go ==\n'
cat -n version/version.go | sed -n '1,220p'
printf '\n== shared/management/types/firewall_helpers.go ==\n'
cat -n shared/management/types/firewall_helpers.go | sed -n '1,220p'
printf '\n== imports of shared/management/types ==\n'
rg -n '^import \(' -A20 shared/management/types/firewall_helpers.go
printf '\n== IsDevelopmentVersion references ==\n'
rg -n 'IsDevelopmentVersion|development' version shared/management/types -g '*.go'Repository: netbirdio/netbird
Length of output: 9594
Use version.IsDevelopmentVersion() here. It avoids matching arbitrary substrings like 0.50.0-dev and keeps dev/CI build handling consistent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shared/management/types/firewall_helpers.go` around lines 113 - 116, Replace
the string-substring check in peerSupportedFirewallFeatures with
version.IsDevelopmentVersion() so development builds are detected consistently
instead of matching arbitrary “dev” substrings. Update the peerVer handling in
that function to use the version helper and keep the existing supportedFeatures
return behavior unchanged for development versions.
Source: Learnings
Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io>
Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io>
Signed-off-by: Dmitri Dolguikh <dmitri.external@netbird.io>
|
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
3 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
PublicIDgeneration for multiple management objects (accounts, groups, networks, routers, resources, routes, policies, DNS name server groups, and posture checks).PublicIDacross create/update flows and tightened database upsert behavior to avoid overwriting it.