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..5108195 100644 --- a/machinery/objects_test.go +++ b/machinery/objects_test.go @@ -1079,6 +1079,7 @@ func TestObjectEngine_Teardown(t *testing.T) { cache *cacheMock, writer *testutil.CtrlClient, actualObject *unstructured.Unstructured, + mode ownerMode, ) modes []ownerMode // nil = both default modes expectedGone bool @@ -1093,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, }, @@ -1115,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, @@ -1131,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, @@ -1151,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, @@ -1168,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, @@ -1192,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, @@ -1216,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, @@ -1248,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, @@ -1259,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, }, @@ -1279,7 +1269,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, @@ -1290,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", }, @@ -1302,7 +1290,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, @@ -1313,8 +1301,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, }, @@ -1327,7 +1318,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, @@ -1349,7 +1340,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, @@ -1360,7 +1351,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, }, @@ -1377,12 +1372,62 @@ 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) }, 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, mode ownerMode, + ) { + 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, + }, + { + 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, mode ownerMode, + ) { + 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 { @@ -1416,7 +1461,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 { @@ -1435,6 +1480,10 @@ func TestObjectEngine_Teardown(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), tc.expectedError) } + + cache.AssertExpectations(t) + writer.AssertExpectations(t) + divergeDetector.AssertExpectations(t) }) } } 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) +}