-
Notifications
You must be signed in to change notification settings - Fork 372
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
🐛 Fix KCPRequestInfoResolver for kind: Cluster #2961
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @nikhita. Thanks for your PR. I'm waiting for a kcp-dev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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. |
/ok-to-test Invited you to the org. |
Closing & re-opening this PR to migrate it to the new Prow. Please do not be alarmed. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@nikhita: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
expectedSubresource string | ||
}{ | ||
"path with /api prefix": { | ||
path: "/api/v1/namespaces/default/pods", |
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.
Why are these valid paths? You need to prefix by cluster or service, there's no default logical cluster to access without specifying.
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.
at some point we redirected to system:admin
, but disabled or broke that behaviour. Now the behaviour is a nasty error message in the log. We should decide how to move forward and make it an hard error early in the stack, e.g. here.
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.
Btw, these tests here might (or not, to be verifiy) actually be correct. In WithInClusterServiceAccountRequestRewrite
we handle the case of a service account calling back without the /cluster
prefix, but with the cluster set in the JWT.
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.
We also strip the /clusters/..
prefix here before passing to the RequestInfoResolver:
kcp/pkg/cache/server/config.go
Lines 172 to 186 in 88d6071
// 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) |
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.
if we strip the cluster prefix, why do we need this special requestinfo resolver in the first place?
@@ -33,16 +34,28 @@ func NewKCPRequestInfoResolver() *KCPRequestInfoResolver { | |||
} | |||
} | |||
|
|||
var clustersRE = regexp.MustCompile(`/clusters/[^/]+/(.*)$`) | |||
var clustersRE = regexp.MustCompile(`/clusters/[^/]+(/.*)$`) |
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.
am curious: why don't we start with ^
aka match the beginning of the string? Now we would also match /foo/cluster/...
. Don't think that's intentional.
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.
am curious: why don't we start with ^ aka match the beginning of the string?
This is to handle all /services/...
URLs. The request resolver was actually added to handle the /services/...
URLs - 963ffde
Some more additional context is in #2479 (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.
the service URLs rechnically have nothing to do with the main kcp server. Would rather expect them to strip the prefix first before apply this resolver.
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.
Im bit lost. Will this solve :ncdc: comment about 60s timeouts? As if I want to build service with streaming to VW backed services, this might be an issue.?
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.
So expectations is that WE ALWAYS trim first prefix for /services
, and first /clusters
?
The cache server preprocesses the path to remove the cluster prefix in https://github.com/kcp-dev/kcp/blob/88d6071b2aaf47bcd5e854f8209e4c3bd8035b35/pkg/cache/server/config.go#L172-L186. When a resource of type "cluster" is created and the first cluster prefix is dropped, the path would be a k8s request info path (/apis/...) but will still have the `clusters` keyword in the path. Due to this, the regex in the kcp request info resolver does not work well. This commit updates the resolver: 1. If a path is a k8s request info path (begins with `/api` or `/apis`), then the path is not checked against the regex. 2. The regex is updated to preserve the forward slash in the strippedURL.
5788e66
to
1c41780
Compare
/test pull-kcp-lint |
/retest |
t := req.Clone(req.Context()) | ||
t.URL.Path = strippedURL | ||
return k.delegate.NewRequestInfo(t) | ||
if !containsPrefix([]string{"/apis/", "/api/"}, req.URL.Path) { |
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.
with my comment above, this line could go and we would be back at the old code, right?
@kcp-dev/kcp-contributors lets pick this up? |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with /lifecycle rotten |
Rotten issues close after 30d of inactivity. /close |
@kcp-ci-bot: Closed this PR. 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
The cache server preprocesses the path to remove the cluster prefix in
kcp/pkg/cache/server/config.go
Lines 172 to 186 in 88d6071
When a resource of
kind: Cluster
is created and the first cluster prefix is dropped, the path would be a k8s request info path (/apis/...
) but will still have theclusters
keyword in the path.Due to this, the regex in the kcp request info resolver does not work well.
This commit updates the resolver to include the following changes:
If a path is a k8s request info path (begins with
/api
or/apis
), then the path is not checked against the regex.The regex is updated to preserve the forward slash in the strippedURL.
Related issue(s)
Fixes #2811