replace validation hack with CEL expressions

On-behalf-of: @SAP [email protected]

Author: xrstf

deploy/crd/kcp.io/syncagent.kcp.io_publishedresources.yaml

@@ -354,6 +354,12 @@ spec:
                     resource, like a Secret containing generated credentials. Related objects are
                     synced along the main resource.
                   items:
+                    description: |-
+                      RelatedResourceSpec describes a single related resource, which might point to
+                      any number of actual Kubernetes objects.
+
+                      (in the following rule, group is optional becaue core/v1 is represented by group="")
+                      group is included here because when an identityHash is used, core/v1 cannot possible be targetted
                     properties:
                       group:
                         description: |-
@@ -378,6 +384,9 @@ spec:
 
                           Deprecated: Use "Resource" instead. This field is limited to "ConfigMap" and "Secret" and will
                           be removed in the future. Kind and Resource cannot be specified at the same time.
+                        enum:
+                          - ConfigMap
+                          - Secret
                         type: string
                       mutation:
                         description: |-
@@ -734,6 +743,15 @@ spec:
                       - object
                       - origin
                     type: object
+                    x-kubernetes-validations:
+                      - message: must specify either kind (deprecated) or group, version, resource
+                        rule: has(self.kind) != (has(self.version) || has(self.resource))
+                      - message: resource and version must be configured together or not at all
+                        rule: has(self.resource) == has(self.version)
+                      - message: configuring a group also requires a version and resource
+                        rule: '!has(self.group) || (has(self.resource) && has(self.version))'
+                      - message: identity hashes can only be used with GVRs
+                        rule: '!has(self.identityHash) || (has(self.group) && has(self.version) && has(self.resource))'
                   type: array
                 resource:
                   description: |-

internal/controller/apiexport/controller.go

@@ -19,7 +19,6 @@ package apiexport
 import (
 	"context"
 	"fmt"
-	"slices"
 
 	"github.com/kcp-dev/logicalcluster/v3"
 	"go.uber.org/zap"
@@ -30,7 +29,6 @@ import (
 	"github.com/kcp-dev/api-syncagent/internal/kcp"
 	"github.com/kcp-dev/api-syncagent/internal/projection"
 	"github.com/kcp-dev/api-syncagent/internal/resources/reconciling"
-	"github.com/kcp-dev/api-syncagent/internal/validation"
 	syncagentv1alpha1 "github.com/kcp-dev/api-syncagent/sdk/apis/syncagent/v1alpha1"
 
 	kcpdevv1alpha1 "github.com/kcp-dev/kcp/sdk/apis/apis/v1alpha1"
@@ -131,31 +129,20 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 }
 
 func (r *Reconciler) reconcile(ctx context.Context, apiExport *kcpdevv1alpha1.APIExport) error {
-	// find all PublishedResources
-	pubResources := &syncagentv1alpha1.PublishedResourceList{}
-	if err := r.localClient.List(ctx, pubResources, &ctrlruntimeclient.ListOptions{
-		LabelSelector: r.prFilter,
-	}); err != nil {
-		return fmt.Errorf("failed to list PublishedResources: %w", err)
-	}
-
-	// filter out those PRs that are invalid; we keep those that are not yet converted into ARS,
+	// find all PublishedResources; we keep those that are not yet converted into ARS,
 	// just to reduce the amount of re-reconciles when the agent processes a number of PRs in a row
 	// and would constantly update the APIExport; instead we rely on kcp to handle the eventual
 	// consistency.
 	// This allows us to already determine all resources we "own", which helps us in
 	// bilding the proper permission claims if one of the related resources is using a resource
 	// type managed via PublishedResource. Otherwise the controller might not see the PR for the
 	// related resource and temporarily incorrectly assume it needs to add a permission claim.
-	filteredPubResources := slices.DeleteFunc(pubResources.Items, func(pr syncagentv1alpha1.PublishedResource) bool {
-		// TODO: Turn this into a webhook or CEL expressions.
-		err := validation.ValidatePublishedResource(&pr)
-		if err != nil {
-			r.log.With("pr", pr.Name, "error", err).Warn("Ignoring invalid PublishedResource.")
-		}
-
-		return err != nil
-	})
+	pubResources := &syncagentv1alpha1.PublishedResourceList{}
+	if err := r.localClient.List(ctx, pubResources, &ctrlruntimeclient.ListOptions{
+		LabelSelector: r.prFilter,
+	}); err != nil {
+		return fmt.Errorf("failed to list PublishedResources: %w", err)
+	}
 
 	// Create two lists of schema names: ready schemas are those already processed by the
 	// apiresourceschema controller, the other list includes all possible schema names.
@@ -164,7 +151,7 @@ func (r *Reconciler) reconcile(ctx context.Context, apiExport *kcpdevv1alpha1.AP
 	// downstream components.
 	readySchemaNames := sets.New[string]()
 	allSchemaNames := sets.New[string]()
-	for _, pubResource := range filteredPubResources {
+	for _, pubResource := range pubResources.Items {
 		schemaName, err := r.getSchemaName(ctx, &pubResource)
 		if err != nil {
 			return fmt.Errorf("failed to determine schema name for PublishedResource %s: %w", pubResource.Name, err)
@@ -198,7 +185,7 @@ func (r *Reconciler) reconcile(ctx context.Context, apiExport *kcpdevv1alpha1.AP
 	// Now we can finally assemble the list of required permission claims.
 	claimedResources := sets.New[permissionClaim]()
 
-	for _, pubResource := range filteredPubResources {
+	for _, pubResource := range pubResources.Items {
 		// to evaluate the namespace filter, the agent needs to fetch the namespace
 		if filter := pubResource.Spec.Filter; filter != nil && filter.Namespace != nil {
 			claimedResources.Insert(permissionClaim{

internal/controller/syncmanager/controller.go

@@ -28,7 +28,6 @@ import (
 	"github.com/kcp-dev/api-syncagent/internal/controllerutil"
 	"github.com/kcp-dev/api-syncagent/internal/controllerutil/predicate"
 	"github.com/kcp-dev/api-syncagent/internal/discovery"
-	"github.com/kcp-dev/api-syncagent/internal/validation"
 	syncagentv1alpha1 "github.com/kcp-dev/api-syncagent/sdk/apis/syncagent/v1alpha1"
 
 	kcpapisv1alpha1 "github.com/kcp-dev/kcp/sdk/apis/apis/v1alpha1"
@@ -416,12 +415,6 @@ func isSyncEnabled(pr *syncagentv1alpha1.PublishedResource) bool {
 func (r *Reconciler) ensureSyncControllers(ctx context.Context, log *zap.SugaredLogger, publishedResources []syncagentv1alpha1.PublishedResource) error {
 	requiredWorkers := sets.New[string]()
 	for _, pr := range publishedResources {
-		// TODO: Turn this into a webhook or CEL expressions.
-		if err := validation.ValidatePublishedResource(&pr); err != nil {
-			r.log.With("pr", pr.Name, "error", err).Warn("Ignoring invalid PublishedResource.")
-			continue
-		}
-
 		if isSyncEnabled(&pr) {
 			requiredWorkers.Insert(getPublishedResourceKey(&pr))
 		}

internal/validation/published_resource.go

@@ -195,6 +195,15 @@ const (
 	RelatedResourceOriginKcp     RelatedResourceOrigin = "kcp"
 )
 
+// RelatedResourceSpec describes a single related resource, which might point to
+// any number of actual Kubernetes objects.
+//
+// (in the following rule, group is optional becaue core/v1 is represented by group="")
+// +kubebuilder:validation:XValidation:rule="has(self.kind) != (has(self.version) || has(self.resource))",message="must specify either kind (deprecated) or group, version, resource"
+// +kubebuilder:validation:XValidation:rule="has(self.resource) == has(self.version)",message="resource and version must be configured together or not at all"
+// +kubebuilder:validation:XValidation:rule="!has(self.group) || (has(self.resource) && has(self.version))",message="configuring a group also requires a version and resource"
+// group is included here because when an identityHash is used, core/v1 cannot possible be targetted
+// +kubebuilder:validation:XValidation:rule="!has(self.identityHash) || (has(self.group) && has(self.version) && has(self.resource))",message="identity hashes can only be used with GVRs"
 type RelatedResourceSpec struct {
 	// Identifier is a unique name for this related resource. The name must be unique within one
 	// PublishedResource and is the key by which consumers (end users) can identify and consume the
@@ -220,6 +229,8 @@ type RelatedResourceSpec struct {
 	//
 	// Deprecated: Use "Resource" instead. This field is limited to "ConfigMap" and "Secret" and will
 	// be removed in the future. Kind and Resource cannot be specified at the same time.
+	//
+	// +kubebuilder:validation:Enum=ConfigMap;Secret
 	Kind string `json:"kind,omitempty"`
 
 	// IdentityHash is the identity hash of a kcp APIExport, in case the given Kind is