From 3e06890177f6cdb7c994e612e5e46487329bd077 Mon Sep 17 00:00:00 2001 From: "Dr. Stefan Schimanski" Date: Thu, 16 Jan 2025 20:29:53 +0100 Subject: [PATCH] reconciler/apibinding: check conflict via bound APIs in bindings, not exports Signed-off-by: Dr. Stefan Schimanski --- .../apis/apibinding/apibinding_reconcile.go | 2 +- .../apibinding/apibinding_reconcile_test.go | 6 +++- .../apis/apibinding/conflict_checker.go | 36 +++---------------- .../apis/apibinding/conflict_checker_test.go | 22 ------------ 4 files changed, 10 insertions(+), 56 deletions(-) diff --git a/pkg/reconciler/apis/apibinding/apibinding_reconcile.go b/pkg/reconciler/apis/apibinding/apibinding_reconcile.go index e306fc2873d..95d7454e9e7 100644 --- 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 index 4695a289eff..628e4808430 100644 --- a/pkg/reconciler/apis/apibinding/apibinding_reconcile_test.go +++ b/pkg/reconciler/apis/apibinding/apibinding_reconcile_test.go @@ -163,6 +163,8 @@ func TestReconcileBinding(t *testing.T) { todaywidgetsuid := withName(newCRD("kcp.io", "widgets"), "todaywidgetsuid") anotherwidgetsuid := withName(newCRD("kcp.io", "widgets"), "anotherwidgetsuid") + uid1 := withName(newCRD("someresources.mygroup", "todays"), "uid1") + uid2 := withName(newCRD("someresources.anothergroup", "todays"), "uid2") type wantAPIExportValid struct { invalidReference bool @@ -298,6 +300,9 @@ func TestReconcileBinding(t *testing.T) { existingAPIBindings: []*apisv1alpha1.APIBinding{ bound.Build(), }, + crds: map[logicalcluster.Name][]*apiextensionsv1.CustomResourceDefinition{ + SystemBoundCRDsClusterName: {anotherwidgetsuid, uid1, uid2}, + }, wantCreateCRD: true, wantAPIExportValid: wantAPIExportValid{ valid: true, @@ -514,7 +519,6 @@ func TestReconcileBinding(t *testing.T) { return crd, nil } } - return nil, apierrors.NewNotFound(schema.GroupResource{}, name) }, listCRDs: func(clusterName logicalcluster.Name) ([]*apiextensionsv1.CustomResourceDefinition, error) { diff --git a/pkg/reconciler/apis/apibinding/conflict_checker.go b/pkg/reconciler/apis/apibinding/conflict_checker.go index 76f9df82e7d..ac0febc4c02 100644 --- 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 index 71e0b1a1050..c1b6add93d7 100644 --- 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 },