Mirror order conditions as False, Unknown, True

This is a breaking change on Conditions.Mirror(). Originally this call
ordered conditions as False, True, Unknown then picked the first of that
list to reflect. However all the use cases (we see) assume that if there
is reasourceA that depends on the status of resourceB then resourceA
reconciler calls:

  resourceB.Status.Mirror(ResourceBSpecificSubConditionOfA)

and expects that the call returns the most sever non True condition if any
or return a True condition otherwise. So the original behavior that
returned True even if there was Unknown condition was unexpected.

This patch changes the order of the status to False, Unknown, True. But
keeps the original logic that if the ReadyCondition is True then the
rest of the conditions are ignored even if there are False conditions in
the list.

modules/common/condition/funcs.go

@@ -295,9 +295,15 @@ func GetHigherPrioCondition(cond1, cond2 *Condition) *Condition {
 }
 
 // Mirror - mirrors Status, Message, Reason and Severity from the latest condition
-// of a sorted conditionGroup list into a target condition of type t.
-// The conditionGroup entries are split by Status with the order False, True, Unknown.
+// of a sorted conditionGroup list into a target condition of type t. If the
+// top level ReadyCondition is True then it is assumed that there are no False
+// or important Uknown conditions present in the list as ReadyCondition is expected
+// to be an aggregated status condition.
+// If ReadyCondition is not True then the conditionGroup entries are split by
+// Status with the order False, Unknown, True.
 // If Status=False its again split into Severity with the order Error, Warning, Info.
+// So Mirror either reflects the ReadyCondition=True or reflects the latest most
+// sever False or Uknown condition.
 func (conditions *Conditions) Mirror(t Type) *Condition {
 
 	if conditions == nil || len(*conditions) == 0 {
@@ -344,8 +350,17 @@ func (conditions *Conditions) Mirror(t Type) *Condition {
 			break
 		}
 
-		mirrorCondition = UnknownCondition(t, c.Reason, c.Message)
-		mirrorCondition.LastTransitionTime = c.LastTransitionTime
+		if c.Status == corev1.ConditionUnknown {
+			mirrorCondition = UnknownCondition(t, c.Reason, c.Message)
+			mirrorCondition.LastTransitionTime = c.LastTransitionTime
+			break
+		}
+
+		// The only valid values for Status is True, False, Unknown are handled
+		// above so if we reach here then we have an invalid status condition.
+		// This should never happen.
+		panic(fmt.Sprintf("Condition %v has invalid status value '%s'. The only valid values are True, False, Unknown", c, c.Status))
+
 	}
 
 	return mirrorCondition
@@ -392,9 +407,9 @@ func groupOrder(c Condition) int {
 		case SeverityInfo:
 			return 2
 		}
-	case corev1.ConditionTrue:
-		return 3
 	case corev1.ConditionUnknown:
+		return 3
+	case corev1.ConditionTrue:
 		return 4
 	}
 

modules/common/condition/funcs_test.go

@@ -387,10 +387,10 @@ func TestMirror(t *testing.T) {
 	g.Expect(conditions).To(haveSameConditionsOf(CreateList(unknownReady, trueA)))
 	targetCondition = conditions.Mirror("targetConditon")
 	// expect to be mirrored trueA
-	g.Expect(targetCondition.Status).To(BeIdenticalTo(trueA.Status))
-	g.Expect(targetCondition.Severity).To(BeIdenticalTo(trueA.Severity))
-	g.Expect(targetCondition.Reason).To(BeIdenticalTo(trueA.Reason))
-	g.Expect(targetCondition.Message).To(BeIdenticalTo(trueA.Message))
+	g.Expect(targetCondition.Status).To(BeIdenticalTo(unknownReady.Status))
+	g.Expect(targetCondition.Severity).To(BeIdenticalTo(unknownReady.Severity))
+	g.Expect(targetCondition.Reason).To(BeIdenticalTo(unknownReady.Reason))
+	g.Expect(targetCondition.Message).To(BeIdenticalTo(unknownReady.Message))
 
 	conditions.Set(falseB)
 	g.Expect(conditions).To(haveSameConditionsOf(CreateList(unknownReady, trueA, falseB)))
@@ -422,6 +422,45 @@ func TestMirror(t *testing.T) {
 	g.Expect(targetCondition.Message).To(BeIdenticalTo(trueReady.Message))
 }
 
+func TestMirrorReadyTrueAndFalse(t *testing.T) {
+	g := NewWithT(t)
+
+	conditions := Conditions{}
+	// We expect that ReadyCondition True means the overall status of the
+	// resource is ready that condition is mirrored even if there are other
+	// conditions in non True state.
+	conditions.Init(&Conditions{*trueReady, *falseB, *unknownA})
+
+	targetCondition := conditions.Mirror("targetConditon")
+
+	g.Expect(targetCondition.Status).To(BeIdenticalTo(trueReady.Status))
+	g.Expect(targetCondition.Severity).To(BeIdenticalTo(trueReady.Severity))
+	g.Expect(targetCondition.Reason).To(BeIdenticalTo(trueReady.Reason))
+	g.Expect(targetCondition.Message).To(BeIdenticalTo(trueReady.Message))
+}
+
+func TestMirrorInvalidStatus(t *testing.T) {
+	g := NewWithT(t)
+
+	conditions := Conditions{}
+	invalidStatusA := Condition{
+		Type:     "a",
+		Status:   "FooBar",
+		Reason:   "",
+		Severity: SeverityNone,
+		Message:  "",
+	}
+	conditions.Init(&Conditions{invalidStatusA})
+	// NOTE(gibi): we always have a ReadyCondition added automatically so it
+	// is picked up by Mirror before reaching to the condition with FooBar
+	// status. So to trigger the error case we need to remove the ReadyCondition
+	// explicitly.
+	conditions.Remove(ReadyCondition)
+
+	g.Expect(func() { conditions.Mirror("targetConditon") }).To(
+		PanicWith(MatchRegexp(`Condition \{a FooBar .*\} has invalid status value 'FooBar'.`)))
+}
+
 func TestIsError(t *testing.T) {
 	g := NewWithT(t)
 
@@ -444,11 +483,15 @@ func TestGetHigherPrioCondition(t *testing.T) {
 	c = GetHigherPrioCondition(nil, unknownA)
 	g.Expect(hasSameState(c, unknownA)).To(BeTrue())
 
-	// Status True has higher prio then Status Unknown
+	// Status Unknown has higher prio then Status True
 	c = GetHigherPrioCondition(unknownA, trueA)
-	g.Expect(hasSameState(c, trueA)).To(BeTrue())
+	g.Expect(hasSameState(c, unknownA)).To(BeTrue())
 
 	// Status False has higher prio then Status Unknown
+	c = GetHigherPrioCondition(falseA, unknownA)
+	g.Expect(hasSameState(c, falseA)).To(BeTrue())
+
+	// Status False has higher prio then Status True
 	c = GetHigherPrioCondition(falseA, trueA)
 	g.Expect(hasSameState(c, falseA)).To(BeTrue())