Repository navigation
router: stop duplicating X-Forwarded-For - #484
Conversation
volcano-sh-bot
added
kind/bug
|
Welcome @altf4-games! It looks like this is your first PR to volcano-sh/agentcube 🎉 |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #484 +/- ##
==========================================
+ Coverage 59.98% 60.45% +0.46%
==========================================
Files 36 35 -1
Lines 3589 3616 +27
==========================================
+ Hits 2153 2186 +33
+ Misses 1216 1211 -5
+ Partials 220 219 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @altf4-games, thanks for your PR. It has been labeled commit 0baf96b818e670237e461de694a1dce3661b3147 (HEAD -> pr484)
Author: Pradyum Mistry <39883432+altf4-games@users.noreply.github.com>
Date: Tue Oct 20 18:33:42 2026 +0530
router: stop duplicating X-Forwarded-For
Fixes #480
Signed-off-by: Pradyum Mistry <39883432+altf4-games@users.noreply.github.com> |
Signed-off-by: Pradyum Mistry <39883432+altf4-games@users.noreply.github.com>
0baf96b to
a7ad1f4
Compare
|
@zhzhuang-zju thanks for pointing that out. I’ve removed it from the commit message. |
|
I reviewed your fix, and it makes sense to me. The custom proxy director first began appending However, in Director mode, Ask @acsoto to take another look |
| if c.Request.TLS != nil { | ||
| req.Header.Set("X-Forwarded-Proto", "https") | ||
| } | ||
| req.Header.Set("X-Forwarded-For", forwardedClientIP(req, c.ClientIP())) |
There was a problem hiding this comment.
Is dropping the X-Real-IP fallback intentional? When an upstream proxy sends only that header, the sandbox's X-Forwarded-For now contains only the proxy address.
Signed-off-by: Pradyum Mistry <39883432+altf4-games@users.noreply.github.com>
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: acsoto The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
The router sent a duplicated X-Forwarded-For header to the sandbox. configureProxyDirector set the header itself, and then httputil.ReverseProxy appended the peer address again, so each hop showed up twice (for example "127.0.0.1, 127.0.0.1").
The Director no longer sets the header, and the unused forwardedClientIP helper is removed. Any X-Forwarded-For the client sent is still forwarded, and the real peer is appended once.
Which issue(s) this PR fixes:
Fixes #480
Special notes for your reviewer:
c.ClientIP() is no longer used for this header.
I added TestForwardToSandbox_XForwardedFor with four cases: no prior header, one entry, several entries, and repeated header lines. It fails without the change. I also built the router from main and from this branch and ran both against a stub workload manager and an echo backend. go test -race ./pkg/router/... and the repo lint under Go 1.26.8 pass.
Not tested: a real cluster or an ingress in front of the router.
AI use: I used Claude to help find the bug and draft the fix and the test. I reviewed and ran all of it and can explain each line.
Does this PR introduce a user-facing change?: