Repository navigation
fix(kubernetes-ingress-nginx): preserve original auth URI - #13754
fredrikblau wants to merge 3 commits into
Conversation
|
Hey @fredrikblau, Thanks for this contribution! |
nmengin
added
kind/enhancement
ghost
left a comment
ghost
left a comment
There was a problem hiding this comment.
The change is small, well-targeted and covered by tests; the fix correctly preserves the pre-rewrite URI for forward-auth. One point worth confirming is the new propagation of X-Forwarded-Uri to upstream backends, and the ingress-nginx docs (docs/content/reference/routing-configuration/kubernetes/ingress-nginx.md) that describe the old behavior may now be stale.
Note
This review was performed by an AI agent. Learn more.
How this review was conducted
I traced the X-Forwarded-Uri flow across the rewrite-target and snippet forward-auth middlewares, confirmed middleware execution order in the ingress-nginx translator, verified the entrypoint forwardedheaders strips the header for untrusted sources, and validated the new test assertions against JoinPath's RawPath behavior.
2 files read beyond the diff
pkg/provider/kubernetes/ingress-nginx/translator.go— lines 460–559 of 634pkg/middlewares/forwardedheaders/forwarded_header.go— lines 1–190 of 321
18 turns
| // Keep the client URI available to downstream middleware, such as the | ||
| // ingress-nginx forward-auth adapter, before rewriting the request path. | ||
| if req.Header.Get(xForwardedURI) == "" { | ||
| req.Header.Set(xForwardedURI, req.URL.RequestURI()) |
There was a problem hiding this comment.
🔵 QUESTION:
Setting X-Forwarded-Uri on req.Header means the original URI is now forwarded to the upstream backend for every rewrite-target request, not only to the forward-auth subrequest. Previously the snippet adapter set it only on the auth forwardReq. Is exposing the pre-rewrite URI to backends intended? (It is consistent with how X-Forwarded-Prefix is handled just above, so this may be deliberate.)
There was a problem hiding this comment.
Good catch. The original URI is needed only by the forward-auth subrequest; exposing it to the upstream backend was not intentional.
I updated the PR to keep the URI in request context and apply X-Forwarded-Uri only while constructing the auth request. The upstream now receives the rewritten request without the new header. I also updated the ingress-nginx documentation and added regression coverage for both behaviors.
The branch has been rebased onto the current master. Focused middleware and provider tests pass.
57aa702 to
22a5c53
Compare
|
Updated the branch with current |
|
@traefik-code-review-oss Thanks for the careful review and your patience. The branch is updated with current |
Fixes #13682
What changed
X-Forwarded-Uribeforerewrite-targetrewrites the request.Validation
go test ./pkg/middlewares/ingressnginx/rewritetarget ./pkg/middlewares/ingressnginx/snippetgo test ./pkg/provider/kubernetes/ingress-nginx