diff --git a/docs/component.md b/docs/component.md index e6af17b4..6e84ee83 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,36 @@ 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. 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` +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`. 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 +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 +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/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 4005b9d7..03553d3e 100644 --- a/pkg/component/delete.go +++ b/pkg/component/delete.go @@ -42,12 +42,14 @@ 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. // -// 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 { @@ -92,22 +94,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 +123,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 +132,35 @@ func deleteEntry( return false, nil } + 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(), + ) + 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..f2451085 --- /dev/null +++ b/pkg/component/delete_owned_test.go @@ -0,0 +1,258 @@ +package component + +import ( + "context" + + . "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" + "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" +) + +// 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)), + ) + + 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("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("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()) + 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..26a28446 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,211 @@ 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{})) + }) + + 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 +// 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..5e755b13 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,15 @@ 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 { + 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..e2d59b7b 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,40 @@ 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. 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 +// left in place and logged, and the reconcile does not fail; the next +// 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. +// +// 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 +282,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 +367,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..94d927cc 100644 --- a/pkg/component/suspend.go +++ b/pkg/component/suspend.go @@ -133,17 +133,25 @@ 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. 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, ) } - 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 +163,26 @@ 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 && !ownedBy(existing, 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 + } + + // 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())) + } } // Create suspension mutation on resource (if any) @@ -163,7 +191,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, @@ -184,8 +221,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/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/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..75ff2b26 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,36 @@ 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. 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` +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`. 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 +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 +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.