diff --git a/internal/operator-controller/controllers/clusterextensionrevision_controller.go b/internal/operator-controller/controllers/clusterextensionrevision_controller.go index 2e9d2fce6..8013f9512 100644 --- a/internal/operator-controller/controllers/clusterextensionrevision_controller.go +++ b/internal/operator-controller/controllers/clusterextensionrevision_controller.go @@ -148,6 +148,12 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, cer return ctrl.Result{}, fmt.Errorf("converting to boxcutter revision: %v", err) } + siblings, err := c.siblingRevisionNames(ctx, cer) + if err != nil { + setRetryingConditions(cer, err.Error()) + return ctrl.Result{}, fmt.Errorf("listing sibling revisions: %v", err) + } + revisionEngine, err := c.RevisionEngineFactory.CreateRevisionEngine(ctx, cer) if err != nil { setRetryingConditions(cer, err.Error()) @@ -210,6 +216,11 @@ func (c *ClusterExtensionRevisionReconciler) reconcile(ctx context.Context, cer if ores.Action() == machinery.ActionCollision { collidingObjs = append(collidingObjs, ores.String()) } + if ores.Action() == machinery.ActionProgressed && siblings != nil { + if ref := foreignRevisionController(ores.Object(), siblings); ref != nil { + collidingObjs = append(collidingObjs, ores.String()+fmt.Sprintf("\nConflicting Owner: %s", ref.String())) + } + } } if len(collidingObjs) > 0 { @@ -522,6 +533,42 @@ func EffectiveCollisionProtection(cp ...ocv1.CollisionProtection) ocv1.Collision return ecp } +// siblingRevisionNames returns the names of all ClusterExtensionRevisions that belong to +// the same ClusterExtension as cer. Returns nil when cer has no owner label. +func (c *ClusterExtensionRevisionReconciler) siblingRevisionNames(ctx context.Context, cer *ocv1.ClusterExtensionRevision) (sets.Set[string], error) { + ownerLabel, ok := cer.Labels[labels.OwnerNameKey] + if !ok { + return nil, nil + } + revList := &ocv1.ClusterExtensionRevisionList{} + if err := c.TrackingCache.List(ctx, revList, client.MatchingLabels{ + labels.OwnerNameKey: ownerLabel, + }); err != nil { + return nil, fmt.Errorf("listing sibling revisions: %w", err) + } + names := sets.New[string]() + for i := range revList.Items { + names.Insert(revList.Items[i].Name) + } + return names, nil +} + +// foreignRevisionController returns the controller OwnerReference when obj is owned by a +// ClusterExtensionRevision that is not in siblings (i.e. belongs to a different ClusterExtension). +// Returns nil when the controller is a sibling or is not a ClusterExtensionRevision. +func foreignRevisionController(obj metav1.Object, siblings sets.Set[string]) *metav1.OwnerReference { + refs := obj.GetOwnerReferences() + for i := range refs { + if refs[i].Controller != nil && *refs[i].Controller && + refs[i].Kind == ocv1.ClusterExtensionRevisionKind && + refs[i].APIVersion == ocv1.GroupVersion.String() && + !siblings.Has(refs[i].Name) { + return &refs[i] + } + } + 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..7864256c7 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,241 @@ 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: "progressed object owned by a foreign CER is treated as a 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"}, + }, + }, + } + cerA2 := newTestClusterExtensionRevision(t, "ext-A-2", extA, testScheme) + 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: &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, + }, + }, + }, + }, + }, + probes: machinerytypes.ProbeResultContainer{ + boxcutter.ProgressProbeType: { + Status: machinerytypes.ProbeStatusTrue, + }, + }, + }, + }, + }, + }, + }, + expectCollision: true, + }, + { + name: "progressed object owned by a sibling CER 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"}, + }, + }, + } + cerA1 := newTestClusterExtensionRevision(t, "ext-A-1", extA, testScheme) + 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: &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, + }, + }, + }, + }, + }, + probes: machinerytypes.ProbeResultContainer{ + boxcutter.ProgressProbeType: { + Status: machinerytypes.ProbeStatusTrue, + }, + }, + }, + }, + }, + }, + }, + expectCollision: false, + }, + { + name: "progressed object owned by a non-CER controller 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: &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, + }, + }, + }, + }, + }, + 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 590ecf126..ef637fcbf 100644 --- a/test/e2e/features/update.feature +++ b/test/e2e/features/update.feature @@ -210,6 +210,58 @@ Feature: Update ClusterExtension And ClusterExtension is available And bundle "test-operator.1.0.4" is installed in version "1.0.4" + @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 + """ + @BoxcutterRuntime Scenario: Each update creates a new revision and resources not present in the new revision are removed from the cluster Given ClusterExtension is applied diff --git a/test/e2e/steps/steps.go b/test/e2e/steps/steps.go index 57cfc864e..f00d722ad 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 saves the current ClusterExtension name in the cleanup list +// so it gets deleted at the end of the scenario. Call this before applying a second ClusterExtension +// in the same scenario, because 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)