From 687b35eab89917093de10a7f4fb5e8c6bff53506 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 13:12:35 +0100 Subject: [PATCH 1/5] feat(component): add DeleteOnlyIfOwned to keep objects the owner does not own A gated-off resource was deleted by name, so an object a user created under the same name was deleted too. With DeleteOnlyIfOwned, a deletion reads the live object and deletes it only when it carries an owner reference with the owner's UID. The delete carries UID and resourceVersion preconditions. An object without the reference, or one that changed after the read, is kept and logged, and the reconcile does not fail. The option covers Delete, DeleteWhen, GatedBy, a disabled component feature gate and DeleteOnSuspend. It is a build error with ReadOnly or Unowned. Closes #233 Co-Authored-By: Claude Opus 5.5 --- docs/component.md | 26 +++ docs/guidelines.md | 11 +- pkg/component/delete.go | 59 ++++-- pkg/component/delete_owned_test.go | 150 ++++++++++++++ pkg/component/delete_test.go | 192 ++++++++++++++++++ pkg/component/orphan.go | 18 +- pkg/component/resource_options.go | 47 +++++ pkg/component/resource_options_test.go | 29 +++ pkg/component/suspend.go | 4 +- plugin/skills/building-components/SKILL.md | 14 +- .../references/component.md | 26 +++ plugin/skills/structuring-operators/SKILL.md | 6 +- .../references/guidelines.md | 10 +- 13 files changed, 549 insertions(+), 43 deletions(-) create mode 100644 pkg/component/delete_owned_test.go diff --git a/docs/component.md b/docs/component.md index e6af17b4..81b836a3 100644 --- a/docs/component.md +++ b/docs/component.md @@ -66,6 +66,7 @@ be passed without a guard. | `component.BlockOnAbsence()` | Read-only only: a NotFound records a blocked status and short-circuits the remaining resources | | `component.IgnoreIfAbsent()` | Read-only only: a NotFound is silently ignored and last-known state is preserved | | `component.BlockOnForeignController()` | Managed only: records a blocked status that names the owner whose controller reference is on the live object, then skips the apply and the remaining resources | +| `component.DeleteOnlyIfOwned()` | Managed only: a deletion removes the live object only when it carries an owner reference to the owner; any other object is left in place | | `component.SuppressGraceInconsistencyWarning()` | Suppresses the grace/convergence inconsistency warning | A read-only resource is not owned by the component, so it is never deleted. `ReadOnly()` is mutually exclusive with @@ -105,6 +106,31 @@ A delete of an object that the read found safe carries the observed UID and reso owner claims the object between the read and the delete, the delete fails and the next reconcile reads the object again. The option requires a managed resource. A combination with `ReadOnly()` is a build error. +`DeleteOnlyIfOwned()` protects an object that the component did not create from a deletion. Without it, every deletion +path removes the object by name, whoever created it. Use the option when the resource name is one that a user can also +create by hand. For example, a component registers `-app` as a ServiceAccount behind `GatedBy()`. When the gate is +off and the user created their own `-app`, the option keeps the object of the user. + +When `Delete()`, `DeleteWhen()`, `GatedBy()`, a disabled component feature gate, or a suspension with +`DeleteOnSuspend()` asks for a deletion, the component first reads the live object, through `ReconcileContext.APIReader` +when it is set and `ReconcileContext.Client` otherwise. The component deletes the object only when the object has an +owner reference with the UID of the owner. This reference does not have to be the controller reference. The component +sets this reference on each object that it applies, with two exceptions: an `Unowned()` resource, and a cluster-scoped +resource of a namespace-scoped owner. The option never deletes an object of the second kind, so do not use it there. If +the object does not have the reference, the component does not write to the object and records no `ResourceDeleted` +event. It logs the skip, and the reconcile does not fail. The resource does not contribute to the component condition. +As with a deletion, a disabled component feature gate reports `Disabled`. While no deletion is asked for, the option has +no effect. + +The delete carries the observed UID and resourceVersion as preconditions. If the object changes, or a different object +takes the name, between the read and the delete, the API server rejects the delete with a conflict. The component then +keeps the object, logs the skip, and does not fail the reconcile. The next reconcile reads the object again. + +The option composes with `BlockOnForeignController()`. With both options, the component keeps an object that another +owner controls, even when the object also references this owner. A conflict on the delete then does not fail the +reconcile. `DeleteOnlyIfOwned()` requires a managed resource that carries the owner reference. A combination with +`ReadOnly()` or `Unowned()` is a build error. + Options compose. Gate a resource and exclude it from health aggregation in one call: ```go diff --git a/docs/guidelines.md b/docs/guidelines.md index 13b07842..bffa32c0 100644 --- a/docs/guidelines.md +++ b/docs/guidelines.md @@ -701,10 +701,13 @@ comp, _ := component.NewComponentBuilder(). Build() ``` -A disabled `GatedBy` gate deletes the resource on the next reconcile. For an optional resource the component does -**not** own (a read-only Secret reference behind an optional spec field), use `IncludeWhen`, which omits the resource -without ever deleting it. The [IncludeWhen vs. GatedBy](component.md#includewhen-vs-gatedby) section covers the -distinction. +A disabled `GatedBy` gate deletes the resource on the next reconcile. The delete is by name, so it also removes an +object that a user created under that name. If users can create an object with the same name, for example a +ServiceAccount that a user brings instead of the one the operator makes, also register the resource with +[`component.DeleteOnlyIfOwned()`](component.md#resource-registration-options). The component then deletes only an object +that carries the owner reference of the owner. For an optional resource the component does **not** own (a read-only +Secret reference behind an optional spec field), use `IncludeWhen`, which omits the resource without ever deleting it. +The [IncludeWhen vs. GatedBy](component.md#includewhen-vs-gatedby) section covers the distinction. ## Provide a User-Override Escape Hatch as the Last Mutation diff --git a/pkg/component/delete.go b/pkg/component/delete.go index 4005b9d7..2d974675 100644 --- a/pkg/component/delete.go +++ b/pkg/component/delete.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "slices" v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -42,6 +43,8 @@ func withDeletionReason(reason string) deleteOption { // live object is controlled by another owner: the object is not this // component's to delete, whichever path (a deletion flag, a disabled feature // gate, suspension) asked for it. The skip is logged with the controlling owner. +// An entry registered with DeleteOnlyIfOwned is left in place, and logged, +// when the live object does not carry an owner reference to the owner. // // For each successful deletion, a "ResourceDeleted" event is recorded on the owner object. // The opts parameter allows callers to customize the event message via functional options. @@ -92,22 +95,19 @@ func deleteResources( // reports whether a deletion happened. An object that is already gone is not // an error and reports false. // -// For an entry registered with BlockOnForeignController the delete is bound to -// what was observed: the live object is read first (through the API reader -// when set), an absent object counts as deleted, an object another owner -// controls is left in place and logged, and an object observed as safe is -// deleted with its UID and resourceVersion as preconditions. An owner that -// claims the object between the read and the delete therefore makes the delete -// conflict instead of removing that owner's object; the error is returned so -// the next reconcile observes the object again. Unlike a lost apply race, a -// lost delete race is not repaired by the next reconcile, which is why the -// delete carries the precondition and the apply does not. +// For an entry registered with BlockOnForeignController or DeleteOnlyIfOwned +// the delete is bound to what was observed: the live object is read first +// (through the API reader when set), an absent object counts as deleted, an +// object another owner controls (BlockOnForeignController) or that does not +// reference the owner (DeleteOnlyIfOwned) is left in place and logged, and any +// other object is deleted with its UID and resourceVersion as preconditions. func deleteEntry( ctx context.Context, rec ReconcileContext, entry reconcileEntry, object client.Object, ) (bool, error) { resource := entry.Resource + options := entry.Options - if !entry.Options.BlockOnForeignController { + if !options.BlockOnForeignController && !options.DeleteOnlyIfOwned { if err := rec.Client.Delete(ctx, object); err != nil { if apierrors.IsNotFound(err) { return false, nil @@ -124,7 +124,8 @@ func deleteEntry( if live == nil { return false, nil } - if controller != nil { + + if options.BlockOnForeignController && controller != nil { log.FromContext(ctx).Info( "skipping deletion of a resource another owner controls", "resource", resource.Identity(), "controller", controller.Kind+" "+controller.Name, @@ -132,15 +133,35 @@ func deleteEntry( return false, nil } + if options.DeleteOnlyIfOwned && !slices.ContainsFunc(live.GetOwnerReferences(), referencesOwner(rec.Owner.GetUID())) { + log.FromContext(ctx).Info( + "skipping deletion of a resource that does not reference the owner", "resource", resource.Identity(), + ) + return false, nil + } + + // An owner that claims the object, or a user that replaces it, between the + // read and the delete makes the delete conflict instead of removing an + // object the checks above did not see. Unlike a lost apply race, a lost + // delete race is not repaired by the next reconcile, which is why the + // delete carries the precondition and the apply does not. uid, resourceVersion := live.GetUID(), live.GetResourceVersion() err = rec.Client.Delete(ctx, live, client.Preconditions{UID: &uid, ResourceVersion: &resourceVersion}) - if err != nil { - if apierrors.IsNotFound(err) { - return false, nil - } - return false, fmt.Errorf( - "failed to delete resource %s as observed: %w", resource.Identity(), err, + switch { + case err == nil: + return true, nil + case apierrors.IsNotFound(err): + return false, nil + case options.DeleteOnlyIfOwned && apierrors.IsConflict(err): + // The object changed after it was read. Leaving it is the safe outcome + // and needs no retry: the next reconcile reads it again. + log.FromContext(ctx).Info( + "skipping deletion of a resource that changed after it was read", "resource", resource.Identity(), ) + return false, nil + default: + // BlockOnForeignController alone returns the conflict so the reconcile + // fails and the object is observed again on the retry. + return false, fmt.Errorf("failed to delete resource %s as observed: %w", resource.Identity(), err) } - return true, nil } diff --git a/pkg/component/delete_owned_test.go b/pkg/component/delete_owned_test.go new file mode 100644 index 00000000..54ba97e1 --- /dev/null +++ b/pkg/component/delete_owned_test.go @@ -0,0 +1,150 @@ +package component + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +// A resource registered with DeleteOnlyIfOwned is deleted by a deletion +// trigger only when the live object carries the owner's reference, so an +// object a user created under the same name survives +// (sourcehawk/operator-component-framework#233). +var _ = Describe("DeleteOnlyIfOwned", func() { + var ( + ctx = context.Background() + namespace string + owner *MockOperatorCRD + ) + + const cmName = "gated-cm" + key := func() client.ObjectKey { return client.ObjectKey{Name: cmName, Namespace: namespace} } + + // trigger builds a component whose only resource is asked to be deleted by + // one of the deletion triggers, with the extra resource options appended. + type trigger func(res Resource, opts ...ResourceOption) (*Component, error) + + gatedBy := func(res Resource, opts ...ResourceOption) (*Component, error) { + return NewComponentBuilder(). + WithName("gated"). + WithConditionType("GatedReady"). + WithResource(res, append([]ResourceOption{GatedBy(&testGate{enabled: false})}, opts...)...). + Build() + } + deleteWhen := func(res Resource, opts ...ResourceOption) (*Component, error) { + return NewComponentBuilder(). + WithName("gated"). + WithConditionType("GatedReady"). + WithResource(res, append([]ResourceOption{DeleteWhen(true)}, opts...)...). + Build() + } + disabledComponentGate := func(res Resource, opts ...ResourceOption) (*Component, error) { + return NewComponentBuilder(). + WithName("gated"). + WithConditionType("GatedReady"). + WithFeatureGate(&testGate{enabled: false}). + WithResource(res, opts...). + Build() + } + + desiredResource := func() *MockResource { + res := &MockResource{} + res.On("Object").Return(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: cmName, Namespace: namespace}}, nil) + res.On("Identity").Return("ConfigMap/" + cmName) + return res + } + + createConfigMap := func(owned bool) *corev1.ConfigMap { + GinkgoHelper() + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: cmName, Namespace: namespace}, + Data: map[string]string{"created-by": "user"}, + } + if owned { + Expect(controllerutil.SetControllerReference(owner, cm, scheme.Scheme)).To(Succeed()) + } + Expect(k8sClient.Create(ctx, cm)).To(Succeed()) + return cm + } + + BeforeEach(func() { + namespace = createNamespace(ctx, "delete-owned-test-") + owner = &MockOperatorCRD{ObjectMeta: metav1.ObjectMeta{Name: "owner", Namespace: namespace}} + Expect(k8sClient.Create(ctx, owner)).To(Succeed()) + }) + + AfterEach(func() { + Expect(k8sClient.Delete(ctx, owner)).To(Succeed()) + }) + + DescribeTable("deletes an object that carries the owner's reference", + func(build trigger) { + createConfigMap(true) + comp, err := build(desiredResource(), DeleteOnlyIfOwned()) + Expect(err).NotTo(HaveOccurred()) + + rec := newTestReconcileContext(owner) + Expect(comp.Reconcile(ctx, rec)).To(Succeed()) + + Expect(apierrors.IsNotFound(k8sClient.Get(ctx, key(), &corev1.ConfigMap{}))).To(BeTrue()) + Expect(rec.EventRecorder.(*spyRecorder).recordedWithReason("ResourceDeleted")).To(HaveLen(1)) + }, + Entry("GatedBy with a disabled gate", trigger(gatedBy)), + Entry("DeleteWhen(true)", trigger(deleteWhen)), + Entry("a disabled component feature gate", trigger(disabledComponentGate)), + ) + + DescribeTable("keeps an object without the owner's reference unchanged", + func(build trigger) { + createConfigMap(false) + before := &corev1.ConfigMap{} + Expect(k8sClient.Get(ctx, key(), before)).To(Succeed()) + comp, err := build(desiredResource(), DeleteOnlyIfOwned()) + Expect(err).NotTo(HaveOccurred()) + + rec := newTestReconcileContext(owner) + Expect(comp.Reconcile(ctx, rec)).To(Succeed()) + + after := &corev1.ConfigMap{} + Expect(k8sClient.Get(ctx, key(), after)).To(Succeed()) + Expect(after.ResourceVersion).To(Equal(before.ResourceVersion), "a kept object must not be written") + Expect(after.Data).To(HaveKeyWithValue("created-by", "user")) + Expect(rec.EventRecorder.(*spyRecorder).recordedWithReason("ResourceDeleted")).To(BeEmpty()) + }, + Entry("GatedBy with a disabled gate", trigger(gatedBy)), + Entry("DeleteWhen(true)", trigger(deleteWhen)), + Entry("a disabled component feature gate", trigger(disabledComponentGate)), + ) + + DescribeTable("without the option, still deletes an object without the owner's reference", + func(build trigger) { + createConfigMap(false) + comp, err := build(desiredResource()) + Expect(err).NotTo(HaveOccurred()) + + Expect(comp.Reconcile(ctx, newTestReconcileContext(owner))).To(Succeed()) + + Expect(apierrors.IsNotFound(k8sClient.Get(ctx, key(), &corev1.ConfigMap{}))).To(BeTrue()) + }, + Entry("GatedBy with a disabled gate", trigger(gatedBy)), + Entry("DeleteWhen(true)", trigger(deleteWhen)), + Entry("a disabled component feature gate", trigger(disabledComponentGate)), + ) + + It("reports Disabled for a disabled component feature gate whether or not the object is kept", func() { + createConfigMap(false) + comp, err := disabledComponentGate(desiredResource(), DeleteOnlyIfOwned()) + Expect(err).NotTo(HaveOccurred()) + + Expect(comp.Reconcile(ctx, newTestReconcileContext(owner))).To(Succeed()) + + Expect(comp.GetCondition(owner).Reason).To(Equal(string(Disabled))) + }) +}) diff --git a/pkg/component/delete_test.go b/pkg/component/delete_test.go index 56ddd765..d4b25b2a 100644 --- a/pkg/component/delete_test.go +++ b/pkg/component/delete_test.go @@ -11,6 +11,7 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -295,3 +296,194 @@ func TestDeleteResources_BlockOnForeignController(t *testing.T) { assert.Equal(t, "other-uid", string(got.OwnerReferences[0].UID)) }) } + +func TestDeleteResources_DeleteOnlyIfOwned(t *testing.T) { + ctx := t.Context() + scheme := setupScheme() + newOwner := func() *MockOperatorCRD { + return &MockOperatorCRD{ObjectMeta: metav1.ObjectMeta{Name: "owner", Namespace: "default", UID: "this-uid"}} + } + ownerRef := func(name string, uid string) metav1.OwnerReference { + return metav1.OwnerReference{APIVersion: GroupVersion.String(), Kind: "MockOperatorCRD", Name: name, UID: types.UID(uid)} + } + newResource := func(name string, refs ...metav1.OwnerReference) (*MockResource, *corev1.ConfigMap) { + obj := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "default", OwnerReferences: refs}} + res := &MockResource{} + res.On("Object").Return(obj, nil) + res.On("Identity").Return("ConfigMap/" + name) + return res, obj + } + ownedOnly := func(res Resource) []reconcileEntry { + return []reconcileEntry{{Resource: res, Options: resourceOptions{Delete: true, DeleteOnlyIfOwned: true}}} + } + countDeletes := func(deletes *int) interceptor.Funcs { + return interceptor.Funcs{ + Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + *deletes++ + return c.Delete(ctx, obj, opts...) + }, + } + } + + t.Run("deletes an object that carries the owner's reference and records the deletion", func(t *testing.T) { + owner := newOwner() + res, obj := newResource("owned", ownerRef("owner", "this-uid")) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj.DeepCopy()).Build() + rec := setupReconcileContext(scheme, owner, cli) + + require.NoError(t, deleteResources(ctx, rec, ownedOnly(res))) + + assert.True(t, apierrors.IsNotFound(cli.Get(ctx, client.ObjectKeyFromObject(obj), &corev1.ConfigMap{}))) + recorder, ok := rec.EventRecorder.(*spyRecorder) + require.True(t, ok) + assert.Len(t, recorder.recordedWithReason("ResourceDeleted"), 1) + }) + + t.Run("deletes an object whose owner reference is not the controller reference", func(t *testing.T) { + owner := newOwner() + controller := ownerRef("other", "other-uid") + controller.Controller = ptr.To(true) + res, obj := newResource("co-owned", controller, ownerRef("owner", "this-uid")) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj.DeepCopy()).Build() + + require.NoError(t, deleteResources(ctx, setupReconcileContext(scheme, owner, cli), ownedOnly(res))) + + assert.True(t, apierrors.IsNotFound(cli.Get(ctx, client.ObjectKeyFromObject(obj), &corev1.ConfigMap{}))) + }) + + t.Run("deletes with the observed UID and resourceVersion as preconditions", func(t *testing.T) { + owner := newOwner() + res, obj := newResource("preconditions", ownerRef("owner", "this-uid")) + var preconditions *metav1.Preconditions + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj.DeepCopy()). + WithInterceptorFuncs(interceptor.Funcs{ + Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + deleteOpts := &client.DeleteOptions{} + deleteOpts.ApplyOptions(opts) + preconditions = deleteOpts.Preconditions + return c.Delete(ctx, obj, opts...) + }, + }).Build() + live := &corev1.ConfigMap{} + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(obj), live)) + + require.NoError(t, deleteResources(ctx, setupReconcileContext(scheme, owner, cli), ownedOnly(res))) + + require.NotNil(t, preconditions) + require.NotNil(t, preconditions.UID) + require.NotNil(t, preconditions.ResourceVersion) + assert.Equal(t, live.UID, *preconditions.UID) + assert.Equal(t, live.ResourceVersion, *preconditions.ResourceVersion) + }) + + t.Run("keeps an object without the owner's reference and writes nothing", func(t *testing.T) { + owner := newOwner() + res, obj := newResource("user-created") + deletes := 0 + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj.DeepCopy()). + WithInterceptorFuncs(countDeletes(&deletes)).Build() + before := &corev1.ConfigMap{} + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(obj), before)) + rec := setupReconcileContext(scheme, owner, cli) + + require.NoError(t, deleteResources(ctx, rec, ownedOnly(res))) + + assert.Equal(t, 0, deletes) + after := &corev1.ConfigMap{} + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(obj), after)) + assert.Equal(t, before.ResourceVersion, after.ResourceVersion) + recorder, ok := rec.EventRecorder.(*spyRecorder) + require.True(t, ok) + assert.Empty(t, recorder.recorded(), "a kept object must not be reported as deleted") + }) + + t.Run("keeps an object that only another owner references", func(t *testing.T) { + owner := newOwner() + res, obj := newResource("other-owned", ownerRef("other", "other-uid")) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj.DeepCopy()).Build() + + require.NoError(t, deleteResources(ctx, setupReconcileContext(scheme, owner, cli), ownedOnly(res))) + + assert.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(obj), &corev1.ConfigMap{})) + }) + + t.Run("treats an absent object as deleted without calling Delete", func(t *testing.T) { + owner := newOwner() + res, _ := newResource("absent") + deletes := 0 + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner). + WithInterceptorFuncs(countDeletes(&deletes)).Build() + + require.NoError(t, deleteResources(ctx, setupReconcileContext(scheme, owner, cli), ownedOnly(res))) + + assert.Equal(t, 0, deletes) + }) + + t.Run("keeps an object that changed after it was read without failing", func(t *testing.T) { + owner := newOwner() + res, obj := newResource("changed", ownerRef("owner", "this-uid")) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj.DeepCopy()). + WithInterceptorFuncs(disownBeforeDelete(t, "changed")).Build() + rec := setupReconcileContext(scheme, owner, cli) + + require.NoError(t, deleteResources(ctx, rec, ownedOnly(res))) + + got := &corev1.ConfigMap{} + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(obj), got)) + assert.Empty(t, got.OwnerReferences) + recorder, ok := rec.EventRecorder.(*spyRecorder) + require.True(t, ok) + assert.Empty(t, recorder.recorded()) + }) + + t.Run("still reports a delete failure that is not a precondition conflict", func(t *testing.T) { + owner := newOwner() + res, obj := newResource("forbidden", ownerRef("owner", "this-uid")) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj.DeepCopy()). + WithInterceptorFuncs(interceptor.Funcs{ + Delete: func(context.Context, client.WithWatch, client.Object, ...client.DeleteOption) error { + return apierrors.NewForbidden(corev1.Resource("configmaps"), "forbidden", errors.New("denied")) + }, + }).Build() + + err := deleteResources(ctx, setupReconcileContext(scheme, owner, cli), ownedOnly(res)) + + require.Error(t, err) + assert.True(t, apierrors.IsForbidden(err)) + }) + + t.Run("keeps an object another owner controls when combined with BlockOnForeignController", func(t *testing.T) { + owner := newOwner() + controller := ownerRef("other", "other-uid") + controller.Controller = ptr.To(true) + res, obj := newResource("foreign", controller, ownerRef("owner", "this-uid")) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj.DeepCopy()).Build() + entries := []reconcileEntry{{ + Resource: res, + Options: resourceOptions{Delete: true, DeleteOnlyIfOwned: true, BlockOnForeignController: true}, + }} + + require.NoError(t, deleteResources(ctx, setupReconcileContext(scheme, owner, cli), entries)) + + assert.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(obj), &corev1.ConfigMap{})) + }) +} + +// disownBeforeDelete returns interceptor funcs that remove every owner +// reference from the named object at the moment it is about to be deleted, +// reproducing a change between the read that found the object owned and the +// delete. +func disownBeforeDelete(t *testing.T, name string) interceptor.Funcs { + t.Helper() + return interceptor.Funcs{ + Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + if obj.GetName() == name { + live := obj.DeepCopyObject().(client.Object) + require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(obj), live)) + live.SetOwnerReferences(nil) + require.NoError(t, c.Update(ctx, live)) + } + return c.Delete(ctx, obj, opts...) + }, + } +} diff --git a/pkg/component/orphan.go b/pkg/component/orphan.go index 6086f969..8813789c 100644 --- a/pkg/component/orphan.go +++ b/pkg/component/orphan.go @@ -4,10 +4,12 @@ import ( "context" "errors" "fmt" + "slices" v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -55,14 +57,8 @@ func orphanResources(ctx context.Context, rec ReconcileContext, resources []Reso return getErr } refs := live.GetOwnerReferences() - kept := make([]metav1.OwnerReference, 0, len(refs)) - for _, ref := range refs { - if ref.UID == ownerUID { - removed = true - continue - } - kept = append(kept, ref) - } + kept := slices.DeleteFunc(slices.Clone(refs), referencesOwner(ownerUID)) + removed = len(kept) != len(refs) if !removed { return nil } @@ -84,3 +80,9 @@ func orphanResources(ctx context.Context, rec ReconcileContext, resources []Reso } return errors.Join(errs...) } + +// referencesOwner matches an owner reference to the owner with ownerUID, +// whether or not it is the controller reference. +func referencesOwner(ownerUID types.UID) func(metav1.OwnerReference) bool { + return func(ref metav1.OwnerReference) bool { return ref.UID == ownerUID } +} diff --git a/pkg/component/resource_options.go b/pkg/component/resource_options.go index 7406df24..957d8364 100644 --- a/pkg/component/resource_options.go +++ b/pkg/component/resource_options.go @@ -34,6 +34,7 @@ type resourceConfig struct { blockOnAbsence bool ignoreIfAbsent bool blockOnForeignController bool + deleteOnlyIfOwned bool suppressGraceInconsistencyWarning bool } @@ -79,6 +80,11 @@ type resourceOptions struct { // paths (a deletion flag, a disabled feature gate, suspension) leave such an // object in place. Mutually exclusive with ReadOnly. BlockOnForeignController bool + // DeleteOnlyIfOwned applies to managed resources. When true, a deletion + // path deletes the live object only when it carries an owner reference + // with the reconciling owner's UID, and leaves any other object in place. + // Mutually exclusive with ReadOnly and Unowned. + DeleteOnlyIfOwned bool } // ReadOnly marks the resource as read-only: the component fetches its current @@ -201,6 +207,33 @@ func BlockOnForeignController() ResourceOption { return func(c *resourceConfig) { c.blockOnForeignController = true } } +// DeleteOnlyIfOwned limits every deletion of the resource to an object the +// owner owns. When Delete, DeleteWhen, GatedBy, a disabled component feature +// gate, or suspension with DeleteOnSuspend asks for the resource to be +// deleted, the component reads the live object, through +// ReconcileContext.APIReader when set and the cached ReconcileContext.Client +// otherwise. It deletes the object only when the object carries an owner +// reference whose UID is the reconciling owner's; the reference need not be +// the controller reference. Any other object is left in place with no write, +// no event, and a log line. Use it when the resource name is one a user may +// also create by hand. +// +// The delete carries the observed UID and resourceVersion as preconditions. +// An object that changes or is replaced between the read and the delete is +// left in place and logged, and the reconcile does not fail; the next +// reconcile reads the object again. Combined with BlockOnForeignController, an +// object another owner controls is kept even when it also references this +// owner, and a precondition conflict does not fail the reconcile. +// +// Without this option a deletion removes the object by name, whoever owns it. +// Requires a managed resource that carries the owner reference: combining it +// with ReadOnly or Unowned is a configuration error returned by Build. A +// cluster-scoped resource of a namespace-scoped owner never carries the +// reference either, so the option never deletes it. +func DeleteOnlyIfOwned() ResourceOption { + return func(c *resourceConfig) { c.deleteOnlyIfOwned = true } +} + // SuppressGraceInconsistencyWarning suppresses the warning log emitted when the // resource's grace handler returns Healthy while its convergence handler returns // non-healthy. Use this when the inconsistency is intentional. @@ -242,6 +275,19 @@ func (c *resourceConfig) resolve() (resourceOptions, error) { ) } + if c.deleteOnlyIfOwned && c.readOnly { + return resourceOptions{}, errors.New( + "resource option DeleteOnlyIfOwned is mutually exclusive with ReadOnly; " + + "a read-only resource is never deleted", + ) + } + if c.deleteOnlyIfOwned && c.unowned { + return resourceOptions{}, errors.New( + "resource option DeleteOnlyIfOwned is mutually exclusive with Unowned; " + + "an unowned resource never carries the owner reference, so it would never be deleted", + ) + } + // A read-only resource is not owned by the component, so it must never be // deleted. Combining ReadOnly with any deletion trigger is a configuration // error; use IncludeWhen to conditionally include a read-only resource. @@ -314,5 +360,6 @@ func (c *resourceConfig) resolve() (resourceOptions, error) { BlockOnAbsence: c.blockOnAbsence, IgnoreIfAbsent: c.ignoreIfAbsent, BlockOnForeignController: c.blockOnForeignController, + DeleteOnlyIfOwned: c.deleteOnlyIfOwned, }, nil } diff --git a/pkg/component/resource_options_test.go b/pkg/component/resource_options_test.go index 162c8199..e565a238 100644 --- a/pkg/component/resource_options_test.go +++ b/pkg/component/resource_options_test.go @@ -114,6 +114,25 @@ func TestResolveResourceOptions(t *testing.T) { opts: []ResourceOption{Unowned(), BlockOnForeignController()}, want: resourceOptions{Unowned: true, BlockOnForeignController: true, ParticipationMode: ParticipationModeRequired}, }, + { + name: "DeleteOnlyIfOwned sets flag", + opts: []ResourceOption{DeleteOnlyIfOwned()}, + want: resourceOptions{DeleteOnlyIfOwned: true, ParticipationMode: ParticipationModeRequired}, + }, + { + name: "DeleteOnlyIfOwned with a disabled gate marks the resource for deletion", + opts: []ResourceOption{GatedBy(&disabledFeature{}), DeleteOnlyIfOwned()}, + want: resourceOptions{Delete: true, DeleteOnlyIfOwned: true, ParticipationMode: ParticipationModeRequired}, + }, + { + name: "DeleteOnlyIfOwned alongside BlockOnForeignController", + opts: []ResourceOption{DeleteOnlyIfOwned(), BlockOnForeignController()}, + want: resourceOptions{ + DeleteOnlyIfOwned: true, + BlockOnForeignController: true, + ParticipationMode: ParticipationModeRequired, + }, + }, { name: "ReadOnly sets flag", opts: []ResourceOption{ReadOnly()}, @@ -221,6 +240,16 @@ func TestResolveResourceOptions_ValidationErrors(t *testing.T) { opts: []ResourceOption{ReadOnly(), BlockOnForeignController()}, wantErrIs: "BlockOnForeignController is mutually exclusive with ReadOnly", }, + { + name: "DeleteOnlyIfOwned with ReadOnly errors", + opts: []ResourceOption{ReadOnly(), DeleteOnlyIfOwned()}, + wantErrIs: "DeleteOnlyIfOwned is mutually exclusive with ReadOnly", + }, + { + name: "DeleteOnlyIfOwned with Unowned errors", + opts: []ResourceOption{Unowned(), DeleteOnlyIfOwned()}, + wantErrIs: "DeleteOnlyIfOwned is mutually exclusive with Unowned", + }, { name: "BlockOnAbsence without ReadOnly errors", opts: []ResourceOption{BlockOnAbsence()}, diff --git a/pkg/component/suspend.go b/pkg/component/suspend.go index 7c0531d0..08e70cd6 100644 --- a/pkg/component/suspend.go +++ b/pkg/component/suspend.go @@ -184,8 +184,8 @@ func suspendResource( } // Delete resource if it should be deleted. deleteEntry re-observes an entry - // registered with BlockOnForeignController, so an owner that claimed the - // object during the suspension apply keeps it. + // registered with BlockOnForeignController or DeleteOnlyIfOwned, so an owner + // that claimed the object during the suspension apply keeps it. if suspendable.DeleteOnSuspend() { deleted, err := deleteEntry(ctx, rec, entry, object) if err != nil { diff --git a/plugin/skills/building-components/SKILL.md b/plugin/skills/building-components/SKILL.md index d4d42c51..57db9132 100644 --- a/plugin/skills/building-components/SKILL.md +++ b/plugin/skills/building-components/SKILL.md @@ -53,11 +53,15 @@ if err != nil { Each `WithResource` call accepts `ResourceOption` values: `component.ReadOnly()`, `component.Delete()` / `component.DeleteWhen(cond)`, `component.GatedBy(gate)`, `component.OrphanWhen(cond)`, `component.Unowned()`, `component.Auxiliary()`, `component.BlockOnAbsence()`, `component.IgnoreIfAbsent()`, -`component.BlockOnForeignController()`. With no options a resource is **Managed**: applied via Server-Side Apply, -required for the condition. `ReadOnly()` is mutually exclusive with the deletion and gating options. -`BlockOnForeignController()` reports `Blocked` and names the owner whose controller reference is on the live object, -instead of applying over it. Register it on any resource that two custom resources can name. See -`references/component.md` for the full option matrix and `IncludeWhen` vs. `GatedBy`. +`component.BlockOnForeignController()`, `component.DeleteOnlyIfOwned()`. With no options a resource is **Managed**: +applied via Server-Side Apply, required for the condition. `ReadOnly()` is mutually exclusive with the deletion and +gating options. `BlockOnForeignController()` reports `Blocked` and names the owner whose controller reference is on the +live object, instead of applying over it. Register it on any resource that two custom resources can name. A deletion +(`Delete()`, `DeleteWhen()`, `GatedBy()`, a disabled component feature gate, `DeleteOnSuspend()`) removes the object by +name, whoever created it. `DeleteOnlyIfOwned()` deletes it only when the live object carries the owner's owner +reference, and leaves any other object unchanged with no event and no reconcile error. Register it on a resource whose +name a user may also create by hand. It cannot combine with `ReadOnly()` or `Unowned()`. See `references/component.md` +for the full option matrix and `IncludeWhen` vs. `GatedBy`. ## Registration order is execution order diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 185ef7be..49022831 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -67,6 +67,7 @@ be passed without a guard. | `component.BlockOnAbsence()` | Read-only only: a NotFound records a blocked status and short-circuits the remaining resources | | `component.IgnoreIfAbsent()` | Read-only only: a NotFound is silently ignored and last-known state is preserved | | `component.BlockOnForeignController()` | Managed only: records a blocked status that names the owner whose controller reference is on the live object, then skips the apply and the remaining resources | +| `component.DeleteOnlyIfOwned()` | Managed only: a deletion removes the live object only when it carries an owner reference to the owner; any other object is left in place | | `component.SuppressGraceInconsistencyWarning()` | Suppresses the grace/convergence inconsistency warning | A read-only resource is not owned by the component, so it is never deleted. `ReadOnly()` is mutually exclusive with @@ -106,6 +107,31 @@ A delete of an object that the read found safe carries the observed UID and reso owner claims the object between the read and the delete, the delete fails and the next reconcile reads the object again. The option requires a managed resource. A combination with `ReadOnly()` is a build error. +`DeleteOnlyIfOwned()` protects an object that the component did not create from a deletion. Without it, every deletion +path removes the object by name, whoever created it. Use the option when the resource name is one that a user can also +create by hand. For example, a component registers `-app` as a ServiceAccount behind `GatedBy()`. When the gate is +off and the user created their own `-app`, the option keeps the object of the user. + +When `Delete()`, `DeleteWhen()`, `GatedBy()`, a disabled component feature gate, or a suspension with +`DeleteOnSuspend()` asks for a deletion, the component first reads the live object, through `ReconcileContext.APIReader` +when it is set and `ReconcileContext.Client` otherwise. The component deletes the object only when the object has an +owner reference with the UID of the owner. This reference does not have to be the controller reference. The component +sets this reference on each object that it applies, with two exceptions: an `Unowned()` resource, and a cluster-scoped +resource of a namespace-scoped owner. The option never deletes an object of the second kind, so do not use it there. If +the object does not have the reference, the component does not write to the object and records no `ResourceDeleted` +event. It logs the skip, and the reconcile does not fail. The resource does not contribute to the component condition. +As with a deletion, a disabled component feature gate reports `Disabled`. While no deletion is asked for, the option has +no effect. + +The delete carries the observed UID and resourceVersion as preconditions. If the object changes, or a different object +takes the name, between the read and the delete, the API server rejects the delete with a conflict. The component then +keeps the object, logs the skip, and does not fail the reconcile. The next reconcile reads the object again. + +The option composes with `BlockOnForeignController()`. With both options, the component keeps an object that another +owner controls, even when the object also references this owner. A conflict on the delete then does not fail the +reconcile. `DeleteOnlyIfOwned()` requires a managed resource that carries the owner reference. A combination with +`ReadOnly()` or `Unowned()` is a build error. + Options compose. Gate a resource and exclude it from health aggregation in one call: ```go diff --git a/plugin/skills/structuring-operators/SKILL.md b/plugin/skills/structuring-operators/SKILL.md index cd4b82e5..947bd0fc 100644 --- a/plugin/skills/structuring-operators/SKILL.md +++ b/plugin/skills/structuring-operators/SKILL.md @@ -172,8 +172,10 @@ cacheComp, err := component.NewComponentBuilder(). ``` A resource-level `component.GatedBy` does the same for one resource the component owns, deleting it once the gate turns -off. For an optional resource the component does not own, such as a read-only Secret reference behind an optional spec -field, use `IncludeWhen` instead, which omits the resource without ever deleting it. +off. The delete is by name: when a user may create an object under the same name, add `component.DeleteOnlyIfOwned()` so +only an object that carries the owner's owner reference is deleted. For an optional resource the component does not own, +such as a read-only Secret reference behind an optional spec field, use `IncludeWhen` instead, which omits the resource +without ever deleting it. `IncludeWhen` also covers the other optional case: **the cluster may not serve the kind at all**. A `ServiceMonitor` exists only where the Prometheus Operator is installed, and the same holds for cert-manager kinds and any other optional diff --git a/plugin/skills/structuring-operators/references/guidelines.md b/plugin/skills/structuring-operators/references/guidelines.md index 8f7e9440..cd549479 100644 --- a/plugin/skills/structuring-operators/references/guidelines.md +++ b/plugin/skills/structuring-operators/references/guidelines.md @@ -707,9 +707,13 @@ comp, _ := component.NewComponentBuilder(). Build() ``` -A disabled `GatedBy` gate deletes the resource on the next reconcile. For an optional resource the component does -**not** own (a read-only Secret reference behind an optional spec field), use `IncludeWhen`, which omits the resource -without ever deleting it. The +A disabled `GatedBy` gate deletes the resource on the next reconcile. The delete is by name, so it also removes an +object that a user created under that name. If users can create an object with the same name, for example a +ServiceAccount that a user brings instead of the one the operator makes, also register the resource with +[`component.DeleteOnlyIfOwned()`](../../building-components/references/component.md#resource-registration-options). The +component then deletes only an object that carries the owner reference of the owner. For an optional resource the +component does **not** own (a read-only Secret reference behind an optional spec field), use `IncludeWhen`, which omits +the resource without ever deleting it. The [IncludeWhen vs. GatedBy](../../building-components/references/component.md#includewhen-vs-gatedby) section covers the distinction. From b70ff4737087db341a2a5960b37d7ca91b0d6ba3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 13:23:13 +0100 Subject: [PATCH 2/5] fix(component): keep an unowned object on delete-on-suspend With DeleteOnlyIfOwned and DeleteOnSuspend, the suspension apply set the owner reference on a user-created object, and the delete after it then found the object owned and deleted it. The suspension now checks the owner reference before Suspend and the apply, and an object without it counts as suspended with no write. The read goes through the API reader when set, so a stale cache that still shows an earlier owned object does not let the apply take over a new one. Co-Authored-By: Claude Opus 5.5 --- docs/component.md | 12 +++-- pkg/component/delete_owned_test.go | 49 +++++++++++++++++++ pkg/component/resource_options.go | 9 ++-- pkg/component/suspend.go | 23 ++++++++- pkg/component/suspend_test.go | 34 +++++++++++++ .../references/component.md | 12 +++-- 6 files changed, 125 insertions(+), 14 deletions(-) diff --git a/docs/component.md b/docs/component.md index 81b836a3..18eae9e2 100644 --- a/docs/component.md +++ b/docs/component.md @@ -107,9 +107,10 @@ owner claims the object between the read and the delete, the delete fails and th The option requires a managed resource. A combination with `ReadOnly()` is a build error. `DeleteOnlyIfOwned()` protects an object that the component did not create from a deletion. Without it, every deletion -path removes the object by name, whoever created it. Use the option when the resource name is one that a user can also -create by hand. For example, a component registers `-app` as a ServiceAccount behind `GatedBy()`. When the gate is -off and the user created their own `-app`, the option keeps the object of the user. +path removes the object by name, whoever created it. The only exception is an object that another owner controls, which +`BlockOnForeignController()` keeps. Use the option when the resource name is one that a user can also create by hand. +For example, a component registers `-app` as a ServiceAccount behind `GatedBy()`. When the gate is off and the +user created their own `-app`, the option keeps the object of the user. When `Delete()`, `DeleteWhen()`, `GatedBy()`, a disabled component feature gate, or a suspension with `DeleteOnSuspend()` asks for a deletion, the component first reads the live object, through `ReconcileContext.APIReader` @@ -119,8 +120,9 @@ sets this reference on each object that it applies, with two exceptions: an `Uno resource of a namespace-scoped owner. The option never deletes an object of the second kind, so do not use it there. If the object does not have the reference, the component does not write to the object and records no `ResourceDeleted` event. It logs the skip, and the reconcile does not fail. The resource does not contribute to the component condition. -As with a deletion, a disabled component feature gate reports `Disabled`. While no deletion is asked for, the option has -no effect. +As with a deletion, a disabled component feature gate reports `Disabled`. During a suspension with `DeleteOnSuspend()`, +the component also skips the suspension apply for such an object, because that apply would set the owner reference. The +resource counts as suspended. While no deletion is asked for, the option has no effect. The delete carries the observed UID and resourceVersion as preconditions. If the object changes, or a different object takes the name, between the read and the delete, the API server rejects the delete with a conflict. The component then diff --git a/pkg/component/delete_owned_test.go b/pkg/component/delete_owned_test.go index 54ba97e1..63474c94 100644 --- a/pkg/component/delete_owned_test.go +++ b/pkg/component/delete_owned_test.go @@ -5,6 +5,8 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/sourcehawk/operator-component-framework/pkg/component/concepts" + "github.com/stretchr/testify/mock" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -138,6 +140,53 @@ var _ = Describe("DeleteOnlyIfOwned", func() { Entry("a disabled component feature gate", trigger(disabledComponentGate)), ) + suspendedComponent := func(res Resource) *Component { + return &Component{ + name: "gated", + conditionType: "GatedReady", + suspended: true, + reconcileResources: []reconcileEntry{{ + Resource: res, Options: resourceOptions{DeleteOnlyIfOwned: true}, + }}, + } + } + + It("deletes an owned object on suspension with DeleteOnSuspend", func() { + createConfigMap(true) + res := &MockSuspendableResource{} + res.On("Object").Return(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: cmName, Namespace: namespace}}, nil) + res.On("Identity").Return("ConfigMap/" + cmName) + res.On("DeleteOnSuspend").Return(true) + res.On("Suspend").Return(nil) + res.On("Mutate", mock.Anything).Return(nil) + res.On("SuspensionStatus").Return(concepts.SuspensionStatusWithReason{Status: concepts.SuspensionStatusSuspended}, nil) + + Expect(suspendedComponent(res).Reconcile(ctx, newTestReconcileContext(owner))).To(Succeed()) + + Expect(apierrors.IsNotFound(k8sClient.Get(ctx, key(), &corev1.ConfigMap{}))).To(BeTrue()) + }) + + It("keeps an object without the owner's reference unchanged on suspension with DeleteOnSuspend", func() { + createConfigMap(false) + before := &corev1.ConfigMap{} + Expect(k8sClient.Get(ctx, key(), before)).To(Succeed()) + res := &MockSuspendableResource{} + res.On("Object").Return(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: cmName, Namespace: namespace}}, nil) + res.On("Identity").Return("ConfigMap/" + cmName) + res.On("DeleteOnSuspend").Return(true) + // No Suspend, Mutate or SuspensionStatus expectation: reaching any of them + // means the suspension apply was about to adopt the user's object, after + // which the delete would find it owned. + comp := suspendedComponent(res) + + Expect(comp.Reconcile(ctx, newTestReconcileContext(owner))).To(Succeed()) + + Expect(comp.GetCondition(owner).Reason).To(Equal(string(Suspended))) + after := &corev1.ConfigMap{} + Expect(k8sClient.Get(ctx, key(), after)).To(Succeed()) + Expect(after.ResourceVersion).To(Equal(before.ResourceVersion), "a kept object must not be written") + }) + It("reports Disabled for a disabled component feature gate whether or not the object is kept", func() { createConfigMap(false) comp, err := disabledComponentGate(desiredResource(), DeleteOnlyIfOwned()) diff --git a/pkg/component/resource_options.go b/pkg/component/resource_options.go index 957d8364..257ddaf4 100644 --- a/pkg/component/resource_options.go +++ b/pkg/component/resource_options.go @@ -215,8 +215,10 @@ func BlockOnForeignController() ResourceOption { // otherwise. It deletes the object only when the object carries an owner // reference whose UID is the reconciling owner's; the reference need not be // the controller reference. Any other object is left in place with no write, -// no event, and a log line. Use it when the resource name is one a user may -// also create by hand. +// no event, and a log line. During suspension with DeleteOnSuspend, an object +// that this read finds without such an owner reference is not suspended either +// and counts as suspended. Use it when the resource name is one a user may also +// create by hand. // // The delete carries the observed UID and resourceVersion as preconditions. // An object that changes or is replaced between the read and the delete is @@ -225,7 +227,8 @@ func BlockOnForeignController() ResourceOption { // object another owner controls is kept even when it also references this // owner, and a precondition conflict does not fail the reconcile. // -// Without this option a deletion removes the object by name, whoever owns it. +// Without this option or BlockOnForeignController, a deletion removes the +// object by name, whoever owns it. // Requires a managed resource that carries the owner reference: combining it // with ReadOnly or Unowned is a configuration error returned by Build. A // cluster-scoped resource of a namespace-scoped owner never carries the diff --git a/pkg/component/suspend.go b/pkg/component/suspend.go index 08e70cd6..91a9cf75 100644 --- a/pkg/component/suspend.go +++ b/pkg/component/suspend.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "slices" "strings" "github.com/sourcehawk/operator-component-framework/pkg/component/concepts" @@ -143,7 +144,13 @@ func suspendResource( "failed to deep copy object of type %T", object, ) } - err := rec.Client.Get(ctx, client.ObjectKeyFromObject(object), existing) + // The ownership check below must not trust a cache that still shows an + // earlier owned object under the name, so it reads like observeController. + var reader client.Reader = rec.Client + if entry.Options.DeleteOnlyIfOwned && rec.APIReader != nil { + reader = rec.APIReader + } + err := reader.Get(ctx, client.ObjectKeyFromObject(object), existing) if apierrors.IsNotFound(err) { return concepts.SuspensionStatusWithReason{ Status: concepts.SuspensionStatusSuspended, @@ -155,6 +162,20 @@ func suspendResource( "failed to check existence of resource %s on suspension: %w", resource.Identity(), err, ) } + + // The suspension apply would set the owner reference on an object the + // owner does not own, and the delete after it would then find the object + // owned. Skip the object before any write, as for another owner's object. + if entry.Options.DeleteOnlyIfOwned && + !slices.ContainsFunc(existing.GetOwnerReferences(), referencesOwner(rec.Owner.GetUID())) { + log.FromContext(ctx).Info( + "skipping suspension of a resource that does not reference the owner", "resource", resource.Identity(), + ) + return concepts.SuspensionStatusWithReason{ + Status: concepts.SuspensionStatusSuspended, + Reason: fmt.Sprintf("Resource %s does not reference the owner; nothing to suspend.", resource.Identity()), + }, nil + } } // Create suspension mutation on resource (if any) diff --git a/pkg/component/suspend_test.go b/pkg/component/suspend_test.go index 8519b9ab..7c97891c 100644 --- a/pkg/component/suspend_test.go +++ b/pkg/component/suspend_test.go @@ -525,3 +525,37 @@ func TestSuspendResource_BlockOnForeignController_DeleteOnSuspend(t *testing.T) assert.True(t, apierrors.IsNotFound(cli.Get(ctx, client.ObjectKeyFromObject(obj), &v1.ConfigMap{}))) }) } + +func TestSuspendResource_DeleteOnlyIfOwned_DeleteOnSuspend(t *testing.T) { + ctx := t.Context() + scheme := setupScheme() + + t.Run("decides ownership from the API reader when the cache still shows the object owned", func(t *testing.T) { + owner := setupTestOwner() + owner.UID = "this-uid" + res := &MockSuspendableResource{} + desired := &v1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "recreated", Namespace: "default"}} + res.On("Object").Return(desired, nil) + res.On("Identity").Return("ConfigMap/recreated") + res.On("DeleteOnSuspend").Return(true) + // Suspend, Mutate and SuspensionStatus carry no expectations: reaching any of + // them means the apply was about to take over the user's object. + + cached := desired.DeepCopy() + cached.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: GroupVersion.String(), Kind: "MockOperatorCRD", Name: owner.Name, UID: owner.UID, + }} + userCreated := desired.DeepCopy() + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, cached).Build() + apiReader := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, userCreated).Build() + rec := setupReconcileContext(scheme, owner, cli) + rec.APIReader = apiReader + entry := reconcileEntry{Resource: res, Options: resourceOptions{DeleteOnlyIfOwned: true}} + + status, err := suspendResource(ctx, rec, entry, res, "test-component", testRESTMapper()) + + require.NoError(t, err) + assert.Equal(t, concepts.SuspensionStatusSuspended, status.Status) + assert.Contains(t, status.Reason, "does not reference the owner") + }) +} diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 49022831..fdf32217 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -108,9 +108,10 @@ owner claims the object between the read and the delete, the delete fails and th The option requires a managed resource. A combination with `ReadOnly()` is a build error. `DeleteOnlyIfOwned()` protects an object that the component did not create from a deletion. Without it, every deletion -path removes the object by name, whoever created it. Use the option when the resource name is one that a user can also -create by hand. For example, a component registers `-app` as a ServiceAccount behind `GatedBy()`. When the gate is -off and the user created their own `-app`, the option keeps the object of the user. +path removes the object by name, whoever created it. The only exception is an object that another owner controls, which +`BlockOnForeignController()` keeps. Use the option when the resource name is one that a user can also create by hand. +For example, a component registers `-app` as a ServiceAccount behind `GatedBy()`. When the gate is off and the +user created their own `-app`, the option keeps the object of the user. When `Delete()`, `DeleteWhen()`, `GatedBy()`, a disabled component feature gate, or a suspension with `DeleteOnSuspend()` asks for a deletion, the component first reads the live object, through `ReconcileContext.APIReader` @@ -120,8 +121,9 @@ sets this reference on each object that it applies, with two exceptions: an `Uno resource of a namespace-scoped owner. The option never deletes an object of the second kind, so do not use it there. If the object does not have the reference, the component does not write to the object and records no `ResourceDeleted` event. It logs the skip, and the reconcile does not fail. The resource does not contribute to the component condition. -As with a deletion, a disabled component feature gate reports `Disabled`. While no deletion is asked for, the option has -no effect. +As with a deletion, a disabled component feature gate reports `Disabled`. During a suspension with `DeleteOnSuspend()`, +the component also skips the suspension apply for such an object, because that apply would set the owner reference. The +resource counts as suspended. While no deletion is asked for, the option has no effect. The delete carries the observed UID and resourceVersion as preconditions. If the object changes, or a different object takes the name, between the read and the delete, the API server rejects the delete with a conflict. The component then From 79b56dc4324c2e7b9c49b5cdad0fbeda864f70df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:28:41 +0100 Subject: [PATCH 3/5] fix(component): read suspension ownership into an empty object The delete-on-suspend existence check read the live object into a copy of the desired object. The ownership check then depended on the decoder clearing ownerReferences that the copy carried from an earlier apply. The read now goes into an empty object of the same type, as observeController does. Co-Authored-By: Claude Opus 5.5 --- pkg/component/delete_owned_test.go | 21 +++++++++++++++++++++ pkg/component/suspend.go | 8 ++++---- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/pkg/component/delete_owned_test.go b/pkg/component/delete_owned_test.go index 63474c94..97ae11a0 100644 --- a/pkg/component/delete_owned_test.go +++ b/pkg/component/delete_owned_test.go @@ -187,6 +187,27 @@ var _ = Describe("DeleteOnlyIfOwned", func() { Expect(after.ResourceVersion).To(Equal(before.ResourceVersion), "a kept object must not be written") }) + It("keeps an ownerless object on suspension when the desired object still carries the owner's reference", func() { + createConfigMap(false) + before := &corev1.ConfigMap{} + Expect(k8sClient.Get(ctx, key(), before)).To(Succeed()) + // The desired object keeps the owner reference that an earlier apply set, + // while the live object under the name is a user's replacement without it. + desired := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: cmName, Namespace: namespace}} + Expect(controllerutil.SetControllerReference(owner, desired, scheme.Scheme)).To(Succeed()) + res := &MockSuspendableResource{} + res.On("Object").Return(desired, nil) + res.On("Identity").Return("ConfigMap/" + cmName) + res.On("DeleteOnSuspend").Return(true) + comp := suspendedComponent(res) + + Expect(comp.Reconcile(ctx, newTestReconcileContext(owner))).To(Succeed()) + + after := &corev1.ConfigMap{} + Expect(k8sClient.Get(ctx, key(), after)).To(Succeed()) + Expect(after.ResourceVersion).To(Equal(before.ResourceVersion), "a kept object must not be written") + }) + It("reports Disabled for a disabled component feature gate whether or not the object is kept", func() { createConfigMap(false) comp, err := disabledComponentGate(desiredResource(), DeleteOnlyIfOwned()) diff --git a/pkg/component/suspend.go b/pkg/component/suspend.go index 91a9cf75..37fb6361 100644 --- a/pkg/component/suspend.go +++ b/pkg/component/suspend.go @@ -138,10 +138,10 @@ func suspendResource( // skip Apply to avoid a create->delete churn loop on every reconcile. // This check runs before Suspend() to avoid queuing a mutation that will never be applied. if suspendable.DeleteOnSuspend() { - existing, ok := object.DeepCopyObject().(client.Object) - if !ok { + existing, err := newEmptyObjectLike(object) + if err != nil { return concepts.SuspensionStatusWithReason{}, fmt.Errorf( - "failed to deep copy object of type %T", object, + "failed to prepare existence check for resource %s on suspension: %w", resource.Identity(), err, ) } // The ownership check below must not trust a cache that still shows an @@ -150,7 +150,7 @@ func suspendResource( if entry.Options.DeleteOnlyIfOwned && rec.APIReader != nil { reader = rec.APIReader } - err := reader.Get(ctx, client.ObjectKeyFromObject(object), existing) + err = reader.Get(ctx, client.ObjectKeyFromObject(object), existing) if apierrors.IsNotFound(err) { return concepts.SuspensionStatusWithReason{ Status: concepts.SuspensionStatusSuspended, From d170fdfaa345b0dc94a751ff4d8b666e1f5ebedb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 14:48:48 +0100 Subject: [PATCH 4/5] fix(component): bind the suspension apply to the ownership read With DeleteOnlyIfOwned and DeleteOnSuspend, an object that was replaced or disowned between the ownership read and the suspension apply was taken over by the apply and then deleted. The suspension apply now carries the observed resourceVersion, so the API server rejects it when the object changed. The resource then reports Suspending, the object is not written, and the reconcile does not fail. The deleteResources doc now says that an entry left in place is not an error, which the skip paths already did. Co-Authored-By: Claude Opus 5.5 --- docs/component.md | 5 ++- pkg/component/create.go | 31 +++++++++++++-- pkg/component/delete.go | 4 +- pkg/component/delete_owned_test.go | 38 +++++++++++++++++++ pkg/component/resource_options.go | 9 +++-- pkg/component/suspend.go | 20 +++++++++- .../references/component.md | 5 ++- 7 files changed, 100 insertions(+), 12 deletions(-) diff --git a/docs/component.md b/docs/component.md index 18eae9e2..6e84ee83 100644 --- a/docs/component.md +++ b/docs/component.md @@ -126,7 +126,10 @@ resource counts as suspended. While no deletion is asked for, the option has no The delete carries the observed UID and resourceVersion as preconditions. If the object changes, or a different object takes the name, between the read and the delete, the API server rejects the delete with a conflict. The component then -keeps the object, logs the skip, and does not fail the reconcile. The next reconcile reads the object again. +keeps the object, logs the skip, and does not fail the reconcile. The next reconcile reads the object again. A +suspension with `DeleteOnSuspend()` binds its apply to the read in the same way, through the observed resourceVersion. +If the object changes between the read and that apply, the API server rejects the apply. The component does not write +the object, the resource reports `Suspending`, and the reconcile does not fail. The option composes with `BlockOnForeignController()`. With both options, the component keeps an object that another owner controls, even when the object also references this owner. A conflict on the delete then does not fail the diff --git a/pkg/component/create.go b/pkg/component/create.go index a283362a..0b305403 100644 --- a/pkg/component/create.go +++ b/pkg/component/create.go @@ -21,6 +21,24 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" ) +// applyConfig holds configuration for an applyResources call. +type applyConfig struct { + resourceVersion string +} + +// applyOption is a functional option for applyResources. +type applyOption func(*applyConfig) + +// withResourceVersion makes the apply conditional: the API server rejects it +// with a conflict when the live object no longer has resourceVersion, because it +// changed or was replaced since it was read. An empty resourceVersion applies +// unconditionally. +func withResourceVersion(resourceVersion string) applyOption { + return func(cfg *applyConfig) { + cfg.resourceVersion = resourceVersion + } +} + // applyResource applies a single resource to the cluster using Server-Side Apply and // collects its converging status. It returns the converging result (nil for static // resources with no status interface) and any error encountered. @@ -30,7 +48,7 @@ import ( // status-collection sequence. func applyResource( ctx context.Context, rec ReconcileContext, resource Resource, componentName string, - fieldOwner client.FieldOwner, mapper meta.RESTMapper, skipOwnerRef bool, + fieldOwner client.FieldOwner, mapper meta.RESTMapper, skipOwnerRef bool, resourceVersion string, ) (*reconcileResult, error) { obj, err := resource.Object() if err != nil { @@ -86,6 +104,7 @@ func applyResource( // server response (including these fields) because Mutate updates the internal // pointer and Patch writes back into the same object. clearServerFields(obj) + obj.SetResourceVersion(resourceVersion) // Re-assert the GVK. A Mutate implementation that assigns the whole struct // (*current = *desired) drops the TypeMeta set above, and SSA requires it. @@ -247,15 +266,19 @@ const fieldManagerMaxLength = 128 // declares. func applyResources( ctx context.Context, rec ReconcileContext, entries []reconcileEntry, - componentName string, mapper meta.RESTMapper, + componentName string, mapper meta.RESTMapper, opts ...applyOption, ) ([]reconcileResult, error) { + var cfg applyConfig + for _, opt := range opts { + opt(&cfg) + } fieldOwner := applyFieldOwner(rec.Owner, componentName) var results []reconcileResult for _, entry := range entries { result, err := applyResource( - ctx, rec, entry.Resource, componentName, fieldOwner, mapper, entry.Options.Unowned, + ctx, rec, entry.Resource, componentName, fieldOwner, mapper, entry.Options.Unowned, cfg.resourceVersion, ) if err != nil { return nil, err @@ -343,7 +366,7 @@ func reconcileResources( result, err = readResource(ctx, rec, resource) } else { result, err = applyResource( - ctx, rec, resource, componentName, fieldOwner, mapper, entry.Options.Unowned, + ctx, rec, resource, componentName, fieldOwner, mapper, entry.Options.Unowned, "", ) } if err != nil { diff --git a/pkg/component/delete.go b/pkg/component/delete.go index 2d974675..a8740120 100644 --- a/pkg/component/delete.go +++ b/pkg/component/delete.go @@ -49,8 +49,8 @@ func withDeletionReason(reason string) deleteOption { // For each successful deletion, a "ResourceDeleted" event is recorded on the owner object. // The opts parameter allows callers to customize the event message via functional options. // -// Returns a joined error containing all encountered errors, or nil if all deletions -// were successful or resulted in "Not Found" errors. +// Returns the errors of all entries joined, or nil when no entry failed; an +// entry left in place is not an error. func deleteResources( ctx context.Context, rec ReconcileContext, entries []reconcileEntry, opts ...deleteOption, ) error { diff --git a/pkg/component/delete_owned_test.go b/pkg/component/delete_owned_test.go index 97ae11a0..f2451085 100644 --- a/pkg/component/delete_owned_test.go +++ b/pkg/component/delete_owned_test.go @@ -12,6 +12,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" ) @@ -208,6 +209,43 @@ var _ = Describe("DeleteOnlyIfOwned", func() { Expect(after.ResourceVersion).To(Equal(before.ResourceVersion), "a kept object must not be written") }) + It("keeps an object disowned between the suspension read and the suspension apply", func() { + createConfigMap(true) + res := &MockSuspendableResource{} + res.On("Object").Return(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: cmName, Namespace: namespace}}, nil) + res.On("Identity").Return("ConfigMap/" + cmName) + res.On("DeleteOnSuspend").Return(true) + res.On("Suspend").Return(nil) + res.On("Mutate", mock.Anything).Return(nil) + res.On("SuspensionStatus").Return(concepts.SuspensionStatusWithReason{Status: concepts.SuspensionStatusSuspended}, nil) + comp := suspendedComponent(res) + + base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme.Scheme}) + Expect(err).NotTo(HaveOccurred()) + disownBeforeApply := interceptor.NewClient(base, interceptor.Funcs{ + Patch: func( + ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption, + ) error { + if obj.GetName() == cmName { + live := &corev1.ConfigMap{} + Expect(c.Get(ctx, key(), live)).To(Succeed()) + live.OwnerReferences = nil + Expect(c.Update(ctx, live)).To(Succeed()) + } + return c.Patch(ctx, obj, patch, opts...) + }, + }) + rec := newTestReconcileContext(owner) + rec.Client = disownBeforeApply + + Expect(comp.Reconcile(ctx, rec)).To(Succeed()) + + after := &corev1.ConfigMap{} + Expect(k8sClient.Get(ctx, key(), after)).To(Succeed()) + Expect(after.OwnerReferences).To(BeEmpty(), "the suspension apply must not take over the disowned object") + Expect(comp.GetCondition(owner).Reason).To(Equal(string(Suspending))) + }) + It("reports Disabled for a disabled component feature gate whether or not the object is kept", func() { createConfigMap(false) comp, err := disabledComponentGate(desiredResource(), DeleteOnlyIfOwned()) diff --git a/pkg/component/resource_options.go b/pkg/component/resource_options.go index 257ddaf4..c6f11436 100644 --- a/pkg/component/resource_options.go +++ b/pkg/component/resource_options.go @@ -223,9 +223,12 @@ func BlockOnForeignController() ResourceOption { // The delete carries the observed UID and resourceVersion as preconditions. // An object that changes or is replaced between the read and the delete is // left in place and logged, and the reconcile does not fail; the next -// reconcile reads the object again. Combined with BlockOnForeignController, an -// object another owner controls is kept even when it also references this -// owner, and a precondition conflict does not fail the reconcile. +// reconcile reads the object again. When the object changes between the read +// and the suspension apply with DeleteOnSuspend, the resource reports +// Suspending and is not written, and the reconcile does not fail. Combined +// with BlockOnForeignController, an object another owner controls is kept +// even when it also references this owner, and a precondition conflict does +// not fail the reconcile. // // Without this option or BlockOnForeignController, a deletion removes the // object by name, whoever owns it. diff --git a/pkg/component/suspend.go b/pkg/component/suspend.go index 37fb6361..afd69018 100644 --- a/pkg/component/suspend.go +++ b/pkg/component/suspend.go @@ -134,6 +134,8 @@ func suspendResource( } } + var applyOpts []applyOption + // Short-circuit: if the resource should be deleted on suspend and already doesn't exist, // skip Apply to avoid a create->delete churn loop on every reconcile. // This check runs before Suspend() to avoid queuing a mutation that will never be applied. @@ -176,6 +178,13 @@ func suspendResource( Reason: fmt.Sprintf("Resource %s does not reference the owner; nothing to suspend.", resource.Identity()), }, nil } + + // Binds the apply to the object the check above saw. An object replaced + // or disowned in between would otherwise be taken over by the apply and + // then found owned by the delete. + if entry.Options.DeleteOnlyIfOwned { + applyOpts = append(applyOpts, withResourceVersion(existing.GetResourceVersion())) + } } // Create suspension mutation on resource (if any) @@ -184,7 +193,16 @@ func suspendResource( } // Apply suspension mutation (if any) - _, err = applyResources(ctx, rec, []reconcileEntry{entry}, componentName, mapper) + _, err = applyResources(ctx, rec, []reconcileEntry{entry}, componentName, mapper, applyOpts...) + if entry.Options.DeleteOnlyIfOwned && apierrors.IsConflict(err) { + log.FromContext(ctx).Info( + "skipping suspension of a resource that changed after it was read", "resource", resource.Identity(), + ) + return concepts.SuspensionStatusWithReason{ + Status: concepts.SuspensionStatusSuspending, + Reason: fmt.Sprintf("Resource %s changed after it was read; suspension retries.", resource.Identity()), + }, nil + } if err != nil { return concepts.SuspensionStatusWithReason{}, fmt.Errorf( "failed to create or update resource %s on suspension: %w", resource.Identity(), err, diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index fdf32217..75ff2b26 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -127,7 +127,10 @@ resource counts as suspended. While no deletion is asked for, the option has no The delete carries the observed UID and resourceVersion as preconditions. If the object changes, or a different object takes the name, between the read and the delete, the API server rejects the delete with a conflict. The component then -keeps the object, logs the skip, and does not fail the reconcile. The next reconcile reads the object again. +keeps the object, logs the skip, and does not fail the reconcile. The next reconcile reads the object again. A +suspension with `DeleteOnSuspend()` binds its apply to the read in the same way, through the observed resourceVersion. +If the object changes between the read and that apply, the API server rejects the apply. The component does not write +the object, the resource reports `Suspending`, and the reconcile does not fail. The option composes with `BlockOnForeignController()`. With both options, the component keeps an object that another owner controls, even when the object also references this owner. A conflict on the delete then does not fail the From 5742eeb12fb2895d49d65dd6284aec15058d199e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Fri, 16 Oct 2026 16:25:07 +0100 Subject: [PATCH 5/5] test(component): cover a delete conflict with both ownership options Add a test for a precondition conflict on the delete when DeleteOnlyIfOwned and BlockOnForeignController are both set. The reconcile does not fail and the object is kept. BlockOnForeignController alone still fails on that conflict, so the combined case needs its own test. Move the owner reference check of the delete and suspension paths into one ownedBy helper. Split the DeleteOnlyIfOwned GoDoc into paragraphs, and give the suspension apply comment a subject. Co-Authored-By: Claude Opus 5.5 --- pkg/component/delete.go | 3 +-- pkg/component/delete_test.go | 17 +++++++++++++++++ pkg/component/orphan.go | 6 ++++++ pkg/component/resource_options.go | 1 + pkg/component/suspend.go | 10 ++++------ 5 files changed, 29 insertions(+), 8 deletions(-) diff --git a/pkg/component/delete.go b/pkg/component/delete.go index a8740120..03553d3e 100644 --- a/pkg/component/delete.go +++ b/pkg/component/delete.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "slices" v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -133,7 +132,7 @@ func deleteEntry( return false, nil } - if options.DeleteOnlyIfOwned && !slices.ContainsFunc(live.GetOwnerReferences(), referencesOwner(rec.Owner.GetUID())) { + if options.DeleteOnlyIfOwned && !ownedBy(live, rec.Owner.GetUID()) { log.FromContext(ctx).Info( "skipping deletion of a resource that does not reference the owner", "resource", resource.Identity(), ) diff --git a/pkg/component/delete_test.go b/pkg/component/delete_test.go index d4b25b2a..26a28446 100644 --- a/pkg/component/delete_test.go +++ b/pkg/component/delete_test.go @@ -467,6 +467,23 @@ func TestDeleteResources_DeleteOnlyIfOwned(t *testing.T) { assert.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(obj), &corev1.ConfigMap{})) }) + + t.Run("keeps an object another owner claims after the read without failing when combined with BlockOnForeignController", func(t *testing.T) { + owner := newOwner() + res, obj := newResource("claimed-combined", ownerRef("owner", "this-uid")) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj.DeepCopy()). + WithInterceptorFuncs(claimBeforeDelete(t, "claimed-combined")).Build() + entries := []reconcileEntry{{ + Resource: res, + Options: resourceOptions{Delete: true, DeleteOnlyIfOwned: true, BlockOnForeignController: true}, + }} + + require.NoError(t, deleteResources(ctx, setupReconcileContext(scheme, owner, cli), entries)) + + got := &corev1.ConfigMap{} + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(obj), got)) + assert.Equal(t, "other-uid", string(got.OwnerReferences[0].UID)) + }) } // disownBeforeDelete returns interceptor funcs that remove every owner diff --git a/pkg/component/orphan.go b/pkg/component/orphan.go index 8813789c..5e755b13 100644 --- a/pkg/component/orphan.go +++ b/pkg/component/orphan.go @@ -81,6 +81,12 @@ func orphanResources(ctx context.Context, rec ReconcileContext, resources []Reso return errors.Join(errs...) } +// ownedBy reports whether obj carries an owner reference to the owner with +// ownerUID, whether or not it is the controller reference. +func ownedBy(obj client.Object, ownerUID types.UID) bool { + return slices.ContainsFunc(obj.GetOwnerReferences(), referencesOwner(ownerUID)) +} + // referencesOwner matches an owner reference to the owner with ownerUID, // whether or not it is the controller reference. func referencesOwner(ownerUID types.UID) func(metav1.OwnerReference) bool { diff --git a/pkg/component/resource_options.go b/pkg/component/resource_options.go index c6f11436..e2d59b7b 100644 --- a/pkg/component/resource_options.go +++ b/pkg/component/resource_options.go @@ -232,6 +232,7 @@ func BlockOnForeignController() ResourceOption { // // Without this option or BlockOnForeignController, a deletion removes the // object by name, whoever owns it. +// // Requires a managed resource that carries the owner reference: combining it // with ReadOnly or Unowned is a configuration error returned by Build. A // cluster-scoped resource of a namespace-scoped owner never carries the diff --git a/pkg/component/suspend.go b/pkg/component/suspend.go index afd69018..94d927cc 100644 --- a/pkg/component/suspend.go +++ b/pkg/component/suspend.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "slices" "strings" "github.com/sourcehawk/operator-component-framework/pkg/component/concepts" @@ -168,8 +167,7 @@ func suspendResource( // The suspension apply would set the owner reference on an object the // owner does not own, and the delete after it would then find the object // owned. Skip the object before any write, as for another owner's object. - if entry.Options.DeleteOnlyIfOwned && - !slices.ContainsFunc(existing.GetOwnerReferences(), referencesOwner(rec.Owner.GetUID())) { + if entry.Options.DeleteOnlyIfOwned && !ownedBy(existing, rec.Owner.GetUID()) { log.FromContext(ctx).Info( "skipping suspension of a resource that does not reference the owner", "resource", resource.Identity(), ) @@ -179,9 +177,9 @@ func suspendResource( }, nil } - // Binds the apply to the object the check above saw. An object replaced - // or disowned in between would otherwise be taken over by the apply and - // then found owned by the delete. + // The apply carries the resourceVersion the check above saw. An object + // replaced or disowned in between would otherwise be taken over by the + // apply and then found owned by the delete. if entry.Options.DeleteOnlyIfOwned { applyOpts = append(applyOpts, withResourceVersion(existing.GetResourceVersion())) }