diff --git a/pkg/lib/csv/replace_finder.go b/pkg/lib/csv/replace_finder.go index 0fc4610ce0..157914285b 100644 --- a/pkg/lib/csv/replace_finder.go +++ b/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 } diff --git a/pkg/lib/csv/replace_finder_test.go b/pkg/lib/csv/replace_finder_test.go new file mode 100644 index 0000000000..5ce7692969 --- /dev/null +++ b/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) + } +}