diff --git a/api/v1/clusterextensionrevision_types.go b/api/v1/clusterextensionrevision_types.go index 79c445e6e..e048e1b54 100644 --- a/api/v1/clusterextensionrevision_types.go +++ b/api/v1/clusterextensionrevision_types.go @@ -19,7 +19,6 @@ package v1 import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/types" ) const ( @@ -40,6 +39,7 @@ const ( ClusterExtensionRevisionReasonIncomplete = "Incomplete" ClusterExtensionRevisionReasonProgressing = "Progressing" ClusterExtensionRevisionReasonArchived = "Archived" + ClusterExtensionRevisionReasonMigrated = "Migrated" ) // ClusterExtensionRevisionSpec defines the desired state of ClusterExtensionRevision. @@ -66,10 +66,6 @@ type ClusterExtensionRevisionSpec struct { // +listMapKey=name // +optional Phases []ClusterExtensionRevisionPhase `json:"phases,omitempty"` - // Previous references previous revisions that objects can be adopted from. - // - // +kubebuilder:validation:XValidation:rule="self == oldSelf", message="previous is immutable" - Previous []ClusterExtensionRevisionPrevious `json:"previous,omitempty"` } // ClusterExtensionRevisionLifecycleState specifies the lifecycle state of the ClusterExtensionRevision. @@ -130,13 +126,6 @@ const ( CollisionProtectionNone CollisionProtection = "None" ) -type ClusterExtensionRevisionPrevious struct { - // +kubebuilder:validation:Required - Name string `json:"name"` - // +kubebuilder:validation:Required - UID types.UID `json:"uid"` -} - // ClusterExtensionRevisionStatus defines the observed state of a ClusterExtensionRevision. type ClusterExtensionRevisionStatus struct { // +listType=map diff --git a/api/v1/zz_generated.deepcopy.go b/api/v1/zz_generated.deepcopy.go index e13f1532b..cc27ec68f 100644 --- a/api/v1/zz_generated.deepcopy.go +++ b/api/v1/zz_generated.deepcopy.go @@ -436,21 +436,6 @@ func (in *ClusterExtensionRevisionPhase) DeepCopy() *ClusterExtensionRevisionPha return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *ClusterExtensionRevisionPrevious) DeepCopyInto(out *ClusterExtensionRevisionPrevious) { - *out = *in -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClusterExtensionRevisionPrevious. -func (in *ClusterExtensionRevisionPrevious) DeepCopy() *ClusterExtensionRevisionPrevious { - if in == nil { - return nil - } - out := new(ClusterExtensionRevisionPrevious) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ClusterExtensionRevisionSpec) DeepCopyInto(out *ClusterExtensionRevisionSpec) { *out = *in @@ -461,11 +446,6 @@ func (in *ClusterExtensionRevisionSpec) DeepCopyInto(out *ClusterExtensionRevisi (*in)[i].DeepCopyInto(&(*out)[i]) } } - if in.Previous != nil { - in, out := &in.Previous, &out.Previous - *out = make([]ClusterExtensionRevisionPrevious, len(*in)) - copy(*out, *in) - } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ClusterExtensionRevisionSpec. diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index 20d55bc3c..2232407bc 100644 --- a/cmd/operator-controller/main.go +++ b/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, } diff --git a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml index b11635674..b25e57903 100644 --- a/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml +++ b/helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensionrevisions.yaml @@ -111,27 +111,6 @@ spec: x-kubernetes-validations: - message: phases is immutable rule: self == oldSelf || oldSelf.size() == 0 - previous: - description: Previous references previous revisions that objects can - be adopted from. - items: - properties: - name: - type: string - uid: - description: |- - UID is a type that holds unique ID values, including UUIDs. Because we - don't ONLY use UUIDs, this is an alias to string. Being a type captures - intent and helps make sure that UIDs and names do not get conflated. - type: string - required: - - name - - uid - type: object - type: array - x-kubernetes-validations: - - message: previous is immutable - rule: self == oldSelf revision: description: |- Revision is a sequence number representing a specific revision of the ClusterExtension instance. diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 1914b80e8..e8be40295 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -32,7 +32,7 @@ import ( ) const ( - ClusterExtensionRevisionPreviousLimit = 5 + ClusterExtensionRevisionRetentionLimit = 5 ) type ClusterExtensionRevisionGenerator interface { @@ -195,22 +195,33 @@ func (r *SimpleRevisionGenerator) buildClusterExtensionRevision( }, }, Spec: ocv1.ClusterExtensionRevisionSpec{ - Phases: PhaseSort(objects), + // Explicitly set LifecycleState to Active. While the CRD has a default, + // being explicit here ensures all code paths are clear and doesn't rely + // on API server defaulting behavior. + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive, + Phases: PhaseSort(objects), }, } } +// BoxcutterStorageMigrator migrates ClusterExtensions from Helm-based storage to +// ClusterExtensionRevision storage, enabling upgrades from older operator-controller versions. type BoxcutterStorageMigrator struct { ActionClientGetter helmclient.ActionClientGetter RevisionGenerator ClusterExtensionRevisionGenerator Client boxcutterStorageMigratorClient + Scheme *runtime.Scheme } type boxcutterStorageMigratorClient interface { List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error + Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error + Status() client.StatusWriter } +// Migrate creates a ClusterExtensionRevision from an existing Helm release if no revisions exist yet. +// The migration is idempotent and skipped if revisions already exist or no Helm release is found. 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{ @@ -242,9 +253,29 @@ func (m *BoxcutterStorageMigrator) Migrate(ctx context.Context, ext *ocv1.Cluste return err } + // Set ownerReference for proper garbage collection when the ClusterExtension is deleted. + 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 } + + // Set Available=Unknown so the revision controller will verify cluster state through probes. + // During migration, ClusterExtension will briefly show as not installed until verification completes. + meta.SetStatusCondition(&rev.Status.Conditions, metav1.Condition{ + Type: ocv1.ClusterExtensionRevisionTypeAvailable, + Status: metav1.ConditionUnknown, + Reason: ocv1.ClusterExtensionRevisionReasonMigrated, + Message: "Migrated from Helm storage, awaiting cluster state verification", + ObservedGeneration: rev.Generation, + }) + + if err := m.Client.Status().Update(ctx, rev); err != nil { + return fmt.Errorf("updating migrated revision status: %w", err) + } + return nil } @@ -304,7 +335,6 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust if len(existingRevisions) > 0 { // try first to update the current revision. currentRevision = &existingRevisions[len(existingRevisions)-1] - desiredRevision.Spec.Previous = currentRevision.Spec.Previous desiredRevision.Spec.Revision = currentRevision.Spec.Revision desiredRevision.Name = currentRevision.Name @@ -354,8 +384,8 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust desiredRevision.Name = fmt.Sprintf("%s-%d", ext.Name, revisionNumber) desiredRevision.Spec.Revision = revisionNumber - if err = bc.setPreviousRevisions(ctx, desiredRevision, prevRevisions); err != nil { - return false, "", fmt.Errorf("garbage collecting old Revisions: %w", err) + if err = bc.garbageCollectOldRevisions(ctx, prevRevisions); err != nil { + return false, "", fmt.Errorf("garbage collecting old revisions: %w", err) } if err := bc.createOrUpdate(ctx, desiredRevision); err != nil { @@ -380,28 +410,21 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust return true, "", nil } -// setPreviousRevisions populates spec.previous of latestRevision, trimming the list of previous _archived_ revisions down to -// ClusterExtensionRevisionPreviousLimit or to the first _active_ revision and deletes trimmed revisions from the cluster. -// NOTE: revisionList must be sorted in chronographical order, from oldest to latest. -func (bc *Boxcutter) setPreviousRevisions(ctx context.Context, latestRevision *ocv1.ClusterExtensionRevision, revisionList []ocv1.ClusterExtensionRevision) error { - // Pre-allocate with capacity limit to reduce allocations - trimmedPrevious := make([]ocv1.ClusterExtensionRevisionPrevious, 0, ClusterExtensionRevisionPreviousLimit) +// garbageCollectOldRevisions deletes archived revisions beyond ClusterExtensionRevisionRetentionLimit. +// Active revisions are never deleted. revisionList must be sorted oldest to newest. +func (bc *Boxcutter) garbageCollectOldRevisions(ctx context.Context, revisionList []ocv1.ClusterExtensionRevision) error { for index, r := range revisionList { - if index < len(revisionList)-ClusterExtensionRevisionPreviousLimit && r.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived { - // Delete oldest CREs from the cluster and list to reach ClusterExtensionRevisionPreviousLimit or latest active revision + // Only delete archived revisions that are beyond the limit + if index < len(revisionList)-ClusterExtensionRevisionRetentionLimit && r.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived { if err := bc.Client.Delete(ctx, &ocv1.ClusterExtensionRevision{ ObjectMeta: metav1.ObjectMeta{ Name: r.Name, }, }); err != nil && !apierrors.IsNotFound(err) { - return fmt.Errorf("deleting previous archived Revision: %w", err) + return fmt.Errorf("deleting archived revision: %w", err) } - } else { - // All revisions within the limit or still active are preserved - trimmedPrevious = append(trimmedPrevious, ocv1.ClusterExtensionRevisionPrevious{Name: r.Name, UID: r.GetUID()}) } } - latestRevision.Spec.Previous = trimmedPrevious return nil } diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index ad30bf2c1..67c59b0c9 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -20,7 +20,6 @@ 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/types" "k8s.io/apimachinery/pkg/util/validation/field" k8scheme "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" @@ -91,7 +90,8 @@ func Test_SimpleRevisionGenerator_GenerateRevisionFromHelmRelease(t *testing.T) }, }, Spec: ocv1.ClusterExtensionRevisionSpec{ - Revision: 1, + LifecycleState: ocv1.ClusterExtensionRevisionLifecycleStateActive, + Revision: 1, Phases: []ocv1.ClusterExtensionRevisionPhase{ { Name: "deploy", @@ -544,9 +544,6 @@ func TestBoxcutter_Apply(t *testing.T) { assert.Equal(t, "test-ext-2", newRev.Name) assert.Equal(t, int64(2), newRev.Spec.Revision) - require.Len(t, newRev.Spec.Previous, 1) - assert.Equal(t, "test-ext-1", newRev.Spec.Previous[0].Name) - assert.Equal(t, types.UID("rev-uid-1"), newRev.Spec.Previous[0].UID) }, }, { @@ -661,10 +658,12 @@ func TestBoxcutter_Apply(t *testing.T) { require.Error(t, err) assert.True(t, apierrors.IsNotFound(err)) - latest := &ocv1.ClusterExtensionRevision{} - err = c.Get(t.Context(), client.ObjectKey{Name: "test-ext-7"}, latest) + // Verify garbage collection: should only keep the limit + 1 (current) revisions + revList := &ocv1.ClusterExtensionRevisionList{} + err = c.List(t.Context(), revList) require.NoError(t, err) - assert.Len(t, latest.Spec.Previous, applier.ClusterExtensionRevisionPreviousLimit) + // Should have ClusterExtensionRevisionRetentionLimit (5) + current (1) = 6 revisions max + assert.LessOrEqual(t, len(revList.Items), applier.ClusterExtensionRevisionRetentionLimit+1) }, }, { @@ -781,15 +780,97 @@ func TestBoxcutter_Apply(t *testing.T) { err = c.Get(t.Context(), client.ObjectKey{Name: "rev-2"}, rev2) require.NoError(t, err) + // Verify active revisions are kept even if beyond the limit rev4 := &ocv1.ClusterExtensionRevision{} err = c.Get(t.Context(), client.ObjectKey{Name: "rev-4"}, rev4) + require.NoError(t, err, "active revision 4 should still exist even though it's beyond the limit") + }, + }, + { + name: "annotation-only update (same phases, different annotations)", + mockBuilder: &mockBundleRevisionBuilder{ + makeRevisionFunc: func(ctx context.Context, bundleFS fs.FS, ext *ocv1.ClusterExtension, objectLabels, revisionAnnotations map[string]string) (*ocv1.ClusterExtensionRevision, error) { + return &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: revisionAnnotations, + Labels: map[string]string{ + controllers.ClusterExtensionRevisionOwnerLabel: ext.Name, + }, + }, + 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, + }, + }, + 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) - latest := &ocv1.ClusterExtensionRevision{} - err = c.Get(t.Context(), client.ObjectKey{Name: "test-ext-8"}, latest) - require.NoError(t, err) - assert.Len(t, latest.Spec.Previous, 6) - assert.Contains(t, latest.Spec.Previous, ocv1.ClusterExtensionRevisionPrevious{Name: "rev-4"}) + 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 owner label is still present + assert.Equal(t, ext.Name, rev.Labels[controllers.ClusterExtensionRevisionOwnerLabel]) }, }, } @@ -814,7 +895,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 != "" { @@ -842,6 +931,9 @@ 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{} @@ -849,6 +941,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) { RevisionGenerator: brb, ActionClientGetter: mag, Client: client, + Scheme: testScheme, } ext := &ocv1.ClusterExtension{ @@ -861,6 +954,12 @@ func TestBoxcutterStorageMigrator(t *testing.T) { client. On("Create", mock.Anything, mock.AnythingOfType("*v1.ClusterExtensionRevision"), mock.Anything). Once(). + Run(func(args mock.Arguments) { + // Simulate real Kubernetes behavior: Create() populates server-managed fields + rev := args.Get(1).(*ocv1.ClusterExtensionRevision) + rev.Generation = 1 + rev.ResourceVersion = "1" + }). Return(nil) err := sm.Migrate(t.Context(), ext, map[string]string{"my-label": "my-value"}) @@ -870,6 +969,9 @@ 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{} @@ -877,6 +979,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) { RevisionGenerator: brb, ActionClientGetter: mag, Client: client, + Scheme: testScheme, } ext := &ocv1.ClusterExtension{ @@ -900,6 +1003,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, @@ -909,6 +1015,7 @@ func TestBoxcutterStorageMigrator(t *testing.T) { RevisionGenerator: brb, ActionClientGetter: mag, Client: client, + Scheme: testScheme, } ext := &ocv1.ClusterExtension{ @@ -940,7 +1047,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 { @@ -952,7 +1067,33 @@ func (m *clientMock) List(ctx context.Context, list client.ObjectList, opts ...c return args.Error(0) } +func (m *clientMock) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + args := m.Called(ctx, key, obj, opts) + return args.Error(0) +} + func (m *clientMock) Create(ctx context.Context, obj client.Object, opts ...client.CreateOption) error { 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 { + // Status updates are expected during migration - return success by default + return nil +} + +func (s *statusWriterMock) Patch(ctx context.Context, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption) error { + return nil +} + +func (s *statusWriterMock) Create(ctx context.Context, obj client.Object, subResource client.Object, opts ...client.SubResourceCreateOption) error { + return nil +} diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index 7bcedde65..e8f035193 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -549,11 +549,11 @@ func (d *BoxcutterRevisionStatesGetter) GetRevisionStates(ctx context.Context, e continue } - // TODO: the setting of these annotations (happens in boxcutter applier when we pass in "storageLabels") + // TODO: the setting of these annotations (happens in boxcutter applier when we pass in "revisionAnnotations") // is fairly decoupled from this code where we get the annotations back out. We may want to co-locate // the set/get logic a bit better to make it more maintainable and less likely to get out of sync. rm := &RevisionMetadata{ - Package: rev.Labels[labels.PackageNameKey], + Package: rev.Annotations[labels.PackageNameKey], Image: rev.Annotations[labels.BundleReferenceKey], BundleMetadata: ocv1.BundleMetadata{ Name: rev.Annotations[labels.BundleNameKey], diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller.go b/internal/operator-controller/controllers/clusterextensionrevision_controller.go index 7e98faa65..871d6d193 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller.go +++ b/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) @@ -218,7 +227,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 { @@ -440,14 +453,47 @@ 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 with lower revision numbers. +// Filters out the current revision, archived revisions, deleting revisions, and revisions with equal or higher numbers. +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.TrackingCache.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 + } + // Only include revisions with lower revision numbers (actual previous revisions) + if r.Spec.Revision >= rev.Spec.Revision { + 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{ @@ -488,7 +534,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 ( diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller_internal_test.go b/internal/operator-controller/controllers/clusterextensionrevision_controller_internal_test.go new file mode 100644 index 000000000..1db30f914 --- /dev/null +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller_internal_test.go @@ -0,0 +1,236 @@ +package controllers + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/sets" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/source" + + ocv1 "github.com/operator-framework/operator-controller/api/v1" +) + +func Test_ClusterExtensionRevisionReconciler_listPreviousRevisions(t *testing.T) { + testScheme := runtime.NewScheme() + require.NoError(t, ocv1.AddToScheme(testScheme)) + + for _, tc := range []struct { + name string + existingObjs func() []client.Object + currentRev string + expectedRevs []string + }{ + { + // Scenario: + // - Three revisions belong to the same owner. + // - We ask for previous revisions of rev-2. + // - Only revisions with lower revision numbers are returned (rev-1). + // - Higher revision numbers (rev-3) are excluded. + name: "should skip current revision when listing previous", + existingObjs: func() []client.Object { + ext := newTestClusterExtensionInternal() + rev1 := newTestClusterExtensionRevisionInternal(t, "rev-1") + rev2 := newTestClusterExtensionRevisionInternal(t, "rev-2") + rev3 := newTestClusterExtensionRevisionInternal(t, "rev-3") + require.NoError(t, controllerutil.SetControllerReference(ext, rev1, testScheme)) + require.NoError(t, controllerutil.SetControllerReference(ext, rev2, testScheme)) + require.NoError(t, controllerutil.SetControllerReference(ext, rev3, testScheme)) + return []client.Object{ext, rev1, rev2, rev3} + }, + currentRev: "rev-2", + expectedRevs: []string{"rev-1"}, + }, + { + // Scenario: + // - One sibling is archived already. + // - The caller should not get archived items. + // - Only active siblings are returned. + name: "should drop archived revisions when listing previous", + existingObjs: func() []client.Object { + ext := newTestClusterExtensionInternal() + rev1 := newTestClusterExtensionRevisionInternal(t, "rev-1") + rev2 := newTestClusterExtensionRevisionInternal(t, "rev-2") + rev2.Spec.LifecycleState = ocv1.ClusterExtensionRevisionLifecycleStateArchived + rev3 := newTestClusterExtensionRevisionInternal(t, "rev-3") + require.NoError(t, controllerutil.SetControllerReference(ext, rev1, testScheme)) + require.NoError(t, controllerutil.SetControllerReference(ext, rev2, testScheme)) + require.NoError(t, controllerutil.SetControllerReference(ext, rev3, testScheme)) + return []client.Object{ext, rev1, rev2, rev3} + }, + currentRev: "rev-3", + expectedRevs: []string{"rev-1"}, + }, + { + // Scenario: + // - One sibling is being deleted. + // - We list previous revisions. + // - The deleting one is filtered out. + name: "should drop deleting revisions when listing previous", + existingObjs: func() []client.Object { + ext := newTestClusterExtensionInternal() + rev1 := newTestClusterExtensionRevisionInternal(t, "rev-1") + rev2 := newTestClusterExtensionRevisionInternal(t, "rev-2") + rev2.Finalizers = []string{"test-finalizer"} + rev2.DeletionTimestamp = &metav1.Time{Time: time.Now()} + rev3 := newTestClusterExtensionRevisionInternal(t, "rev-3") + require.NoError(t, controllerutil.SetControllerReference(ext, rev1, testScheme)) + require.NoError(t, controllerutil.SetControllerReference(ext, rev2, testScheme)) + require.NoError(t, controllerutil.SetControllerReference(ext, rev3, testScheme)) + return []client.Object{ext, rev1, rev2, rev3} + }, + currentRev: "rev-3", + expectedRevs: []string{"rev-1"}, + }, + { + // Scenario: + // - Two different owners have revisions. + // - The owner label is used as the filter. + // - Only siblings with the same owner come back. + name: "should only include revisions matching owner label", + existingObjs: func() []client.Object { + ext := newTestClusterExtensionInternal() + ext2 := newTestClusterExtensionInternal() + ext2.Name = "test-ext-2" + ext2.UID = "test-ext-2" + + rev1 := newTestClusterExtensionRevisionInternal(t, "rev-1") + rev2 := newTestClusterExtensionRevisionInternal(t, "rev-2") + rev2.Labels[ClusterExtensionRevisionOwnerLabel] = "test-ext-2" + rev3 := newTestClusterExtensionRevisionInternal(t, "rev-3") + require.NoError(t, controllerutil.SetControllerReference(ext, rev1, testScheme)) + require.NoError(t, controllerutil.SetControllerReference(ext2, rev2, testScheme)) + require.NoError(t, controllerutil.SetControllerReference(ext, rev3, testScheme)) + return []client.Object{ext, ext2, rev1, rev2, rev3} + }, + currentRev: "rev-3", + expectedRevs: []string{"rev-1"}, + }, + { + // Scenario: + // - The revision has no owner label. + // - Without the label we skip the lookup. + // - The function returns an empty list. + name: "should return empty list when owner label missing", + existingObjs: func() []client.Object { + ext := newTestClusterExtensionInternal() + rev1 := newTestClusterExtensionRevisionInternal(t, "rev-1") + delete(rev1.Labels, ClusterExtensionRevisionOwnerLabel) + require.NoError(t, controllerutil.SetControllerReference(ext, rev1, testScheme)) + return []client.Object{ext, rev1} + }, + currentRev: "rev-1", + expectedRevs: []string{}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + testClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(tc.existingObjs()...). + Build() + + reconciler := &ClusterExtensionRevisionReconciler{ + Client: testClient, + TrackingCache: &mockTrackingCacheInternal{client: testClient}, + } + + currentRev := &ocv1.ClusterExtensionRevision{} + err := testClient.Get(t.Context(), client.ObjectKey{Name: tc.currentRev}, currentRev) + require.NoError(t, err) + + previous, err := reconciler.listPreviousRevisions(t.Context(), currentRev) + require.NoError(t, err) + + var names []string + for _, rev := range previous { + names = append(names, rev.GetName()) + } + + require.ElementsMatch(t, tc.expectedRevs, names) + }) + } +} + +func newTestClusterExtensionInternal() *ocv1.ClusterExtension { + return &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ext", + UID: "test-ext", + }, + Spec: ocv1.ClusterExtensionSpec{ + Namespace: "some-namespace", + ServiceAccount: ocv1.ServiceAccountReference{ + Name: "some-sa", + }, + Source: ocv1.SourceConfig{ + SourceType: "Catalog", + Catalog: &ocv1.CatalogFilter{ + PackageName: "my-package", + }, + }, + }, + } +} + +func newTestClusterExtensionRevisionInternal(t *testing.T, name string) *ocv1.ClusterExtensionRevision { + t.Helper() + + // Extract revision number from name (e.g., "rev-1" -> 1, "test-ext-10" -> 10) + revNum := ExtractRevisionNumber(t, name) + + rev := &ocv1.ClusterExtensionRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + UID: types.UID(name), + Generation: int64(1), + Labels: map[string]string{ + ClusterExtensionRevisionOwnerLabel: "test-ext", + }, + }, + Spec: ocv1.ClusterExtensionRevisionSpec{ + Revision: revNum, + Phases: []ocv1.ClusterExtensionRevisionPhase{ + { + Name: "everything", + Objects: []ocv1.ClusterExtensionRevisionObject{}, + }, + }, + }, + } + rev.SetGroupVersionKind(ocv1.GroupVersion.WithKind("ClusterExtensionRevision")) + return rev +} + +type mockTrackingCacheInternal struct { + client client.Client +} + +func (m *mockTrackingCacheInternal) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + return m.client.Get(ctx, key, obj, opts...) +} + +func (m *mockTrackingCacheInternal) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { + return m.client.List(ctx, list, opts...) +} + +func (m *mockTrackingCacheInternal) Free(ctx context.Context, user client.Object) error { + return nil +} + +func (m *mockTrackingCacheInternal) Watch(ctx context.Context, user client.Object, gvks sets.Set[schema.GroupVersionKind]) error { + return nil +} + +func (m *mockTrackingCacheInternal) Source(h handler.EventHandler, predicates ...predicate.Predicate) source.Source { + return nil +} diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go b/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go index 873a6cc74..f15739f08 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go @@ -49,7 +49,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_RevisionProgression(t *te revisionResult: mockRevisionResult{}, existingObjs: func() []client.Object { ext := newTestClusterExtension() - rev1 := newTestClusterExtensionRevision(clusterExtensionRevisionName) + rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName) require.NoError(t, controllerutil.SetControllerReference(ext, rev1, testScheme)) return []client.Object{ext, rev1} }, @@ -67,7 +67,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_RevisionProgression(t *te revisionResult: mockRevisionResult{}, existingObjs: func() []client.Object { ext := newTestClusterExtension() - rev1 := newTestClusterExtensionRevision(clusterExtensionRevisionName) + rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName) require.NoError(t, controllerutil.SetControllerReference(ext, rev1, testScheme)) return []client.Object{ext, rev1} }, @@ -156,7 +156,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_RevisionProgression(t *te }, existingObjs: func() []client.Object { ext := newTestClusterExtension() - rev1 := newTestClusterExtensionRevision(clusterExtensionRevisionName) + rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName) require.NoError(t, controllerutil.SetControllerReference(ext, rev1, testScheme)) return []client.Object{ext, rev1} }, @@ -181,7 +181,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_RevisionProgression(t *te }, existingObjs: func() []client.Object { ext := newTestClusterExtension() - rev1 := newTestClusterExtensionRevision(clusterExtensionRevisionName) + rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName) require.NoError(t, controllerutil.SetControllerReference(ext, rev1, testScheme)) return []client.Object{ext, rev1} }, @@ -206,7 +206,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_RevisionProgression(t *te }, existingObjs: func() []client.Object { ext := newTestClusterExtension() - rev1 := newTestClusterExtensionRevision(clusterExtensionRevisionName) + rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName) require.NoError(t, controllerutil.SetControllerReference(ext, rev1, testScheme)) meta.SetStatusCondition(&rev1.Status.Conditions, metav1.Condition{ Type: ocv1.TypeProgressing, @@ -234,7 +234,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_RevisionProgression(t *te }, existingObjs: func() []client.Object { ext := newTestClusterExtension() - rev1 := newTestClusterExtensionRevision(clusterExtensionRevisionName) + rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName) require.NoError(t, controllerutil.SetControllerReference(ext, rev1, testScheme)) return []client.Object{ext, rev1} }, @@ -266,20 +266,12 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_RevisionProgression(t *te }, existingObjs: func() []client.Object { ext := newTestClusterExtension() - prevRev1 := newTestClusterExtensionRevision("prev-rev-1") + prevRev1 := newTestClusterExtensionRevision(t, "prev-rev-1") require.NoError(t, controllerutil.SetControllerReference(ext, prevRev1, testScheme)) - prevRev2 := newTestClusterExtensionRevision("prev-rev-2") + prevRev2 := newTestClusterExtensionRevision(t, "prev-rev-2") require.NoError(t, controllerutil.SetControllerReference(ext, prevRev2, testScheme)) - rev1 := newTestClusterExtensionRevision("test-ext-1") - rev1.Spec.Previous = []ocv1.ClusterExtensionRevisionPrevious{ - { - Name: "prev-rev-1", - UID: "prev-rev-1", - }, { - Name: "prev-rev-2", - UID: "prev-rev-2", - }, - } + rev1 := newTestClusterExtensionRevision(t, "test-ext-1") + rev1.Spec.Revision = 3 // Override: name suggests 1, but we want revision 3 require.NoError(t, controllerutil.SetControllerReference(ext, rev1, testScheme)) return []client.Object{ext, prevRev1, prevRev2, rev1} }, @@ -315,7 +307,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_RevisionProgression(t *te return tc.revisionResult, nil }, }, - TrackingCache: &mockTrackingCache{}, + TrackingCache: &mockTrackingCache{client: testClient}, }).Reconcile(t.Context(), ctrl.Request{ NamespacedName: types.NamespacedName{ Name: clusterExtensionRevisionName, @@ -413,7 +405,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_ValidationError_Retries(t } { t.Run(tc.name, func(t *testing.T) { ext := newTestClusterExtension() - rev1 := newTestClusterExtensionRevision(clusterExtensionRevisionName) + rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName) require.NoError(t, controllerutil.SetControllerReference(ext, rev1, testScheme)) // create extension and cluster extension @@ -431,7 +423,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_ValidationError_Retries(t return tc.revisionResult, nil }, }, - TrackingCache: &mockTrackingCache{}, + TrackingCache: &mockTrackingCache{client: testClient}, }).Reconcile(t.Context(), ctrl.Request{ NamespacedName: types.NamespacedName{ Name: clusterExtensionRevisionName, @@ -467,7 +459,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) { name: "teardown finalizer is removed", revisionResult: mockRevisionResult{}, existingObjs: func() []client.Object { - rev1 := newTestClusterExtensionRevision(clusterExtensionRevisionName) + rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName) rev1.Finalizers = []string{ "olm.operatorframework.io/teardown", } @@ -490,7 +482,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) { revisionResult: mockRevisionResult{}, existingObjs: func() []client.Object { ext := newTestClusterExtension() - rev1 := newTestClusterExtensionRevision(clusterExtensionRevisionName) + rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName) rev1.Finalizers = []string{ "olm.operatorframework.io/teardown", } @@ -520,7 +512,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) { revisionResult: mockRevisionResult{}, existingObjs: func() []client.Object { ext := newTestClusterExtension() - rev1 := newTestClusterExtensionRevision(clusterExtensionRevisionName) + rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName) rev1.Finalizers = []string{ "olm.operatorframework.io/teardown", } @@ -549,7 +541,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) { revisionResult: mockRevisionResult{}, existingObjs: func() []client.Object { ext := newTestClusterExtension() - rev1 := newTestClusterExtensionRevision(clusterExtensionRevisionName) + rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName) rev1.Finalizers = []string{ "olm.operatorframework.io/teardown", } @@ -583,7 +575,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) { revisionResult: mockRevisionResult{}, existingObjs: func() []client.Object { ext := newTestClusterExtension() - rev1 := newTestClusterExtensionRevision(clusterExtensionRevisionName) + rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName) rev1.Finalizers = []string{ "olm.operatorframework.io/teardown", } @@ -619,7 +611,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) { revisionResult: mockRevisionResult{}, existingObjs: func() []client.Object { ext := newTestClusterExtension() - rev1 := newTestClusterExtensionRevision(clusterExtensionRevisionName) + rev1 := newTestClusterExtensionRevision(t, clusterExtensionRevisionName) rev1.Finalizers = []string{ "olm.operatorframework.io/teardown", } @@ -661,7 +653,7 @@ func Test_ClusterExtensionRevisionReconciler_Reconcile_Deletion(t *testing.T) { }, teardown: tc.revisionEngineTeardownFn(t), }, - TrackingCache: &mockTrackingCache{}, + TrackingCache: &mockTrackingCache{client: testClient}, }).Reconcile(t.Context(), ctrl.Request{ NamespacedName: types.NamespacedName{ Name: clusterExtensionRevisionName, @@ -703,14 +695,23 @@ func newTestClusterExtension() *ocv1.ClusterExtension { } } -func newTestClusterExtensionRevision(name string) *ocv1.ClusterExtensionRevision { +func newTestClusterExtensionRevision(t *testing.T, name string) *ocv1.ClusterExtensionRevision { + t.Helper() + + // Extract revision number from name (e.g., "rev-1" -> 1, "test-ext-10" -> 10) + revNum := controllers.ExtractRevisionNumber(t, name) + return &ocv1.ClusterExtensionRevision{ ObjectMeta: metav1.ObjectMeta{ Name: name, UID: types.UID(name), Generation: int64(1), + Labels: map[string]string{ + controllers.ClusterExtensionRevisionOwnerLabel: "test-ext", + }, }, Spec: ocv1.ClusterExtensionRevisionSpec{ + Revision: revNum, Phases: []ocv1.ClusterExtensionRevisionPhase{ { Name: "everything", @@ -879,14 +880,16 @@ func (m mockRevisionTeardownResult) String() string { return m.string } -type mockTrackingCache struct{} +type mockTrackingCache struct { + client client.Client +} func (m *mockTrackingCache) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { - panic("not implemented") + return m.client.Get(ctx, key, obj, opts...) } func (m *mockTrackingCache) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error { - panic("not implemented") + return m.client.List(ctx, list, opts...) } func (m *mockTrackingCache) Source(handler handler.EventHandler, predicates ...predicate.Predicate) source.Source { diff --git a/internal/operator-controller/controllers/testhelpers_test.go b/internal/operator-controller/controllers/testhelpers_test.go new file mode 100644 index 000000000..8b5aeaa96 --- /dev/null +++ b/internal/operator-controller/controllers/testhelpers_test.go @@ -0,0 +1,35 @@ +package controllers + +import ( + "strconv" + "strings" + "testing" +) + +// ExtractRevisionNumber parses the revision number from a test revision name. +// It expects names to end with a numeric revision (e.g., "rev-1", "test-ext-10"). +// Returns 1 as default if parsing fails, which is suitable for test fixtures. +// +// Note: This is a test helper and silently defaults to 1 for convenience. +// Callers are responsible for ensuring the name suffix matches the Spec.Revision value they intend to set; +// this function does not enforce or validate such a match. +func ExtractRevisionNumber(t *testing.T, name string) int64 { + t.Helper() + + parts := strings.Split(name, "-") + if len(parts) == 0 { + t.Logf("warning: revision name %q has no parts, defaulting to revision 1", name) + return 1 + } + + lastPart := parts[len(parts)-1] + revNum, err := strconv.ParseInt(lastPart, 10, 64) + if err != nil { + t.Logf("warning: revision name %q does not end with a numeric revision (got %q), defaulting to revision 1. "+ + "Test helper names should follow the pattern 'prefix-' (e.g., 'rev-1', 'test-ext-10')", + name, lastPart) + return 1 + } + + return revNum +} diff --git a/manifests/experimental-e2e.yaml b/manifests/experimental-e2e.yaml index f39f5d765..672830225 100644 --- a/manifests/experimental-e2e.yaml +++ b/manifests/experimental-e2e.yaml @@ -702,27 +702,6 @@ spec: x-kubernetes-validations: - message: phases is immutable rule: self == oldSelf || oldSelf.size() == 0 - previous: - description: Previous references previous revisions that objects can - be adopted from. - items: - properties: - name: - type: string - uid: - description: |- - UID is a type that holds unique ID values, including UUIDs. Because we - don't ONLY use UUIDs, this is an alias to string. Being a type captures - intent and helps make sure that UIDs and names do not get conflated. - type: string - required: - - name - - uid - type: object - type: array - x-kubernetes-validations: - - message: previous is immutable - rule: self == oldSelf revision: description: |- Revision is a sequence number representing a specific revision of the ClusterExtension instance. diff --git a/manifests/experimental.yaml b/manifests/experimental.yaml index 44265e031..199838eac 100644 --- a/manifests/experimental.yaml +++ b/manifests/experimental.yaml @@ -667,27 +667,6 @@ spec: x-kubernetes-validations: - message: phases is immutable rule: self == oldSelf || oldSelf.size() == 0 - previous: - description: Previous references previous revisions that objects can - be adopted from. - items: - properties: - name: - type: string - uid: - description: |- - UID is a type that holds unique ID values, including UUIDs. Because we - don't ONLY use UUIDs, this is an alias to string. Being a type captures - intent and helps make sure that UIDs and names do not get conflated. - type: string - required: - - name - - uid - type: object - type: array - x-kubernetes-validations: - - message: previous is immutable - rule: self == oldSelf revision: description: |- Revision is a sequence number representing a specific revision of the ClusterExtension instance.