🐛 OCPBUGS-78455: fix(Boxcutter-Collision): Fix collision detection bypass after ClusterExtension upgrade

internal/operator-controller/controllers/clusterextensionrevision_controller.go

@@ -142,7 +142,7 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, cer
 		return c.delete(ctx, cer)
 	}
 
-	phases, opts, err := c.buildBoxcutterPhases(ctx, cer)
+	phases, opts, siblingRevisionNames, err := c.buildBoxcutterPhases(ctx, cer)
 	if err != nil {
 		setRetryingConditions(cer, err.Error())
 		return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err)
@@ -210,6 +210,11 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, cer
 			if ores.Action() == machinery.ActionCollision {
 				collidingObjs = append(collidingObjs, ores.String())
 			}
+			if ores.Action() == machinery.ActionProgressed {
+				if controllerRef := getForeignRevisionController(ores.Object(), siblingRevisionNames); controllerRef != nil {
+					collidingObjs = append(collidingObjs, ores.String()+fmt.Sprintf("Conflicting Owner: %s\n", controllerRef.String()))
+				}
+			}
 		}
 
 		if len(collidingObjs) > 0 {
@@ -460,21 +465,38 @@ func (c *ClusterExtensionRevisionReconciler) listPreviousRevisions(ctx context.C
 	return previous, nil
 }
 
-func (c *ClusterExtensionRevisionReconciler) buildBoxcutterPhases(ctx context.Context, cer *ocv1.ClusterExtensionRevision) ([]boxcutter.Phase, []boxcutter.RevisionReconcileOption, error) {
-	previous, err := c.listPreviousRevisions(ctx, cer)
-	if err != nil {
-		return nil, nil, fmt.Errorf("listing previous revisions: %w", err)
-	}
+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,
+		}); 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
+		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)
+		}
 	}
 
 	progressionProbes, err := buildProgressionProbes(cer.Spec.ProgressionProbes)
 	if err != nil {
-		return nil, nil, err
+		return nil, nil, nil, err
 	}
 
 	opts := []boxcutter.RevisionReconcileOption{
@@ -507,7 +529,7 @@ func (c *ClusterExtensionRevisionReconciler) buildBoxcutterPhases(ctx context.Co
 		}
 		phases = append(phases, boxcutter.NewPhase(specPhase.Name, objs))
 	}
-	return phases, opts, nil
+	return phases, opts, siblingRevisionNames, nil
 }
 
 // EffectiveCollisionProtection resolves the collision protection value using
@@ -522,6 +544,24 @@ func EffectiveCollisionProtection(cp ...ocv1.CollisionProtection) ocv1.Collision
 	return ecp
 }
 
+// getForeignRevisionController returns the controller OwnerReference if the object is
+// controlled by a ClusterExtensionRevision from a different ClusterExtension. It returns
+// 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 {
+	refs := obj.GetOwnerReferences()
+	for i := range refs {
+		ref := refs[i]
+		if ref.Controller != nil && *ref.Controller &&
+			ref.Kind == ocv1.ClusterExtensionRevisionKind &&
+			ref.APIVersion == ocv1.GroupVersion.String() {
+			if !siblingRevisionNames.Has(ref.Name) {
+				return &ref
+			}
+		}
+	}
+	return nil
+}
+
 // buildProgressionProbes creates a set of boxcutter probes from the fields provided in the CER's spec.progressionProbes.
 // Returns nil and an error if encountered while attempting to build the probes.
 func buildProgressionProbes(progressionProbes []ocv1.ProgressionProbe) (probing.And, error) {

internal/operator-controller/controllers/clusterextensionrevision_controller_test.go

@@ -1111,7 +1111,7 @@ func newTestClusterExtensionRevision(t *testing.T, revisionName string, ext *ocv
 				labels.ServiceAccountNamespaceKey: ext.Spec.Namespace,
 			},
 			Labels: map[string]string{
-				labels.OwnerNameKey: "test-ext",
+				labels.OwnerNameKey: ext.Name,
 			},
 		},
 		Spec: ocv1.ClusterExtensionRevisionSpec{
@@ -1344,6 +1344,258 @@ func (m *mockTrackingCache) Free(ctx context.Context, user client.Object) error
 	return nil
 }
 
+func Test_ClusterExtensionRevisionReconciler_Reconcile_ForeignRevisionCollision(t *testing.T) {
+	testScheme := newScheme(t)
+
+	for _, tc := range []struct {
+		name                    string
+		reconcilingRevisionName string
+		existingObjs            func() []client.Object
+		revisionResult          machinery.RevisionResult
+		expectCollision         bool
+	}{
+		{
+			name:                    "ActionProgressed with foreign controller ownerRef is treated as collision",
+			reconcilingRevisionName: "ext-B-1",
+			existingObjs: func() []client.Object {
+				extA := &ocv1.ClusterExtension{
+					ObjectMeta: metav1.ObjectMeta{Name: "ext-A", UID: "ext-A-uid"},
+					Spec: ocv1.ClusterExtensionSpec{
+						Namespace:      "ns-a",
+						ServiceAccount: ocv1.ServiceAccountReference{Name: "sa-a"},
+						Source: ocv1.SourceConfig{
+							SourceType: ocv1.SourceTypeCatalog,
+							Catalog:    &ocv1.CatalogFilter{PackageName: "pkg"},
+						},
+					},
+				}
+				extB := &ocv1.ClusterExtension{
+					ObjectMeta: metav1.ObjectMeta{Name: "ext-B", UID: "ext-B-uid"},
+					Spec: ocv1.ClusterExtensionSpec{
+						Namespace:      "ns-b",
+						ServiceAccount: ocv1.ServiceAccountReference{Name: "sa-b"},
+						Source: ocv1.SourceConfig{
+							SourceType: ocv1.SourceTypeCatalog,
+							Catalog:    &ocv1.CatalogFilter{PackageName: "pkg"},
+						},
+					},
+				}
+				// CER from ext-A's upgrade (revision 2) - this is the "foreign" owner
+				cerA2 := newTestClusterExtensionRevision(t, "ext-A-2", extA, testScheme)
+
+				// CER from ext-B (revision 1) - the one being reconciled
+				cerB1 := newTestClusterExtensionRevision(t, "ext-B-1", extB, testScheme)
+
+				return []client.Object{extA, extB, cerA2, cerB1}
+			},
+			revisionResult: mockRevisionResult{
+				phases: []machinery.PhaseResult{
+					mockPhaseResult{
+						name: "everything",
+						objects: []machinery.ObjectResult{
+							mockObjectResult{
+								action: machinery.ActionProgressed,
+								object: func() machinery.Object {
+									obj := &unstructured.Unstructured{
+										Object: map[string]interface{}{
+											"apiVersion": "apiextensions.k8s.io/v1",
+											"kind":       "CustomResourceDefinition",
+											"metadata": map[string]interface{}{
+												"name": "widgets.example.com",
+												"ownerReferences": []interface{}{
+													map[string]interface{}{
+														"apiVersion":         ocv1.GroupVersion.String(),
+														"kind":               ocv1.ClusterExtensionRevisionKind,
+														"name":               "ext-A-2",
+														"uid":                "ext-A-2",
+														"controller":         true,
+														"blockOwnerDeletion": true,
+													},
+												},
+											},
+										},
+									}
+									return obj
+								}(),
+								probes: machinerytypes.ProbeResultContainer{
+									boxcutter.ProgressProbeType: {
+										Status: machinerytypes.ProbeStatusTrue,
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			expectCollision: true,
+		},
+		{
+			name:                    "ActionProgressed with sibling controller ownerRef is NOT a collision",
+			reconcilingRevisionName: "ext-A-1",
+			existingObjs: func() []client.Object {
+				extA := &ocv1.ClusterExtension{
+					ObjectMeta: metav1.ObjectMeta{Name: "ext-A", UID: "ext-A-uid"},
+					Spec: ocv1.ClusterExtensionSpec{
+						Namespace:      "ns-a",
+						ServiceAccount: ocv1.ServiceAccountReference{Name: "sa-a"},
+						Source: ocv1.SourceConfig{
+							SourceType: ocv1.SourceTypeCatalog,
+							Catalog:    &ocv1.CatalogFilter{PackageName: "pkg"},
+						},
+					},
+				}
+				// CER from ext-A revision 1 (being reconciled, older)
+				cerA1 := newTestClusterExtensionRevision(t, "ext-A-1", extA, testScheme)
+				// CER from ext-A revision 2 (newer, already progressed)
+				cerA2 := newTestClusterExtensionRevision(t, "ext-A-2", extA, testScheme)
+
+				return []client.Object{extA, cerA1, cerA2}
+			},
+			revisionResult: mockRevisionResult{
+				phases: []machinery.PhaseResult{
+					mockPhaseResult{
+						name: "everything",
+						objects: []machinery.ObjectResult{
+							mockObjectResult{
+								action: machinery.ActionProgressed,
+								object: func() machinery.Object {
+									obj := &unstructured.Unstructured{
+										Object: map[string]interface{}{
+											"apiVersion": "apiextensions.k8s.io/v1",
+											"kind":       "CustomResourceDefinition",
+											"metadata": map[string]interface{}{
+												"name": "widgets.example.com",
+												"ownerReferences": []interface{}{
+													map[string]interface{}{
+														"apiVersion":         ocv1.GroupVersion.String(),
+														"kind":               ocv1.ClusterExtensionRevisionKind,
+														"name":               "ext-A-2",
+														"uid":                "ext-A-2",
+														"controller":         true,
+														"blockOwnerDeletion": true,
+													},
+												},
+											},
+										},
+									}
+									return obj
+								}(),
+								probes: machinerytypes.ProbeResultContainer{
+									boxcutter.ProgressProbeType: {
+										Status: machinerytypes.ProbeStatusTrue,
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			expectCollision: false,
+		},
+		{
+			name:                    "ActionProgressed with non-CER controller ownerRef is NOT a collision",
+			reconcilingRevisionName: "ext-B-1",
+			existingObjs: func() []client.Object {
+				extB := &ocv1.ClusterExtension{
+					ObjectMeta: metav1.ObjectMeta{Name: "ext-B", UID: "ext-B-uid"},
+					Spec: ocv1.ClusterExtensionSpec{
+						Namespace:      "ns-b",
+						ServiceAccount: ocv1.ServiceAccountReference{Name: "sa-b"},
+						Source: ocv1.SourceConfig{
+							SourceType: ocv1.SourceTypeCatalog,
+							Catalog:    &ocv1.CatalogFilter{PackageName: "pkg"},
+						},
+					},
+				}
+				cerB1 := newTestClusterExtensionRevision(t, "ext-B-1", extB, testScheme)
+
+				return []client.Object{extB, cerB1}
+			},
+			revisionResult: mockRevisionResult{
+				phases: []machinery.PhaseResult{
+					mockPhaseResult{
+						name: "everything",
+						objects: []machinery.ObjectResult{
+							mockObjectResult{
+								action: machinery.ActionProgressed,
+								object: func() machinery.Object {
+									obj := &unstructured.Unstructured{
+										Object: map[string]interface{}{
+											"apiVersion": "v1",
+											"kind":       "ConfigMap",
+											"metadata": map[string]interface{}{
+												"name":      "some-cm",
+												"namespace": "default",
+												"ownerReferences": []interface{}{
+													map[string]interface{}{
+														"apiVersion":         "apps/v1",
+														"kind":               "Deployment",
+														"name":               "some-deployment",
+														"uid":                "deploy-uid",
+														"controller":         true,
+														"blockOwnerDeletion": true,
+													},
+												},
+											},
+										},
+									}
+									return obj
+								}(),
+								probes: machinerytypes.ProbeResultContainer{
+									boxcutter.ProgressProbeType: {
+										Status: machinerytypes.ProbeStatusTrue,
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+			expectCollision: false,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			testClient := fake.NewClientBuilder().
+				WithScheme(testScheme).
+				WithStatusSubresource(&ocv1.ClusterExtensionRevision{}).
+				WithObjects(tc.existingObjs()...).
+				Build()
+
+			mockEngine := &mockRevisionEngine{
+				reconcile: func(ctx context.Context, rev machinerytypes.Revision, opts ...machinerytypes.RevisionReconcileOption) (machinery.RevisionResult, error) {
+					return tc.revisionResult, nil
+				},
+			}
+			result, err := (&controllers.ClusterExtensionRevisionReconciler{
+				Client:                testClient,
+				RevisionEngineFactory: &mockRevisionEngineFactory{engine: mockEngine},
+				TrackingCache:         &mockTrackingCache{client: testClient},
+			}).Reconcile(t.Context(), ctrl.Request{
+				NamespacedName: types.NamespacedName{
+					Name: tc.reconcilingRevisionName,
+				},
+			})
+
+			if tc.expectCollision {
+				require.Equal(t, ctrl.Result{RequeueAfter: 10 * time.Second}, result)
+				require.NoError(t, err)
+
+				rev := &ocv1.ClusterExtensionRevision{}
+				require.NoError(t, testClient.Get(t.Context(), client.ObjectKey{Name: tc.reconcilingRevisionName}, rev))
+				cond := meta.FindStatusCondition(rev.Status.Conditions, ocv1.TypeProgressing)
+				require.NotNil(t, cond)
+				require.Equal(t, metav1.ConditionTrue, cond.Status)
+				require.Equal(t, ocv1.ClusterExtensionRevisionReasonRetrying, cond.Reason)
+				require.Contains(t, cond.Message, "revision object collisions")
+				require.Contains(t, cond.Message, "Conflicting Owner")
+			} else {
+				require.Equal(t, ctrl.Result{}, result)
+				require.NoError(t, err)
+			}
+		})
+	}
+}
+
 func Test_effectiveCollisionProtection(t *testing.T) {
 	for _, tc := range []struct {
 		name     string