Skip to content

fix(e2e): the squid probe's DNS retry was inert — retry in a fresh process (backend#2350) - #811

Merged
LukasWodka merged 1 commit into
developfrom
fix/2350-squid-dns-retry-inert
Aug 24, 2026
Merged

fix(e2e): the squid probe's DNS retry was inert — retry in a fresh process (backend#2350)#811
LukasWodka merged 1 commit into
developfrom
fix/2350-squid-dns-retry-inert

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Summary

E2E auth-proxy (squid) failed 2 of the last 30 develop runs, both on the same
named assertion:

✖ App pod WITH the ingestion proxy env did NOT tunnel through the squid
— ingestion-style backend egress is not proxied (the #119 bug).

The probe already carried retries meant to cover exactly the cluster-DNS startup
window, above a comment asserting that --retry-all-errors covered the
"Could not resolve proxy" case. It does not. curl caches a failed name
resolution for the life of the process, so one curl's --retry re-uses the
failure instead of re-querying the resolver.

Both failing runs say so in their own logs — 9 attempts, 8 of them answered from
the cache
:

* Could not resolve proxy: tb-egress-squid.default.svc.cluster.local
* Negative DNS entry
curl: (5) Could not resolve proxy: tb-egress-squid.default.svc.cluster.local

So exactly one resolver query was ever issued, about a second after the pod
started, and the whole guard turned on whether CoreDNS happened to be serving at
that instant. The documented protection was inert — a mechanism that looks like it
verifies something while being disconnected from what it claims to check
(backend#1729, the house family).

Evidence: A/B in the image the pod actually runs

curlimages/curl:latest, proxy name made resolvable 4 s into the run:

formoutcome
current — one curl, --retry 8 --retry-connrefused --retry-all-errorsall 9 attempts fail on the stale negative entry; never re-resolves
this PR — fresh curl process per attemptattempt 2 still rc=5, attempt 3 onward re-resolved (rc=7, refused — no squid in the harness)

curlimages/curl:8.20.0 (the version pinned elsewhere in e2e-common.sh) behaves
identically, so this was never a floating-tag regression — the claim was wrong
from the start. No image pin is included here for that reason.

The fix

The probe loops fresh curl processes, which cannot inherit the poisoned cache,
and prints attempt number, exit code and elapsed seconds on every attempt — so a
future red says whether it waited. The old failure could not distinguish
"did not tunnel" from "had not tunnelled yet": a negative is satisfied by
"not yet" exactly as well as by "never".

This is not a blind retry. Only exit 5/6/7 (unresolvable proxy,
unresolvable host, refused connection — the three startup-window symptoms) are
retried. Every other outcome including success ends the probe, so a real #119
regression — proxy env ignored, so the call dials direct and succeeds with no
CONNECT — still fails on attempt 1 rather than being retried into a slow green.
A squid that is genuinely down exhausts the deadline and fails.

Tests

The probe is now emitted from e2e-common.sh rather than written inline in the
pod manifest, so scripts/tests/e2e-proxy-probe.bats executes that same text and
not a paraphrase (backend#1729 rule 9). It runs in Unit tests, which is
required on develop — not in an advisory job (rule 2).

Six tests, each mutation-proved with the anchor asserted before reading the log:

mutationreddens
revert to one curl with in-process --retrytests 1, 2, 3, 4
retry every exit code (teeth removed)tests 1, 2, 3
drop the fail-closed host guardtest 5
remove the deadline / give-up reporttests 4, 6
drift the manifest indenttest 6

The exit-code domain is written down in the test independently of the case in
the snippet
, and both halves are exercised — retried and not-retried (rule 9
corollary: never test a list against itself). Test 6 renders the real egress-app
manifest out of e2e-proxy.sh using the script's own interpolation line and parses
it, so the probe is proven to land inside args rather than merely to be indented
by some number.

Verification

  • make lint — 54 shell scripts parse, shellcheck clean
  • shellcheck -S warning -x on both changed shell files — clean
  • awk -f unenforced-assertions.awk scripts/tests/*.bats — clean (every assertion enforcing)
  • bats scripts/tests/e2e-proxy-probe.bats — 6/6, plus the mutation matrix above

Not addressed here — and it needs a decision

The ticket's remaining question is whether this check should be required. It
cannot simply be promoted as-is, and that is worth its own look: installer-tests.yaml
carries paths: [scripts/**] and a branch list of main, develop, openshift
staging is absent. Details and measurements are in a comment on backend#2350.
No protection settings are touched by this PR.

Refs backend#2350.

🤖 Generated with Claude Code


Note

Low Risk
Test-only change to e2e probe retry logic; no production, auth, or data-path code. Worst case is a longer or still-flaky CI check.

Overview
Fixes the squid e2e §A probe so a transient “Could not resolve proxy” no longer fails the run. curl caches a failed lookup for the life of the process, so --retry-all-errors never re-queried DNS.

The probe is now emitted from e2e_proxy_probe_snippet and loops fresh curl processes. Only exits 5/6/7 (unresolvable proxy/host, connection refused) are retried until a deadline; success without a CONNECT still fails on attempt 1 so the #119 regression keeps its teeth. Attempts log rc and elapsed time.

Adds e2e-proxy-probe.bats that runs the same snippet (stubbed curl) and checks YAML interpolation so the probe actually lands in the pod args.

Reviewed by Cursor Bugbot for commit 43eb37d. Bugbot is set up for automated code reviews on this repo. Configure here.

…ocess (backend#2350)
`E2E auth-proxy (squid)` failed 2 of the last 30 develop runs, both times on
the same named assertion:
✖ App pod WITH the ingestion proxy env did NOT tunnel through the squid
The probe already carried retries meant to cover exactly the cluster-DNS
startup window, under a comment asserting that `--retry-all-errors` covered
the "Could not resolve proxy" case. It does not. curl caches a FAILED name
resolution for the life of the process, so a single curl's `--retry` re-uses
the failure instead of re-querying the resolver. Both failing runs show it —
nine attempts, eight of them answered from the cache:
* Could not resolve proxy: tb-egress-squid.default.svc.cluster.local
* Negative DNS entry
curl: (5) Could not resolve proxy: tb-egress-squid...
So exactly ONE resolver query was ever issued, about a second after the pod
started, and the guard turned entirely on whether CoreDNS happened to be
serving at that instant. Confirmed A/B in curlimages/curl:latest — the image
the pod runs — with the proxy name made resolvable 4s into the run: the
one-process form failed all 9 attempts on the stale negative entry, while the
fresh-process loop re-resolved on the very next attempt. 8.20.0 behaves the
same, so this was never a floating-tag regression; the claim was wrong from
the start.
The probe now loops fresh curl PROCESSES, which cannot inherit the poisoned
cache, and reports attempt number, exit code and elapsed seconds so a future
red says whether it waited — the old failure could not distinguish "did not
tunnel" from "had not tunnelled yet".
Not a blind retry: only exit 5/6/7 (unresolvable proxy, unresolvable host,
refused connection) are retried. Every other outcome, success included, ends
the probe, so a real #119 regression — proxy env ignored, so the call dials
direct and succeeds with no CONNECT — still fails on attempt 1 rather than
being retried into a slow green. A squid that is genuinely down exhausts the
deadline and fails.
The probe is emitted from e2e-common.sh rather than written inline in the pod
manifest so the new bats file executes that same text (backend#1729 rule 9),
and it runs in `Unit tests`, which is required on develop. Six tests, each
mutation-proved with the anchor asserted: reverting to the in-process retry,
retrying every exit code, dropping the fail-closed host guard, removing the
give-up report and drifting the manifest indent each redden the specific test
named for them.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@saqlainsyed007saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving. Reviewed last pass; CI is now green with no open threads.

The diagnosis is exact — curl caches a failed name resolution for the process lifetime, so the probe's in-process --retry re-used the negative DNS entry (one real resolver query the whole time), which is why the #119-bug guard turned on whether CoreDNS happened to be serving at that instant. The fix retries in a FRESH curl process so each attempt re-resolves, and the classification is right: transient resolve/connect codes (5/6/7) retry within a deadline (so a genuinely-down squid still exhausts and reddens), while terminal outcomes (0/22/28/35/56/60 — including the real proxy-tunnel result the test asserts) pass through unretried, so the fix can't convert a real red into a slow green. The bats harness pins the classification per rc. Green, mergeable, no threads.

@LukasWodka
LukasWodka merged commit ebb84b1 into developAug 24, 2026
49 checks passed
@LukasWodka
LukasWodka deleted the fix/2350-squid-dns-retry-inert branch August 24, 2026 12:00
aptracebloc added a commit that referenced this pull request Aug 27, 2026
…w (backend#2350) (#867)
The auth-proxy E2E shipped RED to staging on client#789 and blocked nothing
because it lived in installer-tests.yaml — an expensive, path-filtered workflow
whose jobs can never be required status checks (a required check behind a paths
filter never reports on a PR outside its paths, so GitHub holds the PR at
"waiting for status to be reported" forever; a guard that cannot block is advice,
not a gate — backend#1729).
Root cause of the red itself was diagnosed and already fixed on develop: the
squid probe's curl --retry was inert against curl's negative-DNS cache (#811),
and the probe/squid/nginx images are pinned (#813). This change does the
remaining enablement so the check can actually gate.
Extract the e2e-proxy job into e2e-auth-proxy.yaml, mirroring the move
Source-of-truth drift made out of installer-tests' old static job:
- pull_request has NO paths filter, so the context always reports and can be
required on staging/main without deadlocking a promotion PR that touches no
scripts/ file (backmerge, hotfix lane, version-bump promotion).
- push keeps a paths filter (pushes aren't gated) and adds staging; the Monday
cron is preserved so extracting the job doesn't drop its weekly canary.
- job name 'E2E auth-proxy (squid)' is unchanged, so the required-context name
and the release train's by-name check reading are preserved.
Arming it as a required check on staging + main is a follow-up branch-protection
setting, to be flipped after a green-watch window (arm while green). See
backend#2350.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@LukasWodka@saqlainsyed007