🐛 Fix KCPRequestInfoResolver for kind: Cluster #2961

nikhita · 2023-05-21T18:43:28Z

Summary

The cache server preprocesses the path to remove the cluster prefix in

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)

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 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 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

openshift-ci · 2023-05-21T18:43:38Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sttts for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2023-05-21T18:43:40Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

sttts · 2023-06-03T14:23:13Z

/ok-to-test

Invited you to the org.

xrstf · 2023-06-06T17:22:34Z

Closing & re-opening this PR to migrate it to the new Prow. Please do not be alarmed.

kcp-ci-bot · 2023-06-06T17:22:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sttts for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2023-06-06T17:27:24Z

@nikhita: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/lint	`5788e66`	link	true	`/test lint`

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.

stevekuznetsov · 2023-06-06T18:05:17Z

pkg/server/requestinfo/kcp_request_info_resolver_test.go

+		expectedSubresource string
+	}{
+		"path with /api prefix": {
+			path:             "/api/v1/namespaces/default/pods",


Why are these valid paths? You need to prefix by cluster or service, there's no default logical cluster to access without specifying.

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.

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.

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)

if we strip the cluster prefix, why do we need this special requestinfo resolver in the first place?

sttts · 2023-06-13T14:57:17Z

pkg/server/requestinfo/kcp_request_info_resolver.go

@@ -33,16 +34,28 @@ func NewKCPRequestInfoResolver() *KCPRequestInfoResolver {
 	}
 }

-var clustersRE = regexp.MustCompile(`/clusters/[^/]+/(.*)$`)
+var clustersRE = regexp.MustCompile(`/clusters/[^/]+(/.*)$`)


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.

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)

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.

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.?

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.

nikhita · 2023-06-29T12:23:08Z

/test pull-kcp-lint

mjudeikis · 2023-10-17T11:49:29Z

/retest

sttts · 2023-11-13T12:02:34Z

pkg/server/requestinfo/kcp_request_info_resolver.go

-		t := req.Clone(req.Context())
-		t.URL.Path = strippedURL
-		return k.delegate.NewRequestInfo(t)
+	if !containsPrefix([]string{"/apis/", "/api/"}, req.URL.Path) {


with my comment above, this line could go and we would be back at the old code, right?

mjudeikis · 2024-03-25T16:49:08Z

@kcp-dev/kcp-contributors lets pick this up?

openshift-ci bot requested review from scheeles and sttts May 21, 2023 18:43

openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 21, 2023

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 3, 2023

xrstf closed this Jun 6, 2023

xrstf reopened this Jun 6, 2023

kcp-ci-bot added dco-signoff: no Indicates the PR's author has not signed the DCO. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 6, 2023

stevekuznetsov reviewed Jun 6, 2023

View reviewed changes

sttts reviewed Jun 13, 2023

View reviewed changes

nikhita force-pushed the fix-for-kind-cluster branch from 5788e66 to 1c41780 Compare June 29, 2023 11:53

kcp-ci-bot added dco-signoff: yes Indicates the PR's author has signed the DCO. and removed dco-signoff: no Indicates the PR's author has not signed the DCO. labels Jun 29, 2023

sttts reviewed Nov 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fix KCPRequestInfoResolver for kind: Cluster #2961

🐛 Fix KCPRequestInfoResolver for kind: Cluster #2961

nikhita commented May 21, 2023

openshift-ci bot commented May 21, 2023

openshift-ci bot commented May 21, 2023

sttts commented Jun 3, 2023

xrstf commented Jun 6, 2023

kcp-ci-bot commented Jun 6, 2023

openshift-ci bot commented Jun 6, 2023

stevekuznetsov Jun 6, 2023

sttts Jun 13, 2023 •

edited

sttts Jun 13, 2023

nikhita Jun 29, 2023

sttts Nov 13, 2023

sttts Jun 13, 2023

nikhita Jun 29, 2023

sttts Nov 13, 2023

mjudeikis Nov 16, 2023

mjudeikis Nov 17, 2023

nikhita commented Jun 29, 2023

mjudeikis commented Oct 17, 2023

sttts Nov 13, 2023

mjudeikis commented Mar 25, 2024

	// 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)

🐛 Fix KCPRequestInfoResolver for kind: Cluster #2961

Are you sure you want to change the base?

🐛 Fix KCPRequestInfoResolver for kind: Cluster #2961

Conversation

nikhita commented May 21, 2023

Summary

Related issue(s)

openshift-ci bot commented May 21, 2023

openshift-ci bot commented May 21, 2023

sttts commented Jun 3, 2023

xrstf commented Jun 6, 2023

kcp-ci-bot commented Jun 6, 2023

openshift-ci bot commented Jun 6, 2023

Choose a reason for hiding this comment

sttts Jun 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhita commented Jun 29, 2023

mjudeikis commented Oct 17, 2023

Choose a reason for hiding this comment

mjudeikis commented Mar 25, 2024

sttts Jun 13, 2023 •

edited