apis/apibindings: store served group resources consistently on Logica…

…lCluster

Signed-off-by: Dr. Stefan Schimanski <[email protected]>

config/shard/logicalcluster.yaml

@@ -0,0 +1,5 @@
+apiVersion: core.kcp.io/v1alpha1
+kind: LogicalCluster
+metadata:
+  name: cluster
+spec: {}

pkg/admission/crdnooverlappinggvr/crdnooverlappinggvr_admission.go

@@ -20,19 +20,25 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"strings"
+	"time"
 
 	"github.com/kcp-dev/logicalcluster/v3"
 
 	"k8s.io/apiextensions-apiserver/pkg/apis/apiextensions"
-	"k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apiserver/pkg/admission"
 	"k8s.io/apiserver/pkg/endpoints/request"
-	"k8s.io/kubernetes/pkg/controlplane/apiserver"
+	"k8s.io/client-go/util/retry"
+	"k8s.io/utils/ptr"
 
 	"github.com/kcp-dev/kcp/pkg/reconciler/apis/apibinding"
-	apisv1alpha1 "github.com/kcp-dev/kcp/sdk/apis/apis/v1alpha1"
+	corev1alpha1 "github.com/kcp-dev/kcp/sdk/apis/core/v1alpha1"
+	kcpclientset "github.com/kcp-dev/kcp/sdk/client/clientset/versioned/cluster"
 	kcpinformers "github.com/kcp-dev/kcp/sdk/client/informers/externalversions"
-	apisv1alpha1listers "github.com/kcp-dev/kcp/sdk/client/listers/apis/v1alpha1"
+	corev1alpha1listers "github.com/kcp-dev/kcp/sdk/client/listers/core/v1alpha1"
 )
 
 const (
@@ -44,14 +50,18 @@ func Register(plugins *admission.Plugins) {
 		func(_ io.Reader) (admission.Interface, error) {
 			return &crdNoOverlappingGVRAdmission{
 				Handler: admission.NewHandler(admission.Create),
+				now:     metav1.Now,
 			}, nil
 		})
 }
 
 type crdNoOverlappingGVRAdmission struct {
 	*admission.Handler
 
-	apiBindingClusterLister apisv1alpha1listers.APIBindingClusterLister
+	updateLogicalCluster func(ctx context.Context, logicalCluster *corev1alpha1.LogicalCluster, opts metav1.UpdateOptions) (*corev1alpha1.LogicalCluster, error)
+	logicalclusterLister corev1alpha1listers.LogicalClusterClusterLister
+
+	now func() metav1.Time
 }
 
 // Ensure that the required admission interfaces are implemented.
@@ -60,12 +70,21 @@ var _ = admission.InitializationValidator(&crdNoOverlappingGVRAdmission{})
 
 func (p *crdNoOverlappingGVRAdmission) SetKcpInformers(local, global kcpinformers.SharedInformerFactory) {
 	p.SetReadyFunc(local.Apis().V1alpha1().APIBindings().Informer().HasSynced)
-	p.apiBindingClusterLister = local.Apis().V1alpha1().APIBindings().Lister()
+	p.logicalclusterLister = local.Core().V1alpha1().LogicalClusters().Lister()
+}
+
+func (p *crdNoOverlappingGVRAdmission) SetKcpClusterClient(c kcpclientset.ClusterInterface) {
+	p.updateLogicalCluster = func(ctx context.Context, logicalCluster *corev1alpha1.LogicalCluster, opts metav1.UpdateOptions) (*corev1alpha1.LogicalCluster, error) {
+		return c.CoreV1alpha1().LogicalClusters().Cluster(logicalcluster.From(logicalCluster).Path()).Update(ctx, logicalCluster, opts)
+	}
 }
 
 func (p *crdNoOverlappingGVRAdmission) ValidateInitialization() error {
-	if p.apiBindingClusterLister == nil {
-		return fmt.Errorf(PluginName + " plugin needs an APIBindings lister")
+	if p.logicalclusterLister == nil {
+		return fmt.Errorf(PluginName + " plugin needs an LogicalCluster lister")
+	}
+	if p.updateLogicalCluster == nil {
+		return fmt.Errorf(PluginName + " plugin needs a KCP cluster client")
 	}
 	return nil
 }
@@ -78,35 +97,60 @@ func (p *crdNoOverlappingGVRAdmission) Validate(ctx context.Context, a admission
 	if a.GetKind().GroupKind() != apiextensions.Kind("CustomResourceDefinition") {
 		return nil
 	}
-	cluster, err := request.ClusterNameFrom(ctx)
+	if a.GetOperation() != admission.Create {
+		return nil
+	}
+
+	clusterName, err := request.ClusterNameFrom(ctx)
 	if err != nil {
 		return fmt.Errorf("failed to retrieve cluster from context: %w", err)
 	}
-	clusterName := logicalcluster.Name(cluster.String()) // TODO(sttts): remove this cast once ClusterNameFrom returns a tenancy.Name
-	// ignore CRDs targeting system and non-root workspaces
-	if clusterName == apibinding.SystemBoundCRDsClusterName || clusterName == apiserver.LocalAdminCluster {
+
+	if clusterName == apibinding.SystemBoundCRDsClusterName {
+		// bound CRDs will have equal group and resource names.
 		return nil
 	}
 
 	crd, ok := a.GetObject().(*apiextensions.CustomResourceDefinition)
 	if !ok {
 		return fmt.Errorf("unexpected type %T", a.GetObject())
 	}
-	apiBindingsForCurrentClusterName, err := p.listAPIBindingsFor(clusterName)
-	if err != nil {
+
+	// (optimistically) lock group resource for LogicalCluster. If this request
+	// eventually fails, the logicalclustercleanup controller will clean them
+	// up eventually.
+	gr := schema.GroupResource{Group: crd.Spec.Group, Resource: crd.Spec.Names.Plural}
+	var skipped map[schema.GroupResource]apibinding.Lock
+	err = retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		lc, err := p.logicalclusterLister.Cluster(clusterName).Get(corev1alpha1.LogicalClusterName)
+		if errors.IsNotFound(err) && strings.HasPrefix(string(clusterName), "system:") {
+			// in system logical clusters this is not fatal. We usually don't have a LogicalCluster there.
+			return nil
+		} else if err != nil {
+			return fmt.Errorf("failed to get LogicalCluster in logical cluster %q: %w", clusterName, err)
+		}
+
+		var updated *corev1alpha1.LogicalCluster
+		updated, _, skipped, err = apibinding.WithLockedResources(nil, time.Now(), lc, []schema.GroupResource{gr}, apibinding.ExpirableLock{
+			Lock:      apibinding.Lock{CRD: true},
+			CRDExpiry: ptr.To(p.now()),
+		})
+		if err != nil {
+			return fmt.Errorf("failed to lock resources %s in logical cluster %q: %w", gr, logicalcluster.From(crd), err)
+		}
+
+		_, err = p.updateLogicalCluster(ctx, updated, metav1.UpdateOptions{})
+		if err != nil {
+			return err
+		}
 		return err
+	})
+	if err != nil {
+		return fmt.Errorf("failed to lock resources %s in logical cluster %q: %w", gr, logicalcluster.From(crd), err)
 	}
-	for _, apiBindingForCurrentClusterName := range apiBindingsForCurrentClusterName {
-		for _, boundResource := range apiBindingForCurrentClusterName.Status.BoundResources {
-			if boundResource.Group == crd.Spec.Group && boundResource.Resource == crd.Spec.Names.Plural {
-				return admission.NewForbidden(a, fmt.Errorf("cannot create %q CustomResourceDefinition with %q group and %q resource because it overlaps with a bound CustomResourceDefinition for %q APIBinding in %q logical cluster",
-					crd.Name, crd.Spec.Group, crd.Spec.Names.Plural, apiBindingForCurrentClusterName.Name, clusterName))
-			}
-		}
+	if len(skipped) > 0 {
+		return admission.NewForbidden(a, fmt.Errorf("cannot create because resource is bound by APIBinding %q", skipped[gr].Name))
 	}
-	return nil
-}
 
-func (p *crdNoOverlappingGVRAdmission) listAPIBindingsFor(clusterName logicalcluster.Name) ([]*apisv1alpha1.APIBinding, error) {
-	return p.apiBindingClusterLister.Cluster(clusterName).List(labels.Everything())
+	return nil
 }