Address more comments

Signed-off-by: Mangirdas Judeikis <mangirdas@judeikis.lt>
 On-behalf-of: @SAP mangirdas.judeikis@sap.com

Author: mjudeikis

backend/controllers/clusterbinding/clusterbinding_reconcile.go

@@ -19,8 +19,9 @@ package clusterbinding
 import (
 	"context"
 	"fmt"
+	"maps"
 	"reflect"
-	"sort"
+	"slices"
 	"time"
 
 	corev1 "k8s.io/api/core/v1"
@@ -149,37 +150,36 @@ func (r *reconciler) ensureRBACClusterRole(ctx context.Context, client client.Cl
 				},
 			},
 		},
-	}
+		Rules: []rbacv1.PolicyRule{
+			// Always need to be able to get/list/watch the BoundSchemas
+			// to be able to figure out what to bind.
+			{
+				APIGroups: []string{kubebindv1alpha2.GroupName},
+				Resources: []string{"boundschemas"},
+				Verbs:     []string{"get", "list", "watch"},
+			},
+		}}
 	for _, export := range exports {
 		// Collect unique GroupResources and sort for stable rule ordering.
 		grSet := map[string]kubebindv1alpha2.GroupResource{}
 		for _, res := range export.Spec.Resources {
 			key := res.ResourceGroupName()
 			grSet[key] = kubebindv1alpha2.GroupResource{Group: res.Group, Resource: res.Resource}
 		}
-		keys := make([]string, 0, len(grSet))
-		for k := range grSet {
-			keys = append(keys, k)
-		}
-		sort.Strings(keys)
+		keys := slices.Collect(maps.Keys(grSet))
+		slices.Sort(keys)
 		for _, k := range keys {
-			gr := grSet[k]
-			name := gr.Resource + "." + gr.Group
-			schema, err := r.getBoundSchema(ctx, cache, clusterBinding.Namespace, name)
+			// k is already normalized (e.g., "pods.core" for empty group).
+			schema, err := r.getBoundSchema(ctx, cache, clusterBinding.Namespace, k)
 			if err != nil {
-				return fmt.Errorf("failed to get BoundSchema %q: %w", name, err)
+				return fmt.Errorf("failed to get BoundSchema %q: %w", k, err)
 			}
 			expected.Rules = append(expected.Rules,
 				rbacv1.PolicyRule{
 					APIGroups: []string{schema.Spec.Group},
 					Resources: []string{schema.Spec.Names.Plural},
 					Verbs:     []string{"get", "list", "watch", "create", "update", "patch", "delete"},
 				},
-				rbacv1.PolicyRule{
-					APIGroups: []string{kubebindv1alpha2.GroupName},
-					Resources: []string{"boundschemas"},
-					Verbs:     []string{"get", "list", "watch"},
-				},
 			)
 		}
 	}
@@ -205,7 +205,6 @@ func (r *reconciler) ensureRBACClusterRoleBinding(ctx context.Context, client cl
 	if err != nil && !errors.IsNotFound(err) {
 		return fmt.Errorf("failed to get ClusterRoleBinding %s: %w", name, err)
 	}
-
 	if r.scope != kubebindv1alpha2.ClusterScope {
 		if err := r.deleteClusterRoleBinding(ctx, client, name); err != nil && !errors.IsNotFound(err) {
 			return fmt.Errorf("failed to delete ClusterRoleBinding %s: %w", name, err)

backend/controllers/serviceexport/serviceexport_controller.go

@@ -130,18 +130,18 @@ func (r *APIServiceExportReconciler) Reconcile(ctx context.Context, req mcreconc
 		return ctrl.Result{}, err
 	}
 
-	// Update annotatations changed (hash)
+	// Update annotations changed (hash), we need to propagate it and requeue for status changes.
+	// This is why we compare annotations only as we don't expect any changes to spec.
+	// Status changes are handled below.
 	if !equality.Semantic.DeepEqual(original.Annotations, apiServiceExport.Annotations) {
-		err := client.Update(ctx, apiServiceExport)
-		if err != nil {
-			return ctrl.Result{}, fmt.Errorf("failed to update APIServiceExport status: %w", err)
+		if err := client.Update(ctx, apiServiceExport); err != nil {
+			return ctrl.Result{}, fmt.Errorf("failed to update APIServiceExport: %w", err)
 		}
 		logger.Info("APIServiceExport hash updated", "namespace", apiServiceExport.Namespace, "name", apiServiceExport.Name)
 		return ctrl.Result{Requeue: true}, nil
 	}
 	if !equality.Semantic.DeepEqual(original.Status, apiServiceExport.Status) {
-		err := client.Status().Update(ctx, apiServiceExport)
-		if err != nil {
+		if err := client.Status().Update(ctx, apiServiceExport); err != nil {
 			return ctrl.Result{}, fmt.Errorf("failed to update APIServiceExport status: %w", err)
 		}
 		logger.Info("APIServiceExport status updated", "namespace", apiServiceExport.Namespace, "name", apiServiceExport.Name)
@@ -154,7 +154,7 @@ func (r *APIServiceExportReconciler) Reconcile(ctx context.Context, req mcreconc
 func getBoundSchemaMapper(clusterName string, cl cluster.Cluster) handler.TypedEventHandler[client.Object, mcreconcile.Request] {
 	return handler.TypedEnqueueRequestsFromMapFunc(func(ctx context.Context, obj client.Object) []mcreconcile.Request {
 		boundSchema := obj.(*kubebindv1alpha2.BoundSchema)
-		boundSchemaKey := boundSchema.Name
+		boundSchemaKey := boundSchema.Spec.Names.Plural + "." + boundSchema.Spec.Group
 		c := cl.GetClient()
 
 		var exports kubebindv1alpha2.APIServiceExportList

backend/controllers/serviceexport/serviceexport_reconcile.go

@@ -51,15 +51,16 @@ func (r *reconciler) ensureSchema(ctx context.Context, cache cache.Cache, export
 	schemas := make([]*kubebindv1alpha2.BoundSchema, 0, len(export.Spec.Resources))
 
 	for _, res := range export.Spec.Resources {
-		schema, err := r.getBoundSchema(ctx, cache, export.Namespace, res.ResourceGroupName())
+		name := res.ResourceGroupName()
+		schema, err := r.getBoundSchema(ctx, cache, export.Namespace, name)
 		if err != nil {
 			if errors.IsNotFound(err) {
 				conditions.MarkFalse(
 					export,
 					kubebindv1alpha2.APIServiceExportConditionProviderInSync,
 					"BoundSchemaMissing",
 					conditionsapi.ConditionSeverityError,
-					"BoundSchemas are not available: %v", schema.Name)
+					"BoundSchema %q is not available: %v", name, err)
 				return nil
 			}
 			return err
@@ -68,7 +69,10 @@ func (r *reconciler) ensureSchema(ctx context.Context, cache cache.Cache, export
 		schemas = append(schemas, schema)
 	}
 
-	hash := helpers.BoundSchemasSpecHash(schemas)
+	hash, err := helpers.BoundSchemasSpecHash(schemas)
+	if err != nil {
+		return err
+	}
 
 	if export.Annotations[kubebindv1alpha2.SourceSpecHashAnnotationKey] != hash {
 		// both exist, update APIServiceExport

backend/controllers/serviceexportrequest/serviceexportrequest_controller.go

@@ -153,7 +153,7 @@ func getServiceExportRequestMapper(clusterName string, cl cluster.Cluster) handl
 func getBoundSchemaMapper(clusterName string, cl cluster.Cluster) handler.TypedEventHandler[client.Object, mcreconcile.Request] {
 	return handler.TypedEnqueueRequestsFromMapFunc(func(ctx context.Context, obj client.Object) []mcreconcile.Request {
 		boundSchema := obj.(*kubebindv1alpha2.BoundSchema)
-		boundSchemaKey := boundSchema.Name
+		boundSchemaKey := boundSchema.Spec.Names.Plural + "." + boundSchema.Spec.Group
 		c := cl.GetClient()
 
 		var requests kubebindv1alpha2.APIServiceExportRequestList
@@ -215,13 +215,20 @@ func (r *APIServiceExportRequestReconciler) Reconcile(ctx context.Context, req m
 	// Run the reconciliation logic
 	if err := r.reconciler.reconcile(ctx, client, cache, apiServiceExportRequest); err != nil {
 		logger.Error(err, "Failed to reconcile APIServiceExportRequest")
+		if !reflect.DeepEqual(original.Status.Phase, apiServiceExportRequest.Status.Phase) {
+			if err := client.Status().Update(ctx, apiServiceExportRequest); err != nil {
+				logger.Error(err, "Failed to update APIServiceExportRequest status")
+				return ctrl.Result{}, fmt.Errorf("failed to update APIServiceExportRequest status: %w", err)
+			}
+			logger.Info("APIServiceExportRequest status updated", "namespace", apiServiceExportRequest.Namespace, "name", apiServiceExportRequest.Name)
+		}
+
 		return ctrl.Result{}, err
 	}
 
 	// Update status if it has changed
 	if !reflect.DeepEqual(original.Status, apiServiceExportRequest.Status) {
-		err := client.Status().Update(ctx, apiServiceExportRequest)
-		if err != nil {
+		if err := client.Status().Update(ctx, apiServiceExportRequest); err != nil {
 			logger.Error(err, "Failed to update APIServiceExportRequest status")
 			return ctrl.Result{}, fmt.Errorf("failed to update APIServiceExportRequest status: %w", err)
 		}

backend/controllers/serviceexportrequest/serviceexportrequest_reconcile.go

@@ -54,10 +54,12 @@ type reconciler struct {
 
 func (r *reconciler) reconcile(ctx context.Context, cl client.Client, cache cache.Cache, req *kubebindv1alpha2.APIServiceExportRequest) error {
 	if err := r.ensureBoundSchemas(ctx, cl, cache, req); err != nil {
+		conditions.SetSummary(req)
 		return err
 	}
 
 	if err := r.ensureExports(ctx, cl, cache, req); err != nil {
+		conditions.SetSummary(req)
 		return err
 	}
 
@@ -75,7 +77,7 @@ func (r *reconciler) ensureBoundSchemas(ctx context.Context, cl client.Client, c
 	for _, res := range req.Spec.Resources {
 		parts := strings.SplitN(r.schemaSource, ".", 3)
 		if len(parts) != 3 { // We check this in validation, but just in case.
-			return fmt.Errorf("invalid schema source: %q", r.schemaSource)
+			return fmt.Errorf("malformed schema source: %q", r.schemaSource)
 		}
 
 		gvk := schema.GroupVersionKind{
@@ -106,20 +108,57 @@ func (r *reconciler) ensureBoundSchemas(ctx context.Context, cl client.Client, c
 		}
 
 		for _, item := range list.Items {
+			var schemaFailed bool
 			obj := item.UnstructuredContent()
-			spec, ok := obj["spec"].(map[string]interface{})
-			if !ok {
-				klog.FromContext(ctx).Error(nil, "Skipping invalid schema: missing spec", "ns", item.GetNamespace(), "name", item.GetName())
-				continue
+			group, ok, err := unstructured.NestedString(obj, "spec", "group")
+			if !ok || err != nil || group == "" {
+				klog.FromContext(ctx).Error(err, "Skipping invalid schema: missing group", "ns", item.GetNamespace(), "name", item.GetName())
+				schemaFailed = true
 			}
-			g, _ := spec["group"].(string)
-			names, _ := spec["names"].(map[string]interface{})
-			p, _ := names["plural"].(string)
-			if g == "" || p == "" {
-				klog.FromContext(ctx).Error(nil, "Skipping invalid schema: missing group or names.plural", "ns", item.GetNamespace(), "name", item.GetName())
-				continue
+			plural, ok, err := unstructured.NestedString(obj, "spec", "names", "plural")
+			if !ok || err != nil || plural == "" {
+				klog.FromContext(ctx).Error(err, "Skipping invalid schema: missing names.plural", "ns", item.GetNamespace(), "name", item.GetName())
+				schemaFailed = true
 			}
-			if g == res.Group && p == res.Resource {
+
+			scope, ok, err := unstructured.NestedString(obj, "spec", "scope")
+			if !ok || err != nil || scope == "" {
+				klog.FromContext(ctx).Error(err, "Skipping invalid schema: missing scope", "ns", item.GetNamespace(), "name", item.GetName())
+				schemaFailed = true
+			}
+
+			if schemaFailed {
+				conditions.MarkFalse(
+					req,
+					kubebindv1alpha2.APIServiceExportRequestConditionExportsReady,
+					"APIServiceExportRequestInvalid",
+					conditionsapi.ConditionSeverityError,
+					"APIServiceExportRequest %s is invalid: resource %s/%s has invalid schema",
+					req.Name, group, plural,
+				)
+				req.Status.Phase = kubebindv1alpha2.APIServiceExportRequestPhaseFailed
+				return fmt.Errorf("resource %s/%s is invalid", group, plural)
+			}
+
+			if group == res.Group && plural == res.Resource {
+				// Important: This checks if the resource are correctly scoped. If consumer is namespaced, we can't allow this.
+				// We terminate early to prevent triggering other controllers.
+				if r.informerScope.String() != scope && r.informerScope != kubebindv1alpha2.ClusterScope {
+					conditions.MarkFalse(
+						req,
+						kubebindv1alpha2.APIServiceExportRequestConditionExportsReady,
+						"APIServiceExportRequestInvalid",
+						conditionsapi.ConditionSeverityError,
+						"APIServiceExportRequest %s is invalid: resource %s/%s has scope %q which is incompatible with backend informer scope %q",
+						req.Name, group, plural, scope, r.informerScope,
+					)
+					req.Status.Phase = kubebindv1alpha2.APIServiceExportRequestPhaseFailed
+					req.Status.TerminalMessage = conditions.GetMessage(req, kubebindv1alpha2.APIServiceExportRequestConditionExportsReady)
+					// We can't proceed with this request.
+					return fmt.Errorf("resource %s/%s has scope %q which is incompatible with backend informer scope %q", group, plural, scope, r.informerScope)
+				}
+
+				// https://github.com/kube-bind/kube-bind/issues/297 to fix.
 				boundSchema, err := helpers.UnstructuredToBoundSchema(item)
 				if err != nil {
 					return err
@@ -163,9 +202,9 @@ func (r *reconciler) ensureExports(ctx context.Context, cl client.Client, cache
 					conditions.MarkFalse(
 						req,
 						kubebindv1alpha2.APIServiceExportRequestConditionExportsReady,
-						"APIResourceSchemaNotFound",
+						"BoundSchemaNotFound",
 						conditionsapi.ConditionSeverityError,
-						"APIResourceSchema %s in the service provider cluster not found",
+						"BoundSchema %s in the service provider cluster not found",
 						name,
 					)
 					return err
@@ -189,7 +228,11 @@ func (r *reconciler) ensureExports(ctx context.Context, cl client.Client, cache
 			return nil
 		}
 
-		hash := helpers.BoundSchemasSpecHash(schemas)
+		// https://github.com/kube-bind/kube-bind/issues/297 To fix.
+		hash, err := helpers.BoundSchemasSpecHash(schemas)
+		if err != nil {
+			return err
+		}
 		export := &kubebindv1alpha2.APIServiceExport{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:      req.Name,

backend/http/handler.go

@@ -388,6 +388,12 @@ func (h *handler) handleResources(w http.ResponseWriter, r *http.Request) {
 			scope = "-"
 		}
 
+		// TODO(mjudeikis): This logic is very brittle, needs rework.
+		// This will be improved in the permissionClaims PR.
+		if !strings.EqualFold(h.scope.String(), scope.(string)) && h.scope != kubebindv1alpha2.ClusterScope {
+			continue
+		}
+
 		group := item.UnstructuredContent()["spec"].(map[string]interface{})["group"]
 		if group == nil {
 			group = "-"

backend/kubernetes/manager.go

@@ -192,24 +192,3 @@ func (m *Manager) ListDynamicResources(ctx context.Context, cluster string, gvk
 
 	return list, nil
 }
-
-/* func (m *Manager) CreateBoundSchema(ctx context.Context, cluster string, name string, u *unstructured.Unstructured) error {
-	cl, err := m.manager.GetCluster(ctx, cluster)
-	if err != nil {
-		return err
-	}
-	c := cl.GetClient()
-
-	boundSchema := &kubebindv1alpha2.BoundSchema{}
-	err = runtime.DefaultUnstructuredConverter.FromUnstructured(u.UnstructuredContent(), boundSchema)
-	if err != nil {
-		return err
-	}
-
-	boundSchema.ResourceVersion = ""
-	boundSchema.Name = name
-	boundSchema.Spec.InformerScope = m.scope
-
-	return c.Create(ctx, boundSchema)
-}
-*/

backend/options/options.go

@@ -56,7 +56,7 @@ type ExtraOptions struct {
 	// Defines the source of the schema for the bind screen.
 	// Options are:
 	// CustomResourceDefinition.v1.apiextensions.k8s.io
-	// APIResourceSchema.v1alpha2.apis.kcp.io
+	// APIResourceSchema.v1alpha1.apis.kcp.io
 	SchemaSource string
 
 	TestingAutoSelect         string

cli/pkg/kubectl/bind/plugin/bind.go

@@ -159,7 +159,7 @@ func (b *BindOptions) Run(ctx context.Context, urlCh chan<- string) error {
 	}
 
 	if provider.APIVersion != kubebindv1alpha2.GroupVersion {
-		return fmt.Errorf("unsupported binding provider version: %q != %q", provider.APIVersion, kubebindv1alpha2.GroupVersion)
+		return fmt.Errorf("unsupported binding provider version %q, expected %q", provider.APIVersion, kubebindv1alpha2.GroupVersion)
 	}
 
 	ns, err := kubeClient.CoreV1().Namespaces().Get(ctx, "kube-bind", metav1.GetOptions{})

deploy/crd/kube-bind.io_boundschemas.yaml

@@ -15,7 +15,7 @@ spec:
     listKind: BoundSchemaList
     plural: boundschemas
     shortNames:
-    - bas
+    - bs
     singular: boundschema
   scope: Namespaced
   versions: