Skip to content
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.

Sign up for GitHub

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

Jump to bottom

🐛 Fix KCPRequestInfoResolver for kind: Cluster #2961

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🐛 Fix KCPRequestInfoResolver for kind: Cluster #2961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 21 additions & 8 deletions pkg/server/requestinfo/kcp_request_info_resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package requestinfo
import (
"net/http"
"regexp"
"strings"

"k8s.io/apiserver/pkg/endpoints/request"
)
Expand All @@ -33,16 +34,28 @@ func NewKCPRequestInfoResolver() *KCPRequestInfoResolver {
}
}

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

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.

Copy link
Member Author

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)

Copy link
Member

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.

Copy link
Contributor

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

Copy link
Contributor

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?


func (k *KCPRequestInfoResolver) NewRequestInfo(req *http.Request) (*request.RequestInfo, error) {
matches := clustersRE.FindStringSubmatch(req.URL.Path)
if len(matches) == 2 {
// matches[0] is the leftmost pattern that matches (which includes /clusters/...) - skip over that
strippedURL := matches[1]
t := req.Clone(req.Context())
t.URL.Path = strippedURL
return k.delegate.NewRequestInfo(t)
if !containsPrefix([]string{"/apis/", "/api/"}, req.URL.Path) {
Copy link
Member

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?

matches := clustersRE.FindStringSubmatch(req.URL.Path)
if len(matches) == 2 {
// matches[0] is the leftmost pattern that matches (which includes /clusters/...) - skip over that
strippedURL := matches[1]
t := req.Clone(req.Context())
t.URL.Path = strippedURL
return k.delegate.NewRequestInfo(t)
}
}
return k.delegate.NewRequestInfo(req)
}

// containsPrefix returns true if the specified path has a prefix that contained in given prefix Set.
func containsPrefix(prefixSet []string, path string) bool {
for _, prefix := range prefixSet {
if strings.HasPrefix(path, prefix) {
return true
}
}
return false
}
114 changes: 114 additions & 0 deletions pkg/server/requestinfo/kcp_request_info_resolver_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
/*
Copyright 2023 The KCP Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package requestinfo

import (
"net/http"
"net/url"
"testing"

"github.com/stretchr/testify/require"
)

func TestRequestInfoResolver(t *testing.T) {
tests := map[string]struct {
path string
expectedPath string
expectedResource string
expectedSubresource string
}{
"path with /api prefix": {
path: "/api/v1/namespaces/default/pods",
Copy link
Contributor

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.

Copy link
Member

@sttts sttts Jun 13, 2023 •
edited
Loading

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.

Copy link
Member

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.

Copy link
Member Author

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)

Copy link
Member

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?

expectedPath: "/api/v1/namespaces/default/pods",
expectedResource: "pods",
},
"path with /apis prefix": {
path: "/apis/wildwest.dev/v1alpha1/namespaces/default/cowboys/woody",
expectedPath: "/apis/wildwest.dev/v1alpha1/namespaces/default/cowboys/woody",
expectedResource: "cowboys",
},
"path with /apis prefix -- status subresource": {
path: "/apis/wildwest.dev/v1alpha1/namespaces/default/cowboys/woody/status",
expectedPath: "/apis/wildwest.dev/v1alpha1/namespaces/default/cowboys/woody/status",
expectedResource: "cowboys",
expectedSubresource: "status",
},
"path with /apis prefix -- clusters resource": {
path: "/apis/wildwest.dev/v1alpha1/namespaces/default/clusters/woody",
expectedPath: "/apis/wildwest.dev/v1alpha1/namespaces/default/clusters/woody",
expectedResource: "clusters",
},
"path with /apis prefix -- clusters resource, status subresource": {
path: "/apis/wildwest.dev/v1alpha1/namespaces/default/clusters/woody/status",
expectedPath: "/apis/wildwest.dev/v1alpha1/namespaces/default/clusters/woody/status",
expectedResource: "clusters",
expectedSubresource: "status",
},
"path with /clusters prefix": {
path: "/clusters/root/apis/wildwest.dev/v1alpha1/namespaces/default/cowboys/woody",
expectedPath: "/apis/wildwest.dev/v1alpha1/namespaces/default/cowboys/woody",
expectedResource: "cowboys",
},
"path with /clusters prefix -- status subresource": {
path: "/clusters/root/apis/wildwest.dev/v1alpha1/namespaces/default/cowboys/woody/status",
expectedPath: "/apis/wildwest.dev/v1alpha1/namespaces/default/cowboys/woody/status",
expectedResource: "cowboys",
expectedSubresource: "status",
},
"path with /clusters prefix -- clusters resource": {
path: "/clusters/root/apis/wildwest.dev/v1alpha1/namespaces/default/clusters/woody",
expectedPath: "/apis/wildwest.dev/v1alpha1/namespaces/default/clusters/woody",
expectedResource: "clusters",
},
"path with /clusters prefix -- clusters resource, status subresource": {
path: "/clusters/root:my-ws/apis/wildwest.dev/v1alpha1/namespaces/default/clusters/woody/status",
expectedPath: "/apis/wildwest.dev/v1alpha1/namespaces/default/clusters/woody/status",
expectedResource: "clusters",
expectedSubresource: "status",
},
"path with /services prefix": {
path: "/services/apiexport/root:my-ws/my-service/clusters/*/api/v1/configmaps",
expectedPath: "/api/v1/configmaps",
expectedResource: "configmaps",
},
"path with /services prefix -- clusters resource, status subresource": {
path: "/services/apiexport/root:my-ws/my-service/clusters/*/apis/wildwest.dev/v1alpha1/namespaces/default/clusters/woody/status",
expectedPath: "/apis/wildwest.dev/v1alpha1/namespaces/default/clusters/woody/status",
expectedResource: "clusters",
expectedSubresource: "status",
},
}

for name, test := range tests {
t.Run(name, func(t *testing.T) {
url, err := url.Parse(test.path)
require.NoError(t, err, "error parsing path %q to URL", test.path)

req := &http.Request{
URL: url,
Method: http.MethodGet,
}

requestInfo, err := NewKCPRequestInfoResolver().NewRequestInfo(req)
require.NoError(t, err, "unexpected error")

require.Equal(t, test.expectedPath, requestInfo.Path, "unexpected requestInfo.Path")
require.Equal(t, test.expectedResource, requestInfo.Resource, "unexpected requestInfo.Resource")
require.Equal(t, test.expectedSubresource, requestInfo.Subresource, "unexpected requestInfo.Subresource")
})
}
}