fix(openshift): ignore invalid max properties

Fail open when olm.maxOpenShiftVersion properties have invalid
values. This means that all invalid values are logged but are ultimately
ignored when determining cluster upgradeability.

See https://bugzilla.redhat.com/show_bug.cgi?id=1986753 for motivation.

Signed-off-by: Nick Hale <[email protected]>

Author: njhale

pkg/controller/operators/openshift/clusteroperator_controller.go

@@ -229,7 +229,13 @@ func (r *ClusterOperatorReconciler) setUpgradeable(ctx context.Context, co *Clus
 	} else {
 		incompatible, err := incompatibleOperators(ctx, r.Client)
 		if err != nil {
-			return err
+			if trans := transientErrors(err); trans != nil {
+				// Only return the error if it's transient and can be resolved by retrying
+				return trans
+			}
+
+			// TODO(njhale): less intense logging?
+			r.Log.Error(err, "errors encountered while finding incompatible operators")
 		}
 
 		if len(incompatible) > 0 {

pkg/controller/operators/openshift/clusteroperator_controller_test.go

@@ -160,12 +160,15 @@ var _ = Describe("ClusterOperator controller", func() {
 		incompatible.SetName("xenomorph")
 		incompatible.SetNamespace(ns.GetName())
 
-		withMax := func(version string) map[string]string {
-			maxProperty := &api.Property{
-				Type:  MaxOpenShiftVersionProperty,
-				Value: version,
+		withMax := func(versions ...string) map[string]string {
+			var properties []*api.Property
+			for _, v := range versions {
+				properties = append(properties, &api.Property{
+					Type:  MaxOpenShiftVersionProperty,
+					Value: v,
+				})
 			}
-			value, err := projection.PropertiesAnnotationFromPropertyList([]*api.Property{maxProperty})
+			value, err := projection.PropertiesAnnotationFromPropertyList(properties)
 			Expect(err).ToNot(HaveOccurred())
 
 			return map[string]string{
@@ -245,5 +248,29 @@ var _ = Describe("ClusterOperator controller", func() {
 			}.String(),
 			LastTransitionTime: fixedNow(),
 		}))
+
+		By("ignoring invalid versions")
+		incompatible.SetAnnotations(withMax(fmt.Sprintf(`"%s"`, clusterVersion), `" "`, `"garbage"`))
+
+		Eventually(func() error {
+			return k8sClient.Update(ctx, incompatible)
+		}).Should(Succeed())
+
+		Eventually(func() ([]configv1.ClusterOperatorStatusCondition, error) {
+			err := k8sClient.Get(ctx, clusterOperatorName, co)
+			return co.Status.Conditions, err
+		}, timeout).Should(ContainElement(configv1.ClusterOperatorStatusCondition{
+			Type:   configv1.OperatorUpgradeable,
+			Status: configv1.ConditionFalse,
+			Reason: IncompatibleOperatorsInstalled,
+			Message: skews{
+				{
+					namespace:           ns.GetName(),
+					name:                incompatible.GetName(),
+					maxOpenShiftVersion: clusterVersion,
+				},
+			}.String(),
+			LastTransitionTime: fixedNow(),
+		}))
 	})
 })

pkg/controller/operators/openshift/helpers.go

@@ -2,6 +2,7 @@ package openshift
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -98,6 +99,16 @@ func (s skew) String() string {
 	return fmt.Sprintf("Operator %s in namespace %s is not compatible with OpenShift versions greater than %s", s.name, s.namespace, s.maxOpenShiftVersion)
 }
 
+type transientError struct {
+	error
+}
+
+func transientErrors(err error) error {
+	return utilerrors.FilterOut(err, func(e error) bool {
+		return !errors.As(e, new(transientError))
+	})
+}
+
 func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error) {
 	next, err := desiredRelease(ctx, cli)
 	if err != nil {
@@ -111,7 +122,7 @@ func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error
 
 	csvList := &operatorsv1alpha1.ClusterServiceVersionList{}
 	if err := cli.List(ctx, csvList); err != nil {
-		return nil, err
+		return nil, &transientError{err}
 	}
 
 	var (
@@ -126,7 +137,6 @@ func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error
 		max, err := maxOpenShiftVersion(&csv)
 		if err != nil {
 			errs = append(errs, err)
-			continue
 		}
 		if max == nil || max.GTE(*next) {
 			continue
@@ -139,13 +149,13 @@ func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error
 		})
 	}
 
-	return s, utilerrors.NewAggregate(errs)
+	return s, utilerrors.Flatten(utilerrors.NewAggregate(errs)) // Flatten to allow for the use of a .As in a utilerrors.Matcher function
 }
 
 func desiredRelease(ctx context.Context, cli client.Client) (*semver.Version, error) {
 	cv := configv1.ClusterVersion{}
 	if err := cli.Get(ctx, client.ObjectKey{Name: "version"}, &cv); err != nil { // "version" is the name of OpenShift's ClusterVersion singleton
-		return nil, err
+		return nil, &transientError{err}
 	}
 
 	v := cv.Status.Desired.Version
@@ -224,11 +234,7 @@ func maxOpenShiftVersion(csv *operatorsv1alpha1.ClusterServiceVersion) (*semver.
 		}
 	}
 
-	if len(errs) > 0 {
-		return nil, utilerrors.NewAggregate(errs)
-	}
-
-	return max, nil
+	return max, utilerrors.NewAggregate(errs)
 }
 
 func notCopiedSelector() (labels.Selector, error) {

pkg/controller/operators/openshift/helpers_test.go

@@ -532,7 +532,7 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 			},
 			expect: expect{
 				err: true,
-				max: nil,
+				max: mustParse("1.0.0"),
 			},
 		},
 		{
@@ -585,7 +585,19 @@ func TestMaxOpenShiftVersion(t *testing.T) {
 			},
 			expect: expect{
 				err: true,
-				max: nil,
+				max: mustParse("1.0.0+1"), // test assumes the order is stable
+			},
+		},
+		{
+			description: "Ambiguous/NonMax",
+			in: []string{
+				`"1.0.0"`,
+				`"1.0.0+1"`,
+				`"2.0.0"`,
+			},
+			expect: expect{
+				err: false,
+				max: mustParse("2.0.0"),
 			},
 		},
 		{