Skip to content

internal/envoy: always set NumRetries so numRetries=-1 actually disables retries#7525

Open
SAY-5 wants to merge 3 commits into
projectcontour:mainfrom
SAY-5:fix/retry-policy-num-retries-zero-5944
Open

internal/envoy: always set NumRetries so numRetries=-1 actually disables retries#7525
SAY-5 wants to merge 3 commits into
projectcontour:mainfrom
SAY-5:fix/retry-policy-num-retries-zero-5944

Conversation

@SAY-5
Copy link
Copy Markdown

@SAY-5 SAY-5 commented Apr 21, 2026

Summary

HTTPProxy documents numRetries: -1 as "disable retries". The DAG translator in internal/dag/policy.go already maps the API value -1 to the DAG value 0 (with DAG value 1 standing for the default one-retry behaviour).

internal/envoy/v3/route.retryPolicy, however, only forwarded NumRetries to Envoy when the DAG value was > 0. When a user set numRetries: -1, the DAG value 0 silently disappeared from the Envoy RetryPolicy, and Envoy's documented default of 1 kicked in, giving exactly one retry instead of none.

Fix

Always set NumRetries (including 0) so the Envoy config matches what the API promised.

Fixes #5944.

Test

  • go build ./internal/envoy/... green.
  • go test ./internal/envoy/v3/... -count=1 -short passes.
  • go test ./internal/xdscache/v3/... -count=1 -short passes (TestRouteVisit goldens unchanged).

Release Note

Fix `numRetries: -1` on HTTPProxy retry policies so it disables retries as documented, instead of falling back to Envoy's default of one retry.

/kind bug
/area httpproxy

…les retries

HTTPProxy documents `numRetries: -1` as "disable retries". The DAG
translator in internal/dag/policy.go already maps the API value -1 to
the DAG value 0 (with DAG value 1 standing for the default one-retry
behaviour). internal/envoy/v3/route.retryPolicy, however, only
forwarded NumRetries to Envoy when the DAG value was > 0. When a user
set numRetries: -1, the DAG value 0 silently disappeared from the
Envoy RetryPolicy, and Envoy's documented default of 1 kicked in --
giving exactly one retry instead of none (projectcontour#5944).

Always set NumRetries (including 0) so the Envoy config matches what
the API promised.

Fixes projectcontour#5944.

Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
@SAY-5 SAY-5 requested a review from a team as a code owner April 21, 2026 20:08
@SAY-5 SAY-5 requested review from sunjayBhatia and tsaarni and removed request for a team April 21, 2026 20:08
Signed-off-by: SAY-5 <saiasish.cnp@gmail.com>
@SAY-5
Copy link
Copy Markdown
Author

SAY-5 commented May 6, 2026

Updated PR body with a release-note section per the template (I cannot self-apply the release-note/small label as a non-maintainer). go test ./internal/envoy/v3/... ./internal/xdscache/v3/... passes locally on this branch, TestRouteVisit goldens are unchanged by the NumRetries fix. Awaiting maintainer ok-to-test for the workflow run.

@tsaarni tsaarni added the release-note/small A small change that needs one line of explanation in the release notes. label May 7, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.32%. Comparing base (a94de15) to head (7f23167).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7525      +/-   ##
==========================================
- Coverage   82.32%   82.32%   -0.01%     
==========================================
  Files         130      130              
  Lines       15624    15622       -2     
==========================================
- Hits        12863    12861       -2     
  Misses       2478     2478              
  Partials      283      283              
Files with missing lines Coverage Δ
internal/envoy/v3/route.go 92.84% <100.00%> (-0.02%) ⬇️
internal/featuretests/v3/envoy.go 99.34% <100.00%> (-0.01%) ⬇️
🚀 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.

Signed-off-by: SAY-5 <saiasish.cnp@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-note/small A small change that needs one line of explanation in the release notes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

setting NumRetries to -1 does not appear to disable retries

2 participants