Skip to content

Fix DNS watcher SRV path wiping all endpoints when every target lookup fails - #7745

Open
sandy2008 wants to merge 2 commits into
cortexproject:masterfrom
sandy2008:fix/7730-dns-srv-target-failures
Open

Fix DNS watcher SRV path wiping all endpoints when every target lookup fails#7745
sandy2008 wants to merge 2 commits into
cortexproject:masterfrom
sandy2008:fix/7730-dns-srv-target-failures

Conversation

@sandy2008

Copy link
Copy Markdown
Contributor

What this PR does:

lookupSRV() in the gRPC DNS watcher (pkg/util/grpcutil/dns_resolver.go) pre-allocates its result map and continues when an SRV target's A record lookup fails, so an SRV response whose targets all fail to resolve produced a non-nil empty map. lookup() treated that as an authoritative empty result and emitted Delete updates for every previously known endpoint — the same wipe-on-transient-failure that #7698 fixed for the plain A record path.

Changes:

  • lookupSRV() now honours the same nil-means-failure contract as lookupHost() (the contract is now documented on both functions): if the SRV response contained resolvable targets but none of them yielded a usable address, it returns nil. lookup() then falls back to the plain host A record lookup and, if that also fails, retains the previously resolved endpoints.
  • SRV targets of "." (RFC 2782: service decidedly not available) are treated as an authoritative empty answer rather than a failed target lookup: they still clear all endpoints and never trigger the host fallback. Previously they only behaved correctly by accident (the . lookup happened to fail); with the new guard they need explicit handling.
  • Fixes the level.Error call in the SRV loop that was missing its "msg" key (go-kit's logfmt encoder rejected the line and logged nothing), matching the A record path.

Semantics worth calling out explicitly for review:

  • Cached endpoints are retained only when both the SRV target resolution and the host A record fallback fail. If the watched host itself resolves, the balancer gets those A record addresses on the watcher's port, which may differ from the SRV-advertised ports. This matches the watcher's pre-existing behavior when the SRV query itself fails; if you would rather retain the cache immediately (skipping the A fallback) when SRV targets fail, I'm happy to adjust — TestDNSWatcher_Lookup_SRVAllTargetsFailFallsBackToHost pins the current choice either way.
  • Reachability: no in-tree caller currently passes a non-empty service to Resolver.Resolve (the DNS watcher used by the querier worker and query-frontend v2 passes ""), so this is a latent contract fix in the copied grpc-go resolver rather than a live production outage — the issue's original repro steps involving dnssrv:// addresses actually route through the vendored Thanos DNS provider, which is unaffected.
  • An alternative design — returning an explicit (addrs, ok bool) pair from both helpers instead of the nil sentinel — was considered and set aside to keep the diff minimal and consistent with Fix DNS watcher from clearing known endpoints on lookup failure #7698; the nil-vs-empty contract is now documented on both functions instead.

Testing: the two behavior-changing scenarios have red→green regression tests (TestDNSWatcher_Lookup_SRVTargetTransientFailureRetainsCache, TestDNSWatcher_Lookup_SRVTargetDotClearsCache, and TestDNSWatcher_Lookup_SRVTargetFailureAcrossPolls all fail on master); the remaining tests pin the pre-existing behaviors the guard must not change (partial target success, genuine zero-record responses, SRV-query-failure fallback). Full package passes with -tags "netgo slicelabels" and with -race.

Which issue(s) this PR fixes:
Fixes #7730

Checklist

  • Tests updated
  • Documentation added (n/a — no config or flag changes)
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]
  • docs/configuration/v1-guarantees.md updated if this PR introduces experimental flags (n/a)

Per the Generative AI Contribution Policy: this change was developed with substantial AI assistance (multiple independent AI implementations reconciled and then adversarially cross-reviewed, under my direction). I have reviewed and validated all of it and take responsibility for its correctness.

The SRV path of the gRPC DNS watcher returned a non-nil empty address map
when the SRV query succeeded but no usable address was obtained from its
targets, so lookup() treated it as an authoritative empty result and
deleted every previously known endpoint. This is the same failure mode
that cortexproject#7698 fixed for the plain A record path.

lookupSRV() now honours the same nil-means-failure contract as
lookupHost(), documented on both functions: if the SRV response contained
resolvable targets but none of them yielded a usable address, it returns
nil, so lookup() falls back to the plain host A record lookup and, if that
also fails, retains the previously resolved endpoints. Note the fallback
addresses use the watcher's port, which may differ from the SRV-advertised
ports; this matches the existing behavior when the SRV query itself fails.

SRV targets of "." (RFC 2782: service decidedly not available) are now
treated as an authoritative empty answer rather than a failed target
lookup, so they still clear all endpoints and never trigger the host
fallback. A genuine zero-record SRV response keeps doing the same.

Signed-off-by: Sandy Chen <Yuxuan.Chen@morganstanley.com>
Signed-off-by: Sandy Chen <Yuxuan.Chen@morganstanley.com>
@sandy2008
sandy2008 marked this pull request as ready for review August 3, 2026 02:43
@dosubot dosubot Bot added go Pull requests that update Go code type/bug type/tests labels Aug 3, 2026

@SungJin1212 SungJin1212 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Aug 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

go Pull requests that update Go code lgtm This PR has been approved by a maintainer size/L type/bug type/tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DNS watcher still clears all endpoints on the SRV path when target A-record lookups fail (incomplete #7698 fix)

2 participants