diff --git a/pkg/cvo/availableupdates.go b/pkg/cvo/availableupdates.go index 7a79c773b..683540c1b 100644 --- a/pkg/cvo/availableupdates.go +++ b/pkg/cvo/availableupdates.go @@ -428,17 +428,31 @@ func (u *availableUpdates) evaluateConditionalUpdates(ctx context.Context) { return vi.GTE(vj) }) for i, conditionalUpdate := range u.ConditionalUpdates { - condition := evaluateConditionalUpdate(ctx, conditionalUpdate.Risks, u.ConditionRegistry) - if condition.Status == metav1.ConditionTrue { - u.addUpdate(conditionalUpdate.Release) - } else { - u.removeUpdate(conditionalUpdate.Release.Image) + conditions := evaluateConditionalUpdate(ctx, conditionalUpdate.Risks, u.ConditionRegistry) + + knownTypes := make(map[string]struct{}, len(conditions)) + for _, condition := range conditions { + knownTypes[condition.Type] = struct{}{} + if condition.Type == ConditionalUpdateConditionTypeRecommended { + if condition.Status == metav1.ConditionTrue { + u.addUpdate(conditionalUpdate.Release) + } else { + u.removeUpdate(conditionalUpdate.Release.Image) + } + } + + meta.SetStatusCondition(&conditionalUpdate.Conditions, condition) } - meta.SetStatusCondition(&conditionalUpdate.Conditions, condition) - u.ConditionalUpdates[i].Conditions = conditionalUpdate.Conditions + for i := len(conditionalUpdate.Conditions) - 1; i >= 0; i-- { + conditionType := conditionalUpdate.Conditions[i].Type + if _, ok := knownTypes[conditionType]; !ok { + meta.RemoveStatusCondition(&conditionalUpdate.Conditions, conditionType) + } + } + u.ConditionalUpdates[i].Conditions = conditionalUpdate.Conditions } } @@ -491,6 +505,7 @@ const ( // 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_])?$`) +var reasonAlwaysSafeCharacters = regexp.MustCompile(`[A-Za-z]`) func newRecommendedReason(now, want string) string { switch { @@ -503,7 +518,7 @@ func newRecommendedReason(now, want string) string { } } -func evaluateConditionalUpdate(ctx context.Context, risks []configv1.ConditionalUpdateRisk, conditionRegistry clusterconditions.ConditionRegistry) metav1.Condition { +func evaluateConditionalUpdate(ctx context.Context, risks []configv1.ConditionalUpdateRisk, conditionRegistry clusterconditions.ConditionRegistry) []metav1.Condition { recommended := metav1.Condition{ Type: ConditionalUpdateConditionTypeRecommended, Status: metav1.ConditionTrue, @@ -511,13 +526,31 @@ func evaluateConditionalUpdate(ctx context.Context, risks []configv1.Conditional Reason: recommendedReasonRisksNotExposed, Message: "The update is recommended, because none of the conditional update risks apply to this cluster.", } + conditions := make([]metav1.Condition, 0, len(risks)+1) var errorMessages []string for _, risk := range risks { + cleanName := risk.Name + if !reasonPattern.MatchString(cleanName) { + safeCharacters := reasonAlwaysSafeCharacters.FindAllString(cleanName, -1) + if len(safeCharacters) > 0 { + cleanName = strings.Join(safeCharacters, "") + } else { + cleanName = "UnsalvageableRiskName" + } + } + conditionType := fmt.Sprintf("%s/%s", strings.ToLower(ConditionalUpdateConditionTypeRecommended), cleanName) if match, err := conditionRegistry.Match(ctx, risk.MatchingRules); err != nil { recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionUnknown) recommended.Reason = newRecommendedReason(recommended.Reason, recommendedReasonEvaluationFailed) - errorMessages = append(errorMessages, unknownExposureMessage(risk, err)) + msg := unknownExposureMessage(risk, err) + errorMessages = append(errorMessages, msg) + conditions = append(conditions, metav1.Condition{ + Type: conditionType, + Status: metav1.ConditionUnknown, + Reason: recommendedReasonEvaluationFailed, + Message: msg, + }) } else if match { recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionFalse) wantReason := recommendedReasonExposed @@ -525,14 +558,27 @@ func evaluateConditionalUpdate(ctx context.Context, risks []configv1.Conditional wantReason = risk.Name } recommended.Reason = newRecommendedReason(recommended.Reason, wantReason) - errorMessages = append(errorMessages, fmt.Sprintf("%s %s", risk.Message, risk.URL)) + msg := fmt.Sprintf("%s %s", risk.Message, risk.URL) + errorMessages = append(errorMessages, msg) + conditions = append(conditions, metav1.Condition{ + Type: conditionType, + Status: metav1.ConditionFalse, + Reason: wantReason, + Message: msg, + }) } } if len(errorMessages) > 0 { recommended.Message = strings.Join(errorMessages, "\n\n") } - return recommended + for foundTypeOverlap := true; foundTypeOverlap; { + foundTypeOverlap = false + // FIXME: add suffixes to any matching types in conditions + } + + conditions = append(conditions, recommended) + return conditions } func injectClusterIdIntoConditionalUpdates(clusterId string, updates []configv1.ConditionalUpdate) []configv1.ConditionalUpdate { diff --git a/pkg/cvo/availableupdates_test.go b/pkg/cvo/availableupdates_test.go index c73ec59ef..05643ba19 100644 --- a/pkg/cvo/availableupdates_test.go +++ b/pkg/cvo/availableupdates_test.go @@ -92,6 +92,13 @@ func osusWithSingleConditionalEdge() (*httptest.Server, clusterconditions.Condit }, }, Conditions: []metav1.Condition{ + { + Type: "recommended/FourFiveSix", + Status: metav1.ConditionFalse, + Reason: "FourFiveSix", + Message: "Four Five Five is just fine https://example.com/" + to, + LastTransitionTime: metav1.Now(), + }, { Type: "Recommended", Status: metav1.ConditionFalse, @@ -233,8 +240,10 @@ func TestSyncAvailableUpdates_ConditionalUpdateRecommendedConditions(t *testing. availableUpdates, optr := newOperator(fakeOsus.URL, version, mockPromql) optr.availableUpdates = availableUpdates optr.availableUpdates.ConditionalUpdates = conditionalUpdates - expectedConditions := []metav1.Condition{{}} - conditionalUpdates[0].Conditions[0].DeepCopyInto(&expectedConditions[0]) + expectedConditions := make([]metav1.Condition, 0, len(conditionalUpdates[0].Conditions)) + for _, condition := range conditionalUpdates[0].Conditions { + expectedConditions = append(expectedConditions, *condition.DeepCopy()) + } cv := cvFixture.DeepCopy() tc.modifyOriginalState(optr) tc.modifyCV(cv, conditionalUpdates[0]) @@ -268,16 +277,16 @@ func TestEvaluateConditionalUpdate(t *testing.T) { name string risks []configv1.ConditionalUpdateRisk mockPromql clusterconditions.Condition - expected metav1.Condition + expected []metav1.Condition }{ { name: "no risks", - expected: metav1.Condition{ + 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", @@ -293,12 +302,12 @@ func TestEvaluateConditionalUpdate(t *testing.T) { ValidQueue: []error{nil}, MatchQueue: []mock.MatchResult{{Match: false, Error: nil}}, }, - expected: metav1.Condition{ + 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", @@ -314,12 +323,17 @@ func TestEvaluateConditionalUpdate(t *testing.T) { ValidQueue: []error{nil}, MatchQueue: []mock.MatchResult{{Match: true, Error: nil}}, }, - expected: metav1.Condition{ + expected: []metav1.Condition{{ + Type: "recommended/RiskThatApplies", + Status: metav1.ConditionFalse, + Reason: "RiskThatApplies", + Message: "This is a risk! https://match.es", + }, { 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", @@ -335,12 +349,17 @@ func TestEvaluateConditionalUpdate(t *testing.T) { ValidQueue: []error{nil}, MatchQueue: []mock.MatchResult{{Match: true, Error: nil}}, }, - expected: metav1.Condition{ + expected: []metav1.Condition{{ + Type: "recommended/RISKTHATAPPLIES", + Status: metav1.ConditionFalse, + Reason: recommendedReasonExposed, + Message: "This is a risk! https://match.es", + }, { Type: "Recommended", Status: metav1.ConditionFalse, Reason: recommendedReasonExposed, Message: "This is a risk! https://match.es", - }, + }}, }, { name: "two risks that match", @@ -362,12 +381,22 @@ func TestEvaluateConditionalUpdate(t *testing.T) { ValidQueue: []error{nil, nil}, MatchQueue: []mock.MatchResult{{Match: true, Error: nil}, {Match: true, Error: nil}}, }, - expected: metav1.Condition{ + expected: []metav1.Condition{{ + Type: "recommended/RiskThatApplies", + Status: metav1.ConditionFalse, + Reason: "RiskThatApplies", + Message: "This is a risk! https://match.es", + }, { + Type: "recommended/RiskThatAppliesToo", + Status: metav1.ConditionFalse, + Reason: "RiskThatAppliesToo", + Message: "This is a risk too! https://match.es/too", + }, { 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", @@ -389,7 +418,19 @@ func TestEvaluateConditionalUpdate(t *testing.T) { ValidQueue: []error{nil, nil}, MatchQueue: []mock.MatchResult{{Match: true, Error: nil}, {Match: false, Error: errors.New("ERROR")}}, }, - expected: metav1.Condition{ + expected: []metav1.Condition{{ + Type: "recommended/RiskThatApplies", + Status: metav1.ConditionFalse, + Reason: "RiskThatApplies", + Message: "This is a risk! https://match.es", + }, { + Type: "recommended/RiskThatFailsToEvaluate", + Status: metav1.ConditionUnknown, + Reason: recommendedReasonEvaluationFailed, + Message: "Could not evaluate exposure to update risk RiskThatFailsToEvaluate (ERROR)\n" + + " RiskThatFailsToEvaluate description: This is a risk too!\n" + + " RiskThatFailsToEvaluate URL: https://whokno.ws", + }, { Type: "Recommended", Status: metav1.ConditionFalse, Reason: recommendedReasonMultiple, @@ -397,7 +438,7 @@ func TestEvaluateConditionalUpdate(t *testing.T) { "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", @@ -413,14 +454,21 @@ func TestEvaluateConditionalUpdate(t *testing.T) { ValidQueue: []error{nil}, MatchQueue: []mock.MatchResult{{Match: false, Error: errors.New("ERROR")}}, }, - expected: metav1.Condition{ + expected: []metav1.Condition{{ + Type: "recommended/RiskThatFailsToEvaluate", + 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", + }, { 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 {