From d03f8768a8d1a6315b7708de94bedffa822ea780 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Mon, 21 Sep 2026 13:45:19 -0400 Subject: [PATCH 1/2] fix: adopt migrated ClusterObjectSets Signed-off-by: Todd Short --- .../operator-controller/applier/boxcutter.go | 51 +++++++++++++++++++ .../applier/boxcutter_test.go | 46 ++++++++++++++++- internal/operator-controller/labels/labels.go | 6 +++ 3 files changed, 102 insertions(+), 1 deletion(-) diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index a52fa21c7e..55131de6a6 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -478,6 +478,17 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust return true, "", nil } + // The migration tool creates a successful revision 1 before it creates the + // ClusterExtension. Do not render that same bundle into a second revision: + // claim the pre-existing revision and use it as the initial installed state. + // Comparing the resolved bundle metadata keeps normal upgrades intact. + if revision := migratedSubscriptionRevision(existingRevisions, revisionAnnotations); revision != nil { + if err := bc.adoptRevision(ctx, ext, revision); err != nil { + return false, "", fmt.Errorf("adopting migrated revision %s: %w", revision.Name, err) + } + return true, "", nil + } + // Generate desired revision desiredRevision, err := bc.RevisionGenerator.GenerateRevision(ctx, contentFS, ext, objectLabels, revisionAnnotations) if err != nil { @@ -574,6 +585,46 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust return true, "", nil } +func migratedSubscriptionRevision(revisions []ocv1.ClusterObjectSet, desiredAnnotations map[string]string) *ocv1.ClusterObjectSet { + for i := range revisions { + revision := &revisions[i] + if revision.Spec.Revision != 1 || + revision.Annotations[labels.MigratedFromSubscriptionKey] == "" || + !meta.IsStatusConditionTrue(revision.Status.Conditions, ocv1.ClusterObjectSetTypeSucceeded) { + continue + } + + matchesBundle := true + for _, key := range []string{labels.PackageNameKey, labels.BundleNameKey, labels.BundleVersionKey, labels.BundleReferenceKey, labels.BundleReleaseKey} { + if revision.Annotations[key] != desiredAnnotations[key] { + matchesBundle = false + break + } + } + if matchesBundle { + return revision + } + } + return nil +} + +func (bc *Boxcutter) adoptRevision(ctx context.Context, ext *ocv1.ClusterExtension, revision *ocv1.ClusterObjectSet) error { + for _, ref := range revision.OwnerReferences { + if ref.Controller != nil && *ref.Controller && ref.UID != ext.UID { + return fmt.Errorf("revision is already controlled by %s %q", ref.Kind, ref.Name) + } + } + + for _, ref := range revision.OwnerReferences { + if ref.Controller != nil && *ref.Controller && ref.UID == ext.UID { + return nil + } + } + + revision.OwnerReferences = append(revision.OwnerReferences, *metav1.NewControllerRef(ext, ocv1.SchemeGroupVersion.WithKind(ocv1.ClusterExtensionKind))) + return bc.Client.Update(ctx, revision) +} + // createExternalizedRevision creates a new COS with all objects externalized to Secrets. // It follows a crash-safe three-step sequence: create Secrets, create COS, patch ownerRefs. func (bc *Boxcutter) createExternalizedRevision(ctx context.Context, ext *ocv1.ClusterExtension, desiredRevision *ocv1ac.ClusterObjectSetApplyConfiguration, existingRevisions []ocv1.ClusterObjectSet) error { diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index 25963c9a01..f432de924f 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -666,6 +666,42 @@ func TestBoxcutter_Apply(t *testing.T) { assert.Equal(t, "test-ext-1", revList.Items[0].Name) }, }, + { + name: "adopts matching migration revision without creating a duplicate", + mockBuilder: func(t *testing.T) applier.ClusterObjectSetGenerator { + ctrl := gomock.NewController(t) + return mockapplier.NewMockClusterObjectSetGenerator(ctrl) + }, + existingObjs: []client.Object{ + &ocv1.ClusterObjectSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-ext-1", + Labels: map[string]string{labels.OwnerNameKey: ext.Name}, + Annotations: map[string]string{ + labels.MigratedFromSubscriptionKey: "test-namespace/test-subscription", + labels.PackageNameKey: "test-package", + labels.BundleNameKey: "test-package.v1.0.0", + labels.BundleVersionKey: "1.0.0", + labels.BundleReferenceKey: "registry.example/test-package@sha256:123", + }, + }, + Spec: ocv1.ClusterObjectSetSpec{Revision: 1}, + Status: ocv1.ClusterObjectSetStatus{Conditions: []metav1.Condition{{ + Type: ocv1.ClusterObjectSetTypeSucceeded, + Status: metav1.ConditionTrue, + }}}, + }, + }, + validate: func(t *testing.T, c client.Client) { + revList := &ocv1.ClusterObjectSetList{} + require.NoError(t, c.List(t.Context(), revList, client.MatchingLabels{labels.OwnerNameKey: ext.Name})) + require.Len(t, revList.Items, 1) + assert.Equal(t, "test-ext-1", revList.Items[0].Name) + require.Len(t, revList.Items[0].OwnerReferences, 1) + assert.Equal(t, ext.Name, revList.Items[0].OwnerReferences[0].Name) + assert.Equal(t, ext.UID, revList.Items[0].OwnerReferences[0].UID) + }, + }, { name: "new revision created when objects in new revision are different", mockBuilder: func(t *testing.T) applier.ClusterObjectSetGenerator { @@ -1081,12 +1117,20 @@ func TestBoxcutter_Apply(t *testing.T) { // Execute revisionAnnotations := map[string]string{} - if tc.name == "annotation-only update (same phases, different annotations)" { + switch tc.name { + case "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", } + case "adopts matching migration revision without creating a duplicate": + revisionAnnotations = map[string]string{ + labels.PackageNameKey: "test-package", + labels.BundleNameKey: "test-package.v1.0.0", + labels.BundleVersionKey: "1.0.0", + labels.BundleReferenceKey: "registry.example/test-package@sha256:123", + } } completed, status, err := boxcutter.Apply(t.Context(), testFS, ext, nil, revisionAnnotations) diff --git a/internal/operator-controller/labels/labels.go b/internal/operator-controller/labels/labels.go index 3a0cdaf46a..e2dfd37aca 100644 --- a/internal/operator-controller/labels/labels.go +++ b/internal/operator-controller/labels/labels.go @@ -51,4 +51,10 @@ const ( // that were created during migration from Helm releases. This label is used // to distinguish migrated revisions from those created by normal Boxcutter operation. MigratedFromHelmKey = "olm.operatorframework.io/migrated-from-helm" + + // MigratedFromSubscriptionKey is the annotation placed on a revision created + // by the OLM v0-to-v1 migration tool. A matching revision 1 is adopted by a + // subsequently-created ClusterExtension instead of being replaced by its + // initial Boxcutter reconciliation. + MigratedFromSubscriptionKey = "olm.operatorframework.io/migrated-from-subscription" ) From 03bd94dd62a864949dc5e543ab02e6ab55339079 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Mon, 21 Sep 2026 15:23:31 -0400 Subject: [PATCH 2/2] test: cover migrated revision release metadata Signed-off-by: Todd Short --- internal/operator-controller/applier/boxcutter.go | 4 ++++ internal/operator-controller/applier/boxcutter_test.go | 2 ++ 2 files changed, 6 insertions(+) diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 55131de6a6..412b8e4e52 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -585,6 +585,8 @@ func (bc *Boxcutter) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust return true, "", nil } +// migratedSubscriptionRevision finds a successful migration-created initial +// revision whose resolved bundle metadata matches the desired revision. func migratedSubscriptionRevision(revisions []ocv1.ClusterObjectSet, desiredAnnotations map[string]string) *ocv1.ClusterObjectSet { for i := range revisions { revision := &revisions[i] @@ -608,6 +610,8 @@ func migratedSubscriptionRevision(revisions []ocv1.ClusterObjectSet, desiredAnno return nil } +// adoptRevision gives the ClusterExtension controller ownership of a matching +// migration-created revision without rendering a duplicate revision. func (bc *Boxcutter) adoptRevision(ctx context.Context, ext *ocv1.ClusterExtension, revision *ocv1.ClusterObjectSet) error { for _, ref := range revision.OwnerReferences { if ref.Controller != nil && *ref.Controller && ref.UID != ext.UID { diff --git a/internal/operator-controller/applier/boxcutter_test.go b/internal/operator-controller/applier/boxcutter_test.go index f432de924f..f46cd6b41f 100644 --- a/internal/operator-controller/applier/boxcutter_test.go +++ b/internal/operator-controller/applier/boxcutter_test.go @@ -683,6 +683,7 @@ func TestBoxcutter_Apply(t *testing.T) { labels.BundleNameKey: "test-package.v1.0.0", labels.BundleVersionKey: "1.0.0", labels.BundleReferenceKey: "registry.example/test-package@sha256:123", + labels.BundleReleaseKey: "42", }, }, Spec: ocv1.ClusterObjectSetSpec{Revision: 1}, @@ -1130,6 +1131,7 @@ func TestBoxcutter_Apply(t *testing.T) { labels.BundleNameKey: "test-package.v1.0.0", labels.BundleVersionKey: "1.0.0", labels.BundleReferenceKey: "registry.example/test-package@sha256:123", + labels.BundleReleaseKey: "42", } } completed, status, err := boxcutter.Apply(t.Context(), testFS, ext, nil, revisionAnnotations)