diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index e558e206ff..ce9d2f0537 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -19,11 +19,13 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" metav1ac "k8s.io/client-go/applyconfigurations/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" "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" @@ -468,6 +470,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 { @@ -525,6 +536,46 @@ 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 + } + + gvk, err := apiutil.GVKForObject(ext, bc.Scheme) + if err != nil { + return fmt.Errorf("getting GVK for ClusterExtension %q: %w", ext.Name, err) + } + if cgv, parseErr := schema.ParseGroupVersion(controller.APIVersion); parseErr == nil && + cgv.Group == gvk.Group && + controller.Kind == gvk.Kind && + controller.Name == ext.Name { + 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..5d31d7219c 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,124 @@ 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("adopts a deployment controlled by a prior incarnation of this extension (same name, different UID)", func(t *testing.T) { + prior := &ocv1.ClusterExtension{ObjectMeta: metav1.ObjectMeta{Name: ext.Name, UID: "old-uid"}} + existing := &ocv1.ClusterObjectDeployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: ext.Name, + OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(prior, 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.Log("by re-stamping the current extension as the sole controller owner") + cod := &ocv1.ClusterObjectDeployment{} + require.NoError(t, cl.Get(t.Context(), client.ObjectKey{Name: ext.Name}, cod)) + controllers := 0 + for _, o := range cod.OwnerReferences { + if o.Controller != nil && *o.Controller { + controllers++ + } + } + assert.Equal(t, 1, controllers, "exactly one controller owner reference after adoption") + owner := metav1.GetControllerOf(cod) + require.NotNil(t, owner) + assert.Equal(t, ext.UID, owner.UID, "adoption should re-stamp the current extension's UID") + }) + + 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 8099f29960..decafe6468 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{}).