-
Notifications
You must be signed in to change notification settings - Fork 876
grpcutil: keep cached endpoints on SRV-target A-record failure (#7730) - #7777
grpcutil: keep cached endpoints on SRV-target A-record failure (#7730) #7777mehrdadbn9 wants to merge 6 commits into
Conversation
trimStringByBytes scans backwards from the truncation point to find a
UTF-8 rune start, but the loop had no lower bound. When a request field
consists only of UTF-8 continuation bytes (0x80-0xBF) there is no rune
start to land on, so size underflows past zero and bytesStr[-1] panics
with 'index out of range [-1]'. Because the active query tracker is
enabled by default and the panic happens on a goroutine without recover(),
an attacker-supplied match[]/query value can crash the querier.
Bound the scan with 'size > 0' so it stops at the start of the string
instead of underflowing. When no rune start exists the field is truncated
to an empty string, which is safe.
Add TestTrimForJsonMarshalContinuationBytes covering the all-continuation-
byte case; existing multi-byte tests use valid UTF-8 ('世') which always
terminates the scan at index 0 and therefore never exercised the underflow.
Fixes cortexproject#7729
Signed-off-by: ...
Signed-off-by: Mehrdad Biukian Naeini <mehrdad.biu@mtnirancell.ir>
...roject#7731) Signed-off-by: Mehrdad Biukian Naeini <mehrdad.biu@mtnirancell.ir>
...#7731) Signed-off-by: Mehrdad Biukian Naeini <mehrdad.biu@mtnirancell.ir>
Signed-off-by: Mehrdad Biukian Naeini <mehrdad.biu@mtnirancell.ir>
...xproject#7730) Signed-off-by: Mehrdad Biukian Naeini <mehrdad.biu@mtnirancell.ir>
mehrdadbn9
commented
Aug 17, 2026
CI note: the integration_querier failure on ubuntu-24.04-arm (arm64) is an infra flake, not a regression from this change.
Evidence:
- The failing job's log shows the test harness could not reach its own Docker backing services:
Error response from daemon: No such container: e2e-cortex-test-consul/minio/querier/ingester/...anddial tcp 172.18.0.2:8500: connection refused. The Go tests that don't need the stack (TestQuerierWithBlocksStorageLimits,TestHashCollisionHandling,TestQuerierMaxSamplesLimit,TestQuerierDistributedExecution, ...) still PASS. - The amd64
integration_querierrun (same code) PASSED. A logic regression would fail on both arches.
This change only touches the dnssrv:///dnssrvnoa:// SRV-target-failure path in pkg/util/grpcutil/dns_resolver.go (returns nil so cached endpoints are kept when SRV records exist but none of their targets resolve). That path is not exercised by the integration stack tests, so it cannot be the cause.
Could a maintainer please re-run the failed integration_querier arm64 job? Thanks!
mehrdadbn9
commented
Aug 22, 2026
Follow-up on the earlier CI note — root-caused the [Parquet] blocks sharding enabled, memcached parquet cache arm64 failure precisely:
Failure point: integration/querier_test.go:236 — the test waits cortex_parquet_converter_blocks_converted_total == 2*NumInstances() (=4 with the 2-replica composite cluster). The log shows the converter had already converted both uploaded blocks (Last values: [2]), i.e. each block converted once via the sharded converter ring, and the wait simply timed out before replicas re-observed/converted their ring-owned blocks.
Why it's environmental, not from this PR: this PR touches only ingester metrics labels, distributor queryable dedup, grpcutil DNS resolver, and request-tracker extraction — nothing in the parquet conversion path (pkg/parquetconverter/ untouched; verified via git diff upstream/master...HEAD). The identical test passes on current master's arm64 integration_querier job (run 32523725801, all arm jobs green).
Ask: /retest — if it reproduces consistently I'm happy to dig further, but the evidence points to a slow-converter timing flake on arm64.
Signed-off-by: Mehrdad Biukian Naeini <mehrdadbiukian@gmail.com>
Summary
Fixes #7730.
The gRPC DNS watcher (
dnssrv:///dnssrvnoa://) takes the SRV path inlookup(). #7698 madelookup()keep cached endpoints when both the SRVand A-record lookups fail, but the SRV path was still vulnerable: when the
SRV query succeeds but every target's A-record resolution fails transiently,
lookupSRV()returned a non-nil empty map.lookup()then neither fellback to the A-record path nor took the "keep cached" early return, and
proceeded to
compileUpdate(empty), deleting every known endpoint.Solution
lookupSRV()now returnsnil(not an empty map) when SRV records existedbut none of their targets resolved, so
lookup()keeps the previouslycached endpoints — matching the A-record path contract. A genuine
zero-record SRV result (
srvsempty) still returns the empty map and ishandled as before.
Verification
Added
TestDNSWatcher_LookupSRV_TransientTargetFailureRetainsCache, whichstubs the SRV query to return a record whose target A-lookup fails and
asserts
lookup()returnsniland retainscurAddrs. The existingTestDNSWatcher_Lookup_TransientFailureRetainsCache(A-record path) stillpasses.
go test ./pkg/util/grpcutil/ -run TestDNSWatcher -count=1Fixes #7730
Signed-off-by: mehrdadbn9 mehrdadbn9@users.noreply.github.com