From 7279366bcddcc9fea61cfa2be514256148ddeb14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Thu, 25 Jun 2026 17:09:16 +0200 Subject: [PATCH 1/3] feat(component): Unowned() resource option to suppress owner reference MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add Unowned() ResourceOption for resources that must outlive their owner CR, such as backup records. The component creates and updates the resource normally but omits the controller owner reference, so Kubernetes GC does not cascade-delete it when the owner is removed. Explicit deletion paths (Delete(), DeleteWhen(), GatedBy() when disabled, and suspension's DeleteOnSuspend()) are unaffected — only GC is suppressed. The Unowned flag is propagated through the suspension path so that suspension mutations applied to an Unowned resource also skip setting the owner reference. Co-Authored-By: Claude Sonnet 4.6 --- docs/component.md | 28 +++++--- pkg/component/component.go | 8 +-- pkg/component/create.go | 22 ++++--- pkg/component/create_test.go | 90 ++++++++++++++++++++++---- pkg/component/resource_options.go | 16 +++++ pkg/component/resource_options_test.go | 10 +++ pkg/component/suspend.go | 14 ++-- pkg/component/suspend_test.go | 56 +++++++++++----- 8 files changed, 189 insertions(+), 55 deletions(-) diff --git a/docs/component.md b/docs/component.md index c422af1a..d63b16bd 100644 --- a/docs/component.md +++ b/docs/component.md @@ -67,21 +67,29 @@ if err != nil { Each resource is registered via `WithResource`. The second argument accepts zero or more `ResourceOption` values that control how the component interacts with the resource: -| Option | Behavior | -| --------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------- | -| (none) | **Managed**: created or updated; health contributes to the condition | -| `component.ReadOnly()` | **Read-only**: fetched but never modified; health still contributes | -| `component.Delete()` / `component.DeleteWhen(cond)` | **Delete**: removed from the cluster (unconditionally, or when `cond` is true); does not contribute to health | -| `component.GatedBy(gate)` | Deletes the resource when the feature gate is disabled; managed when enabled | -| `component.Auxiliary()` | The resource's health does not contribute to the component condition (a blocked guard still does) | -| `component.SuppressGraceInconsistencyWarning()` | Suppresses the grace/convergence inconsistency warning | -| `component.ReadOnly(), component.BlockOnAbsence()` | **Read-only with watch-driven retry**: NotFound records a blocked status and short-circuits the remaining resources | -| `component.ReadOnly(), component.IgnoreIfAbsent()` | **Optional read-only**: NotFound is silently ignored; last-known state preserved | +| Option | Behavior | +| --------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------- | +| (none) | **Managed**: created or updated; health contributes to the condition | +| `component.ReadOnly()` | **Read-only**: fetched but never modified; health still contributes | +| `component.Delete()` / `component.DeleteWhen(cond)` | **Delete**: removed from the cluster (unconditionally, or when `cond` is true); does not contribute to health | +| `component.GatedBy(gate)` | Deletes the resource when the feature gate is disabled; managed when enabled | +| `component.Unowned()` | **Unowned**: created and updated normally, but no owner reference is set; not garbage-collected on owner CR deletion | +| `component.Auxiliary()` | The resource's health does not contribute to the component condition (a blocked guard still does) | +| `component.SuppressGraceInconsistencyWarning()` | Suppresses the grace/convergence inconsistency warning | +| `component.ReadOnly(), component.BlockOnAbsence()` | **Read-only with watch-driven retry**: NotFound records a blocked status and short-circuits the remaining resources | +| `component.ReadOnly(), component.IgnoreIfAbsent()` | **Optional read-only**: NotFound is silently ignored; last-known state preserved | A read-only resource is not owned by the component, so it is never deleted. `ReadOnly()` is mutually exclusive with `Delete()`, `DeleteWhen()`, and `GatedBy()`; combining them is a build error. To conditionally include a read-only resource, use [`IncludeWhen`](#includewhen), which omits the resource without deleting it. +`Unowned()` resources are created and updated by the component but are not garbage-collected when the owner CR is +deleted, because no controller owner reference is set. This is intended for resources that must outlive the management +lifecycle — for example, backup records that should persist after the application CR is removed. An `Unowned` resource +is still subject to explicit deletion: `Delete()`, `DeleteWhen()`, `GatedBy()` (when the gate is disabled), and +suspension with `DeleteOnSuspend()` all delete it directly, regardless of the `Unowned` flag. Only Kubernetes GC +(triggered by owner CR deletion) is suppressed. + ### Conditional and Optional Resources Pass functional options directly on the `WithResource` call to express feature gating, auxiliary participation, or any diff --git a/pkg/component/component.go b/pkg/component/component.go index d2083817..cb590418 100644 --- a/pkg/component/component.go +++ b/pkg/component/component.go @@ -452,13 +452,13 @@ func (c *Component) evaluatePrerequisites(rec ReconcileContext) (PrerequisiteRes return PrerequisiteResult{Status: PrerequisiteStatusMet}, nil } -// managedResources returns only the non-read-only resources from the reconcile list. +// managedResources returns only the non-read-only entries from the reconcile list. // This is used during suspension, where only managed resources are suspended. -func (c *Component) managedResources() []Resource { - var managed []Resource +func (c *Component) managedResources() []reconcileEntry { + var managed []reconcileEntry for _, entry := range c.reconcileResources { if !entry.Options.ReadOnly { - managed = append(managed, entry.Resource) + managed = append(managed, entry) } } return managed diff --git a/pkg/component/create.go b/pkg/component/create.go index 6395ab03..1c8724dc 100644 --- a/pkg/component/create.go +++ b/pkg/component/create.go @@ -25,7 +25,7 @@ import ( // status-collection sequence. func applyResource( ctx context.Context, rec ReconcileContext, resource Resource, - fieldOwner client.FieldOwner, mapper meta.RESTMapper, + fieldOwner client.FieldOwner, mapper meta.RESTMapper, skipOwnerRef bool, ) (*reconcileResult, error) { obj, err := resource.Object() if err != nil { @@ -52,7 +52,7 @@ func applyResource( preMutate := obj.DeepCopyObject() // Apply mutations to desired state - ownerRefSkipped, err := mutateResource(resource, obj, rec.Owner, rec.Scheme, mapper) + ownerRefSkipped, err := mutateResource(resource, obj, rec.Owner, rec.Scheme, mapper, skipOwnerRef) if err != nil { return nil, fmt.Errorf( "failed to mutate resource %s: %w", resource.Identity(), err, @@ -135,7 +135,7 @@ func applyResource( // (e.g., "ExampleApp/web-interface"). Forced ownership means the framework takes control // of any conflicting fields from other managers for fields it explicitly declares. func applyResources( - ctx context.Context, rec ReconcileContext, resources []Resource, + ctx context.Context, rec ReconcileContext, entries []reconcileEntry, componentName string, mapper meta.RESTMapper, ) ([]reconcileResult, error) { fieldOwner := client.FieldOwner( @@ -144,8 +144,8 @@ func applyResources( var results []reconcileResult - for _, resource := range resources { - result, err := applyResource(ctx, rec, resource, fieldOwner, mapper) + for _, entry := range entries { + result, err := applyResource(ctx, rec, entry.Resource, fieldOwner, mapper, entry.Options.Unowned) if err != nil { return nil, err } @@ -205,7 +205,7 @@ func reconcileResources( if entry.Options.ReadOnly { result, err = readResource(ctx, rec, resource) } else { - result, err = applyResource(ctx, rec, resource, fieldOwner, mapper) + result, err = applyResource(ctx, rec, resource, fieldOwner, mapper, entry.Options.Unowned) } if err != nil { if entry.Options.ReadOnly && apierrors.IsNotFound(err) { @@ -242,15 +242,21 @@ func reconcileResources( return results, nil } -// mutateResource applies all desired-state mutations and sets the controller owner reference. +// 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. func mutateResource( resource Resource, obj client.Object, owner client.Object, - scheme *runtime.Scheme, mapper meta.RESTMapper, + scheme *runtime.Scheme, mapper meta.RESTMapper, skipOwnerRef bool, ) (ownerRefSkipped bool, err error) { if err := resource.Mutate(obj); err != nil { return false, err } + if skipOwnerRef { + return false, nil + } + canSet, err := scope.CanSetOwnerReference(owner, obj, scheme, mapper) if err != nil { return false, fmt.Errorf( diff --git a/pkg/component/create_test.go b/pkg/component/create_test.go index b9f46c25..c7f73683 100644 --- a/pkg/component/create_test.go +++ b/pkg/component/create_test.go @@ -59,7 +59,7 @@ func TestApplyResources(t *testing.T) { resource.On("Mutate", mock.Anything).Return(nil) // When - results, err := applyResources(ctx, reconcileContext, []Resource{resource}, "test-component", createTestRESTMapper()) + results, err := applyResources(ctx, reconcileContext, []reconcileEntry{{Resource: resource}}, "test-component", createTestRESTMapper()) // Then require.NoError(t, err) @@ -96,7 +96,7 @@ func TestApplyResources(t *testing.T) { resource2.On("Mutate", mock.Anything).Return(nil) // When - results, err := applyResources(ctx, reconcileContext, []Resource{resource1, resource2}, "test-component", createTestRESTMapper()) + results, err := applyResources(ctx, reconcileContext, []reconcileEntry{{Resource: resource1}, {Resource: resource2}}, "test-component", createTestRESTMapper()) // Then require.NoError(t, err) @@ -140,7 +140,7 @@ func TestApplyResources(t *testing.T) { }, nil) // When - results, err := applyResources(ctx, reconcileContext, []Resource{regularResource, aliveResource}, "test-component", createTestRESTMapper()) + results, err := applyResources(ctx, reconcileContext, []reconcileEntry{{Resource: regularResource}, {Resource: aliveResource}}, "test-component", createTestRESTMapper()) // Then require.NoError(t, err) @@ -170,7 +170,7 @@ func TestApplyResources(t *testing.T) { resource3 := &MockResource{} // Should not be processed // When - results, err := applyResources(ctx, reconcileContext, []Resource{resource1, resource2, resource3}, "test-component", createTestRESTMapper()) + results, err := applyResources(ctx, reconcileContext, []reconcileEntry{{Resource: resource1}, {Resource: resource2}, {Resource: resource3}}, "test-component", createTestRESTMapper()) // Then require.Error(t, err) @@ -211,7 +211,7 @@ func TestApplyResources(t *testing.T) { }).Return(nil) // When - results, err := applyResources(ctx, reconcileContext, []Resource{resource}, "test-component", createTestRESTMapper()) + results, err := applyResources(ctx, reconcileContext, []reconcileEntry{{Resource: resource}}, "test-component", createTestRESTMapper()) // Then require.NoError(t, err) @@ -276,7 +276,7 @@ func TestApplyResources(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - results, err := applyResources(ctx, reconcileContext, []Resource{tc.resource}, "test-component", createTestRESTMapper()) + results, err := applyResources(ctx, reconcileContext, []reconcileEntry{{Resource: tc.resource}}, "test-component", createTestRESTMapper()) require.NoError(t, err) require.Len(t, results, 1) @@ -291,7 +291,7 @@ func TestApplyResources(t *testing.T) { resource.On("Object").Return(nil, fmt.Errorf("object error")) // When - _, err := applyResources(ctx, reconcileContext, []Resource{resource}, "test-component", createTestRESTMapper()) + _, err := applyResources(ctx, reconcileContext, []reconcileEntry{{Resource: resource}}, "test-component", createTestRESTMapper()) // Then require.Error(t, err) @@ -312,7 +312,7 @@ func TestApplyResources(t *testing.T) { resource.On("Mutate", mock.Anything).Return(fmt.Errorf("mutation failed")) // When - _, err := applyResources(ctx, reconcileContext, []Resource{resource}, "test-component", createTestRESTMapper()) + _, err := applyResources(ctx, reconcileContext, []reconcileEntry{{Resource: resource}}, "test-component", createTestRESTMapper()) // Then require.Error(t, err) @@ -348,7 +348,7 @@ func TestMutateResource(t *testing.T) { resource.On("Identity").Maybe().Return("v1/ConfigMap/test-namespace/test-mutate") // When - _, err := mutateResource(resource, resourceObject, owner, scheme, mapper) + _, err := mutateResource(resource, resourceObject, owner, scheme, mapper, false) // Then require.NoError(t, err) @@ -372,13 +372,35 @@ func TestMutateResource(t *testing.T) { resource.On("Identity").Maybe().Return("v1/ConfigMap/test-namespace/test-mutate-existing") // When - _, err := mutateResource(resource, resourceObject, owner, scheme, mapper) + _, err := mutateResource(resource, resourceObject, owner, scheme, mapper, false) // Then require.NoError(t, err) resource.AssertExpectations(t) }) + t.Run("should not set owner reference when skipOwnerRef is true", func(t *testing.T) { + // Given + resource := &MockResource{} + resourceObject := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-unowned", + Namespace: namespace, + }, + } + resource.On("Mutate", mock.Anything).Return(nil) + resource.On("Identity").Maybe().Return("v1/ConfigMap/test-namespace/test-unowned") + + // When + skipped, err := mutateResource(resource, resourceObject, owner, scheme, mapper, true) + + // Then + require.NoError(t, err) + assert.False(t, skipped, "intentional Unowned skip must not be reported as a scope-incompatibility skip") + assert.Empty(t, resourceObject.OwnerReferences, "no owner reference should be set for an Unowned resource") + resource.AssertExpectations(t) + }) + t.Run("should skip owner reference for cluster-scoped resource with namespaced owner", func(t *testing.T) { // Given clusterScopedMapper := meta.NewDefaultRESTMapper([]schema.GroupVersion{ @@ -402,7 +424,7 @@ func TestMutateResource(t *testing.T) { resource.On("Identity").Maybe().Return("rbac.authorization.k8s.io/v1/ClusterRole/test-cluster-role") // When - skipped, err := mutateResource(resource, resourceObject, owner, scheme, clusterScopedMapper) + skipped, err := mutateResource(resource, resourceObject, owner, scheme, clusterScopedMapper, false) // Then require.NoError(t, err) @@ -453,7 +475,7 @@ func TestApplyResources_ClusterScopedResource(t *testing.T) { resource.On("Identity").Maybe().Return("rbac.authorization.k8s.io/v1/ClusterRole/test-cluster-role-create") // When - results, err := applyResources(ctx, reconcileContext, []Resource{resource}, "test-component", clusterScopedMapper) + results, err := applyResources(ctx, reconcileContext, []reconcileEntry{{Resource: resource}}, "test-component", clusterScopedMapper) // Then require.NoError(t, err) @@ -467,6 +489,50 @@ func TestApplyResources_ClusterScopedResource(t *testing.T) { }) } +func TestReconcileResources_Unowned(t *testing.T) { + var ( + scheme = setupScheme() + namespace = "test-namespace" + owner = &MockOperatorCRD{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-owner", + Namespace: namespace, + Generation: 1, + }, + } + fakeClient = fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner).WithStatusSubresource(owner).Build() + reconcileContext = setupReconcileContext(scheme, owner, fakeClient) + mapper = createTestRESTMapper() + ctx = t.Context() + ) + + t.Run("does not set owner reference on an Unowned resource", func(t *testing.T) { + resourceObject := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-unowned-cm", + Namespace: namespace, + }, + } + resource := &MockResource{} + resource.On("Object").Return(resourceObject, nil) + resource.On("Mutate", mock.Anything).Return(nil) + + entry := reconcileEntry{ + Resource: resource, + Options: resourceOptions{Unowned: true, ParticipationMode: ParticipationModeRequired}, + } + + _, err := reconcileResources(ctx, reconcileContext, []reconcileEntry{entry}, "comp", mapper) + require.NoError(t, err) + + created := &corev1.ConfigMap{} + err = fakeClient.Get(ctx, client.ObjectKey{Name: "test-unowned-cm", Namespace: namespace}, created) + require.NoError(t, err) + assert.Empty(t, created.OwnerReferences, "Unowned resource must not have an owner reference") + resource.AssertExpectations(t) + }) +} + func TestReconcileResources_BlockOnAbsence(t *testing.T) { var ( scheme = setupScheme() diff --git a/pkg/component/resource_options.go b/pkg/component/resource_options.go index 36608d5d..644363df 100644 --- a/pkg/component/resource_options.go +++ b/pkg/component/resource_options.go @@ -26,6 +26,7 @@ type ResourceOption func(*resourceConfig) // before resolution. type resourceConfig struct { readOnly bool + unowned bool deleteConditions []bool gate feature.Gate participationMode ParticipationMode @@ -43,6 +44,11 @@ type resourceOptions struct { Delete bool // ReadOnly reports that the resource is read-only. ReadOnly bool + // Unowned reports that the component must not set a controller owner reference + // on this resource. The resource is still created and updated normally, but + // Kubernetes will not garbage-collect it when the owner CR is deleted. Use this + // for resources that must outlive the owner, such as backup records. + Unowned bool // ParticipationMode describes how the resource participates in the component // health aggregation. Defaults to ParticipationModeRequired. ParticipationMode ParticipationMode @@ -125,6 +131,15 @@ func IgnoreIfAbsent() ResourceOption { return func(c *resourceConfig) { c.ignoreIfAbsent = true } } +// Unowned marks the resource as unowned: the component creates and updates it +// normally, but does not set a controller owner reference. Without an owner +// reference, Kubernetes will not garbage-collect the resource when the owner CR +// is deleted. Use this for resources that must outlive the owner, such as backup +// records. +func Unowned() ResourceOption { + return func(c *resourceConfig) { c.unowned = 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. @@ -205,6 +220,7 @@ func (c *resourceConfig) resolve() (resourceOptions, error) { return resourceOptions{ Delete: shouldDelete, ReadOnly: c.readOnly, + Unowned: c.unowned, ParticipationMode: mode, SuppressGraceInconsistencyWarning: c.suppressGraceInconsistencyWarning, BlockOnAbsence: c.blockOnAbsence, diff --git a/pkg/component/resource_options_test.go b/pkg/component/resource_options_test.go index 99edaf39..516805e3 100644 --- a/pkg/component/resource_options_test.go +++ b/pkg/component/resource_options_test.go @@ -128,6 +128,16 @@ func TestResolveResourceOptions(t *testing.T) { opts: []ResourceOption{GatedBy(&disabledFeature{}), GatedBy(&enabledFeature{})}, want: resourceOptions{ParticipationMode: ParticipationModeRequired}, }, + { + name: "Unowned sets unowned flag", + opts: []ResourceOption{Unowned()}, + want: resourceOptions{Unowned: true, ParticipationMode: ParticipationModeRequired}, + }, + { + name: "Unowned combined with Auxiliary", + opts: []ResourceOption{Unowned(), Auxiliary()}, + want: resourceOptions{Unowned: true, ParticipationMode: ParticipationModeAuxiliary}, + }, } for _, tt := range tests { diff --git a/pkg/component/suspend.go b/pkg/component/suspend.go index b2cd53b0..b82a98ca 100644 --- a/pkg/component/suspend.go +++ b/pkg/component/suspend.go @@ -48,15 +48,15 @@ func (s suspensionResults) summary() concepts.SuspensionStatusWithReason { // It ensures that suspension mutations are applied and tracks the progress of each resource. // If any resource fails to suspend, all encountered errors are joined and returned. func suspendResources( - ctx context.Context, rec ReconcileContext, resources []Resource, + ctx context.Context, rec ReconcileContext, entries []reconcileEntry, componentName string, mapper meta.RESTMapper, ) ([]concepts.SuspensionStatusWithReason, error) { var results []concepts.SuspensionStatusWithReason var errs []error - for _, resource := range resources { - if suspendable, ok := resource.(concepts.Suspendable); ok { - status, err := suspendResource(ctx, rec, resource, suspendable, componentName, mapper) + for _, entry := range entries { + if suspendable, ok := entry.Resource.(concepts.Suspendable); ok { + status, err := suspendResource(ctx, rec, entry, suspendable, componentName, mapper) if err != nil { // gather the errors to suspend as many resources as possible errs = append(errs, err) @@ -98,9 +98,11 @@ func suspendResources( // - Deletion is deferred until the Suspended state is reached to allow for graceful // shutdown or final state persistence (e.g., via finalizers or pre-stop hooks). func suspendResource( - ctx context.Context, rec ReconcileContext, resource Resource, suspendable concepts.Suspendable, + ctx context.Context, rec ReconcileContext, entry reconcileEntry, suspendable concepts.Suspendable, componentName string, mapper meta.RESTMapper, ) (concepts.SuspensionStatusWithReason, error) { + resource := entry.Resource + // Get the object if possible object, err := resource.Object() if err != nil { @@ -137,7 +139,7 @@ func suspendResource( } // Apply suspension mutation (if any) - _, err = applyResources(ctx, rec, []Resource{resource}, componentName, mapper) + _, err = applyResources(ctx, rec, []reconcileEntry{entry}, componentName, mapper) if err != nil { return concepts.SuspensionStatusWithReason{}, fmt.Errorf( "failed to create or update resource %s on suspension: %w", resource.Identity(), err, diff --git a/pkg/component/suspend_test.go b/pkg/component/suspend_test.go index 2d1fef33..661815bc 100644 --- a/pkg/component/suspend_test.go +++ b/pkg/component/suspend_test.go @@ -158,7 +158,7 @@ func TestSuspendResources(t *testing.T) { cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj1, obj2).Build() rec := setupReconcileContext(scheme, owner, cli) - resources := []Resource{r1, r2, nonSuspendable} + resources := []reconcileEntry{{Resource: r1}, {Resource: r2}, {Resource: nonSuspendable}} results, err := suspendResources(ctx, rec, resources, "test-component", testRESTMapper()) require.NoError(t, err) @@ -180,7 +180,7 @@ func TestSuspendResources(t *testing.T) { r2.On("DeleteOnSuspend").Return(false) r2.On("Suspend").Return(errors.New("fail2")) - resources := []Resource{r1, r2} + resources := []reconcileEntry{{Resource: r1}, {Resource: r2}} results, err := suspendResources(ctx, rec, resources, "test-component", testRESTMapper()) require.Error(t, err) @@ -193,7 +193,7 @@ func TestSuspendResources(t *testing.T) { rec := setupReconcileContext(scheme, nil, &MockClient{}) nonSuspendable := &MockResource{} - resources := []Resource{nonSuspendable} + resources := []reconcileEntry{{Resource: nonSuspendable}} results, err := suspendResources(ctx, rec, resources, "test-component", testRESTMapper()) require.NoError(t, err) @@ -201,6 +201,32 @@ func TestSuspendResources(t *testing.T) { }) } +func TestSuspendResource_Unowned(t *testing.T) { + ctx := t.Context() + scheme := setupScheme() + + t.Run("does not set owner reference on Unowned resource during suspension", func(t *testing.T) { + owner := setupTestOwner() + res, obj := setupMockResource("cm-unowned-suspend", concepts.SuspensionStatusSuspended, "Done", false) + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner).Build() + rec := setupReconcileContext(scheme, owner, cli) + + entry := reconcileEntry{ + Resource: res, + Options: resourceOptions{Unowned: true, ParticipationMode: ParticipationModeRequired}, + } + + status, err := suspendResource(ctx, rec, entry, res, "test-component", testRESTMapper()) + require.NoError(t, err) + assert.Equal(t, concepts.SuspensionStatusSuspended, status.Status) + + created := &v1.ConfigMap{} + err = cli.Get(ctx, client.ObjectKeyFromObject(obj), created) + require.NoError(t, err) + assert.Empty(t, created.OwnerReferences, "Unowned resource must not have an owner reference during suspension") + }) +} + func TestSuspendResource(t *testing.T) { ctx := t.Context() scheme := setupScheme() @@ -211,7 +237,7 @@ func TestSuspendResource(t *testing.T) { cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj).Build() rec := setupReconcileContext(scheme, owner, cli) - status, err := suspendResource(ctx, rec, res, res, "test-component", testRESTMapper()) + status, err := suspendResource(ctx, rec, reconcileEntry{Resource: res}, res, "test-component", testRESTMapper()) require.NoError(t, err) assert.Equal(t, concepts.SuspensionStatusSuspended, status.Status) @@ -226,7 +252,7 @@ func TestSuspendResource(t *testing.T) { cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj).Build() rec := setupReconcileContext(scheme, owner, cli) - status, err := suspendResource(ctx, rec, res, res, "test-component", testRESTMapper()) + status, err := suspendResource(ctx, rec, reconcileEntry{Resource: res}, res, "test-component", testRESTMapper()) require.NoError(t, err) assert.Equal(t, concepts.SuspensionStatusSuspended, status.Status) @@ -256,7 +282,7 @@ func TestSuspendResource(t *testing.T) { cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(owner, obj).Build() rec := setupReconcileContext(scheme, owner, cli) - status, err := suspendResource(ctx, rec, res, res, "test-component", testRESTMapper()) + status, err := suspendResource(ctx, rec, reconcileEntry{Resource: res}, res, "test-component", testRESTMapper()) require.NoError(t, err) assert.Equal(t, concepts.SuspensionStatusSuspending, status.Status) @@ -276,7 +302,7 @@ func TestSuspendResource(t *testing.T) { cli.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) cli.On("Delete", ctx, obj, mock.Anything).Return(apierrors.NewNotFound(schema.GroupResource{}, "cm")) - status, err := suspendResource(ctx, rec, res, res, "test-component", testRESTMapper()) + status, err := suspendResource(ctx, rec, reconcileEntry{Resource: res}, res, "test-component", testRESTMapper()) require.NoError(t, err) assert.Equal(t, concepts.SuspensionStatusSuspended, status.Status) }) @@ -290,7 +316,7 @@ func TestSuspendResource(t *testing.T) { res.On("DeleteOnSuspend").Return(false) res.On("Suspend").Return(errors.New("suspend fail")) - _, err := suspendResource(ctx, rec, res, res, "test-component", testRESTMapper()) + _, err := suspendResource(ctx, rec, reconcileEntry{Resource: res}, res, "test-component", testRESTMapper()) require.Error(t, err) assert.Contains(t, err.Error(), "suspend fail") }) @@ -301,7 +327,7 @@ func TestSuspendResource(t *testing.T) { res := &MockSuspendableResource{} res.On("Object").Return(nil, errors.New("object fail")) - _, err := suspendResource(ctx, rec, res, res, "test-component", testRESTMapper()) + _, err := suspendResource(ctx, rec, reconcileEntry{Resource: res}, res, "test-component", testRESTMapper()) require.Error(t, err) assert.Contains(t, err.Error(), "object fail") }) @@ -313,7 +339,7 @@ func TestSuspendResource(t *testing.T) { cli.On("Get", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(errors.New("get fail")) - _, err := suspendResource(ctx, rec, res, res, "test-component", testRESTMapper()) + _, err := suspendResource(ctx, rec, reconcileEntry{Resource: res}, res, "test-component", testRESTMapper()) require.Error(t, err) assert.Contains(t, err.Error(), "get fail") }) @@ -329,7 +355,7 @@ func TestSuspendResource(t *testing.T) { res.On("SuspensionStatus").Unset() res.On("SuspensionStatus").Return(concepts.SuspensionStatusWithReason{}, errors.New("status fail")) - _, err := suspendResource(ctx, rec, res, res, "test-component", testRESTMapper()) + _, err := suspendResource(ctx, rec, reconcileEntry{Resource: res}, res, "test-component", testRESTMapper()) require.Error(t, err) assert.Contains(t, err.Error(), "status fail") }) @@ -343,7 +369,7 @@ func TestSuspendResource(t *testing.T) { cli.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) cli.On("Delete", ctx, obj, mock.Anything).Return(errors.New("delete fail")) - _, err := suspendResource(ctx, rec, res, res, "test-component", testRESTMapper()) + _, err := suspendResource(ctx, rec, reconcileEntry{Resource: res}, res, "test-component", testRESTMapper()) require.Error(t, err) assert.Contains(t, err.Error(), "delete fail") }) @@ -357,7 +383,7 @@ func TestSuspendResource(t *testing.T) { cli.On("Patch", mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil) cli.On("Delete", ctx, obj, mock.Anything).Return(apierrors.NewNotFound(schema.GroupResource{Group: "v1", Resource: "ConfigMap"}, "cm")) - status, err := suspendResource(ctx, rec, res, res, "test-component", testRESTMapper()) + status, err := suspendResource(ctx, rec, reconcileEntry{Resource: res}, res, "test-component", testRESTMapper()) require.NoError(t, err) assert.Equal(t, concepts.SuspensionStatusSuspended, status.Status) cli.AssertCalled(t, "Delete", ctx, obj, mock.Anything) @@ -371,7 +397,7 @@ func TestSuspendResource(t *testing.T) { cli.On("Get", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(apierrors.NewNotFound(schema.GroupResource{Group: "", Resource: "configmaps"}, "cm")) - status, err := suspendResource(ctx, rec, res, res, "test-component", nil) + status, err := suspendResource(ctx, rec, reconcileEntry{Resource: res}, res, "test-component", nil) require.NoError(t, err) assert.Equal(t, concepts.SuspensionStatusSuspended, status.Status) assert.Contains(t, status.Reason, "already deleted") @@ -392,7 +418,7 @@ func TestSuspendResource(t *testing.T) { cli.On("Get", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return(errors.New("network error")) - _, err := suspendResource(ctx, rec, res, res, "test-component", nil) + _, err := suspendResource(ctx, rec, reconcileEntry{Resource: res}, res, "test-component", nil) require.Error(t, err) assert.Contains(t, err.Error(), "network error") }) From f6cbbc8936ca0183bdb9bf4245c5de5b25e91f72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Thu, 25 Jun 2026 19:07:32 +0200 Subject: [PATCH 2/3] fix(component): clear cached owner refs on Unowned resources; fix docs wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When `Unowned()` is set and an owner reference was present from a previous reconcile (the DesiredObject pointer retains the server response), SSA would re-apply the cached reference. Clear it explicitly before patching so SSA removes any entry this field manager previously owned. Also corrects the resource-options table in docs/component.md: "no owner reference is set" → "no controller owner reference is set", matching the actual implementation which only skips `ctrl.SetControllerReference`. Co-Authored-By: Claude Sonnet 4.6 --- docs/component.md | 2 +- pkg/component/create.go | 5 +++++ pkg/component/create_test.go | 26 ++++++++++++++++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) diff --git a/docs/component.md b/docs/component.md index c89a1240..b4fba04f 100644 --- a/docs/component.md +++ b/docs/component.md @@ -57,7 +57,7 @@ be passed without a guard. | `component.Delete()` / `component.DeleteWhen(cond)` | **Delete**: removed from the cluster (unconditionally, or when `cond` is true); does not contribute to health | | `component.GatedBy(gate)` | Deletes the resource when the feature gate is disabled; managed when enabled | | `component.OrphanWhen(cond)` | **Orphan**: when `cond` is true, removes the component's owner reference and stops managing the resource, leaving the object in the cluster; does not contribute to health. Mutually exclusive with the deletion options and `ReadOnly` | -| `component.Unowned()` | **Unowned**: created and updated normally, but no owner reference is set; not garbage-collected on owner CR deletion | +| `component.Unowned()` | **Unowned**: created and updated normally, but no controller owner reference is set; not garbage-collected on owner CR deletion | | `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 | diff --git a/pkg/component/create.go b/pkg/component/create.go index 1c8724dc..a484db09 100644 --- a/pkg/component/create.go +++ b/pkg/component/create.go @@ -254,6 +254,11 @@ func mutateResource( } if skipOwnerRef { + // Clear any owner references cached from a previous reconcile (the + // DesiredObject pointer retains the server response, which may include an + // owner ref set before Unowned() was added). Sending nil via SSA removes + // any entry this field manager previously owned. + obj.SetOwnerReferences(nil) return false, nil } diff --git a/pkg/component/create_test.go b/pkg/component/create_test.go index c7f73683..5ac6ddad 100644 --- a/pkg/component/create_test.go +++ b/pkg/component/create_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" ) @@ -401,6 +402,31 @@ func TestMutateResource(t *testing.T) { resource.AssertExpectations(t) }) + t.Run("should clear a previously-cached owner reference when skipOwnerRef is true", func(t *testing.T) { + // The DesiredObject pointer retains the owner ref written back by the server + // after the first reconcile (Patch writes into the same pointer). If Unowned() + // is added later, that cached ref must be cleared so the SSA patch does not + // re-apply it. + resource := &MockResource{} + resourceObject := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-unowned-cached", + Namespace: namespace, + OwnerReferences: []metav1.OwnerReference{ + {APIVersion: "test/v1", Kind: "MockOperatorCRD", Name: owner.Name, Controller: ptr.To(true)}, + }, + }, + } + resource.On("Mutate", mock.Anything).Return(nil) + resource.On("Identity").Maybe().Return("v1/ConfigMap/test-namespace/test-unowned-cached") + + _, err := mutateResource(resource, resourceObject, owner, scheme, mapper, true) + + require.NoError(t, err) + assert.Empty(t, resourceObject.OwnerReferences, "cached owner reference must be cleared for an Unowned resource") + resource.AssertExpectations(t) + }) + t.Run("should skip owner reference for cluster-scoped resource with namespaced owner", func(t *testing.T) { // Given clusterScopedMapper := meta.NewDefaultRESTMapper([]schema.GroupVersion{ From 8819491ba582eeab9c04ab50d32956ccd01c5e52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Thu, 25 Jun 2026 19:18:57 +0200 Subject: [PATCH 3/3] fix(component): filter owner ref by UID rather than clearing all refs SetOwnerReferences(nil) cleared every owner reference on the object, including refs that Mutate() may have set for a different owner. Replace it with an in-place filter that removes only the entry whose UID matches the component owner, preserving any other owner references intact. Co-Authored-By: Claude Sonnet 4.6 --- pkg/component/create.go | 20 ++++++++++++----- pkg/component/create_test.go | 42 ++++++++++++++++++++++++++++++++++-- 2 files changed, 55 insertions(+), 7 deletions(-) diff --git a/pkg/component/create.go b/pkg/component/create.go index a484db09..11b1969a 100644 --- a/pkg/component/create.go +++ b/pkg/component/create.go @@ -254,11 +254,21 @@ func mutateResource( } if skipOwnerRef { - // Clear any owner references cached from a previous reconcile (the - // DesiredObject pointer retains the server response, which may include an - // owner ref set before Unowned() was added). Sending nil via SSA removes - // any entry this field manager previously owned. - obj.SetOwnerReferences(nil) + // Remove only the owner reference that points to the component owner, preserving + // any other owner references that Mutate() may have set for different objects. + // This also clears a cached ref from a previous reconcile where Unowned() was not set + // (the DesiredObject pointer retains the server response). Omitting or emptying the + // field in the SSA patch removes entries this field manager previously owned. + ownerUID := owner.GetUID() + existing := obj.GetOwnerReferences() + n := 0 + for _, ref := range existing { + if ref.UID != ownerUID { + existing[n] = ref + n++ + } + } + obj.SetOwnerReferences(existing[:n]) return false, nil } diff --git a/pkg/component/create_test.go b/pkg/component/create_test.go index 5ac6ddad..6f9980a4 100644 --- a/pkg/component/create_test.go +++ b/pkg/component/create_test.go @@ -407,26 +407,64 @@ func TestMutateResource(t *testing.T) { // after the first reconcile (Patch writes into the same pointer). If Unowned() // is added later, that cached ref must be cleared so the SSA patch does not // re-apply it. + localOwner := &MockOperatorCRD{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-owner-cached", + Namespace: namespace, + UID: "owner-uid-cached", + }, + } resource := &MockResource{} resourceObject := &corev1.ConfigMap{ ObjectMeta: metav1.ObjectMeta{ Name: "test-unowned-cached", Namespace: namespace, OwnerReferences: []metav1.OwnerReference{ - {APIVersion: "test/v1", Kind: "MockOperatorCRD", Name: owner.Name, Controller: ptr.To(true)}, + {APIVersion: "test/v1", Kind: "MockOperatorCRD", Name: localOwner.Name, UID: localOwner.UID, Controller: ptr.To(true)}, }, }, } resource.On("Mutate", mock.Anything).Return(nil) resource.On("Identity").Maybe().Return("v1/ConfigMap/test-namespace/test-unowned-cached") - _, err := mutateResource(resource, resourceObject, owner, scheme, mapper, true) + _, err := mutateResource(resource, resourceObject, localOwner, scheme, mapper, true) require.NoError(t, err) assert.Empty(t, resourceObject.OwnerReferences, "cached owner reference must be cleared for an Unowned resource") resource.AssertExpectations(t) }) + t.Run("should preserve ownerReferences to other objects when skipOwnerRef is true", func(t *testing.T) { + // Mutate() may add owner references to objects other than the component's owner + // CR (e.g., ownership by a different controller). Those must be preserved; only + // the reference pointing to the component owner should be suppressed. + resource := &MockResource{} + otherRef := metav1.OwnerReference{ + APIVersion: "v1", + Kind: "Pod", + Name: "other-owner", + UID: "other-owner-uid", + } + resourceObject := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-unowned-other-refs", + Namespace: namespace, + }, + } + resource.On("Mutate", mock.Anything).Run(func(args mock.Arguments) { + obj := args.Get(0).(client.Object) + obj.SetOwnerReferences([]metav1.OwnerReference{otherRef}) + }).Return(nil) + resource.On("Identity").Maybe().Return("v1/ConfigMap/test-namespace/test-unowned-other-refs") + + _, err := mutateResource(resource, resourceObject, owner, scheme, mapper, true) + + require.NoError(t, err) + assert.Equal(t, []metav1.OwnerReference{otherRef}, resourceObject.OwnerReferences, + "ownerReferences to other objects set in Mutate must be preserved") + resource.AssertExpectations(t) + }) + t.Run("should skip owner reference for cluster-scoped resource with namespaced owner", func(t *testing.T) { // Given clusterScopedMapper := meta.NewDefaultRESTMapper([]schema.GroupVersion{