Deflake Envoy AllowPort spec's allowed-port leg - #6077
Closed
JAORMX wants to merge 1 commit into
Closed
Conversation
The AllowPort spec fetched https://example.com exactly once, immediately after workload readiness. The allowed leg needs a working DNS -> TLS -> upstream chain through Envoy's dynamic_forward_proxy, and the first egress request after startup can race the isolation stack's own warm-up, failing instantly with a local error before anything reaches the network. Observed twice on 2026-07-28 (PR heads 1588e6a and 5c901a9, within the same minute): IsError ~40ms and ~190ms after readiness — far too fast for genuine external TLS trouble — while the same spec passed on main minutes later. The property this spec pins is steady-state reachability of an allowed port; first-request-after-boot success is not something the DFP path promises. Retry the allowed leg until the deadline, and surface the fetch tool's error text on failure — the diagnostic that distinguishes a warm-up race from a genuinely broken AllowPort, which the old boolean-only assertion discarded. A broken AllowPort still fails, now with evidence. The blocked port-80 leg is unchanged: denial is enforced locally and deterministically once the stack is warm, which the preceding allowed leg has just proven. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam
JAORMX
requested review from
ChrisJBurns,
amirejaz,
aponcedeleonch,
jhrozek,
rdimitrov and
reyortiz3
as code owners
July 28, 2026 07:45
jhrozek
approved these changes
Jul 28, 2026
Collaborator
Author
|
Superseded by #6076: the maintainer asked for the statuses fix and this deflake to land as a single PR, so this change (identical commit) is now the second commit there. Closing. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6077 +/- ##
==========================================
+ Coverage 72.25% 72.30% +0.04%
==========================================
Files 724 725 +1
Lines 75348 75358 +10
==========================================
+ Hits 54445 54486 +41
+ Misses 17022 16971 -51
- Partials 3881 3901 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Summary
E2E Tests Core (network-isolation)failed on the pre-merge heads of both #6050 (1588e6a4) and #6051 (5c901a9d) — the latter containing zero production code — and passed onmainminutes later. Both failures were the same spec, same assertion, within the same minute: the AllowPort spec's allowed leg,https://example.com must be allowed (port 443 is in AllowPort).Root cause: the allowed leg fetched
https://example.comexactly once, immediately after workload readiness. It needs a working DNS → TLS → upstream chain through Envoy'sdynamic_forward_proxy, and the first egress request after startup can race the isolation stack's own warm-up (DFP DNS cache, the per-workload DNS container), failing instantly with a local error. The observed failures returned ~40ms and ~190ms after readiness — far too fast for genuine external TLS trouble — and the boolean-only assertion discarded the fetch tool's error text, so the log could not say why.What changed:
Eventually(60s budget, 2s poll) and records the last fetch error text, which is printed on failure.Why this is fixing an incidental assertion, not weakening a real one:
AllowPortend-to-end) is steady-state reachability of an allowed port. First-request-after-boot success is not something Envoy's DFP path promises, so a single-shot first fetch asserts warm-up behaviour the product never guaranteed.AllowPortstill fails — after the retry budget, and now with the fetch error text surfaced, which is the diagnostic that distinguishes a warm-up race from a real enforcement break on the next occurrence.No timeout was raised; the change addresses the mechanism (first-request warm-up) and improves the failure's diagnosability.
Type of change
Test plan
task lint;go vet ./...run separately)Test-only change to a suite that requires a container runtime, which the dev environment lacks — the retry's effect on the flake is only observable in CI. Verified by compilation (
go vet ./test/e2e/), lint, and by checking the failure-time evidence (both CI logs) against the retry semantics: the observed failure mode (instantIsErrorright after readiness) is exactly what the retry absorbs, and the assertion's contract is unchanged.Does this introduce a user-facing change?
No.
Special notes for reviewers
The warm-up diagnosis is the best fit for the evidence (instant local failure, correlated timing across two runners, immediate pass on
main) but could not be pinned to the exact Envoy error because the old assertion discarded the error text. If this spec ever fails again, the surfaced text settles it.Generated with Claude Code