[fix] Logical mistake while determining local resolvers #1773
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request modify the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resolver
participant Network
Client->>Resolver: Request to add resolver
Resolver->>Network: Check if resolver is local
Network-->>Resolver: Return network info
alt If network info is valid
Resolver->>Resolver: Add to localResolvers
else
Resolver->>Resolver: Do not add to localResolvers
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (2)
service/resolver/resolvers.go (2)
513-515: Consider handling the error from GetLocalNetworkThe logical fix to include resolvers from local networks is correct. However, ignoring the error from
GetLocalNetworkmight lead to incorrect categorization if there are network configuration issues.Consider this improved implementation:
- } else if net, _ := netenv.GetLocalNetwork(resolver.Info.IP); net != nil { + } else { + net, err := netenv.GetLocalNetwork(resolver.Info.IP) + if err != nil { + log.Debugf("resolver: error checking local network for %s: %v", resolver.Info.IP, err) + continue + } + if net != nil { + localResolvers = append(localResolvers, resolver) + }This change:
- Properly handles potential errors from GetLocalNetwork
- Logs issues for debugging
- Makes the code more maintainable with better error visibility
Line range hint
511-524: Consider refactoring resolver categorization logicThe current implementation mixes different concerns in the same function. Consider extracting the resolver categorization logic into separate helper functions for better maintainability.
Example refactor:
+func isLocalResolver(resolver *Resolver) bool { + if resolver.Info.IPScope.IsLAN() { + return true + } + net, err := netenv.GetLocalNetwork(resolver.Info.IP) + if err != nil { + log.Debugf("resolver: error checking local network for %s: %v", resolver.Info.IP, err) + return false + } + return net != nil +} func setScopedResolvers(resolvers []*Resolver) { localResolvers = make([]*Resolver, 0) systemResolvers = make([]*Resolver, 0) localScopes = make([]*Scope, 0) for _, resolver := range resolvers { - if resolver.Info.IPScope.IsLAN() { - localResolvers = append(localResolvers, resolver) - } else if net, _ := netenv.GetLocalNetwork(resolver.Info.IP); net != nil { + if isLocalResolver(resolver) { localResolvers = append(localResolvers, resolver) }This refactoring:
- Improves code readability
- Makes the logic more testable
- Centralizes the local resolver determination logic
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
service/resolver/resolvers.go(1 hunks)
🔇 Additional comments (1)
service/resolver/resolvers.go (1)
Line range hint 511-524: Verify impact on search domains and system resolvers
The changes to local resolver determination might affect how search domains are handled for these newly categorized local resolvers. Please verify that:
- Search domains are properly handled for resolvers that are local but not LAN
- System resolvers that are local but not LAN are properly categorized
✅ Verification successful
Let me gather more information about the search domain handling to make a conclusive verification.
Based on the gathered information, I can now provide a conclusive verification response.
The changes to local resolver handling are safe and maintain proper scope boundaries
The code maintains proper handling of search domains and system resolvers:
- Search domains are correctly processed regardless of resolver type (LAN or local network)
- System resolvers are properly categorized in a separate slice, independent of the local network detection
- The changes maintain the existing scope boundaries and DNS server horizon rules, as evidenced by the comprehensive scope checking throughout the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential impact on search domains and system resolvers
# Check for resolvers with search domains
ast-grep --pattern 'resolver.Search != nil {
$$$
}'
# Check for system resolver handling
rg -A 5 'ServerSourceOperatingSystem'
Length of output: 2586
Script:
#!/bin/bash
# Check how search domains are processed and added
ast-grep --pattern 'for _, search := range resolver.Search {
$$$
}'
# Check for local network handling
rg -A 5 'GetLocalNetwork'
# Check for scope handling
rg -B 2 -A 3 'IsLAN\(\)'
Length of output: 5989
|
Mistakenly closed by me. New PR: #1780 |
It seems to be a logical mistake.
Summary by CodeRabbit
Bug Fixes
Chores