Merge pull request #3537 from ntnn/kcp2811

Fix `Cluster` kind and multiple `/clusters/` in URLs

Author: kcp-ci-bot

pkg/server/config.go

@@ -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"
@@ -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()
-
 	// 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.
@@ -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.
@@ -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

pkg/server/filters/filters.go

@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
-	"regexp"
 	"strings"
 
 	"github.com/munnerz/goautoneg"
@@ -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)
 )
@@ -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(

pkg/server/filters/filters_test.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"regexp"
 	"runtime"
 	"strings"
 	"testing"
@@ -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

pkg/server/filters/inactivelogicalcluster.go

@@ -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 {
@@ -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
 			}
 		}
@@ -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,