cdncheck icon indicating copy to clipboard operation
cdncheck copied to clipboard

Better connectivity check for DefaultResolvers

Open BPplays opened this issue 4 months ago β€’ 2 comments

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

    • Dynamic DNS resolver selection that detects available IP versions (IPv4/IPv6) at runtime and adapts the resolver list.
    • Runtime connectivity checks to choose reachable resolvers and an automatic fallback to ensure resolution when one IP family is unavailable.
  • Tests

    • Added connectivity tests validating resolver reachability across protocols and ports.

✏️ Tip: You can customize this high-level summary in your review settings.

BPplays avatar Sep 19 '25 16:09 BPplays

@Mzack9999 no, this doesn't do any DNS queries.
the reason i made this was because the original implementation only checks for IPv6 and i thought it might be useful to check IPv4 too for IPv6-only environments, but i can also add a check for unresponsive resolvers if you think it would be a good idea.

BPplays avatar Nov 19 '25 11:11 BPplays

Walkthrough

Init now probes runtime IPv4/IPv6 reachability and dynamically builds DefaultResolvers from IPv6Resolvers and a new IPv4Resolvers; probing uses concurrent dial checks via checkDialConnectivity and availableIpVersions, with a fallback to include both resolver lists when detection yields no results.

Changes

Cohort / File(s) Summary
Connectivity Detection & Resolver Initialization
cdncheck.go
Added IPv4Resolvers and changed DefaultResolvers to a nil slice populated at init. Introduced checkDialConnectivity(IPs []string, proto string) and availableIpVersions() to probe network reachability concurrently. Removed checkIPv6Connectivity() and updated init() to conditionally append IPv6/IPv4 resolvers, falling back to both when none detected.
Tests: Connectivity checks
cdncheck_test.go
Added TestConnCheckValid() to validate checkDialConnectivity() behavior: expecting success for DefaultResolvers over UDP and failure for provided IPv6 placeholder addresses over TCP.

Sequence Diagram(s)

sequenceDiagram
    participant Init as init()
    participant Probe as availableIpVersions()
    participant Check as checkDialConnectivity()
    participant Net as net.Dial

    rect rgb(245,248,255)
    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 rgb(245,255,245)
    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

  • Check concurrency, dial timeouts, and goroutine handling in checkDialConnectivity.
  • Verify race-safety and aggregation logic in availableIpVersions.
  • Confirm init() ordering and fallback behavior.
  • Review test for network flakiness and deterministic assertions.

Poem

πŸ‡ I sniffed the cables, twitching nose,
I hopped from v6 to v4 in rows.
With quick little dials I check each way,
pick resolvers that answer 'hey'.
If silence comes, I bring them both to play.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
βœ… Passed checks (2 passed)
Check name Status Explanation
Description Check βœ… Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check βœ… Passed The title accurately describes the main change: improving the connectivity check logic for DefaultResolvers by adding concurrent IPv6/IPv4 checks and fallback behavior.
✨ Finishing touches
  • [ ] πŸ“ Generate docstrings
πŸ§ͺ Generate unit tests (beta)
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

πŸ“œ Recent review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

πŸ“₯ Commits

Reviewing files that changed from the base of the PR and between 0479207ef5ee608a87651b918197e8d4fa84fa36 and 48a1123527e6d1b80d93487f9df43b4ce3320134.

πŸ“’ Files selected for processing (1)
  • cdncheck_test.go (1 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)

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.

❀️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Nov 20 '25 13:11 coderabbitai[bot]