Skip to content

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.

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

Merged
kcp-ci-bot merged 9 commits into from
Aug 22, 2025
Merged

Fix Cluster kind and multiple /clusters/ in URLs #3537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a57957d
Move regex into test file
ntnn Aug 15, 2025
4d324f3
Return an error if attempts are made to parse a cluster when one is a…
ntnn Aug 15, 2025
fd2a0ed
Don't build longer and longer chains with each call
ntnn Aug 15, 2025
5601438
Remove KCPRequestInfoResolver
ntnn Aug 20, 2025
c26a909
Drop KCPRequestInfoResolver
ntnn Aug 20, 2025
3420364
Remove middleware unnecessarily pruning /clusters/.../ from URL path
ntnn Aug 20, 2025
365ff2f
Add tests for URLs
ntnn Aug 21, 2025
b3a728f
Move and export InactiveAnnotation
ntnn Aug 21, 2025
a5bd98d
Add test for inactive logicalclusters
ntnn Aug 21, 2025
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
7 changes: 1 addition & 6 deletions pkg/server/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,6 @@ import (
"github.com/kcp-dev/kcp/pkg/server/openapiv3"
kcpserveroptions "github.com/kcp-dev/kcp/pkg/server/options"
"github.com/kcp-dev/kcp/pkg/server/options/batteries"
"github.com/kcp-dev/kcp/pkg/server/requestinfo"
apisv1alpha1 "github.com/kcp-dev/kcp/sdk/apis/apis/v1alpha1"
kcpclientset "github.com/kcp-dev/kcp/sdk/client/clientset/versioned/cluster"
kcpinformers "github.com/kcp-dev/kcp/sdk/client/informers/externalversions"
Expand Down Expand Up @@ -406,9 +405,6 @@ func NewConfig(ctx context.Context, opts kcpserveroptions.CompletedOptions) (*Co
virtualWorkspaceServerProxyTransport = transport
}

// Make sure to set our RequestInfoResolver that is capable of populating a RequestInfo even for /services/... URLs.
c.GenericConfig.RequestInfoResolver = requestinfo.NewKCPRequestInfoResolver()
Comment on lines -409 to -410
Copy link
Member Author

@ntnn ntnn Aug 21, 2025

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


// Prepare an authentication index to be used later by a middleware. We start it early
// because it can potentially fail and the BuildHandlerChainFunc() has no way to return
// an error.
Expand All @@ -424,7 +420,6 @@ func NewConfig(ctx context.Context, opts kcpserveroptions.CompletedOptions) (*Co

authIndex = authIndexState
}

// preHandlerChainMux is called before the actual handler chain. Note that BuildHandlerChainFunc below
// is called multiple times, but only one of the handler chain will actually be used. Hence, we wrap it
// to give handlers below one mux.Handle func to call.
Expand Down Expand Up @@ -516,7 +511,7 @@ func NewConfig(ctx context.Context, opts kcpserveroptions.CompletedOptions) (*Co
// that path itself, instead of the rest of the handler chain above handling it.
mux := http.NewServeMux()
mux.Handle("/", apiHandler)
*c.preHandlerChainMux = append(*c.preHandlerChainMux, mux)
*c.preHandlerChainMux = []*http.ServeMux{mux}
apiHandler = mux

apiHandler = filters.WithAuditInit(apiHandler) // Must run before any audit annotation is made
Expand Down
25 changes: 14 additions & 11 deletions pkg/server/filters/filters.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"fmt"
"net/http"
"net/url"
"regexp"
"strings"

"github.com/munnerz/goautoneg"
Expand All @@ -45,21 +44,11 @@ type (
const (
workspaceAnnotation = "tenancy.kcp.io/workspace"

// inactiveAnnotation is the annotation denoting a logical cluster should be
// deemed unreachable.
inactiveAnnotation = "internal.kcp.io/inactive"

// clusterKey is the context key for the request namespace.
acceptHeaderContextKey acceptHeaderContextKeyType = iota
)

var (
// reClusterName is a regular expression for cluster names. It is based on
// modified RFC 1123. It allows for 63 characters for single name and includes
// KCP specific ':' separator for workspace nesting. We are not re-using k8s
// validation regex because its purpose is for single name validation.
reClusterName = regexp.MustCompile(`^([a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?:)*[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?$`)

errorScheme = runtime.NewScheme()
errorCodecs = serializer.NewCodecFactory(errorScheme)
)
Expand Down Expand Up @@ -87,6 +76,20 @@ func WithAuditEventClusterAnnotation(handler http.Handler) http.HandlerFunc {
// It also trims "/clusters/" prefix from the URL.
func WithClusterScope(apiHandler http.Handler) http.HandlerFunc {
return func(w http.ResponseWriter, req *http.Request) {
cl, err := request.ValidClusterFrom(req.Context())
if err == nil {
// This is a failsafe - this should not happen.
// If a cluste is already set in the request context it
// should not be present in the URL path.
responsewriters.ErrorNegotiated(
apierrors.NewBadRequest(
fmt.Sprintf("found cluster %q in the request context, when trying to read cluster from URL", cl.Name.String()),
),
errorCodecs, schema.GroupVersion{},
w, req)
return
}

path, newURL, found, err := ClusterPathFromAndStrip(req)
if err != nil {
responsewriters.ErrorNegotiated(
Expand Down
9 changes: 9 additions & 0 deletions pkg/server/filters/filters_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"fmt"
"os"
"path/filepath"
"regexp"
"runtime"
"strings"
"testing"
Expand All @@ -30,6 +31,14 @@ import (
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
)

var (
// reClusterName is a regular expression for cluster names. It is based on
// modified RFC 1123. It allows for 63 characters for single name and includes
// KCP specific ':' separator for workspace nesting. We are not re-using k8s
// validation regex because its purpose is for single name validation.
reClusterName = regexp.MustCompile(`^([a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?:)*[a-z0-9]([a-z0-9-]{0,61}[a-z0-9])?$`)
)

func Test_isPartialMetadataHeader(t *testing.T) {
tests := map[string]struct {
accept string
Expand Down
16 changes: 8 additions & 8 deletions pkg/server/filters/inactivelogicalcluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,12 @@ import (
corev1alpha1informers "github.com/kcp-dev/kcp/sdk/client/informers/externalversions/core/v1alpha1"
)

const (
// InactiveAnnotation is the annotation denoting a logical cluster should be
// deemed unreachable.
InactiveAnnotation = "internal.kcp.io/inactive"
)

// WithBlockInactiveLogicalClusters ensures that any requests to logical
// clusters marked inactive are rejected.
func WithBlockInactiveLogicalClusters(handler http.Handler, kcpClusterClient corev1alpha1informers.LogicalClusterClusterInformer) http.HandlerFunc {
Expand All @@ -39,15 +45,9 @@ func WithBlockInactiveLogicalClusters(handler http.Handler, kcpClusterClient cor
}

return func(w http.ResponseWriter, req *http.Request) {
_, newURL, _, err := ClusterPathFromAndStrip(req)
if err != nil {
responsewriters.InternalError(w, req, err)
return
}

isException := false
for _, prefix := range allowedPathPrefixes {
if strings.HasPrefix(newURL.String(), prefix) {
if strings.HasPrefix(req.URL.String(), prefix) {
isException = true
}
}
Expand All @@ -56,7 +56,7 @@ func WithBlockInactiveLogicalClusters(handler http.Handler, kcpClusterClient cor
if cluster != nil && !cluster.Name.Empty() && !isException {
logicalCluster, err := kcpClusterClient.Cluster(cluster.Name).Lister().Get(corev1alpha1.LogicalClusterName)
if err == nil {
if ann, ok := logicalCluster.ObjectMeta.Annotations[inactiveAnnotation]; ok && ann == "true" {
if ann, ok := logicalCluster.ObjectMeta.Annotations[InactiveAnnotation]; ok && ann == "true" {
responsewriters.ErrorNegotiated(
apierrors.NewForbidden(corev1alpha1.Resource("logicalclusters"), cluster.Name.String(), errors.New("logical cluster is marked inactive")),
errorCodecs, schema.GroupVersion{}, w, req,
Expand Down
48 changes: 0 additions & 48 deletions pkg/server/requestinfo/kcp_request_info_resolver.go
View file
Open in desktop

This file was deleted.

Loading