-
Notifications
You must be signed in to change notification settings - Fork 423
Fix Cluster kind and multiple /clusters/ in URLs
#3537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
68aefe0 to
1d1637b
Compare
The regex is only used within the tests and not the filter. Signed-off-by: Nelo-T. Wallus <[email protected]>
…lready present in context Signed-off-by: Nelo-T. Wallus <[email protected]> Signed-off-by: Nelo-T. Wallus <[email protected]>
Signed-off-by: Nelo-T. Wallus <[email protected]>
Signed-off-by: Nelo-T. Wallus <[email protected]> Signed-off-by: Nelo-T. Wallus <[email protected]>
Signed-off-by: Nelo-T. Wallus <[email protected]> Signed-off-by: Nelo-T. Wallus <[email protected]>
Signed-off-by: Nelo-T. Wallus <[email protected]> Signed-off-by: Nelo-T. Wallus <[email protected]>
Cluster KindCluster kind and multiple /clusters/ in URLs
| // Make sure to set our RequestInfoResolver that is capable of populating a RequestInfo even for /services/... URLs. | ||
| c.GenericConfig.RequestInfoResolver = requestinfo.NewKCPRequestInfoResolver() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One places where this was used was in the cache server, where it would prune the cluster anyhow:
kcp/pkg/cache/server/config.go
Lines 169 to 194 in 339e173
| func(rq *http.Request) (string, string, error) { | |
| if serverConfig.Config.RequestInfoResolver == nil { | |
| return "", "", errors.New("no RequestInfoResolver provided") | |
| } | |
| // the k8s request info resolver expects a cluster-less path, but the client we're using knows how to | |
| // add the cluster we are targeting to the path before this round-tripper fires, so we need to strip it | |
| // to use the k8s library | |
| parts := strings.Split(rq.URL.Path, "/") | |
| if len(parts) < 4 { | |
| return "", "", fmt.Errorf("RequestInfoResolver: got invalid path: %v", rq.URL.Path) | |
| } | |
| if parts[1] != "clusters" { | |
| return "", "", fmt.Errorf("RequestInfoResolver: got path without cluster prefix: %v", rq.URL.Path) | |
| } | |
| // we clone the request here to safely mutate the URL path, but this cloned request is never realized | |
| // into anything on the network, just inspected by the k8s request info libraries | |
| clone := rq.Clone(rq.Context()) | |
| clone.URL.Path = strings.Join(parts[3:], "/") | |
| requestInfo, err := serverConfig.Config.RequestInfoResolver.NewRequestInfo(clone) | |
| if err != nil { | |
| return "", "", err | |
| } | |
| return requestInfo.Resource, requestInfo.Verb, nil | |
| }, | |
| "customresourcedefinitions") | |
| rt = rest.AddUserAgent(rt, "kcp-cache-server") |
|
/cherry-pick release-0.28 |
|
@ntnn: once the present PR merges, I will cherry-pick it on top of release-0.28 in a new PR and assign it to you. In response to this:
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. |
| // TODO(ntnn): Replace with t.Context in go1.24 | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| t.Cleanup(cancel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using this instead of t.Context for the cherry-pick to 0.28
| limitations under the License. | ||
| */ | ||
|
|
||
| package server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be a new e2e package but I didn't see any other that would be fitting.
Signed-off-by: Nelo-T. Wallus <[email protected]> Signed-off-by: Nelo-T. Wallus <[email protected]>
Signed-off-by: Nelo-T. Wallus <[email protected]> Signed-off-by: Nelo-T. Wallus <[email protected]>
Signed-off-by: Nelo-T. Wallus <[email protected]> Signed-off-by: Nelo-T. Wallus <[email protected]>
|
/retest |
embik
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
|
LGTM label has been added. Git tree hash: 7b6987a5fd53bc34da241b3f07fbb748b03eb985
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: embik The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Known flake #3522 |
|
/retest Known flake #3522 |
|
@ntnn: #3537 failed to apply on top of branch "release-0.28": In response to this:
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. |
Summary
See #2811 and #3517
The KCPRequestInfo handler didn't have any actual use and its existence had been questioned before (I'll post the link to the comment if I find it again) and it was the major contributor for the multiple
/cluster/...issue.What Type of PR Is This?
/kind bug
/kind cleanup
Related Issue(s)
Fixes #2811
Fixes #3517
Release Notes