internal/cli: consolidate duplicated SNI-DNS check#528
Open
dolonet wants to merge 1 commit into
Open
Conversation
`doctor`'s checkSecretHost and the proxy-startup warnSNIMismatch each carried their own copy of the same logic: resolve the secret hostname, determine the server's public IPv4/IPv6 (config first, getIP fallback), and compare the two sets. Extract that data-gathering into runSNICheck (internal/cli/sni_check.go), returning an sniCheckResult. The success decision stays with each caller because the rules genuinely differ — `doctor` reports OK when any family matches, while the startup warning requires every detected family to match — so only the gathering is shared, not the verdict. No behavior change: both callers produce byte-identical output and the same return values as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pure refactor extracted from the now-closed #474, against current master (so it sits cleanly on top of #505).
Problem
Doctor.checkSecretHost(internal/cli/doctor.go) andwarnSNIMismatch(internal/cli/run_proxy.go) each carry their own copy of the same routine:getIPfallback;Three steps, two copies, kept in sync by hand.
Change
The data-gathering moves into
runSNICheck(newinternal/cli/sni_check.go), which returns ansniCheckResult— resolved records, the two public IPs, per-family match booleans, and any resolve error.The success verdict stays with each caller, on purpose: the two are not the same check.
doctorreports ✅ when any family matches; the startup warning stays silent only when every detected family matches. Sharing the verdict would silently change one of them — so only the gathering is consolidated.No behavior change
Both callers produce byte-identical output and the same return values as before:
doctor— same ✅/❌ templates (including doctor: surface both public IPs in SNI-DNS mismatch message #505's wording), same matched-IP shown in the success line, same return bool.warnSNIMismatch— same log lines, sameipv4_match/ipv6_matchbindings, same early-returns.getIPis unchanged and still called sequentially per family.Verification
go build ./...,go vet ./...,go test ./...— all clean.gofmtclean on the three touched files (the one unrelated alignment nitgofmtreports indoctor.gois pre-existing in theDoctorstruct tags and left untouched).Splitting note: #474 also proposed a
network.public-ip-endpointsconfig option. That's a config-surface change rather than a refactor, so it is intentionally not here — I'll raise it separately for your call.