Fix infinite loop on cyclic CSV replacement chains

pkg/lib/csv/replace_finder.go

@@ -41,6 +41,11 @@ func (r *replace) IsBeingReplaced(in *v1alpha1.ClusterServiceVersion, csvsInName
 			continue
 		}
 
+		// a CSV cannot replace itself
+		if csv.GetName() == in.GetName() {
+			continue
+		}
+
 		r.logger.Debugf("checking %s", csv.GetName())
 
 		if csv.Spec.Replaces == in.GetName() {
@@ -63,6 +68,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()).Warn("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 +90,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()).Warn("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)
+	}
+}