use typed variables for name/paths, ensure the path is handed through the callstack

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

Author: xrstf

internal/controller/sync/controller.go

@@ -21,8 +21,6 @@ import (
 	"fmt"
 	"time"
 
-	kcpcore "github.com/kcp-dev/kcp/sdk/apis/core"
-	kcpdevcorev1alpha1 "github.com/kcp-dev/kcp/sdk/apis/core/v1alpha1"
 	"github.com/kcp-dev/logicalcluster/v3"
 	"go.uber.org/zap"
 
@@ -32,6 +30,9 @@ import (
 	"github.com/kcp-dev/api-syncagent/internal/sync"
 	syncagentv1alpha1 "github.com/kcp-dev/api-syncagent/sdk/apis/syncagent/v1alpha1"
 
+	kcpcore "github.com/kcp-dev/kcp/sdk/apis/core"
+	kcpdevcorev1alpha1 "github.com/kcp-dev/kcp/sdk/apis/core/v1alpha1"
+
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/utils/ptr"

internal/controller/syncmanager/lifecycle/cluster.go

@@ -24,10 +24,11 @@ import (
 	"regexp"
 	"strings"
 
-	kcpdevcorev1alpha1 "github.com/kcp-dev/kcp/sdk/apis/core/v1alpha1"
 	"github.com/kcp-dev/logicalcluster/v3"
 	"go.uber.org/zap"
 
+	kcpdevcorev1alpha1 "github.com/kcp-dev/kcp/sdk/apis/core/v1alpha1"
+
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/client-go/rest"

internal/sync/context.go

@@ -20,6 +20,7 @@ import (
 	"context"
 
 	"github.com/kcp-dev/logicalcluster/v3"
+
 	"sigs.k8s.io/controller-runtime/pkg/kontext"
 )
 

internal/sync/meta.go

@@ -19,6 +19,7 @@ package sync
 import (
 	"context"
 
+	"github.com/kcp-dev/logicalcluster/v3"
 	"go.uber.org/zap"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -65,7 +66,7 @@ func ensureFinalizer(ctx context.Context, log *zap.SugaredLogger, client ctrlrun
 	finalizers.Insert(deletionFinalizer)
 	obj.SetFinalizers(sets.List(finalizers))
 
-	log.Debugw("Adding finalizer…", "on", newObjectKey(obj, ""), "finalizer", finalizer)
+	log.Debugw("Adding finalizer…", "on", newObjectKey(obj, "", logicalcluster.None), "finalizer", finalizer)
 	if err := client.Patch(ctx, obj, ctrlruntimeclient.MergeFrom(original)); err != nil {
 		return false, err
 	}
@@ -84,7 +85,7 @@ func removeFinalizer(ctx context.Context, log *zap.SugaredLogger, client ctrlrun
 	finalizers.Delete(deletionFinalizer)
 	obj.SetFinalizers(sets.List(finalizers))
 
-	log.Debugw("Removing finalizer…", "on", newObjectKey(obj, ""), "finalizer", finalizer)
+	log.Debugw("Removing finalizer…", "on", newObjectKey(obj, "", logicalcluster.None), "finalizer", finalizer)
 	if err := client.Patch(ctx, obj, ctrlruntimeclient.MergeFrom(original)); err != nil {
 		return false, err
 	}
@@ -93,16 +94,18 @@ func removeFinalizer(ctx context.Context, log *zap.SugaredLogger, client ctrlrun
 }
 
 type objectKey struct {
-	Cluster   string
-	Namespace string
-	Name      string
+	ClusterName logicalcluster.Name
+	ClusterPath logicalcluster.Path
+	Namespace   string
+	Name        string
 }
 
-func newObjectKey(obj metav1.Object, clusterName string) objectKey {
+func newObjectKey(obj metav1.Object, clusterName logicalcluster.Name, clusterPath logicalcluster.Path) objectKey {
 	return objectKey{
-		Cluster:   clusterName,
-		Namespace: obj.GetNamespace(),
-		Name:      obj.GetName(),
+		ClusterName: clusterName,
+		ClusterPath: clusterPath,
+		Namespace:   obj.GetNamespace(),
+		Name:        obj.GetName(),
 	}
 }
 
@@ -111,8 +114,8 @@ func (k objectKey) String() string {
 	if k.Namespace != "" {
 		result = k.Namespace + "/" + result
 	}
-	if k.Cluster != "" {
-		result = k.Cluster + "|" + result
+	if k.ClusterName != "" {
+		result = string(k.ClusterName) + "|" + result
 	}
 
 	return result
@@ -123,17 +126,23 @@ func (k objectKey) Key() string {
 	if k.Namespace != "" {
 		result = k.Namespace + "_" + result
 	}
-	if k.Cluster != "" {
-		result = k.Cluster + "_" + result
+	if k.ClusterName != "" {
+		result = string(k.ClusterName) + "_" + result
 	}
 
 	return result
 }
 
 func (k objectKey) Labels() labels.Set {
-	return labels.Set{
-		remoteObjectClusterLabel:   k.Cluster,
+	s := labels.Set{
+		remoteObjectClusterLabel:   string(k.ClusterName),
 		remoteObjectNamespaceLabel: k.Namespace,
 		remoteObjectNameLabel:      k.Name,
 	}
+
+	if !k.ClusterPath.Empty() {
+		s[remoteObjectClusterPathLabel] = k.ClusterPath.String()
+	}
+
+	return s
 }

internal/sync/meta_test.go

@@ -19,6 +19,8 @@ package sync
 import (
 	"testing"
 
+	"github.com/kcp-dev/logicalcluster/v3"
+
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
@@ -34,7 +36,8 @@ func createNewObject(name, namespace string) metav1.Object {
 func TestObjectKey(t *testing.T) {
 	testcases := []struct {
 		object      metav1.Object
-		clusterName string
+		clusterName logicalcluster.Name
+		clusterPath logicalcluster.Path
 		expected    string
 	}{
 		{
@@ -57,11 +60,17 @@ func TestObjectKey(t *testing.T) {
 			clusterName: "abc123",
 			expected:    "abc123|namespace/test",
 		},
+		{
+			object:      createNewObject("test", "namespace"),
+			clusterName: "abc123",
+			clusterPath: logicalcluster.NewPath("this:should:not:appear:in:the:key"),
+			expected:    "abc123|namespace/test",
+		},
 	}
 
 	for _, testcase := range testcases {
 		t.Run("", func(t *testing.T) {
-			key := newObjectKey(testcase.object, testcase.clusterName)
+			key := newObjectKey(testcase.object, testcase.clusterName, testcase.clusterPath)
 
 			if stringified := key.String(); stringified != testcase.expected {
 				t.Fatalf("Expected %q but got %q.", testcase.expected, stringified)

internal/sync/object_syncer.go

@@ -22,6 +22,7 @@ import (
 	"slices"
 
 	jsonpatch "github.com/evanphx/json-patch/v5"
+	"github.com/kcp-dev/logicalcluster/v3"
 	"go.uber.org/zap"
 	"k8c.io/reconciler/pkg/equality"
 
@@ -54,7 +55,8 @@ type objectSyncer struct {
 
 type syncSide struct {
 	ctx         context.Context
-	clusterName string
+	clusterName logicalcluster.Name
+	clusterPath logicalcluster.Path
 	client      ctrlruntimeclient.Client
 	object      *unstructured.Unstructured
 }
@@ -104,7 +106,7 @@ func (s *objectSyncer) Sync(log *zap.SugaredLogger, source, dest syncSide) (requ
 	// do not try to update a destination object that is in deletion
 	// (this should only happen if a service admin manually deletes something on the service cluster)
 	if dest.object.GetDeletionTimestamp() != nil {
-		log.Debugw("Destination object is in deletion, skipping any further synchronization", "dest-object", newObjectKey(dest.object, dest.clusterName))
+		log.Debugw("Destination object is in deletion, skipping any further synchronization", "dest-object", newObjectKey(dest.object, dest.clusterName, logicalcluster.None))
 		return false, nil
 	}
 
@@ -173,7 +175,7 @@ func (s *objectSyncer) syncObjectSpec(log *zap.SugaredLogger, source, dest syncS
 	sourceObjCopy := source.object.DeepCopy()
 	stripMetadata(sourceObjCopy)
 
-	log = log.With("dest-object", newObjectKey(dest.object, dest.clusterName))
+	log = log.With("dest-object", newObjectKey(dest.object, dest.clusterName, logicalcluster.None))
 
 	// calculate the patch to go from the last known state to the current source object's state
 	if lastKnownSourceState != nil {
@@ -271,11 +273,11 @@ func (s *objectSyncer) ensureDestinationObject(log *zap.SugaredLogger, source, d
 	stripMetadata(destObj)
 
 	// remember the connection between the source and destination object
-	sourceObjKey := newObjectKey(source.object, source.clusterName)
+	sourceObjKey := newObjectKey(source.object, source.clusterName, source.clusterPath)
 	ensureLabels(destObj, sourceObjKey.Labels())
 
 	// finally, we can create the destination object
-	objectLog := log.With("dest-object", newObjectKey(destObj, dest.clusterName))
+	objectLog := log.With("dest-object", newObjectKey(destObj, dest.clusterName, logicalcluster.None))
 	objectLog.Debugw("Creating destination object…")
 
 	if err := dest.client.Create(dest.ctx, destObj); err != nil {
@@ -356,7 +358,7 @@ func (s *objectSyncer) handleDeletion(log *zap.SugaredLogger, source, dest syncS
 	// if the destination object still exists, delete it and wait for it to be cleaned up
 	if dest.object != nil {
 		if dest.object.GetDeletionTimestamp() == nil {
-			log.Debugw("Deleting destination object…", "dest-object", newObjectKey(dest.object, dest.clusterName))
+			log.Debugw("Deleting destination object…", "dest-object", newObjectKey(dest.object, dest.clusterName, logicalcluster.None))
 			if err := dest.client.Delete(dest.ctx, dest.object); err != nil {
 				return false, fmt.Errorf("failed to delete destination object: %w", err)
 			}

internal/sync/state_store.go

@@ -23,6 +23,8 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/kcp-dev/logicalcluster/v3"
+
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/labels"
@@ -32,7 +34,7 @@ import (
 
 type ObjectStateStore interface {
 	Get(source syncSide) (*unstructured.Unstructured, error)
-	Put(obj *unstructured.Unstructured, clusterName string, subresources []string) error
+	Put(obj *unstructured.Unstructured, clusterName logicalcluster.Name, subresources []string) error
 }
 
 // objectStateStore is capable of creating/updating a target Kubernetes object
@@ -71,7 +73,7 @@ func (op *objectStateStore) Get(source syncSide) (*unstructured.Unstructured, er
 	return lastKnown, nil
 }
 
-func (op *objectStateStore) Put(obj *unstructured.Unstructured, clusterName string, subresources []string) error {
+func (op *objectStateStore) Put(obj *unstructured.Unstructured, clusterName logicalcluster.Name, subresources []string) error {
 	encoded, err := op.snapshotObject(obj, subresources)
 	if err != nil {
 		return err
@@ -99,8 +101,8 @@ func (op *objectStateStore) snapshotObject(obj *unstructured.Unstructured, subre
 }
 
 type backend interface {
-	Get(obj *unstructured.Unstructured, clusterName string) ([]byte, error)
-	Put(obj *unstructured.Unstructured, clusterName string, data []byte) error
+	Get(obj *unstructured.Unstructured, clusterName logicalcluster.Name) ([]byte, error)
+	Put(obj *unstructured.Unstructured, clusterName logicalcluster.Name, data []byte) error
 }
 
 type kubernetesBackend struct {
@@ -131,7 +133,7 @@ func hashObject(obj *unstructured.Unstructured) string {
 func newKubernetesBackend(namespace string, primaryObject, stateCluster syncSide) *kubernetesBackend {
 	keyHash := hashObject(primaryObject.object)
 
-	secretLabels := newObjectKey(primaryObject.object, primaryObject.clusterName).Labels()
+	secretLabels := newObjectKey(primaryObject.object, primaryObject.clusterName, primaryObject.clusterPath).Labels()
 	secretLabels[objectStateLabelName] = objectStateLabelValue
 
 	return &kubernetesBackend{
@@ -145,13 +147,13 @@ func newKubernetesBackend(namespace string, primaryObject, stateCluster syncSide
 	}
 }
 
-func (b *kubernetesBackend) Get(obj *unstructured.Unstructured, clusterName string) ([]byte, error) {
+func (b *kubernetesBackend) Get(obj *unstructured.Unstructured, clusterName logicalcluster.Name) ([]byte, error) {
 	secret := corev1.Secret{}
 	if err := b.stateCluster.client.Get(b.stateCluster.ctx, b.secretName, &secret); ctrlruntimeclient.IgnoreNotFound(err) != nil {
 		return nil, err
 	}
 
-	sourceKey := newObjectKey(obj, clusterName).Key()
+	sourceKey := newObjectKey(obj, clusterName, logicalcluster.None).Key()
 	data, ok := secret.Data[sourceKey]
 	if !ok {
 		return nil, nil
@@ -160,7 +162,7 @@ func (b *kubernetesBackend) Get(obj *unstructured.Unstructured, clusterName stri
 	return data, nil
 }
 
-func (b *kubernetesBackend) Put(obj *unstructured.Unstructured, clusterName string, data []byte) error {
+func (b *kubernetesBackend) Put(obj *unstructured.Unstructured, clusterName logicalcluster.Name, data []byte) error {
 	secret := corev1.Secret{}
 	if err := b.stateCluster.client.Get(b.stateCluster.ctx, b.secretName, &secret); ctrlruntimeclient.IgnoreNotFound(err) != nil {
 		return err
@@ -170,7 +172,7 @@ func (b *kubernetesBackend) Put(obj *unstructured.Unstructured, clusterName stri
 		secret.Data = map[string][]byte{}
 	}
 
-	sourceKey := newObjectKey(obj, clusterName).Key()
+	sourceKey := newObjectKey(obj, clusterName, logicalcluster.None).Key()
 	secret.Data[sourceKey] = data
 	secret.Labels = b.labels
 

internal/sync/syncer.go

@@ -19,7 +19,6 @@ package sync
 import (
 	"fmt"
 
-	"github.com/kcp-dev/logicalcluster/v3"
 	"go.uber.org/zap"
 
 	"github.com/kcp-dev/api-syncagent/internal/mutation"
@@ -113,7 +112,7 @@ func NewResourceSyncer(
 // case, the caller should re-fetch the remote object and call Process() again (most likely in the
 // next reconciliation). Only when (false, nil) is returned is the entire process finished.
 func (s *ResourceSyncer) Process(ctx Context, remoteObj *unstructured.Unstructured) (requeue bool, err error) {
-	log := s.log.With("source-object", newObjectKey(remoteObj, ctx.clusterName))
+	log := s.log.With("source-object", newObjectKey(remoteObj, ctx.clusterName, ctx.clusterPath))
 
 	// find the local equivalent object in the local service cluster
 	localObj, err := s.findLocalObject(ctx, remoteObj)
@@ -129,6 +128,7 @@ func (s *ResourceSyncer) Process(ctx Context, remoteObj *unstructured.Unstructur
 	sourceSide := syncSide{
 		ctx:         ctx.remote,
 		clusterName: ctx.clusterName,
+		clusterPath: ctx.clusterPath,
 		client:      s.remoteClient,
 		object:      remoteObj,
 	}
@@ -182,7 +182,7 @@ func (s *ResourceSyncer) Process(ctx Context, remoteObj *unstructured.Unstructur
 }
 
 func (s *ResourceSyncer) findLocalObject(ctx Context, remoteObj *unstructured.Unstructured) (*unstructured.Unstructured, error) {
-	localSelector := labels.SelectorFromSet(newObjectKey(remoteObj, ctx.clusterName).Labels())
+	localSelector := labels.SelectorFromSet(newObjectKey(remoteObj, ctx.clusterName, ctx.clusterPath).Labels())
 
 	localObjects := &unstructured.UnstructuredList{}
 	localObjects.SetAPIVersion(s.destDummy.GetAPIVersion())
@@ -215,7 +215,7 @@ func (s *ResourceSyncer) createLocalObjectCreator(ctx Context) objectCreatorFunc
 		destScope := syncagentv1alpha1.ResourceScope(s.localCRD.Spec.Scope)
 
 		// map namespace/name
-		mappedName := projection.GenerateLocalObjectName(s.pubRes, remoteObj, logicalcluster.Name(ctx.clusterName))
+		mappedName := projection.GenerateLocalObjectName(s.pubRes, remoteObj, ctx.clusterName)
 
 		switch destScope {
 		case syncagentv1alpha1.ClusterScoped:

internal/sync/syncer_test.go

@@ -825,7 +825,7 @@ func TestSyncerProcessingSingleResourceWithoutStatus(t *testing.T) {
 				if backend == nil {
 					backend = newKubernetesBackend(stateNamespace, primaryObject, stateCluster)
 					if testcase.existingState != "" {
-						if err := backend.Put(testcase.remoteObject, clusterName.String(), []byte(testcase.existingState)); err != nil {
+						if err := backend.Put(testcase.remoteObject, clusterName, []byte(testcase.existingState)); err != nil {
 							t.Fatalf("Failed to prime state store: %v", err)
 						}
 					}
@@ -894,7 +894,7 @@ func TestSyncerProcessingSingleResourceWithoutStatus(t *testing.T) {
 						t.Fatal("Cannot check object state, state store was never instantiated.")
 					}
 
-					finalState, err := backend.Get(testcase.expectedRemoteObject, clusterName.String())
+					finalState, err := backend.Get(testcase.expectedRemoteObject, clusterName)
 					if err != nil {
 						t.Fatalf("Failed to get final state: %v", err)
 					} else if !bytes.Equal(finalState, []byte(testcase.expectedState)) {
@@ -1122,7 +1122,7 @@ func TestSyncerProcessingSingleResourceWithStatus(t *testing.T) {
 				if backend == nil {
 					backend = newKubernetesBackend(stateNamespace, primaryObject, stateCluster)
 					if testcase.existingState != "" {
-						if err := backend.Put(testcase.remoteObject, clusterName.String(), []byte(testcase.existingState)); err != nil {
+						if err := backend.Put(testcase.remoteObject, clusterName, []byte(testcase.existingState)); err != nil {
 							t.Fatalf("Failed to prime state store: %v", err)
 						}
 					}
@@ -1191,7 +1191,7 @@ func TestSyncerProcessingSingleResourceWithStatus(t *testing.T) {
 						t.Fatal("Cannot check object state, state store was never instantiated.")
 					}
 
-					finalState, err := backend.Get(testcase.expectedRemoteObject, clusterName.String())
+					finalState, err := backend.Get(testcase.expectedRemoteObject, clusterName)
 					if err != nil {
 						t.Fatalf("Failed to get final state: %v", err)
 					} else if !bytes.Equal(finalState, []byte(testcase.expectedState)) {