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.

Author: erdii

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.

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)
+				}
 			})
 		}
 	}

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)
+}