Better connectivity check for DefaultResolvers #453
Conversation
…ectivity. added tests for checkConnectivity
…sn't detect either
|
@Mzack9999 no, this doesn't do any DNS queries. |
WalkthroughInit probes runtime IPv4/IPv6 reachability and builds Changes
Sequence Diagram(s)sequenceDiagram
participant Init as init()
participant Probe as availableIpVersions()
participant Check as checkDialConnectivity()
participant Net as net.Dial
rect `#F5F8FF`
note over Init: startup — detect reachable IP versions
Init->>Probe: availableIpVersions()
end
par IPv6 probe
Probe->>Check: checkDialConnectivity(IPv6Resolvers, proto)
Check->>Net: concurrently dial IPv6 endpoints
Net-->>Check: success / failure
Check-->>Probe: hasV6
and IPv4 probe
Probe->>Check: checkDialConnectivity(IPv4Resolvers, proto)
Check->>Net: concurrently dial IPv4 endpoints
Net-->>Check: success / failure
Check-->>Probe: hasV4
end
rect `#F5FFF5`
Probe-->>Init: (hasV6, hasV4)
end
alt hasV6 && hasV4
Init->>Init: append IPv6Resolvers then IPv4Resolvers
else hasV6 only
Init->>Init: append IPv6Resolvers
else hasV4 only
Init->>Init: append IPv4Resolvers
else Neither
Init->>Init: append both IPv6Resolvers and IPv4Resolvers (fallback)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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 (2)
cdncheck_test.go (1)
45-68: Consider decouplingTestConnCheckValidfrom real network availability
TestConnCheckValidasserts actual dialability ofDefaultResolvers(and some invalid addresses), so it can fail in offline or heavily firewalled CI environments even when the library logic is correct. If you start seeing flaky builds, consider:
- Injecting a dial function into
checkDialConnectivityso tests can stub it, or- Guarding this test behind an env flag and skipping it when external connectivity isn’t guaranteed.
Functionally the assertions match the current implementation; this is just about long‑term test robustness.
cdncheck.go (1)
36-65: Code is safe on Go 1.24.0; refactoring recommended as best practiceThe
checkDialConnectivityfunction at lines 36-65 does not have a critical bug on the current Go version (1.24.0). Go 1.22 introduced per-iteration scope for range variables, making the closure capture safe. The code works correctly as-is.However, the proposed refactoring—passing
IPas a parameter—is still recommended as a best practice for code clarity and portability. It makes the captured value explicit and avoids surprises if the code is later compiled with older Go versions.for _, IP := range IPs { wg.Add(1) - go func(){ + go func(ip string) { defer wg.Done() - conn, err := net.DialTimeout(proto, IP, 3*time.Second) + conn, err := net.DialTimeout(proto, ip, 3*time.Second) if conn != nil { defer conn.Close() } results <- err == nil - }() + }(IP) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cdncheck.go(2 hunks)cdncheck_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cdncheck_test.go (1)
cdncheck.go (1)
DefaultResolvers(18-18)
⏰ 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). (3)
- GitHub Check: Test Builds (macOS-latest, 1.24.x)
- GitHub Check: Test Builds (windows-latest, 1.24.x)
- GitHub Check: Functional Test (windows-latest)
🔇 Additional comments (3)
cdncheck.go (2)
18-34: Dynamic resolver wiring and init fallback look correct
DefaultResolversbeing populated ininit()fromIPv6Resolvers/IPv4Resolversbased on detected availability, with a final fallback to appending both lists when neither protocol is detected, matches the PR description and preserves a sane default even when connectivity checks fail. The newIPv4Resolverslist is consistent with the existing IPv6 list and keeps defaults explicit and discoverable.Also applies to: 92-107
67-89: IPv4/IPv6 detection and resolver selection behavior aligns with intent
availableIpVersionsprobing IPv6 and IPv4 concurrently andinit()using those flags to include the corresponding resolver lists (with a fallback to both when neither probe succeeds) gives the desired behavior for IPv6-only, IPv4-only, dual-stack, and “detection failed” environments. The concurrency and simple boolean flags keep startup overhead bounded while avoiding unnecessary DNS queries.cdncheck_test.go (1)
10-18: Test helper for rewriting resolver ports is clear and fineThe
allHostportsWithPortForTesthelper cleanly rewrites resolver hostports to a custom port for connectivity testing and keepsTestConnCheckValidreadable. Panicking on split errors is acceptable in this test-only utility.
…ectivity. added tests for checkConnectivity
…sn't detect either
…just opens socket
dogancanbakir
force-pushed
the
better_conn_check
branch
from
557f338 to
a96c0a1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cdncheck_test.go (2)
10-21: Prefer test failures over panics in helper
allHostportsWithPortForTestpanics onSplitHostPorterrors. It’s fine for internal constants but harder to debug if something changes. Consider passing*testing.Tin and usingrequire.NoError(t, err)instead ofpanic, so failures are reported as regular test failures.
48-71: Network-dependent test may be flaky across environments
TestConnCheckValidassumes successful UDPnet.DialTimeoutto public resolvers (including port10000) and failure to the[::]addresses. In CI environments without IPv6 or with restricted outbound UDP, this can intermittently fail even though the code is correct.You may want to gate or soften this test (e.g., skip if
testing.Short(), or behind an env flag) so basic unit test runs don’t depend on external network conditions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cdncheck.go(2 hunks)cdncheck_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cdncheck_test.go (1)
cdncheck.go (1)
DefaultResolvers(18-18)
🔇 Additional comments (1)
cdncheck.go (1)
18-34: Resolver initialization and IP-version probing behavior looks soundThe new
availableIpVersions+init()flow (populateDefaultResolversfromIPv6Resolvers/IPv4Resolvers, fallback to both on detection failure) matches the PR description and should behave correctly across IPv4-only, IPv6-only, dual-stack, and “no outbound” environments, assuming the underlyingnet.DialTimeoutcalls can complete.No changes needed here beyond the
checkDialConnectivityfix above.Also applies to: 67-88, 92-107
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cdncheck.go (1)
36-65: Goroutine capture issue resolved; consider parameter naming.The critical goroutine loop variable capture issue from the previous review has been correctly fixed by passing
IPas a parameter.Minor style note: the parameter name
IP(uppercase) is unconventional in Go. Consider using lowercaseipfor consistency with Go idioms, as uppercase names typically indicate exported identifiers.Optional: idiomatic parameter naming
-func checkDialConnectivity(IPs []string, proto string) bool { +func checkDialConnectivity(ips []string, proto string) bool { var wg sync.WaitGroup - results := make(chan bool, len(IPs)) + results := make(chan bool, len(ips)) - for _, IP := range IPs { + for _, ip := range ips { wg.Add(1) - go func(IP string){ + go func(ip string){ defer wg.Done() - conn, err := net.DialTimeout(proto, IP, 3*time.Second) + conn, err := net.DialTimeout(proto, ip, 3*time.Second) if conn != nil { defer conn.Close() } results <- err == nil - }(IP) + }(ip) }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cdncheck.go(2 hunks)cdncheck_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cdncheck_test.go
⏰ 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: Test Builds (windows-latest, 1.24.x)
🔇 Additional comments (3)
cdncheck.go (3)
18-18: LGTM: Dynamic initialization approach.Switching from a hardcoded list to runtime population based on connectivity is a solid improvement.
28-34: LGTM: Reliable IPv4 resolvers.The IPv4 resolver list includes well-known, trustworthy public DNS servers (Cloudflare and Google).
92-108: LGTM: Sensible fallback strategy.The init logic correctly appends resolvers based on detected connectivity and provides a reasonable fallback (both IPv6 and IPv4) when detection yields no results, ensuring
DefaultResolversis never empty.
…dp 53 then make sure someone knows it's not actually sending data
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cdncheck.go (2)
67-89: Consider testing multiple resolvers per protocol for more robust detection.The function tests only a single IP address per protocol (Google DNS at lines 73 and 81). If that specific server is unreachable (e.g., due to firewall rules, routing issues, or regional blocking), the detection will fail even though other resolvers might be reachable. This could lead to suboptimal resolver selection.
While the fallback logic in
init()(lines 104-107) mitigates this by including both resolver sets when detection fails, testing against multiple IPs would provide more accurate detection of actual connectivity.🔎 Suggested enhancement to test multiple IPs
func availableIpVersions() (hasV6 bool, hasV4 bool) { var wg sync.WaitGroup wg.Add(1) go func(){ defer wg.Done() - if checkDialConnectivity([]string{"[2001:4860:4860::8888]:53"}, "udp") { + if checkDialConnectivity([]string{ + "[2001:4860:4860::8888]:53", + "[2606:4700:4700::1111]:53", + }, "udp") { hasV6 = true } }() wg.Add(1) go func(){ defer wg.Done() - if checkDialConnectivity([]string{"8.8.8.8:53"}, "udp") { + if checkDialConnectivity([]string{ + "8.8.8.8:53", + "1.1.1.1:53", + }, "udp") { hasV4 = true } }() wg.Wait() return hasV6, hasV4 }
92-108: LGTM! Init logic correctly implements dynamic resolver selection.The initialization correctly:
- Detects available IP versions concurrently
- Populates DefaultResolvers based on detected connectivity
- Falls back to including both resolver sets if detection fails
This ensures DefaultResolvers is never empty while preferring resolvers that are more likely to be reachable based on the initial connectivity checks.
Optional nitpick: Line 104 uses
len(DefaultResolvers) <= 0but slice length cannot be negative, so== 0would be more idiomatic. This is purely a style consideration and doesn't affect functionality.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cdncheck.go
⏰ 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: Test Builds (macOS-latest, 1.24.x)
- GitHub Check: Test Builds (windows-latest, 1.24.x)
🔇 Additional comments (2)
cdncheck.go (2)
28-34: Good addition of trusted IPv4 resolvers.The IPv4Resolvers list mirrors the structure of IPv6Resolvers and uses well-known, reliable public DNS servers (Cloudflare and Google).
36-65: LGTM! Loop variable capture fixed correctly.The goroutine now properly captures the loop variable by passing it as a parameter (line 47), addressing the critical issue from the previous review. The implementation correctly tests concurrent connectivity, with proper resource cleanup and a buffered channel to prevent goroutine leaks.
|
The existing functionality checks if ipv6 stack is enabled and in case add ipv6 resolvers. For backward compatibility we are fine with the assumption that ipv4 is always available and only ipv6 needs a pre-flight check, and in case we are able to perform any connection to an Ipv6 address we add the trusted resolvers. Closing the PR as not planned |
2 participants
this PR makes it check both IPv6 and IPv4 at the same time and falls back to both if neither is detected.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.