Skip to content

fix: use ownership instead of revision to guard teardown deletion #542

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
erdii wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix: use ownership instead of revision to guard teardown deletion #542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
90da9d2
fix: use ownership instead of revision to guard teardown deletion
erdii Jun 5, 2026
6dd5ae6
f: address test feedback
erdii Jun 23, 2026
b4f4349
f: address test feedback
erdii Jun 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 14 additions & 13 deletions machinery/objects.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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))

@erdii erdii Jun 18, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think using a merge patch here is dangerous and could result in accidentally overwriting the object.

@erdii erdii Jun 19, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe sending an empty SSA intent with resourceVersion set to the observer version from actualObject instead is safe?

This would:

  • give us 100% certainty that we know what we're overwriting because of optimistic locking
  • remove all field values that we still own from the object including the owner reference

There's a pitfall though: Other actors in the system would have to deliberately own all fields that they want to keep when we tear down the object.

@erdii erdii Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually have to remove the owner ref at all?

Asumme:

  • we have owner-old and owner-new.
  • owner-new takes the controlling owner reference and demotes our owner reference to non controlling. (It actually doesn't matter if the owner ref is demoted or not.)
  • teardown of the owner-old is triggered

then:

  1. we skip deletion of the managed object during teardown
  2. we remove the owner-old
  3. the owner ref to owner-old starts dangling
  4. the kubernetes garbage collector removes the danling owner ref
  5. the managed object still exists.

@erdii erdii Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the case where ownership is transfered to a new owner is handled.

I think the only dangerous case is where our owner ref is demoted from controller: true to controller: false without adding a new owner.
If that's the case and we don't remove our owner ref then the garbage collector would still remove the managed object after the owner is deleted.

}
} 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.
Expand Down
109 changes: 79 additions & 30 deletions machinery/objects_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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,
},
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
},
Expand All @@ -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,
Expand All @@ -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",
},
Expand All @@ -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,
Expand All @@ -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,
},
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
},
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
})
}
}
Expand Down
Loading