[client] Fall through dns chain for custom dns zones#5081
Conversation
📝 WalkthroughWalkthroughThe changes refactor DNS zone handling by replacing the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LocalResolver as Local DNS Resolver
participant UpstreamResolver as Upstream Resolver
Client->>LocalResolver: Query for non-authoritative zone
LocalResolver->>LocalResolver: findZone(qname)
alt Zone found and non-authoritative
LocalResolver->>LocalResolver: Check NXDOMAIN condition
LocalResolver->>Client: Return NXDOMAIN + set Zero bit (fallthrough signal)
Client->>UpstreamResolver: Forward to next handler (Zero bit triggers fallthrough)
UpstreamResolver->>UpstreamResolver: Clear Zero bit to prevent cascading signals
UpstreamResolver->>Client: Return upstream response
else Zone found and authoritative
LocalResolver->>LocalResolver: Serve from local records
LocalResolver->>Client: Return response
else Zone not found
LocalResolver->>UpstreamResolver: Forward to next handler
UpstreamResolver->>Client: Return response
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dns/dns.go (1)
50-51: LGTM! Better semantic naming.The rename from
SkipPTRProcesstoNonAuthoritativebetter conveys the semantic meaning of this field. Consider expanding the comment to mention the fallthrough behavior on NXDOMAIN, similar to the proto definition comment.📝 Optional: More descriptive comment
- // NonAuthoritative marks user-created zones + // NonAuthoritative marks user-created zones; non-authoritative zones will + // fallthrough to lower-priority handlers on NXDOMAIN and skip PTR processing. NonAuthoritative bool
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
shared/management/proto/management.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (11)
client/internal/dns.goclient/internal/dns/local/local.goclient/internal/dns/local/local_test.goclient/internal/dns/server.goclient/internal/dns/server_test.goclient/internal/dns/upstream.goclient/internal/engine.goclient/internal/routemanager/dnsinterceptor/handler.godns/dns.gomanagement/internals/shared/grpc/conversion.goshared/management/proto/management.proto
🧰 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:
client/internal/engine.goclient/internal/dns/upstream.goclient/internal/dns/server.goclient/internal/dns.goclient/internal/dns/local/local.goclient/internal/dns/server_test.goclient/internal/dns/local/local_test.go
🧬 Code graph analysis (3)
client/internal/engine.go (2)
dns/dns.go (1)
CustomZone(43-52)shared/management/proto/management.pb.go (3)
CustomZone(2868-2877)CustomZone(2892-2892)CustomZone(2907-2909)
management/internals/shared/grpc/conversion.go (2)
dns/dns.go (1)
SimpleRecord(55-66)shared/management/proto/management.pb.go (3)
SimpleRecord(2940-2950)SimpleRecord(2965-2965)SimpleRecord(2980-2982)
client/internal/dns/server.go (1)
dns/dns.go (3)
CustomZone(43-52)SimpleRecord(55-66)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). (11)
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: release_ui
- GitHub Check: release_ui_darwin
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: Build Cache
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
🔇 Additional comments (20)
client/internal/engine.go (1)
1254-1263: Backward compatibility logic is well-implemented.The approach of treating single-zone scenarios as authoritative to maintain backward compatibility with older servers that only send the peer FQDN zone is sound. The comment clearly documents the reasoning, and the
&& !singleZoneCompatcondition correctly ensures:
- Old servers (single zone, no field 4 set) → zone treated as authoritative (existing behavior preserved)
- New servers (multiple zones, field 4 set) → respects
NonAuthoritativeflag for fallthrough behaviorThe extraction of
protoZonesbefore the loop avoids redundant getter calls.client/internal/dns/upstream.go (1)
205-207: LGTM! Good defensive measure.Clearing the Zero bit from external upstream responses prevents external DNS servers from inadvertently triggering the internal fallthrough signaling mechanism. This is a sensible security hardening for the new handler chain continuation logic.
management/internals/shared/grpc/conversion.go (1)
375-390: LGTM! Correct proto field mapping.The
NonAuthoritativefield is properly propagated from the domain model to the proto message, maintaining consistency with the renamed field in the proto definition.client/internal/routemanager/dnsinterceptor/handler.go (1)
328-330: LGTM! Consistent with upstream.go change.Clearing the Zero bit from peer DNS responses provides the same defensive measure as in
upstream.go, preventing external sources (routing peers in this case) from manipulating the internal fallthrough signaling mechanism.shared/management/proto/management.proto (1)
467-469: LGTM! Wire-compatible field rename with good documentation.The field tag number (4) is preserved, ensuring wire compatibility with existing clients. The comment clearly documents both behaviors: fallthrough to lower-priority handlers on NXDOMAIN and PTR processing skip for user-created zones.
client/internal/dns.go (1)
79-81: LGTM!The field rename from
SkipPTRProcesstoNonAuthoritativeis consistent with the broader refactoring. The logic correctly skips PTR record collection for non-authoritative (user-created) zones.client/internal/dns/server_test.go (3)
131-131: LGTM!The test field rename from
initLocalRecordstoinitLocalZoneswith type[]nbdns.CustomZonealigns with the updated API.
388-388: LGTM!The test correctly passes
[]nbdns.CustomZoneto the updatedUpdatemethod.
2015-2016: LGTM!The test correctly calls
buildLocalHandlerUpdatewithconfig.CustomZonesand expects the updated return signature([]handlerWrapper, []nbdns.CustomZone, error).client/internal/dns/server.go (2)
488-501: LGTM!The updated call site correctly uses the new return signature and passes the CustomZone slice to the local resolver.
661-691: LGTM!The refactored
buildLocalHandlerUpdatefunction:
- Correctly processes
CustomZoneobjects instead of separate records and zone domains- Filters out invalid class types while preserving valid records
- Returns the processed zones preserving the
NonAuthoritativeflag for downstream useThe loop variable
customZoneis a struct copy, so assigningcustomZone.Records = localRecordscorrectly builds a filtered copy without mutating the original input.client/internal/dns/local/local_test.go (4)
127-143: LGTM!Test correctly updated to use
[]nbdns.CustomZonewith proper structure containingDomainandRecordsfields.
1094-1188: Comprehensive fallthrough test coverage.This test properly validates:
- Authoritative zones return NXDOMAIN without fallthrough (Zero bit = false)
- Non-authoritative zones trigger fallthrough (Zero bit = true)
- Record matches in both zone types return normally without fallthrough
The use of
responseMSG.MsgHdr.Zeroto signal fallthrough is correctly verified.
1319-1342: Good edge case coverage for case-insensitive domain matching.This ensures the fallthrough logic correctly handles zone domains with different casing (e.g.,
EXAMPLE.COM.matches queries fornonexistent.example.com.).
1344-1420: Useful performance benchmarks.The benchmarks cover:
- Best case: immediate zone match
- Worst case: many zones, no match, many labels
- Typical case: few zones with subdomain match
- Many zones: 100 zones with mid-list match
These help ensure zone lookup remains efficient as the number of zones grows.
client/internal/dns/local/local.go (5)
30-36: LGTM!The zones map with
boolvalue forNonAuthoritativestatus is a clean representation that supports O(1) zone lookup and O(k) suffix matching where k is the number of labels.
102-105: Fallthrough signaling correctly implemented.When NXDOMAIN is determined for a query in a non-authoritative zone, the resolver signals the handler chain to continue to the next handler. This enables the overlay behavior described in the PR objectives.
130-145: Efficient zone lookup implementation.The
findZonefunction correctly performs reverse suffix matching with O(k) complexity where k is the number of labels in the query name. The algorithm strips leading labels progressively to find the longest matching zone.Note: This function must only be called with the read lock held (via
shouldFallthroughorisInManagedZone), which is currently the case.
383-402: LGTM!The
Updatefunction is cleanly refactored to:
- Accept
[]nbdns.CustomZoneas a single parameter- Clear existing state before populating
- Properly normalize zone domains (lowercase + FQDN)
- Store
NonAuthoritativeflag per zone- Register individual records within each zone
This aligns with the zone-centric data model introduced by the PR.
157-164: No action required. The Zero bit mechanism for handler chain fallthrough signaling is correctly implemented. TheResponseWriterChain.WriteMsg()inhandler_chain.goproperly checks for the signal (NXDOMAIN with Zero bit set) and continues to the next handler, whileupstream.goprotects against abuse by clearing the bit from external responses.
2 participants
Describe your changes
Custom zones act as "overlays". You define specific records while unmatched queries in that zone fall through to lower-priority handlers (DNS routes, upstream). Without this, the local resolver claims authority over the entire zone and returns NXDOMAIN for any record it doesn't have, blocking CNAMEs from resolving via other handlers.
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
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.