Apiserver proxy rewrites URL when service returns absolute path with request's host #52556
Conversation
k8s-ci-robot
added
size/M
|
Hi @roycaihw. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
needs-ok-to-test
k8s-github-robot
added
the
release-note
|
/assign @liggitt |
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
/sig api-machinery |
k8s-ci-robot
added
the
sig/api-machinery
| func (t *Transport) rewriteURL(targetURL string, sourceURL *url.URL) string { | ||
| // occurred, or if the URL matches the sourceHost. If any error occurs (e.g. | ||
| // parsing), it returns targetURL. | ||
| func (t *Transport) rewriteURL(targetURL string, sourceURL *url.URL, sourceHost string) string { |
There was a problem hiding this comment.
nit: It was a little confusing for me to read sourceURL.host vs sourceHost. It is much clear to call it sourceReqHost or sourceRequestHost
There was a problem hiding this comment.
Agree that the names are almost impenetrable
|
just one nit. /lgtm |
k8s-ci-robot
added
the
lgtm
|
@caesarxuchao for approval. |
| url, err := url.Parse(targetURL) | ||
| if err != nil { | ||
| return targetURL | ||
| } | ||
|
|
||
| isDifferentHost := url.Host != "" && url.Host != sourceURL.Host | ||
| isDifferentHost := url.Host != "" && url.Host != sourceURL.Host && url.Host != sourceHost |
There was a problem hiding this comment.
This is difficult to reason about if sourceURL.Host and sourceHost are both non-empty… would it make sense to collapse to a single sourceHost param to this function?
There was a problem hiding this comment.
@liggitt Sorry I didn't get the idea here. I think there are cases where sourceURL.Host and sourceRequestHost are both non-empty, and we need to examine both of them to tell if the targetURL is in a different host.
| func (t *Transport) rewriteURL(targetURL string, sourceURL *url.URL) string { | ||
| // occurred, or if the URL matches the sourceHost. If any error occurs (e.g. | ||
| // parsing), it returns targetURL. | ||
| func (t *Transport) rewriteURL(targetURL string, sourceURL *url.URL, sourceHost string) string { |
There was a problem hiding this comment.
Agree that the names are almost impenetrable
k8s-github-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
| url, err := url.Parse(targetURL) | ||
| if err != nil { | ||
| return targetURL | ||
| } | ||
|
|
||
| isDifferentHost := url.Host != "" && url.Host != sourceURL.Host | ||
| // Example: |
There was a problem hiding this comment.
Thanks. An example's worth a thousand words.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, mbohlool, roycaihw Associated issue: 51790 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
When a service responses with an URL using an absolute path and the request's host (e.g. in redirection location), current transport recognizes the URL as a different host and doesn't rewrite the absolute path.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #51790Special notes for your reviewer:
Release note: