Merge pull request kubernetes#129 from ncdc/kcp-fix-partial-metadata

Support multiple CRDs/workspaces/versions
ncdc · Feb 9, 2023 · 05662f8 · 05662f8
2 parents f48a2e5 + 736ed65
commit 05662f8
Show file tree

Hide file tree

Showing 5 changed files with 90 additions and 17 deletions.
diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/conversion/converter.go
@@ -18,6 +18,7 @@ package conversion
 
 import (
 	"fmt"
+	"strings"
 
 	autoscalingv1 "k8s.io/api/autoscaling/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
@@ -104,6 +105,10 @@ func NewDelegatingConverter(crd *apiextensionsv1.CustomResourceDefinition, deleg
 		validVersions: validVersions,
 		clusterScoped: crd.Spec.Scope == apiextensionsv1.ClusterScoped,
 		converter:     delegate,
+
+		// If this is a wildcard partial metadata CRD variant, we don't require that the CRD serves the appropriate
+		// version, because the schema does not matter.
+		requireValidVersion: !strings.HasSuffix(string(crd.UID), ".wildcard.partial-metadata"),
 	}
 	return &safeConverterWrapper{unsafe}, unsafe, nil
 }
@@ -123,6 +128,9 @@ type delegatingCRConverter struct {
 	converter     CRConverter
 	validVersions map[schema.GroupVersion]bool
 	clusterScoped bool
+
+	// If true, require that the CRD serves the appropriate version
+	requireValidVersion bool
 }
 
 func (c *delegatingCRConverter) ConvertFieldLabel(gvk schema.GroupVersionKind, label, value string) (string, string, error) {
@@ -203,18 +211,18 @@ func (c *delegatingCRConverter) ConvertToVersion(in runtime.Object, target runti
 	}
 
 	desiredAPIVersion := toGVK.GroupVersion().String()
-	if !c.validVersions[toGVK.GroupVersion()] {
+	if c.requireValidVersion && !c.validVersions[toGVK.GroupVersion()] {
 		return nil, fmt.Errorf("request to convert CR to an invalid group/version: %s", desiredAPIVersion)
 	}
 	// Note that even if the request is for a list, the GV of the request UnstructuredList is what
 	// is expected to convert to. As mentioned in the function's document, it is not expected to
 	// get a v1.List.
-	if !c.validVersions[fromGVK.GroupVersion()] {
+	if c.requireValidVersion && !c.validVersions[fromGVK.GroupVersion()] {
 		return nil, fmt.Errorf("request to convert CR from an invalid group/version: %s", fromGVK.GroupVersion().String())
 	}
 
 	var objectsToConvert []*unstructured.Unstructured
-	objectsToConvert, err := getObjectsToConvert(list, desiredAPIVersion, c.validVersions)
+	objectsToConvert, err := getObjectsToConvert(list, desiredAPIVersion, c.validVersions, c.requireValidVersion)
 	if err != nil {
 		return nil, err
 	}
@@ -296,11 +304,12 @@ func getObjectsToConvert(
 	list *unstructured.UnstructuredList,
 	desiredAPIVersion string,
 	validVersions map[schema.GroupVersion]bool,
+	requireValidVersion bool,
 ) ([]*unstructured.Unstructured, error) {
 	var objectsToConvert []*unstructured.Unstructured
 	for i := range list.Items {
 		expectedGV := list.Items[i].GroupVersionKind().GroupVersion()
-		if !validVersions[expectedGV] {
+		if requireValidVersion && !validVersions[expectedGV] {
 			return nil, fmt.Errorf("request to convert CR list failed, list index %d has invalid group/version: %s", i, expectedGV.String())
 		}
 

diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_handler.go
@@ -348,7 +348,9 @@ func (r *crdHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		return
 	}
 
-	if !apiextensionshelpers.HasServedCRDVersion(crd, requestInfo.APIVersion) {
+	wildcardPartialMetadata := strings.HasSuffix(string(crd.UID), ".wildcard.partial-metadata")
+	// For wildcard partial metadata requests, we don't care if the CRD serves the version being requested or not.
+	if !wildcardPartialMetadata && !apiextensionshelpers.HasServedCRDVersion(crd, requestInfo.APIVersion) {
 		r.delegate.ServeHTTP(w, req)
 		return
 	}
@@ -384,7 +386,9 @@ func (r *crdHandler) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 		)
 		return
 	}
-	if !hasServedCRDVersion(crdInfo.spec, requestInfo.APIVersion) {
+
+	// For wildcard partial metadata requests, we don't care if the CRD serves the version being requested or not.
+	if !wildcardPartialMetadata && !hasServedCRDVersion(crdInfo.spec, requestInfo.APIVersion) {
 		r.delegate.ServeHTTP(w, req)
 		return
 	}
@@ -507,8 +511,31 @@ func ConvertProtobufRequestsToJson(verb string, req *http.Request, gvk schema.Gr
 }
 
 func (r *crdHandler) serveResource(w http.ResponseWriter, req *http.Request, requestInfo *apirequest.RequestInfo, crdInfo *crdInfo, crd *apiextensionsv1.CustomResourceDefinition, terminating bool, supportedTypes []string) http.HandlerFunc {
+	wildcardPartialMetadata := strings.HasSuffix(string(crd.UID), ".wildcard.partial-metadata")
+
 	requestScope := crdInfo.requestScopes[requestInfo.APIVersion]
+	if requestScope == nil && wildcardPartialMetadata {
+		// If requestScope is nil and this is a wildcard partial metadata request, it means the request was for e.g.
+		// v1 but the initial CRD used to create the wildcard partial metadata variant doesn't have v1. This is ok!
+		// Because this is a wildcard partial metadata request, we need *any* requestScope for *any* valid version
+		// from this CRD. Iterate through the valid requestScopes and pick the first one.
+		for _, s := range crdInfo.requestScopes {
+			requestScope = s
+			break
+		}
+	}
+
 	storage := crdInfo.storages[requestInfo.APIVersion].CustomResource
+	if storage == nil && wildcardPartialMetadata {
+		// If storage is nil and this is a wildcard partial metadata request, it means the request was for e.g.
+		// v1 but the initial CRD used to create the wildcard partial metadata variant doesn't have v1. This is ok!
+		// Because this is a wildcard partial metadata request, we need *any* storage for *any* valid version
+		// from this CRD. Iterate through the valid storages and pick the first one.
+		for _, s := range crdInfo.storages {
+			storage = s.CustomResource
+			break
+		}
+	}
 
 	switch requestInfo.Verb {
 	case "get":
@@ -534,12 +561,6 @@ func (r *crdHandler) serveResource(w http.ResponseWriter, req *http.Request, req
 			return nil
 		}
 
-		// kcp 2278 debugging
-		if strings.HasSuffix(string(crd.UID), ".wildcard.partial-metadata") {
-			klog.Errorf("kcp 2278: create with wildcard uid %v", string(crd.UID))
-		}
-		// kcp 2278 debugging
-
 		return handlers.CreateResource(storage, requestScope, r.admission)
 	case "update":
 		return handlers.UpdateResource(storage, requestScope, r.admission)
@@ -860,6 +881,10 @@ func (r *crdHandler) getOrCreateServingInfoFor(crd *apiextensionsv1.CustomResour
 		return nil, fmt.Errorf("error creating converter for %s: %w", crd.Name, err)
 	}
 
+	if strings.HasSuffix(string(crd.UID), ".wildcard.partial-metadata") {
+		converter = conversion.NewNOPConverter()
+	}
+
 	safeConverter, unsafeConverter, err := conversion.NewDelegatingConverter(crd, converter)
 	if err != nil {
 		return nil, err

diff --git a/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/kcp_rest_options_getter.go b/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/kcp_rest_options_getter.go
@@ -5,10 +5,10 @@ import (
 	"strings"
 
 	"github.com/kcp-dev/logicalcluster/v3"
+	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	"k8s.io/apimachinery/pkg/runtime/schema"
-	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/registry/generic"
 	"k8s.io/apiserver/pkg/storage/storagebackend"
 )
@@ -41,10 +41,20 @@ func (t apiBindingAwareCRDRESTOptionsGetter) GetRESTOptions(resource schema.Grou
 		return ret, nil
 	}
 
-	ret.StorageConfig.KcpExtraStorageMetadata.Cluster = genericapirequest.Cluster{Wildcard: true}
-	// Normal CRDs (not coming from an APIBinding) are stored in e.g. /registry/mygroup.io/widgets/customresources/...
+	ret.StorageConfig.KcpExtraStorageMetadata.Cluster.Wildcard = true
+
+	// Normal CRDs (not coming from an APIBinding) are stored in e.g. /registry/mygroup.io/widgets/<cluster name>/...
 	if _, bound := t.crd.Annotations["apis.kcp.io/bound-crd"]; !bound {
 		ret.ResourcePrefix += "/customresources"
+
+		clusterName := logicalcluster.From(t.crd)
+		if clusterName != "system:system-crds" {
+			// For all normal CRDs outside of the system:system-crds logical cluster, tell the watch cache the name
+			// of the logical cluster to use, and turn off wildcarding. This ensures the watch cache is just for
+			// this logical cluster.
+			ret.StorageConfig.KcpExtraStorageMetadata.Cluster.Name = clusterName
+			ret.StorageConfig.KcpExtraStorageMetadata.Cluster.Wildcard = false
+		}
 		return ret, nil
 	}
 

diff --git a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher.go
@@ -1109,7 +1109,8 @@ func (lw *cacherListerWatcher) List(options metav1.ListOptions) (runtime.Object,
 		Predicate:            pred,
 		Recursive:            true,
 	}
-	if err := lw.storage.GetList(createKCPClusterAwareContext(lw.kcpExtraStorageMetadata), lw.resourcePrefix, storageOpts, list); err != nil {
+
+	if err := lw.storage.GetList(createKCPClusterAwareContext(lw.kcpExtraStorageMetadata), lw.kcpAwareResourcePrefix(), storageOpts, list); err != nil {
 		return nil, err
 	}
 	return list, nil
@@ -1123,7 +1124,8 @@ func (lw *cacherListerWatcher) Watch(options metav1.ListOptions) (watch.Interfac
 		Recursive:       true,
 		ProgressNotify:  true,
 	}
-	return lw.storage.Watch(createKCPClusterAwareContext(lw.kcpExtraStorageMetadata), lw.resourcePrefix, opts)
+
+	return lw.storage.Watch(createKCPClusterAwareContext(lw.kcpExtraStorageMetadata), lw.kcpAwareResourcePrefix(), opts)
 }
 
 func createKCPClusterAwareContext(meta *storagebackend.KcpStorageMetadata) context.Context {

diff --git a/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_patch.go b/staging/src/k8s.io/apiserver/pkg/storage/cacher/cacher_patch.go
@@ -0,0 +1,27 @@
+/*
+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 cacher
+
+func (lw *cacherListerWatcher) kcpAwareResourcePrefix() string {
+	if lw.kcpExtraStorageMetadata.Cluster.Wildcard {
+		return lw.resourcePrefix
+	}
+
+	// This is a request for normal (non-bound) CRs outside of system:system-crds. Make sure we only list in the
+	// specific logical cluster.
+	return lw.resourcePrefix + "/" + lw.kcpExtraStorageMetadata.Cluster.Name.String()
+}