From 6ad05bf0291f769c0a838eec102fed3def11d768 Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Fri, 20 Mar 2026 10:21:05 +0100 Subject: [PATCH] fix(Boxcutter-Collision): Fix collision detection bypass after ClusterExtension 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 --- .../clusterextensionrevision_controller.go | 64 ++++- ...lusterextensionrevision_controller_test.go | 254 +++++++++++++++++- test/e2e/features/update.feature | 52 ++++ test/e2e/steps/steps.go | 13 + 4 files changed, 370 insertions(+), 13 deletions(-) diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller.go b/internal/operator-controller/controllers/clusterextensionrevision_controller.go index 2e9d2fce6..115ecf854 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller.go +++ b/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) { diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go b/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go index 682c10174..05a8a7f7f 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller_test.go +++ b/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.ClusterExtensionRevisionTypeProgressing) + 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 diff --git a/test/e2e/features/update.feature b/test/e2e/features/update.feature index e1b4becca..6d3c9fa28 100644 --- a/test/e2e/features/update.feature +++ b/test/e2e/features/update.feature @@ -243,3 +243,55 @@ Feature: Update ClusterExtension And ClusterExtensionRevision "${NAME}-2" reports Progressing as True with Reason RollingOut And ClusterExtensionRevision "${NAME}-2" reports Available as False with Reason ProbeFailure + @BoxcutterRuntime + Scenario: Detect collision when a second ClusterExtension installs the same package after an upgrade + Given ClusterExtension is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterExtension + metadata: + name: ${NAME} + spec: + namespace: ${TEST_NAMESPACE} + serviceAccount: + name: olm-sa + source: + sourceType: Catalog + catalog: + packageName: test + selector: + matchLabels: + "olm.operatorframework.io/metadata.name": test-catalog + version: 1.0.0 + """ + And ClusterExtension is rolled out + And ClusterExtension is available + When ClusterExtension is updated to version "1.0.1" + Then ClusterExtension is rolled out + And ClusterExtension is available + And bundle "test-operator.1.0.1" is installed in version "1.0.1" + And the current ClusterExtension is tracked for cleanup + When ClusterExtension is applied + """ + apiVersion: olm.operatorframework.io/v1 + kind: ClusterExtension + metadata: + name: ${NAME}-dup + spec: + namespace: ${TEST_NAMESPACE} + serviceAccount: + name: olm-sa + source: + sourceType: Catalog + catalog: + packageName: test + selector: + matchLabels: + "olm.operatorframework.io/metadata.name": test-catalog + version: 1.0.1 + """ + Then ClusterExtension reports Progressing as True with Reason Retrying and Message includes: + """ + revision object collisions + """ + diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index 57cfc864e..81449d567 100644 --- a/test/e2e/steps/steps.go +++ b/test/e2e/steps/steps.go @@ -138,6 +138,8 @@ func RegisterSteps(sc *godog.ScenarioContext) { sc.Step(`^(?i)min value for (ClusterExtension|ClusterExtensionRevision) ((?:\.[a-zA-Z]+)+) is set to (\d+)$`, SetCRDFieldMinValue) + sc.Step(`^(?i)the current ClusterExtension is tracked for cleanup$`, TrackCurrentClusterExtensionForCleanup) + // Upgrade-specific steps sc.Step(`^(?i)the latest stable OLM release is installed$`, LatestStableOLMReleaseIsInstalled) sc.Step(`^(?i)OLM is upgraded$`, OLMIsUpgraded) @@ -253,6 +255,17 @@ func ResourceApplyFails(ctx context.Context, errMsg string, yamlTemplate *godog. return nil } +// TrackCurrentClusterExtensionForCleanup adds the current ClusterExtension name to the cleanup list. +// Use this before applying a second ClusterExtension in the same scenario so that the first one +// is properly deleted during cleanup (since ResourceIsApplied overwrites the tracked name). +func TrackCurrentClusterExtensionForCleanup(ctx context.Context) error { + sc := scenarioCtx(ctx) + if sc.clusterExtensionName != "" { + sc.addedResources = append(sc.addedResources, resource{name: sc.clusterExtensionName, kind: "clusterextension"}) + } + return nil +} + // ClusterExtensionVersionUpdate patches the ClusterExtension's catalog version to the specified value. func ClusterExtensionVersionUpdate(ctx context.Context, version string) error { sc := scenarioCtx(ctx)