From 90da9d29e25cd77258d5d92a0a5fed97a8fd3bf6 Mon Sep 17 00:00:00 2001 From: Josh Gwosdz Date: Fri, 5 Jun 2026 13:23:13 +0200 Subject: [PATCH 1/3] fix: use ownership instead of revision to guard teardown deletion When an Owner is provided, check whether we are the controller via detectOwner rather than comparing revision annotations. Owner refs are the authoritative signal for ownership and correctly handle orphaning deletions where the revision annotation may have been updated by a different reconciliation pass. Callers without an Owner fall back to the existing revision comparison. --- machinery/objects.go | 27 +++++----- machinery/objects_test.go | 59 +++++++++++++++++++++ test/objects_test.go | 109 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 182 insertions(+), 13 deletions(-) diff --git a/machinery/objects.go b/machinery/objects.go index 9630c80..a1a0b17 100644 --- a/machinery/objects.go +++ b/machinery/objects.go @@ -151,19 +151,10 @@ func (e *ObjectEngine) Teardown( return false, fmt.Errorf("getting object before deletion: %w", err) } - // Check revision matches. - actualRevision, err := e.getObjectRevision(actualObject) - if err != nil { - return false, fmt.Errorf("getting object revision: %w", err) - } - - // Object is not owned by this revision - if actualRevision != revision { - if options.Owner == nil { - // No Owner to remove from the object, return. - return true, nil - } - + if options.Owner != nil { + // Check ownership instead of revision to determine if we should delete. + // If we're not the controller, only remove our owner ref and leave the object in place. + // A possible reason for this could be an orphaning deletion. ctrlSit, _ := e.detectOwner(options.Owner, options.OwnerStrategy, actualObject, nil) if ctrlSit != ctrlSituationIsController { // Remove us from owners list: @@ -173,6 +164,16 @@ func (e *ObjectEngine) Teardown( // TODO should we check if the patch differs from actualObject before firing the request? return true, e.writer.Patch(ctx, patch, client.MergeFrom(actualObject)) } + } else { + // No Owner to check against. Fall back to revision comparison. + actualRevision, err := e.getObjectRevision(actualObject) + if err != nil { + return false, fmt.Errorf("getting object revision: %w", err) + } + + if actualRevision != revision { + return true, nil + } } // Actually delete the object. diff --git a/machinery/objects_test.go b/machinery/objects_test.go index c1e32c4..557cd58 100644 --- a/machinery/objects_test.go +++ b/machinery/objects_test.go @@ -1083,6 +1083,7 @@ func TestObjectEngine_Teardown(t *testing.T) { modes []ownerMode // nil = both default modes expectedGone bool expectedError string + afterAssert func(t *testing.T, writer *testutil.CtrlClient) }{ { name: "Orphan", @@ -1383,6 +1384,60 @@ func TestObjectEngine_Teardown(t *testing.T) { }, expectedGone: true, }, + { + name: "Not controller, revision matches, removes owner ref", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("1"), + withOwnerRef("v1", "ConfigMap", "owner", "12345-678", false), + withOwnerRef("v1", "Node", "node1", "xxxx", true)), + modes: []ownerMode{withNativeOwnerMode}, + mockSetup: func( + cache *cacheMock, writer *testutil.CtrlClient, + actualObject *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKeyFromObject(actualObject), + mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + obj := args.Get(2).(*unstructured.Unstructured) + *obj = *actualObject + }). + Return(nil) + writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + }, + expectedGone: true, + afterAssert: func(t *testing.T, writer *testutil.CtrlClient) { + t.Helper() + writer.AssertNotCalled(t, "Delete") + }, + }, + { + name: "Is controller, revision mismatch, still deletes", + revision: 1, + desiredObject: buildObj("testi", "test"), + actualObject: buildObj("testi", "test", withRevision("4"), withManaged), + modes: []ownerMode{withNativeOwnerMode}, + mockSetup: func( + cache *cacheMock, writer *testutil.CtrlClient, + actualObject *unstructured.Unstructured, + ) { + cache. + On("Get", mock.Anything, + client.ObjectKeyFromObject(actualObject), + mock.Anything, mock.Anything). + Run(func(args mock.Arguments) { + obj := args.Get(2).(*unstructured.Unstructured) + *obj = *actualObject + }). + Return(nil) + writer. + On("Delete", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return(nil) + }, + expectedGone: false, + }, } for _, tc := range sharedTests { @@ -1435,6 +1490,10 @@ func TestObjectEngine_Teardown(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), tc.expectedError) } + + if tc.afterAssert != nil { + tc.afterAssert(t, writer) + } }) } } diff --git a/test/objects_test.go b/test/objects_test.go index aec4796..5d61605 100644 --- a/test/objects_test.go +++ b/test/objects_test.go @@ -526,3 +526,112 @@ func TestObjectEngine_StaleManagedFieldMigration(t *testing.T) { } } } + +// TestObjectEngine_TeardownNotControllerRevisionMatch verifies that teardown +// with an owner that is no longer the controller only removes the owner +// reference and does NOT delete the object, even when the revision matches. +func TestObjectEngine_TeardownNotControllerRevisionMatch(t *testing.T) { + os := ownerhandling.NewNative(Scheme) + comp := machinery.NewComparator(DiscoveryClient, Scheme, fieldOwner) + oe := machinery.NewObjectEngine( + Scheme, Client, Client, comp, fieldOwner, systemPrefix, "", nil, + ) + + ctx := t.Context() + + ownerA := &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]any{ + "name": "oe-teardown-owner-a", + "namespace": "default", + }, + }, + } + ownerB := &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]any{ + "name": "oe-teardown-owner-b", + "namespace": "default", + }, + }, + } + + require.NoError(t, Client.Create(ctx, ownerA, client.FieldOwner(fieldOwner))) + cleanupOnSuccess(t, ownerA) + require.NoError(t, Client.Create(ctx, ownerB, client.FieldOwner(fieldOwner))) + cleanupOnSuccess(t, ownerB) + + configMap := &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]any{ + "name": "oe-teardown-not-ctrl", + "namespace": "default", + }, + "data": map[string]any{ + "key": "value", + }, + }, + } + + // Step 1: Create the object owned by ownerA at revision 1. + res, err := oe.Reconcile(ctx, 1, configMap, types.WithOwner(ownerA, os)) + require.NoError(t, err) + assert.Equal(t, machinery.ActionCreated, res.Action()) + + // Step 2: ownerB adopts the object (becomes controller, ownerA becomes non-controller). + res, err = oe.Reconcile(ctx, 1, configMap, + types.WithOwner(ownerB, os), + types.WithCollisionProtection(types.CollisionProtectionNone), + ) + require.NoError(t, err) + assert.Equal(t, machinery.ActionUpdated, res.Action()) + + // Verify ownerB is now the controller and ownerA's ref is still present. + actual := configMap.DeepCopy() + require.NoError(t, Client.Get(ctx, client.ObjectKeyFromObject(configMap), actual)) + assert.True(t, os.IsController(ownerB, actual), "ownerB should be controller") + assert.False(t, os.IsController(ownerA, actual), "ownerA should not be controller") + + // Step 3: Teardown with ownerA at revision 1 — revision matches but + // ownerA is not the controller. The object must NOT be deleted. + gone, err := oe.Teardown(ctx, 1, configMap, types.WithOwner(ownerA, os)) + require.NoError(t, err) + assert.True(t, gone) + + // Verify the object still exists on the cluster. + stillExists := configMap.DeepCopy() + err = Client.Get(ctx, client.ObjectKeyFromObject(configMap), stillExists) + require.NoError(t, err, "object must still exist after teardown by non-controller owner") + + // Verify ownerA's ref has been removed but ownerB's ref remains. + hasOwnerA := false + hasOwnerB := false + + for _, ref := range stillExists.GetOwnerReferences() { + if ref.UID == ownerA.GetUID() { + hasOwnerA = true + } + + if ref.UID == ownerB.GetUID() { + hasOwnerB = true + } + } + + assert.False(t, hasOwnerA, "ownerA ref should have been removed") + assert.True(t, hasOwnerB, "ownerB ref should still be present") + + // Cleanup: teardown with ownerB (the actual controller). + gone, err = oe.Teardown(ctx, 1, configMap, types.WithOwner(ownerB, os)) + require.NoError(t, err) + assert.False(t, gone) + + gone, err = oe.Teardown(ctx, 1, configMap, types.WithOwner(ownerB, os)) + require.NoError(t, err) + assert.True(t, gone) +} From 6dd5ae6878dbb0080234a555e5f8d9b0e0914152 Mon Sep 17 00:00:00 2001 From: Josh Gwosdz Date: Tue, 23 Jun 2026 16:49:17 +0200 Subject: [PATCH 2/3] f: address test feedback --- machinery/objects_test.go | 74 ++++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 40 deletions(-) diff --git a/machinery/objects_test.go b/machinery/objects_test.go index 557cd58..f73617e 100644 --- a/machinery/objects_test.go +++ b/machinery/objects_test.go @@ -1079,11 +1079,11 @@ func TestObjectEngine_Teardown(t *testing.T) { cache *cacheMock, writer *testutil.CtrlClient, actualObject *unstructured.Unstructured, + mode ownerMode, ) modes []ownerMode // nil = both default modes expectedGone bool expectedError string - afterAssert func(t *testing.T, writer *testutil.CtrlClient) }{ { name: "Orphan", @@ -1094,19 +1094,10 @@ func TestObjectEngine_Teardown(t *testing.T) { return []types.ObjectTeardownOption{types.WithOrphan()} }, mockSetup: func( - cache *cacheMock, writer *testutil.CtrlClient, - actualObject *unstructured.Unstructured, + _ *cacheMock, writer *testutil.CtrlClient, + _ *unstructured.Unstructured, _ ownerMode, ) { writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) - cache. - On("Get", mock.Anything, - client.ObjectKey{Name: "testi", Namespace: "test"}, - mock.Anything, mock.Anything). - Run(func(args mock.Arguments) { - obj := args.Get(2).(*unstructured.Unstructured) - *obj = *actualObject - }). - Return(nil) }, expectedGone: true, }, @@ -1116,7 +1107,7 @@ func TestObjectEngine_Teardown(t *testing.T) { desiredObject: buildObj("testi", "test"), mockSetup: func( cache *cacheMock, _ *testutil.CtrlClient, - _ *unstructured.Unstructured, + _ *unstructured.Unstructured, _ ownerMode, ) { cache. On("Get", mock.Anything, @@ -1132,7 +1123,7 @@ func TestObjectEngine_Teardown(t *testing.T) { desiredObject: buildObj("testi", "test"), mockSetup: func( cache *cacheMock, _ *testutil.CtrlClient, - _ *unstructured.Unstructured, + _ *unstructured.Unstructured, _ ownerMode, ) { cache. On("Get", mock.Anything, @@ -1152,7 +1143,7 @@ func TestObjectEngine_Teardown(t *testing.T) { desiredObject: buildObj("testi", "test"), mockSetup: func( cache *cacheMock, _ *testutil.CtrlClient, - _ *unstructured.Unstructured, + _ *unstructured.Unstructured, _ ownerMode, ) { cache. On("Get", mock.Anything, @@ -1169,7 +1160,7 @@ func TestObjectEngine_Teardown(t *testing.T) { actualObject: buildObj("testi", "test", withRevision("1"), withManaged), mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, - actualObject *unstructured.Unstructured, + actualObject *unstructured.Unstructured, mode ownerMode, ) { cache. On("Get", mock.Anything, @@ -1193,7 +1184,7 @@ func TestObjectEngine_Teardown(t *testing.T) { actualObject: buildObj("testi", "test", withRevision("1"), withManaged), mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, - actualObject *unstructured.Unstructured, + actualObject *unstructured.Unstructured, mode ownerMode, ) { cache. On("Get", mock.Anything, @@ -1217,7 +1208,7 @@ func TestObjectEngine_Teardown(t *testing.T) { actualObject: buildObj("testi", "test", withRevision("1"), withManaged), mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, - actualObject *unstructured.Unstructured, + actualObject *unstructured.Unstructured, mode ownerMode, ) { cache. On("Get", mock.Anything, @@ -1249,7 +1240,7 @@ func TestObjectEngine_Teardown(t *testing.T) { }, mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, - actualObject *unstructured.Unstructured, + actualObject *unstructured.Unstructured, mode ownerMode, ) { cache. On("Get", mock.Anything, @@ -1260,8 +1251,8 @@ func TestObjectEngine_Teardown(t *testing.T) { *obj = *actualObject }). Return(nil) - writer.On("Delete", mock.Anything, mock.Anything, mock.Anything). - Panic("Delete should not be called on the engine writer") + // writer.On("Delete", mock.Anything, mock.Anything, mock.Anything). + // Panic("Delete should not be called on the engine writer") }, expectedGone: false, }, @@ -1280,7 +1271,7 @@ func TestObjectEngine_Teardown(t *testing.T) { }, mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, - actualObject *unstructured.Unstructured, + actualObject *unstructured.Unstructured, mode ownerMode, ) { cache. On("Get", mock.Anything, @@ -1291,8 +1282,8 @@ func TestObjectEngine_Teardown(t *testing.T) { *obj = *actualObject }). Return(nil) - writer.On("Delete", mock.Anything, mock.Anything, mock.Anything). - Panic("Delete should not be called on the engine writer") + // writer.On("Delete", mock.Anything, mock.Anything, mock.Anything). + // Panic("Delete should not be called on the engine writer") }, expectedError: "deleting object: teardown delete failed", }, @@ -1303,7 +1294,7 @@ func TestObjectEngine_Teardown(t *testing.T) { actualObject: buildObj("testi", "test", withRevision("4")), mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, - actualObject *unstructured.Unstructured, + actualObject *unstructured.Unstructured, mode ownerMode, ) { cache. On("Get", mock.Anything, @@ -1314,8 +1305,11 @@ func TestObjectEngine_Teardown(t *testing.T) { *obj = *actualObject }). Return(nil) + // Patch may be called in with-owner mode to remove owner ref. - writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + if mode.teardownOpts() != nil { + writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + } }, expectedGone: true, }, @@ -1328,7 +1322,7 @@ func TestObjectEngine_Teardown(t *testing.T) { modes: []ownerMode{withNativeOwnerMode}, mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, - actualObject *unstructured.Unstructured, + actualObject *unstructured.Unstructured, mode ownerMode, ) { cache. On("Get", mock.Anything, @@ -1350,7 +1344,7 @@ func TestObjectEngine_Teardown(t *testing.T) { actualObject: buildObj("testi", "test", withRevision("2")), mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, - actualObject *unstructured.Unstructured, + actualObject *unstructured.Unstructured, mode ownerMode, ) { cache. On("Get", mock.Anything, @@ -1361,7 +1355,11 @@ func TestObjectEngine_Teardown(t *testing.T) { *obj = *actualObject }). Return(nil) - writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + + // Patch may be called in with-owner mode to remove owner ref. + if mode.teardownOpts() != nil { + writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) + } }, expectedGone: true, }, @@ -1378,7 +1376,7 @@ func TestObjectEngine_Teardown(t *testing.T) { })}, mockSetup: func( _ *cacheMock, writer *testutil.CtrlClient, - _ *unstructured.Unstructured, + _ *unstructured.Unstructured, _ ownerMode, ) { writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) }, @@ -1394,7 +1392,7 @@ func TestObjectEngine_Teardown(t *testing.T) { modes: []ownerMode{withNativeOwnerMode}, mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, - actualObject *unstructured.Unstructured, + actualObject *unstructured.Unstructured, mode ownerMode, ) { cache. On("Get", mock.Anything, @@ -1408,10 +1406,6 @@ func TestObjectEngine_Teardown(t *testing.T) { writer.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) }, expectedGone: true, - afterAssert: func(t *testing.T, writer *testutil.CtrlClient) { - t.Helper() - writer.AssertNotCalled(t, "Delete") - }, }, { name: "Is controller, revision mismatch, still deletes", @@ -1421,7 +1415,7 @@ func TestObjectEngine_Teardown(t *testing.T) { modes: []ownerMode{withNativeOwnerMode}, mockSetup: func( cache *cacheMock, writer *testutil.CtrlClient, - actualObject *unstructured.Unstructured, + actualObject *unstructured.Unstructured, mode ownerMode, ) { cache. On("Get", mock.Anything, @@ -1471,7 +1465,7 @@ func TestObjectEngine_Teardown(t *testing.T) { actualObject = tc.actualObject(&mode) } - tc.mockSetup(cache, writer, actualObject) + tc.mockSetup(cache, writer, actualObject, mode) opts := mode.teardownOpts() if tc.opts != nil { @@ -1491,9 +1485,9 @@ func TestObjectEngine_Teardown(t *testing.T) { assert.Contains(t, err.Error(), tc.expectedError) } - if tc.afterAssert != nil { - tc.afterAssert(t, writer) - } + cache.AssertExpectations(t) + writer.AssertExpectations(t) + divergeDetector.AssertExpectations(t) }) } } From b4f43496c2a0883c7ff22f8fce692685407566bd Mon Sep 17 00:00:00 2001 From: Josh Gwosdz Date: Tue, 23 Jun 2026 16:49:17 +0200 Subject: [PATCH 3/3] f: address test feedback --- machinery/objects_test.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/machinery/objects_test.go b/machinery/objects_test.go index f73617e..5108195 100644 --- a/machinery/objects_test.go +++ b/machinery/objects_test.go @@ -1251,8 +1251,6 @@ func TestObjectEngine_Teardown(t *testing.T) { *obj = *actualObject }). Return(nil) - // writer.On("Delete", mock.Anything, mock.Anything, mock.Anything). - // Panic("Delete should not be called on the engine writer") }, expectedGone: false, }, @@ -1282,8 +1280,6 @@ func TestObjectEngine_Teardown(t *testing.T) { *obj = *actualObject }). Return(nil) - // writer.On("Delete", mock.Anything, mock.Anything, mock.Anything). - // Panic("Delete should not be called on the engine writer") }, expectedError: "deleting object: teardown delete failed", },