feat: ICANN resolver support #471
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds DNS root-hints support. Two new fields (DnsConfig.RootHints and DnsConfig.RootHintsFile) and an embedded default root hints file ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)internal/dns/dns.go (1)
⏰ 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). (1)
🔇 Additional comments (3)
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: 2
🧹 Nitpick comments (2)
internal/config/config.go (1)
180-187: Consider wrapping the error with context.The error from
os.ReadFileis returned directly. Adding context would help users understand what went wrong during configuration loading.// Load root hints if globalConfig.Dns.RootHintsFile != "" { hintsContent, err := os.ReadFile(globalConfig.Dns.RootHintsFile) if err != nil { - return nil, err + return nil, fmt.Errorf("error reading root hints file: %w", err) } globalConfig.Dns.RootHints = string(hintsContent) }internal/dns/dns.go (1)
87-87: Consider compiling the regexp at package level.The regex is compiled on each call to
loadRootHints. While this only runs at startup, moving it to a package-level variable is a minor best-practice improvement.+var commentRe = regexp.MustCompile(`^\s*;`) + func loadRootHints(cfg *config.Config) error { - commentRe := regexp.MustCompile(`^\s*;`) rootHints = make(map[uint16]map[string][]dns.RR)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/config/config.go(5 hunks)internal/config/named.root(1 hunks)internal/dns/dns.go(5 hunks)
⏰ 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). (2)
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
internal/config/named.root (1)
1-92: LGTM - Standard root hints file.This is the standard IANA root hints file format containing all 13 root servers with their IPv4 and IPv6 addresses. The data is current (November 2025) and follows the expected BIND zone file format.
internal/config/config.go (3)
10-10: LGTM - Embed import for root hints.The blank identifier import for
embedis required to enable the//go:embeddirective.
40-41: LGTM - Root hints configuration fields.The dual approach (inline content via
RootHintsor file path viaRootHintsFile) provides flexibility for different deployment scenarios.
75-77: LGTM - Embedded default root hints.Good use of
//go:embedto bundle the root hints file at compile time, ensuring the server works out of the box without external dependencies.internal/dns/dns.go (2)
34-35: LGTM - Root hints cache declaration.The nested map structure appropriately indexes records by type and name for efficient lookups.
46-49: LGTM - Root hints loading at startup.Loading root hints before setting up DNS handlers ensures the cache is populated before any queries arrive.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 3 files
Prompt for AI agents (all 4 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="internal/config/config.go">
<violation number="1" location="internal/config/config.go:184">
P2: Error returned without context wrapping. For consistency with other error handling in this function (e.g., `fmt.Errorf("error reading config file: %w", err)`), this error should be wrapped with descriptive context.</violation>
</file>
<file name="internal/config/named.root">
<violation number="1" location="internal/config/named.root:92">
P3: File is missing a trailing newline. POSIX text files should end with a newline character. While most modern DNS parsers handle this gracefully, adding a trailing newline ensures compatibility with all tools and follows standard conventions.</violation>
</file>
<file name="internal/dns/dns.go">
<violation number="1" location="internal/dns/dns.go:88">
P0: Variable shadowing: using `:=` creates a local `rootHints` that shadows the package-level variable. The package-level `rootHints` is never populated. Use `=` instead and initialize the package-level variable before assignment.</violation>
<violation number="2" location="internal/dns/dns.go:91">
P1: Empty lines will cause nil pointer panic. `dns.NewRR("")` returns `nil, nil`, and calling `tmpRR.Header()` on nil will panic. Add a check for empty lines or verify `tmpRR` is not nil.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
agaffney
force-pushed
the
feat/icann-resolver
branch
from
e89f264 to
eb844ff
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/config/config.go (1)
180-187: Wrap the error with context.The error at line 184 should be wrapped with descriptive context for consistency with other error handling in this function (e.g., line 121, line 125).
Apply this diff:
if globalConfig.Dns.RootHintsFile != "" { hintsContent, err := os.ReadFile(globalConfig.Dns.RootHintsFile) if err != nil { - return nil, err + return nil, fmt.Errorf("error reading root hints file: %w", err) } globalConfig.Dns.RootHints = string(hintsContent) }
🧹 Nitpick comments (1)
internal/dns/dns.go (1)
45-48: Consider adding success logging for root hints load.While treating root hints load failure as fatal is appropriate, logging successful loading would aid debugging and provide visibility into startup initialization.
Apply this diff:
// Load root hints if err := loadRootHints(cfg); err != nil { return err } + slog.Info("root hints loaded successfully")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/config/config.go(5 hunks)internal/config/named.root(1 hunks)internal/dns/dns.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/named.root
🧰 Additional context used
🧬 Code graph analysis (1)
internal/dns/dns.go (1)
internal/config/config.go (1)
Config(19-28)
⏰ 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). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (6)
internal/config/config.go (4)
10-10: LGTM! Import added for embed support.The blank import of the
embedpackage enables the//go:embeddirective used at line 75.
40-41: LGTM! New configuration fields for root hints.The
RootHintsandRootHintsFilefields are properly configured with YAML tags and environment variable bindings.
75-76: LGTM! Embedded root hints for default configuration.Using
//go:embedto embed thenamed.rootfile provides sensible defaults without requiring external files.
87-87: LGTM! Proper initialization from embedded defaults.Converting the embedded
defaultRootHintsbytes to a string for the initialRootHintsvalue is appropriate.internal/dns/dns.go (2)
33-33: LGTM! Package-level root hints cache.The global
rootHintsvariable provides a centralized cache for parsed root hints, properly typed as a nested map structure.
85-105: LGTM! Root hints loading implementation is correct.The function properly:
- Initializes the package-level
rootHintsvariable (no shadowing)- Handles nil RRs from empty lines and comments (lines 92-94)
- Uses correct nested map types with slice values
- Appends to slices instead of overwriting entries
Previous review concerns have been addressed.
agaffney
force-pushed
the
feat/icann-resolver
branch
from
eb844ff to
5c6c7d3
Compare
Fixes #264 Signed-off-by: Aurora Gaffney <aurora@blinklabs.io>
agaffney
force-pushed
the
feat/icann-resolver
branch
from
5c6c7d3 to
6e3dd20
Compare
Fixes #264
Summary by cubic
Adds ICANN resolver support using embedded root hints so we can perform true recursive resolution from the root servers. Root hints can be overridden via config or env.
Written for commit 6e3dd20. Summary will update automatically on new commits.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.