Fix infinite loop on cyclic CSV replacement chains

pkg/controller/operators/olm/operator.go

@@ -2539,7 +2539,10 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 		// If there is a succeeded replacement, mark this for deletion
 		next := a.isBeingReplaced(out, a.csvSet(out.GetNamespace(), v1alpha1.CSVPhaseAny))
 		// Get the newest CSV in the replacement chain if fail forward upgrades are enabled.
-		if operatorGroup.UpgradeStrategy() == operatorsv1.UpgradeStrategyUnsafeFailForward {
+		// next can be nil (e.g. the replacement was deleted, or a self-referencing
+		// spec.replaces is ignored by the finder); fall through to the
+		// "no replacement CSV found" error below instead of dereferencing it.
+		if next != nil && operatorGroup.UpgradeStrategy() == operatorsv1.UpgradeStrategyUnsafeFailForward {
 			csvs, err := a.lister.OperatorsV1alpha1().ClusterServiceVersionLister().ClusterServiceVersions(next.GetNamespace()).List(labels.Everything())
 			if err != nil {
 				syncError = err

pkg/controller/operators/olm/operator_test.go

@@ -3464,6 +3464,71 @@ func TestTransitionCSVFailForward(t *testing.T) {
 				},
 			},
 		},
+		{
+			// A CSV left in Replacing by a pre-cycle-guard OLM whose spec.replaces
+			// names itself: with the self-reference guards, isBeingReplaced returns
+			// nil, and the fail-forward chain walk must not dereference it.
+			name: "FailForwardEnabled/SelfReferencingCSV/NoPanicReplacementNotFound",
+			initial: initial{
+				csvs: []*v1alpha1.ClusterServiceVersion{
+					csvWithAnnotations(csv("csv1",
+						namespace,
+						"1.0.0",
+						"csv1",
+						installStrategy("csv1-dep1", nil, nil),
+						[]*apiextensionsv1.CustomResourceDefinition{crd("c1", "v1", "g1")},
+						[]*apiextensionsv1.CustomResourceDefinition{},
+						v1alpha1.CSVPhaseReplacing,
+					), addAnnotations(defaultTemplateAnnotations, map[string]string{})),
+				},
+				clientObjs: []runtime.Object{
+					func() *operatorsv1.OperatorGroup {
+						og := defaultOperatorGroup.DeepCopy()
+						og.Spec.UpgradeStrategy = operatorsv1.UpgradeStrategyUnsafeFailForward
+						return og
+					}(),
+				},
+			},
+			expected: expected{
+				csvStates: map[string]csvState{
+					"csv1": {exists: true, phase: v1alpha1.CSVPhaseReplacing},
+				},
+				err: map[string]error{
+					"csv1": fmt.Errorf("marked as replacement, but no replacement CSV found in cluster"),
+				},
+			},
+		},
+		{
+			name: "FailForwardDisabled/SelfReferencingCSV/ReplacementNotFound",
+			initial: initial{
+				csvs: []*v1alpha1.ClusterServiceVersion{
+					csvWithAnnotations(csv("csv1",
+						namespace,
+						"1.0.0",
+						"csv1",
+						installStrategy("csv1-dep1", nil, nil),
+						[]*apiextensionsv1.CustomResourceDefinition{crd("c1", "v1", "g1")},
+						[]*apiextensionsv1.CustomResourceDefinition{},
+						v1alpha1.CSVPhaseReplacing,
+					), addAnnotations(defaultTemplateAnnotations, map[string]string{})),
+				},
+				clientObjs: []runtime.Object{
+					func() *operatorsv1.OperatorGroup {
+						og := defaultOperatorGroup.DeepCopy()
+						og.Spec.UpgradeStrategy = operatorsv1.UpgradeStrategyDefault
+						return og
+					}(),
+				},
+			},
+			expected: expected{
+				csvStates: map[string]csvState{
+					"csv1": {exists: true, phase: v1alpha1.CSVPhaseReplacing},
+				},
+				err: map[string]error{
+					"csv1": fmt.Errorf("marked as replacement, but no replacement CSV found in cluster"),
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

pkg/lib/csv/replace_finder.go

@@ -41,6 +41,12 @@ func (r *replace) IsBeingReplaced(in *v1alpha1.ClusterServiceVersion, csvsInName
 			continue
 		}
 
+		// a CSV cannot replace itself
+		if csv.GetName() == in.GetName() {
+			r.logger.WithField("csv", in.GetName()).Debug("ignoring self-referencing spec.replaces")
+			continue
+		}
+
 		r.logger.Debugf("checking %s", csv.GetName())
 
 		if csv.Spec.Replaces == in.GetName() {
@@ -63,6 +69,12 @@ func (r *replace) IsReplacing(in *v1alpha1.ClusterServiceVersion) *v1alpha1.Clus
 		return nil
 	}
 
+	// a CSV cannot replace itself
+	if in.Spec.Replaces == in.GetName() {
+		r.logger.WithField("csv", in.GetName()).Debug("ignoring self-referencing spec.replaces")
+		return nil
+	}
+
 	// using the client instead of a lister; missing an object because of a cache sync can cause upgrades to fail
 	previous, err := r.client.OperatorsV1alpha1().ClusterServiceVersions(in.GetNamespace()).Get(context.TODO(), in.Spec.Replaces, metav1.GetOptions{})
 	if err != nil {
@@ -79,12 +91,20 @@ func (r *replace) IsReplacing(in *v1alpha1.ClusterServiceVersion) *v1alpha1.Clus
 // If the corresponding ClusterServiceVersion is not found nil is returned.
 func (r *replace) GetFinalCSVInReplacing(in *v1alpha1.ClusterServiceVersion, csvsInNamespace map[string]*v1alpha1.ClusterServiceVersion) (replacedBy *v1alpha1.ClusterServiceVersion) {
 	current := in
+	visited := map[string]struct{}{in.GetName(): {}}
 	for {
 		next := r.IsBeingReplaced(current, csvsInNamespace)
 		if next == nil {
 			break
 		}
 
+		// a cycle in the replacement chain would loop forever
+		if _, ok := visited[next.GetName()]; ok {
+			r.logger.WithField("csv", next.GetName()).Debug("cycle detected in replacement chain")
+			break
+		}
+		visited[next.GetName()] = struct{}{}
+
 		replacedBy = next
 		current = next
 	}

pkg/lib/csv/replace_finder_test.go

@@ -0,0 +1,71 @@
+package csv
+
+import (
+	"testing"
+
+	"github.com/sirupsen/logrus"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	"github.com/operator-framework/api/pkg/operators/v1alpha1"
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/client/clientset/versioned/fake"
+)
+
+func newCSV(name, replaces string) *v1alpha1.ClusterServiceVersion {
+	return &v1alpha1.ClusterServiceVersion{
+		ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "ns"},
+		Spec:       v1alpha1.ClusterServiceVersionSpec{Replaces: replaces},
+	}
+}
+
+func setOf(csvs ...*v1alpha1.ClusterServiceVersion) map[string]*v1alpha1.ClusterServiceVersion {
+	set := map[string]*v1alpha1.ClusterServiceVersion{}
+	for _, csv := range csvs {
+		set[csv.GetName()] = csv
+	}
+	return set
+}
+
+func TestIsBeingReplacedIgnoresSelf(t *testing.T) {
+	finder := NewReplaceFinder(logrus.New(), fake.NewSimpleClientset())
+	self := newCSV("a", "a")
+	if got := finder.IsBeingReplaced(self, setOf(self)); got != nil {
+		t.Fatalf("self-replacing CSV reported as being replaced by %q", got.GetName())
+	}
+}
+
+func TestIsReplacingIgnoresSelf(t *testing.T) {
+	self := newCSV("a", "a")
+	finder := NewReplaceFinder(logrus.New(), fake.NewSimpleClientset(self))
+	if got := finder.IsReplacing(self); got != nil {
+		t.Fatalf("self-replacing CSV reported as replacing %q", got.GetName())
+	}
+}
+
+// Regression test for OCPBUGS-23954: these calls looped forever before the
+// cycle guard.
+func TestGetFinalCSVInReplacingTerminates(t *testing.T) {
+	finder := NewReplaceFinder(logrus.New(), fake.NewSimpleClientset())
+
+	// self-loop: a replaces a
+	self := newCSV("a", "a")
+	if got := finder.GetFinalCSVInReplacing(self, setOf(self)); got != nil {
+		t.Fatalf("self-loop: expected nil, got %q", got.GetName())
+	}
+
+	// two-CSV cycle: a replaces b, b replaces a
+	a := newCSV("a", "b")
+	b := newCSV("b", "a")
+	got := finder.GetFinalCSVInReplacing(a, setOf(a, b))
+	if got == nil || got.GetName() != "b" {
+		t.Fatalf("two-CSV cycle: expected b, got %v", got)
+	}
+
+	// linear chain: c replaces b replaces a, walk from a ends at c
+	a = newCSV("a", "")
+	b = newCSV("b", "a")
+	c := newCSV("c", "b")
+	got = finder.GetFinalCSVInReplacing(a, setOf(a, b, c))
+	if got == nil || got.GetName() != "c" {
+		t.Fatalf("linear chain: expected c, got %v", got)
+	}
+}