diff --git a/docs/component.md b/docs/component.md index 4d1cfcd3..4601e868 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 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 @@ -79,6 +80,31 @@ 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()` 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: ```go @@ -1121,8 +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 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. +- 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. + 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 87f0e672..d14fb66f 100644 --- a/docs/primitives.md +++ b/docs/primitives.md @@ -149,10 +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. `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 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/pkg/component/builder.go b/pkg/component/builder.go index a9089110..41ca672d 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 @@ -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.go b/pkg/component/create.go index 706d065d..a283362a 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 := observeController(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,57 @@ func reconcileResources( return results, nil } +// 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 observeController( + ctx context.Context, rec ReconcileContext, resource Resource, +) (client.Object, *metav1.OwnerReference, error) { + obj, err := resource.Object() + if err != nil { + 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, nil, fmt.Errorf( + "failed to prepare controller check for resource %s: %w", resource.Identity(), err, + ) + } + 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, nil + } + 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 live, nil, nil + } + return live, 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..e9f44774 --- /dev/null +++ b/pkg/component/create_foreign_controller_test.go @@ -0,0 +1,206 @@ +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("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("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()) + + 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/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/delete.go b/pkg/component/delete.go index 7e9404c0..4005b9d7 100644 --- a/pkg/component/delete.go +++ b/pkg/component/delete.go @@ -7,6 +7,8 @@ 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" ) // deleteConfig holds configuration for a deleteResources call. @@ -36,13 +38,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 +58,9 @@ 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 + object, err := resource.Object() if err != nil { errs = append(errs, fmt.Errorf( @@ -61,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 } @@ -76,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 ee687bf4..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) { @@ -46,7 +49,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 +76,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 +103,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 +151,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 +179,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 +208,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) @@ -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 3e114f9b..7406df24 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. Deletion + // paths (a deletion flag, a disabled feature gate, suspension) leave such an + // object in place. Mutually exclusive with ReadOnly. + BlockOnForeignController bool } // ReadOnly marks the resource as read-only: the component fetches its current @@ -158,6 +166,41 @@ 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, 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 +// 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. +// +// 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. 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. +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 +235,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 +313,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..7c0531d0 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 @@ -109,6 +110,29 @@ 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. 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 := observeController(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( + "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. @@ -159,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 ad73ed19..8519b9ab 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,108 @@ 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") + }) +} + +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/SKILL.md b/plugin/skills/building-components/SKILL.md index daf407a7..2750eecd 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` 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 4d1cfcd3..4601e868 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 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 @@ -79,6 +80,31 @@ 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()` 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: ```go @@ -1121,8 +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 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. +- 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. + 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 99b838d2..10927fd9 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 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. **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: diff --git a/plugin/skills/using-primitives/references/primitives.md b/plugin/skills/using-primitives/references/primitives.md index 87f0e672..d14fb66f 100644 --- a/plugin/skills/using-primitives/references/primitives.md +++ b/plugin/skills/using-primitives/references/primitives.md @@ -149,10 +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. `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 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"