fix: redirect port leaking across multiple listeners#9349
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 206c4249b7
ℹ️ 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".
| // This is important when a route has multiple parent refs (listeners) | ||
| // with different ports, as the redirect port needs to be derived | ||
| // independently for each listener. | ||
| routeRoute := routeRoute.DeepCopy() |
There was a problem hiding this comment.
Because this changes user-visible redirect behavior when one HTTPRoute attaches to multiple listeners with different ports, it should be included in the compiled release notes. The repository’s release-notes/current/README.md says bug fixes need a new fragment under release-notes/current/bug_fixes/, but this commit only updates the translator and testdata, so users upgrading would not see this fix documented.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Fixes a translation bug where an HTTPRoute redirect filter with no explicit port could “leak” the derived redirect port from the first processed Gateway listener to subsequent listeners with different ports. The change ensures redirect port derivation is isolated per listener, and adds a regression test case covering the multi-listener scenario.
Changes:
- Deep-copies per-route IR before deriving a default redirect port so listener-specific derived state doesn’t persist across listeners.
- Adds new translator golden testdata for an HTTPRoute attached to two listeners (8080 and 9090) with a redirect filter missing
port. - Validates generated xDS IR contains listener-appropriate redirect ports (8080 and 9090 respectively).
Review Findings
Required fixes
- Add a release-notes fragment for this user-visible bug fix (commented at
internal/gatewayapi/route.go:1281-1283).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/gatewayapi/route.go | Deep-copies IR route objects before deriving redirect port so derived state doesn’t leak across listeners. |
| internal/gatewayapi/testdata/httproute-redirect-port-multi-listener.in.yaml | New regression input: one Gateway with two listeners and an HTTPRoute redirect filter without an explicit port. |
| internal/gatewayapi/testdata/httproute-redirect-port-multi-listener.out.yaml | Expected output verifying each listener gets the correct derived redirect port in xDS IR. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Deep copy the route first to avoid modifying the original and | ||
| // affecting other listeners that may be attached to the same route. | ||
| // This is important when a route has multiple parent refs (listeners) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9349 +/- ##
==========================================
+ Coverage 75.35% 75.36% +0.01%
==========================================
Files 252 252
Lines 41475 41476 +1
==========================================
+ Hits 31254 31259 +5
+ Misses 8108 8106 -2
+ Partials 2113 2111 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/retest |
When a HTTPRoute with redirect filter is attached to multiple listeners with different ports, the redirect port derived from one listener could be reused by subsequent listeners because route translation mutated the shared route object. Deep copy the route while processing each listener so redirect ports are derived independently per listener. Add golden coverage for multi-listener redirects, including scheme-derived well-known ports. Signed-off-by: liuhy <liuhongyu@apache.org>
Aias00
force-pushed
the
worktree-fix-redirect-port-leak
branch
2 times, most recently
from
a4b7671 to
0637a3e
Compare
Document the fix for redirect port leaking across multiple listeners. Signed-off-by: liuhy <liuhongyu@apache.org>
Aias00
force-pushed
the
worktree-fix-redirect-port-leak
branch
from
0637a3e to
64604dd
Compare
Problem Description
When a HTTPRoute with a redirect filter (without an explicit port) is attached to multiple Gateway listeners with different ports, the redirect port derived from the first listener is incorrectly reused by subsequent listeners.
Root Cause
In
internal/gatewayapi/route.go:1282-1296, the code directly modifiesrouteRoute.Redirect.Portwithout first deep copying therouteRouteobject. When processing subsequent listeners, the checkrouteRoute.Redirect.Port == nilfails because the first listener already set it, causing all listeners to reuse the first listener's port.Fix
Add a
DeepCopy()ofrouteRouteat the beginning of the listener processing loop to ensure each listener works with an independent copy and derives the correct redirect port independently.Test Case
Added
httproute-redirect-port-multi-listenertest case:Related Issue
Fixes #9348