[management] Add custom dns zones #4849
Conversation
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
WalkthroughAdds DNS zones and DNS records: new domain/record models, CRUD HTTP APIs and REST client, store persistence, managers with permissions/validation/events, integration into network-map/account flows to apply per-account zones and peer filtering, plus tests and OpenAPI additions. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Server as HTTP API
participant ZonesManager
participant Store
participant AccountMgr as AccountManager
Note over Client,API_Server: Create DNS Zone request
Client->>API_Server: POST /api/dns/zones (ZoneRequest)
API_Server->>ZonesManager: CreateZone(ctx, accountID, userID, Zone)
ZonesManager->>Store: Begin Transaction
ZonesManager->>Store: GetZoneByDomain(lock) -> maybe NotFound
ZonesManager->>Store: CreateZone(zone)
ZonesManager->>Store: Commit Transaction
ZonesManager->>AccountMgr: StoreEvent(DNSZoneCreated)
ZonesManager->>AccountMgr: Trigger background peer updates (async)
AccountMgr-->>ZonesManager: ack
ZonesManager-->>API_Server: Zone (created)
API_Server-->>Client: 201 Created (Zone)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ 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 |
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/server/types/account.go (1)
269-279: Based on my thorough verification of the codebase, I can now provide the final assessment:Both issues in the review comment are confirmed and valid
The code inspection reveals:
Flag dropping is confirmed: Both
Account.GetPeerNetworkMapandNetworkMapBuilder.assembleNetworkMapcreate a newCustomZonewith onlyDomainandRecordsfields:customZones[i] = nbdns.CustomZone{ Domain: customZone.Domain, Records: records, }This leaves
SearchDomainDisabledandSkipPTRProcesszero-initialized (false), dropping the original values.In-place mutation is confirmed: The input
customZonesslice is modified directly in-place. While current call patterns typically pass fresh or empty slices, the function signature permits slice reuse, and any such reuse would result in cumulative filtering across multiple peers.The
CustomZonestruct indns/dns.godefinitively has all four fields (Domain, Records, SearchDomainDisabled, SkipPTRProcess), confirming the impact of this code pattern.
Preserve
CustomZoneflags and avoid surprising slice mutation inGetPeerNetworkMapTwo issues in this block:
Flags are dropped for the account DNS zone
When a zone matches
dns.Fqdn(dnsDomain), the code replaces the entire element:customZones[i] = nbdns.CustomZone{ Domain: customZone.Domain, Records: records, }This zeroes
SearchDomainDisabledandSkipPTRProcess, changing resolver behavior for that zone.At minimum, keep the original flags:
records := filterZoneRecordsForPeers(peer, customZone, peersToConnectIncludingRouters, expiredPeers)customZones[i] = nbdns.CustomZone{Domain: customZone.Domain,Records: records,}
records := filterZoneRecordsForPeers(peer, customZone, peersToConnectIncludingRouters, expiredPeers)customZones[i].Records = records
In‑place mutation can corrupt subsequent calls
GetPeerNetworkMapmutates the passedcustomZonesslice. If callers reuse a single slice for multiple peers, records are progressively intersected (peer 2 sees only a subset derived from peer 1's view).Consider copying before filtering to make the function side‑effect‑free for callers:
- if dnsManagementStatus {
for i, customZone := range customZones {
- if dnsManagementStatus {
filteredZones := slices.Clone(customZones)for i, customZone := range filteredZones { if customZone.Domain == dns.Fqdn(dnsDomain) {
records := filterZoneRecordsForPeers(peer, customZone, peersToConnectIncludingRouters, expiredPeers)customZones[i].Records = records
filteredZones[i].Records = filterZoneRecordsForPeers(peer, customZone, peersToConnectIncludingRouters, expiredPeers) }
}dnsUpdate.CustomZones = customZones
}dnsUpdate.CustomZones = filteredZonesAlso applies to:
NetworkMapBuilder.assembleNetworkMap
🧹 Nitpick comments (8)
management/server/util/util.go (1)
3-6: Domain validation helper is fine; just confirm label policy
IsValidDomainand the compileddomainRegexgive a clear, centralized domain check and cover common cases (including*.example.com).Be aware of two behavioral details:
- Labels with underscores (e.g.
_acme-challenge.example.com) are rejected.- Labels starting/ending with
-are accepted, even though they’re not valid hostnames per RFCs.If that matches your product’s intended constraints, this is good to go; otherwise you may want to tighten/relax the regex accordingly.
Also applies to: 58-63
management/server/http/handlers/peers/peers_handler.go (1)
13-14: Avoid unnecessarydns.CustomZonedependency in handler
GetAccessiblePeersdoesn’t consumeCustomZonesfrom the returned network map, so you can avoid importinggithub.com/netbirdio/netbird/dnsand allocating an empty slice. Passingnilis simpler and keeps DNS concerns encapsulated deeper in the stack.-import ( +import ( ... - "github.com/netbirdio/netbird/dns" ... ) ... - netMap := account.GetPeerNetworkMap(r.Context(), peerID, dnsDomain, []dns.CustomZone{}, validPeers, account.GetResourcePoliciesMap(), account.GetResourceRoutersMap(), nil) + netMap := account.GetPeerNetworkMap(r.Context(), peerID, dnsDomain, nil, validPeers, account.GetResourcePoliciesMap(), account.GetResourceRoutersMap(), nil)Also applies to: 324-327
shared/management/http/api/openapi.yml (2)
1765-1786: Clarify whether DNS recordnameis FQDN or zone‑relative
DNSRecordRequest.nameis documented with example"www", which suggests a label relative to the zone:DNSRecordRequest: properties: name: description: DNS record name example: wwwHowever, the server‑side validation in
management/internals/modules/zones/records/manager/manager.gocurrently requiresrecord.NametoHasSuffix(zone.Domain), and the integration test creates records with full FQDNs like"api.test.example.com"for a zone domain"test.example.com".This mismatch can confuse API consumers.
Suggestion
Either:
- Keep the current server behavior (full FQDN) and update the description/example to something like
"www.example.com"/"api.test.example.com", or- Change server validation to accept zone‑relative labels and append the zone domain internally.
1816-1834: Extendactivity_codeenum with new DNS zone/record activitiesThe
Event.activity_codeenum doesn’t list the new DNS activities (dns.zone.create/update/delete,dns.zone.record.create/update/delete) you introduced inmanagement/server/activity/codes.go. As a result, documented values are out of sync with what the backend can emit.Consider appending these new codes to the enum so API clients see the full set of possible
activity_codevalues.management/internals/modules/zones/interface.go (1)
7-12: Zone Manager interface is fine; consider tighteningGetZonenamingThe CRUD interface here is consistent with the
Zonemodel and store APIs. For readability and consistency withDeleteZone(..., zoneID string), consider renaming the last parameter ofGetZonefromzonetozoneIDin implementations and usages.management/internals/modules/zones/zone.go (1)
60-77: Consider adding a maximum length check for the domain field.While
util.IsValidDomainvalidates the format, DNS domains have a maximum length of 253 characters. Consider adding a length check similar to the name field validation for defense in depth.if !util.IsValidDomain(z.Domain) { return errors.New("invalid zone domain format") } + + if len(z.Domain) > 253 { + return errors.New("zone domain exceeds maximum length of 253 characters") + }management/server/types/networkmapbuilder.go (1)
13-13: Normalize domains consistently and avoid dropping CustomZone fieldsThe new DNS zone filtering logic is sound, but two details are worth tightening up:
The comparison
customZone.Domain == dns.Fqdn(dnsDomain)will only match ifcustomZone.Domainis already in fully‑qualified form (trailing dot). IfZone.Domainis stored without a trailing dot, this will silently skip zones. Consider normalizing both sides (e.g.,dns.Fqdnon both or a shared helper) to avoid mismatches.When you rebuild a matching zone as
nbdns.CustomZone{Domain: customZone.Domain, Records: records}, any other fields onnbdns.CustomZoneare zeroed out. To future‑proof this, it’s safer to copy the existing struct and only overrideRecords(e.g.,z := customZone; z.Records = records; customZones[i] = z).Also applies to: 939-942, 978-982, 1028-1040
management/server/store/sql_store.go (1)
113-119: Zone/DNS record store methods mostly align; consider association scope and error detailThe new zone/DNS‑record CRUD methods look consistent with existing store patterns, but a couple of refinements would make them safer and easier to debug:
UpdateZone(and to a lesser extentUpdateDNSRecord) usesSelect("*").Save(zone). Givenzones.ZonedeclaresRecords []*records.Record \gorm:"foreignKey:ZoneID;references:ID"`inmanagement/internals/modules/zones/zone.golines 12–21, this can implicitly upsert associated records when callers only intend to update zone metadata. If records are supposed to be managed exclusively via the DNS record methods, it’s safer toOmit(clause.Associations)or explicitly select just the zone fields to avoid accidental writes toRecords`.The new log messages are quite generic (e.g.,
"failed to create to store"/"failed to delete dns record from store"). Including the relevant IDs (accountID,zoneID,recordID) in the log and/or wrapping error text will make production debugging a lot easier.These are behavioural/observability tweaks; the core implementation and account scoping look correct.
Also applies to: 4131-4294
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
management/internals/controllers/network_map/controller/controller.go(13 hunks)management/internals/controllers/network_map/controller/repository.go(3 hunks)management/internals/modules/zones/interface.go(1 hunks)management/internals/modules/zones/manager/api.go(1 hunks)management/internals/modules/zones/manager/manager.go(1 hunks)management/internals/modules/zones/records/interface.go(1 hunks)management/internals/modules/zones/records/manager/api.go(1 hunks)management/internals/modules/zones/records/manager/manager.go(1 hunks)management/internals/modules/zones/records/record.go(1 hunks)management/internals/modules/zones/zone.go(1 hunks)management/internals/server/boot.go(1 hunks)management/internals/server/modules.go(2 hunks)management/internals/shared/grpc/conversion.go(1 hunks)management/server/account_test.go(1 hunks)management/server/activity/codes.go(2 hunks)management/server/http/handler.go(3 hunks)management/server/http/handlers/peers/peers_handler.go(2 hunks)management/server/http/testing/testing_tools/channel/channel.go(2 hunks)management/server/store/sql_store.go(3 hunks)management/server/store/store.go(2 hunks)management/server/types/account.go(2 hunks)management/server/types/networkmap.go(1 hunks)management/server/types/networkmap_golden_test.go(18 hunks)management/server/types/networkmapbuilder.go(5 hunks)management/server/util/util.go(2 hunks)shared/management/client/rest/client.go(2 hunks)shared/management/client/rest/dns_zones.go(1 hunks)shared/management/client/rest/dns_zones_test.go(1 hunks)shared/management/http/api/openapi.yml(3 hunks)shared/management/http/api/types.gen.go(4 hunks)shared/management/status/error.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (17)
shared/management/client/rest/client.go (2)
shared/management/client/rest/dns.go (1)
DNSAPI(12-14)shared/management/client/rest/dns_zones.go (1)
DNSZonesAPI(12-14)
management/internals/modules/zones/records/interface.go (1)
management/internals/modules/zones/records/record.go (1)
Record(21-29)
management/internals/modules/zones/interface.go (2)
management/internals/modules/zones/zone.go (1)
Zone(13-22)shared/management/http/api/types.gen.go (1)
Zone(1910-1928)
management/server/http/handler.go (4)
management/internals/modules/zones/records/interface.go (1)
Manager(7-13)management/internals/modules/zones/interface.go (1)
Manager(7-13)management/internals/modules/zones/manager/api.go (1)
RegisterEndpoints(20-30)management/internals/modules/zones/records/manager/api.go (1)
RegisterEndpoints(20-30)
management/internals/shared/grpc/conversion.go (1)
shared/management/proto/management.pb.go (3)
SimpleRecord(2752-2762)SimpleRecord(2777-2777)SimpleRecord(2792-2794)
management/server/types/networkmap.go (2)
management/server/telemetry/accountmanager_metrics.go (1)
AccountManagerMetrics(11-17)management/server/types/networkmapbuilder.go (1)
NetworkMapCache(33-59)
management/internals/modules/zones/records/manager/manager.go (9)
management/server/store/store.go (1)
Store(52-222)management/internals/modules/zones/records/interface.go (1)
Manager(7-13)management/internals/modules/zones/manager/manager.go (1)
NewManager(23-29)management/internals/modules/zones/records/record.go (3)
Record(21-29)NewRecord(31-41)RecordTypeCNAME(18-18)management/server/permissions/modules/module.go (1)
Dns(11-11)management/server/permissions/operations/operation.go (2)
Read(7-7)Delete(9-9)shared/management/status/error.go (6)
NewPermissionValidationError(213-215)NewPermissionDeniedError(209-211)Type(46-46)Errorf(70-75)InvalidArgument(27-27)AlreadyExists(30-30)management/internals/modules/zones/zone.go (1)
Zone(13-22)route/route.go (1)
ID(48-48)
management/server/types/account.go (2)
dns/dns.go (1)
CustomZone(43-52)shared/management/proto/management.pb.go (3)
CustomZone(2680-2689)CustomZone(2704-2704)CustomZone(2719-2721)
management/internals/server/boot.go (1)
management/server/http/handler.go (1)
NewAPIHandler(57-145)
shared/management/client/rest/dns_zones_test.go (4)
shared/management/http/api/types.gen.go (8)
DNSRecord(418-433)DNSRecordTypeA(17-17)PostApiDnsZonesJSONRequestBody(2027-2027)PutApiDnsZonesZoneIdJSONRequestBody(2030-2030)PostApiDnsZonesZoneIdRecordsJSONRequestBody(2033-2033)PutApiDnsZonesZoneIdRecordsRecordIdJSONRequestBody(2036-2036)ZoneRequest(1931-1946)DNSRecordRequest(436-448)shared/management/http/util/util.go (1)
ErrorResponse(21-24)management/server/activity/codes.go (1)
Code(9-12)shared/management/status/error.go (1)
Error(54-57)
management/internals/controllers/network_map/controller/repository.go (1)
management/server/store/store.go (1)
LockingStrengthNone(49-49)
management/internals/modules/zones/records/record.go (2)
shared/management/http/api/types.gen.go (3)
DNSRecord(418-433)DNSRecordType(451-451)DNSRecordRequest(436-448)management/server/util/util.go (1)
IsValidDomain(58-63)
management/internals/controllers/network_map/controller/controller.go (4)
management/internals/controllers/network_map/interface.go (1)
Controller(23-39)management/server/types/account.go (1)
LookupMap(57-57)management/internals/modules/zones/records/record.go (3)
RecordTypeA(16-16)RecordTypeAAAA(17-17)RecordTypeCNAME(18-18)dns/dns.go (1)
DefaultClass(21-21)
management/server/store/store.go (3)
management/internals/modules/zones/zone.go (1)
Zone(13-22)shared/management/http/api/types.gen.go (1)
Zone(1910-1928)management/internals/modules/zones/records/record.go (1)
Record(21-29)
management/server/account_test.go (2)
dns/dns.go (1)
CustomZone(43-52)shared/management/proto/management.pb.go (3)
CustomZone(2680-2689)CustomZone(2704-2704)CustomZone(2719-2721)
management/server/store/sql_store.go (4)
management/internals/modules/zones/zone.go (1)
Zone(13-22)management/internals/modules/zones/records/record.go (1)
Record(21-29)shared/management/status/error.go (4)
Error(54-57)Errorf(70-75)NewZoneNotFoundError(257-259)NewDNSRecordNotFoundError(262-264)management/server/store/store.go (2)
LockingStrength(42-42)LockingStrengthNone(49-49)
management/internals/modules/zones/zone.go (3)
management/internals/modules/zones/records/record.go (1)
Record(21-29)shared/management/http/api/types.gen.go (2)
Zone(1910-1928)ZoneRequest(1931-1946)management/server/util/util.go (1)
IsValidDomain(58-63)
🔇 Additional comments (47)
management/internals/modules/zones/manager/api.go (1)
16-160: Zones HTTP handlers follow existing patterns cleanlyAuth extraction, validation, manager delegation, and JSON responses look consistent and correct; no issues spotted in routing or parameter handling.
management/internals/shared/grpc/conversion.go (1)
329-345: gRPC custom zone conversion correctly propagatesSearchDomainDisabledThe added
SearchDomainDisabledfield is wired through alongsideDomainand preallocatedRecords; conversion logic remains consistent.management/server/http/testing/testing_tools/channel/channel.go (1)
12-15: Test HTTP channel correctly wires zones and records managersUsing real
zonesManager.NewManagerandrecordsManager.NewManagerwith the test store and passing them intoNewAPIHandlerkeeps the black-box tests exercising the new DNS zones/records API surface as expected.Also applies to: 92-101
management/internals/server/modules.go (1)
9-12: Zones/records manager accessors align with existing server module patterns
ZonesManager()andRecordsManager()follow the same lazy-initialization pattern as other managers and use the expected dependencies (Store,AccountManager,PermissionsManager), so the wiring looks correct.Also applies to: 123-133
management/server/types/networkmap.go (1)
24-34: Network map API cleanly extended for multi‑zone DNSExtending
GetPeerNetworkMapExpwithdnsDomainandcustomZonesand forwarding them toNetworkMapCache.GetPeerNetworkMapis a minimal, coherent change; cache initialization semantics remain unchanged.shared/management/client/rest/client.go (1)
60-67: REST client DNSZones API wiring is consistent and completeAdding the
DNSZonesfield and initializing it ininitialize()mirrors the existingDNSAPI pattern and should make the new endpoints immediately usable via the client.Also applies to: 107-121
shared/management/status/error.go (1)
256-264: LGTM!The new error constructors follow the established pattern in this file and are consistent with other
NotFounderror types.management/server/http/handler.go (3)
17-20: LGTM!Import aliases are appropriately named to avoid conflicts with the interface types.
72-73: LGTM!New manager parameters follow the established injection pattern.
141-142: LGTM!Endpoint registration follows the established pattern and maintains logical grouping with DNS-related handlers.
management/internals/controllers/network_map/controller/repository.go (1)
6-6: LGTM!The new
GetAccountZonesmethod follows the established repository pattern, usingLockingStrengthNoneconsistent with other read methods in this file.Also applies to: 16-16, 42-45
shared/management/http/api/types.gen.go (1)
1-4: Generated code - no manual changes required.This file is auto-generated by
oapi-codegen. The new DNS zone and record types appear correctly structured with appropriate field types and optionality for request/response patterns.management/server/account_test.go (1)
397-399: Test correctly adapts to updatedGetPeerNetworkMapsignature with domain and zones slice parameters.The test at lines 397-399 properly implements the new signature:
- Passes
"netbird.io"as thednsDomainstring parameter- Wraps
customZonein[]nbdns.CustomZone{customZone}slice to support multiple zones- All other parameters (context, peerID, validatedPeers, policies, routers, metrics) match the function signature
The implementation aligns with the underlying function that iterates over the zones slice to filter records per domain.
management/internals/server/boot.go (1)
96-96: Perfect! All verifications are complete. The signature shows:
- Parameter 15:
zManager zones.Manager- Parameter 16:
rManager records.Manager- Parameter 17:
networkMapController network_map.ControllerThese match the
ZonesManager()andRecordsManager()methods that returnzones.Managerandrecords.Managerrespectively. The code change is correctly implemented.
LGTM!
Both
ZonesManager()andRecordsManager()methods exist onBaseServerand return the correct types (zones.Managerandrecords.Manager). TheNewAPIHandlersignature correctly accepts these parameters at positions 15-16, and they are properly passed from the call site.management/server/activity/codes.go (1)
183-189: DNS zone/record activity codes and mappings look consistentNew DNS activity constants and their
activityMapentries follow the existing naming and coding conventions; no functional issues spotted here.Also applies to: 300-306
management/internals/modules/zones/records/interface.go (1)
7-13: Records Manager interface shape is reasonableThe CRUD interface is coherent (context-first, explicit account/user/zone IDs) and matches the underlying
Recordmodel and store APIs.management/server/types/account.go (1)
1755-1778: Per‑peer DNS record filtering helper looks correct
filterZoneRecordsForPeersbuilds a set of IPs for the target peer, its reachable peers, and expired peers, and filters zone records byRDatamembership. The logic is straightforward and matches the goal of scoping DNS responses to the peer’s connectivity set.shared/management/http/api/openapi.yml (1)
1710-1756: Zone schemas look consistent with generated types
ZoneRequest/Zoneschemas (fields, required sets, and examples) line up with the generatedapi.Zone/api.ZoneRequeststructs and the domain model (zones.Zone). The use ofEnabled *boolin the request and requiredenabledin the response is a sensible pattern for create/update semantics.management/internals/modules/zones/records/manager/manager.go (1)
33-55: Permissions + transactional flow for DNS records look solid
- Permission checks for read/create/update/delete use the DNS module and appropriate operations and map errors via
status.NewPermissionValidationError/NewPermissionDeniedError.- All mutating operations run inside
ExecuteInTransaction, lock the zone (LockingStrengthUpdate), update the record, and increment the network serial before emitting events.This pattern matches the rest of the codebase’s account/DNS workflows.
Also applies to: 101-205
shared/management/client/rest/dns_zones_test.go (3)
40-213: Mocked DNS zones client tests provide good coverageThe mocked tests for zone CRUD operations verify HTTP methods, request body shapes (via unmarshalling), and error handling for 4xx responses. This should catch most regressions in the REST client layer.
215-390: Mocked DNS record client tests are comprehensiveSimilarly, record CRUD tests validate correct endpoints, methods, and JSON payloads, plus propagate server error responses into Go errors, which is exactly what the client should be doing.
392-460: End‑to‑end integration test nicely validates the full DNS zones/records flow
TestDNSZones_Integrationwalks through create/list/get/update/delete for both zones and records against a black‑box server, asserting realistic values (full FQDN for record name, distribution groups, etc.). This is a valuable high‑level guardrail for the new feature.management/internals/modules/zones/records/manager/api.go (6)
16-30: LGTM!The handler struct and route registration are well-structured. The RESTful endpoints follow proper conventions for DNS record CRUD operations.
32-57: LGTM!The
getAllRecordshandler correctly validates authentication, zone ID, and properly converts internal records to API responses.
59-93: LGTM!The
createRecordhandler follows the standard pattern: decode request, convert to domain model, validate, and delegate to manager. The error handling is consistent.
95-121: LGTM!The
getRecordhandler correctly validates both zone and record IDs before delegating to the manager.
123-164: LGTM!The
updateRecordhandler correctly uses the path parameter for the record ID, ensuring the URL is the source of truth for the resource identifier.
166-191: LGTM!The
deleteRecordhandler follows proper REST conventions, returning an empty object on successful deletion.management/internals/modules/zones/zone.go (5)
13-22: LGTM!The
Zonestruct is well-defined with appropriate GORM tags. The JSON serializer forDistributionGroupsand the foreign key relationship forRecordsare correctly configured.
24-34: LGTM!The constructor properly initializes all fields and generates a unique ID using
xid.
36-45: LGTM!The API response mapping is complete and correctly maps all Zone fields to the API representation.
47-58: LGTM!The
FromAPIRequestmethod correctly handles the optionalEnabledfield by defaulting totruewhen not provided, which is a sensible default for new zones.
79-81: LGTM!The
EventMetamethod provides appropriate metadata for event tracking.management/internals/controllers/network_map/controller/controller.go (6)
174-178: LGTM!Proper error handling for zone retrieval with logging and early return on failure.
831-888: LGTM!The
filterPeerAppliedZonesfunction correctly:
- Filters zones by peer group membership
- Skips disabled zones and zones without records
- Maps record types to DNS type constants
- Logs warnings for unknown record types instead of failing
- Properly inverts
EnableSearchDomaintoSearchDomainDisabled
320-328: LGTM!The zone retrieval and filtering logic in
UpdateAccountPeeris consistent with the pattern used in other methods.
459-470: LGTM!Zone retrieval and filtering follows the established pattern in the controller.
789-800: LGTM!The zone handling in
GetNetworkMapis consistent with other controller methods.
500-518: LGTM!The signature update to accept
customZones []nbdns.CustomZoneproperly supports the multi-zone architecture.management/server/types/networkmap_golden_test.go (2)
107-108: LGTM!The test signature updates are consistent across all test functions and benchmarks.
72-72: Golden tests intentionally use empty zones; CustomZone functionality is tested separately.The golden tests pass empty
[]dns.CustomZone{}by design—they validate baseline network map structure rather than DNS zone filtering. Comprehensive CustomZone testing already exists inmanagement/server/account_test.go(line 398), whereGetPeersCustomZone()is called with actual zone data, and inmanagement/server/types/account_test.go(lines 1119–1209), which covers zone record filtering scenarios. No additional coverage is needed in these golden tests.management/internals/modules/zones/records/record.go (5)
13-29: LGTM!The
RecordTypeconstants andRecordstruct are well-defined with appropriate GORM tags for indexing and primary key.
31-41: LGTM!The constructor properly initializes all fields and generates a unique ID.
43-59: LGTM!The API conversion methods correctly map between internal and external record representations.
61-96: LGTM!The validation logic is comprehensive:
- Name format validation using domain regex
- Type-specific content validation (IPv4, IPv6, domain)
- Non-negative TTL check
- Clear error messages for each validation failure
98-107: LGTM!The
EventMetamethod appropriately includes both record details and zone context for comprehensive event tracking.management/server/store/store.go (1)
26-28: DNS zone and DNS record methods on Store look consistentThe new zone and DNS record operations on
Storefollow existing patterns (account‑scoped IDs, read methods withLockingStrength) and match theSqlStoreimplementations. No issues from the interface design side.Also applies to: 209-221
management/internals/controllers/network_map/controller/controller.go
Outdated
Show resolved
Hide resolved
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
management/internals/modules/zones/manager/manager.go (1)
167-172: Fix closure capture of loop variablerecord.This issue was flagged in a previous review and remains unfixed. The closures built inside the loop capture
recordby reference, so when they execute (lines 184-186), all of them will reference the last record from the loop. This causes incorrect event IDs and metadata for all deleted records except the last one.Apply this diff to capture the current record value in each iteration:
- for _, record := range records { - eventsToStore = append(eventsToStore, func() { - meta := record.EventMeta(zone.ID, zone.Name) - m.accountManager.StoreEvent(ctx, userID, record.ID, accountID, activity.DNSRecordDeleted, meta) - }) - } + for _, record := range records { + rec := record + eventsToStore = append(eventsToStore, func() { + meta := rec.EventMeta(zone.ID, zone.Name) + m.accountManager.StoreEvent(ctx, userID, rec.ID, accountID, activity.DNSRecordDeleted, meta) + }) + }
🧹 Nitpick comments (2)
management/server/store/sql_store_test.go (1)
3859-3879: Consider testing cascading delete behavior.While the current tests verify zone deletion works correctly, it would be helpful to optionally add a test that verifies whether DNS records are automatically deleted when their parent zone is deleted (if cascading delete is implemented), or if they need to be deleted separately.
Example test structure:
func TestSqlStore_DeleteZoneCascade(t *testing.T) { // Create zone // Create DNS records in zone // Delete zone // Verify records are also deleted (or handle accordingly) }management/internals/modules/zones/manager/manager.go (1)
188-188: Consider using a detached context for background peer updates.The goroutine receives the request context
ctx, which may be canceled when the HTTP request completes. IfUpdateAccountPeersis a long-running operation, it could be interrupted prematurely.If peer updates should complete regardless of client disconnection, consider using a detached context:
go m.accountManager.UpdateAccountPeers(context.Background(), accountID)However, if cancellation is intentional (e.g., to respect request timeouts), the current implementation is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
management/internals/controllers/network_map/controller/controller.go(13 hunks)management/internals/controllers/network_map/controller/controller_test.go(2 hunks)management/internals/modules/zones/manager/manager.go(1 hunks)management/internals/modules/zones/records/manager/manager.go(1 hunks)management/internals/modules/zones/records/record.go(1 hunks)management/server/store/sql_store_test.go(2 hunks)shared/management/http/api/openapi.yml(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
management/internals/modules/zones/records/record.go (2)
shared/management/http/api/types.gen.go (3)
DNSRecord(418-433)DNSRecordType(451-451)DNSRecordRequest(436-448)management/server/util/util.go (1)
IsValidDomain(58-63)
management/internals/modules/zones/records/manager/manager.go (8)
management/server/store/store.go (1)
Store(52-222)management/internals/modules/zones/records/interface.go (1)
Manager(7-13)management/internals/modules/zones/manager/manager.go (1)
NewManager(23-29)management/internals/modules/zones/records/record.go (3)
Record(21-29)NewRecord(31-41)RecordTypeCNAME(18-18)management/server/permissions/modules/module.go (1)
Dns(11-11)management/server/permissions/operations/operation.go (2)
Read(7-7)Delete(9-9)shared/management/status/error.go (4)
NewPermissionValidationError(213-215)NewPermissionDeniedError(209-211)Type(46-46)Errorf(70-75)management/internals/modules/zones/zone.go (1)
Zone(13-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: release_ui
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Build Cache
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
🔇 Additional comments (33)
management/internals/modules/zones/records/manager/manager.go (6)
19-31: LGTM!The manager implementation follows the established pattern with proper dependency injection of store, account manager, and permissions manager.
33-55: LGTM!Read operations properly implement permission checks and use appropriate locking strength for concurrent reads.
57-101: LGTM!The previous concern about missing peer updates has been resolved. The method now correctly triggers
UpdateAccountPeersat line 98, ensuring new DNS records are propagated to peers proactively, consistent with the update and delete paths.
103-160: LGTM!The update logic correctly distinguishes between changes that require conflict revalidation (Name, Type, Content) and those that don't (TTL). The implementation properly updates all fields while only triggering expensive conflict checks when necessary.
162-207: LGTM!Delete operation correctly implemented with proper locking, transactional safety, event logging, and peer updates.
209-236: LGTM!The zone membership validation has been corrected to properly check label boundaries (line 211), addressing the previous review concern. The implementation now correctly accepts only
example.comor*.example.comfor zoneexample.com, while rejectingnotexample.com.The duplicate and CNAME conflict checks are also correctly implemented.
shared/management/http/api/openapi.yml (3)
1710-1801: LGTM!The previous issue with
DNSRecordrequiring an undefinedenabledfield has been resolved. The schema now correctly requires onlyid(line 1800) and inherits the necessary fields fromDNSRecordRequest. All DNS Zone and Record schemas are well-defined and consistent with the backend implementation.
4592-4741: LGTM!All DNS Zone CRUD endpoints are properly defined with:
- Appropriate HTTP methods and paths
- Correct security requirements
- Proper request/response schemas
- Comprehensive error response codes
The API design follows RESTful conventions consistently.
4742-4932: LGTM!DNS Record endpoints are well-structured as nested resources under zones (
/api/dns/zones/{zoneId}/records). All endpoints properly:
- Define required path parameters (zoneId, recordId)
- Use correct request/response schemas
- Include appropriate error codes (notably 404 for resource-not-found scenarios)
- Follow consistent patterns with the parent zone endpoints
management/server/store/sql_store_test.go (3)
25-26: LGTM! New imports are correctly added for zone and DNS record testing.The imports properly reference the new internal modules for zones and DNS records that align with the test coverage being added.
3723-3879: Excellent test coverage for zone CRUD operations.The zone tests comprehensively cover creation, retrieval, update, and deletion with proper error handling and NotFound verification after deletion. The test structure follows existing patterns in the file and uses appropriate assertions.
3881-4126: Comprehensive DNS record test coverage with excellent structure.The DNS record tests thoroughly cover CRUD operations including:
- Creation with proper zone dependencies
- Retrieval by ID and by name
- Updates to all record fields
- Individual and bulk deletion
- Multiple record types (A, AAAA, CNAME)
All tests include proper error handling and NotFound verification.
management/internals/modules/zones/records/record.go (6)
13-29: Well-structured Record model with proper GORM configuration.The RecordType constants provide type safety for the three supported DNS record types, and the Record struct includes appropriate GORM tags for database indexing and primary key designation.
31-41: LGTM! Constructor properly initializes Record with unique ID.The use of
xidfor ID generation ensures uniqueness, and all fields are properly initialized.
43-59: API conversion methods correctly map between internal and external types.Both
ToAPIResponseandFromAPIRequestproperly convert between the internal Record model and the API types, with correct field mappings.
61-96: Comprehensive validation with appropriate type-specific checks.The validation logic properly enforces:
- Non-empty name with domain format validation
- Type-specific content validation (IPv4 for A, IPv6 for AAAA, domain for CNAME)
- Non-negative TTL
Note: The TTL validation allows 0, which is valid in DNS for indicating no caching should occur.
120-129: IPv6 validation fix has been correctly applied.The
validateIPv6function now properly rejects IPv4 addresses by checkingip.To4() != nil. This correctly addresses the critical bug from the previous review whereip.To16() == nilwould incorrectly accept IPv4 addresses (sinceTo16()returns a 16-byte IPv4-mapped representation for IPv4 addresses).The current implementation ensures only true IPv6 addresses are accepted for AAAA records.
98-118: EventMeta and validateIPv4 are correctly implemented.The
EventMetamethod provides comprehensive metadata for event tracking, andvalidateIPv4properly validates IPv4 addresses using the correctip.To4() == nilcheck.management/internals/controllers/network_map/controller/controller_test.go (2)
4-17: LGTM!The new imports are appropriate for the test function and support comprehensive DNS zone testing.
120-630: Excellent test coverage!The table-driven test comprehensively validates
filterPeerAppliedZonesacross multiple scenarios:
- Access control via distribution groups
- Zone enablement filtering
- Empty record handling
- Multiple record types (A, AAAA, CNAME)
- Search domain configuration
- Multi-zone and multi-group scenarios
The assertions thoroughly verify both zone-level properties and individual record details.
management/internals/controllers/network_map/controller/controller.go (12)
15-15: LGTM!The new imports support DNS zone filtering and record type mapping in the
filterPeerAppliedZonesfunction.Also applies to: 23-24
174-178: LGTM!Proper error handling for account zones retrieval with logging and early return.
203-205: LGTM! Bug from previous review has been fixed.The code correctly uses
p.ID(goroutine parameter) instead of the loop variable, avoiding the closure capture issue flagged in the previous review. The customZones construction properly filters zones by peer group membership and appends the peers custom zone.
210-213: LGTM!Both experimental and non-experimental network map generation paths correctly updated to accept the
customZonesslice parameter.
320-327: LGTM!Consistent zone retrieval and filtering pattern applied correctly.
332-335: LGTM!Function calls correctly updated to pass
customZonesparameter in both code paths.
459-469: LGTM!Proper sequence: retrieve account zones, get DNS domain and peers custom zone, then filter zones based on peer groups.
480-483: LGTM!Function calls consistently updated with
customZonesparameter.
500-518: LGTM!The
getPeerNetworkMapExpfunction signature correctly updated to acceptcustomZonesslice, and properly retrieves DNS domain from account settings before passing to the network map builder.
790-799: LGTM!Consistent zone retrieval and filtering pattern properly implemented in
GetNetworkMap.
810-813: LGTM!Network map generation calls correctly updated with
customZonesparameter.
831-888: Well-implemented helper function!The
filterPeerAppliedZonesfunction correctly:
- Filters zones based on peer group access control
- Skips disabled zones and zones without records
- Maps DNS record types (A, AAAA, CNAME) with proper fallback for unknown types
- Constructs properly formatted FQDNs using
dns.Fqdn()- Inverts
EnableSearchDomaintoSearchDomainDisabledcorrectlyThe early return optimization and access control logic are both sound.
management/internals/modules/zones/manager/manager.go (1)
88-129: Verify if peer updates and serial increment are needed after zone updates.
UpdateZonecorrectly handles the transaction error (lines 122-124, resolving the previous review concern), but likeCreateZone, it does not callIncrementNetworkSerialorUpdateAccountPeers. Changes to zone properties (enabled status, distribution groups, domain, search domain settings) likely affect the network map and should trigger peer updates.Use the verification script from the
CreateZonecomment above to check if Update operations typically call these methods.
…ne management Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
shared/management/http/api/openapi.yml (3)
4594-4743: /api/dns/zones collection and item endpoints match existing REST patterns
- List/create (
GET/POST) endpoints for zones reuse the standard security blocks and response code conventions (200 + 400/401/403/500 on write).- Single-zone
GET/PUT/DELETEcorrectly usezoneIdas a path param with type string and add404for missing zones, which is actually stricter than some of the older DNS endpoints.One minor consistency nit: existing
DELETEendpoints often include an explicit emptycontent: { }block on 200, whereasDELETE /api/dns/zones/{zoneId}omitscontent. Both are valid OpenAPI, but you may want to align with the existing deletes for uniformity.
4744-4814: /api/dns/zones/{zoneId}/records collection endpoints are well-shaped
GETreturnsDNSRecord[]and includes404when the parent zone is missing.POSTacceptsDNSRecordRequestand returns a fullDNSRecord, consistent with other create endpoints returning the created resource.Again, optional nit: you don’t expose a
400for theGETdespite other list endpoints sometimes defining it even without query params; that’s fine semantically, but if you aim for strict uniformity you might consider adding it later.
4816-4934: Record-level endpoints look correct; consider aligning delete response shape
GET/PUTfor/api/dns/zones/{zoneId}/records/{recordId}use bothzoneIdandrecordIdwith clear descriptions and return a singleDNSRecord, including404for not found.PUTreusesDNSRecordRequestfor the body, which matches the Go types and keeps id server-controlled.For
DELETE, you return a bare 200 with description only, whereas most existing deletes specifycontent: { }. It’s valid as-is, but you might want to add the emptycontentblock for consistency with the rest of the API surface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
management/internals/modules/zones/zone.go(1 hunks)management/server/store/sql_store.go(3 hunks)shared/management/http/api/openapi.yml(3 hunks)shared/management/http/api/types.gen.go(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
📚 Learning: 2025-11-28T12:20:47.254Z
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
Applied to files:
shared/management/http/api/openapi.ymlshared/management/http/api/types.gen.gomanagement/server/store/sql_store.gomanagement/internals/modules/zones/zone.go
🧬 Code graph analysis (1)
management/internals/modules/zones/zone.go (3)
management/internals/modules/zones/records/record.go (1)
Record(21-29)shared/management/http/api/types.gen.go (3)
Zone(1910-1931)DNSRecord(418-433)ZoneRequest(1934-1949)management/server/util/util.go (1)
IsValidDomain(58-63)
🔇 Additional comments (12)
management/internals/modules/zones/zone.go (1)
13-89: LGTM! Clean zone model implementation.The Zone type is well-structured with appropriate GORM tags, proper foreign key relationships, sensible validation rules, and clean API conversion logic. The default handling for the optional
Enabledfield (defaulting totruewhennil) aligns with typical feature flag patterns.management/server/store/sql_store.go (4)
30-31: LGTM! Proper integration into auto-migration.The imports and auto-migration additions correctly integrate the new zones and records models into the store initialization, ensuring database schema readiness.
Also applies to: 118-118
4141-4149: LGTM!The use of
Select("*").Save()for full updates is consistent with other entity update patterns in this store (e.g.,SavePolicy).
4151-4199: LGTM! Standard CRUD patterns.The zone retrieval and deletion methods follow established store conventions with proper locking support, association preloading, and error handling.
4235-4295: LGTM! DNS record retrieval methods are properly scoped.The
GetDNSRecordByID,GetZoneDNSRecords,GetZoneDNSRecordsByName, andDeleteZoneDNSRecordsmethods all correctly scope operations by bothaccountIDandzoneID, ensuring proper data isolation.shared/management/http/api/types.gen.go (5)
15-20: DNSRecordType constants correctly mirror OpenAPI enumThe A/AAAA/CNAME constants align with the
DNSRecordTypestring enum in the OpenAPI spec; nothing to change here.
417-452: DNSRecord / DNSRecordRequest structs match schema expectations
DNSRecordRequestandDNSRecordfields (name, type, content, ttl, id) line up with the OpenAPI definitions and required sets; using a separate request type withoutidis consistent with other resources in this API.
450-451: String alias for DNSRecordType is appropriateDefining
DNSRecordTypeasstringkeeps the generated client simple while still being constrained by the enum constants above; this matches existing enum patterns in this file.
1909-1949: Zone / ZoneRequest shapes are consistent but rely on backend defaults forenabled
ZoneRequest.Enabledas*bool(omittable) vsZone.Enabledasboolmatches the OpenAPI idea ofenabledbeing optional on write with a default oftrue, and required on read.- This puts the onus on handlers to interpret
nilas “use default” and not asfalse.Please double-check the zone create/update handlers treat
nilEnabledcorrectly (defaulting totrueon create and/or leaving it unchanged on update) so behavior matches the OpenAPI default.
2029-2040: Request body aliases for DNS zones/records follow existing patternsThe new
PostApiDnsZones*/PutApiDnsZones*and records request-body aliases correctly point toZoneRequest/DNSRecordRequest, matching the OpenAPI requestBody schemas and the conventions used elsewhere in this generated file.shared/management/http/api/openapi.yml (2)
28-29: New “DNS Zones” tag integrates cleanly into tag listTag naming and description match the style of existing tags and will group the new endpoints coherently in generated docs.
1710-1803: Zone/DNSRecord schemas are structurally sound and consistent**
ZoneRequestandZonefollow the existing pattern of *Request vs full resource (allOfwith anid-carrying object), similar to networks/resources/routes.enabledbeing optional onZoneRequest(with a default) and required viaZone’srequiredarray is a validallOfusage and matches the generated Go types.DNSRecordTypeenum andDNSRecordRequest/DNSRecordshapes line up with the Go types (A/AAAA/CNAME;name,type,content,ttl, plusidon the read model).No inconsistencies spotted between these schemas and the generated
apipackage.
# Conflicts: # management/internals/controllers/network_map/controller/controller.go # management/internals/controllers/network_map/controller/repository.go # management/internals/server/modules.go # management/server/activity/codes.go
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
management/server/account_test.go (1)
399-401: Avoid hard‑coded DNS domain in test; useaccount.DomainThe new
GetPeerNetworkMapsignature usage looks correct, but you can tighten the test by deriving the domain from the account instead of repeating the literal:- customZone := account.GetPeersCustomZone(context.Background(), "netbird.io") - networkMap := account.GetPeerNetworkMap(context.Background(), testCase.peerID, "netbird.io", []nbdns.CustomZone{customZone}, validatedPeers, account.GetResourcePoliciesMap(), account.GetResourceRoutersMap(), nil) + customZone := account.GetPeersCustomZone(context.Background(), account.Domain) + networkMap := account.GetPeerNetworkMap(context.Background(), testCase.peerID, account.Domain, []nbdns.CustomZone{customZone}, validatedPeers, account.GetResourcePoliciesMap(), account.GetResourceRoutersMap(), nil)shared/management/http/api/openapi.yml (1)
4604-4787: Add400responses to DNS Zones list endpoints for consistencyMost list endpoints in this spec expose a
400response (e.g./api/dns/nameservers,/api/peers, etc.), but:
GET /api/dns/zonesGET /api/dns/zones/{zoneId}/recordscurrently only define
200, auth, and500(plus404on the records list).For consistency with the rest of the API surface and to accommodate potential client or path validation errors, consider adding
400to both:/api/dns/zones: get: @@ responses: '200': description: A JSON Array of DNS Zones content: application/json: schema: type: array items: $ref: '#/components/schemas/Zone' + '400': + "$ref": "#/components/responses/bad_request" '401': "$ref": "#/components/responses/requires_authentication" @@ /api/dns/zones/{zoneId}/records: get: @@ responses: '200': description: A JSON Array of DNS Records content: application/json: schema: type: array items: $ref: '#/components/schemas/DNSRecord' + '400': + "$ref": "#/components/responses/bad_request" '401': "$ref": "#/components/responses/requires_authentication"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
management/internals/controllers/network_map/controller/controller.go(14 hunks)management/internals/controllers/network_map/controller/repository.go(3 hunks)management/internals/server/boot.go(1 hunks)management/internals/server/modules.go(2 hunks)management/server/account_test.go(1 hunks)management/server/activity/codes.go(2 hunks)management/server/http/handler.go(3 hunks)management/server/http/handlers/peers/peers_handler.go(2 hunks)management/server/http/testing/testing_tools/channel/channel.go(2 hunks)shared/management/http/api/openapi.yml(3 hunks)shared/management/http/api/types.gen.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- management/server/http/handlers/peers/peers_handler.go
- management/server/http/testing/testing_tools/channel/channel.go
- management/server/http/handler.go
- management/internals/server/boot.go
- management/internals/controllers/network_map/controller/repository.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
📚 Learning: 2025-11-28T12:20:47.254Z
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
Applied to files:
management/internals/server/modules.goshared/management/http/api/openapi.ymlmanagement/internals/controllers/network_map/controller/controller.goshared/management/http/api/types.gen.gomanagement/server/account_test.go
🧬 Code graph analysis (2)
management/internals/server/modules.go (5)
management/internals/modules/zones/records/interface.go (1)
Manager(7-13)management/internals/modules/zones/interface.go (1)
Manager(7-13)management/internals/server/container.go (1)
Create(6-10)management/internals/modules/zones/manager/manager.go (1)
NewManager(23-29)management/internals/modules/zones/records/manager/manager.go (1)
NewManager(25-31)
management/server/account_test.go (2)
dns/dns.go (1)
CustomZone(43-52)shared/management/proto/management.pb.go (3)
CustomZone(2680-2689)CustomZone(2704-2704)CustomZone(2719-2721)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Android / Build
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: iOS / Build
- GitHub Check: Darwin
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
- GitHub Check: release_ui
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: release
🔇 Additional comments (14)
management/internals/server/modules.go (1)
10-13: Zones/records manager wiring is consistent and correctThe new
ZonesManagerandRecordsManageraccessors follow the existing lazyCreatepattern, inject the expected dependencies (store, account, permissions), and mirrorNetworksManager/ResourcesManagerstyle. No changes needed.Also applies to: 126-136
shared/management/http/api/openapi.yml (1)
28-29: DNS Zones tagging and schemas look coherentThe new
DNS Zonestag plusZoneRequest/Zoneand DNS record schemas are internally consistent and match existing patterns:
Zonecorrectly composesZoneRequestviaallOf, soenabledis defined where it’s required.ZoneRequestvsZoneseparation (optionalenabledon request, required on response) is a nice touch.DNSRecordType/DNSRecordRequest/DNSRecordcleanly model the limited A/AAAA/CNAME surface.No changes needed here.
Also applies to: 1710-1803
shared/management/http/api/types.gen.go (4)
15-20: LGTM: DNS record type constants and enum definition.The DNS record type constants cover the essential record types (A, AAAA, CNAME) for custom DNS zones. The string-based enum follows the existing pattern in this generated file.
Also applies to: 450-451
417-448: LGTM: DNS record models follow REST conventions.The separation between
DNSRecord(with ID) andDNSRecordRequest(without ID) follows standard RESTful API patterns. The field types are appropriate for DNS record data.
1909-1949: LGTM: Zone models are well-structured.The
ZoneandZoneRequeststructures provide comprehensive fields for DNS zone management. The use ofDistributionGroupsfor access control aligns with NetBird's group-based architecture, and embeddingRecordsin the zone response is good for API ergonomics.
2035-2045: LGTM: Request body type aliases are consistent.The request body type aliases follow the established oapi-codegen pattern and cover all necessary CRUD operations for DNS zones and records.
management/internals/controllers/network_map/controller/controller.go (7)
15-15: LGTM: New imports support DNS zones feature.The addition of
miekg/dns(standard Go DNS library) and the internal zones/records modules are necessary for the multi-zone DNS support being added.Also applies to: 24-25
202-206: LGTM: Comprehensive error handling and clean zone filtering pattern.The account zones retrieval includes proper error logging and propagation. The pattern of filtering zones by peer groups and then appending the peers custom zone is clear and correctly uses the goroutine parameter
p.Also applies to: 231-233, 238-238, 240-240
348-356: LGTM: Consistent zone handling pattern in UpdateAccountPeer.The zone retrieval and filtering logic matches the pattern in
sendUpdateAccountPeers, maintaining consistency across the codebase.Also applies to: 360-360, 362-362
455-465: LGTM: Zone handling consistent in GetValidatedPeerWithMap.The zone filtering follows the established pattern with appropriate error handling.
Also applies to: 476-476, 478-478
501-501: LGTM: Function signature updated for multi-zone support.The change from a single custom zone to a slice of custom zones enables multiple DNS zones per account. Extracting
dnsDomainfrom account settings before passing to the network map builder is appropriate.Also applies to: 512-513
834-843: LGTM: Zone handling in GetNetworkMap follows the pattern.The zone retrieval and filtering logic is consistent with the other updated functions.
Also applies to: 854-854, 856-856
871-928: LGTM: Well-structured zone filtering implementation.The
filterPeerAppliedZonesfunction correctly implements multi-zone DNS filtering:
- Early return for peers without groups is efficient
- Filtering logic correctly checks zone enabled status and non-empty records (consistent with the learning that zones without records don't affect network maps)
- Distribution group access check properly evaluates peer membership
- DNS record type mapping covers the standard types (A, AAAA, CNAME) with appropriate warning logging for unknown types
- Use of
dns.Fqdn()ensures proper DNS format for names and domains- The function is defensive and handles edge cases appropriately
Based on learnings, zones without records are correctly filtered out.
management/server/activity/codes.go (1)
184-190: LGTM: DNS activity codes follow established patterns.The six new activity constants (DNS zone and record create/update/delete) use sequential values and follow the existing naming conventions. The activity map entries have clear, descriptive messages and use hierarchical code strings (
dns.zone.*anddns.zone.record.*) consistent with other activity codes in the system.Also applies to: 302-308
Signed-off-by: bcmmbaga <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
management/server/store/sql_store.go (1)
4141-4149: Consider omitting associations in UpdateZone.
UpdateZoneusesSelect("*").Save(zone)without explicitly omitting associations. If a zone is retrieved viaGetZoneByID(which preloadsRecords), then modified and saved, GORM might attempt to update the nested records as well. For consistency with similar operations (e.g.,UpdateGroupat line 3163) and to prevent unintended association updates, consider usingOmit(clause.Associations):func (s *SqlStore) UpdateZone(ctx context.Context, zone *zones.Zone) error { - result := s.db.Select("*").Save(zone) + result := s.db.Omit(clause.Associations).Save(zone) if result.Error != nil { log.WithContext(ctx).Errorf("failed to update zone to store: %v", result.Error) return status.Errorf(status.Internal, "failed to update zone to store")This ensures that records are only modified through their dedicated CRUD operations (
CreateDNSRecord,UpdateDNSRecord, etc.).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
management/internals/modules/zones/records/manager/manager.go(1 hunks)management/server/store/sql_store.go(3 hunks)management/server/store/sql_store_test.go(2 hunks)management/server/store/store.go(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
📚 Learning: 2025-11-28T12:20:47.254Z
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
Applied to files:
management/server/store/sql_store_test.gomanagement/server/store/store.gomanagement/server/store/sql_store.gomanagement/internals/modules/zones/records/manager/manager.go
🧬 Code graph analysis (2)
management/server/store/store.go (3)
management/internals/modules/zones/zone.go (1)
Zone(13-22)shared/management/http/api/types.gen.go (1)
Zone(1910-1931)management/internals/modules/zones/records/record.go (1)
Record(21-29)
management/server/store/sql_store.go (4)
management/internals/modules/zones/zone.go (1)
Zone(13-22)management/internals/modules/zones/records/record.go (1)
Record(21-29)shared/management/status/error.go (4)
Error(54-57)Errorf(70-75)NewZoneNotFoundError(257-259)NewDNSRecordNotFoundError(262-264)management/server/store/store.go (2)
LockingStrength(42-42)LockingStrengthNone(49-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (26)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: iOS / Build
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
🔇 Additional comments (16)
management/internals/modules/zones/records/manager/manager.go (6)
19-31: LGTM!Clean constructor and dependency injection pattern for the records manager.
33-55: LGTM!Read operations correctly validate permissions and use appropriate locking strength for queries.
57-101: LGTM!The CreateRecord implementation correctly:
- Validates permissions before mutation
- Uses transactional boundaries with appropriate locking
- Validates record conflicts within the transaction
- Increments network serial to propagate changes
- Triggers peer updates asynchronously (as addressed from previous review)
103-160: LGTM!UpdateRecord correctly:
- Fetches existing record with appropriate locking
- Only re-validates conflicts when name/type/content changes (TTL changes correctly bypass conflict validation)
- Increments serial and triggers peer updates for all changes including TTL-only
162-207: LGTM!DeleteRecord correctly handles transactional deletion with proper locking, event logging, and peer notification.
209-236: LGTM!The zone membership check correctly enforces label boundary (exact match or proper subdomain with dot prefix), and the CNAME/duplicate conflict detection logic is sound.
management/server/store/sql_store_test.go (3)
25-26: LGTM!Imports correctly added for the new zones and records test coverage.
3723-3879: LGTM!Comprehensive test coverage for zone CRUD operations with proper table-driven tests for error cases and field validation.
3881-4126: LGTM!Thorough test coverage for DNS record operations including:
- Multiple record types (A, AAAA, CNAME)
- Name-based filtering
- Bulk deletion
- Proper error handling verification
management/server/store/store.go (3)
26-27: LGTM!Imports correctly added for the new zone and record types used in the interface.
208-213: LGTM!Zone interface methods follow the established patterns in the Store interface with consistent parameter ordering and locking strength handling for read operations.
215-222: LGTM!DNS record interface methods are comprehensive and properly scoped to zones:
- CRUD operations with correct parameter ordering
- Zone-scoped bulk operations (GetZoneDNSRecords, DeleteZoneDNSRecords)
- Name-based filtering (GetZoneDNSRecordsByName)
management/server/store/sql_store.go (4)
30-31: LGTM: Imports and migration setup.The new zones and records imports are correctly added, and the entities are properly registered in AutoMigrate for schema management.
Also applies to: 118-118
4134-4134: Previous review issues resolved.The typo in the error message (line 4134) has been corrected, and
DeleteDNSRecordnow properly scopes deletion by all three identifiers (accountID,zoneID, andrecordID), fixing the data integrity concern from the earlier review.Also applies to: 4221-4222
4131-4139: LGTM: Zone and DNS record CRUD operations.The remaining CRUD implementations follow consistent patterns throughout the file:
- Proper locking strength support on retrieval operations
- Appropriate error logging and status error types
- Correct use of
NewZoneNotFoundErrorandNewDNSRecordNotFoundError- Consistent query conditions (e.g.,
accountAndIDQueryConditionfor scoped operations)DeleteZoneDNSRecordscorrectly omitsRowsAffectedcheck since deleting zero records for a zone is validThe implementations integrate well with the existing store architecture.
Also applies to: 4165-4199, 4201-4219, 4235-4295
4151-4163: The zone deletion is properly handled at the manager layer. BeforeDeleteZoneis called in the store (line 162 in manager.go),DeleteZoneDNSRecordsis invoked first (line 157), ensuring all associated DNS records are removed within a transaction. This orchestration prevents both foreign key constraint errors and orphaned records. The simple delete implementation insql_store.gois appropriate since cascade logic is correctly managed at the manager layer.Likely an incorrect or invalid review comment.
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
management/internals/modules/zones/manager/manager.go (1)
184-193: Fix closure capture ofrecordwhen queuing delete eventsInside
DeleteZone, the callbacks you append toeventsToStoreclose over the loop variablerecord. When the loop finishes, all callbacks will see the lastrecordvalue, so you’ll emit incorrect IDs/metadata for most deleted records.Capture the current record in a new variable inside the loop:
- for _, record := range records { - eventsToStore = append(eventsToStore, func() { - meta := record.EventMeta(zone.ID, zone.Name) - m.accountManager.StoreEvent(ctx, userID, record.ID, accountID, activity.DNSRecordDeleted, meta) - }) - } + for _, record := range records { + rec := record + eventsToStore = append(eventsToStore, func() { + meta := rec.EventMeta(zone.ID, zone.Name) + m.accountManager.StoreEvent(ctx, userID, rec.ID, accountID, activity.DNSRecordDeleted, meta) + }) + }
🧹 Nitpick comments (3)
management/internals/modules/zones/manager/manager.go (1)
123-145: Consider incrementing network serial onUpdateZonefor consistency withDeleteZone
DeleteZoneincrements the network serial inside the transaction and then callsUpdateAccountPeers, butUpdateZoneonly callsUpdateAccountPeerswithout bumping the serial. For zones that already have records, togglingEnabled,EnableSearchDomain, or distribution groups changes peers’ DNS view just like deletion.To keep the network serial consistent with other config changes (e.g., DNS settings, routes, zone deletion), consider adding:
if err = transaction.UpdateZone(ctx, zone); err != nil { return fmt.Errorf("failed to update zone: %w", err) } + + if err = transaction.IncrementNetworkSerial(ctx, accountID); err != nil { + return fmt.Errorf("failed to increment network serial: %w", err) + }and leaving
CreateZoneas-is, since empty zones are filtered out of the network map until records exist (records manager will handle serial bumps). Based on learnings, empty zones are skipped in network map generation.management/internals/modules/zones/manager/manager_test.go (1)
455-493: TightenDeleteZonetests to assert per-record event IDs/metadataRight now
TestManagerImpl_DeleteZone/success with recordsonly checks thatStoreEventwas called three times and that initiator/account IDs match. This won’t catch issues in how record events are built (e.g., the closure-capture bug inDeleteZone).Consider extending the test to verify:
- You receive two
DNSRecordDeletedevents and oneDNSZoneDeleted.- The record deletion events reference the expected record IDs and zone metadata.
Capturing the
(activityID, targetID)pairs into a slice and asserting them against the created records would harden the test and prevent regressions in the event-building logic.management/server/store/store.go (1)
209-222: Zone/DNS record Store methods are consistent; consider optional re-groupingThe new zone and DNS record methods follow existing CRUD and locking patterns (ctx first,
LockingStrengthon read paths, account/zone/record IDs ordered consistently). From a contract perspective this looks solid. If you want slightly better cohesion, you could optionally move these methods nearer to other DNS-related operations (GetAccountDNSSettings, NameServerGroup methods) to keep related concerns clustered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
management/internals/controllers/network_map/controller/controller.go(14 hunks)management/internals/modules/zones/manager/manager.go(1 hunks)management/internals/modules/zones/manager/manager_test.go(1 hunks)management/internals/server/modules.go(2 hunks)management/server/account.go(1 hunks)management/server/account_test.go(3 hunks)management/server/store/sql_store.go(3 hunks)management/server/store/sql_store_test.go(2 hunks)management/server/store/store.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- management/server/account_test.go
- management/server/store/sql_store.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
📚 Learning: 2025-11-28T12:20:47.254Z
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
Applied to files:
management/server/account.gomanagement/internals/server/modules.gomanagement/internals/modules/zones/manager/manager_test.gomanagement/server/store/sql_store_test.gomanagement/internals/controllers/network_map/controller/controller.gomanagement/internals/modules/zones/manager/manager.gomanagement/server/store/store.go
🧬 Code graph analysis (4)
management/server/account.go (1)
shared/management/status/error.go (2)
Errorf(70-75)InvalidArgument(27-27)
management/internals/server/modules.go (5)
management/internals/modules/zones/interface.go (1)
Manager(7-13)management/internals/modules/zones/records/interface.go (1)
Manager(7-13)management/internals/server/container.go (1)
Create(6-10)management/internals/modules/zones/manager/manager.go (1)
NewManager(24-31)management/internals/modules/zones/records/manager/manager.go (1)
NewManager(25-31)
management/server/store/sql_store_test.go (3)
management/internals/modules/zones/zone.go (1)
NewZone(24-34)shared/management/status/error.go (3)
Error(54-57)FromError(78-87)NotFound(21-21)management/internals/modules/zones/records/record.go (1)
NewRecord(31-41)
management/internals/controllers/network_map/controller/controller.go (4)
shared/management/proto/management.pb.go (6)
CustomZone(2680-2689)CustomZone(2704-2704)CustomZone(2719-2721)SimpleRecord(2752-2762)SimpleRecord(2777-2777)SimpleRecord(2792-2794)management/server/types/account.go (1)
LookupMap(57-57)management/internals/modules/zones/records/record.go (3)
RecordTypeA(16-16)RecordTypeAAAA(17-17)RecordTypeCNAME(18-18)dns/dns.go (1)
DefaultClass(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Client / Unit (386)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: release_ui
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
🔇 Additional comments (4)
management/internals/server/modules.go (1)
10-13: Zones/records manager wiring matches existing patternThe ZonesManager and RecordsManager accessors follow the established lazy
Createpattern and pass the expected dependencies (Store,AccountManager,PermissionsManager, and DNS domain for zones). No issues spotted with init order or signatures.Also applies to: 126-136
management/server/store/sql_store_test.go (1)
25-27: Zone and DNS record store tests look comprehensiveThe new tests cover full CRUD and query flows for zones and DNS records (by ID, by domain/name, and bulk deletion) against the SQL store and assert expected
NotFoundbehavior. This gives solid coverage for the new persistence APIs.Also applies to: 3722-4194
management/internals/controllers/network_map/controller/controller.go (1)
179-206: Custom DNS zones wiring and filtering logic look correctThe new flow—fetching account zones once, deriving per‑peer
peerGroups, then buildingcustomZonesviafilterPeerAppliedZonesand appending the per‑accountpeersCustomZone—is consistent acrosssendUpdateAccountPeers,UpdateAccountPeer,GetValidatedPeerWithMap,GetNetworkMap, and the experimental builder path.
filterPeerAppliedZonesin particular:
- Skips disabled zones and zones with no records, matching the design that empty zones shouldn’t affect the network map.
- Applies distribution groups correctly using
types.LookupMap.- Restricts to supported record types (A/AAAA/CNAME), formats names/RDATA as FQDNs with
dns.Fqdn, and usesnbdns.DefaultClassandEnableSearchDomainas expected.Overall, the multi‑zone DNS integration into network map generation looks solid.
Also applies to: 231-241, 331-356, 455-479, 834-857, 871-929
management/server/store/store.go (1)
25-27: Zones/records imports look correctAdding
zonesandzones/recordshere cleanly wires the new domain types into the store layer; import style is consistent with existing internal modules and doesn’t introduce any obvious coupling or versioning issues.
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
Signed-off-by: bcmmbaga <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
management/server/types/account.go (1)
1787-1847: filterPeerAppliedZones helper is duplicated; consider centralizingThis free function is effectively identical to
NetworkMapBuilder.filterPeerAppliedZonesinnetworkmapbuilder.go. To avoid future drift in DNS record handling (e.g., when adding new record types or changing search-domain behaviour), it would be better to have a single shared implementation and call it from both the Account and NetworkMapBuilder paths.
🧹 Nitpick comments (3)
management/internals/controllers/network_map/controller/controller_test.go (1)
610-628: Test assertions may be order-dependent.The test compares zones and records by index (
tt.expected[i],result[i]), which assumesfilterPeerAppliedZonesreturns zones in the same order as the inputaccountZones. If the implementation changes iteration order (e.g., using maps internally), tests could fail unexpectedly.Consider using
ElementsMatchfrom testify for order-independent comparison, or document that order preservation is a contract offilterPeerAppliedZones.management/internals/controllers/network_map/controller/controller.go (1)
898-901: Consider adding zone context to the warning log.The warning at line 899 logs
record.IDbut doesn't include the zone context, which could make debugging harder when multiple zones have records.- log.WithContext(ctx).Warnf("unknown DNS record type %s for record %s", record.Type, record.ID) + log.WithContext(ctx).Warnf("unknown DNS record type %s for record %s in zone %s", record.Type, record.ID, zone.ID)management/server/types/networkmapbuilder.go (1)
2036-2095: Deduplicate NetworkMapBuilder.filterPeerAppliedZones with the Account helperThis method’s logic (enabled check,
len(zone.Records) > 0guard, distribution-group filtering, and record-type mapping) is identical to the package-levelfilterPeerAppliedZonesinaccount.go. Duplicating this behaviour in two places risks divergence if you add record types or tweak zone semantics later. Consider extracting a shared helper (e.g., in a small DNS/zones utility) and calling it from both paths. Thelen(zone.Records) == 0guard also matches the intended “empty zones don’t affect network maps” behaviour. Based on learnings, empty zones are safe to ignore.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
management/internals/controllers/network_map/controller/controller.go(14 hunks)management/internals/controllers/network_map/controller/controller_test.go(2 hunks)management/server/account_test.go(5 hunks)management/server/http/handlers/peers/peers_handler.go(2 hunks)management/server/types/account.go(7 hunks)management/server/types/networkmap.go(2 hunks)management/server/types/networkmap_golden_test.go(18 hunks)management/server/types/networkmapbuilder.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- management/server/http/handlers/peers/peers_handler.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
📚 Learning: 2025-11-28T12:20:47.254Z
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
Applied to files:
management/server/types/networkmap.gomanagement/internals/controllers/network_map/controller/controller.gomanagement/internals/controllers/network_map/controller/controller_test.gomanagement/server/types/account.gomanagement/server/types/networkmapbuilder.gomanagement/server/account_test.gomanagement/server/types/networkmap_golden_test.go
🧬 Code graph analysis (6)
management/server/types/networkmap.go (2)
management/server/telemetry/accountmanager_metrics.go (1)
AccountManagerMetrics(11-17)management/server/types/networkmapbuilder.go (1)
NetworkMapCache(35-61)
management/internals/controllers/network_map/controller/controller.go (5)
route/route.go (1)
ID(48-48)shared/management/proto/management.pb.go (6)
CustomZone(2680-2689)CustomZone(2704-2704)CustomZone(2719-2721)SimpleRecord(2752-2762)SimpleRecord(2777-2777)SimpleRecord(2792-2794)management/server/types/account.go (1)
LookupMap(59-59)management/internals/modules/zones/records/record.go (3)
RecordTypeA(16-16)RecordTypeAAAA(17-17)RecordTypeCNAME(18-18)dns/dns.go (1)
DefaultClass(21-21)
management/internals/controllers/network_map/controller/controller_test.go (4)
management/server/types/account.go (1)
LookupMap(59-59)route/route.go (1)
ID(48-48)management/internals/modules/zones/records/record.go (3)
RecordTypeA(16-16)RecordTypeAAAA(17-17)RecordTypeCNAME(18-18)dns/dns.go (1)
DefaultClass(21-21)
management/server/types/account.go (2)
dns/dns.go (3)
CustomZone(43-52)SimpleRecord(55-66)DefaultClass(21-21)management/internals/modules/zones/records/record.go (3)
RecordTypeA(16-16)RecordTypeAAAA(17-17)RecordTypeCNAME(18-18)
management/server/types/networkmapbuilder.go (3)
management/server/types/account.go (1)
LookupMap(59-59)management/internals/modules/zones/records/record.go (3)
RecordTypeA(16-16)RecordTypeAAAA(17-17)RecordTypeCNAME(18-18)dns/dns.go (1)
DefaultClass(21-21)
management/server/account_test.go (1)
management/server/types/settings.go (2)
Settings(10-55)ExtraSettings(82-99)
🔇 Additional comments (11)
management/internals/controllers/network_map/controller/controller_test.go (1)
119-630: Comprehensive test coverage for zone filtering logic.The test thoroughly covers:
- Empty peer groups and zones
- All DNS record types (A, AAAA, CNAME)
- Search domain enabled/disabled states
- Disabled zones filtering
- Empty record zones filtering
- Multiple zones with mixed access
- Peer membership across multiple groups
The CNAME RData correctly includes trailing dot via
dns.Fqdn()transformation (line 418).management/server/types/networkmap.go (1)
25-35: Signature extension looks correct for multi-zone DNS support.The
accountZonesparameter is appropriately added and forwarded toNetworkMapCache.GetPeerNetworkMap. This aligns with the network map generation flow where account zones are filtered per-peer based on group membership.management/internals/controllers/network_map/controller/controller.go (2)
860-920: Well-structured zone filtering implementation.The
filterPeerAppliedZonesfunction correctly:
- Returns early for empty peer groups (line 864-866)
- Filters disabled zones and zones without records (line 869) - aligning with the learning that empty zones don't affect network maps
- Uses set lookup for efficient group membership check (lines 873-879)
- Properly converts record types to DNS wire types (lines 890-901)
- Applies
dns.Fqdn()to ensure proper DNS name formatting for CNAME content, record names, and zone domains
202-206: Consistent error handling for account zones retrieval.The error handling pattern is consistently applied across all call sites (
sendUpdateAccountPeers,UpdateAccountPeer,GetValidatedPeerWithMap,GetNetworkMap), logging the error and returning it appropriately.management/server/types/networkmap_golden_test.go (1)
72-72: Golden test signatures correctly updated.All
GetPeerNetworkMapcalls are consistently updated to include the newaccountZonesparameter (passed asnil), maintaining test focus on network map structure while the zone filtering is tested separately incontroller_test.go.management/server/account_test.go (3)
2062-2089: Good test for DNS domain conflict validation.The test correctly validates that setting
DNSDomainto a value that conflicts with an existing custom DNS zone's domain produces an appropriate error. This prevents configuration conflicts that could cause DNS resolution issues.
1680-1680: GetRoutesToSync signature updated to include peer groups.The updated call now passes
account.GetPeerGroups("peer-2")to enable route filtering based on peer group membership, which aligns with the broader multi-zone DNS and access control changes.
401-401: GetPeerNetworkMap call updated consistently.The signature update passes
nilforaccountZones, which is appropriate for this test that focuses on peer login expiration behavior rather than DNS zone filtering.management/server/types/networkmapbuilder.go (1)
941-965: Multi-zone DNS wiring in NetworkMapBuilder looks consistentPassing
accountZonesintoGetPeerNetworkMap/assembleNetworkMapand combining the per-peer custom zone withfilterPeerAppliedZonesoutput underServiceEnablepreserves previous DNSConfig semantics while correctly scoping account zones by peer group membership and ignoring empty/disabled zones. No functional issues from my side.Also applies to: 980-1051
management/server/types/account.go (2)
149-167: Reusing peerGroups in GetRoutesToSync keeps semantics and saves workPassing
peerGroupsintoGetRoutesToSyncand using it infilterRoutesByGroupspreserves the previous behaviour while avoiding recomputing group membership inside the function. This is a clean, minimal change.
270-351: Account-level DNS zones are correctly filtered before inclusion in DNSConfigComputing
peerGroups := a.GetPeerGroups(peerID)once and reusing it for bothGetRoutesToSyncandfilterPeerAppliedZones(ctx, accountZones, peerGroups)ensures:
- only peers in a zone’s
DistributionGroupssee that zone, and- zones that are disabled or have no records are ignored.
This aligns with the intended behaviour that empty zones don’t affect network maps and so don’t require triggering peer updates. Based on learnings, this is the expected contract.
# Conflicts: # management/server/store/sql_store.go
Signed-off-by: bcmmbaga <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
management/server/store/sql_store.go (1)
4070-4153: Zone CRUD implementation matches store patterns; consider small robustness tweaksThe zone create/update/delete/get methods follow existing store conventions (scoping by
account_id, preloading associations, and using specificZoneNotFounderrors), which is consistent and correct.Two optional improvements you may want to consider:
- Add nil‑argument validation for
CreateZone/UpdateZone(mirroringCreateGroup/UpdateGroup) to fail fast on programmer errors instead of relying on GORM behavior with nil pointers.- If zone domains are stored normalized (e.g., lower‑cased on write), consider normalizing the
domainparameter inGetZoneByDomainfor case‑insensitive behavior and consistency with other domain lookups.Example for the first point:
func (s *SqlStore) CreateZone(ctx context.Context, zone *zones.Zone) error { - result := s.db.Create(zone) + if zone == nil { + return status.Errorf(status.InvalidArgument, "zone is nil") + } + + result := s.db.Create(zone) @@ func (s *SqlStore) UpdateZone(ctx context.Context, zone *zones.Zone) error { - result := s.db.Select("*").Save(zone) + if zone == nil { + return status.Errorf(status.InvalidArgument, "zone is nil") + } + + result := s.db.Select("*").Save(zone)Also, please double‑check that zone deletion will not leave orphan DNS records (either via FK cascade or higher‑level use of
DeleteZoneDNSRecords), as that invariant isn’t visible from this file alone.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
management/internals/controllers/network_map/controller/controller.go(12 hunks)management/internals/server/modules.go(2 hunks)management/server/http/handler.go(3 hunks)management/server/store/sql_store.go(3 hunks)management/server/types/account_test.go(2 hunks)management/server/types/networkmapbuilder.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- management/internals/server/modules.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
📚 Learning: 2025-11-28T12:20:47.254Z
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
Applied to files:
management/server/http/handler.gomanagement/internals/controllers/network_map/controller/controller.gomanagement/server/types/networkmapbuilder.gomanagement/server/store/sql_store.gomanagement/server/types/account_test.go
🧬 Code graph analysis (3)
management/server/http/handler.go (4)
management/internals/modules/zones/records/interface.go (1)
Manager(7-13)management/internals/modules/zones/interface.go (1)
Manager(7-13)management/internals/modules/zones/manager/api.go (1)
RegisterEndpoints(20-30)management/internals/modules/zones/records/manager/api.go (1)
RegisterEndpoints(20-30)
management/internals/controllers/network_map/controller/controller.go (1)
shared/management/proto/management.pb.go (3)
CustomZone(2680-2689)CustomZone(2704-2704)CustomZone(2719-2721)
management/server/types/account_test.go (3)
management/server/types/account.go (1)
LookupMap(59-59)management/internals/modules/zones/records/record.go (3)
RecordTypeA(16-16)RecordTypeAAAA(17-17)RecordTypeCNAME(18-18)dns/dns.go (1)
DefaultClass(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (25)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: Windows
- GitHub Check: iOS / Build
- GitHub Check: release_ui
- GitHub Check: Darwin
- GitHub Check: release
- GitHub Check: Android / Build
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
🔇 Additional comments (6)
management/server/types/account_test.go (1)
1244-1754: Excellent test coverage for zone filtering logic.This test comprehensively validates the
filterPeerAppliedZonesfunction across multiple scenarios including group-based access control, zone state filtering (disabled/empty), record type handling (A/AAAA/CNAME), and DNS formatting conventions. The test structure is clear and each scenario is well-documented.management/server/types/networkmapbuilder.go (1)
1028-1049: DNS zone assembly logic looks correct.The implementation properly:
- Converts peer groups from slice to lookup map for efficient filtering
- Handles the legacy peersCustomZone (if domain is present)
- Filters account zones based on peer's group membership
- Combines both zone types into the final DNS configuration
The peerGroups map is rebuilt on each call, which ensures consistency with the latest cache state and is acceptable given its lightweight nature.
management/internals/controllers/network_map/controller/controller.go (1)
200-204: Consistent integration of account zones across all network map generation paths.The
GetAccountZonescalls are properly integrated with consistent error handling across:
- Bulk peer updates (
sendUpdateAccountPeers)- Single peer updates (
UpdateAccountPeer)- Peer validation with map (
GetValidatedPeerWithMap)- Network map retrieval (
GetNetworkMap)All call sites follow the same pattern: retrieve zones, log errors, and propagate failures appropriately.
Also applies to: 343-347, 448-452, 824-828
management/server/http/handler.go (1)
17-20: Zones and DNS records HTTP wiring looks consistent and correctThe added
zones/recordsimports, newzManagerandrManagerparameters inNewAPIHandler, and theRegisterEndpointscalls all line up with the managers’RegisterEndpointssignatures and are placed on the same authenticated, metered router as existing APIs. No issues from a wiring perspective.Also applies to: 72-73, 142-143
management/server/store/sql_store.go (2)
30-31: Zones and DNS record models are correctly integrated into the storeImporting
zonesandrecordsand adding&zones.Zone{}/&records.Record{}toAutoMigratefollows the existing pattern for other managed entities and ensures persistence is available for the new APIs.Also applies to: 117-118
4155-4249: DNS record CRUD looks correct and fixes earlier scoping concernThe DNS record methods are consistent with other store helpers:
CreateDNSRecord/UpdateDNSRecordfollow the established logging +status.Internalpattern.DeleteDNSRecordnow scopes byaccount_id,zone_id, andid, aligning its behavior withGetDNSRecordByIDand resolving the earlier inconsistency about missingzone_idin the delete path.- Zone‑scoped list and delete helpers (
GetZoneDNSRecords*,DeleteZoneDNSRecords) are straightforward and correctly filter byaccount_idandzone_id.Given these are bulk/maintenance operations, returning success from
DeleteZoneDNSRecordseven when no rows are affected is reasonable and matches how some other batch deletes behave in this store.
# Conflicts: # management/server/account.go # management/server/account_test.go # management/server/store/sql_store_test.go
Signed-off-by: bcmmbaga <[email protected]>
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/server/store/sql_store.go (1)
112-121: Add explicit database-level FK constraint with CASCADE delete for records.Record → zones.Zone relationship.The deletion of records.Record is currently handled only at the application level (in manager.DeleteZone via an explicit transaction), not at the database schema level. While this works in the managed code path, if sql_store.DeleteZone is called directly (bypassing the manager), orphaned records will remain. Add
onDelete:cascadeto the Zone.Records GORM tag or define explicit database constraints to ensure referential integrity at the schema level.
🧹 Nitpick comments (7)
management/server/account_test.go (2)
400-404:GetPeerNetworkMaparg update: consider passing peer-groups (and renamecustomZone) to avoid “nil means ignore” drift.If the new signature is effectively
(ctx, peerID, accountZones, peerGroups, ...), hardcodingnilfor peer-groups can mask regressions in peer-group filtering logic. AlsocustomZoneis confusing if it’s actually a zone list.- customZone := account.GetPeersCustomZone(context.Background(), "netbird.io") - networkMap := account.GetPeerNetworkMap(context.Background(), testCase.peerID, customZone, nil, validatedPeers, account.GetResourcePoliciesMap(), account.GetResourceRoutersMap(), nil) + accountZones := account.GetPeersCustomZone(context.Background(), "netbird.io") + peerGroups := account.GetPeerGroups(testCase.peerID) + networkMap := account.GetPeerNetworkMap(context.Background(), testCase.peerID, accountZones, peerGroups, validatedPeers, account.GetResourcePoliciesMap(), account.GetResourceRoutersMap(), nil)
2099-2126: DNSDomain conflict test is good; make it robust against “effective zones only” and domain normalization.If conflict validation ever starts considering only zones that “apply” (e.g., enabled + matching groups + possibly having records), this could become a false negative since the zone is created with empty
DistributionGroupsand no records. Also, if validation normalizes domains (lowercase, trailing dot), asserting against raw strings might drift.Concrete hardening options:
- Add a minimal record (if the schema allows) so the zone is unquestionably “active”.
- Ensure the test uses a normalized domain variant consistent with validation (e.g., lowercased / no trailing dot), or assert using a structured error type if available instead of
err.Error().management/server/store/store.go (2)
25-28: Consider import aliasing to avoid “records” ambiguity at call sites (Line 25-Line 28).
Withdnsalready in this file, importingzones/recordsas plainrecordscan make call sites read a bit ambiguously (DNS “records” vs package “records”). Aliasing tozonesrecords(or similar) tends to reduce confusion.
209-223: Store interface additions look coherent; consider locking + consistency for domain lookup (Line 209-Line 223).
GetZoneByDomain(ctx, accountID, domain)lackslockStrengthwhile other “get” methods expose it. If this is expected to be usable in transactional flows (or needs consistent read semantics), consider addinglockStrengthfor symmetry.management/server/store/sql_store.go (3)
29-32: Imports OK; consider aliasingzones/recordsfor readability (Line 29-Line 32).
Same note asstore.go:recordscan read ambiguously in DNS-heavy code; an alias may improve clarity.
4237-4251: GetZoneDNSRecordsByName: verify name normalization expectations (Line 4237-Line 4251).
DNS labels are case-insensitive in practice; if the system normalizes record names elsewhere, this is fine—otherwise, consider normalizingname(or documenting exact-match requirement).
4253-4261: DeleteZoneDNSRecords: OK; consider whether callers need a not-found signal (Line 4253-Line 4261).
Bulk delete returningnilwhen nothing matched is usually fine—just ensure manager/API expectations align.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
management/server/account.go(3 hunks)management/server/account_test.go(5 hunks)management/server/store/sql_store.go(3 hunks)management/server/store/sql_store_test.go(2 hunks)management/server/store/store.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- management/server/account.go
- management/server/store/sql_store_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
📚 Learning: 2025-11-28T12:20:47.254Z
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
Applied to files:
management/server/account_test.gomanagement/server/store/store.gomanagement/server/store/sql_store.go
🧬 Code graph analysis (2)
management/server/store/store.go (3)
management/internals/modules/zones/zone.go (1)
Zone(13-22)shared/management/http/api/types.gen.go (1)
Zone(1910-1931)management/internals/modules/zones/records/record.go (1)
Record(21-29)
management/server/store/sql_store.go (4)
management/internals/modules/zones/zone.go (1)
Zone(13-22)management/internals/modules/zones/records/record.go (1)
Record(21-29)shared/management/status/error.go (4)
Error(54-57)Errorf(70-75)NewZoneNotFoundError(257-259)NewDNSRecordNotFoundError(262-264)management/server/store/store.go (2)
LockingStrength(42-42)LockingStrengthNone(49-49)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit (386)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: release
🔇 Additional comments (9)
management/server/account_test.go (2)
24-32: Importingzonesis fine; keep package naming unambiguous.
zones.Zonereads clearly and avoids alias noise—no issues here.
1680-1692: Good test update to includeaccount.GetPeerGroups(...)inGetRoutesToSynccalls.This better reflects the real call path after peer-group filtering was introduced.
management/server/store/sql_store.go (7)
4116-4134: GetZoneByID: LGTM (locking + Preload) — verify GORM pointer behavior is intentional (Line 4116-Line 4134).
Pattern matches other store methods (var x *T; Take(&x, ...)). Just ensure this is a deliberate GORM usage style across the repo.
4151-4165: GetAccountZones: LGTM (locking + Preload) (Line 4151-Line 4165).
4187-4199: DeleteDNSRecord: good scoping by account_id + zone_id + id (Line 4187-Line 4199).
This matchesGetDNSRecordByIDscoping and avoids cross-zone deletes by record ID alone.
4201-4219: GetDNSRecordByID: LGTM (scoped query + optional locking) (Line 4201-Line 4219).
4221-4235: GetZoneDNSRecords: LGTM (Line 4221-Line 4235).
4082-4090: No changes needed. TheCreateZonefunction correctly handles zones without records. Records are never populated at zone creation time—they are managed through a separate Records API. The zone parameter is already validated by the caller before reaching this store method, and GORM does not persist nil/empty associations. The learning from the codebase confirms that zones without records are filtered downstream in network map generation, so no defensive checks or association omission is required here.Likely an incorrect or invalid review comment.
4102-4114: DeleteZone at the store layer can leave orphan DNS records, but this is mitigated by the manager layer.The
SqlStore.DeleteZonemethod (lines 4102-4114) deletes only the zone without handling related records. However, examiningmanagement/internals/modules/zones/manager/manager.go, theDeleteZonemanager function wraps the store operation in a transaction that explicitly callsDeleteZoneDNSRecordsbefore deleting the zone. While the public API is safe, the store method itself exposes a data integrity risk if called directly. Consider adding explicit record deletion withinDeleteZonefor consistency and safety, or document that this method must be called within a transaction that handles records.
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
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.