Add CatalogEntry validation in controller

1. Validate export references in entry spec
2. Aggregate PermissionClaims and API resources info from
referenced APIExport to entry status

Signed-off-by: Vu Dinh <[email protected]>

config/kcp/catalogentry.apiexport.yaml

@@ -0,0 +1,7 @@
+apiVersion: apis.kcp.dev/v1alpha1
+kind: APIExport
+metadata:
+  name: catalog.kcp.dev
+spec:
+  latestResourceSchemas:
+    - catalogentry.catalog.kcp.dev

config/kcp/catalogentry.apiresourceschemas.yaml

@@ -0,0 +1,174 @@
+---
+apiVersion: apis.kcp.dev/v1alpha1
+kind: APIResourceSchema
+metadata:
+  creationTimestamp: null
+  name: catalogentries.catalog.kcp.dev
+spec:
+  group: catalog.kcp.dev
+  names:
+    kind: CatalogEntry
+    listKind: CatalogEntryList
+    plural: catalogentries
+    singular: catalogentry
+  scope: Cluster
+  versions:
+  - name: v1alpha1
+    schema:
+      openAPIV3Schema:
+        description: CatalogEntry is the Schema for the catalogentries API
+        properties:
+          apiVersion:
+            description: 'APIVersion defines the versioned schema of this representation
+              of an object. Servers should convert recognized schemas to the latest
+              internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
+            type: string
+          kind:
+            description: 'Kind is a string value representing the REST resource this
+              object represents. Servers may infer this from the endpoint the client
+              submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
+            type: string
+          metadata:
+            type: object
+          spec:
+            description: CatalogEntrySpec defines the desired state of CatalogEntry
+            properties:
+              description:
+                description: description is a human-readable message to describe the
+                  information regarding the capabilities and features that the API
+                  provides
+                type: string
+              exports:
+                description: exports is a list of references to APIExports.
+                items:
+                  description: ExportReference describes a reference to an APIExport.
+                    Exactly one of the fields must be set.
+                  properties:
+                    workspace:
+                      description: workspace is a reference to an APIExport in the
+                        same organization. The creator of the APIBinding needs to
+                        have access to the APIExport with the verb `bind` in order
+                        to bind to it.
+                      properties:
+                        exportName:
+                          description: Name of the APIExport that describes the API.
+                          type: string
+                        path:
+                          description: path is an absolute reference to a workspace,
+                            e.g. root:org:ws. The workspace must be some ancestor
+                            or a child of some ancestor. If it is unset, the path
+                            of the APIBinding is used.
+                          pattern: ^root(:[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$
+                          type: string
+                      required:
+                      - exportName
+                      type: object
+                  type: object
+                minItems: 1
+                type: array
+            required:
+            - exports
+            type: object
+          status:
+            description: CatalogEntryStatus defines the observed state of CatalogEntry
+            properties:
+              conditions:
+                description: conditions is a list of conditions that apply to the
+                  CatalogEntry.
+                items:
+                  description: Condition defines an observation of a object operational
+                    state.
+                  properties:
+                    lastTransitionTime:
+                      description: Last time the condition transitioned from one status
+                        to another. This should be when the underlying condition changed.
+                        If that is not known, then using the time when the API field
+                        changed is acceptable.
+                      format: date-time
+                      type: string
+                    message:
+                      description: A human readable message indicating details about
+                        the transition. This field may be empty.
+                      type: string
+                    reason:
+                      description: The reason for the condition's last transition
+                        in CamelCase. The specific API may choose whether or not this
+                        field is considered a guaranteed API. This field may not be
+                        empty.
+                      type: string
+                    severity:
+                      description: Severity provides an explicit classification of
+                        Reason code, so the users or machines can immediately understand
+                        the current situation and act accordingly. The Severity field
+                        MUST be set only when Status=False.
+                      type: string
+                    status:
+                      description: Status of the condition, one of True, False, Unknown.
+                      type: string
+                    type:
+                      description: Type of condition in CamelCase or in foo.example.com/CamelCase.
+                        Many .condition.type values are consistent across resources
+                        like Available, but because arbitrary conditions can be useful
+                        (see .node.status.conditions), the ability to deconflict is
+                        important.
+                      type: string
+                  required:
+                  - lastTransitionTime
+                  - status
+                  - type
+                  type: object
+                type: array
+              exportPermissionClaims:
+                description: exportPermissionClaims is a list of permissions requested
+                  by the API provider(s) for this catalog entry.
+                items:
+                  description: PermissionClaim identifies an object by GR and identity
+                    hash. It's purpose is to determine the added permisions that a
+                    service provider may request and that a consumer may accept and
+                    alllow the service provider access to.
+                  properties:
+                    group:
+                      description: group is the name of an API group. For core groups
+                        this is the empty string '""'.
+                      pattern: ^(|[a-z0-9]([-a-z0-9]*[a-z0-9](\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*)?)$
+                      type: string
+                    identityHash:
+                      description: This is the identity for a given APIExport that
+                        the APIResourceSchema belongs to. The hash can be found on
+                        APIExport and APIResourceSchema's status. It will be empty
+                        for core types. Note that one must look this up for a particular
+                        KCP instance.
+                      type: string
+                    resource:
+                      description: 'resource is the name of the resource. Note: it
+                        is worth noting that you can not ask for permissions for resource
+                        provided by a CRD not provided by an api export.'
+                      pattern: ^[a-z][-a-z0-9]*[a-z0-9]$
+                      type: string
+                  required:
+                  - resource
+                  type: object
+                type: array
+              resources:
+                description: resources is the list of APIs that are provided by this
+                  catalog entry.
+                items:
+                  description: GroupResource specifies a Group and a Resource, but
+                    does not force a version.  This is useful for identifying concepts
+                    during lookup stages without having partially valid types
+                  properties:
+                    group:
+                      type: string
+                    resource:
+                      type: string
+                  required:
+                  - group
+                  - resource
+                  type: object
+                type: array
+            type: object
+        type: object
+    served: true
+    storage: true
+    subresources:
+      status: {}

controllers/catalogentry_controller.go

@@ -22,6 +22,9 @@ import (
 	"github.com/kcp-dev/catalog/api/v1alpha1"
 	catalogv1alpha1 "github.com/kcp-dev/catalog/api/v1alpha1"
 	apisv1alpha1 "github.com/kcp-dev/kcp/pkg/apis/apis/v1alpha1"
+	"github.com/kcp-dev/kcp/pkg/apis/third_party/conditions/util/conditions"
+	conditionsapi "github.com/kcp-dev/kcp/pkg/apis/third_party/conditions/apis/conditions/v1alpha1"
+	"github.com/kcp-dev/kcp/pkg/logging"
 	"github.com/kcp-dev/logicalcluster"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -31,6 +34,10 @@ import (
 	ctrllog "sigs.k8s.io/controller-runtime/pkg/log"
 )
 
+const (
+	controllerName = "kcp-catalogentry"
+)
+
 // CatalogEntryReconciler reconciles a CatalogEntry object
 type CatalogEntryReconciler struct {
 	client.Client
@@ -41,50 +48,123 @@ type CatalogEntryReconciler struct {
 //+kubebuilder:rbac:groups=catalog.kcp.dev,resources=catalogentries/status,verbs=get;update;patch
 //+kubebuilder:rbac:groups=catalog.kcp.dev,resources=catalogentries/finalizers,verbs=update
 
-// Reconcile is part of the main kubernetes reconciliation loop which aims to
-// move the current state of the cluster closer to the desired state.
-// TODO(user): Modify the Reconcile function to compare the state specified by
-// the CatalogEntry object against the actual cluster state, and then
-// perform operations to make the cluster state reflect the state specified by
-// the user.
-//
-// For more details, check Reconcile and its Result here:
-// - https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/reconcile
+// Reconcile validates exports in CatalogEntry spec and add a condition to status
+// to reflect the outcome of the validation.
+// It also aggregates all permissionClaims and api resources from referenced APIExport
+// to CatalogEntry status
 func (r *CatalogEntryReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	log := ctrllog.FromContext(ctx)
-
+	logger := logging.WithReconciler(klog.Background(), controllerName)
+	logger = logger.WithValues("clusterName", req.ClusterName)
 	ctx = logicalcluster.WithCluster(ctx, logicalcluster.New(req.ClusterName))
+
 	// Fetch the catalog entry from the request
 	catalogEntry := &v1alpha1.CatalogEntry{}
 	err := r.Get(ctx, req.NamespacedName, catalogEntry)
 	if err != nil {
 		if errors.IsNotFound(err) {
 			// Request object not found, could have been deleted after reconcile request.
 			// Owned objects are automatically garbage collected.
-			log.Info("Catalog Entry not found. Ignoring since object must be deleted")
+			logger.Info("CatalogEntry not found")
 			return ctrl.Result{}, nil
 		}
 		// Error reading the object - requeue the request.
-		log.Error(err, "Failed to get resource")
+		logger.Error(err, "failed to get resource")
 		return ctrl.Result{}, err
 	}
 
-	apiExportNameReferences := catalogEntry.Spec.References
-
-	for _, exportRef := range apiExportNameReferences {
+	changed := false
+	entryStatus := &v1alpha1.CatalogEntryStatus{}
+	resources := []metav1.GroupResource{}
+	exportPermissionClaims := []apisv1alpha1.PermissionClaim
+	invalidExports := []string
+	for _, exportRef := range catalogEntry.Spec.Exports {
 		// TODO: verify if path contains the entire heirarchy or just the clusterName.
 		// If it contains the heirarchy then extract the clusterName
 		path := exportRef.Workspace.Path
 		name := exportRef.Workspace.ExportName
-		clusterApiExport := apisv1alpha1.APIExport{}
-		err := r.Get(logicalcluster.WithCluster(ctx, logicalcluster.New(path)), types.NamespacedName{Name: name, Namespace: req.Namespace}, &clusterApiExport)
+		logger = logger.WithValues(
+			"path", path,
+			"exportName", name,
+		)
+		logger.V(2).Info("reconciling CatalogEntry")
+		export := apisv1alpha1.APIExport{}
+		err := r.Get(logicalcluster.WithCluster(ctx, logicalcluster.New(path)), types.NamespacedName{Name: name, Namespace: req.Namespace}, &export)
 		if err != nil {
+			invalidExports = append(invalidExports, fmt.Sprintf("%s/%s", path, name))
 			if errors.IsNotFound(err) {
-				log.Error(err, "APIExport referenced in catalog entry does not exist")
-				return ctrl.Result{}, err
+				logger.Error(err, "APIExport referenced in catalog entry does not exist")
+				continue
 			}
 			// Error reading the object - requeue the request.
-			log.Error(err, "Failed to get resource")
+			logger.Error(err, "failed to get resource")
+			continue
+		}
+
+		// Extract permission and API resource info
+		for _, claim := range export.Spec.PermissionClaims {
+			exportPermissionClaims = append(exportPermissionClaims, claim)
+		}
+		for _, schemaName := range export.Spec.LatestResourceSchemas {
+			_, resource, group, ok := split3(schemaName, ".")
+			if !ok {
+				continue
+			}
+			gr := metav1.GroupVersion{
+				Group:    group,
+				Resource: resource,
+			}
+			resources = append(resources, gr)
+		}
+	}
+
+	if len(invalidExports) = 0 {
+		// All exports are valid. Set APIExportValid condition to true if not existed already
+		if !conditions.IsTrue(catalogEntry, catalogv1alpha1.APIExportValidType) {
+			changed = true
+			validCond := conditionsapi.Condition{
+				Type:               catalogv1alpha1.APIExportValidType,
+				Status:             corev1.ConditionTrue,
+				Severity:           conditionsapi.ConditionSeverityNone,
+				LastTransitionTime: metav1.Now(),
+			}
+			conditions.Set(catalogEntry, validCond)
+		}
+	} else {
+		message := fmt.Sprintf("invalid export(s): %s", strings.Join(invalidExports, " ,"))
+		invalidCond := conditionsapi.Condition{
+			Type:               catalogv1alpha1.APIExportValidType,
+			Status:             corev1.ConditionFalse,
+			Severity:           conditionsapi.ConditionSeverity,
+			LastTransitionTime: metav1.Now(),
+			Message: message,
+		}
+		cond := conditions.Get(catalogEntry, invalidCond)
+		if cond != nil {
+			if !cond.Match(invalidCond) {
+				changed = true
+				conditions.Set(catalogEntry, invalidCond)
+			}
+		} else {
+			changed = true
+			conditions.Set(catalogEntry, invalidCond)
+		}
+	}
+
+	// Check if status is changed
+	if !reflect.DeepEqual(catalogEntry.Status.PermissionClaim, export.Spec.PermissionClaims) {
+		changed = true
+		entryStatus.ExportPermissionClaims = export.Spec.PermissionClaims
+	}
+	if !reflect.DeepEqual(catalogEntry.Status.Resources, resources) {
+		changed = true
+		entryStatus.Resources = resources
+	}
+
+	// Update the catalog entry if status is changed
+	if changed {
+		err = r.Client.Status().Update(context.TODO(), catalogEntry)
+		if err != nil {
+			logger.Error(err, "failed to update CatalogEntry")
 			return ctrl.Result{}, err
 		}
 	}