From f404ab758642ee76e26f006801b421f2ccf06b26 Mon Sep 17 00:00:00 2001 From: Igor Fominykh Date: Fri, 20 Feb 2026 14:29:58 +0100 Subject: [PATCH 1/3] test: add e2e test for related resource cleanup on primary deletion Adds a regression test that verifies syncagent.kcp.io/cleanup finalizers are properly removed from kcp-origin related resources when the primary object is deleted. Without the fix, the finalizer remains indefinitely, blocking namespace/workspace deletion. Signed-off-by: Igor Fominykh --- test/e2e/sync/related_test.go | 186 ++++++++++++++++++++++++++++++++++ 1 file changed, 186 insertions(+) diff --git a/test/e2e/sync/related_test.go b/test/e2e/sync/related_test.go index ea724e3..4c0ef5a 100644 --- a/test/e2e/sync/related_test.go +++ b/test/e2e/sync/related_test.go @@ -1551,6 +1551,192 @@ func TestSyncNonStandardRelatedResourcesMultipleAPIExports(t *testing.T) { } } +// TestDeletePrimaryWithRelatedKcpResource verifies that when a primary object +// is deleted, related resources with origin:kcp are properly cleaned up: +// the local copy is deleted and the finalizer on the kcp-side source object +// is removed. This is a regression test for +// https://github.com/kcp-dev/api-syncagent/issues/116. +func TestDeletePrimaryWithRelatedKcpResource(t *testing.T) { + const apiExportName = "kcp.example.com" + + ctrlruntime.SetLogger(logr.Discard()) + + ctx := t.Context() + + // setup a test environment in kcp + orgKubconfig := utils.CreateOrganization(t, ctx, "delete-primary-related-kcp", apiExportName) + + // start a service cluster + envtestKubeconfig, envtestClient, _ := utils.RunEnvtest(t, []string{ + "test/crds/crontab.yaml", + }) + + // publish Crontabs with a related Secret (origin: kcp) + t.Log("Publishing CRDs…") + prCrontabs := &syncagentv1alpha1.PublishedResource{ + ObjectMeta: metav1.ObjectMeta{ + Name: "publish-crontabs", + }, + Spec: syncagentv1alpha1.PublishedResourceSpec{ + Resource: syncagentv1alpha1.SourceResourceDescriptor{ + APIGroup: "example.com", + Version: "v1", + Kind: "CronTab", + }, + Naming: &syncagentv1alpha1.ResourceNaming{ + Name: "{{ .Object.metadata.name }}", + Namespace: "synced-{{ .Object.metadata.namespace }}", + }, + Projection: &syncagentv1alpha1.ResourceProjection{ + Group: "kcp.example.com", + }, + Related: []syncagentv1alpha1.RelatedResourceSpec{ + { + Identifier: "credentials", + Origin: syncagentv1alpha1.RelatedResourceOriginKcp, + Kind: "Secret", + Object: syncagentv1alpha1.RelatedResourceObject{ + RelatedResourceObjectSpec: syncagentv1alpha1.RelatedResourceObjectSpec{ + Template: &syncagentv1alpha1.TemplateExpression{ + Template: "my-credentials", + }, + }, + }, + }, + }, + }, + } + + if err := envtestClient.Create(ctx, prCrontabs); err != nil { + t.Fatalf("Failed to create PublishedResource: %v", err) + } + + // start the agent + utils.RunAgent(ctx, t, "bob", orgKubconfig, envtestKubeconfig, apiExportName, "") + + // wait until the API is available + kcpClusterClient := utils.GetKcpAdminClusterClient(t) + + teamClusterPath := logicalcluster.NewPath("root").Join("delete-primary-related-kcp").Join("team-1") + teamClient := kcpClusterClient.Cluster(teamClusterPath) + + utils.WaitForBoundAPI(t, ctx, teamClient, schema.GroupVersionResource{ + Group: apiExportName, + Version: "v1", + Resource: "crontabs", + }) + + // Step 1: Create a CronTab in kcp + t.Log("Creating CronTab in kcp…") + + crontab := &unstructured.Unstructured{} + crontab.SetAPIVersion("kcp.example.com/v1") + crontab.SetKind("CronTab") + crontab.SetName("my-crontab") + crontab.SetNamespace("default") + if err := unstructured.SetNestedField(crontab.Object, "* * *", "spec", "cronSpec"); err != nil { + t.Fatalf("Failed to set cronSpec: %v", err) + } + + if err := teamClient.Create(ctx, crontab); err != nil { + t.Fatalf("Failed to create CronTab in kcp: %v", err) + } + + // Step 2: Create the related Secret in kcp (origin: kcp means the source is on the kcp side) + t.Log("Creating credential Secret in kcp…") + + kcpSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-credentials", + Namespace: "default", + }, + Data: map[string][]byte{ + "password": []byte("hunter2"), + }, + Type: corev1.SecretTypeOpaque, + } + + if err := teamClient.Create(ctx, kcpSecret); err != nil { + t.Fatalf("Failed to create Secret in kcp: %v", err) + } + + // Step 3: Wait for the Secret to be synced down to the service cluster + t.Log("Waiting for Secret to be synced to service cluster…") + + localSecret := &corev1.Secret{} + err := wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, 30*time.Second, false, func(ctx context.Context) (done bool, err error) { + return envtestClient.Get(ctx, types.NamespacedName{Name: "my-credentials", Namespace: "synced-default"}, localSecret) == nil, nil + }) + if err != nil { + t.Fatalf("Secret was not synced to service cluster: %v", err) + } + + // Step 4: Verify the kcp Secret has a finalizer (the agent should have added one) + t.Log("Verifying finalizer on kcp Secret…") + + err = wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, 30*time.Second, false, func(ctx context.Context) (done bool, err error) { + secret := &corev1.Secret{} + if err := teamClient.Get(ctx, types.NamespacedName{Name: "my-credentials", Namespace: "default"}, secret); err != nil { + return false, nil + } + for _, f := range secret.Finalizers { + if f == "syncagent.kcp.io/cleanup" { + return true, nil + } + } + return false, nil + }) + if err != nil { + t.Fatalf("kcp Secret never received the cleanup finalizer: %v", err) + } + + // Step 5: Delete the primary CronTab + t.Log("Deleting CronTab in kcp…") + + if err := teamClient.Delete(ctx, crontab); err != nil { + t.Fatalf("Failed to delete CronTab: %v", err) + } + + // Step 6: Verify the finalizer on the kcp Secret is removed + t.Log("Waiting for finalizer to be removed from kcp Secret…") + + err = wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, 60*time.Second, false, func(ctx context.Context) (done bool, err error) { + secret := &corev1.Secret{} + if err := teamClient.Get(ctx, types.NamespacedName{Name: "my-credentials", Namespace: "default"}, secret); err != nil { + // Secret might have been deleted too, which is also fine + if apierrors.IsNotFound(err) { + return true, nil + } + return false, nil + } + for _, f := range secret.Finalizers { + if f == "syncagent.kcp.io/cleanup" { + return false, nil + } + } + return true, nil + }) + if err != nil { + t.Fatalf("Finalizer was not removed from kcp Secret after primary deletion: %v", err) + } + + // Step 7: Verify the local copy on the service cluster is also deleted + t.Log("Verifying local Secret copy is deleted…") + + err = wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, 30*time.Second, false, func(ctx context.Context) (done bool, err error) { + err = envtestClient.Get(ctx, types.NamespacedName{Name: "my-credentials", Namespace: "synced-default"}, &corev1.Secret{}) + if apierrors.IsNotFound(err) { + return true, nil + } + return false, nil + }) + if err != nil { + t.Fatalf("Local Secret copy was not deleted after primary deletion: %v", err) + } + + t.Log("Primary deletion correctly cleaned up related resources") +} + func toUnstructured(t *testing.T, obj ctrlruntimeclient.Object) *unstructured.Unstructured { data, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj) if err != nil { From 5493247bf550a8973b7fcb6ed8f15093d6839529 Mon Sep 17 00:00:00 2001 From: Igor Fominykh Date: Fri, 20 Feb 2026 14:30:05 +0100 Subject: [PATCH 2/3] fix: clean up related resources when primary object is deleted (#116) When a primary object is deleted, related resources with origin kcp retain their syncagent.kcp.io/cleanup finalizer indefinitely because processRelatedResources() is never called during deletion. Process related resources before the primary syncer handles deletion, passing forceDelete=true for kcp-origin related resources so their finalizers are properly removed. Signed-off-by: Igor Fominykh --- internal/sync/object_syncer.go | 17 +++++++---------- internal/sync/syncer.go | 23 +++++++++++++++++++++-- internal/sync/syncer_related.go | 9 ++++++--- 3 files changed, 34 insertions(+), 15 deletions(-) diff --git a/internal/sync/object_syncer.go b/internal/sync/object_syncer.go index a514e96..54aa942 100644 --- a/internal/sync/object_syncer.go +++ b/internal/sync/object_syncer.go @@ -63,6 +63,10 @@ type objectSyncer struct { // receive events. Since these objects might be created during the sync, // they cannot be specified here directly. eventObjSide syncSideType + // forceDelete triggers the deletion flow even when the source object is not + // being deleted; used to clean up related resources when the primary object + // is being deleted. + forceDelete bool } type syncSideType int @@ -93,8 +97,8 @@ func (s *objectSyncer) recordEvent(ctx context.Context, source, dest syncSide, e } func (s *objectSyncer) Sync(ctx context.Context, log *zap.SugaredLogger, source, dest syncSide) (requeue bool, err error) { - // handle deletion: if source object is in deletion, delete the destination object (the clone) - if source.object.GetDeletionTimestamp() != nil { + // handle deletion: if source object is in deletion (or forced), delete the destination object (the clone) + if source.object.GetDeletionTimestamp() != nil || s.forceDelete { return s.handleDeletion(ctx, log, source, dest) } @@ -458,16 +462,9 @@ func (s *objectSyncer) handleDeletion(ctx context.Context, log *zap.SugaredLogge // if we just removed the finalizer, we can requeue the source object if updated { s.recordEvent(ctx, source, dest, corev1.EventTypeNormal, "ObjectDeleted", "Object deletion has been completed, finalizer has been removed.") - return true, nil } - // For now we do not delete related resources; since after this step the destination object is - // gone already, the remaining syncer logic would fail if it attempts to sync relate objects. - // For the MVP it's fine to just leave related resources around, but in the future this behaviour - // might be configurable per PublishedResource, in which case this `return true` here would need - // to go away and the cleanup in general would need to be rethought a bit (maybe owner refs would - // be a good idea?). - return true, nil + return updated, nil } func (s *objectSyncer) removeSubresources(obj *unstructured.Unstructured) *unstructured.Unstructured { diff --git a/internal/sync/syncer.go b/internal/sync/syncer.go index 7aad7f2..986a144 100644 --- a/internal/sync/syncer.go +++ b/internal/sync/syncer.go @@ -195,7 +195,19 @@ func (s *ResourceSyncer) Process(ctx context.Context, remoteObj *unstructured.Un // (i.e. on the service cluster), so that the original and copy are linked // together and can be found. metadataOnDestination: true, - eventObjSide: syncSideSource, + eventObjSide: syncSideSource, + } + + // When the primary object is being deleted, clean up related resources FIRST, + // while the local object still exists (needed for reference resolution). + if remoteObj.GetDeletionTimestamp() != nil && destSide.object != nil { + relRequeue, relErr := s.processRelatedResources(ctx, log, stateStore, sourceSide, destSide, true) + if relErr != nil { + return false, fmt.Errorf("failed to clean up related resources during primary deletion: %w", relErr) + } + if relRequeue { + return true, nil + } } requeue, err = syncer.Sync(ctx, log, sourceSide, destSide) @@ -215,7 +227,14 @@ func (s *ResourceSyncer) Process(ctx context.Context, remoteObj *unstructured.Un // it modifies the state of the world, otherwise the objects in // source/dest.object might be ouf date. - return s.processRelatedResources(ctx, log, stateStore, sourceSide, destSide) + // Guard: related resource resolution requires both sync sides to exist. + // destSide.object can be nil if the primary was in deletion and the local + // copy was already removed. In that case there is nothing left to sync. + if destSide.object == nil { + return false, nil + } + + return s.processRelatedResources(ctx, log, stateStore, sourceSide, destSide, false) } func (s *ResourceSyncer) findLocalObject(ctx context.Context, objectKey objectKey) (*unstructured.Unstructured, error) { diff --git a/internal/sync/syncer_related.go b/internal/sync/syncer_related.go index c65f0a1..35aceb6 100644 --- a/internal/sync/syncer_related.go +++ b/internal/sync/syncer_related.go @@ -40,9 +40,9 @@ import ( ctrlruntimeclient "sigs.k8s.io/controller-runtime/pkg/client" ) -func (s *ResourceSyncer) processRelatedResources(ctx context.Context, log *zap.SugaredLogger, stateStore ObjectStateStore, remote, local syncSide) (requeue bool, err error) { +func (s *ResourceSyncer) processRelatedResources(ctx context.Context, log *zap.SugaredLogger, stateStore ObjectStateStore, remote, local syncSide, primaryDeleting bool) (requeue bool, err error) { for _, relatedResource := range s.pubRes.Spec.Related { - requeue, err := s.processRelatedResource(ctx, log.With("identifier", relatedResource.Identifier), stateStore, remote, local, relatedResource) + requeue, err := s.processRelatedResource(ctx, log.With("identifier", relatedResource.Identifier), stateStore, remote, local, relatedResource, primaryDeleting) if err != nil { return false, fmt.Errorf("failed to process related resource %s: %w", relatedResource.Identifier, err) } @@ -62,7 +62,7 @@ type relatedObjectAnnotation struct { Kind string `json:"kind"` } -func (s *ResourceSyncer) processRelatedResource(ctx context.Context, log *zap.SugaredLogger, stateStore ObjectStateStore, remote, local syncSide, relRes syncagentv1alpha1.RelatedResourceSpec) (requeue bool, err error) { +func (s *ResourceSyncer) processRelatedResource(ctx context.Context, log *zap.SugaredLogger, stateStore ObjectStateStore, remote, local syncSide, relRes syncagentv1alpha1.RelatedResourceSpec, primaryDeleting bool) (requeue bool, err error) { // decide what direction to sync (local->remote vs. remote->local) var ( origin syncSide @@ -163,6 +163,9 @@ func (s *ResourceSyncer) processRelatedResource(ctx context.Context, log *zap.Su metadataOnDestination: false, // events are always created on the kcp side eventObjSide: eventObjSide, + // force deletion of related resources when the primary object is being deleted + // (only for origin:kcp resources that have blockSourceDeletion) + forceDelete: primaryDeleting && relRes.Origin == syncagentv1alpha1.RelatedResourceOriginKcp, } req, err := syncer.Sync(ctx, log, sourceSide, destSide) From b0766d26bae726a1ada9b213802f6cc9de07465a Mon Sep 17 00:00:00 2001 From: Igor Fominykh Date: Fri, 20 Feb 2026 15:27:08 +0100 Subject: [PATCH 3/3] fix: align struct field formatting in syncer.go Fix whitespace alignment flagged by gimps import formatter. Signed-off-by: Igor Fominykh --- internal/sync/syncer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/sync/syncer.go b/internal/sync/syncer.go index 986a144..7108fca 100644 --- a/internal/sync/syncer.go +++ b/internal/sync/syncer.go @@ -195,7 +195,7 @@ func (s *ResourceSyncer) Process(ctx context.Context, remoteObj *unstructured.Un // (i.e. on the service cluster), so that the original and copy are linked // together and can be found. metadataOnDestination: true, - eventObjSide: syncSideSource, + eventObjSide: syncSideSource, } // When the primary object is being deleted, clean up related resources FIRST,