fix(service-proxy): avoid Duplicate Proxy Path in URL Rewrite Function #868
Conversation
…tion and related tests Signed-off-by: Akshay Iyyadurai Balasundaram <akshay.iyyadurai.balasundaram@sap.com>
Signed-off-by: Akshay Iyyadurai Balasundaram <akshay.iyyadurai.balasundaram@sap.com>
ibakshay
changed the title
github-actions
bot
added
the
documentation
ibakshay
changed the title
Signed-off-by: Akshay Iyyadurai Balasundaram <akshay.iyyadurai.balasundaram@sap.com>
…e-proxy' into ibakshay/fix-service-proxy
Signed-off-by: Akshay Iyyadurai Balasundaram <akshay.iyyadurai.balasundaram@sap.com>
Signed-off-by: Akshay Iyyadurai Balasundaram <akshay.iyyadurai.balasundaram@sap.com>
…st URLs Signed-off-by: Akshay Iyyadurai Balasundaram <akshay.iyyadurai.balasundaram@sap.com>
Signed-off-by: Akshay Iyyadurai Balasundaram <akshay.iyyadurai.balasundaram@sap.com>
There was a problem hiding this comment.
Thanks @ibakshay for taking this!
Some comments agreeing with @abhijith-darshan on mock data...
cmd/service-proxy/proxy.go
Outdated
| req.Out.URL.Path = strings.TrimSuffix(upstreamServiceRouteURL.Path, "/") + req.Out.URL.Path | ||
| } | ||
|
|
||
| // Set the correct upstream service route URL details | ||
| req.Out.URL.Scheme = upstreamServiceRouteURL.Scheme | ||
| req.Out.URL.Host = upstreamServiceRouteURL.Host |
There was a problem hiding this comment.
The rewrite of the out.URL was previously covered by req.SetURL(..) in line 227. With this change the query is no longer considered, right?
There was a problem hiding this comment.
the req.SetURL(...) was not handling this test case (valid host with already prefix path) properly.
Below is the test failure output when we have req.SetURL(...)
expected URL https://api.test-api-server.com/api/v1/namespaces/kube-monitoring/services/test-service:8080/existing-path
got https://api.test-api-server.com/api/v1/namespaces/kube-monitoring/services/test-service:8080/api/v1/namespaces/kube-monitoring/services/test-service:8080/existing-pathThat is why I had to manually set the req.out.URL... instead of relying on req.SetURL(..) function.
| } | ||
|
|
||
| // modifyResponse strips the k8s API server proxy path prepended to the location header during redirects: | ||
| // https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apimachinery/pkg/util/proxy/transport.go#L113 | ||
| func (pm *ProxyManager) modifyResponse(resp *http.Response) error { | ||
| logger := log.FromContext(resp.Request.Context()) | ||
| logger.Info("Modifying response", "statusCode", resp.StatusCode, "originalLocation", resp.Header.Get("Location")) | ||
|
|
There was a problem hiding this comment.
I am not very deep into how the whole service proxy works. The comment on this method indicates that it was originally intended to handle the API server proxy path during redirects.
cc @uwe-mayer
There was a problem hiding this comment.
The comment on this method indicates that it was originally intended to handle the API server proxy path during redirects.
Yes, that is correct. I have tested and I can confirm thatmodifyResponse does handle redirects properly without any issues. I have also added tests for this method :)
Description
This PR fixes an issue in the
rewritefunction of theservice-proxy, where outgoing requests to a upstream service could contain duplicate proxy paths. The issue primarily affected applications likeperses, where requests for static assets (e.g., JavaScript, CSS, and favicon files) resulted in 404 Not Found errors due to incorrect URL rewriting.Root Cause
rewritefunction previously unconditionally appended the Kubernetes API proxy path (/api/v1/.../proxy/) toreq.Out.URL.Path, even when the request already contained the full proxy path.I have also added a lot more tests to cover more scenarios.
Related Issues
Closes #838