Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions docs/component.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +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 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 |
Expand All @@ -67,6 +68,13 @@ A read-only resource is not owned by the component, so it is never deleted. `Rea
`IgnoreIfAbsent()` each require `ReadOnly()` and are mutually exclusive with each other. To conditionally include a
read-only resource, use [`IncludeWhen`](#includewhen-vs-gatedby), 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.

Options compose. Gate a resource and exclude it from health aggregation in one call:

```go
Expand Down
8 changes: 4 additions & 4 deletions pkg/component/component.go
Original file line number Diff line number Diff line change
Expand Up @@ -465,13 +465,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
Expand Down
37 changes: 29 additions & 8 deletions pkg/component/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
Expand Down Expand Up @@ -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(
Expand All @@ -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
}
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -242,15 +242,36 @@ 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 {
// 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
}
Comment on lines +256 to +273

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in f6cbbc8. Added obj.SetOwnerReferences(nil) before returning in the skipOwnerRef branch so any owner reference cached from a previous reconcile is cleared. SSA then removes the entry this field manager previously owned. Test added: "should clear a previously-cached owner reference when skipOwnerRef is true".


canSet, err := scope.CanSetOwnerReference(owner, obj, scheme, mapper)
if err != nil {
return false, fmt.Errorf(
Expand Down
154 changes: 142 additions & 12 deletions pkg/component/create_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -59,7 +60,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)
Expand Down Expand Up @@ -96,7 +97,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)
Expand Down Expand Up @@ -140,7 +141,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)
Expand Down Expand Up @@ -170,7 +171,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)
Expand Down Expand Up @@ -211,7 +212,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)
Expand Down Expand Up @@ -276,7 +277,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)
Expand All @@ -291,7 +292,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)
Expand All @@ -312,7 +313,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)
Expand Down Expand Up @@ -348,7 +349,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)
Expand All @@ -372,13 +373,98 @@ 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 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.
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: 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, 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{
Expand All @@ -402,7 +488,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)
Expand Down Expand Up @@ -453,7 +539,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)
Expand All @@ -467,6 +553,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()
Expand Down
Loading
Loading