OCPBUGS-18454: Avoid using risk names as condition reasons when invalid

pkg/cvo/availableupdates.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/url"
+	"regexp"
 	"sort"
 	"strings"
 	"time"
@@ -389,19 +390,15 @@ func (u *availableUpdates) evaluateConditionalUpdates(ctx context.Context) {
 		return vi.GTE(vj)
 	})
 	for i, conditionalUpdate := range u.ConditionalUpdates {
-		if errorCondition := evaluateConditionalUpdate(ctx, &conditionalUpdate, u.ConditionRegistry); errorCondition != nil {
-			meta.SetStatusCondition(&conditionalUpdate.Conditions, *errorCondition)
-			u.removeUpdate(conditionalUpdate.Release.Image)
-		} else {
-			meta.SetStatusCondition(&conditionalUpdate.Conditions, metav1.Condition{
-				Type:   ConditionalUpdateConditionTypeRecommended,
-				Status: metav1.ConditionTrue,
-				// FIXME: ObservedGeneration?  That would capture upstream/channel, but not necessarily the currently-reconciling version.
-				Reason:  "AsExpected",
-				Message: "The update is recommended, because none of the conditional update risks apply to this cluster.",
-			})
+		condition := evaluateConditionalUpdate(ctx, conditionalUpdate.Risks, u.ConditionRegistry)
+
+		if condition.Status == metav1.ConditionTrue {
 			u.addUpdate(conditionalUpdate.Release)
+		} else {
+			u.removeUpdate(conditionalUpdate.Release.Image)
 		}
+
+		meta.SetStatusCondition(&conditionalUpdate.Conditions, condition)
 		u.ConditionalUpdates[i].Conditions = conditionalUpdate.Conditions
 
 	}
@@ -432,36 +429,71 @@ func unknownExposureMessage(risk configv1.ConditionalUpdateRisk, err error) stri
 	return fmt.Sprintf(template, risk.Name, err, risk.Name, risk.Message, risk.Name, risk.URL)
 }
 
-func evaluateConditionalUpdate(ctx context.Context, conditionalUpdate *configv1.ConditionalUpdate, conditionRegistry clusterconditions.ConditionRegistry) *metav1.Condition {
-	recommended := &metav1.Condition{
-		Type: ConditionalUpdateConditionTypeRecommended,
+func newRecommendedStatus(now, want metav1.ConditionStatus) metav1.ConditionStatus {
+	switch {
+	case now == metav1.ConditionFalse || want == metav1.ConditionFalse:
+		return metav1.ConditionFalse
+	case now == metav1.ConditionUnknown || want == metav1.ConditionUnknown:
+		return metav1.ConditionUnknown
+	default:
+		return want
 	}
-	messages := []string{}
-	for _, risk := range conditionalUpdate.Risks {
+}
+
+const (
+	recommendedReasonRisksNotExposed  = "NotExposedToRisks"
+	recommendedReasonEvaluationFailed = "EvaluationFailed"
+	recommendedReasonMultiple         = "MultipleReasons"
+
+	// recommendedReasonExposed is used instead of the original name if it does
+	// not match the pattern for a valid k8s condition reason.
+	recommendedReasonExposed = "ExposedToRisks"
+)
+
+// Reasons follow same pattern as k8s Condition Reasons
+// https://github.com/openshift/api/blob/59fa376de7cb668ddb95a7ee4e9879d7f6ca2767/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L1535-L1536
+var reasonPattern = regexp.MustCompile(`^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$`)
+
+func newRecommendedReason(now, want string) string {
+	switch {
+	case now == recommendedReasonRisksNotExposed:
+		return want
+	case now == want:
+		return now
+	default:
+		return recommendedReasonMultiple
+	}
+}
+
+func evaluateConditionalUpdate(ctx context.Context, risks []configv1.ConditionalUpdateRisk, conditionRegistry clusterconditions.ConditionRegistry) metav1.Condition {
+	recommended := metav1.Condition{
+		Type:   ConditionalUpdateConditionTypeRecommended,
+		Status: metav1.ConditionTrue,
+		// FIXME: ObservedGeneration?  That would capture upstream/channel, but not necessarily the currently-reconciling version.
+		Reason:  recommendedReasonRisksNotExposed,
+		Message: "The update is recommended, because none of the conditional update risks apply to this cluster.",
+	}
+
+	var errorMessages []string
+	for _, risk := range risks {
 		if match, err := conditionRegistry.Match(ctx, risk.MatchingRules); err != nil {
-			if recommended.Status != metav1.ConditionFalse {
-				recommended.Status = metav1.ConditionUnknown
-			}
-			if recommended.Reason == "" || recommended.Reason == "EvaluationFailed" {
-				recommended.Reason = "EvaluationFailed"
-			} else {
-				recommended.Reason = "MultipleReasons"
-			}
-			messages = append(messages, unknownExposureMessage(risk, err))
+			recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionUnknown)
+			recommended.Reason = newRecommendedReason(recommended.Reason, recommendedReasonEvaluationFailed)
+			errorMessages = append(errorMessages, unknownExposureMessage(risk, err))
 		} else if match {
-			recommended.Status = metav1.ConditionFalse
-			if recommended.Reason == "" {
-				recommended.Reason = risk.Name
-			} else {
-				recommended.Reason = "MultipleReasons"
+			recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionFalse)
+			wantReason := recommendedReasonExposed
+			if reasonPattern.MatchString(risk.Name) {
+				wantReason = risk.Name
 			}
-			messages = append(messages, fmt.Sprintf("%s %s", risk.Message, risk.URL))
+			recommended.Reason = newRecommendedReason(recommended.Reason, wantReason)
+			errorMessages = append(errorMessages, fmt.Sprintf("%s %s", risk.Message, risk.URL))
 		}
 	}
-	if recommended.Status == "" {
-		return nil
+	if len(errorMessages) > 0 {
+		recommended.Message = strings.Join(errorMessages, "\n\n")
 	}
-	recommended.Message = strings.Join(messages, "\n\n")
+
 	return recommended
 }
 

pkg/cvo/availableupdates_test.go

@@ -2,6 +2,7 @@ package cvo
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"net/http"
 	"net/http/httptest"
@@ -12,7 +13,7 @@ import (
 	"github.com/google/go-cmp/cmp/cmpopts"
 
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -32,7 +33,7 @@ func (n notFoundProxyLister) List(labels.Selector) ([]*configv1.Proxy, error) {
 }
 
 func (n notFoundProxyLister) Get(name string) (*configv1.Proxy, error) {
-	return nil, errors.NewNotFound(schema.GroupResource{Group: configv1.GroupName, Resource: "proxy"}, name)
+	return nil, k8serrors.NewNotFound(schema.GroupResource{Group: configv1.GroupName, Resource: "proxy"}, name)
 }
 
 type notFoundConfigMapLister struct{}
@@ -42,7 +43,7 @@ func (n notFoundConfigMapLister) List(labels.Selector) ([]*corev1.ConfigMap, err
 }
 
 func (n notFoundConfigMapLister) Get(name string) (*corev1.ConfigMap, error) {
-	return nil, errors.NewNotFound(schema.GroupResource{Group: "", Resource: "configmap"}, name)
+	return nil, k8serrors.NewNotFound(schema.GroupResource{Group: "", Resource: "configmap"}, name)
 }
 
 // osusWithSingleConditionalEdge helper returns:
@@ -260,3 +261,175 @@ func TestSyncAvailableUpdates_ConditionalUpdateRecommendedConditions(t *testing.
 		})
 	}
 }
+
+func TestEvaluateConditionalUpdate(t *testing.T) {
+	testcases := []struct {
+		name       string
+		risks      []configv1.ConditionalUpdateRisk
+		mockPromql clusterconditions.Condition
+		expected   metav1.Condition
+	}{
+		{
+			name: "no risks",
+			expected: metav1.Condition{
+				Type:    "Recommended",
+				Status:  metav1.ConditionTrue,
+				Reason:  recommendedReasonRisksNotExposed,
+				Message: "The update is recommended, because none of the conditional update risks apply to this cluster.",
+			},
+		},
+		{
+			name: "one risk that does not match",
+			risks: []configv1.ConditionalUpdateRisk{
+				{
+					URL:           "https://doesnotmat.ch",
+					Name:          "ShouldNotApply",
+					Message:       "ShouldNotApply",
+					MatchingRules: []configv1.ClusterCondition{{Type: "PromQL"}},
+				},
+			},
+			mockPromql: &mock.Mock{
+				ValidQueue: []error{nil},
+				MatchQueue: []mock.MatchResult{{Match: false, Error: nil}},
+			},
+			expected: metav1.Condition{
+				Type:    "Recommended",
+				Status:  metav1.ConditionTrue,
+				Reason:  recommendedReasonRisksNotExposed,
+				Message: "The update is recommended, because none of the conditional update risks apply to this cluster.",
+			},
+		},
+		{
+			name: "one risk that matches",
+			risks: []configv1.ConditionalUpdateRisk{
+				{
+					URL:           "https://match.es",
+					Name:          "RiskThatApplies",
+					Message:       "This is a risk!",
+					MatchingRules: []configv1.ClusterCondition{{Type: "PromQL"}},
+				},
+			},
+			mockPromql: &mock.Mock{
+				ValidQueue: []error{nil},
+				MatchQueue: []mock.MatchResult{{Match: true, Error: nil}},
+			},
+			expected: metav1.Condition{
+				Type:    "Recommended",
+				Status:  metav1.ConditionFalse,
+				Reason:  "RiskThatApplies",
+				Message: "This is a risk! https://match.es",
+			},
+		},
+		{
+			name: "matching risk with name that cannot be used as a condition reason",
+			risks: []configv1.ConditionalUpdateRisk{
+				{
+					URL:           "https://match.es",
+					Name:          "RISK-THAT-APPLIES", // Condition reasons are CamelCase names, must not contain dashes
+					Message:       "This is a risk!",
+					MatchingRules: []configv1.ClusterCondition{{Type: "PromQL"}},
+				},
+			},
+			mockPromql: &mock.Mock{
+				ValidQueue: []error{nil},
+				MatchQueue: []mock.MatchResult{{Match: true, Error: nil}},
+			},
+			expected: metav1.Condition{
+				Type:    "Recommended",
+				Status:  metav1.ConditionFalse,
+				Reason:  recommendedReasonExposed,
+				Message: "This is a risk! https://match.es",
+			},
+		},
+		{
+			name: "two risks that match",
+			risks: []configv1.ConditionalUpdateRisk{
+				{
+					URL:           "https://match.es",
+					Name:          "RiskThatApplies",
+					Message:       "This is a risk!",
+					MatchingRules: []configv1.ClusterCondition{{Type: "PromQL"}},
+				},
+				{
+					URL:           "https://match.es/too",
+					Name:          "RiskThatAppliesToo",
+					Message:       "This is a risk too!",
+					MatchingRules: []configv1.ClusterCondition{{Type: "PromQL"}},
+				},
+			},
+			mockPromql: &mock.Mock{
+				ValidQueue: []error{nil, nil},
+				MatchQueue: []mock.MatchResult{{Match: true, Error: nil}, {Match: true, Error: nil}},
+			},
+			expected: metav1.Condition{
+				Type:    "Recommended",
+				Status:  metav1.ConditionFalse,
+				Reason:  recommendedReasonMultiple,
+				Message: "This is a risk! https://match.es\n\nThis is a risk too! https://match.es/too",
+			},
+		},
+		{
+			name: "first risk matches, second fails to evaluate",
+			risks: []configv1.ConditionalUpdateRisk{
+				{
+					URL:           "https://match.es",
+					Name:          "RiskThatApplies",
+					Message:       "This is a risk!",
+					MatchingRules: []configv1.ClusterCondition{{Type: "PromQL"}},
+				},
+				{
+					URL:           "https://whokno.ws",
+					Name:          "RiskThatFailsToEvaluate",
+					Message:       "This is a risk too!",
+					MatchingRules: []configv1.ClusterCondition{{Type: "PromQL"}},
+				},
+			},
+			mockPromql: &mock.Mock{
+				ValidQueue: []error{nil, nil},
+				MatchQueue: []mock.MatchResult{{Match: true, Error: nil}, {Match: false, Error: errors.New("ERROR")}},
+			},
+			expected: metav1.Condition{
+				Type:   "Recommended",
+				Status: metav1.ConditionFalse,
+				Reason: recommendedReasonMultiple,
+				Message: "This is a risk! https://match.es\n\n" +
+					"Could not evaluate exposure to update risk RiskThatFailsToEvaluate (ERROR)\n" +
+					"  RiskThatFailsToEvaluate description: This is a risk too!\n" +
+					"  RiskThatFailsToEvaluate URL: https://whokno.ws",
+			},
+		},
+		{
+			name: "one risk that fails to evaluate",
+			risks: []configv1.ConditionalUpdateRisk{
+				{
+					URL:           "https://whokno.ws",
+					Name:          "RiskThatFailsToEvaluate",
+					Message:       "This is a risk!",
+					MatchingRules: []configv1.ClusterCondition{{Type: "PromQL"}},
+				},
+			},
+			mockPromql: &mock.Mock{
+				ValidQueue: []error{nil},
+				MatchQueue: []mock.MatchResult{{Match: false, Error: errors.New("ERROR")}},
+			},
+			expected: metav1.Condition{
+				Type:   "Recommended",
+				Status: metav1.ConditionUnknown,
+				Reason: recommendedReasonEvaluationFailed,
+				Message: "Could not evaluate exposure to update risk RiskThatFailsToEvaluate (ERROR)\n" +
+					"  RiskThatFailsToEvaluate description: This is a risk!\n" +
+					"  RiskThatFailsToEvaluate URL: https://whokno.ws",
+			},
+		},
+	}
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			registry := clusterconditions.NewConditionRegistry()
+			registry.Register("PromQL", tc.mockPromql)
+			actual := evaluateConditionalUpdate(context.Background(), tc.risks, registry)
+			if diff := cmp.Diff(tc.expected, actual); diff != "" {
+				t.Errorf("actual condition differs from expected:\n%s", diff)
+			}
+		})
+	}
+}