🐛 OCPBUGS-78455: fix(Boxcutter-Collision): Fix collision detection bypass after ClusterExtension upgrade by camilamacedo86 · Pull Request #2578 · operator-framework/operator-controller

camilamacedo86 · 2026-03-20T09:22:50Z

When a ClusterExtension was upgraded (e.g., v1.0.0 to v1.0.1), the upgrade set a higher revision number on managed objects. A second

ClusterExtension installing the same package was then allowed because boxcutter's revision linearity check returned ActionProgressed instead of ActionCollision, silently skipping the conflict.

Fix: after boxcutter reconcile, check ActionProgressed results for objects whose controller ownerReference belongs to a revision from a different ClusterExtension, and treat those as collisions.

Motivated by: https://redhat.atlassian.net/browse/OCPBUGS-78455

netlify · 2026-03-20T09:22:55Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`b3688d3`
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/69c242654b49360008b28620
😎 Deploy Preview	https://deploy-preview-2578--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

openshift-ci · 2026-03-20T09:22:56Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign grokspawn for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copilot

Pull request overview

Fixes a Boxcutter collision-detection bypass that could allow a second ClusterExtension to install the same package after the first ClusterExtension upgraded and bumped managed-object revision numbers.

Changes:

Treat ActionProgressed results as collisions when the reconciled object is controller-owned by a different ClusterExtensionRevision (foreign ownerRef).
Add an E2E scenario covering “upgrade then duplicate install” and a new step to ensure both ClusterExtension resources are cleaned up.
Add unit tests validating the new “foreign revision ownerRef” collision behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
`internal/operator-controller/controllers/clusterextensionrevision_controller.go`	Adds foreign-ownerRef collision detection for `ActionProgressed` objects and tracks sibling CER names during reconcile.
`internal/operator-controller/controllers/clusterextensionrevision_controller_test.go`	Adds unit coverage for foreign/sibling/non-CER controller ownerRef cases.
`test/e2e/steps/steps.go`	Adds a step to track the currently-applied `ClusterExtension` for cleanup when a scenario applies a second CE.
`test/e2e/features/update.feature`	Adds an E2E scenario asserting collisions are detected after an upgrade when installing a duplicate CE.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/operator-controller/controllers/clusterextensionrevision_controller.go

codecov · 2026-03-20T10:00:06Z

Codecov Report

❌ Patch coverage is 91.66667% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.89%. Comparing base (a307a6d) to head (b3688d3).

Files with missing lines	Patch %	Lines
...controllers/clusterextensionrevision_controller.go	91.66%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2578      +/-   ##
==========================================
+ Coverage   65.84%   67.89%   +2.04%     
==========================================
  Files         137      137              
  Lines        9560     9586      +26     
==========================================
+ Hits         6295     6508     +213     
+ Misses       2795     2581     -214     
- Partials      470      497      +27

Flag	Coverage Δ
e2e	`38.04% <0.00%> (+26.86%)`	⬆️
experimental-e2e	`51.24% <80.55%> (+0.17%)`	⬆️
unit	`52.95% <86.11%> (+0.14%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

grokspawn · 2026-03-20T13:56:20Z

internal/operator-controller/controllers/clusterextensionrevision_controller.go

+// nil when the controller is a sibling revision (same ClusterExtension) or not a CER at all.
+func getForeignRevisionController(obj metav1.Object, siblingRevisionNames sets.Set[string]) *metav1.OwnerReference {
+	for i := range obj.GetOwnerReferences() {
+		ref := obj.GetOwnerReferences()[i]


why are we avoiding setting this to a local and iterating over it? If we're concerned that the contents can change over the course of this call then couldn't we end up in a situation where the first call determines a bounds which exceeds the returned results later in the loop?
Like if L551 says i := 12, but then obj.GetOwnerReferences() is updated to hold fewer than 12 items.

Why not

refs := obj.GetOwnerReferences() for i := range refs { ref := refs[i]

I think we can. Goog Catcher 🥇

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/operator-controller/controllers/clusterextensionrevision_controller_test.go

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

internal/operator-controller/controllers/clusterextensionrevision_controller.go

internal/operator-controller/controllers/clusterextensionrevision_controller_test.go

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-23T16:48:44Z

internal/operator-controller/controllers/clusterextensionrevision_controller.go

+			if ores.Action() == machinery.ActionProgressed {
+				if controllerRef := getForeignRevisionController(ores.Object(), siblingRevisionNames); controllerRef != nil {
+					collidingObjs = append(collidingObjs, ores.String()+fmt.Sprintf("\nConflicting Owner: %s", controllerRef.String()))
+				}
+			}


The new foreign-owner collision check depends on siblingRevisionNames being complete. If labels.OwnerNameKey is missing on the reconciling CER, buildBoxcutterPhases only seeds the set with cer.Name, so an ActionProgressed object still controlled by another revision of the same ClusterExtension will be misclassified as a foreign collision. Consider skipping the foreign-owner collision check (or treating all CER controllers as "sibling") when the owner label is missing, to preserve the current “missing label => no sibling lookup” behavior.

Copilot · 2026-03-23T16:48:45Z

internal/operator-controller/controllers/clusterextensionrevision_controller.go

+func (c *ClusterExtensionRevisionReconciler) buildBoxcutterPhases(ctx context.Context, cer *ocv1.ClusterExtensionRevision) ([]boxcutter.Phase, []boxcutter.RevisionReconcileOption, sets.Set[string], error) {
+	siblingRevisionNames := sets.New[string](cer.Name)
+	var previousObjs []client.Object
+
+	if ownerLabel, ok := cer.Labels[labels.OwnerNameKey]; ok {
+		revList := &ocv1.ClusterExtensionRevisionList{}
+		if err := c.TrackingCache.List(ctx, revList, client.MatchingLabels{
+			labels.OwnerNameKey: ownerLabel,


When labels.OwnerNameKey is missing on the CER, this function returns siblingRevisionNames containing only cer.Name and an empty previousObjs. With the new ActionProgressed foreign-controller detection, that incomplete sibling set can cause false-positive collisions (any CER controller ref other than cer.Name will look “foreign”). Consider returning an explicit hasOwnerLabel flag (or otherwise ensuring the sibling set is complete) so callers can safely decide whether to enforce the foreign-owner collision check.

…rExtension upgrade When a ClusterExtension was upgraded (e.g., v1.0.0 to v1.0.1), the upgrade set a higher revision number on managed objects. A second ClusterExtension installing the same package was then allowed because boxcutter's revision linearity check returned ActionProgressed instead of ActionCollision, silently skipping the conflict. Fix: after boxcutter reconcile, check ActionProgressed results for objects whose controller ownerReference belongs to a revision from a different ClusterExtension, and treat those as collisions. Generated-by: Claude/Cursor

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-24T07:55:17Z

internal/operator-controller/controllers/clusterextensionrevision_controller.go

+func (c *ClusterExtensionRevisionReconciler) buildBoxcutterPhases(ctx context.Context, cer *ocv1.ClusterExtensionRevision) ([]boxcutter.Phase, []boxcutter.RevisionReconcileOption, sets.Set[string], error) {
+	var siblingRevisionNames sets.Set[string]
+	previousObjs := make([]client.Object, 0)
+
+	if ownerLabel, ok := cer.Labels[labels.OwnerNameKey]; ok {
+		revList := &ocv1.ClusterExtensionRevisionList{}
+		if err := c.TrackingCache.List(ctx, revList, client.MatchingLabels{
+			labels.OwnerNameKey: ownerLabel,
+		}); err != nil {
+			return nil, nil, nil, fmt.Errorf("listing revisions: %w", err)
+		}

-	// Convert to []client.Object for boxcutter
-	previousObjs := make([]client.Object, len(previous))
-	for i, rev := range previous {
-		previousObjs[i] = rev
+		siblingRevisionNames = sets.New[string](cer.Name)
+		for i := range revList.Items {
+			r := &revList.Items[i]
+			siblingRevisionNames.Insert(r.Name)
+			if r.Name == cer.Name {
+				continue
+			}
+			if r.Spec.LifecycleState == ocv1.ClusterExtensionRevisionLifecycleStateArchived ||
+				!r.DeletionTimestamp.IsZero() {
+				continue
+			}
+			if r.Spec.Revision >= cer.Spec.Revision {
+				continue
+			}
+			previousObjs = append(previousObjs, r)
+		}


buildBoxcutterPhases re-implements the same filtering/listing logic as listPreviousRevisions (owner label lookup, TrackingCache.List, and the archived/deleting/revision-number filters). This duplication makes it easy for the two paths (previous owners used for boxcutter vs. previous revisions archived on completion) to drift over time. Consider refactoring so buildBoxcutterPhases reuses listPreviousRevisions for previousObjs and only does the additional sibling-name collection separately (or extend listPreviousRevisions to optionally return both).

Copilot AI review requested due to automatic review settings March 20, 2026 09:22

openshift-ci bot requested review from OchiengEd and pedjak March 20, 2026 09:22

camilamacedo86 changed the title ~~🐛 fix(Boxcutter-Collision): Fix collision detection bypass after Cluste…~~ 🐛 fix(Boxcutter-Collision): Fix collision detection bypass after ClusterExtension upgrade Mar 20, 2026

Copilot started reviewing on behalf of camilamacedo86 March 20, 2026 09:23 View session

camilamacedo86 requested a review from perdasilva March 20, 2026 09:25

Copilot AI reviewed Mar 20, 2026

View reviewed changes

internal/operator-controller/controllers/clusterextensionrevision_controller.go Outdated Show resolved Hide resolved

internal/operator-controller/controllers/clusterextensionrevision_controller.go Outdated Show resolved Hide resolved

camilamacedo86 force-pushed the fix-issu-install-package-upgrade-same-box branch from a73cf6d to 8b504c8 Compare March 20, 2026 09:39

grokspawn reviewed Mar 20, 2026

View reviewed changes

camilamacedo86 force-pushed the fix-issu-install-package-upgrade-same-box branch from 8b504c8 to 36f661c Compare March 20, 2026 14:00

Copilot AI review requested due to automatic review settings March 20, 2026 14:00

Copilot started reviewing on behalf of camilamacedo86 March 20, 2026 14:00 View session

Copilot AI reviewed Mar 20, 2026

View reviewed changes

internal/operator-controller/controllers/clusterextensionrevision_controller_test.go Outdated Show resolved Hide resolved

camilamacedo86 force-pushed the fix-issu-install-package-upgrade-same-box branch from 36f661c to 2b87748 Compare March 20, 2026 14:13

camilamacedo86 requested review from Copilot and grokspawn March 20, 2026 14:28

Copilot started reviewing on behalf of camilamacedo86 March 20, 2026 14:28 View session

Copilot AI reviewed Mar 20, 2026

View reviewed changes

camilamacedo86 changed the title ~~🐛 fix(Boxcutter-Collision): Fix collision detection bypass after ClusterExtension upgrade~~ 🐛 OCPBUGS-78455: fix(Boxcutter-Collision): Fix collision detection bypass after ClusterExtension upgrade Mar 20, 2026

camilamacedo86 force-pushed the fix-issu-install-package-upgrade-same-box branch 2 times, most recently from 6ad05bf to b7f071a Compare March 23, 2026 16:43

Copilot AI review requested due to automatic review settings March 23, 2026 16:43

Copilot started reviewing on behalf of camilamacedo86 March 23, 2026 16:43 View session

Copilot AI reviewed Mar 23, 2026

View reviewed changes

camilamacedo86 force-pushed the fix-issu-install-package-upgrade-same-box branch from b7f071a to 4bc4776 Compare March 23, 2026 17:46

Copilot AI review requested due to automatic review settings March 24, 2026 07:50

camilamacedo86 force-pushed the fix-issu-install-package-upgrade-same-box branch from 4bc4776 to b3688d3 Compare March 24, 2026 07:50

Copilot started reviewing on behalf of camilamacedo86 March 24, 2026 07:51 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

Conversation

camilamacedo86 commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for olmv1 ready!

Uh oh!

openshift-ci bot commented Mar 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Mar 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

grokspawn Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Mar 20, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

camilamacedo86 commented Mar 20, 2026 •

edited

Loading

netlify bot commented Mar 20, 2026 •

edited

Loading

codecov bot commented Mar 20, 2026 •

edited

Loading