(chore): Use label-based cache for revision lookups instead of explicit chains

Instead of tracking revision history through Spec.Previous fields, we now
find related revisions using labels and the controller-runtime cache. This
is more efficient and works better with controller-runtime's caching layer.

To support this change, the Helm-to-Boxcutter migration now sets
ownerReferences on migrated revisions, ensuring they work identically
to newly created revisions.

Assisted-by: Cursor

Author: camilamacedo86

cmd/operator-controller/main.go

@@ -586,6 +586,7 @@ func setupBoxcutter(
 	ceReconciler.RevisionStatesGetter = &controllers.BoxcutterRevisionStatesGetter{Reader: mgr.GetClient()}
 	ceReconciler.StorageMigrator = &applier.BoxcutterStorageMigrator{
 		Client:             mgr.GetClient(),
+		Scheme:             mgr.GetScheme(),
 		ActionClientGetter: acg,
 		RevisionGenerator:  rg,
 	}

internal/operator-controller/applier/boxcutter.go

@@ -140,30 +140,56 @@ func (r *SimpleRevisionGenerator) buildClusterExtensionRevision(
 	ext *ocv1.ClusterExtension,
 	annotations map[string]string,
 ) *ocv1.ClusterExtensionRevision {
+	revisionLabels := map[string]string{
+		controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
+	}
+	// Package name must be in labels for BoxcutterRevisionStatesGetter to find it
+	if pkg, ok := annotations[labels.PackageNameKey]; ok {
+		revisionLabels[labels.PackageNameKey] = pkg
+	}
+
 	return &ocv1.ClusterExtensionRevision{
 		ObjectMeta: metav1.ObjectMeta{
 			Annotations: annotations,
-			Labels: map[string]string{
-				controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
-			},
+			Labels:      revisionLabels,
 		},
 		Spec: ocv1.ClusterExtensionRevisionSpec{
 			Phases: PhaseSort(objects),
 		},
 	}
 }
 
+// BoxcutterStorageMigrator migrates ClusterExtensions from Helm-based storage to Boxcutter's
+// ClusterExtensionRevision storage.
+//
+// Scenario: Given a ClusterExtension, determine if migration from Helm storage is needed
+//   - When ClusterExtensionRevisions already exist, no migration needed (return early)
+//   - When a Helm release exists but no revisions, create ClusterExtensionRevision from Helm release
+//   - When no Helm release exists and no revisions, this is a fresh install (no migration needed)
+//
+// The migrated revision includes:
+//   - Owner label (olm.operatorframework.io/owner) for label-based lookups
+//   - OwnerReference to parent ClusterExtension for proper garbage collection
+//   - All objects from the Helm release manifest
+//
+// This ensures migrated revisions have the same structure as revisions created by normal
+// Boxcutter flow, enabling successful upgrades after migration.
 type BoxcutterStorageMigrator struct {
 	ActionClientGetter helmclient.ActionClientGetter
 	RevisionGenerator  ClusterExtensionRevisionGenerator
 	Client             boxcutterStorageMigratorClient
+	Scheme             *runtime.Scheme // Required for setting ownerReferences on migrated revisions
 }
 
 type boxcutterStorageMigratorClient interface {
 	List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error
 	Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error
 }
 
+// Migrate performs a one-time migration from Helm storage to ClusterExtensionRevision storage.
+//
+// This enables upgrades from older operator-controller versions that used Helm for state management.
+// The migration is idempotent - it checks for existing revisions before attempting to migrate.
 func (m *BoxcutterStorageMigrator) Migrate(ctx context.Context, ext *ocv1.ClusterExtension, objectLabels map[string]string) error {
 	existingRevisionList := ocv1.ClusterExtensionRevisionList{}
 	if err := m.Client.List(ctx, &existingRevisionList, client.MatchingLabels{
@@ -195,6 +221,12 @@ func (m *BoxcutterStorageMigrator) Migrate(ctx context.Context, ext *ocv1.Cluste
 		return err
 	}
 
+	// Set ownerReference to ensure proper lifecycle management and garbage collection.
+	// This makes the migrated revision consistent with revisions created by normal Boxcutter flow.
+	if err := controllerutil.SetControllerReference(ext, rev, m.Scheme); err != nil {
+		return fmt.Errorf("set ownerref: %w", err)
+	}
+
 	if err := m.Client.Create(ctx, rev); err != nil {
 		return err
 	}

internal/operator-controller/applier/boxcutter_test.go

@@ -69,7 +69,8 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T)
 				"olm.operatorframework.io/package-name":     "my-package",
 			},
 			Labels: map[string]string{
-				"olm.operatorframework.io/owner": "test-123",
+				"olm.operatorframework.io/owner":        "test-123",
+				"olm.operatorframework.io/package-name": "my-package",
 			},
 		},
 		Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -758,6 +759,100 @@ func TestBoxcutter_Apply(t *testing.T) {
 				assert.Contains(t, latest.Spec.Previous, ocv1.ClusterExtensionRevisionPrevious{Name: "rev-4"})
 			},
 		},
+		{
+			name: "annotation-only update (same phases, different annotations)",
+			mockBuilder: &mockBundleRevisionBuilder{
+				makeRevisionFunc: func(bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) {
+					// Return revision with UPDATED annotations but SAME phases
+					revisionLabels := map[string]string{
+						controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
+					}
+					if pkg, ok := revisionAnnotations[labels.PackageNameKey]; ok {
+						revisionLabels[labels.PackageNameKey] = pkg
+					}
+					return &ocv1.ClusterExtensionRevision{
+						ObjectMeta: metav1.ObjectMeta{
+							Annotations: revisionAnnotations,
+							Labels:      revisionLabels,
+						},
+						Spec: ocv1.ClusterExtensionRevisionSpec{
+							Phases: []ocv1.ClusterExtensionRevisionPhase{
+								{
+									Name: string(applier.PhaseDeploy),
+									Objects: []ocv1.ClusterExtensionRevisionObject{
+										{
+											Object: unstructured.Unstructured{
+												Object: map[string]interface{}{
+													"apiVersion": "v1",
+													"kind":       "ConfigMap",
+													"metadata": map[string]interface{}{
+														"name": "test-cm",
+													},
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					}, nil
+				},
+			},
+			existingObjs: []client.Object{
+				ext,
+				&ocv1.ClusterExtensionRevision{
+					ObjectMeta: metav1.ObjectMeta{
+						Name: "test-ext-1",
+						Annotations: map[string]string{
+							labels.BundleVersionKey: "1.0.0",
+							labels.PackageNameKey:   "test-package",
+						},
+						Labels: map[string]string{
+							controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
+							labels.PackageNameKey:                          "test-package",
+						},
+					},
+					Spec: ocv1.ClusterExtensionRevisionSpec{
+						Revision: 1,
+						Phases: []ocv1.ClusterExtensionRevisionPhase{
+							{
+								Name: string(applier.PhaseDeploy),
+								Objects: []ocv1.ClusterExtensionRevisionObject{
+									{
+										Object: unstructured.Unstructured{
+											Object: map[string]interface{}{
+												"apiVersion": "v1",
+												"kind":       "ConfigMap",
+												"metadata": map[string]interface{}{
+													"name": "test-cm",
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			validate: func(t *testing.T, c client.Client) {
+				revList := &ocv1.ClusterExtensionRevisionList{}
+				err := c.List(context.Background(), revList, client.MatchingLabels{controllers.ClusterExtensionRevisionOwnerLabel: ext.Name})
+				require.NoError(t, err)
+				// Should still be only 1 revision (in-place update, not new revision)
+				require.Len(t, revList.Items, 1)
+
+				rev := revList.Items[0]
+				assert.Equal(t, "test-ext-1", rev.Name)
+				assert.Equal(t, int64(1), rev.Spec.Revision)
+				// Verify annotations were updated
+				assert.Equal(t, "1.0.1", rev.Annotations[labels.BundleVersionKey])
+				assert.Equal(t, "test-package", rev.Annotations[labels.PackageNameKey])
+				// Verify labels still have package name
+				assert.Equal(t, ext.Name, rev.Labels[controllers.ClusterExtensionRevisionOwnerLabel])
+				assert.Equal(t, "test-package", rev.Labels[labels.PackageNameKey])
+			},
+		},
 	}
 
 	for _, tc := range testCases {
@@ -780,7 +875,15 @@ func TestBoxcutter_Apply(t *testing.T) {
 			testFS := fstest.MapFS{}
 
 			// Execute
-			installSucceeded, installStatus, err := boxcutter.Apply(t.Context(), testFS, ext, nil, nil)
+			revisionAnnotations := map[string]string{}
+			if tc.name == "annotation-only update (same phases, different annotations)" {
+				// For annotation-only update test, pass NEW annotations
+				revisionAnnotations = map[string]string{
+					labels.BundleVersionKey: "1.0.1",
+					labels.PackageNameKey:   "test-package",
+				}
+			}
+			installSucceeded, installStatus, err := boxcutter.Apply(t.Context(), testFS, ext, nil, revisionAnnotations)
 
 			// Assert
 			if tc.expectedErr != "" {
@@ -808,13 +911,17 @@ func TestBoxcutter_Apply(t *testing.T) {
 
 func TestBoxcutterStorageMigrator(t *testing.T) {
 	t.Run("creates revision", func(t *testing.T) {
+		testScheme := runtime.NewScheme()
+		require.NoError(t, ocv1.AddToScheme(testScheme))
+
 		brb := &mockBundleRevisionBuilder{}
 		mag := &mockActionGetter{}
 		client := &clientMock{}
 		sm := &applier.BoxcutterStorageMigrator{
 			RevisionGenerator:  brb,
 			ActionClientGetter: mag,
 			Client:             client,
+			Scheme:             testScheme,
 		}
 
 		ext := &ocv1.ClusterExtension{
@@ -836,13 +943,17 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
 	})
 
 	t.Run("does not create revision when revisions exist", func(t *testing.T) {
+		testScheme := runtime.NewScheme()
+		require.NoError(t, ocv1.AddToScheme(testScheme))
+
 		brb := &mockBundleRevisionBuilder{}
 		mag := &mockActionGetter{}
 		client := &clientMock{}
 		sm := &applier.BoxcutterStorageMigrator{
 			RevisionGenerator:  brb,
 			ActionClientGetter: mag,
 			Client:             client,
+			Scheme:             testScheme,
 		}
 
 		ext := &ocv1.ClusterExtension{
@@ -866,6 +977,9 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
 	})
 
 	t.Run("does not create revision when no helm release", func(t *testing.T) {
+		testScheme := runtime.NewScheme()
+		require.NoError(t, ocv1.AddToScheme(testScheme))
+
 		brb := &mockBundleRevisionBuilder{}
 		mag := &mockActionGetter{
 			getClientErr: driver.ErrReleaseNotFound,
@@ -875,6 +989,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
 			RevisionGenerator:  brb,
 			ActionClientGetter: mag,
 			Client:             client,
+			Scheme:             testScheme,
 		}
 
 		ext := &ocv1.ClusterExtension{
@@ -905,7 +1020,15 @@ func (m *mockBundleRevisionBuilder) GenerateRevisionFromHelmRelease(
 	helmRelease *release.Release, ext *ocv1.ClusterExtension,
 	objectLabels map[string]string,
 ) (*ocv1.ClusterExtensionRevision, error) {
-	return nil, nil
+	return &ocv1.ClusterExtensionRevision{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test-revision",
+			Labels: map[string]string{
+				controllers.ClusterExtensionRevisionOwnerLabel: ext.Name,
+			},
+		},
+		Spec: ocv1.ClusterExtensionRevisionSpec{},
+	}, nil
 }
 
 type clientMock struct {

internal/operator-controller/controllers/clusterextensionrevision_controller.go

@@ -16,7 +16,6 @@ import (
 	"k8s.io/apimachinery/pkg/api/equality"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -116,7 +115,17 @@ func checkForUnexpectedClusterExtensionRevisionFieldChange(a, b ocv1.ClusterExte
 func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev *ocv1.ClusterExtensionRevision) (ctrl.Result, error) {
 	l := log.FromContext(ctx)
 
-	revision, opts, previous := toBoxcutterRevision(rev)
+	revision, opts, err := c.toBoxcutterRevision(ctx, rev)
+	if err != nil {
+		meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+			Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
+			Status:             metav1.ConditionFalse,
+			Reason:             ocv1.ClusterExtensionRevisionReasonReconcileFailure,
+			Message:            err.Error(),
+			ObservedGeneration: rev.Generation,
+		})
+		return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err)
+	}
 
 	if !rev.DeletionTimestamp.IsZero() || rev.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived {
 		return c.teardown(ctx, rev, revision)
@@ -212,7 +221,11 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, rev
 
 	//nolint:nestif
 	if rres.IsComplete() {
-		// Archive other revisions.
+		// Archive previous revisions
+		previous, err := c.ListPreviousRevisions(ctx, rev)
+		if err != nil {
+			return ctrl.Result{}, fmt.Errorf("listing previous revisions: %v", err)
+		}
 		for _, a := range previous {
 			patch := []byte(`{"spec":{"lifecycleState":"Archived"}}`)
 			if err := c.Client.Patch(ctx, a, client.RawPatch(types.MergePatchType, patch)); err != nil {
@@ -428,14 +441,48 @@ func (c *ClusterExtensionRevisionReconciler) removeFinalizer(ctx context.Context
 	return nil
 }
 
-func toBoxcutterRevision(rev *ocv1.ClusterExtensionRevision) (*boxcutter.Revision, []boxcutter.RevisionReconcileOption, []client.Object) {
-	previous := make([]client.Object, 0, len(rev.Spec.Previous))
-	for _, specPrevious := range rev.Spec.Previous {
-		prev := &unstructured.Unstructured{}
-		prev.SetName(specPrevious.Name)
-		prev.SetUID(specPrevious.UID)
-		prev.SetGroupVersionKind(ocv1.GroupVersion.WithKind(ocv1.ClusterExtensionRevisionKind))
-		previous = append(previous, prev)
+// ListPreviousRevisions returns active revisions belonging to the same ClusterExtension.
+//
+// Scenario: Given a ClusterExtensionRevision, find all related active revisions
+//   - When the revision has no owner label, return empty list (revision not properly labeled)
+//   - When looking up by owner label, filter out the current revision itself
+//   - When a revision is archived or being deleted, exclude it from results
+//   - Otherwise include active and paused revisions with matching owner
+func (c *ClusterExtensionRevisionReconciler) ListPreviousRevisions(ctx context.Context, rev *ocv1.ClusterExtensionRevision) ([]client.Object, error) {
+	ownerLabel, ok := rev.Labels[ClusterExtensionRevisionOwnerLabel]
+	if !ok {
+		// No owner label means this revision isn't properly labeled - return empty list
+		return nil, nil
+	}
+
+	revList := &ocv1.ClusterExtensionRevisionList{}
+	if err := c.Client.List(ctx, revList, client.MatchingLabels{
+		ClusterExtensionRevisionOwnerLabel: ownerLabel,
+	}); err != nil {
+		return nil, fmt.Errorf("listing revisions: %w", err)
+	}
+
+	previous := make([]client.Object, 0, len(revList.Items))
+	for i := range revList.Items {
+		r := &revList.Items[i]
+		if r.Name == rev.Name {
+			continue
+		}
+		// Skip archived or deleting revisions
+		if r.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived ||
+			!r.DeletionTimestamp.IsZero() {
+			continue
+		}
+		previous = append(previous, r)
+	}
+
+	return previous, nil
+}
+
+func (c *ClusterExtensionRevisionReconciler) toBoxcutterRevision(ctx context.Context, rev *ocv1.ClusterExtensionRevision) (*boxcutter.Revision, []boxcutter.RevisionReconcileOption, error) {
+	previous, err := c.ListPreviousRevisions(ctx, rev)
+	if err != nil {
+		return nil, nil, fmt.Errorf("listing previous revisions: %w", err)
 	}
 
 	opts := []boxcutter.RevisionReconcileOption{
@@ -476,7 +523,7 @@ func toBoxcutterRevision(rev *ocv1.ClusterExtensionRevision) (*boxcutter.Revisio
 	if rev.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStatePaused {
 		opts = append(opts, boxcutter.WithPaused{})
 	}
-	return r, opts, previous
+	return r, opts, nil
 }
 
 var (