resolve Christoph's comments

fix linter issue

fix verify ci

Author: iakmc

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

@@ -785,28 +785,63 @@ spec:
                           resource type and uses the configured rule to enqueue the correct primary object.
                           Without this field, changes to origin:kcp related resources do not trigger reconciliation.
                         properties:
-                          byLabel:
-                            additionalProperties:
-                              type: string
-                            description: |-
-                              ByLabel configures the watch handler to list primary objects matching a label selector
-                              derived from the changed object. Each map key is a label key on the primary object;
-                              each value is a Go template expression evaluated with the changed object available as
-                              .watchObject (with fields .name, .namespace, .labels).
-                            type: object
                           byOwner:
                             description: |-
                               ByOwner configures the watch handler to inspect the OwnerReferences of the changed
                               object. When an OwnerReference with the given Kind is found, the referenced owner
                               is enqueued as the primary object.
+                            type: object
+                          bySelector:
+                            description: |-
+                              BySelector configures the watch handler to list primary objects matching the given label
+                              selector. When a related object changes, all primary objects matching this selector
+                              are enqueued for reconciliation.
                             properties:
-                              kind:
-                                description: Kind is the Kind to look for in the OwnerReferences of the changed related object.
-                                type: string
-                            required:
-                              - kind
+                              matchExpressions:
+                                description: matchExpressions is a list of label selector requirements. The requirements are ANDed.
+                                items:
+                                  description: |-
+                                    A label selector requirement is a selector that contains values, a key, and an operator that
+                                    relates the key and values.
+                                  properties:
+                                    key:
+                                      description: key is the label key that the selector applies to.
+                                      type: string
+                                    operator:
+                                      description: |-
+                                        operator represents a key's relationship to a set of values.
+                                        Valid operators are In, NotIn, Exists and DoesNotExist.
+                                      type: string
+                                    values:
+                                      description: |-
+                                        values is an array of string values. If the operator is In or NotIn,
+                                        the values array must be non-empty. If the operator is Exists or DoesNotExist,
+                                        the values array must be empty. This array is replaced during a strategic
+                                        merge patch.
+                                      items:
+                                        type: string
+                                      type: array
+                                      x-kubernetes-list-type: atomic
+                                  required:
+                                    - key
+                                    - operator
+                                  type: object
+                                type: array
+                                x-kubernetes-list-type: atomic
+                              matchLabels:
+                                additionalProperties:
+                                  type: string
+                                description: |-
+                                  matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels
+                                  map is equivalent to an element of matchExpressions, whose key field is "key", the
+                                  operator is "In", and the values array contains only "value". The requirements are ANDed.
+                                type: object
                             type: object
+                            x-kubernetes-map-type: atomic
                         type: object
+                        x-kubernetes-validations:
+                          - message: exactly one of byOwner or bySelector must be set
+                            rule: has(self.byOwner) != has(self.bySelector)
                     required:
                       - identifier
                       - object

hack/tools.checksums

@@ -6,6 +6,7 @@ etcd|GOARCH=arm64;GOOS=linux|cc8c645e5a8df0f35f2a5c51d9b9383037eef0cf0167c52e648
 gimps|GOARCH=amd64;GOOS=linux|b597efc7e2c72097a44c001b41a06ccca97610963e1f1aec74c3d99c0e0b6c11
 gimps|GOARCH=arm64;GOOS=linux|2588daec997b4f4b3a8d8875f780fd6faf3c39c933519e7899e19a686476c8e4
 golangci-lint|GOARCH=amd64;GOOS=linux|8a01a08dad47a14824d7d0f14af07c7144105fc079386c9c31fbe85f08f91643
+golangci-lint|GOARCH=arm64;GOOS=darwin|5fd0b6a09353eb0101d3ae81d5e3cf4707b77210c66fb92ae152d7280d959419
 golangci-lint|GOARCH=arm64;GOOS=linux|2ed9cf2ad070dabc7947ba34cdc5142910be830306f063719898bc8fb44a7074
 kube-apiserver|GOARCH=amd64;GOOS=linux|ca822082ec39e54a25836a4011ddb66e482e317a7a4f1a1f73882bbd2cf5a2a1
 kube-apiserver|GOARCH=arm64;GOOS=linux|6ade6c2646e2c01fde1095407452afc2b65e89d6da16da29ee39f6223ccaf63b

internal/controller/sync/controller.go

@@ -36,6 +36,7 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/labels"
@@ -164,11 +165,31 @@ func Create(
 		return nil, fmt.Errorf("failed to setup local-side watch: %w", err)
 	}
 
-	// Watch origin:kcp related resources so that changes to them trigger reconciliation
-	// of the owning primary object. Only related resources with a Watch config are covered.
-	watchedGVKs := sets.New[schema.GroupVersionKind]()
+	if err := setupRelatedResourceWatches(c, localManager, remoteManager, pubRes, localDummy, remoteDummy, log); err != nil {
+		return nil, err
+	}
+
+	log.Info("Done setting up unmanaged controller.")
+
+	return c, nil
+}
+
+// setupRelatedResourceWatches sets up watches for all related resources that have a Watch
+// config, on their respective origin side, so that changes trigger primary reconciliation.
+func setupRelatedResourceWatches(
+	c mccontroller.Controller,
+	localManager manager.Manager,
+	remoteManager mcmanager.Manager,
+	pubRes *syncagentv1alpha1.PublishedResource,
+	localDummy, remoteDummy *unstructured.Unstructured,
+	log *zap.SugaredLogger,
+) error {
+	// Deduplication is per-origin to allow the same GVK on both sides.
+	watchedKcpGVKs := sets.New[schema.GroupVersionKind]()
+	watchedServiceGVKs := sets.New[schema.GroupVersionKind]()
+
 	for _, relRes := range pubRes.Spec.Related {
-		if relRes.Origin != syncagentv1alpha1.RelatedResourceOriginKcp || relRes.Watch == nil {
+		if relRes.Watch == nil {
 			continue
 		}
 
@@ -178,62 +199,155 @@ func Create(
 			Resource: relRes.Resource,
 		}
 
-		// Use the local REST mapper to determine the Kind.
-		gvk, err := localManager.GetRESTMapper().KindFor(gvr)
-		if err != nil {
-			log.Warnw("Failed to determine Kind for origin:kcp related resource, skipping watch", "gvr", gvr, "error", err)
-			continue
+		// Use the REST mapper of the origin side: related resources may have projected GVKs
+		// that differ between kcp and the service cluster, so we must resolve using the
+		// mapper that actually knows about the GVR on that side.
+		var originRESTMapper meta.RESTMapper
+		if relRes.Origin == syncagentv1alpha1.RelatedResourceOriginKcp {
+			originRESTMapper = remoteManager.GetLocalManager().GetRESTMapper()
+		} else {
+			originRESTMapper = localManager.GetRESTMapper()
 		}
 
-		// Deduplicate: only set up one watch per GVK.
-		if watchedGVKs.Has(gvk) {
-			continue
+		gvk, err := originRESTMapper.KindFor(gvr)
+		if err != nil {
+			return fmt.Errorf("failed to determine Kind for related resource %v (origin: %s): %w", gvr, relRes.Origin, err)
 		}
-		watchedGVKs.Insert(gvk)
 
 		relatedDummy := &unstructured.Unstructured{}
 		relatedDummy.SetGroupVersionKind(gvk)
 
-		var enqueueForRelated mchandler.TypedEventHandlerFunc[*unstructured.Unstructured, mcreconcile.Request]
+		if relRes.Origin == syncagentv1alpha1.RelatedResourceOriginKcp {
+			if watchedKcpGVKs.Has(gvk) {
+				continue
+			}
+			watchedKcpGVKs.Insert(gvk)
 
-		switch {
-		case relRes.Watch.ByOwner != nil:
-			ownerKind := relRes.Watch.ByOwner.Kind
-			enqueueForRelated = func(clusterName string, _ cluster.Cluster) handler.TypedEventHandler[*unstructured.Unstructured, mcreconcile.Request] {
-				return &byOwnerEventHandler{
-					clusterName: clusterName,
-					ownerKind:   ownerKind,
-				}
+			enqueueForRelated, err := buildKcpRelatedHandler(relRes.Watch, gvk, remoteDummy, log)
+			if err != nil {
+				return err
 			}
 
-		case relRes.Watch.ByLabel != nil:
-			labelTemplates := relRes.Watch.ByLabel
-			primaryDummy := remoteDummy.DeepCopy()
-			enqueueForRelated = func(clusterName string, cl cluster.Cluster) handler.TypedEventHandler[*unstructured.Unstructured, mcreconcile.Request] {
-				return &byLabelEventHandler{
-					clusterName:    clusterName,
-					client:         cl.GetClient(),
-					primaryDummy:   primaryDummy,
-					labelTemplates: labelTemplates,
-					log:            log,
-				}
+			if err := c.MultiClusterWatch(mcsource.TypedKind(relatedDummy, enqueueForRelated)); err != nil {
+				return fmt.Errorf("failed to setup watch for kcp-origin related resource %v: %w", gvk, err)
+			}
+		} else {
+			if watchedServiceGVKs.Has(gvk) {
+				continue
 			}
+			watchedServiceGVKs.Insert(gvk)
 
-		default:
-			log.Warnw("origin:kcp related resource has Watch set but neither byOwner nor byLabel configured, skipping", "gvk", gvk)
-			continue
-		}
+			enqueueForRelated, err := buildServiceRelatedHandler(relRes.Watch, gvk, localDummy, localManager, log)
+			if err != nil {
+				return err
+			}
 
-		if err := c.MultiClusterWatch(mcsource.TypedKind(relatedDummy, enqueueForRelated)); err != nil {
-			return nil, fmt.Errorf("failed to setup watch for origin:kcp related resource %v: %w", gvk, err)
+			if err := c.Watch(source.TypedKind(localManager.GetCache(), relatedDummy, enqueueForRelated)); err != nil {
+				return fmt.Errorf("failed to setup watch for service-origin related resource %v: %w", gvk, err)
+			}
 		}
 
-		log.Infow("Set up watch for origin:kcp related resource", "gvk", gvk)
+		log.Infow("Set up watch for related resource", "gvk", gvk, "origin", relRes.Origin)
 	}
 
-	log.Info("Done setting up unmanaged controller.")
+	return nil
+}
 
-	return c, nil
+// buildKcpRelatedHandler constructs the per-cluster event handler for a kcp-origin related resource.
+func buildKcpRelatedHandler(
+	watch *syncagentv1alpha1.RelatedResourceWatch,
+	gvk schema.GroupVersionKind,
+	remoteDummy *unstructured.Unstructured,
+	log *zap.SugaredLogger,
+) (mchandler.TypedEventHandlerFunc[*unstructured.Unstructured, mcreconcile.Request], error) {
+	switch {
+	case watch.ByOwner != nil:
+		ownerGVK := remoteDummy.GroupVersionKind()
+		return func(clusterName string, _ cluster.Cluster) handler.TypedEventHandler[*unstructured.Unstructured, mcreconcile.Request] {
+			return &byOwnerEventHandler{
+				clusterName: clusterName,
+				ownerGVK:    ownerGVK,
+			}
+		}, nil
+
+	case watch.BySelector != nil:
+		labelSelector := watch.BySelector
+		primaryDummy := remoteDummy.DeepCopy()
+		return func(clusterName string, cl cluster.Cluster) handler.TypedEventHandler[*unstructured.Unstructured, mcreconcile.Request] {
+			return &bySelectorEventHandler{
+				clusterName:   clusterName,
+				client:        cl.GetClient(),
+				primaryDummy:  primaryDummy,
+				labelSelector: labelSelector,
+				log:           log,
+			}
+		}, nil
+
+	default:
+		return nil, fmt.Errorf("related resource %v (origin: kcp) has Watch set but neither byOwner nor bySelector configured", gvk)
+	}
+}
+
+// buildServiceRelatedHandler constructs the event handler for a service-cluster-origin related resource.
+// It maps the changed related resource back to the remote (kcp) primary via sync metadata on the local primary.
+func buildServiceRelatedHandler(
+	watch *syncagentv1alpha1.RelatedResourceWatch,
+	gvk schema.GroupVersionKind,
+	localDummy *unstructured.Unstructured,
+	localManager manager.Manager,
+	log *zap.SugaredLogger,
+) (handler.TypedEventHandler[*unstructured.Unstructured, mcreconcile.Request], error) {
+	localClient := localManager.GetClient()
+
+	switch {
+	case watch.ByOwner != nil:
+		ownerGVK := localDummy.GroupVersionKind()
+		primaryDummy := localDummy.DeepCopy()
+		return handler.TypedEnqueueRequestsFromMapFunc(func(ctx context.Context, obj *unstructured.Unstructured) []mcreconcile.Request {
+			for _, ref := range obj.GetOwnerReferences() {
+				refGV, err := schema.ParseGroupVersion(ref.APIVersion)
+				if err != nil || refGV.Group != ownerGVK.Group || refGV.Version != ownerGVK.Version || ref.Kind != ownerGVK.Kind {
+					continue
+				}
+				localPrimary := primaryDummy.DeepCopy()
+				if err := localClient.Get(ctx, types.NamespacedName{Namespace: obj.GetNamespace(), Name: ref.Name}, localPrimary); err != nil {
+					log.Warnw("Failed to fetch local primary for byOwner watch", "owner", ref.Name, "error", err)
+					return nil
+				}
+				if req := sync.RemoteNameForLocalObject(localPrimary); req != nil {
+					return []mcreconcile.Request{*req}
+				}
+				return nil
+			}
+			return nil
+		}), nil
+
+	case watch.BySelector != nil:
+		selector, err := metav1.LabelSelectorAsSelector(watch.BySelector)
+		if err != nil {
+			return nil, fmt.Errorf("failed to convert bySelector for service-origin related resource %v: %w", gvk, err)
+		}
+		primaryDummy := localDummy.DeepCopy()
+		return handler.TypedEnqueueRequestsFromMapFunc(func(ctx context.Context, _ *unstructured.Unstructured) []mcreconcile.Request {
+			primaryList := &unstructured.UnstructuredList{}
+			primaryList.SetAPIVersion(primaryDummy.GetAPIVersion())
+			primaryList.SetKind(primaryDummy.GetKind() + "List")
+			if err := localClient.List(ctx, primaryList, &ctrlruntimeclient.ListOptions{LabelSelector: selector}); err != nil {
+				log.Warnw("Failed to list local primary objects for bySelector watch", "selector", selector.String(), "error", err)
+				return nil
+			}
+			var reqs []mcreconcile.Request
+			for i := range primaryList.Items {
+				if req := sync.RemoteNameForLocalObject(&primaryList.Items[i]); req != nil {
+					reqs = append(reqs, *req)
+				}
+			}
+			return reqs
+		}), nil
+
+	default:
+		return nil, fmt.Errorf("related resource %v (origin: service) has Watch set but neither byOwner nor bySelector configured", gvk)
+	}
 }
 
 func (r *Reconciler) Reconcile(ctx context.Context, request mcreconcile.Request) (reconcile.Result, error) {