reconciler/apibinding: check conflict via bound APIs in bindings, not…

… exports Signed-off-by: Dr. Stefan Schimanski <[email protected]>
kcp-dev · Jan 17, 2025 · 260974e · 260974e
1 parent fc1e66a
commit 260974e
Show file tree

Hide file tree

Showing 5 changed files with 28 additions and 58 deletions.
diff --git a/pkg/admission/apibinding/validation.go b/pkg/admission/apibinding/validation.go
@@ -54,7 +54,7 @@ func ValidateAPIBindingUpdate(oldBinding, newBinding *apisv1alpha1.APIBinding) f
 // ValidateAPIBindingReference validates an APIBinding's BindingReference.
 func ValidateAPIBindingReference(reference apisv1alpha1.BindingReference, path *field.Path) field.ErrorList {
 	allErrs := field.ErrorList{}
-	
+
 	if reference.Export == nil {
 		allErrs = append(allErrs, field.Required(path.Child("export"), ""))
 	} else if reference.Export.Name == "" {

diff --git a/pkg/reconciler/apis/apibinding/apibinding_reconcile.go b/pkg/reconciler/apis/apibinding/apibinding_reconcile.go
@@ -211,7 +211,7 @@ func (r *bindingReconciler) reconcile(ctx context.Context, apiBinding *apisv1alp
 
 	var needToWaitForRequeueWhenEstablished []string
 
-	checker, err := newConflictChecker(logicalcluster.From(apiBinding), r.listAPIBindings, r.getAPIExportByPath, r.getAPIResourceSchema, r.getCRD, r.listCRDs)
+	checker, err := newConflictChecker(logicalcluster.From(apiBinding), r.listAPIBindings, r.getAPIResourceSchema, r.getCRD, r.listCRDs)
 	if err != nil {
 		return reconcileStatusContinue, err
 	}

diff --git a/pkg/reconciler/apis/apibinding/apibinding_reconcile_test.go b/pkg/reconciler/apis/apibinding/apibinding_reconcile_test.go
@@ -479,7 +479,8 @@ func TestReconcileBinding(t *testing.T) {
 						},
 					}
 
-					if name == "anotherwidgetsuid" {
+					switch name {
+					case "anotherwidgetsuid":
 						crd.Spec.Group = "kcp.io"
 						crd.Spec.Names = apiextensionsv1.CustomResourceDefinitionNames{
 							Plural: "widgets",
@@ -488,10 +489,29 @@ func TestReconcileBinding(t *testing.T) {
 							Plural: "widgets",
 						}
 						return crd, nil
+					case "uid1":
+						crd.Spec.Group = "someresources.mygroup"
+						crd.Spec.Names = apiextensionsv1.CustomResourceDefinitionNames{
+							Plural: "todays",
+						}
+						crd.Status.AcceptedNames = apiextensionsv1.CustomResourceDefinitionNames{
+							Plural: "todays",
+						}
+						return crd, nil
+					case "uid2":
+						crd.Spec.Group = "someresources.anothergroup"
+						crd.Spec.Names = apiextensionsv1.CustomResourceDefinitionNames{
+							Plural: "todays",
+						}
+						crd.Status.AcceptedNames = apiextensionsv1.CustomResourceDefinitionNames{
+							Plural: "todays",
+						}
+						return crd, nil
+					default:
 					}
 
 					if !tc.crdExists {
-						return nil, apierrors.NewNotFound(schema.GroupResource{}, "")
+						return nil, apierrors.NewNotFound(schema.GroupResource{}, name)
 					}
 
 					if tc.crdEstablished {

diff --git a/pkg/reconciler/apis/apibinding/conflict_checker.go b/pkg/reconciler/apis/apibinding/conflict_checker.go
@@ -37,7 +37,6 @@ func (u byUID) Swap(i, j int)      { u[i], u[j] = u[j], u[i] }
 
 type conflictChecker struct {
 	listAPIBindings      func(clusterName logicalcluster.Name) ([]*apisv1alpha1.APIBinding, error)
-	getAPIExportByPath   func(path logicalcluster.Path, name string) (*apisv1alpha1.APIExport, error)
 	getAPIResourceSchema func(clusterName logicalcluster.Name, name string) (*apisv1alpha1.APIResourceSchema, error)
 	getCRD               func(clusterName logicalcluster.Name, name string) (*apiextensionsv1.CustomResourceDefinition, error)
 	listCRDs             func(clusterName logicalcluster.Name) ([]*apiextensionsv1.CustomResourceDefinition, error)
@@ -50,14 +49,12 @@ type conflictChecker struct {
 // newConflictChecker creates a CRD conflict checker for the given cluster.
 func newConflictChecker(clusterName logicalcluster.Name,
 	listAPIBindings func(clusterName logicalcluster.Name) ([]*apisv1alpha1.APIBinding, error),
-	getAPIExportByPath func(path logicalcluster.Path, name string) (*apisv1alpha1.APIExport, error),
 	getAPIResourceSchema func(clusterName logicalcluster.Name, name string) (*apisv1alpha1.APIResourceSchema, error),
 	getCRD func(clusterName logicalcluster.Name, name string) (*apiextensionsv1.CustomResourceDefinition, error),
 	listCRDs func(clusterName logicalcluster.Name) ([]*apiextensionsv1.CustomResourceDefinition, error),
 ) (*conflictChecker, error) {
 	ncc := &conflictChecker{
 		listAPIBindings:      listAPIBindings,
-		getAPIExportByPath:   getAPIExportByPath,
 		getAPIResourceSchema: getAPIResourceSchema,
 		getCRD:               getCRD,
 		listCRDs:             listCRDs,
@@ -71,35 +68,8 @@ func newConflictChecker(clusterName logicalcluster.Name,
 		return nil, err
 	}
 	for _, b := range bindings {
-		if b.Spec.Reference.Export == nil {
-			// this should not happen because of validation.
-			return nil, fmt.Errorf("APIBinding %s|%s has no cluster reference", logicalcluster.From(b), b.Name)
-		}
-		path := logicalcluster.NewPath(b.Spec.Reference.Export.Path)
-		if path.Empty() {
-			path = logicalcluster.From(b).Path()
-		}
-		apiExport, err := ncc.getAPIExportByPath(path, b.Spec.Reference.Export.Name)
-		if err != nil {
-			return nil, err
-		}
-
-		boundSchemaUIDs := sets.New[string]()
-		for _, boundResource := range b.Status.BoundResources {
-			boundSchemaUIDs.Insert(boundResource.Schema.UID)
-		}
-
-		for _, schemaName := range apiExport.Spec.LatestResourceSchemas {
-			schema, err := ncc.getAPIResourceSchema(logicalcluster.From(apiExport), schemaName)
-			if err != nil {
-				return nil, err
-			}
-
-			if !boundSchemaUIDs.Has(string(schema.UID)) {
-				continue
-			}
-
-			crd, err := ncc.getCRD(SystemBoundCRDsClusterName, string(schema.UID))
+		for _, br := range b.Status.BoundResources {
+			crd, err := ncc.getCRD(SystemBoundCRDsClusterName, br.Schema.UID)
 			if err != nil {
 				return nil, err
 			}
@@ -121,6 +91,8 @@ func newConflictChecker(clusterName logicalcluster.Name,
 	return ncc, nil
 }
 
+// Check checks if the given schema from the given APIBinding conflicts with any
+// CRD or any other APIBinding.
 func (ncc *conflictChecker) Check(binding *apisv1alpha1.APIBinding, s *apisv1alpha1.APIResourceSchema) error {
 	for _, crd := range ncc.crds {
 		if other, found := ncc.crdToBinding[crd.Name]; found && other.Name == binding.Name {

diff --git a/pkg/reconciler/apis/apibinding/conflict_checker_test.go b/pkg/reconciler/apis/apibinding/conflict_checker_test.go
@@ -60,24 +60,6 @@ func TestNameConflictCheckerGetBoundCRDs(t *testing.T) {
 		).
 		Build()
 
-	apiExports := map[string]*apisv1alpha1.APIExport{
-		"export0": {
-			Spec: apisv1alpha1.APIExportSpec{
-				LatestResourceSchemas: []string{"export0-schema1"},
-			},
-		},
-		"export1": {
-			Spec: apisv1alpha1.APIExportSpec{
-				LatestResourceSchemas: []string{"export1-schema1", "export1-schema2", "export1-schema3"},
-			},
-		},
-		"export2": {
-			Spec: apisv1alpha1.APIExportSpec{
-				LatestResourceSchemas: []string{"export2-schema1", "export2-schema2", "export2-schema3"},
-			},
-		},
-	}
-
 	apiResourceSchemas := map[string]*apisv1alpha1.APIResourceSchema{
 		"export0-schema1": {ObjectMeta: metav1.ObjectMeta{UID: "e0-s1"}},
 		"export1-schema1": {ObjectMeta: metav1.ObjectMeta{UID: "e1-s1"}},
@@ -96,9 +78,6 @@ func TestNameConflictCheckerGetBoundCRDs(t *testing.T) {
 				existingBinding2,
 			}, nil
 		},
-		func(path logicalcluster.Path, name string) (*apisv1alpha1.APIExport, error) {
-			return apiExports[name], nil
-		},
 		func(clusterName logicalcluster.Name, name string) (*apisv1alpha1.APIResourceSchema, error) {
 			return apiResourceSchemas[name], nil
 		},
@@ -276,7 +255,6 @@ func TestCRDs(t *testing.T) {
 				func(clusterName logicalcluster.Name) ([]*apisv1alpha1.APIBinding, error) {
 					return nil, nil
 				},
-				func(path logicalcluster.Path, name string) (*apisv1alpha1.APIExport, error) { return nil, nil },
 				func(clusterName logicalcluster.Name, name string) (*apisv1alpha1.APIResourceSchema, error) {
 					return nil, nil
 				},