Skip to content

fix(e2e): RateLimit flaky - #9911

Merged
zhaohuabing merged 3 commits into
envoyproxy:mainfrom
zirain:e2e/ratelimit
Sep 10, 2026
Merged

fix(e2e): RateLimit flaky#9911
zhaohuabing merged 3 commits into
envoyproxy:mainfrom
zirain:e2e/ratelimit

Conversation

@zirain

@zirain zirain commented Sep 1, 2026

Copy link
Copy Markdown
Member

Fixes: #9994 #9993 #9992

Problem

The rate-limit e2e suite failed intermittently in CI with assertions like "expected 429, got 200" or "expected N matching responses, got M" — not hard errors, just occasional wrong-looking traffic outcomes. Several independent races were compounding to cause this, so tests could fail differently from run to run.

Root causes fixed

  1. Ratelimit config-sync race. BackendTrafficPolicyMustBeAccepted only confirms EG's controller finished translating a policy — not that the separate envoy-ratelimit service has received and applied the descriptor config for that domain over its own xDS stream (internal/globalratelimit/runner). Sending traffic before that lands looks like "rate limiting isn't working" (unmatched domain = unlimited, not rejected).
  2. Gateway pod-readiness race. The Gateway/Service being Accepted (and getting an address via MetalLB) happens almost instantly, well before the newly-provisioned Envoy Proxy pod behind it is actually Ready — first request(s) could race pod startup.
  3. Envoy→ratelimit-service RPC timeout too tight for CI. rateLimit.timeout defaults to 20ms when unset (api/v1alpha1/envoygateway_types.go), tuned for a colocated, low-latency deployment. That call also covers the ratelimit service's own Redis round trip, which can exceed 20ms under CI load. Since failClosed defaults to false, a timed-out check fails open (request let through unlimited) instead of erroring — the flaky extra 200 where a 429 was expected.
  4. Shared/collision-prone test fixtures. Several tests reused generic Gateway names (same-namespace, eg-rate-limit) and paths (/), so concurrent or back-to-back tests could race on the same Gateway object, listener domain, or rate-limit budget.
  5. Single-replica assumption in metrics helper. OverLimitCount only ever port-forwarded to pods.Items[0], undercounting when gateway-namespace-mode.yaml runs the ratelimit deployment with 2 replicas.

Changes

  1. New polling helpers (utils.go):
    WaitForGatewayPodsReady — waits for the Envoy Proxy pod(s) behind a given Gateway to reach Ready before sending traffic.
  2. DumpRateLimitConfig — fetches the envoy-ratelimit service's /rlconfig debug endpoint (port 6070), the same one egctl config envoy-ratelimit uses.
    WaitForRateLimitDomainToBeLoaded / WaitForRateLimitDomainsToBeLoaded — polls /rlconfig until the domain(s) a test depends on are actually loaded, with a bounded (3s) HTTP client per attempt so a single stuck call can't hang the whole retry loop.
  3. RateLimitListenerDomain / RateLimitSharedDomain — build EG's internal rate-limit domain names correctly for non-shared (listener-keyed) vs. shared (policy-keyed) rules, so tests can wait on the exact right domain.
    OverLimitCount now sums the metric across all envoy-ratelimit pods instead of just the first, via extracted getOverLimitCount.
    1 rateLimit.timeout: 1s added to all five e2e EnvoyGateway configs (test/config/envoy-gateaway-config/*.yaml), replacing the 20ms default, with an inline comment explaining why.
  4. Test isolation: nearly every rate-limit test (test/e2e/testdata/ratelimit-*.yaml) now gets its own uniquely-named Gateway and request path instead of sharing same-namespace/eg-rate-limit//, and every test now calls the new WaitForGatewayPodsReady / WaitForRateLimitDomain(s)ToBeLoaded helpers before firing traffic (ratelimit.go).
  5. Fail-fast-on-first-error assertions: swapped t.Errorf for tlog.Fatalf (→ t.Fatal) on request-count/response assertions, so a subtest stops immediately on the first wrong outcome instead of cascading into confusing secondary failures (e.g. querying Prometheus after the traffic assertion already failed).
  6. Minor: three ConformanceTest vars renamed for naming consistency (...Test suffix dropped to match ShortName); suiteOpts.FailFast commented out in e2e_test.go (worth double-checking before merge — this was useful for surfacing every flaky test during debugging, but may want re-enabling for normal CI runs).

@netlify

netlify Bot commented Sep 1, 2026

Copy link
Copy Markdown

Deploy Preview for cerulean-figolla-1f9435 ready!

Name Link
🔨 Latest commit e04b118
🔍 Latest deploy log https://app.netlify.com/projects/cerulean-figolla-1f9435/deploys/6aa25f3cc840a40008b3fa8d
😎 Deploy Preview https://deploy-preview-9911--cerulean-figolla-1f9435.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.31%. Comparing base (de29eb6) to head (e04b118).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/kubernetes/port_forwarder.go 0.00% 5 Missing ⚠️

❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #9911      +/-   ##
==========================================
- Coverage   81.33%   81.31%   -0.03%     
==========================================
  Files         264      264              
  Lines       40963    40967       +4     
==========================================
- Hits        33319    33311       -8     
- Misses       7643     7655      +12     
  Partials        1        1              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zirain
zirain force-pushed the e2e/ratelimit branch 6 times, most recently from 10da35a to f373a67 Compare September 2, 2026 07:24
@zirain
zirain marked this pull request as ready for review September 3, 2026 03:48
@zirain
zirain requested a review from a team as a code owner September 3, 2026 03:48

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a4a03fe0e6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread test/e2e/tests/ratelimit.go Outdated
Comment thread test/e2e/tests/utils.go Outdated
@zirain
zirain marked this pull request as draft September 3, 2026 08:40
@zirain
zirain force-pushed the e2e/ratelimit branch 9 times, most recently from fb742f1 to bd5c301 Compare September 10, 2026 03:57
Signed-off-by: zirain <zirain2009@gmail.com>
@zirain
zirain marked this pull request as ready for review September 10, 2026 05:50

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b63ff6af32

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread test/e2e/tests/utils.go Outdated
Signed-off-by: zirain <zirain2009@gmail.com>
@rudrakhp
rudrakhp requested a balanced review from Copilot September 10, 2026 06:41

Copilot AI 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.

🟡 Changes recommended

Port-forward startup remains unbounded, and the normal E2E fail-fast behavior is disabled.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Improves rate-limit E2E reliability by synchronizing gateway and rate-limit readiness, isolating fixtures, and increasing CI timeout tolerance.

Changes:

  • Adds gateway-pod and rate-limit-config readiness polling.
  • Isolates rate-limit fixtures with dedicated Gateways and paths.
  • Aggregates replica metrics and increases rate-limit RPC timeouts.
File summaries
File Description
test/e2e/tests/utils.go Adds readiness/config helpers and multi-pod metrics.
test/e2e/tests/ratelimit.go Uses readiness waits and fail-fast assertions.
test/e2e/e2e_test.go Renames a test reference and alters fail-fast configuration.
test/e2e/testdata/ratelimit-usage-ratelimit.yaml Adds an isolated Gateway.
test/e2e/testdata/ratelimit-query-parameters.yaml Adds an isolated Gateway.
test/e2e/testdata/ratelimit-path-match.yaml Adds an isolated Gateway.
test/e2e/testdata/ratelimit-multiple-listeners.yaml Renames and isolates resources.
test/e2e/testdata/ratelimit-method-match.yaml Adds an isolated Gateway.
test/e2e/testdata/ratelimit-headers-disabled.yaml Isolates Gateway and policy targets.
test/e2e/testdata/ratelimit-headers-and-cidr-match.yaml Adds an isolated Gateway.
test/e2e/testdata/ratelimit-header-match.yaml Adds an isolated Gateway.
test/e2e/testdata/ratelimit-header-invert-match-global.yaml Isolates Gateway and route names.
test/e2e/testdata/ratelimit-global-shared-gateway-header-match.yaml Renames the shared Gateway.
test/e2e/testdata/ratelimit-global-shared-cidr-match.yaml Adds an isolated Gateway.
test/e2e/testdata/ratelimit-global-shared-and-unshared-header-match.yaml Renames the merge-test Gateway.
test/e2e/testdata/ratelimit-global-shadow-mode.yaml Adds an isolated Gateway.
test/e2e/testdata/ratelimit-cidr-match.yaml Adds a Gateway and dedicated path.
test/e2e/testdata/ratelimit-cidr-invert-match-always-enforce.yaml Adds isolated Gateway and path.
test/e2e/testdata/ratelimit-cidr-invert-always-exempt.yaml Adds an isolated Gateway.
test/e2e/testdata/ratelimit-based-jwt-claims.yaml Adds an isolated Gateway.
test/config/envoy-gateaway-config/xds-name-scheme-v2.yaml Sets a one-second rate-limit timeout.
test/config/envoy-gateaway-config/watch-namespaces.yaml Sets a one-second rate-limit timeout.
test/config/envoy-gateaway-config/gateway-namespace-mode.yaml Sets a one-second rate-limit timeout.
test/config/envoy-gateaway-config/default.yaml Sets a one-second rate-limit timeout.
test/config/envoy-gateaway-config/debounce.yaml Sets a one-second rate-limit timeout.
Review details
  • Files reviewed: 25/25 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread test/e2e/tests/utils.go Outdated
Comment thread test/e2e/e2e_test.go Outdated
Signed-off-by: zirain <zirain2009@gmail.com>

@zhaohuabing zhaohuabing 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.

Great job!

@zhaohuabing
zhaohuabing merged commit 7496c1f into envoyproxy:main Sep 10, 2026
71 of 75 checks passed
@zirain
zirain deleted the e2e/ratelimit branch September 10, 2026 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

FAIL: TestE2E/RateLimitCIDRInvertMatchAlwaysEnforce (14.64s)

4 participants