From 4618467850647296184d8b42b53daf4ffa155b27 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Tue, 11 Nov 2025 15:28:34 -0500 Subject: [PATCH] Fix duplicate ClusterExtensionRevisions during Helm-to-Boxcutter migration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit fixes OCPBUGS-62943 where two ClusterExtensionRevisions were being created during Helm-to-Boxcutter migration instead of just one. Root causes: 1. Manifest ordering inconsistency: CRDs from Helm release manifest and bundle manifest appeared in different orders, causing PhaseSort to produce different phase structures even though they contained the same objects. 2. CollisionProtection mismatch: The migrated revision had collisionProtection=None (needed to adopt Helm-managed resources) while the bundle-generated revision had collisionProtection=Prevent (the default value). Solution: 1. Added deterministic sorting in PhaseSort (phase.go): - Sort objects within each phase by Group, Version, Kind, Namespace, Name - Ensures consistent phase structure regardless of input order - Critical for comparing revisions from different sources 2. Added CollisionProtection preservation (boxcutter.go): - New preserveCollisionProtection() function copies CollisionProtection values from current revision to desired revision for matching objects - New objectKey() helper generates unique keys based on GVK+namespace+name - Called before patching to ensure CollisionProtection values match With these changes, only a single ClusterExtensionRevision is created during Helm-to-Boxcutter migration, as expected. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Signed-off-by: Todd Short --- .../operator-controller/applier/boxcutter.go | 38 +++++++++++++ internal/operator-controller/applier/phase.go | 56 +++++++++++++++++++ .../operator-controller/applier/phase_test.go | 8 +-- 3 files changed, 98 insertions(+), 4 deletions(-) diff --git a/internal/operator-controller/applier/boxcutter.go b/internal/operator-controller/applier/boxcutter.go index 1914b80e8f..3d53bac5d3 100644 --- a/internal/operator-controller/applier/boxcutter.go +++ b/internal/operator-controller/applier/boxcutter.go @@ -308,6 +308,12 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust desiredRevision.Spec.Revision = currentRevision.Spec.Revision desiredRevision.Name = currentRevision.Name + // Preserve CollisionProtection settings from the current revision to avoid + // creating a new revision when the only difference is the CollisionProtection value. + // This is critical during Helm-to-Boxcutter migration where the migrated revision + // has CollisionProtection=None to adopt Helm-managed resources. + preserveCollisionProtection(desiredRevision, currentRevision) + err := bc.createOrUpdate(ctx, desiredRevision) switch { case apierrors.IsInvalid(err): @@ -380,6 +386,38 @@ func (bc *Boxcutter) apply(ctx context.Context, contentFS fs.FS, ext *ocv1.Clust return true, "", nil } +// preserveCollisionProtection copies the CollisionProtection settings from the current revision to the desired revision +// for objects that have matching GVK, namespace, and name. This ensures that when patching an existing revision, +// we don't inadvertently change the CollisionProtection value, which would cause the patch to fail since +// CollisionProtection is an immutable field. +func preserveCollisionProtection(desired, current *ocv1.ClusterExtensionRevision) { + // Build a map of objects in the current revision by their identity (GVK + namespace + name) + currentObjects := make(map[string]ocv1.CollisionProtection) + for _, phase := range current.Spec.Phases { + for _, obj := range phase.Objects { + key := objectKey(&obj.Object) + currentObjects[key] = obj.CollisionProtection + } + } + + // Update desired revision objects to use the same CollisionProtection as current revision + for i := range desired.Spec.Phases { + for j := range desired.Spec.Phases[i].Objects { + desiredObj := &desired.Spec.Phases[i].Objects[j] + key := objectKey(&desiredObj.Object) + if collisionProtection, found := currentObjects[key]; found { + desiredObj.CollisionProtection = collisionProtection + } + } + } +} + +// objectKey generates a unique key for an object based on its GVK, namespace, and name +func objectKey(obj *unstructured.Unstructured) string { + gvk := obj.GroupVersionKind() + return fmt.Sprintf("%s/%s/%s/%s/%s", gvk.Group, gvk.Version, gvk.Kind, obj.GetNamespace(), obj.GetName()) +} + // 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. diff --git a/internal/operator-controller/applier/phase.go b/internal/operator-controller/applier/phase.go index 9ae31db6a7..94e16dc524 100644 --- a/internal/operator-controller/applier/phase.go +++ b/internal/operator-controller/applier/phase.go @@ -1,6 +1,8 @@ package applier import ( + "slices" + "k8s.io/apimachinery/pkg/runtime/schema" ocv1 "github.com/operator-framework/operator-controller/api/v1" @@ -111,6 +113,57 @@ func init() { } } +// Sort objects within the phase deterministically by Group, Version, Kind, Namespace, Name +// to ensure consistent ordering regardless of input order. This is critical for +// Helm-to-Boxcutter migration where the same resources may come from different sources +// (Helm release manifest vs bundle manifest) and need to produce identical phases. +func compareClusterExtensionRevisionObjects(a, b ocv1.ClusterExtensionRevisionObject) int { + aGVK := a.Object.GroupVersionKind() + bGVK := b.Object.GroupVersionKind() + + // Compare Group + if aGVK.Group < bGVK.Group { + return -1 + } else if aGVK.Group > bGVK.Group { + return 1 + } + + // Compare Version + if aGVK.Version < bGVK.Version { + return -1 + } else if aGVK.Version > bGVK.Version { + return 1 + } + + // Compare Kind + if aGVK.Kind < bGVK.Kind { + return -1 + } else if aGVK.Kind > bGVK.Kind { + return 1 + } + + // Compare Namespace + aNs := a.Object.GetNamespace() + bNs := b.Object.GetNamespace() + if aNs < bNs { + return -1 + } else if aNs > bNs { + return 1 + } + + // Compare Name + aName := a.Object.GetName() + bName := b.Object.GetName() + if aName < bName { + return -1 + } + if aName > bName { + return 1 + } + + return 0 +} + // PhaseSort takes an unsorted list of objects and organizes them into sorted phases. // Each phase will be applied in order according to DefaultPhaseOrder. Objects // within a single phase are applied simultaneously. @@ -125,6 +178,9 @@ func PhaseSort(unsortedObjs []ocv1.ClusterExtensionRevisionObject) []ocv1.Cluste for _, phaseName := range defaultPhaseOrder { if objs, ok := phaseMap[phaseName]; ok { + // Sort objects within the phase deterministically + slices.SortFunc(objs, compareClusterExtensionRevisionObjects) + phasesSorted = append(phasesSorted, ocv1.ClusterExtensionRevisionPhase{ Name: string(phaseName), Objects: objs, diff --git a/internal/operator-controller/applier/phase_test.go b/internal/operator-controller/applier/phase_test.go index 3f2d85d0b1..ae43325bfd 100644 --- a/internal/operator-controller/applier/phase_test.go +++ b/internal/operator-controller/applier/phase_test.go @@ -262,16 +262,16 @@ func Test_PhaseSort(t *testing.T) { { Object: unstructured.Unstructured{ Object: map[string]interface{}{ - "apiVersion": "apps/v1", - "kind": "Deployment", + "apiVersion": "v1", + "kind": "ConfigMap", }, }, }, { Object: unstructured.Unstructured{ Object: map[string]interface{}{ - "apiVersion": "v1", - "kind": "ConfigMap", + "apiVersion": "apps/v1", + "kind": "Deployment", }, }, },