From b3527cfb86fd7b28c543738fb5c379e1eda0b1c4 Mon Sep 17 00:00:00 2001 From: "Per G. da Silva" Date: Wed, 28 Oct 2026 16:22:49 +0100 Subject: [PATCH 1/3] =?UTF-8?q?=E2=9C=A8=20Adopt=20ClusterObjectDeployment?= =?UTF-8?q?=20only=20when=20not=20controlled=20by=20another=20owner?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The applier now refuses to take over a ClusterObjectDeployment that already carries a controller ownerReference belonging to a different owner, reporting the ClusterExtension as Progressing=False/Blocked instead of fighting that owner (which the apiserver rejects, causing a retry loop). A COD with no controller ownerReference, or one already controlled by this ClusterExtension, is still adopted. Co-Authored-By: Claude Opus 4.8 Signed-off-by: Per G. da Silva --- .../operator-controller/applier/boxcutter.go | 39 ++++++++ .../applier/boxcutter_test.go | 89 +++++++++++++++++++ .../clusterextension_controller.go | 7 ++ 3 files changed, 135 insertions(+) diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index b2621f11d9..7a94c3dd34 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -24,6 +24,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/apiutil" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/yaml" helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client" @@ -465,6 +466,15 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust } } + // Refuse to adopt a ClusterObjectDeployment that is already controlled by a + // different owner. We adopt one that has no controller (stamping our controller + // ownerReference via the apply below) or one we already control, but taking over a + // deployment controlled by someone else would fight that owner. A plain terminal + // error surfaces this as Progressing=False with the Blocked reason. + if err := bc.checkAdoptable(ctx, ext); err != nil { + return false, "", err + } + // Generate the desired ClusterObjectDeployment and the Secrets its template refs. cod, secrets, err := bc.RevisionGenerator.GenerateClusterObjectDeployment(ctx, contentFS, ext, objectLabels, revisionAnnotations) if err != nil { @@ -522,6 +532,35 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust return true, "", nil } +// checkAdoptable verifies that the applier may own the ClusterObjectDeployment named +// after the ClusterExtension. It is adoptable when it does not yet exist, has no +// controller ownerReference, is already controlled by this ClusterExtension, or is +// controlled by a reference that matches this ClusterExtension by group, kind, and name +// but with a different UID — the latter indicates the ClusterExtension was deleted and +// recreated (for example a backup/restore), which comes back with a fresh UID, so the +// deployment is logically still ours. When it is controlled by a genuinely different +// owner, it returns a terminal error (no specific reason, so the ClusterExtension reports +// Progressing=False with the Blocked reason) rather than fighting that owner for control. +func (bc *Boxcutter) checkAdoptable(ctx context.Context, ext *ocv1.ClusterExtension) error { + existing := &ocv1.ClusterObjectDeployment{} + switch err := bc.Client.Get(ctx, client.ObjectKey{Name: ext.Name}, existing); { + case apierrors.IsNotFound(err): + return nil + case err != nil: + return fmt.Errorf("getting ClusterObjectDeployment %q: %w", ext.Name, err) + } + + controller := metav1.GetControllerOf(existing) + if controller == nil || controller.UID == ext.UID { + return nil + } + + return reconcile.TerminalError(fmt.Errorf( + "ClusterObjectDeployment %q is already controlled by %s %q; refusing to adopt it", + ext.Name, controller.Kind, controller.Name, + )) +} + // runPreflights runs the configured preflight checks for a desired apply. It determines whether // this is an install or an upgrade from whether the ClusterObjectDeployment already exists (a // direct, controller-owned signal, rather than our own ClusterExtension status which may be stale diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index 6d7351f403..64d0b3f509 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -18,12 +18,14 @@ import ( "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/reconcile" ocv1 "github.com/operator-framework/operator-controller/api/v1" ocv1ac "github.com/operator-framework/operator-controller/applyconfigurations/api/v1" coscontrollers "github.com/operator-framework/operator-controller/internal/object-controller/controllers" "github.com/operator-framework/operator-controller/internal/operator-controller/applier" "github.com/operator-framework/operator-controller/internal/shared/labels" + errorutil "github.com/operator-framework/operator-controller/internal/shared/util/error" bundlecsv "github.com/operator-framework/operator-controller/internal/testing/bundle/csv" bundlefs "github.com/operator-framework/operator-controller/internal/testing/bundle/fs" "github.com/operator-framework/operator-controller/internal/testutil" @@ -869,6 +871,93 @@ func TestBoxcutterApply_NilContentUsesExistingCOD(t *testing.T) { }) } +// TestBoxcutterApply_Adoption verifies the applier only adopts an existing +// ClusterObjectDeployment when it is not already controlled by a different owner. +func TestBoxcutterApply_Adoption(t *testing.T) { + scheme := codTestScheme(t) + ext := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "test-uid"}, + } + objs := []client.Object{ + &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "test-cm", Namespace: "test-ns"}}, + } + + t.Run("adopts an existing deployment with no controller owner reference", func(t *testing.T) { + existing := &ocv1.ClusterObjectDeployment{ + ObjectMeta: metav1.ObjectMeta{Name: ext.Name}, + } + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existing).Build() + bc := newCODBoxcutter(t, scheme, cl, objs) + + completed, status, err := bc.Apply(t.Context(), dummyBundle, ext, map[string]string{"some": "label"}, map[string]string{}) + require.NoError(t, err) + require.True(t, completed) + require.Empty(t, status) + + cod := &ocv1.ClusterObjectDeployment{} + require.NoError(t, cl.Get(t.Context(), client.ObjectKey{Name: ext.Name}, cod)) + owner := metav1.GetControllerOf(cod) + require.NotNil(t, owner, "expected a controller owner reference after adoption") + assert.Equal(t, ext.Name, owner.Name) + assert.Equal(t, ext.UID, owner.UID) + assert.Equal(t, ocv1.ClusterExtensionKind, owner.Kind) + }) + + t.Run("proceeds when the existing deployment is already controlled by this extension", func(t *testing.T) { + existing := &ocv1.ClusterObjectDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: ext.Name, + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(ext, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind))}, + }, + } + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existing).Build() + bc := newCODBoxcutter(t, scheme, cl, objs) + + completed, status, err := bc.Apply(t.Context(), dummyBundle, ext, map[string]string{"some": "label"}, map[string]string{}) + require.NoError(t, err) + require.True(t, completed) + require.Empty(t, status) + }) + + t.Run("refuses to adopt a deployment controlled by a different owner", func(t *testing.T) { + foreign := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "other-ext", UID: "other-uid"}} + existing := &ocv1.ClusterObjectDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: ext.Name, + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(foreign, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind))}, + }, + } + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existing).Build() + bc := newCODBoxcutter(t, scheme, cl, objs) + + completed, status, err := bc.Apply(t.Context(), dummyBundle, ext, map[string]string{"some": "label"}, map[string]string{}) + require.Error(t, err) + assert.False(t, completed) + assert.Empty(t, status) + + t.Log("by surfacing a terminal error so Progressing becomes False") + require.ErrorIs(t, err, reconcile.TerminalError(nil), "error must be terminal") + t.Log("by carrying no specific reason so it falls back to the Blocked Progressing reason") + _, ok := errorutil.ExtractTerminalReason(err) + assert.False(t, ok, "terminal error should fall back to the Blocked reason") + t.Log("by naming the deployment and the conflicting owner in the message") + assert.Contains(t, err.Error(), ext.Name) + assert.Contains(t, err.Error(), "other-ext") + + t.Log("by leaving the foreign-owned deployment untouched") + cod := &ocv1.ClusterObjectDeployment{} + require.NoError(t, cl.Get(t.Context(), client.ObjectKey{Name: ext.Name}, cod)) + owner := metav1.GetControllerOf(cod) + require.NotNil(t, owner) + assert.Equal(t, "other-ext", owner.Name) + + t.Log("by not creating any packed Secrets for the refused deployment") + secretList := &corev1.SecretList{} + require.NoError(t, cl.List(t.Context(), secretList, client.InNamespace("olmv1-system"))) + assert.Empty(t, secretList.Items) + }) +} + // TestBoxcutterApply_Preflights verifies which preflight hook runs for each rollout state // and that a preflight failure propagates out of Apply. Each row is a sequence of applies // against the same Boxcutter: a step renders its own objects, so identical content between diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index ae6531f2e6..78d86eabfd 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -429,6 +429,13 @@ func collectDeprecationMessages(entries []declcfg.DeprecationEntry) []string { // for each ClusterExtension. The ClusterExtension observes the deployment's rolled-up rollout status, // and the deployment is controller-owned by the ClusterExtension, so an owner watch re-reconciles the // ClusterExtension whenever its deployment's status changes. +// +// TODO: the owner watch only enqueues the ClusterExtension that controls a deployment. When the applier +// refuses to adopt a deployment controlled by a different owner (see Boxcutter.checkAdoptable), the +// blocked ClusterExtension does not own that deployment, so deleting the conflicting deployment does not +// enqueue it; it recovers only on its next spec/catalog change or the periodic resync. Add a name-keyed +// watch that enqueues the same-named ClusterExtension on any ClusterObjectDeployment change so a blocked +// extension recovers promptly once the conflict clears. func (r *ClusterExtensionReconciler) SetupWithManagerForBoxcutter(mgr ctrl.Manager) (crcontroller.Controller, error) { return r.newControllerBuilder(mgr). Owns(&ocv1.ClusterObjectDeployment{}). From d2bd67f1b66f24778dcbb62b183592fef377b7ad Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Wed, 28 Oct 2026 16:02:04 -0400 Subject: [PATCH 2/3] =?UTF-8?q?=E2=9C=A8=20Retry=20ClusterObjectDeployment?= =?UTF-8?q?=20adoption=20conflicts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Return a non-terminal error when a ClusterObjectDeployment is controlled by a different owner. This reports the ClusterExtension as Progressing=True/Retrying and lets controller-runtime retry with its standard backoff. Use isolated table-driven test cases to cover adoption outcomes. --- .../operator-controller/applier/boxcutter.go | 13 +- .../applier/boxcutter_test.go | 158 +++++++++--------- .../clusterextension_controller.go | 7 - 3 files changed, 88 insertions(+), 90 deletions(-) diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 7a94c3dd34..3abe5e9223 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -24,7 +24,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/apiutil" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/yaml" helmclient "github.com/operator-framework/helm-operator-plugins/pkg/client" @@ -469,8 +468,9 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust // Refuse to adopt a ClusterObjectDeployment that is already controlled by a // different owner. We adopt one that has no controller (stamping our controller // ownerReference via the apply below) or one we already control, but taking over a - // deployment controlled by someone else would fight that owner. A plain terminal - // error surfaces this as Progressing=False with the Blocked reason. + // deployment controlled by someone else would fight that owner. Returning an + // ordinary error lets the ClusterExtension report Progressing=True/Retrying and + // use the normal controller retry backoff. if err := bc.checkAdoptable(ctx, ext); err != nil { return false, "", err } @@ -539,8 +539,7 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust // but with a different UID — the latter indicates the ClusterExtension was deleted and // recreated (for example a backup/restore), which comes back with a fresh UID, so the // deployment is logically still ours. When it is controlled by a genuinely different -// owner, it returns a terminal error (no specific reason, so the ClusterExtension reports -// Progressing=False with the Blocked reason) rather than fighting that owner for control. +// owner, it returns an error rather than fighting that owner for control. func (bc *Boxcutter) checkAdoptable(ctx context.Context, ext *ocv1.ClusterExtension) error { existing := &ocv1.ClusterObjectDeployment{} switch err := bc.Client.Get(ctx, client.ObjectKey{Name: ext.Name}, existing); { @@ -555,10 +554,10 @@ func (bc *Boxcutter) checkAdoptable(ctx context.Context, ext *ocv1.ClusterExtens return nil } - return reconcile.TerminalError(fmt.Errorf( + return fmt.Errorf( "ClusterObjectDeployment %q is already controlled by %s %q; refusing to adopt it", ext.Name, controller.Kind, controller.Name, - )) + ) } // runPreflights runs the configured preflight checks for a desired apply. It determines whether diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index 64d0b3f509..4bdb426929 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -25,7 +25,6 @@ import ( coscontrollers "github.com/operator-framework/operator-controller/internal/object-controller/controllers" "github.com/operator-framework/operator-controller/internal/operator-controller/applier" "github.com/operator-framework/operator-controller/internal/shared/labels" - errorutil "github.com/operator-framework/operator-controller/internal/shared/util/error" bundlecsv "github.com/operator-framework/operator-controller/internal/testing/bundle/csv" bundlefs "github.com/operator-framework/operator-controller/internal/testing/bundle/fs" "github.com/operator-framework/operator-controller/internal/testutil" @@ -874,88 +873,95 @@ func TestBoxcutterApply_NilContentUsesExistingCOD(t *testing.T) { // TestBoxcutterApply_Adoption verifies the applier only adopts an existing // ClusterObjectDeployment when it is not already controlled by a different owner. func TestBoxcutterApply_Adoption(t *testing.T) { - scheme := codTestScheme(t) - ext := &ocv1.ClusterExtension{ - ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "test-uid"}, - } - objs := []client.Object{ - &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "test-cm", Namespace: "test-ns"}}, + type testCase struct { + name string + ext *ocv1.ClusterExtension + cod *ocv1.ClusterObjectDeployment + assert func(t *testing.T, cl client.Client, applyCompleted bool, applyStatus string, applyErr error) + wantExtControllerOwner bool } - t.Run("adopts an existing deployment with no controller owner reference", func(t *testing.T) { - existing := &ocv1.ClusterObjectDeployment{ - ObjectMeta: metav1.ObjectMeta{Name: ext.Name}, - } - cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existing).Build() - bc := newCODBoxcutter(t, scheme, cl, objs) - - completed, status, err := bc.Apply(t.Context(), dummyBundle, ext, map[string]string{"some": "label"}, map[string]string{}) - require.NoError(t, err) - require.True(t, completed) - require.Empty(t, status) - - cod := &ocv1.ClusterObjectDeployment{} - require.NoError(t, cl.Get(t.Context(), client.ObjectKey{Name: ext.Name}, cod)) - owner := metav1.GetControllerOf(cod) - require.NotNil(t, owner, "expected a controller owner reference after adoption") - assert.Equal(t, ext.Name, owner.Name) - assert.Equal(t, ext.UID, owner.UID) - assert.Equal(t, ocv1.ClusterExtensionKind, owner.Kind) - }) - - t.Run("proceeds when the existing deployment is already controlled by this extension", func(t *testing.T) { - existing := &ocv1.ClusterObjectDeployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: ext.Name, - OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(ext, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind))}, + for _, tc := range []testCase{ + { + name: "adopts an existing deployment with no controller owner reference", + ext: &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "test-uid"}}, + cod: &ocv1.ClusterObjectDeployment{ + ObjectMeta: metav1.ObjectMeta{Name: "test-ext"}, }, - } - cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existing).Build() - bc := newCODBoxcutter(t, scheme, cl, objs) + assert: func(t *testing.T, _ client.Client, applyCompleted bool, applyStatus string, applyErr error) { + require.NoError(t, applyErr) + require.True(t, applyCompleted) + require.Empty(t, applyStatus) + }, + wantExtControllerOwner: true, + }, + { + name: "proceeds when the existing deployment is already controlled by this extension", + ext: &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "test-uid"}}, + cod: &ocv1.ClusterObjectDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ext", + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(&ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "test-uid"}}, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind))}, + }, + }, + assert: func(t *testing.T, _ client.Client, applyCompleted bool, applyStatus string, applyErr error) { + require.NoError(t, applyErr) + require.True(t, applyCompleted) + require.Empty(t, applyStatus) + }, + wantExtControllerOwner: true, + }, + { + name: "refuses to adopt a deployment controlled by a different owner", + ext: &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "test-ext", UID: "test-uid"}}, + cod: &ocv1.ClusterObjectDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ext", + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(&ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "other-ext", UID: "other-uid"}}, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind))}, + }, + }, + assert: func(t *testing.T, cl client.Client, applyCompleted bool, applyStatus string, applyErr error) { + require.Error(t, applyErr) + assert.False(t, applyCompleted) + assert.Empty(t, applyStatus) - completed, status, err := bc.Apply(t.Context(), dummyBundle, ext, map[string]string{"some": "label"}, map[string]string{}) - require.NoError(t, err) - require.True(t, completed) - require.Empty(t, status) - }) + t.Log("by surfacing a non-terminal error so retries occur without CE changes") + require.NotErrorIs(t, applyErr, reconcile.TerminalError(nil), "error was terminal, but expected non-terminal error so that CE retries occur") - t.Run("refuses to adopt a deployment controlled by a different owner", func(t *testing.T) { - foreign := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: "other-ext", UID: "other-uid"}} - existing := &ocv1.ClusterObjectDeployment{ - ObjectMeta: metav1.ObjectMeta{ - Name: ext.Name, - OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(foreign, ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionKind))}, - }, - } - cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existing).Build() - bc := newCODBoxcutter(t, scheme, cl, objs) + t.Log("by naming the deployment and the conflicting owner in the message") + assert.Contains(t, applyErr.Error(), "test-ext") + assert.Contains(t, applyErr.Error(), "other-ext") - completed, status, err := bc.Apply(t.Context(), dummyBundle, ext, map[string]string{"some": "label"}, map[string]string{}) - require.Error(t, err) - assert.False(t, completed) - assert.Empty(t, status) + secretList := &corev1.SecretList{} + require.NoError(t, cl.List(t.Context(), secretList, client.InNamespace("olmv1-system"))) + assert.Empty(t, secretList.Items) + }, + wantExtControllerOwner: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + scheme := codTestScheme(t) + cl := fake.NewClientBuilder().WithScheme(scheme).WithObjects(tc.cod).Build() + bc := newCODBoxcutter(t, scheme, cl, []client.Object{ + &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Name: "test-cm", Namespace: "test-ns"}}, + }) - t.Log("by surfacing a terminal error so Progressing becomes False") - require.ErrorIs(t, err, reconcile.TerminalError(nil), "error must be terminal") - t.Log("by carrying no specific reason so it falls back to the Blocked Progressing reason") - _, ok := errorutil.ExtractTerminalReason(err) - assert.False(t, ok, "terminal error should fall back to the Blocked reason") - t.Log("by naming the deployment and the conflicting owner in the message") - assert.Contains(t, err.Error(), ext.Name) - assert.Contains(t, err.Error(), "other-ext") - - t.Log("by leaving the foreign-owned deployment untouched") - cod := &ocv1.ClusterObjectDeployment{} - require.NoError(t, cl.Get(t.Context(), client.ObjectKey{Name: ext.Name}, cod)) - owner := metav1.GetControllerOf(cod) - require.NotNil(t, owner) - assert.Equal(t, "other-ext", owner.Name) - - t.Log("by not creating any packed Secrets for the refused deployment") - secretList := &corev1.SecretList{} - require.NoError(t, cl.List(t.Context(), secretList, client.InNamespace("olmv1-system"))) - assert.Empty(t, secretList.Items) - }) + actualCompleted, actualStatus, actualErr := bc.Apply(t.Context(), dummyBundle, tc.ext, map[string]string{"some": "label"}, map[string]string{}) + tc.assert(t, cl, actualCompleted, actualStatus, actualErr) + + cod := &ocv1.ClusterObjectDeployment{} + require.NoError(t, cl.Get(t.Context(), client.ObjectKey{Name: tc.ext.Name}, cod)) + owner := metav1.GetControllerOf(cod) + if tc.wantExtControllerOwner { + require.NotNil(t, owner) + assert.Equal(t, tc.ext.Name, owner.Name) + assert.Equal(t, tc.ext.UID, owner.UID) + assert.Equal(t, ocv1.ClusterExtensionKind, owner.Kind) + } else { + require.Equal(t, metav1.GetControllerOf(tc.cod), owner) + } + }) + } } // TestBoxcutterApply_Preflights verifies which preflight hook runs for each rollout state diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index 78d86eabfd..ae6531f2e6 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -429,13 +429,6 @@ func collectDeprecationMessages(entries []declcfg.DeprecationEntry) []string { // for each ClusterExtension. The ClusterExtension observes the deployment's rolled-up rollout status, // and the deployment is controller-owned by the ClusterExtension, so an owner watch re-reconciles the // ClusterExtension whenever its deployment's status changes. -// -// TODO: the owner watch only enqueues the ClusterExtension that controls a deployment. When the applier -// refuses to adopt a deployment controlled by a different owner (see Boxcutter.checkAdoptable), the -// blocked ClusterExtension does not own that deployment, so deleting the conflicting deployment does not -// enqueue it; it recovers only on its next spec/catalog change or the periodic resync. Add a name-keyed -// watch that enqueues the same-named ClusterExtension on any ClusterObjectDeployment change so a blocked -// extension recovers promptly once the conflict clears. func (r *ClusterExtensionReconciler) SetupWithManagerForBoxcutter(mgr ctrl.Manager) (crcontroller.Controller, error) { return r.newControllerBuilder(mgr). Owns(&ocv1.ClusterObjectDeployment{}). From 4615b2439ce889c75e8444d8f813f5ba9aed2de3 Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Thu, 29 Oct 2026 10:22:40 -0400 Subject: [PATCH 3/3] =?UTF-8?q?=E2=9C=A8=20Cover=20foreign=20COD=20ownersh?= =?UTF-8?q?ip=20in=20E2E?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Per G. da Silva --- .../features/cod_adoption_conflict.feature | 56 +++++++++++++++++++ test/e2e/steps/group_steps.go | 6 +- test/e2e/steps/steps.go | 3 +- 3 files changed, 61 insertions(+), 4 deletions(-) create mode 100644 test/e2e/features/cod_adoption_conflict.feature diff --git a/test/e2e/features/cod_adoption_conflict.feature b/test/e2e/features/cod_adoption_conflict.feature new file mode 100644 index 0000000000..d3d454f2d0 --- /dev/null +++ b/test/e2e/features/cod_adoption_conflict.feature @@ -0,0 +1,56 @@ +@BoxcutterRuntime +Feature: ClusterExtension refuses to adopt a ClusterObjectDeployment owned by another resource + + As an OLM user, I want a ClusterExtension to report and retry an adoption conflict, + so that it does not take over a ClusterObjectDeployment controlled by another owner + and can proceed after that deployment is removed. + + Background: + Given OLM is available + And an image registry is available + And namespace "${TEST_NAMESPACE}" is available + And a catalog "test" with packages: + | package | version | channel | replaces | contents | + | test | 1.2.0 | beta | | CRD, Deployment, ConfigMap | + + Scenario: ClusterExtension retries after a foreign-owned ClusterObjectDeployment is removed + # Note: we're using a namespace owner in lieu of an existing controller to simulate foreign controller ownership + Given ClusterObjectDeployment with Namespace owner "${TEST_NAMESPACE}" is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterObjectDeployment + metadata: + name: ${NAME} + spec: + template: + spec: + collisionProtection: Prevent + phases: + - name: resources + objects: [] + """ + When ClusterExtension is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterExtension + metadata: + name: ${NAME} + spec: + namespace: ${TEST_NAMESPACE} + source: + sourceType: Catalog + catalog: + packageName: ${PACKAGE:test} + version: 1.2.0 + selector: + matchLabels: + "olm.operatorframework.io/metadata.name": ${CATALOG:test} + """ + Then ClusterExtension reports Progressing as True with Reason Retrying and Message includes: + """ + ClusterObjectDeployment "${NAME}" is already controlled by Namespace "${TEST_NAMESPACE}" + """ + When resource "clusterobjectdeployment/${NAME}" is removed + Then ClusterExtension is rolled out + And ClusterExtension is available + And ClusterObjectDeployment "${NAME}" is owned by ClusterExtension "${NAME}" diff --git a/test/e2e/steps/group_steps.go b/test/e2e/steps/group_steps.go index 685c4914b4..8451d3b59f 100644 --- a/test/e2e/steps/group_steps.go +++ b/test/e2e/steps/group_steps.go @@ -17,9 +17,9 @@ import ( ocv1 "github.com/operator-framework/operator-controller/api/v1" ) -// An empty owner name creates an ownerless revision. Resolve a real cluster-scoped -// owner before creation so garbage collection and reconciliation see valid metadata. -func ClusterObjectSetWithNamespaceOwnerApplied(ctx context.Context, ownerName string, template *godog.DocString) error { +// ResourceWithNamespaceOwnerApplied resolves a real cluster-scoped Namespace owner +// before applying the resource so garbage collection sees valid owner metadata. +func ResourceWithNamespaceOwnerApplied(ctx context.Context, ownerName string, template *godog.DocString) error { sc := scenarioCtx(ctx) obj, err := toUnstructured(substituteScenarioVars(template.Content, sc)) if err != nil { diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index d524d5ccdd..90ee4cc27f 100644 --- a/test/e2e/steps/steps.go +++ b/test/e2e/steps/steps.go @@ -134,7 +134,8 @@ func RegisterSteps(sc *godog.ScenarioContext) { sc.Step(`^(?i)ClusterObjectDeployment "([^"]+)" consistently reports ([[:alnum:]]+) as ([[:alnum:]]+) with Reason ([[:alnum:]]+) for "([^"]+)"$`, ClusterObjectDeploymentConsistentlyReportsCondition) sc.Step(`^(?i)ClusterExtension reports ([[:alnum:]]+) transition between (\d+) and (\d+) minutes since ClusterObjectSet "([^"]+)" was created$`, ClusterExtensionReportsConditionTransitionTime) sc.Step(`^(?i)ClusterObjectSet is applied(?:\s+.*)?$`, ResourceIsApplied) - sc.Step(`^(?i)ClusterObjectSet with Namespace owner "([^"]*)" is applied$`, ClusterObjectSetWithNamespaceOwnerApplied) + sc.Step(`^(?i)ClusterObjectSet with Namespace owner "([^"]*)" is applied$`, ResourceWithNamespaceOwnerApplied) + sc.Step(`^(?i)ClusterObjectDeployment with Namespace owner "([^"]*)" is applied$`, ResourceWithNamespaceOwnerApplied) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" reconciliation is triggered$`, TriggerClusterObjectSetReconciliation) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" has observed phase "([^"]+)" with a non-empty digest$`, ClusterObjectSetHasObservedPhase) sc.Step(`^(?i)ClusterObjectSet "([^"]+)" is archived$`, ClusterObjectSetIsArchived)