From efec17d7b3a115f5adbaefe7eb8fcdc13583d538 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Wed, 26 Aug 2026 01:59:04 +0200 Subject: [PATCH 1/8] feat(component): block a resource whose live object is controlled by another owner (#199) Add the BlockOnForeignController resource option. Before each apply the component reads the live object and, when it carries a controller reference to an owner other than the reconciling one, records Blocked with "controlled by " and performs no apply, like any blocked guard. During suspension such a resource is neither scaled down nor deleted and reports Suspended with the same reason. Combining the option with ReadOnly is a build error. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01MZRpj8ChzesLeqLkWT31vS --- docs/component.md | 20 ++- docs/primitives.md | 11 +- pkg/component/create.go | 60 ++++++++ .../create_foreign_controller_test.go | 138 ++++++++++++++++++ pkg/component/resource_options.go | 41 ++++++ pkg/component/resource_options_test.go | 15 ++ pkg/component/suspend.go | 17 +++ pkg/component/suspend_test.go | 71 +++++++++ .../references/component.md | 20 ++- .../using-primitives/references/primitives.md | 11 +- 10 files changed, 394 insertions(+), 10 deletions(-) create mode 100644 pkg/component/create_foreign_controller_test.go diff --git a/docs/component.md b/docs/component.md index 4d1cfcd3..49fb1942 100644 --- a/docs/component.md +++ b/docs/component.md @@ -65,6 +65,7 @@ be passed without a guard. | `component.Auxiliary()` | The resource's health does not contribute to the component condition (a blocked guard still does) | | `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 naming the owner whose controller reference is on the live object, performs no apply, and short-circuits the remaining resources | | `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 @@ -79,6 +80,18 @@ is still subject to explicit deletion: `Delete()`, `DeleteWhen()`, `GatedBy()` ( suspension with `DeleteOnSuspend()` all delete it directly, regardless of the `Unowned` flag. Only Kubernetes GC (triggered by owner CR deletion) is suppressed. +`BlockOnForeignController()` guards a managed resource against an object that another owner already controls. Before +each apply the component reads the live object; when it exists and carries a controller owner reference whose UID is not +the reconciling owner's, the resource reports `Blocked` with the message `controlled by `, no apply is +performed, and the resources after it are skipped, exactly as for a [blocked guard](#guards). The block clears on the +reconcile after that reference is gone. Reach for it wherever two custom resources may name one object: with the default +controller reference it replaces the API server's rejection of a second controller with a readable condition, and with +`Unowned()` it stops the second owner's forced apply from taking the object's fields at all (see +[Server-Side Apply](primitives.md#server-side-apply)). An object with no controller reference is never blocked, so +contention between two owners that both apply without one is not detected. Unlike a custom guard, the check also runs +during suspension: a resource another owner controls is neither scaled down nor deleted, and reports `Suspended` with +the same reason. It requires a managed resource; combining it with `ReadOnly()` is a build error. + Options compose. Gate a resource and exclude it from health aggregation in one call: ```go @@ -1121,8 +1134,13 @@ registered custom guard; it does not affect declared data guards. regardless of its participation mode, and all resources after it are skipped entirely. This override exists because a blocked guard halts the entire pipeline; subsequent required resources would otherwise be silently absent from health aggregation. +- After a resource's own guard clears, a managed resource registered with + [`BlockOnForeignController()`](#resource-registration-options) is also checked against the live object's controller + reference; another owner's reference records `Blocked` the same way, with the message `controlled by `. - On the next reconcile, if the guard clears (`Unblocked`), the resource is applied normally. -- Guards are **not** evaluated during suspension. The suspension path always proceeds regardless of guard state. +- Guards are **not** evaluated during suspension. The suspension path always proceeds regardless of guard state. The + exception is [`BlockOnForeignController()`](#resource-registration-options), which is checked on every path so that a + suspension never scales down or deletes an object another owner controls. - A guard evaluation error is treated as a reconciliation failure and sets the condition to `Error`. A blocked guard produces a condition like: diff --git a/docs/primitives.md b/docs/primitives.md index 87f0e672..fd5a2cc8 100644 --- a/docs/primitives.md +++ b/docs/primitives.md @@ -149,10 +149,13 @@ converged, and neither would ever see a conflict. Naming the owner also makes The rejection depends on the controller reference. For a resource registered with `Unowned()`, or one whose owner reference cannot be set because of a scope mismatch, nothing stops the second owner's forced apply from taking the -fields it declares, and the fields move between the two owners' managers on every reconcile. `managedFields` then names -the owner that wrote each field, but the framework does not detect the contention. A shared name between two owners is -the operator's responsibility in that case; a [guard](component.md#guards) that reads the live object and blocks when -another owner controls it is the way to make it explicit. +fields it declares, and the fields move between the two owners' managers on every reconcile. Register the resource with +[`component.BlockOnForeignController()`](component.md#resource-registration-options) to make the contention explicit: +before each apply the component reads the live object and, when another owner's controller reference is on it, reports +`Blocked` naming that owner instead of applying. This also turns the rejection in the default case into a readable +condition. The check compares controller references, so between two owners that both apply without one (two `Unowned()` +registrations, or owners the object's scope keeps from being referenced) nothing on the object carries either identity, +the fields keep moving between the two managers, and a shared name remains the operator's responsibility. !!! note "Upgrading from a release without the UID in the manager name" diff --git a/pkg/component/create.go b/pkg/component/create.go index 706d065d..c000fa7b 100644 --- a/pkg/component/create.go +++ b/pkg/component/create.go @@ -14,6 +14,7 @@ import ( "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -316,6 +317,25 @@ func reconcileResources( } } + // A managed resource that another owner controls is blocked before the + // apply, so the forced apply never takes that owner's fields. + if entry.Options.BlockOnForeignController && !entry.Options.ReadOnly { + controller, err := foreignController(ctx, rec, resource) + if err != nil { + return nil, err + } + if controller != nil { + results = append(results, reconcileResult{ + Entry: entry, + Status: convergingStatusWithReason{ + Status: convergingStatusGuardBlocked, + Reason: foreignControllerReason(controller), + }, + }) + return results, nil + } + } + // Process the resource based on its mode var result *reconcileResult var err error @@ -360,6 +380,46 @@ func reconcileResources( return results, nil } +// foreignController reads the live object of resource through the client and +// returns its controller owner reference when that reference points at an +// owner other than rec.Owner. It returns nil when the object does not exist, +// has no controller reference, or is controlled by rec.Owner. +func foreignController( + ctx context.Context, rec ReconcileContext, resource Resource, +) (*metav1.OwnerReference, error) { + obj, err := resource.Object() + if err != nil { + return nil, fmt.Errorf( + "failed to retrieve object for resource %s: %w", resource.Identity(), err, + ) + } + live, err := newEmptyObjectLike(obj) + if err != nil { + return nil, fmt.Errorf( + "failed to prepare controller check for resource %s: %w", resource.Identity(), err, + ) + } + if err := rec.Client.Get(ctx, client.ObjectKeyFromObject(obj), live); err != nil { + if apierrors.IsNotFound(err) { + return nil, nil + } + return nil, fmt.Errorf( + "failed to read resource %s for controller check: %w", resource.Identity(), err, + ) + } + controller := metav1.GetControllerOf(live) + if controller == nil || controller.UID == rec.Owner.GetUID() { + return nil, nil + } + return controller, nil +} + +// foreignControllerReason is the blocked reason for an object controlled by +// another owner, for example "controlled by DatabaseServer primary". +func foreignControllerReason(controller *metav1.OwnerReference) string { + return fmt.Sprintf("controlled by %s %s", controller.Kind, controller.Name) +} + // mutateResource applies all desired-state mutations and sets the controller owner // reference. When skipOwnerRef is true the owner reference is intentionally omitted; // the resource is not garbage-collected when the owner CR is deleted. diff --git a/pkg/component/create_foreign_controller_test.go b/pkg/component/create_foreign_controller_test.go new file mode 100644 index 00000000..f2f0736e --- /dev/null +++ b/pkg/component/create_foreign_controller_test.go @@ -0,0 +1,138 @@ +package component + +import ( + "context" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/stretchr/testify/mock" + corev1 "k8s.io/api/core/v1" + 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 BlockOnForeignController reports Blocked, naming +// the controlling owner, instead of applying over an object that another owner +// controls (sourcehawk/operator-component-framework#199). +var _ = Describe("BlockOnForeignController", func() { + var ( + ctx = context.Background() + namespace string + ownerA *MockOperatorCRD + ownerB *MockOperatorCRD + ) + + const cmName = "shared-cm" + key := func() client.ObjectKey { return client.ObjectKey{Name: cmName, Namespace: namespace} } + + sharedConfigMapComponent := func(owner *MockOperatorCRD, opts resourceOptions) *Component { + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: cmName, Namespace: namespace}, + Data: map[string]string{"owner": owner.Name}, + } + res := &MockResource{} + res.On("Object").Return(cm, nil) + res.On("Identity").Return("ConfigMap/" + cmName) + res.On("Mutate", mock.Anything).Return(nil) + return &Component{ + name: "shared", + conditionType: "SharedReady", + reconcileResources: []reconcileEntry{{Resource: res, Options: opts}}, + } + } + + liveConfigMap := func() *corev1.ConfigMap { + GinkgoHelper() + cm := &corev1.ConfigMap{} + Expect(k8sClient.Get(ctx, key(), cm)).To(Succeed()) + return cm + } + + BeforeEach(func() { + namespace = createNamespace(ctx, "foreign-controller-test-") + ownerA = &MockOperatorCRD{ObjectMeta: metav1.ObjectMeta{Name: "owner-a", Namespace: namespace}} + ownerB = &MockOperatorCRD{ObjectMeta: metav1.ObjectMeta{Name: "owner-b", Namespace: namespace}} + Expect(k8sClient.Create(ctx, ownerA)).To(Succeed()) + Expect(k8sClient.Create(ctx, ownerB)).To(Succeed()) + }) + + AfterEach(func() { + Expect(k8sClient.Delete(ctx, ownerA)).To(Succeed()) + Expect(k8sClient.Delete(ctx, ownerB)).To(Succeed()) + }) + + It("blocks the second owner and leaves the first owner's object untouched", func() { + Expect(sharedConfigMapComponent(ownerA, resourceOptions{}).Reconcile(ctx, newTestReconcileContext(ownerA))).To(Succeed()) + before := liveConfigMap() + + comp := sharedConfigMapComponent(ownerB, resourceOptions{BlockOnForeignController: true}) + Expect(comp.Reconcile(ctx, newTestReconcileContext(ownerB))).To(Succeed()) + + cond := comp.GetCondition(ownerB) + Expect(cond.Status).To(Equal(metav1.ConditionFalse)) + Expect(cond.Reason).To(Equal(string(GuardBlocked))) + Expect(cond.Message).To(Equal("controlled by MockOperatorCRD owner-a")) + + after := liveConfigMap() + Expect(after.ResourceVersion).To(Equal(before.ResourceVersion), "the blocked owner must not write the object") + Expect(after.Data).To(HaveKeyWithValue("owner", ownerA.Name)) + Expect(after.OwnerReferences).To(Equal(before.OwnerReferences)) + Expect(after.ManagedFields).To(Equal(before.ManagedFields)) + }) + + It("blocks an unowned registration when another owner's controller reference is on the object", func() { + cm := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{Name: cmName, Namespace: namespace}, + Data: map[string]string{"owner": ownerA.Name}, + } + Expect(controllerutil.SetControllerReference(ownerA, cm, scheme.Scheme)).To(Succeed()) + Expect(k8sClient.Create(ctx, cm)).To(Succeed()) + Expect(sharedConfigMapComponent(ownerA, resourceOptions{Unowned: true}).Reconcile(ctx, newTestReconcileContext(ownerA))).To(Succeed()) + before := liveConfigMap() + + comp := sharedConfigMapComponent(ownerB, resourceOptions{Unowned: true, BlockOnForeignController: true}) + Expect(comp.Reconcile(ctx, newTestReconcileContext(ownerB))).To(Succeed()) + Expect(comp.GetCondition(ownerB).Reason).To(Equal(string(GuardBlocked))) + Expect(liveConfigMap().ResourceVersion).To(Equal(before.ResourceVersion)) + }) + + It("does not block an unowned registration when the object has no controller", func() { + Expect(sharedConfigMapComponent(ownerA, resourceOptions{Unowned: true}).Reconcile(ctx, newTestReconcileContext(ownerA))).To(Succeed()) + + comp := sharedConfigMapComponent(ownerB, resourceOptions{Unowned: true, BlockOnForeignController: true}) + Expect(comp.Reconcile(ctx, newTestReconcileContext(ownerB))).To(Succeed()) + Expect(comp.GetCondition(ownerB).Reason).NotTo(Equal(string(GuardBlocked))) + Expect(liveConfigMap().Data).To(HaveKeyWithValue("owner", ownerB.Name)) + }) + + It("does not block the owner that controls the object", func() { + comp := sharedConfigMapComponent(ownerA, resourceOptions{BlockOnForeignController: true}) + rec := newTestReconcileContext(ownerA) + Expect(comp.Reconcile(ctx, rec)).To(Succeed()) + Expect(comp.Reconcile(ctx, rec)).To(Succeed()) + Expect(comp.GetCondition(ownerA).Reason).NotTo(Equal(string(GuardBlocked))) + Expect(liveConfigMap().OwnerReferences[0].UID).To(Equal(ownerA.UID)) + }) + + It("unblocks once the controlling owner's reference is gone", func() { + Expect(sharedConfigMapComponent(ownerA, resourceOptions{}).Reconcile(ctx, newTestReconcileContext(ownerA))).To(Succeed()) + + comp := sharedConfigMapComponent(ownerB, resourceOptions{BlockOnForeignController: true}) + rec := newTestReconcileContext(ownerB) + Expect(comp.Reconcile(ctx, rec)).To(Succeed()) + Expect(comp.GetCondition(ownerB).Reason).To(Equal(string(GuardBlocked))) + + released := liveConfigMap() + released.OwnerReferences = nil + Expect(k8sClient.Update(ctx, released)).To(Succeed()) + + Expect(comp.Reconcile(ctx, rec)).To(Succeed()) + Expect(comp.GetCondition(ownerB).Reason).NotTo(Equal(string(GuardBlocked))) + after := liveConfigMap() + Expect(after.Data).To(HaveKeyWithValue("owner", ownerB.Name)) + Expect(after.OwnerReferences).To(HaveLen(1)) + Expect(after.OwnerReferences[0].UID).To(Equal(ownerB.UID)) + }) +}) diff --git a/pkg/component/resource_options.go b/pkg/component/resource_options.go index 3e114f9b..e03f28c0 100644 --- a/pkg/component/resource_options.go +++ b/pkg/component/resource_options.go @@ -33,6 +33,7 @@ type resourceConfig struct { participationMode ParticipationMode blockOnAbsence bool ignoreIfAbsent bool + blockOnForeignController bool suppressGraceInconsistencyWarning bool } @@ -71,6 +72,13 @@ type resourceOptions struct { // reconciliation of subsequent resources continues. Last-known state is // preserved across an absence. Mutually exclusive with BlockOnAbsence. IgnoreIfAbsent bool + // BlockOnForeignController applies to managed resources. When true, the + // live object is read before every apply and, when it carries a controller + // reference to an owner other than the reconciling one, the resource records + // a blocked status naming that owner and no apply is performed. During + // suspension such a resource reports Suspended without being applied or + // deleted. Mutually exclusive with ReadOnly. + BlockOnForeignController bool } // ReadOnly marks the resource as read-only: the component fetches its current @@ -158,6 +166,32 @@ func Unowned() ResourceOption { return func(c *resourceConfig) { c.unowned = true } } +// BlockOnForeignController blocks the resource while the live object is +// controlled by another owner. Before every apply the component reads the live +// object; when it exists and carries a controller owner reference whose UID is +// not the reconciling owner's, the resource records a blocked status with a +// reason naming the controlling owner ("controlled by ") and, like +// any blocked guard, stops the resources after it. The block clears on the +// reconcile after that reference is gone. An object with no controller +// reference is never blocked, so contention between two owners that both apply +// without one (two Unowned registrations, or owners the object's scope keeps +// from being referenced) is not detected. +// +// Use it wherever two custom resources may name one object: with the default +// controller reference it turns the API server's rejection of a second +// controller into a readable condition, and with Unowned it stops the second +// owner's forced apply from taking the object's fields at all. +// +// During suspension the resource is not applied or deleted while another owner +// controls it; it reports Suspended with the same reason, since the component +// holds nothing there to suspend. +// +// Requires a managed resource: combining it with ReadOnly is a configuration +// error returned by Build. +func BlockOnForeignController() ResourceOption { + return func(c *resourceConfig) { c.blockOnForeignController = 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. @@ -192,6 +226,12 @@ func (c *resourceConfig) resolve() (resourceOptions, error) { if c.ignoreIfAbsent && !c.readOnly { return resourceOptions{}, errors.New("resource option IgnoreIfAbsent requires ReadOnly") } + if c.blockOnForeignController && c.readOnly { + return resourceOptions{}, errors.New( + "resource option BlockOnForeignController is mutually exclusive with ReadOnly; " + + "a read-only resource is never applied", + ) + } // 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 @@ -264,5 +304,6 @@ func (c *resourceConfig) resolve() (resourceOptions, error) { SuppressGraceInconsistencyWarning: c.suppressGraceInconsistencyWarning, BlockOnAbsence: c.blockOnAbsence, IgnoreIfAbsent: c.ignoreIfAbsent, + BlockOnForeignController: c.blockOnForeignController, }, nil } diff --git a/pkg/component/resource_options_test.go b/pkg/component/resource_options_test.go index f6fc9c35..162c8199 100644 --- a/pkg/component/resource_options_test.go +++ b/pkg/component/resource_options_test.go @@ -104,6 +104,16 @@ func TestResolveResourceOptions(t *testing.T) { opts: []ResourceOption{GatedBy(&disabledFeature{}), Auxiliary()}, want: resourceOptions{Delete: true, ParticipationMode: ParticipationModeAuxiliary}, }, + { + name: "BlockOnForeignController sets flag", + opts: []ResourceOption{BlockOnForeignController()}, + want: resourceOptions{BlockOnForeignController: true, ParticipationMode: ParticipationModeRequired}, + }, + { + name: "BlockOnForeignController alongside Unowned", + opts: []ResourceOption{Unowned(), BlockOnForeignController()}, + want: resourceOptions{Unowned: true, BlockOnForeignController: true, ParticipationMode: ParticipationModeRequired}, + }, { name: "ReadOnly sets flag", opts: []ResourceOption{ReadOnly()}, @@ -206,6 +216,11 @@ func TestResolveResourceOptions_ValidationErrors(t *testing.T) { opts: []ResourceOption{IgnoreIfAbsent()}, wantErrIs: "IgnoreIfAbsent requires ReadOnly", }, + { + name: "BlockOnForeignController with ReadOnly errors", + opts: []ResourceOption{ReadOnly(), BlockOnForeignController()}, + wantErrIs: "BlockOnForeignController is mutually exclusive with ReadOnly", + }, { name: "BlockOnAbsence without ReadOnly errors", opts: []ResourceOption{BlockOnAbsence()}, diff --git a/pkg/component/suspend.go b/pkg/component/suspend.go index 57346c45..3bd8eda5 100644 --- a/pkg/component/suspend.go +++ b/pkg/component/suspend.go @@ -109,6 +109,23 @@ func suspendResource( return concepts.SuspensionStatusWithReason{}, fmt.Errorf("failed to get object on suspension: %w", err) } + // An object another owner controls is not this component's to scale down or + // delete. Report it suspended, since the component holds nothing there. + if entry.Options.BlockOnForeignController { + controller, err := foreignController(ctx, rec, resource) + if err != nil { + return concepts.SuspensionStatusWithReason{}, err + } + if controller != nil { + return concepts.SuspensionStatusWithReason{ + Status: concepts.SuspensionStatusSuspended, + Reason: fmt.Sprintf( + "Resource %s is %s; nothing to suspend.", resource.Identity(), foreignControllerReason(controller), + ), + }, nil + } + } + // 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. diff --git a/pkg/component/suspend_test.go b/pkg/component/suspend_test.go index ad73ed19..92aa59ab 100644 --- a/pkg/component/suspend_test.go +++ b/pkg/component/suspend_test.go @@ -13,6 +13,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" ) @@ -419,3 +420,73 @@ func TestSuspendResource(t *testing.T) { assert.Contains(t, err.Error(), "network error") }) } + +func TestSuspendResource_BlockOnForeignController(t *testing.T) { + ctx := t.Context() + scheme := setupScheme() + foreign := metav1.OwnerReference{ + APIVersion: "test/v1", Kind: "MockOperatorCRD", Name: "other-owner", UID: "other-uid", + Controller: ptr.To(true), + } + + t.Run("reports Suspended without touching an object another owner controls", func(t *testing.T) { + owner := setupTestOwner() + owner.UID = "this-uid" + res := &MockSuspendableResource{} + desired := &v1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "cm", Namespace: "default"}} + res.On("Object").Return(desired, nil) + res.On("Identity").Return("ConfigMap/cm") + res.On("DeleteOnSuspend").Return(true) + // Suspend, Mutate and SuspensionStatus carry no expectations: reaching any of + // them means the foreign object was about to be applied or deleted. + + live := desired.DeepCopy() + live.OwnerReferences = []metav1.OwnerReference{foreign} + live.Data = map[string]string{"owner": "other-owner"} + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, live).Build() + rec := setupReconcileContext(scheme, owner, cli) + entry := reconcileEntry{Resource: res, Options: resourceOptions{BlockOnForeignController: 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, "controlled by MockOperatorCRD other-owner") + + got := &v1.ConfigMap{} + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(desired), got)) + assert.Equal(t, live.Data, got.Data) + assert.Equal(t, live.OwnerReferences, got.OwnerReferences) + }) + + t.Run("suspends an object this owner controls", func(t *testing.T) { + owner := setupTestOwner() + owner.UID = "this-uid" + res, obj := setupMockResource("cm", concepts.SuspensionStatusSuspended, "Done", false) + obj.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: GroupVersion.String(), Kind: "MockOperatorCRD", Name: owner.Name, UID: owner.UID, Controller: ptr.To(true), + }} + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj).Build() + rec := setupReconcileContext(scheme, owner, cli) + entry := reconcileEntry{Resource: res, Options: resourceOptions{BlockOnForeignController: true}} + + status, err := suspendResource(ctx, rec, entry, res, "test-component", testRESTMapper()) + require.NoError(t, err) + assert.Equal(t, concepts.SuspensionStatusSuspended, status.Status) + assert.Equal(t, "Done", status.Reason) + res.AssertCalled(t, "Suspend") + }) + + t.Run("suspends an object with no controller", func(t *testing.T) { + owner := setupTestOwner() + owner.UID = "this-uid" + res, obj := setupMockResource("cm", concepts.SuspensionStatusSuspended, "Done", false) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj).Build() + rec := setupReconcileContext(scheme, owner, cli) + entry := reconcileEntry{Resource: res, Options: resourceOptions{BlockOnForeignController: true}} + + status, err := suspendResource(ctx, rec, entry, res, "test-component", testRESTMapper()) + require.NoError(t, err) + assert.Equal(t, concepts.SuspensionStatusSuspended, status.Status) + res.AssertCalled(t, "Suspend") + }) +} diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 4d1cfcd3..49fb1942 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -65,6 +65,7 @@ be passed without a guard. | `component.Auxiliary()` | The resource's health does not contribute to the component condition (a blocked guard still does) | | `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 naming the owner whose controller reference is on the live object, performs no apply, and short-circuits the remaining resources | | `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 @@ -79,6 +80,18 @@ is still subject to explicit deletion: `Delete()`, `DeleteWhen()`, `GatedBy()` ( suspension with `DeleteOnSuspend()` all delete it directly, regardless of the `Unowned` flag. Only Kubernetes GC (triggered by owner CR deletion) is suppressed. +`BlockOnForeignController()` guards a managed resource against an object that another owner already controls. Before +each apply the component reads the live object; when it exists and carries a controller owner reference whose UID is not +the reconciling owner's, the resource reports `Blocked` with the message `controlled by `, no apply is +performed, and the resources after it are skipped, exactly as for a [blocked guard](#guards). The block clears on the +reconcile after that reference is gone. Reach for it wherever two custom resources may name one object: with the default +controller reference it replaces the API server's rejection of a second controller with a readable condition, and with +`Unowned()` it stops the second owner's forced apply from taking the object's fields at all (see +[Server-Side Apply](primitives.md#server-side-apply)). An object with no controller reference is never blocked, so +contention between two owners that both apply without one is not detected. Unlike a custom guard, the check also runs +during suspension: a resource another owner controls is neither scaled down nor deleted, and reports `Suspended` with +the same reason. It requires a managed resource; combining it with `ReadOnly()` is a build error. + Options compose. Gate a resource and exclude it from health aggregation in one call: ```go @@ -1121,8 +1134,13 @@ registered custom guard; it does not affect declared data guards. regardless of its participation mode, and all resources after it are skipped entirely. This override exists because a blocked guard halts the entire pipeline; subsequent required resources would otherwise be silently absent from health aggregation. +- After a resource's own guard clears, a managed resource registered with + [`BlockOnForeignController()`](#resource-registration-options) is also checked against the live object's controller + reference; another owner's reference records `Blocked` the same way, with the message `controlled by `. - On the next reconcile, if the guard clears (`Unblocked`), the resource is applied normally. -- Guards are **not** evaluated during suspension. The suspension path always proceeds regardless of guard state. +- Guards are **not** evaluated during suspension. The suspension path always proceeds regardless of guard state. The + exception is [`BlockOnForeignController()`](#resource-registration-options), which is checked on every path so that a + suspension never scales down or deletes an object another owner controls. - A guard evaluation error is treated as a reconciliation failure and sets the condition to `Error`. A blocked guard produces a condition like: diff --git a/plugin/skills/using-primitives/references/primitives.md b/plugin/skills/using-primitives/references/primitives.md index 87f0e672..fd5a2cc8 100644 --- a/plugin/skills/using-primitives/references/primitives.md +++ b/plugin/skills/using-primitives/references/primitives.md @@ -149,10 +149,13 @@ converged, and neither would ever see a conflict. Naming the owner also makes The rejection depends on the controller reference. For a resource registered with `Unowned()`, or one whose owner reference cannot be set because of a scope mismatch, nothing stops the second owner's forced apply from taking the -fields it declares, and the fields move between the two owners' managers on every reconcile. `managedFields` then names -the owner that wrote each field, but the framework does not detect the contention. A shared name between two owners is -the operator's responsibility in that case; a [guard](component.md#guards) that reads the live object and blocks when -another owner controls it is the way to make it explicit. +fields it declares, and the fields move between the two owners' managers on every reconcile. Register the resource with +[`component.BlockOnForeignController()`](component.md#resource-registration-options) to make the contention explicit: +before each apply the component reads the live object and, when another owner's controller reference is on it, reports +`Blocked` naming that owner instead of applying. This also turns the rejection in the default case into a readable +condition. The check compares controller references, so between two owners that both apply without one (two `Unowned()` +registrations, or owners the object's scope keeps from being referenced) nothing on the object carries either identity, +the fields keep moving between the two managers, and a shared name remains the operator's responsibility. !!! note "Upgrading from a release without the UID in the manager name" From a0401020d4270d69c7cd50f0aa0f558b2638dc41 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Wed, 26 Aug 2026 02:04:32 +0200 Subject: [PATCH 2/8] docs(component): state the exact suspension reason for a foreign-controlled resource (#199) The suspension path names the resource and the controlling owner in the style of the other suspension reasons; the GoDoc and docs claimed it was the same string as the reconcile-time block. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01MZRpj8ChzesLeqLkWT31vS --- docs/component.md | 3 ++- pkg/component/resource_options.go | 5 +++-- plugin/skills/building-components/references/component.md | 3 ++- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/docs/component.md b/docs/component.md index 49fb1942..be7d8c13 100644 --- a/docs/component.md +++ b/docs/component.md @@ -90,7 +90,8 @@ controller reference it replaces the API server's rejection of a second controll [Server-Side Apply](primitives.md#server-side-apply)). An object with no controller reference is never blocked, so contention between two owners that both apply without one is not detected. Unlike a custom guard, the check also runs during suspension: a resource another owner controls is neither scaled down nor deleted, and reports `Suspended` with -the same reason. It requires a managed resource; combining it with `ReadOnly()` is a build error. +the reason `Resource is controlled by ; nothing to suspend.`. It requires a managed resource; +combining it with `ReadOnly()` is a build error. Options compose. Gate a resource and exclude it from health aggregation in one call: diff --git a/pkg/component/resource_options.go b/pkg/component/resource_options.go index e03f28c0..cb7a78a1 100644 --- a/pkg/component/resource_options.go +++ b/pkg/component/resource_options.go @@ -183,8 +183,9 @@ func Unowned() ResourceOption { // owner's forced apply from taking the object's fields at all. // // During suspension the resource is not applied or deleted while another owner -// controls it; it reports Suspended with the same reason, since the component -// holds nothing there to suspend. +// controls it; it reports Suspended with a reason naming that owner +// ("Resource is controlled by ; nothing to suspend."), +// since the component holds nothing there to suspend. // // Requires a managed resource: combining it with ReadOnly is a configuration // error returned by Build. diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 49fb1942..be7d8c13 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -90,7 +90,8 @@ controller reference it replaces the API server's rejection of a second controll [Server-Side Apply](primitives.md#server-side-apply)). An object with no controller reference is never blocked, so contention between two owners that both apply without one is not detected. Unlike a custom guard, the check also runs during suspension: a resource another owner controls is neither scaled down nor deleted, and reports `Suspended` with -the same reason. It requires a managed resource; combining it with `ReadOnly()` is a build error. +the reason `Resource is controlled by ; nothing to suspend.`. It requires a managed resource; +combining it with `ReadOnly()` is a build error. Options compose. Gate a resource and exclude it from health aggregation in one call: From c5a3668dc92921a55f93fb51c0e27f630baebedb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Wed, 26 Aug 2026 02:09:27 +0200 Subject: [PATCH 3/8] fix(component): read the controller check through the API reader and surface the option (#199) The foreign-controller check read through the cached client, which can still hold the object without the controller reference the API server already carries; for an Unowned resource the forced apply would then take the other owner's fields, the case the option exists to stop. Read through ReconcileContext.APIReader when set, as the status-conflict path does. Name BlockOnForeignController in the WithResource GoDoc and in the plugin skills that list resource options and describe Server-Side Apply. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01MZRpj8ChzesLeqLkWT31vS --- docs/component.md | 24 +++++++------- pkg/component/builder.go | 4 +-- pkg/component/create.go | 20 +++++++++--- pkg/component/create_test.go | 32 +++++++++++++++++++ pkg/component/resource_options.go | 3 +- plugin/skills/building-components/SKILL.md | 9 ++++-- .../references/component.md | 24 +++++++------- plugin/skills/using-primitives/SKILL.md | 9 ++++-- 8 files changed, 89 insertions(+), 36 deletions(-) diff --git a/docs/component.md b/docs/component.md index be7d8c13..35e53f64 100644 --- a/docs/component.md +++ b/docs/component.md @@ -81,17 +81,19 @@ suspension with `DeleteOnSuspend()` all delete it directly, regardless of the `U (triggered by owner CR deletion) is suppressed. `BlockOnForeignController()` guards a managed resource against an object that another owner already controls. Before -each apply the component reads the live object; when it exists and carries a controller owner reference whose UID is not -the reconciling owner's, the resource reports `Blocked` with the message `controlled by `, no apply is -performed, and the resources after it are skipped, exactly as for a [blocked guard](#guards). The block clears on the -reconcile after that reference is gone. Reach for it wherever two custom resources may name one object: with the default -controller reference it replaces the API server's rejection of a second controller with a readable condition, and with -`Unowned()` it stops the second owner's forced apply from taking the object's fields at all (see -[Server-Side Apply](primitives.md#server-side-apply)). An object with no controller reference is never blocked, so -contention between two owners that both apply without one is not detected. Unlike a custom guard, the check also runs -during suspension: a resource another owner controls is neither scaled down nor deleted, and reports `Suspended` with -the reason `Resource is controlled by ; nothing to suspend.`. It requires a managed resource; -combining it with `ReadOnly()` is a build error. +each apply the component reads the live object, through `ReconcileContext.APIReader` when set (the cached client can +still miss a controller reference the API server already carries) and `ReconcileContext.Client` otherwise; when it +exists and carries a controller owner reference whose UID is not the reconciling owner's, the resource reports `Blocked` +with the message `controlled by `, no apply is performed, and the resources after it are skipped, exactly +as for a [blocked guard](#guards). The block clears on the reconcile after that reference is gone. Reach for it wherever +two custom resources may name one object: with the default controller reference it replaces the API server's rejection +of a second controller with a readable condition, and with `Unowned()` it stops the second owner's forced apply from +taking the object's fields at all (see [Server-Side Apply](primitives.md#server-side-apply)). An object with no +controller reference is never blocked, so contention between two owners that both apply without one is not detected. +Unlike a custom guard, the check also runs during suspension: a resource another owner controls is neither scaled down +nor deleted, and reports `Suspended` with the reason +`Resource is controlled by ; nothing to suspend.`. It requires a managed resource; combining it +with `ReadOnly()` is a build error. Options compose. Gate a resource and exclude it from health aggregation in one call: diff --git a/pkg/component/builder.go b/pkg/component/builder.go index a9089110..a76e4c85 100644 --- a/pkg/component/builder.go +++ b/pkg/component/builder.go @@ -114,8 +114,8 @@ func (b *Builder) WithConditionType(conditionType ConditionType) *Builder { // // Options configure the resource's lifecycle and its participation in health // aggregation; see the ResourceOption constructors (ReadOnly, Delete, DeleteWhen, -// GatedBy, Auxiliary, BlockOnAbsence, IgnoreIfAbsent, -// SuppressGraceInconsistencyWarning). With no options the resource is created or +// GatedBy, OrphanWhen, Unowned, Auxiliary, BlockOnAbsence, IgnoreIfAbsent, +// BlockOnForeignController, SuppressGraceInconsistencyWarning). With no options the resource is created or // updated and is required for the component to become Ready. // // A nil resource (a nil interface or a typed-nil pointer) is rejected with a diff --git a/pkg/component/create.go b/pkg/component/create.go index c000fa7b..ca8232cb 100644 --- a/pkg/component/create.go +++ b/pkg/component/create.go @@ -380,10 +380,16 @@ func reconcileResources( return results, nil } -// foreignController reads the live object of resource through the client and -// returns its controller owner reference when that reference points at an -// owner other than rec.Owner. It returns nil when the object does not exist, -// has no controller reference, or is controlled by rec.Owner. +// foreignController reads the live object of resource and returns its +// controller owner reference when that reference points at an owner other than +// rec.Owner. It returns nil when the object does not exist, has no controller +// reference, or is controlled by rec.Owner. +// +// The read goes through rec.APIReader when one is set and rec.Client otherwise. +// The manager's Client serves reads from the informer cache, which can still +// hold the object without the controller reference the API server already +// carries; a forced apply decided on that stale read would take the other +// owner's fields, which is what the option exists to stop. func foreignController( ctx context.Context, rec ReconcileContext, resource Resource, ) (*metav1.OwnerReference, error) { @@ -399,7 +405,11 @@ func foreignController( "failed to prepare controller check for resource %s: %w", resource.Identity(), err, ) } - if err := rec.Client.Get(ctx, client.ObjectKeyFromObject(obj), live); err != nil { + var reader client.Reader = rec.Client + if rec.APIReader != nil { + reader = rec.APIReader + } + if err := reader.Get(ctx, client.ObjectKeyFromObject(obj), live); err != nil { if apierrors.IsNotFound(err) { return nil, nil } diff --git a/pkg/component/create_test.go b/pkg/component/create_test.go index c46365ca..60266a8f 100644 --- a/pkg/component/create_test.go +++ b/pkg/component/create_test.go @@ -1140,3 +1140,35 @@ func TestApplyFieldOwner(t *testing.T) { assert.NotEqual(t, applyFieldOwner(newOwner(), component), applyFieldOwner(other, component)) }) } + +func TestReconcileResources_BlockOnForeignController_ReadsThroughAPIReader(t *testing.T) { + scheme := setupScheme() + owner := &MockOperatorCRD{ObjectMeta: metav1.ObjectMeta{Name: "test-owner", Namespace: "ns", UID: "this-uid"}} + desired := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "shared", Namespace: "ns"}} + + // The cache still holds the object without a controller; the API server + // already has another owner's controller reference on it. + stale := desired.DeepCopy() + live := desired.DeepCopy() + live.OwnerReferences = []metav1.OwnerReference{{ + APIVersion: GroupVersion.String(), Kind: "MockOperatorCRD", Name: "other", UID: "other-uid", + Controller: ptr.To(true), + }} + cache := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, stale).Build() + apiServer := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, live).Build() + + rec := setupReconcileContext(scheme, owner, cache) + rec.APIReader = apiServer + + resource := &MockResource{} + resource.On("Object").Return(desired, nil) + resource.On("Identity").Return("ConfigMap/shared") + entry := reconcileEntry{Resource: resource, Options: resourceOptions{Unowned: true, BlockOnForeignController: true}} + + results, err := reconcileResources(t.Context(), rec, []reconcileEntry{entry}, "comp", createTestRESTMapper()) + require.NoError(t, err) + require.Len(t, results, 1) + assert.Equal(t, convergingStatusGuardBlocked, results[0].Status.Status) + assert.Equal(t, "controlled by MockOperatorCRD other", results[0].Status.Reason) + resource.AssertNotCalled(t, "Mutate", mock.Anything) +} diff --git a/pkg/component/resource_options.go b/pkg/component/resource_options.go index cb7a78a1..94673bd1 100644 --- a/pkg/component/resource_options.go +++ b/pkg/component/resource_options.go @@ -168,7 +168,8 @@ func Unowned() ResourceOption { // BlockOnForeignController blocks the resource while the live object is // controlled by another owner. Before every apply the component reads the live -// object; when it exists and carries a controller owner reference whose UID is +// object, through ReconcileContext.APIReader when set and the cached +// ReconcileContext.Client otherwise; when it exists and carries a controller owner reference whose UID is // not the reconciling owner's, the resource records a blocked status with a // reason naming the controlling owner ("controlled by ") and, like // any blocked guard, stops the resources after it. The block clears on the diff --git a/plugin/skills/building-components/SKILL.md b/plugin/skills/building-components/SKILL.md index daf407a7..8c4fa952 100644 --- a/plugin/skills/building-components/SKILL.md +++ b/plugin/skills/building-components/SKILL.md @@ -51,9 +51,12 @@ 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()`. 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. See `references/component.md` for the full option matrix and `IncludeWhen` vs. `GatedBy`. +`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` naming the owner whose controller reference is on the live object instead +of applying over it; register it on any resource two custom resources may name. 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 be7d8c13..35e53f64 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -81,17 +81,19 @@ suspension with `DeleteOnSuspend()` all delete it directly, regardless of the `U (triggered by owner CR deletion) is suppressed. `BlockOnForeignController()` guards a managed resource against an object that another owner already controls. Before -each apply the component reads the live object; when it exists and carries a controller owner reference whose UID is not -the reconciling owner's, the resource reports `Blocked` with the message `controlled by `, no apply is -performed, and the resources after it are skipped, exactly as for a [blocked guard](#guards). The block clears on the -reconcile after that reference is gone. Reach for it wherever two custom resources may name one object: with the default -controller reference it replaces the API server's rejection of a second controller with a readable condition, and with -`Unowned()` it stops the second owner's forced apply from taking the object's fields at all (see -[Server-Side Apply](primitives.md#server-side-apply)). An object with no controller reference is never blocked, so -contention between two owners that both apply without one is not detected. Unlike a custom guard, the check also runs -during suspension: a resource another owner controls is neither scaled down nor deleted, and reports `Suspended` with -the reason `Resource is controlled by ; nothing to suspend.`. It requires a managed resource; -combining it with `ReadOnly()` is a build error. +each apply the component reads the live object, through `ReconcileContext.APIReader` when set (the cached client can +still miss a controller reference the API server already carries) and `ReconcileContext.Client` otherwise; when it +exists and carries a controller owner reference whose UID is not the reconciling owner's, the resource reports `Blocked` +with the message `controlled by `, no apply is performed, and the resources after it are skipped, exactly +as for a [blocked guard](#guards). The block clears on the reconcile after that reference is gone. Reach for it wherever +two custom resources may name one object: with the default controller reference it replaces the API server's rejection +of a second controller with a readable condition, and with `Unowned()` it stops the second owner's forced apply from +taking the object's fields at all (see [Server-Side Apply](primitives.md#server-side-apply)). An object with no +controller reference is never blocked, so contention between two owners that both apply without one is not detected. +Unlike a custom guard, the check also runs during suspension: a resource another owner controls is neither scaled down +nor deleted, and reports `Suspended` with the reason +`Resource is controlled by ; nothing to suspend.`. It requires a managed resource; combining it +with `ReadOnly()` is a build error. Options compose. Gate a resource and exclude it from health aggregation in one call: diff --git a/plugin/skills/using-primitives/SKILL.md b/plugin/skills/using-primitives/SKILL.md index 99b838d2..9e81f677 100644 --- a/plugin/skills/using-primitives/SKILL.md +++ b/plugin/skills/using-primitives/SKILL.md @@ -178,9 +178,12 @@ stripped server defaults. The owner's UID makes each owner a distinct manager: w reference (the default), two owners of one kind rendering the same object do not silently take each other's fields; the second owner's apply is rejected because of its second controller reference. For `Unowned()` resources, or where a scope mismatch prevents the owner reference, the second owner's forced apply still takes the fields it declares; -`managedFields` names each owner, but the framework does not detect the contention. When the readable manager would -exceed the API server's 128-character limit, the framework uses the hex-encoded SHA-256 of it instead (64 characters, -deterministic, still distinct per owner), so long kinds or component names show up in `managedFields` as a hash. +`managedFields` names each owner, but the framework does not detect the contention unless the resource is registered +with `component.BlockOnForeignController()`, which reports `Blocked` naming the owner whose controller reference is on +the live object instead of applying; two owners that both apply without a controller reference stay undetected. When the +readable manager would exceed the API server's 128-character limit, the framework uses the hex-encoded SHA-256 of it +instead (64 characters, deterministic, still distinct per owner), so long kinds or component names show up in +`managedFields` as a hash. **A Go type that overstates the CRD schema breaks Apply.** The API server's field manager types the patch against the target's OpenAPI schema before merging anything, so an undeclared field fails the whole apply and the server returns: From 4dd72deaa74a1969df971e614468b7c9ebf2bf6f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Wed, 26 Aug 2026 02:19:36 +0200 Subject: [PATCH 4/8] fix(component): log the owner that keeps a suspended resource from being touched (#199) The per-resource suspension reason is folded into "All resources are suspended." by the aggregation, so the controlling owner never reached the component condition while the docs said it did. Log the owner at the skip, describe the condition the component actually reports, and cover the suspended path through Component.Reconcile. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01MZRpj8ChzesLeqLkWT31vS --- docs/component.md | 6 ++-- .../create_foreign_controller_test.go | 30 +++++++++++++++++++ pkg/component/resource_options.go | 8 ++--- pkg/component/suspend.go | 9 +++++- .../references/component.md | 6 ++-- 5 files changed, 48 insertions(+), 11 deletions(-) diff --git a/docs/component.md b/docs/component.md index 35e53f64..23047395 100644 --- a/docs/component.md +++ b/docs/component.md @@ -91,9 +91,9 @@ of a second controller with a readable condition, and with `Unowned()` it stops taking the object's fields at all (see [Server-Side Apply](primitives.md#server-side-apply)). An object with no controller reference is never blocked, so contention between two owners that both apply without one is not detected. Unlike a custom guard, the check also runs during suspension: a resource another owner controls is neither scaled down -nor deleted, and reports `Suspended` with the reason -`Resource is controlled by ; nothing to suspend.`. It requires a managed resource; combining it -with `ReadOnly()` is a build error. +nor deleted. It counts as suspended, so the component condition reads `Suspended` with the usual +`All resources are suspended.` message, and the controlling owner is logged. It requires a managed resource; combining +it with `ReadOnly()` is a build error. Options compose. Gate a resource and exclude it from health aggregation in one call: diff --git a/pkg/component/create_foreign_controller_test.go b/pkg/component/create_foreign_controller_test.go index f2f0736e..ef087fd1 100644 --- a/pkg/component/create_foreign_controller_test.go +++ b/pkg/component/create_foreign_controller_test.go @@ -116,6 +116,36 @@ var _ = Describe("BlockOnForeignController", func() { Expect(liveConfigMap().OwnerReferences[0].UID).To(Equal(ownerA.UID)) }) + It("reports a suspended component without scaling down or deleting the object another owner controls", func() { + Expect(sharedConfigMapComponent(ownerA, resourceOptions{}).Reconcile(ctx, newTestReconcileContext(ownerA))).To(Succeed()) + before := liveConfigMap() + + desired := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: cmName, Namespace: namespace}} + res := &MockSuspendableResource{} + res.On("Object").Return(desired, nil) + res.On("Identity").Return("ConfigMap/" + cmName) + res.On("DeleteOnSuspend").Return(true) + // No Suspend, Mutate or SuspensionStatus expectation: reaching any of them + // means the component was about to apply or delete the other owner's object. + comp := &Component{ + name: "shared", + conditionType: "SharedReady", + suspended: true, + reconcileResources: []reconcileEntry{{ + Resource: res, Options: resourceOptions{Unowned: true, BlockOnForeignController: true}, + }}, + } + + Expect(comp.Reconcile(ctx, newTestReconcileContext(ownerB))).To(Succeed()) + cond := comp.GetCondition(ownerB) + Expect(cond.Reason).To(Equal(string(Suspended))) + Expect(cond.Message).To(Equal("All resources are suspended.")) + + after := liveConfigMap() + Expect(after.ResourceVersion).To(Equal(before.ResourceVersion)) + Expect(after.OwnerReferences[0].UID).To(Equal(ownerA.UID)) + }) + It("unblocks once the controlling owner's reference is gone", func() { Expect(sharedConfigMapComponent(ownerA, resourceOptions{}).Reconcile(ctx, newTestReconcileContext(ownerA))).To(Succeed()) diff --git a/pkg/component/resource_options.go b/pkg/component/resource_options.go index 94673bd1..c1967240 100644 --- a/pkg/component/resource_options.go +++ b/pkg/component/resource_options.go @@ -76,7 +76,7 @@ type resourceOptions struct { // live object is read before every apply and, when it carries a controller // reference to an owner other than the reconciling one, the resource records // a blocked status naming that owner and no apply is performed. During - // suspension such a resource reports Suspended without being applied or + // suspension such a resource counts as suspended without being applied or // deleted. Mutually exclusive with ReadOnly. BlockOnForeignController bool } @@ -184,9 +184,9 @@ func Unowned() ResourceOption { // owner's forced apply from taking the object's fields at all. // // During suspension the resource is not applied or deleted while another owner -// controls it; it reports Suspended with a reason naming that owner -// ("Resource is controlled by ; nothing to suspend."), -// since the component holds nothing there to suspend. +// controls it. It counts as suspended, since the component holds nothing there +// to suspend, so the component condition reads Suspended with the usual "All +// resources are suspended." message; the controlling owner is logged. // // Requires a managed resource: combining it with ReadOnly is a configuration // error returned by Build. diff --git a/pkg/component/suspend.go b/pkg/component/suspend.go index 3bd8eda5..178614b4 100644 --- a/pkg/component/suspend.go +++ b/pkg/component/suspend.go @@ -11,6 +11,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" ) type suspensionResults []concepts.SuspensionStatusWithReason @@ -110,13 +111,19 @@ func suspendResource( } // An object another owner controls is not this component's to scale down or - // delete. Report it suspended, since the component holds nothing there. + // delete. Report it suspended, since the component holds nothing there. The + // per-resource reason is folded into "All resources are suspended." by + // suspensionResults.summary, so the controlling owner is logged here. if entry.Options.BlockOnForeignController { controller, err := foreignController(ctx, rec, resource) if err != nil { return concepts.SuspensionStatusWithReason{}, err } if controller != nil { + log.FromContext(ctx).Info( + "skipping suspension of a resource another owner controls", + "resource", resource.Identity(), "controller", controller.Kind+" "+controller.Name, + ) return concepts.SuspensionStatusWithReason{ Status: concepts.SuspensionStatusSuspended, Reason: fmt.Sprintf( diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 35e53f64..23047395 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -91,9 +91,9 @@ of a second controller with a readable condition, and with `Unowned()` it stops taking the object's fields at all (see [Server-Side Apply](primitives.md#server-side-apply)). An object with no controller reference is never blocked, so contention between two owners that both apply without one is not detected. Unlike a custom guard, the check also runs during suspension: a resource another owner controls is neither scaled down -nor deleted, and reports `Suspended` with the reason -`Resource is controlled by ; nothing to suspend.`. It requires a managed resource; combining it -with `ReadOnly()` is a build error. +nor deleted. It counts as suspended, so the component condition reads `Suspended` with the usual +`All resources are suspended.` message, and the controlling owner is logged. It requires a managed resource; combining +it with `ReadOnly()` is a build error. Options compose. Gate a resource and exclude it from health aggregation in one call: From 46256e24c5fa113e8eaf18bc9914157dedd19854 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Wed, 26 Aug 2026 02:27:59 +0200 Subject: [PATCH 5/8] fix(component): keep a foreign-controlled object on every deletion path (#199) The delete list carried bare resources, so BlockOnForeignController was lost once a resource was marked for deletion, gated off, or the component gate turned off, and the object the option protects could still be deleted. Delete entries now carry their options and deleteResources skips an entry whose live object another owner controls, logging that owner. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01MZRpj8ChzesLeqLkWT31vS --- docs/component.md | 9 +++-- pkg/component/builder.go | 5 ++- pkg/component/builder_test.go | 2 +- pkg/component/component.go | 19 +++++----- pkg/component/component_test.go | 6 +-- .../create_foreign_controller_test.go | 38 +++++++++++++++++++ pkg/component/delete.go | 27 ++++++++++++- pkg/component/delete_test.go | 12 +++--- pkg/component/resource_options.go | 17 +++++---- .../references/component.md | 9 +++-- 10 files changed, 107 insertions(+), 37 deletions(-) diff --git a/docs/component.md b/docs/component.md index 23047395..ba9aacd0 100644 --- a/docs/component.md +++ b/docs/component.md @@ -90,10 +90,11 @@ two custom resources may name one object: with the default controller reference of a second controller with a readable condition, and with `Unowned()` it stops the second owner's forced apply from taking the object's fields at all (see [Server-Side Apply](primitives.md#server-side-apply)). An object with no controller reference is never blocked, so contention between two owners that both apply without one is not detected. -Unlike a custom guard, the check also runs during suspension: a resource another owner controls is neither scaled down -nor deleted. It counts as suspended, so the component condition reads `Suspended` with the usual -`All resources are suspended.` message, and the controlling owner is logged. It requires a managed resource; combining -it with `ReadOnly()` is a build error. +Unlike a custom guard, the check also covers every path that would delete the object. During suspension a resource +another owner controls is neither scaled down nor deleted; it counts as suspended, so the component condition reads +`Suspended` with the usual `All resources are suspended.` message. A deletion asked for by `Delete()`, `DeleteWhen()`, +`GatedBy()` or a disabled component feature gate is skipped the same way. Each skip is logged with the controlling +owner. It requires a managed resource; combining it with `ReadOnly()` is a build error. Options compose. Gate a resource and exclude it from health aggregation in one call: diff --git a/pkg/component/builder.go b/pkg/component/builder.go index a76e4c85..41ca672d 100644 --- a/pkg/component/builder.go +++ b/pkg/component/builder.go @@ -161,7 +161,10 @@ func (b *Builder) WithResource(resource Resource, opts ...ResourceOption) *Build case options.Orphan: b.component.orphanResources = append(b.component.orphanResources, resource) case options.Delete: - b.component.deleteResources = append(b.component.deleteResources, resource) + b.component.deleteResources = append(b.component.deleteResources, reconcileEntry{ + Resource: resource, + Options: options, + }) default: b.component.reconcileResources = append(b.component.reconcileResources, reconcileEntry{ Resource: resource, diff --git a/pkg/component/builder_test.go b/pkg/component/builder_test.go index 20449ec9..be322a5b 100644 --- a/pkg/component/builder_test.go +++ b/pkg/component/builder_test.go @@ -110,7 +110,7 @@ func TestBuilder_WithResource(t *testing.T) { assert.True(t, comp.reconcileResources[1].Options.ReadOnly) assert.Len(t, comp.deleteResources, 1) - assert.Equal(t, res3, comp.deleteResources[0]) + assert.Equal(t, res3, comp.deleteResources[0].Resource) assert.Len(t, comp.resourceLookup, 3) } diff --git a/pkg/component/component.go b/pkg/component/component.go index 02f3974a..4ebef9d8 100644 --- a/pkg/component/component.go +++ b/pkg/component/component.go @@ -147,7 +147,7 @@ type Component struct { // reconcileResources holds all non-delete resources in registration order. // Each entry pairs the resource with its full options. reconcileResources []reconcileEntry - deleteResources []Resource + deleteResources []reconcileEntry orphanResources []Resource resourceLookup map[string]Resource @@ -494,18 +494,19 @@ func (c *Component) Reconcile(ctx context.Context, rec ReconcileContext) error { // allManagedResources returns every managed (non-read-only) resource known to // the component, combining non-read-only reconcile entries and delete entries -// into a single slice. This is used when the feature gate is disabled and all -// managed resources must be deleted. Read-only resources are excluded because -// they are never created or modified by the component. -func (c *Component) allManagedResources() []Resource { - resources := make([]Resource, 0, len(c.reconcileResources)+len(c.deleteResources)) +// into a single slice, each with its resolved options. This is used when the +// feature gate is disabled and all managed resources must be deleted. Read-only +// resources are excluded because they are never created or modified by the +// component. +func (c *Component) allManagedResources() []reconcileEntry { + entries := make([]reconcileEntry, 0, len(c.reconcileResources)+len(c.deleteResources)) for _, entry := range c.reconcileResources { if !entry.Options.ReadOnly { - resources = append(resources, entry.Resource) + entries = append(entries, entry) } } - resources = append(resources, c.deleteResources...) - return resources + entries = append(entries, c.deleteResources...) + return entries } // prerequisiteBarrierActive reports whether the prerequisite initialization diff --git a/pkg/component/component_test.go b/pkg/component/component_test.go index 464e13b0..ea60cccf 100644 --- a/pkg/component/component_test.go +++ b/pkg/component/component_test.go @@ -582,7 +582,7 @@ var _ = Describe("Component Reconciler", func() { res.On("Object").Return(cm, nil) res.On("Identity").Return("ConfigMap/to-be-deleted") - comp.deleteResources = []Resource{res} + comp.deleteResources = []reconcileEntry{{Resource: res}} // When err := comp.Reconcile(ctx, recCtx) @@ -648,7 +648,7 @@ var _ = Describe("Component Reconciler", func() { res.On("Object").Return(nil, fmt.Errorf("delete object error")) res.On("Identity").Return("failing-delete-resource") - comp.deleteResources = []Resource{res} + comp.deleteResources = []reconcileEntry{{Resource: res}} // When err := comp.Reconcile(ctx, recCtx) @@ -709,7 +709,7 @@ var _ = Describe("Component Reconciler", func() { delRes.On("Identity").Return("failing-suspend-delete-resource") comp.reconcileResources = []reconcileEntry{{Resource: susRes}} - comp.deleteResources = []Resource{delRes} + comp.deleteResources = []reconcileEntry{{Resource: delRes}} // When err := comp.Reconcile(ctx, recCtx) diff --git a/pkg/component/create_foreign_controller_test.go b/pkg/component/create_foreign_controller_test.go index ef087fd1..e9f44774 100644 --- a/pkg/component/create_foreign_controller_test.go +++ b/pkg/component/create_foreign_controller_test.go @@ -146,6 +146,44 @@ var _ = Describe("BlockOnForeignController", func() { Expect(after.OwnerReferences[0].UID).To(Equal(ownerA.UID)) }) + It("does not delete the object another owner controls when the feature gate is disabled", func() { + Expect(sharedConfigMapComponent(ownerA, resourceOptions{}).Reconcile(ctx, newTestReconcileContext(ownerA))).To(Succeed()) + before := liveConfigMap() + + res := &MockResource{} + res.On("Object").Return(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: cmName, Namespace: namespace}}, nil) + res.On("Identity").Return("ConfigMap/" + cmName) + comp, err := NewComponentBuilder(). + WithName("shared"). + WithConditionType("SharedReady"). + WithFeatureGate(&testGate{enabled: false}). + WithResource(res, Unowned(), BlockOnForeignController()). + Build() + Expect(err).NotTo(HaveOccurred()) + + Expect(comp.Reconcile(ctx, newTestReconcileContext(ownerB))).To(Succeed()) + Expect(comp.GetCondition(ownerB).Reason).To(Equal(string(Disabled))) + Expect(liveConfigMap().ResourceVersion).To(Equal(before.ResourceVersion)) + }) + + It("does not delete the object another owner controls when the resource is marked for deletion", func() { + Expect(sharedConfigMapComponent(ownerA, resourceOptions{}).Reconcile(ctx, newTestReconcileContext(ownerA))).To(Succeed()) + before := liveConfigMap() + + res := &MockResource{} + res.On("Object").Return(&corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: cmName, Namespace: namespace}}, nil) + res.On("Identity").Return("ConfigMap/" + cmName) + comp, err := NewComponentBuilder(). + WithName("shared"). + WithConditionType("SharedReady"). + WithResource(res, Delete(), BlockOnForeignController()). + Build() + Expect(err).NotTo(HaveOccurred()) + + Expect(comp.Reconcile(ctx, newTestReconcileContext(ownerB))).To(Succeed()) + Expect(liveConfigMap().ResourceVersion).To(Equal(before.ResourceVersion)) + }) + It("unblocks once the controlling owner's reference is gone", func() { Expect(sharedConfigMapComponent(ownerA, resourceOptions{}).Reconcile(ctx, newTestReconcileContext(ownerA))).To(Succeed()) diff --git a/pkg/component/delete.go b/pkg/component/delete.go index 7e9404c0..f6db3ff8 100644 --- a/pkg/component/delete.go +++ b/pkg/component/delete.go @@ -7,6 +7,7 @@ import ( v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + "sigs.k8s.io/controller-runtime/pkg/log" ) // deleteConfig holds configuration for a deleteResources call. @@ -36,13 +37,18 @@ func withDeletionReason(reason string) deleteOption { // any errors encountered (e.g., failure to retrieve the underlying object or // failure of the delete operation) and continues with the remaining resources. // +// An entry registered with BlockOnForeignController is left in place when the +// 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. +// // 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. func deleteResources( - ctx context.Context, rec ReconcileContext, resources []Resource, opts ...deleteOption, + ctx context.Context, rec ReconcileContext, entries []reconcileEntry, opts ...deleteOption, ) error { cfg := deleteConfig{reason: "resource deletion flag"} for _, opt := range opts { @@ -51,7 +57,24 @@ func deleteResources( // we gather errors in order to delete as many resources as possible var errs []error - for _, resource := range resources { + for _, entry := range entries { + resource := entry.Resource + + if entry.Options.BlockOnForeignController { + controller, err := foreignController(ctx, rec, resource) + if err != nil { + errs = append(errs, err) + continue + } + if controller != nil { + log.FromContext(ctx).Info( + "skipping deletion of a resource another owner controls", + "resource", resource.Identity(), "controller", controller.Kind+" "+controller.Name, + ) + continue + } + } + object, err := resource.Object() if err != nil { errs = append(errs, fmt.Errorf( diff --git a/pkg/component/delete_test.go b/pkg/component/delete_test.go index ee687bf4..c6b72671 100644 --- a/pkg/component/delete_test.go +++ b/pkg/component/delete_test.go @@ -46,7 +46,7 @@ func TestDeleteResources(t *testing.T) { resource.On("Identity").Return("v1/ConfigMap/test-cm") // When - err = deleteResources(ctx, reconcileContext, []Resource{resource}) + err = deleteResources(ctx, reconcileContext, []reconcileEntry{{Resource: resource}}) // Then require.NoError(t, err) @@ -73,7 +73,7 @@ func TestDeleteResources(t *testing.T) { resource.On("Object").Return(resourceObject, nil) // When - err := deleteResources(ctx, reconcileContext, []Resource{resource}) + err := deleteResources(ctx, reconcileContext, []reconcileEntry{{Resource: resource}}) // Then require.NoError(t, err) @@ -100,7 +100,7 @@ func TestDeleteResources(t *testing.T) { resource2.On("Identity").Return("v1/ConfigMap/test-cm-2") // When - err = deleteResources(ctx, reconcileContext, []Resource{resource1, resource2}) + err = deleteResources(ctx, reconcileContext, []reconcileEntry{{Resource: resource1}, {Resource: resource2}}) // Then require.Error(t, err) @@ -148,7 +148,7 @@ func TestDeleteResources(t *testing.T) { resource2.On("Identity").Return("v1/ConfigMap/test-cm-2") // When - err := deleteResources(ctx, recCtx, []Resource{resource1, resource2}) + err := deleteResources(ctx, recCtx, []reconcileEntry{{Resource: resource1}, {Resource: resource2}}) // Then require.Error(t, err) @@ -176,7 +176,7 @@ func TestDeleteResources(t *testing.T) { recCtx := setupReconcileContext(scheme, owner, fakeClient) // When - err := deleteResources(ctx, recCtx, []Resource{resource}, withDeletionReason("suspension")) + err := deleteResources(ctx, recCtx, []reconcileEntry{{Resource: resource}}, withDeletionReason("suspension")) // Then require.NoError(t, err) @@ -205,7 +205,7 @@ func TestDeleteResources(t *testing.T) { recCtx := setupReconcileContext(scheme, owner, fakeClient) // When - err := deleteResources(ctx, recCtx, []Resource{resource}) + err := deleteResources(ctx, recCtx, []reconcileEntry{{Resource: resource}}) // Then require.NoError(t, err) diff --git a/pkg/component/resource_options.go b/pkg/component/resource_options.go index c1967240..1667b448 100644 --- a/pkg/component/resource_options.go +++ b/pkg/component/resource_options.go @@ -75,9 +75,9 @@ type resourceOptions struct { // BlockOnForeignController applies to managed resources. When true, the // live object is read before every apply and, when it carries a controller // reference to an owner other than the reconciling one, the resource records - // a blocked status naming that owner and no apply is performed. During - // suspension such a resource counts as suspended without being applied or - // deleted. Mutually exclusive with ReadOnly. + // a blocked status naming that owner and no apply is performed. Deletion + // paths (a deletion flag, a disabled feature gate, suspension) leave such an + // object in place. Mutually exclusive with ReadOnly. BlockOnForeignController bool } @@ -183,10 +183,13 @@ func Unowned() ResourceOption { // controller into a readable condition, and with Unowned it stops the second // owner's forced apply from taking the object's fields at all. // -// During suspension the resource is not applied or deleted while another owner -// controls it. It counts as suspended, since the component holds nothing there -// to suspend, so the component condition reads Suspended with the usual "All -// resources are suspended." message; the controlling owner is logged. +// The object is protected on every path that would write or delete it, not +// only the apply. During suspension the resource is not applied or deleted +// while another owner controls it; it counts as suspended, since the component +// holds nothing there to suspend, so the component condition reads Suspended +// with the usual "All resources are suspended." message. A deletion asked for +// by Delete, DeleteWhen, GatedBy or a disabled component feature gate is +// skipped the same way. Each skip is logged with the controlling owner. // // Requires a managed resource: combining it with ReadOnly is a configuration // error returned by Build. diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 23047395..ba9aacd0 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -90,10 +90,11 @@ two custom resources may name one object: with the default controller reference of a second controller with a readable condition, and with `Unowned()` it stops the second owner's forced apply from taking the object's fields at all (see [Server-Side Apply](primitives.md#server-side-apply)). An object with no controller reference is never blocked, so contention between two owners that both apply without one is not detected. -Unlike a custom guard, the check also runs during suspension: a resource another owner controls is neither scaled down -nor deleted. It counts as suspended, so the component condition reads `Suspended` with the usual -`All resources are suspended.` message, and the controlling owner is logged. It requires a managed resource; combining -it with `ReadOnly()` is a build error. +Unlike a custom guard, the check also covers every path that would delete the object. During suspension a resource +another owner controls is neither scaled down nor deleted; it counts as suspended, so the component condition reads +`Suspended` with the usual `All resources are suspended.` message. A deletion asked for by `Delete()`, `DeleteWhen()`, +`GatedBy()` or a disabled component feature gate is skipped the same way. Each skip is logged with the controlling +owner. It requires a managed resource; combining it with `ReadOnly()` is a build error. Options compose. Gate a resource and exclude it from health aggregation in one call: From 32b053fd07c3c466871f3fab2160c55c80b38189 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Wed, 26 Aug 2026 02:35:08 +0200 Subject: [PATCH 6/8] fix(component): bind a guarded delete to the object it observed (#199) A delete decided on an earlier read could still remove an object that another owner claimed in between, and unlike a lost apply race a lost delete is not repaired by the next reconcile. For an entry registered with BlockOnForeignController the delete now re-observes the live object and carries its UID and resourceVersion as preconditions, on the deletion flags, the disabled gate and delete-on-suspend alike; an absent object counts as deleted and a claimed one is left in place. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01MZRpj8ChzesLeqLkWT31vS --- docs/component.md | 4 +- pkg/component/create.go | 27 +++--- pkg/component/delete.go | 83 ++++++++++++++----- pkg/component/delete_test.go | 80 ++++++++++++++++++ pkg/component/resource_options.go | 6 +- pkg/component/suspend.go | 16 ++-- pkg/component/suspend_test.go | 35 ++++++++ .../references/component.md | 4 +- 8 files changed, 214 insertions(+), 41 deletions(-) diff --git a/docs/component.md b/docs/component.md index ba9aacd0..148db497 100644 --- a/docs/component.md +++ b/docs/component.md @@ -94,7 +94,9 @@ Unlike a custom guard, the check also covers every path that would delete the ob another owner controls is neither scaled down nor deleted; it counts as suspended, so the component condition reads `Suspended` with the usual `All resources are suspended.` message. A deletion asked for by `Delete()`, `DeleteWhen()`, `GatedBy()` or a disabled component feature gate is skipped the same way. Each skip is logged with the controlling -owner. It requires a managed resource; combining it with `ReadOnly()` is a build error. +owner. A delete of an object observed as safe carries the observed UID and resourceVersion as preconditions, so an owner +that claims the object between the read and the delete keeps it: the delete fails and the next reconcile observes the +object again. It requires a managed resource; combining it with `ReadOnly()` is a build error. Options compose. Gate a resource and exclude it from health aggregation in one call: diff --git a/pkg/component/create.go b/pkg/component/create.go index ca8232cb..a283362a 100644 --- a/pkg/component/create.go +++ b/pkg/component/create.go @@ -320,7 +320,7 @@ func reconcileResources( // A managed resource that another owner controls is blocked before the // apply, so the forced apply never takes that owner's fields. if entry.Options.BlockOnForeignController && !entry.Options.ReadOnly { - controller, err := foreignController(ctx, rec, resource) + _, controller, err := observeController(ctx, rec, resource) if err != nil { return nil, err } @@ -380,28 +380,29 @@ func reconcileResources( return results, nil } -// foreignController reads the live object of resource and returns its -// controller owner reference when that reference points at an owner other than -// rec.Owner. It returns nil when the object does not exist, has no controller -// reference, or is controlled by rec.Owner. +// observeController reads the live object of resource and returns it together +// with its controller owner reference when that reference points at an owner +// other than rec.Owner. The object is nil when it does not exist; the +// controller is nil when the object has no controller reference or is +// controlled by rec.Owner. // // The read goes through rec.APIReader when one is set and rec.Client otherwise. // The manager's Client serves reads from the informer cache, which can still // hold the object without the controller reference the API server already // carries; a forced apply decided on that stale read would take the other // owner's fields, which is what the option exists to stop. -func foreignController( +func observeController( ctx context.Context, rec ReconcileContext, resource Resource, -) (*metav1.OwnerReference, error) { +) (client.Object, *metav1.OwnerReference, error) { obj, err := resource.Object() if err != nil { - return nil, fmt.Errorf( + return nil, nil, fmt.Errorf( "failed to retrieve object for resource %s: %w", resource.Identity(), err, ) } live, err := newEmptyObjectLike(obj) if err != nil { - return nil, fmt.Errorf( + return nil, nil, fmt.Errorf( "failed to prepare controller check for resource %s: %w", resource.Identity(), err, ) } @@ -411,17 +412,17 @@ func foreignController( } if err := reader.Get(ctx, client.ObjectKeyFromObject(obj), live); err != nil { if apierrors.IsNotFound(err) { - return nil, nil + return nil, nil, nil } - return nil, fmt.Errorf( + return nil, nil, fmt.Errorf( "failed to read resource %s for controller check: %w", resource.Identity(), err, ) } controller := metav1.GetControllerOf(live) if controller == nil || controller.UID == rec.Owner.GetUID() { - return nil, nil + return live, nil, nil } - return controller, nil + return live, controller, nil } // foreignControllerReason is the blocked reason for an object controlled by diff --git a/pkg/component/delete.go b/pkg/component/delete.go index f6db3ff8..4005b9d7 100644 --- a/pkg/component/delete.go +++ b/pkg/component/delete.go @@ -7,6 +7,7 @@ import ( v1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" ) @@ -60,21 +61,6 @@ func deleteResources( for _, entry := range entries { resource := entry.Resource - if entry.Options.BlockOnForeignController { - controller, err := foreignController(ctx, rec, resource) - if err != nil { - errs = append(errs, err) - continue - } - if controller != nil { - log.FromContext(ctx).Info( - "skipping deletion of a resource another owner controls", - "resource", resource.Identity(), "controller", controller.Kind+" "+controller.Name, - ) - continue - } - } - object, err := resource.Object() if err != nil { errs = append(errs, fmt.Errorf( @@ -84,10 +70,12 @@ func deleteResources( continue } - if err := rec.Client.Delete(ctx, object); err != nil { - if !apierrors.IsNotFound(err) { - errs = append(errs, fmt.Errorf("failed to delete resource %s: %w", resource.Identity(), err)) - } + deleted, err := deleteEntry(ctx, rec, entry, object) + if err != nil { + errs = append(errs, err) + continue + } + if !deleted { continue } @@ -99,3 +87,60 @@ func deleteResources( return errors.Join(errs...) } + +// deleteEntry deletes object, the desired object of entry's resource, and +// 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. +func deleteEntry( + ctx context.Context, rec ReconcileContext, entry reconcileEntry, object client.Object, +) (bool, error) { + resource := entry.Resource + + if !entry.Options.BlockOnForeignController { + if err := rec.Client.Delete(ctx, object); err != nil { + if apierrors.IsNotFound(err) { + return false, nil + } + return false, fmt.Errorf("failed to delete resource %s: %w", resource.Identity(), err) + } + return true, nil + } + + live, controller, err := observeController(ctx, rec, resource) + if err != nil { + return false, err + } + if live == nil { + return false, nil + } + if controller != nil { + log.FromContext(ctx).Info( + "skipping deletion of a resource another owner controls", + "resource", resource.Identity(), "controller", controller.Kind+" "+controller.Name, + ) + return false, nil + } + + 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, + ) + } + return true, nil +} diff --git a/pkg/component/delete_test.go b/pkg/component/delete_test.go index c6b72671..56ddd765 100644 --- a/pkg/component/delete_test.go +++ b/pkg/component/delete_test.go @@ -1,6 +1,7 @@ package component import ( + "context" "errors" "testing" @@ -10,8 +11,10 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" ) func TestDeleteResources(t *testing.T) { @@ -215,3 +218,80 @@ func TestDeleteResources(t *testing.T) { assert.Empty(t, recorder.recorded()) }) } + +// claimBeforeDelete returns interceptor funcs that hand another owner the +// controller reference of the named object at the moment it is about to be +// deleted, reproducing an owner claiming the object between the check that +// observed it as safe and the delete. +func claimBeforeDelete(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([]metav1.OwnerReference{{ + APIVersion: GroupVersion.String(), Kind: "MockOperatorCRD", Name: "other", UID: "other-uid", + Controller: ptr.To(true), + }}) + require.NoError(t, c.Update(ctx, live)) + } + return c.Delete(ctx, obj, opts...) + }, + } +} + +func TestDeleteResources_BlockOnForeignController(t *testing.T) { + ctx := t.Context() + scheme := setupScheme() + newOwner := func() *MockOperatorCRD { + return &MockOperatorCRD{ObjectMeta: metav1.ObjectMeta{Name: "owner", Namespace: "default", UID: "this-uid"}} + } + newResource := func(name string) (*MockResource, *corev1.ConfigMap) { + obj := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "default"}} + res := &MockResource{} + res.On("Object").Return(obj, nil) + res.On("Identity").Return("ConfigMap/" + name) + return res, obj + } + guarded := func(res Resource) []reconcileEntry { + return []reconcileEntry{{Resource: res, Options: resourceOptions{BlockOnForeignController: true}}} + } + + 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(interceptor.Funcs{ + Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + deletes++ + return c.Delete(ctx, obj, opts...) + }, + }).Build() + + require.NoError(t, deleteResources(ctx, setupReconcileContext(scheme, owner, cli), guarded(res))) + assert.Equal(t, 0, deletes) + }) + + t.Run("deletes an object it observed as safe", func(t *testing.T) { + owner := newOwner() + res, obj := newResource("safe") + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj.DeepCopy()).Build() + + require.NoError(t, deleteResources(ctx, setupReconcileContext(scheme, owner, cli), guarded(res))) + assert.True(t, apierrors.IsNotFound(cli.Get(ctx, client.ObjectKeyFromObject(obj), &corev1.ConfigMap{}))) + }) + + t.Run("does not delete an object another owner claims after it was observed as safe", func(t *testing.T) { + owner := newOwner() + res, obj := newResource("claimed") + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj.DeepCopy()). + WithInterceptorFuncs(claimBeforeDelete(t, "claimed")).Build() + + err := deleteResources(ctx, setupReconcileContext(scheme, owner, cli), guarded(res)) + require.Error(t, err, "a delete that lost the race must be reported so the next reconcile rechecks") + got := &corev1.ConfigMap{} + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(obj), got)) + assert.Equal(t, "other-uid", string(got.OwnerReferences[0].UID)) + }) +} diff --git a/pkg/component/resource_options.go b/pkg/component/resource_options.go index 1667b448..7406df24 100644 --- a/pkg/component/resource_options.go +++ b/pkg/component/resource_options.go @@ -189,7 +189,11 @@ func Unowned() ResourceOption { // holds nothing there to suspend, so the component condition reads Suspended // with the usual "All resources are suspended." message. A deletion asked for // by Delete, DeleteWhen, GatedBy or a disabled component feature gate is -// skipped the same way. Each skip is logged with the controlling owner. +// skipped the same way. Each skip is logged with the controlling owner. A +// delete of an object observed as safe carries the observed UID and +// resourceVersion as preconditions, so an owner that claims the object between +// the read and the delete keeps it: the delete fails and the next reconcile +// observes the object again. // // Requires a managed resource: combining it with ReadOnly is a configuration // error returned by Build. diff --git a/pkg/component/suspend.go b/pkg/component/suspend.go index 178614b4..7c0531d0 100644 --- a/pkg/component/suspend.go +++ b/pkg/component/suspend.go @@ -115,7 +115,7 @@ func suspendResource( // per-resource reason is folded into "All resources are suspended." by // suspensionResults.summary, so the controlling owner is logged here. if entry.Options.BlockOnForeignController { - controller, err := foreignController(ctx, rec, resource) + _, controller, err := observeController(ctx, rec, resource) if err != nil { return concepts.SuspensionStatusWithReason{}, err } @@ -183,12 +183,16 @@ func suspendResource( return suspension, nil } - // Delete resource if it should be deleted + // 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. if suspendable.DeleteOnSuspend() { - if err := rec.Client.Delete(ctx, object); err != nil { - if !apierrors.IsNotFound(err) { - return suspension, fmt.Errorf("failed to delete resource: %w", err) - } + deleted, err := deleteEntry(ctx, rec, entry, object) + if err != nil { + return suspension, err + } + if !deleted { + return suspension, nil } rec.EventRecorder.Eventf( diff --git a/pkg/component/suspend_test.go b/pkg/component/suspend_test.go index 92aa59ab..8519b9ab 100644 --- a/pkg/component/suspend_test.go +++ b/pkg/component/suspend_test.go @@ -490,3 +490,38 @@ func TestSuspendResource_BlockOnForeignController(t *testing.T) { res.AssertCalled(t, "Suspend") }) } + +func TestSuspendResource_BlockOnForeignController_DeleteOnSuspend(t *testing.T) { + ctx := t.Context() + scheme := setupScheme() + + t.Run("does not delete on suspend an object another owner claims after it was observed as safe", func(t *testing.T) { + owner := setupTestOwner() + owner.UID = "this-uid" + res, obj := setupMockResource("claimed", concepts.SuspensionStatusSuspended, "Done", true) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj.DeepCopy()). + WithInterceptorFuncs(claimBeforeDelete(t, "claimed")).Build() + rec := setupReconcileContext(scheme, owner, cli) + entry := reconcileEntry{Resource: res, Options: resourceOptions{Unowned: true, BlockOnForeignController: true}} + + _, err := suspendResource(ctx, rec, entry, res, "test-component", testRESTMapper()) + require.Error(t, err) + got := &v1.ConfigMap{} + require.NoError(t, cli.Get(ctx, client.ObjectKeyFromObject(obj), got)) + assert.Equal(t, "other-uid", string(got.OwnerReferences[0].UID)) + }) + + t.Run("deletes on suspend an object it observed as safe", func(t *testing.T) { + owner := setupTestOwner() + owner.UID = "this-uid" + res, obj := setupMockResource("safe", concepts.SuspensionStatusSuspended, "Done", true) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj.DeepCopy()).Build() + rec := setupReconcileContext(scheme, owner, cli) + entry := reconcileEntry{Resource: res, Options: resourceOptions{Unowned: true, BlockOnForeignController: true}} + + status, err := suspendResource(ctx, rec, entry, res, "test-component", testRESTMapper()) + require.NoError(t, err) + assert.Equal(t, concepts.SuspensionStatusSuspended, status.Status) + assert.True(t, apierrors.IsNotFound(cli.Get(ctx, client.ObjectKeyFromObject(obj), &v1.ConfigMap{}))) + }) +} diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index ba9aacd0..148db497 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -94,7 +94,9 @@ Unlike a custom guard, the check also covers every path that would delete the ob another owner controls is neither scaled down nor deleted; it counts as suspended, so the component condition reads `Suspended` with the usual `All resources are suspended.` message. A deletion asked for by `Delete()`, `DeleteWhen()`, `GatedBy()` or a disabled component feature gate is skipped the same way. Each skip is logged with the controlling -owner. It requires a managed resource; combining it with `ReadOnly()` is a build error. +owner. A delete of an object observed as safe carries the observed UID and resourceVersion as preconditions, so an owner +that claims the object between the read and the delete keeps it: the delete fails and the next reconcile observes the +object again. It requires a managed resource; combining it with `ReadOnly()` is a build error. Options compose. Gate a resource and exclude it from health aggregation in one call: From 4b7bac9ed2bd95d7c4c684c050119635629c5d9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Wed, 26 Aug 2026 02:44:55 +0200 Subject: [PATCH 7/8] docs(component): write the BlockOnForeignController docs in simplified technical English (#199) Short sentences, one topic per paragraph, active voice, no semicolons, in the option docs, the guards section, the Server-Side Apply section and the two plugin skills that mention the option. No behaviour change. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01MZRpj8ChzesLeqLkWT31vS --- docs/component.md | 53 +++++++++++-------- docs/primitives.md | 16 +++--- plugin/skills/building-components/SKILL.md | 6 +-- .../references/component.md | 53 +++++++++++-------- plugin/skills/using-primitives/SKILL.md | 8 +-- .../using-primitives/references/primitives.md | 16 +++--- 6 files changed, 85 insertions(+), 67 deletions(-) diff --git a/docs/component.md b/docs/component.md index 148db497..7c2efc8c 100644 --- a/docs/component.md +++ b/docs/component.md @@ -65,7 +65,7 @@ be passed without a guard. | `component.Auxiliary()` | The resource's health does not contribute to the component condition (a blocked guard still does) | | `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 naming the owner whose controller reference is on the live object, performs no apply, and short-circuits the remaining resources | +| `component.BlockOnForeignController()` | Managed only: records a blocked status that names the owner whose controller reference is on the live object, performs no apply, and skips the remaining resources | | `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 @@ -80,23 +80,30 @@ is still subject to explicit deletion: `Delete()`, `DeleteWhen()`, `GatedBy()` ( suspension with `DeleteOnSuspend()` all delete it directly, regardless of the `Unowned` flag. Only Kubernetes GC (triggered by owner CR deletion) is suppressed. -`BlockOnForeignController()` guards a managed resource against an object that another owner already controls. Before -each apply the component reads the live object, through `ReconcileContext.APIReader` when set (the cached client can -still miss a controller reference the API server already carries) and `ReconcileContext.Client` otherwise; when it -exists and carries a controller owner reference whose UID is not the reconciling owner's, the resource reports `Blocked` -with the message `controlled by `, no apply is performed, and the resources after it are skipped, exactly -as for a [blocked guard](#guards). The block clears on the reconcile after that reference is gone. Reach for it wherever -two custom resources may name one object: with the default controller reference it replaces the API server's rejection -of a second controller with a readable condition, and with `Unowned()` it stops the second owner's forced apply from -taking the object's fields at all (see [Server-Side Apply](primitives.md#server-side-apply)). An object with no -controller reference is never blocked, so contention between two owners that both apply without one is not detected. -Unlike a custom guard, the check also covers every path that would delete the object. During suspension a resource -another owner controls is neither scaled down nor deleted; it counts as suspended, so the component condition reads -`Suspended` with the usual `All resources are suspended.` message. A deletion asked for by `Delete()`, `DeleteWhen()`, -`GatedBy()` or a disabled component feature gate is skipped the same way. Each skip is logged with the controlling -owner. A delete of an object observed as safe carries the observed UID and resourceVersion as preconditions, so an owner -that claims the object between the read and the delete keeps it: the delete fails and the next reconcile observes the -object again. It requires a managed resource; combining it with `ReadOnly()` is a build error. +`BlockOnForeignController()` protects a managed resource from an object that another owner already controls. Before each +apply, the component reads the live object. If the object has a controller reference to a different owner, the resource +reports `Blocked` with the message `controlled by `. The component performs no apply and skips the +resources after it, exactly as for a [blocked guard](#guards). The block clears on the first reconcile after that +reference is gone. An object with no controller reference is never blocked, so the option does not detect two owners +that both apply without one. + +The read goes through `ReconcileContext.APIReader` when it is set, and through `ReconcileContext.Client` otherwise. The +cached client can miss a controller reference that the API server already has. + +Use the option on any resource that two custom resources can name. With the default controller reference, it replaces +the rejection by the API server of a second controller with a readable condition. With `Unowned()`, it stops the forced +apply of the second owner from taking the fields of the object (see +[Server-Side Apply](primitives.md#server-side-apply)). + +Unlike a custom guard, the check also covers every path that deletes the object. During suspension, the component does +not scale down or delete a resource that another owner controls. The resource counts as suspended, so the component +condition reads `Suspended` with the usual `All resources are suspended.` message. The component also skips a deletion +that `Delete()`, `DeleteWhen()`, `GatedBy()` or a disabled feature gate asks for. The component logs each skip with the +controlling owner. + +A delete of an object that the read found safe carries the observed UID and resourceVersion as preconditions. If another +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. Options compose. Gate a resource and exclude it from health aggregation in one call: @@ -1140,13 +1147,13 @@ registered custom guard; it does not affect declared data guards. regardless of its participation mode, and all resources after it are skipped entirely. This override exists because a blocked guard halts the entire pipeline; subsequent required resources would otherwise be silently absent from health aggregation. -- After a resource's own guard clears, a managed resource registered with - [`BlockOnForeignController()`](#resource-registration-options) is also checked against the live object's controller - reference; another owner's reference records `Blocked` the same way, with the message `controlled by `. +- After the guard of a resource clears, the component also compares the controller reference of the live object for a + managed resource registered with [`BlockOnForeignController()`](#resource-registration-options). A reference to + another owner records `Blocked` in the same way, with the message `controlled by `. - On the next reconcile, if the guard clears (`Unblocked`), the resource is applied normally. - Guards are **not** evaluated during suspension. The suspension path always proceeds regardless of guard state. The - exception is [`BlockOnForeignController()`](#resource-registration-options), which is checked on every path so that a - suspension never scales down or deletes an object another owner controls. + exception is [`BlockOnForeignController()`](#resource-registration-options), which the component checks on every path. + As a result, a suspension never scales down or deletes an object that another owner controls. - A guard evaluation error is treated as a reconciliation failure and sets the condition to `Error`. A blocked guard produces a condition like: diff --git a/docs/primitives.md b/docs/primitives.md index fd5a2cc8..d14fb66f 100644 --- a/docs/primitives.md +++ b/docs/primitives.md @@ -149,13 +149,15 @@ converged, and neither would ever see a conflict. Naming the owner also makes The rejection depends on the controller reference. For a resource registered with `Unowned()`, or one whose owner reference cannot be set because of a scope mismatch, nothing stops the second owner's forced apply from taking the -fields it declares, and the fields move between the two owners' managers on every reconcile. Register the resource with -[`component.BlockOnForeignController()`](component.md#resource-registration-options) to make the contention explicit: -before each apply the component reads the live object and, when another owner's controller reference is on it, reports -`Blocked` naming that owner instead of applying. This also turns the rejection in the default case into a readable -condition. The check compares controller references, so between two owners that both apply without one (two `Unowned()` -registrations, or owners the object's scope keeps from being referenced) nothing on the object carries either identity, -the fields keep moving between the two managers, and a shared name remains the operator's responsibility. +fields it declares, and the fields move between the two owners' managers on every reconcile. + +Register the resource with [`component.BlockOnForeignController()`](component.md#resource-registration-options) to make +the contention visible. Before each apply, the component reads the live object. If the object has a controller reference +to another owner, the resource reports `Blocked` and names that owner instead of applying. In the default case, this +also turns the rejection by the API server into a readable condition. The check compares controller references only, so +two owners that both apply without one leave no identity on the object (two `Unowned()` registrations, or owners that +the scope of the object keeps from being referenced). Then the fields keep moving between the two managers, and a shared +name remains the responsibility of the operator. !!! note "Upgrading from a release without the UID in the manager name" diff --git a/plugin/skills/building-components/SKILL.md b/plugin/skills/building-components/SKILL.md index 8c4fa952..2750eecd 100644 --- a/plugin/skills/building-components/SKILL.md +++ b/plugin/skills/building-components/SKILL.md @@ -54,9 +54,9 @@ Each `WithResource` call accepts `ResourceOption` values: `component.ReadOnly()` `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` naming the owner whose controller reference is on the live object instead -of applying over it; register it on any resource two custom resources may name. See `references/component.md` for the -full option matrix and `IncludeWhen` vs. `GatedBy`. +`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`. ## Registration order is execution order diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 148db497..7c2efc8c 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -65,7 +65,7 @@ be passed without a guard. | `component.Auxiliary()` | The resource's health does not contribute to the component condition (a blocked guard still does) | | `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 naming the owner whose controller reference is on the live object, performs no apply, and short-circuits the remaining resources | +| `component.BlockOnForeignController()` | Managed only: records a blocked status that names the owner whose controller reference is on the live object, performs no apply, and skips the remaining resources | | `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 @@ -80,23 +80,30 @@ is still subject to explicit deletion: `Delete()`, `DeleteWhen()`, `GatedBy()` ( suspension with `DeleteOnSuspend()` all delete it directly, regardless of the `Unowned` flag. Only Kubernetes GC (triggered by owner CR deletion) is suppressed. -`BlockOnForeignController()` guards a managed resource against an object that another owner already controls. Before -each apply the component reads the live object, through `ReconcileContext.APIReader` when set (the cached client can -still miss a controller reference the API server already carries) and `ReconcileContext.Client` otherwise; when it -exists and carries a controller owner reference whose UID is not the reconciling owner's, the resource reports `Blocked` -with the message `controlled by `, no apply is performed, and the resources after it are skipped, exactly -as for a [blocked guard](#guards). The block clears on the reconcile after that reference is gone. Reach for it wherever -two custom resources may name one object: with the default controller reference it replaces the API server's rejection -of a second controller with a readable condition, and with `Unowned()` it stops the second owner's forced apply from -taking the object's fields at all (see [Server-Side Apply](primitives.md#server-side-apply)). An object with no -controller reference is never blocked, so contention between two owners that both apply without one is not detected. -Unlike a custom guard, the check also covers every path that would delete the object. During suspension a resource -another owner controls is neither scaled down nor deleted; it counts as suspended, so the component condition reads -`Suspended` with the usual `All resources are suspended.` message. A deletion asked for by `Delete()`, `DeleteWhen()`, -`GatedBy()` or a disabled component feature gate is skipped the same way. Each skip is logged with the controlling -owner. A delete of an object observed as safe carries the observed UID and resourceVersion as preconditions, so an owner -that claims the object between the read and the delete keeps it: the delete fails and the next reconcile observes the -object again. It requires a managed resource; combining it with `ReadOnly()` is a build error. +`BlockOnForeignController()` protects a managed resource from an object that another owner already controls. Before each +apply, the component reads the live object. If the object has a controller reference to a different owner, the resource +reports `Blocked` with the message `controlled by `. The component performs no apply and skips the +resources after it, exactly as for a [blocked guard](#guards). The block clears on the first reconcile after that +reference is gone. An object with no controller reference is never blocked, so the option does not detect two owners +that both apply without one. + +The read goes through `ReconcileContext.APIReader` when it is set, and through `ReconcileContext.Client` otherwise. The +cached client can miss a controller reference that the API server already has. + +Use the option on any resource that two custom resources can name. With the default controller reference, it replaces +the rejection by the API server of a second controller with a readable condition. With `Unowned()`, it stops the forced +apply of the second owner from taking the fields of the object (see +[Server-Side Apply](primitives.md#server-side-apply)). + +Unlike a custom guard, the check also covers every path that deletes the object. During suspension, the component does +not scale down or delete a resource that another owner controls. The resource counts as suspended, so the component +condition reads `Suspended` with the usual `All resources are suspended.` message. The component also skips a deletion +that `Delete()`, `DeleteWhen()`, `GatedBy()` or a disabled feature gate asks for. The component logs each skip with the +controlling owner. + +A delete of an object that the read found safe carries the observed UID and resourceVersion as preconditions. If another +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. Options compose. Gate a resource and exclude it from health aggregation in one call: @@ -1140,13 +1147,13 @@ registered custom guard; it does not affect declared data guards. regardless of its participation mode, and all resources after it are skipped entirely. This override exists because a blocked guard halts the entire pipeline; subsequent required resources would otherwise be silently absent from health aggregation. -- After a resource's own guard clears, a managed resource registered with - [`BlockOnForeignController()`](#resource-registration-options) is also checked against the live object's controller - reference; another owner's reference records `Blocked` the same way, with the message `controlled by `. +- After the guard of a resource clears, the component also compares the controller reference of the live object for a + managed resource registered with [`BlockOnForeignController()`](#resource-registration-options). A reference to + another owner records `Blocked` in the same way, with the message `controlled by `. - On the next reconcile, if the guard clears (`Unblocked`), the resource is applied normally. - Guards are **not** evaluated during suspension. The suspension path always proceeds regardless of guard state. The - exception is [`BlockOnForeignController()`](#resource-registration-options), which is checked on every path so that a - suspension never scales down or deletes an object another owner controls. + exception is [`BlockOnForeignController()`](#resource-registration-options), which the component checks on every path. + As a result, a suspension never scales down or deletes an object that another owner controls. - A guard evaluation error is treated as a reconciliation failure and sets the condition to `Error`. A blocked guard produces a condition like: diff --git a/plugin/skills/using-primitives/SKILL.md b/plugin/skills/using-primitives/SKILL.md index 9e81f677..10927fd9 100644 --- a/plugin/skills/using-primitives/SKILL.md +++ b/plugin/skills/using-primitives/SKILL.md @@ -178,10 +178,10 @@ stripped server defaults. The owner's UID makes each owner a distinct manager: w reference (the default), two owners of one kind rendering the same object do not silently take each other's fields; the second owner's apply is rejected because of its second controller reference. For `Unowned()` resources, or where a scope mismatch prevents the owner reference, the second owner's forced apply still takes the fields it declares; -`managedFields` names each owner, but the framework does not detect the contention unless the resource is registered -with `component.BlockOnForeignController()`, which reports `Blocked` naming the owner whose controller reference is on -the live object instead of applying; two owners that both apply without a controller reference stay undetected. When the -readable manager would exceed the API server's 128-character limit, the framework uses the hex-encoded SHA-256 of it +`managedFields` names each owner, but the framework does not detect the contention unless you register the resource with +`component.BlockOnForeignController()`. That option reports `Blocked` and names the owner whose controller reference is +on the live object, instead of applying. Two owners that both apply without a controller reference stay undetected. When +the readable manager would exceed the API server's 128-character limit, the framework uses the hex-encoded SHA-256 of it instead (64 characters, deterministic, still distinct per owner), so long kinds or component names show up in `managedFields` as a hash. diff --git a/plugin/skills/using-primitives/references/primitives.md b/plugin/skills/using-primitives/references/primitives.md index fd5a2cc8..d14fb66f 100644 --- a/plugin/skills/using-primitives/references/primitives.md +++ b/plugin/skills/using-primitives/references/primitives.md @@ -149,13 +149,15 @@ converged, and neither would ever see a conflict. Naming the owner also makes The rejection depends on the controller reference. For a resource registered with `Unowned()`, or one whose owner reference cannot be set because of a scope mismatch, nothing stops the second owner's forced apply from taking the -fields it declares, and the fields move between the two owners' managers on every reconcile. Register the resource with -[`component.BlockOnForeignController()`](component.md#resource-registration-options) to make the contention explicit: -before each apply the component reads the live object and, when another owner's controller reference is on it, reports -`Blocked` naming that owner instead of applying. This also turns the rejection in the default case into a readable -condition. The check compares controller references, so between two owners that both apply without one (two `Unowned()` -registrations, or owners the object's scope keeps from being referenced) nothing on the object carries either identity, -the fields keep moving between the two managers, and a shared name remains the operator's responsibility. +fields it declares, and the fields move between the two owners' managers on every reconcile. + +Register the resource with [`component.BlockOnForeignController()`](component.md#resource-registration-options) to make +the contention visible. Before each apply, the component reads the live object. If the object has a controller reference +to another owner, the resource reports `Blocked` and names that owner instead of applying. In the default case, this +also turns the rejection by the API server into a readable condition. The check compares controller references only, so +two owners that both apply without one leave no identity on the object (two `Unowned()` registrations, or owners that +the scope of the object keeps from being referenced). Then the fields keep moving between the two managers, and a shared +name remains the responsibility of the operator. !!! note "Upgrading from a release without the UID in the manager name" From 8f68998c9e5762711eb4defadf84844a317ac68c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Wed, 26 Aug 2026 02:45:19 +0200 Subject: [PATCH 8/8] docs(component): shorten the option table row and guard bullet (#199) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01MZRpj8ChzesLeqLkWT31vS --- docs/component.md | 8 ++++---- plugin/skills/building-components/references/component.md | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/docs/component.md b/docs/component.md index 7c2efc8c..4601e868 100644 --- a/docs/component.md +++ b/docs/component.md @@ -65,7 +65,7 @@ be passed without a guard. | `component.Auxiliary()` | The resource's health does not contribute to the component condition (a blocked guard still does) | | `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, performs no apply, and skips the remaining resources | +| `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.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 @@ -1147,9 +1147,9 @@ registered custom guard; it does not affect declared data guards. regardless of its participation mode, and all resources after it are skipped entirely. This override exists because a blocked guard halts the entire pipeline; subsequent required resources would otherwise be silently absent from health aggregation. -- After the guard of a resource clears, the component also compares the controller reference of the live object for a - managed resource registered with [`BlockOnForeignController()`](#resource-registration-options). A reference to - another owner records `Blocked` in the same way, with the message `controlled by `. +- After the guard of a resource clears, the component also reads the controller reference of the live object for a + resource registered with [`BlockOnForeignController()`](#resource-registration-options). A reference to another owner + records `Blocked` in the same way, with the message `controlled by `. - On the next reconcile, if the guard clears (`Unblocked`), the resource is applied normally. - Guards are **not** evaluated during suspension. The suspension path always proceeds regardless of guard state. The exception is [`BlockOnForeignController()`](#resource-registration-options), which the component checks on every path. diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 7c2efc8c..4601e868 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -65,7 +65,7 @@ be passed without a guard. | `component.Auxiliary()` | The resource's health does not contribute to the component condition (a blocked guard still does) | | `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, performs no apply, and skips the remaining resources | +| `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.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 @@ -1147,9 +1147,9 @@ registered custom guard; it does not affect declared data guards. regardless of its participation mode, and all resources after it are skipped entirely. This override exists because a blocked guard halts the entire pipeline; subsequent required resources would otherwise be silently absent from health aggregation. -- After the guard of a resource clears, the component also compares the controller reference of the live object for a - managed resource registered with [`BlockOnForeignController()`](#resource-registration-options). A reference to - another owner records `Blocked` in the same way, with the message `controlled by `. +- After the guard of a resource clears, the component also reads the controller reference of the live object for a + resource registered with [`BlockOnForeignController()`](#resource-registration-options). A reference to another owner + records `Blocked` in the same way, with the message `controlled by `. - On the next reconcile, if the guard clears (`Unblocked`), the resource is applied normally. - Guards are **not** evaluated during suspension. The suspension path always proceeds regardless of guard state. The exception is [`BlockOnForeignController()`](#resource-registration-options), which the component checks on every path.