Skip to content

Commit c5053ba

Browse files
wkingwking
committed
pkg/cvo/availableupdates: Add recommended/... conditions for each matching or eval-failed risk
To make it easier for clients to explain individual risks, instead of only having access to a generic MultipleReasons 'reason' and a long 'message' covering all the risks. The "recommended/" prefix conforms to [1]: +kubebuilder:validation:Pattern=`^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$` [1]: https://github.com/kubernetes/apimachinery/blob/3e8e52d6a1259ada73f63c1c7d1fad39d4ba9fb4/pkg/apis/meta/v1/types.go#L1609-L1613
1 parent 6e02b08 commit c5053ba

File tree

2 files changed

+122
-28
lines changed

2 files changed

+122
-28
lines changed

‎pkg/cvo/availableupdates.go

+57-11
Original file line numberDiff line numberDiff line change
@@ -428,17 +428,31 @@ func (u *availableUpdates) evaluateConditionalUpdates(ctx context.Context) {
428428
return vi.GTE(vj)
429429
})
430430
for i, conditionalUpdate := range u.ConditionalUpdates {
431-
condition := evaluateConditionalUpdate(ctx, conditionalUpdate.Risks, u.ConditionRegistry)
432431

433-
if condition.Status == metav1.ConditionTrue {
434-
u.addUpdate(conditionalUpdate.Release)
435-
} else {
436-
u.removeUpdate(conditionalUpdate.Release.Image)
432+
conditions := evaluateConditionalUpdate(ctx, conditionalUpdate.Risks, u.ConditionRegistry)
433+
434+
knownTypes := make(map[string]struct{}, len(conditions))
435+
for _, condition := range conditions {
436+
knownTypes[condition.Type] = struct{}{}
437+
if condition.Type == ConditionalUpdateConditionTypeRecommended {
438+
if condition.Status == metav1.ConditionTrue {
439+
u.addUpdate(conditionalUpdate.Release)
440+
} else {
441+
u.removeUpdate(conditionalUpdate.Release.Image)
442+
}
443+
}
444+
445+
meta.SetStatusCondition(&conditionalUpdate.Conditions, condition)
437446
}
438447

439-
meta.SetStatusCondition(&conditionalUpdate.Conditions, condition)
440-
u.ConditionalUpdates[i].Conditions = conditionalUpdate.Conditions
448+
for i := len(conditionalUpdate.Conditions) - 1; i >= 0; i-- {
449+
conditionType := conditionalUpdate.Conditions[i].Type
450+
if _, ok := knownTypes[conditionType]; !ok {
451+
meta.RemoveStatusCondition(&conditionalUpdate.Conditions, conditionType)
452+
}
453+
}
441454

455+
u.ConditionalUpdates[i].Conditions = conditionalUpdate.Conditions
442456
}
443457
}
444458

@@ -491,6 +505,7 @@ const (
491505
// Reasons follow same pattern as k8s Condition Reasons
492506
// https://github.com/openshift/api/blob/59fa376de7cb668ddb95a7ee4e9879d7f6ca2767/vendor/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L1535-L1536
493507
var reasonPattern = regexp.MustCompile(`^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$`)
508+
var reasonAlwaysSafeCharacters = regexp.MustCompile(`[A-Za-z]`)
494509

495510
func newRecommendedReason(now, want string) string {
496511
switch {
@@ -503,36 +518,67 @@ func newRecommendedReason(now, want string) string {
503518
}
504519
}
505520

506-
func evaluateConditionalUpdate(ctx context.Context, risks []configv1.ConditionalUpdateRisk, conditionRegistry clusterconditions.ConditionRegistry) metav1.Condition {
521+
func evaluateConditionalUpdate(ctx context.Context, risks []configv1.ConditionalUpdateRisk, conditionRegistry clusterconditions.ConditionRegistry) []metav1.Condition {
507522
recommended := metav1.Condition{
508523
Type: ConditionalUpdateConditionTypeRecommended,
509524
Status: metav1.ConditionTrue,
510525
// FIXME: ObservedGeneration? That would capture upstream/channel, but not necessarily the currently-reconciling version.
511526
Reason: recommendedReasonRisksNotExposed,
512527
Message: "The update is recommended, because none of the conditional update risks apply to this cluster.",
513528
}
529+
conditions := make([]metav1.Condition, 0, len(risks)+1)
514530

515531
var errorMessages []string
516532
for _, risk := range risks {
533+
cleanName := risk.Name
534+
if !reasonPattern.MatchString(cleanName) {
535+
safeCharacters := reasonAlwaysSafeCharacters.FindAllString(cleanName, -1)
536+
if len(safeCharacters) > 0 {
537+
cleanName = strings.Join(safeCharacters, "")
538+
} else {
539+
cleanName = "UnsalvageableRiskName"
540+
}
541+
}
542+
conditionType := fmt.Sprintf("%s/%s", strings.ToLower(ConditionalUpdateConditionTypeRecommended), cleanName)
517543
if match, err := conditionRegistry.Match(ctx, risk.MatchingRules); err != nil {
518544
recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionUnknown)
519545
recommended.Reason = newRecommendedReason(recommended.Reason, recommendedReasonEvaluationFailed)
520-
errorMessages = append(errorMessages, unknownExposureMessage(risk, err))
546+
msg := unknownExposureMessage(risk, err)
547+
errorMessages = append(errorMessages, msg)
548+
conditions = append(conditions, metav1.Condition{
549+
Type: conditionType,
550+
Status: metav1.ConditionUnknown,
551+
Reason: recommendedReasonEvaluationFailed,
552+
Message: msg,
553+
})
521554
} else if match {
522555
recommended.Status = newRecommendedStatus(recommended.Status, metav1.ConditionFalse)
523556
wantReason := recommendedReasonExposed
524557
if reasonPattern.MatchString(risk.Name) {
525558
wantReason = risk.Name
526559
}
527560
recommended.Reason = newRecommendedReason(recommended.Reason, wantReason)
528-
errorMessages = append(errorMessages, fmt.Sprintf("%s %s", risk.Message, risk.URL))
561+
msg := fmt.Sprintf("%s %s", risk.Message, risk.URL)
562+
errorMessages = append(errorMessages, msg)
563+
conditions = append(conditions, metav1.Condition{
564+
Type: conditionType,
565+
Status: metav1.ConditionFalse,
566+
Reason: wantReason,
567+
Message: msg,
568+
})
529569
}
530570
}
531571
if len(errorMessages) > 0 {
532572
recommended.Message = strings.Join(errorMessages, "\n\n")
533573
}
534574

535-
return recommended
575+
for foundTypeOverlap := true; foundTypeOverlap; {
576+
foundTypeOverlap = false
577+
// FIXME: add suffixes to any matching types in conditions
578+
}
579+
580+
conditions = append(conditions, recommended)
581+
return conditions
536582
}
537583

538584
func injectClusterIdIntoConditionalUpdates(clusterId string, updates []configv1.ConditionalUpdate) []configv1.ConditionalUpdate {

‎pkg/cvo/availableupdates_test.go

+65-17
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,13 @@ func osusWithSingleConditionalEdge() (*httptest.Server, clusterconditions.Condit
9292
},
9393
},
9494
Conditions: []metav1.Condition{
95+
{
96+
Type: "recommended/FourFiveSix",
97+
Status: metav1.ConditionFalse,
98+
Reason: "FourFiveSix",
99+
Message: "Four Five Five is just fine https://example.com/" + to,
100+
LastTransitionTime: metav1.Now(),
101+
},
95102
{
96103
Type: "Recommended",
97104
Status: metav1.ConditionFalse,
@@ -233,8 +240,10 @@ func TestSyncAvailableUpdates_ConditionalUpdateRecommendedConditions(t *testing.
233240
availableUpdates, optr := newOperator(fakeOsus.URL, version, mockPromql)
234241
optr.availableUpdates = availableUpdates
235242
optr.availableUpdates.ConditionalUpdates = conditionalUpdates
236-
expectedConditions := []metav1.Condition{{}}
237-
conditionalUpdates[0].Conditions[0].DeepCopyInto(&expectedConditions[0])
243+
expectedConditions := make([]metav1.Condition, 0, len(conditionalUpdates[0].Conditions))
244+
for _, condition := range conditionalUpdates[0].Conditions {
245+
expectedConditions = append(expectedConditions, *condition.DeepCopy())
246+
}
238247
cv := cvFixture.DeepCopy()
239248
tc.modifyOriginalState(optr)
240249
tc.modifyCV(cv, conditionalUpdates[0])
@@ -268,16 +277,16 @@ func TestEvaluateConditionalUpdate(t *testing.T) {
268277
name string
269278
risks []configv1.ConditionalUpdateRisk
270279
mockPromql clusterconditions.Condition
271-
expected metav1.Condition
280+
expected []metav1.Condition
272281
}{
273282
{
274283
name: "no risks",
275-
expected: metav1.Condition{
284+
expected: []metav1.Condition{{
276285
Type: "Recommended",
277286
Status: metav1.ConditionTrue,
278287
Reason: recommendedReasonRisksNotExposed,
279288
Message: "The update is recommended, because none of the conditional update risks apply to this cluster.",
280-
},
289+
}},
281290
},
282291
{
283292
name: "one risk that does not match",
@@ -293,12 +302,12 @@ func TestEvaluateConditionalUpdate(t *testing.T) {
293302
ValidQueue: []error{nil},
294303
MatchQueue: []mock.MatchResult{{Match: false, Error: nil}},
295304
},
296-
expected: metav1.Condition{
305+
expected: []metav1.Condition{{
297306
Type: "Recommended",
298307
Status: metav1.ConditionTrue,
299308
Reason: recommendedReasonRisksNotExposed,
300309
Message: "The update is recommended, because none of the conditional update risks apply to this cluster.",
301-
},
310+
}},
302311
},
303312
{
304313
name: "one risk that matches",
@@ -314,12 +323,17 @@ func TestEvaluateConditionalUpdate(t *testing.T) {
314323
ValidQueue: []error{nil},
315324
MatchQueue: []mock.MatchResult{{Match: true, Error: nil}},
316325
},
317-
expected: metav1.Condition{
326+
expected: []metav1.Condition{{
327+
Type: "recommended/RiskThatApplies",
328+
Status: metav1.ConditionFalse,
329+
Reason: "RiskThatApplies",
330+
Message: "This is a risk! https://match.es",
331+
}, {
318332
Type: "Recommended",
319333
Status: metav1.ConditionFalse,
320334
Reason: "RiskThatApplies",
321335
Message: "This is a risk! https://match.es",
322-
},
336+
}},
323337
},
324338
{
325339
name: "matching risk with name that cannot be used as a condition reason",
@@ -335,12 +349,17 @@ func TestEvaluateConditionalUpdate(t *testing.T) {
335349
ValidQueue: []error{nil},
336350
MatchQueue: []mock.MatchResult{{Match: true, Error: nil}},
337351
},
338-
expected: metav1.Condition{
352+
expected: []metav1.Condition{{
353+
Type: "recommended/RISKTHATAPPLIES",
354+
Status: metav1.ConditionFalse,
355+
Reason: recommendedReasonExposed,
356+
Message: "This is a risk! https://match.es",
357+
}, {
339358
Type: "Recommended",
340359
Status: metav1.ConditionFalse,
341360
Reason: recommendedReasonExposed,
342361
Message: "This is a risk! https://match.es",
343-
},
362+
}},
344363
},
345364
{
346365
name: "two risks that match",
@@ -362,12 +381,22 @@ func TestEvaluateConditionalUpdate(t *testing.T) {
362381
ValidQueue: []error{nil, nil},
363382
MatchQueue: []mock.MatchResult{{Match: true, Error: nil}, {Match: true, Error: nil}},
364383
},
365-
expected: metav1.Condition{
384+
expected: []metav1.Condition{{
385+
Type: "recommended/RiskThatApplies",
386+
Status: metav1.ConditionFalse,
387+
Reason: "RiskThatApplies",
388+
Message: "This is a risk! https://match.es",
389+
}, {
390+
Type: "recommended/RiskThatAppliesToo",
391+
Status: metav1.ConditionFalse,
392+
Reason: "RiskThatAppliesToo",
393+
Message: "This is a risk too! https://match.es/too",
394+
}, {
366395
Type: "Recommended",
367396
Status: metav1.ConditionFalse,
368397
Reason: recommendedReasonMultiple,
369398
Message: "This is a risk! https://match.es\n\nThis is a risk too! https://match.es/too",
370-
},
399+
}},
371400
},
372401
{
373402
name: "first risk matches, second fails to evaluate",
@@ -389,15 +418,27 @@ func TestEvaluateConditionalUpdate(t *testing.T) {
389418
ValidQueue: []error{nil, nil},
390419
MatchQueue: []mock.MatchResult{{Match: true, Error: nil}, {Match: false, Error: errors.New("ERROR")}},
391420
},
392-
expected: metav1.Condition{
421+
expected: []metav1.Condition{{
422+
Type: "recommended/RiskThatApplies",
423+
Status: metav1.ConditionFalse,
424+
Reason: "RiskThatApplies",
425+
Message: "This is a risk! https://match.es",
426+
}, {
427+
Type: "recommended/RiskThatFailsToEvaluate",
428+
Status: metav1.ConditionUnknown,
429+
Reason: recommendedReasonEvaluationFailed,
430+
Message: "Could not evaluate exposure to update risk RiskThatFailsToEvaluate (ERROR)\n" +
431+
" RiskThatFailsToEvaluate description: This is a risk too!\n" +
432+
" RiskThatFailsToEvaluate URL: https://whokno.ws",
433+
}, {
393434
Type: "Recommended",
394435
Status: metav1.ConditionFalse,
395436
Reason: recommendedReasonMultiple,
396437
Message: "This is a risk! https://match.es\n\n" +
397438
"Could not evaluate exposure to update risk RiskThatFailsToEvaluate (ERROR)\n" +
398439
" RiskThatFailsToEvaluate description: This is a risk too!\n" +
399440
" RiskThatFailsToEvaluate URL: https://whokno.ws",
400-
},
441+
}},
401442
},
402443
{
403444
name: "one risk that fails to evaluate",
@@ -413,14 +454,21 @@ func TestEvaluateConditionalUpdate(t *testing.T) {
413454
ValidQueue: []error{nil},
414455
MatchQueue: []mock.MatchResult{{Match: false, Error: errors.New("ERROR")}},
415456
},
416-
expected: metav1.Condition{
457+
expected: []metav1.Condition{{
458+
Type: "recommended/RiskThatFailsToEvaluate",
459+
Status: metav1.ConditionUnknown,
460+
Reason: recommendedReasonEvaluationFailed,
461+
Message: "Could not evaluate exposure to update risk RiskThatFailsToEvaluate (ERROR)\n" +
462+
" RiskThatFailsToEvaluate description: This is a risk!\n" +
463+
" RiskThatFailsToEvaluate URL: https://whokno.ws",
464+
}, {
417465
Type: "Recommended",
418466
Status: metav1.ConditionUnknown,
419467
Reason: recommendedReasonEvaluationFailed,
420468
Message: "Could not evaluate exposure to update risk RiskThatFailsToEvaluate (ERROR)\n" +
421469
" RiskThatFailsToEvaluate description: This is a risk!\n" +
422470
" RiskThatFailsToEvaluate URL: https://whokno.ws",
423-
},
471+
}},
424472
},
425473
}
426474
for _, tc := range testcases {

0 commit comments

Comments
 (0)