Skip to content

Commit 5d9fc7a

Browse files
authored
Merge pull request #133 from ifdotpy/fix/116-related-resource-cleanup
fix: clean up related resources when primary object is deleted
2 parents 5d5ef78 + b0766d2 commit 5d9fc7a

4 files changed

Lines changed: 219 additions & 14 deletions

File tree

internal/sync/object_syncer.go

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,10 @@ type objectSyncer struct {
6363
// receive events. Since these objects might be created during the sync,
6464
// they cannot be specified here directly.
6565
eventObjSide syncSideType
66+
// forceDelete triggers the deletion flow even when the source object is not
67+
// being deleted; used to clean up related resources when the primary object
68+
// is being deleted.
69+
forceDelete bool
6670
}
6771

6872
type syncSideType int
@@ -93,8 +97,8 @@ func (s *objectSyncer) recordEvent(ctx context.Context, source, dest syncSide, e
9397
}
9498

9599
func (s *objectSyncer) Sync(ctx context.Context, log *zap.SugaredLogger, source, dest syncSide) (requeue bool, err error) {
96-
// handle deletion: if source object is in deletion, delete the destination object (the clone)
97-
if source.object.GetDeletionTimestamp() != nil {
100+
// handle deletion: if source object is in deletion (or forced), delete the destination object (the clone)
101+
if source.object.GetDeletionTimestamp() != nil || s.forceDelete {
98102
return s.handleDeletion(ctx, log, source, dest)
99103
}
100104

@@ -458,16 +462,9 @@ func (s *objectSyncer) handleDeletion(ctx context.Context, log *zap.SugaredLogge
458462
// if we just removed the finalizer, we can requeue the source object
459463
if updated {
460464
s.recordEvent(ctx, source, dest, corev1.EventTypeNormal, "ObjectDeleted", "Object deletion has been completed, finalizer has been removed.")
461-
return true, nil
462465
}
463466

464-
// For now we do not delete related resources; since after this step the destination object is
465-
// gone already, the remaining syncer logic would fail if it attempts to sync relate objects.
466-
// For the MVP it's fine to just leave related resources around, but in the future this behaviour
467-
// might be configurable per PublishedResource, in which case this `return true` here would need
468-
// to go away and the cleanup in general would need to be rethought a bit (maybe owner refs would
469-
// be a good idea?).
470-
return true, nil
467+
return updated, nil
471468
}
472469

473470
func (s *objectSyncer) removeSubresources(obj *unstructured.Unstructured) *unstructured.Unstructured {

internal/sync/syncer.go

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -198,6 +198,18 @@ func (s *ResourceSyncer) Process(ctx context.Context, remoteObj *unstructured.Un
198198
eventObjSide: syncSideSource,
199199
}
200200

201+
// When the primary object is being deleted, clean up related resources FIRST,
202+
// while the local object still exists (needed for reference resolution).
203+
if remoteObj.GetDeletionTimestamp() != nil && destSide.object != nil {
204+
relRequeue, relErr := s.processRelatedResources(ctx, log, stateStore, sourceSide, destSide, true)
205+
if relErr != nil {
206+
return false, fmt.Errorf("failed to clean up related resources during primary deletion: %w", relErr)
207+
}
208+
if relRequeue {
209+
return true, nil
210+
}
211+
}
212+
201213
requeue, err = syncer.Sync(ctx, log, sourceSide, destSide)
202214
if err != nil {
203215
return false, err
@@ -215,7 +227,14 @@ func (s *ResourceSyncer) Process(ctx context.Context, remoteObj *unstructured.Un
215227
// it modifies the state of the world, otherwise the objects in
216228
// source/dest.object might be ouf date.
217229

218-
return s.processRelatedResources(ctx, log, stateStore, sourceSide, destSide)
230+
// Guard: related resource resolution requires both sync sides to exist.
231+
// destSide.object can be nil if the primary was in deletion and the local
232+
// copy was already removed. In that case there is nothing left to sync.
233+
if destSide.object == nil {
234+
return false, nil
235+
}
236+
237+
return s.processRelatedResources(ctx, log, stateStore, sourceSide, destSide, false)
219238
}
220239

221240
func (s *ResourceSyncer) findLocalObject(ctx context.Context, objectKey objectKey) (*unstructured.Unstructured, error) {

internal/sync/syncer_related.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,9 @@ import (
4040
ctrlruntimeclient "sigs.k8s.io/controller-runtime/pkg/client"
4141
)
4242

43-
func (s *ResourceSyncer) processRelatedResources(ctx context.Context, log *zap.SugaredLogger, stateStore ObjectStateStore, remote, local syncSide) (requeue bool, err error) {
43+
func (s *ResourceSyncer) processRelatedResources(ctx context.Context, log *zap.SugaredLogger, stateStore ObjectStateStore, remote, local syncSide, primaryDeleting bool) (requeue bool, err error) {
4444
for _, relatedResource := range s.pubRes.Spec.Related {
45-
requeue, err := s.processRelatedResource(ctx, log.With("identifier", relatedResource.Identifier), stateStore, remote, local, relatedResource)
45+
requeue, err := s.processRelatedResource(ctx, log.With("identifier", relatedResource.Identifier), stateStore, remote, local, relatedResource, primaryDeleting)
4646
if err != nil {
4747
return false, fmt.Errorf("failed to process related resource %s: %w", relatedResource.Identifier, err)
4848
}
@@ -62,7 +62,7 @@ type relatedObjectAnnotation struct {
6262
Kind string `json:"kind"`
6363
}
6464

65-
func (s *ResourceSyncer) processRelatedResource(ctx context.Context, log *zap.SugaredLogger, stateStore ObjectStateStore, remote, local syncSide, relRes syncagentv1alpha1.RelatedResourceSpec) (requeue bool, err error) {
65+
func (s *ResourceSyncer) processRelatedResource(ctx context.Context, log *zap.SugaredLogger, stateStore ObjectStateStore, remote, local syncSide, relRes syncagentv1alpha1.RelatedResourceSpec, primaryDeleting bool) (requeue bool, err error) {
6666
// decide what direction to sync (local->remote vs. remote->local)
6767
var (
6868
origin syncSide
@@ -163,6 +163,9 @@ func (s *ResourceSyncer) processRelatedResource(ctx context.Context, log *zap.Su
163163
metadataOnDestination: false,
164164
// events are always created on the kcp side
165165
eventObjSide: eventObjSide,
166+
// force deletion of related resources when the primary object is being deleted
167+
// (only for origin:kcp resources that have blockSourceDeletion)
168+
forceDelete: primaryDeleting && relRes.Origin == syncagentv1alpha1.RelatedResourceOriginKcp,
166169
}
167170

168171
req, err := syncer.Sync(ctx, log, sourceSide, destSide)

test/e2e/sync/related_test.go

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1551,6 +1551,192 @@ func TestSyncNonStandardRelatedResourcesMultipleAPIExports(t *testing.T) {
15511551
}
15521552
}
15531553

1554+
// TestDeletePrimaryWithRelatedKcpResource verifies that when a primary object
1555+
// is deleted, related resources with origin:kcp are properly cleaned up:
1556+
// the local copy is deleted and the finalizer on the kcp-side source object
1557+
// is removed. This is a regression test for
1558+
// https://github.com/kcp-dev/api-syncagent/issues/116.
1559+
func TestDeletePrimaryWithRelatedKcpResource(t *testing.T) {
1560+
const apiExportName = "kcp.example.com"
1561+
1562+
ctrlruntime.SetLogger(logr.Discard())
1563+
1564+
ctx := t.Context()
1565+
1566+
// setup a test environment in kcp
1567+
orgKubconfig := utils.CreateOrganization(t, ctx, "delete-primary-related-kcp", apiExportName)
1568+
1569+
// start a service cluster
1570+
envtestKubeconfig, envtestClient, _ := utils.RunEnvtest(t, []string{
1571+
"test/crds/crontab.yaml",
1572+
})
1573+
1574+
// publish Crontabs with a related Secret (origin: kcp)
1575+
t.Log("Publishing CRDs…")
1576+
prCrontabs := &syncagentv1alpha1.PublishedResource{
1577+
ObjectMeta: metav1.ObjectMeta{
1578+
Name: "publish-crontabs",
1579+
},
1580+
Spec: syncagentv1alpha1.PublishedResourceSpec{
1581+
Resource: syncagentv1alpha1.SourceResourceDescriptor{
1582+
APIGroup: "example.com",
1583+
Version: "v1",
1584+
Kind: "CronTab",
1585+
},
1586+
Naming: &syncagentv1alpha1.ResourceNaming{
1587+
Name: "{{ .Object.metadata.name }}",
1588+
Namespace: "synced-{{ .Object.metadata.namespace }}",
1589+
},
1590+
Projection: &syncagentv1alpha1.ResourceProjection{
1591+
Group: "kcp.example.com",
1592+
},
1593+
Related: []syncagentv1alpha1.RelatedResourceSpec{
1594+
{
1595+
Identifier: "credentials",
1596+
Origin: syncagentv1alpha1.RelatedResourceOriginKcp,
1597+
Kind: "Secret",
1598+
Object: syncagentv1alpha1.RelatedResourceObject{
1599+
RelatedResourceObjectSpec: syncagentv1alpha1.RelatedResourceObjectSpec{
1600+
Template: &syncagentv1alpha1.TemplateExpression{
1601+
Template: "my-credentials",
1602+
},
1603+
},
1604+
},
1605+
},
1606+
},
1607+
},
1608+
}
1609+
1610+
if err := envtestClient.Create(ctx, prCrontabs); err != nil {
1611+
t.Fatalf("Failed to create PublishedResource: %v", err)
1612+
}
1613+
1614+
// start the agent
1615+
utils.RunAgent(ctx, t, "bob", orgKubconfig, envtestKubeconfig, apiExportName, "")
1616+
1617+
// wait until the API is available
1618+
kcpClusterClient := utils.GetKcpAdminClusterClient(t)
1619+
1620+
teamClusterPath := logicalcluster.NewPath("root").Join("delete-primary-related-kcp").Join("team-1")
1621+
teamClient := kcpClusterClient.Cluster(teamClusterPath)
1622+
1623+
utils.WaitForBoundAPI(t, ctx, teamClient, schema.GroupVersionResource{
1624+
Group: apiExportName,
1625+
Version: "v1",
1626+
Resource: "crontabs",
1627+
})
1628+
1629+
// Step 1: Create a CronTab in kcp
1630+
t.Log("Creating CronTab in kcp…")
1631+
1632+
crontab := &unstructured.Unstructured{}
1633+
crontab.SetAPIVersion("kcp.example.com/v1")
1634+
crontab.SetKind("CronTab")
1635+
crontab.SetName("my-crontab")
1636+
crontab.SetNamespace("default")
1637+
if err := unstructured.SetNestedField(crontab.Object, "* * *", "spec", "cronSpec"); err != nil {
1638+
t.Fatalf("Failed to set cronSpec: %v", err)
1639+
}
1640+
1641+
if err := teamClient.Create(ctx, crontab); err != nil {
1642+
t.Fatalf("Failed to create CronTab in kcp: %v", err)
1643+
}
1644+
1645+
// Step 2: Create the related Secret in kcp (origin: kcp means the source is on the kcp side)
1646+
t.Log("Creating credential Secret in kcp…")
1647+
1648+
kcpSecret := &corev1.Secret{
1649+
ObjectMeta: metav1.ObjectMeta{
1650+
Name: "my-credentials",
1651+
Namespace: "default",
1652+
},
1653+
Data: map[string][]byte{
1654+
"password": []byte("hunter2"),
1655+
},
1656+
Type: corev1.SecretTypeOpaque,
1657+
}
1658+
1659+
if err := teamClient.Create(ctx, kcpSecret); err != nil {
1660+
t.Fatalf("Failed to create Secret in kcp: %v", err)
1661+
}
1662+
1663+
// Step 3: Wait for the Secret to be synced down to the service cluster
1664+
t.Log("Waiting for Secret to be synced to service cluster…")
1665+
1666+
localSecret := &corev1.Secret{}
1667+
err := wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, 30*time.Second, false, func(ctx context.Context) (done bool, err error) {
1668+
return envtestClient.Get(ctx, types.NamespacedName{Name: "my-credentials", Namespace: "synced-default"}, localSecret) == nil, nil
1669+
})
1670+
if err != nil {
1671+
t.Fatalf("Secret was not synced to service cluster: %v", err)
1672+
}
1673+
1674+
// Step 4: Verify the kcp Secret has a finalizer (the agent should have added one)
1675+
t.Log("Verifying finalizer on kcp Secret…")
1676+
1677+
err = wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, 30*time.Second, false, func(ctx context.Context) (done bool, err error) {
1678+
secret := &corev1.Secret{}
1679+
if err := teamClient.Get(ctx, types.NamespacedName{Name: "my-credentials", Namespace: "default"}, secret); err != nil {
1680+
return false, nil
1681+
}
1682+
for _, f := range secret.Finalizers {
1683+
if f == "syncagent.kcp.io/cleanup" {
1684+
return true, nil
1685+
}
1686+
}
1687+
return false, nil
1688+
})
1689+
if err != nil {
1690+
t.Fatalf("kcp Secret never received the cleanup finalizer: %v", err)
1691+
}
1692+
1693+
// Step 5: Delete the primary CronTab
1694+
t.Log("Deleting CronTab in kcp…")
1695+
1696+
if err := teamClient.Delete(ctx, crontab); err != nil {
1697+
t.Fatalf("Failed to delete CronTab: %v", err)
1698+
}
1699+
1700+
// Step 6: Verify the finalizer on the kcp Secret is removed
1701+
t.Log("Waiting for finalizer to be removed from kcp Secret…")
1702+
1703+
err = wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, 60*time.Second, false, func(ctx context.Context) (done bool, err error) {
1704+
secret := &corev1.Secret{}
1705+
if err := teamClient.Get(ctx, types.NamespacedName{Name: "my-credentials", Namespace: "default"}, secret); err != nil {
1706+
// Secret might have been deleted too, which is also fine
1707+
if apierrors.IsNotFound(err) {
1708+
return true, nil
1709+
}
1710+
return false, nil
1711+
}
1712+
for _, f := range secret.Finalizers {
1713+
if f == "syncagent.kcp.io/cleanup" {
1714+
return false, nil
1715+
}
1716+
}
1717+
return true, nil
1718+
})
1719+
if err != nil {
1720+
t.Fatalf("Finalizer was not removed from kcp Secret after primary deletion: %v", err)
1721+
}
1722+
1723+
// Step 7: Verify the local copy on the service cluster is also deleted
1724+
t.Log("Verifying local Secret copy is deleted…")
1725+
1726+
err = wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, 30*time.Second, false, func(ctx context.Context) (done bool, err error) {
1727+
err = envtestClient.Get(ctx, types.NamespacedName{Name: "my-credentials", Namespace: "synced-default"}, &corev1.Secret{})
1728+
if apierrors.IsNotFound(err) {
1729+
return true, nil
1730+
}
1731+
return false, nil
1732+
})
1733+
if err != nil {
1734+
t.Fatalf("Local Secret copy was not deleted after primary deletion: %v", err)
1735+
}
1736+
1737+
t.Log("Primary deletion correctly cleaned up related resources")
1738+
}
1739+
15541740
func toUnstructured(t *testing.T, obj ctrlruntimeclient.Object) *unstructured.Unstructured {
15551741
data, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
15561742
if err != nil {

0 commit comments

Comments
 (0)