(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

@@ -84,6 +84,9 @@ func (r *SimpleRevisionGenerator) GenerateRevisionFromHelmRelease(
 	})
 	rev.Name = fmt.Sprintf("%s-1", ext.Name)
 	rev.Spec.Revision = 1
+	// Explicitly set LifecycleState to Active to ensure migrated revision is recognized as active
+	// even before CRD default is applied by the API server
+	rev.Spec.LifecycleState = ocv1.ClusterExtensionRevisionLifecycleStateActive
 	return rev, nil
 }
 
@@ -140,30 +143,57 @@ 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
+	Status() client.StatusWriter
 }
 
+// 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,9 +225,38 @@ 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
 	}
+
+	// Mark the migrated revision as succeeded since the Helm release was already deployed.
+	// Without Succeeded=True, BoxcutterRevisionStatesGetter treats it as RollingOut instead of Installed,
+	// which breaks resolution and prevents upgrades.
+	meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+		Type:               ocv1.ClusterExtensionRevisionTypeAvailable,
+		Status:             metav1.ConditionTrue,
+		Reason:             ocv1.ClusterExtensionRevisionReasonAvailable,
+		Message:            "Migrated from Helm storage",
+		ObservedGeneration: rev.Generation,
+	})
+	meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{
+		Type:               ocv1.ClusterExtensionRevisionTypeSucceeded,
+		Status:             metav1.ConditionTrue,
+		Reason:             ocv1.ClusterExtensionRevisionReasonRolloutSuccess,
+		Message:            "Migrated from Helm storage",
+		ObservedGeneration: rev.Generation,
+	})
+
+	if err := m.Client.Status().Update(ctx, rev); err != nil {
+		return fmt.Errorf("updating migrated revision status: %w", err)
+	}
+
 	return nil
 }
 

internal/operator-controller/applier/boxcutter_test.go

@@ -69,11 +69,13 @@ 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{
-			Revision: 1,
+			LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive,
+			Revision:       1,
 			Phases: []ocv1.ClusterExtensionRevisionPhase{
 				{
 					Name: "deploy",
@@ -758,6 +760,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 +876,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 +912,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{
@@ -828,6 +936,10 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
 			On("Create", mock.Anything, mock.AnythingOfType("*v1.ClusterExtensionRevision"), mock.Anything).
 			Once().
 			Return(nil)
+		client.
+			On("Update", mock.Anything, mock.AnythingOfType("*v1.ClusterExtensionRevision"), mock.Anything).
+			Once().
+			Return(nil)
 
 		err := sm.Migrate(t.Context(), ext, map[string]string{"my-label": "my-value"})
 		require.NoError(t, err)
@@ -836,13 +948,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 +982,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 +994,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) {
 			RevisionGenerator:  brb,
 			ActionClientGetter: mag,
 			Client:             client,
+			Scheme:             testScheme,
 		}
 
 		ext := &ocv1.ClusterExtension{
@@ -905,7 +1025,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 {
@@ -921,3 +1049,26 @@ func (m *clientMock) Create(ctx context.Context, obj client.Object, opts ...clie
 	args := m.Called(ctx, obj, opts)
 	return args.Error(0)
 }
+
+func (m *clientMock) Status() client.StatusWriter {
+	return &statusWriterMock{mock: &m.Mock}
+}
+
+type statusWriterMock struct {
+	mock *mock.Mock
+}
+
+func (s *statusWriterMock) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error {
+	args := s.mock.Called(ctx, obj, opts)
+	return args.Error(0)
+}
+
+func (s *statusWriterMock) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error {
+	args := s.mock.Called(ctx, obj, patch, opts)
+	return args.Error(0)
+}
+
+func (s *statusWriterMock) Create(ctx context.Context, obj client.Object, subResource client.Object, opts ...client.SubResourceCreateOption) error {
+	args := s.mock.Called(ctx, obj, subResource, opts)
+	return args.Error(0)
+}