Add support negative polarity conditions

Author: fabriziopandini

docs/proposals/20200506-conditions.md

@@ -6,7 +6,7 @@ reviewers:
   - "@vincepri"
   - "@ncdc"
 creation-date: 2020-05-06
-last-updated: 2020-05-20
+last-updated: 2024-05-03
 status: implementable
 see-also:
 replaces:
@@ -232,7 +232,7 @@ const (
 )
 ```
 
-Condition types MUST have a consistent polarity (i.e. "True = good");
+Condition type names should make sense for humans; neither positive nor negative polarity can be recommended as a general rule.
 
 Condition types SHOULD have one of the following suffix:
 
@@ -243,8 +243,8 @@ When the above suffix are not adequate for a specific condition type, other suff
 (e.g. `Completed`, `Healthy`); however, it is recommended to balance this flexibility with the objective to provide
 a consistent condition naming across all the Cluster API objects.
 
-The `Severity` field MUST be set only when `Status=False` and it is designed to provide a standard classification
-of possible conditions failure `Reason`. 
+The `Severity` field MUST be set only when `Status=False` + positive polarity or when `Status=True` + negative polarity;
+severity it is designed to provide a standard classification of possible conditions failure `Reason`. 
 
 Please note that the combination of `Reason` and `Severity` gives different meaning to a condition failure
 allowing to detect when a long-running task is still ongoing:
@@ -298,6 +298,8 @@ the following constraints/design principles MUST be applied:
   of the underlying `Machines.Status.Conditions[Ready]` conditions.
 - A corollary of the above set of constraints is that an object SHOULD NEVER be in status `Ready=True`
   if one of the object's conditions are `false` or if one of the object dependents is in status `Ready=False`.
+- Condition that do not represent the operational state of the component, MUST not be included in the  `Ready` condition
+  (e.g. `Paused`, which represent a state of the controller that manages the component).
 
 ##### Controller changes
 
@@ -471,6 +473,7 @@ enhance the condition utilities to handle those situations in a generalized way.
 - Risk: This proposal aims to be consistent with the target state of conditions in Kubernetes, but this
   is still under definition (see [KEP](https://github.com/kubernetes/enhancements/pull/1624)).
   - Mitigation: Periodically re-evaluate this proposal vs the Kubernetes KEP. 
+    -  2024-05-03: Change allowing conditions with negative polarity goes into this direction
 
 - Risk: Cluster API presents some specific challenges that are not common to the core Kubernetes objects.
   - Mitigation: To allow a minimal set of carefully evaluated differences between Cluster API and Kubernetes
@@ -480,25 +483,6 @@ enhance the condition utilities to handle those situations in a generalized way.
 - Risk: This proposal allows for implementing conditions in incremental fashion, and this makes it complex
   to ensure a consistent approach across all objects.
   - Mitigation: Ensure all the implementations comply with the defined set of constraints/design principles. 
-
-- Risk: Having a consistent polarity ensures a simple and clear contract with the consumers, and it allows 
-  processing conditions in a simple and consistent way without being forced to implement specific logic
-  for each condition type. However, we are aware about the fact that enforcing of consistent polarity (truthy)
-  combined with the usage of recommended suffix for condition types can lead to verbal contortions to express 
-  conditions, especially in case of conditions designed to signal problems or in case of conditions
-  that might exist or not.
-  - Mitigation: We are relaxing the rule about recommended suffix and allowing usage of custom suffix.
-  - Mitigation: We are recommending the condition adhere to the design principle to express the operational state
-    of the component, and this should help in avoiding conditions name to surface internal implementation details.
-  - Mitigation: We should recommend condition implementers to clearly document the meaning of Unknown state, because as
-    discussed also in the recent [Kubernetes KEP about standardizing conditions](https://github.com/kubernetes/enhancements/pull/1624#pullrequestreview-388777427),
-    _"Unknown" is a fact about the writer of the condition, and not a claim about the object_.
-  - Mitigation: We should recommend developers of code relying on conditions to treat Unknown as a separated state vs
-    assimilating it to True or False, because this can vary case by case and generate confusion in readers.
-    
-  As a final consideration about the risk related to using a consistent polarity, it is important to notice that a
-  consistent polarity ensure a clear meaning for True or o False states, which is already an improvement vs having
-  different interpretations for all the three possible condition states.
 
 ## Alternatives
 
@@ -569,5 +553,6 @@ NA
 
 ## Implementation History
 
-- [ ] 2020-04-27: Compile a Google Doc following the CAEP template
-- [ ] 2020-05-06: Create CAEP PR 
+- [x] 2020-04-27: Compile a Google Doc following the CAEP template
+- [x] 2020-05-06: Create CAEP PR 
+- [x] 2024-05-03: Edited allowing conditions with negative polarity 

util/conditions/getter.go

@@ -117,6 +117,7 @@ func GetLastTransitionTime(from Getter, t clusterv1.ConditionType) *metav1.Time
 
 // summary returns a Ready condition with the summary of all the conditions existing
 // on an object. If the object does not have other conditions, no summary condition is generated.
+// NOTE: The resulting Ready condition will have positive polarity; the we are starting from might have positive or negative polarity.
 func summary(from Getter, options ...MergeOption) *clusterv1.Condition {
 	conditions := from.GetConditions()
 
@@ -147,8 +148,18 @@ func summary(from Getter, options ...MergeOption) *clusterv1.Condition {
 			}
 		}
 
+		// Keep track of the polarity of the condition we are starting from.
+		polarity := PositivePolarity
+		for _, t := range mergeOpt.negativeConditionTypes {
+			if c.Type == t {
+				polarity = NegativePolarity
+				break
+			}
+		}
+
 		conditionsInScope = append(conditionsInScope, localizedCondition{
 			Condition: &c,
+			Polarity:  polarity,
 			Getter:    from,
 		})
 	}
@@ -210,6 +221,7 @@ func WithFallbackValue(fallbackValue bool, reason string, severity clusterv1.Con
 
 // mirror mirrors the Ready condition from a dependent object into the target condition;
 // if the Ready condition does not exists in the source object, no target conditions is generated.
+// NOTE: Considering that we are mirroring Ready conditions with positive polarity, also the resulting condition will have positive polarity.
 func mirror(from Getter, targetCondition clusterv1.ConditionType, options ...MirrorOptions) *clusterv1.Condition {
 	mirrorOpt := &mirrorOptions{}
 	for _, o := range options {
@@ -237,13 +249,15 @@ func mirror(from Getter, targetCondition clusterv1.ConditionType, options ...Mir
 // Aggregates all the Ready condition from a list of dependent objects into the target object;
 // if the Ready condition does not exists in one of the source object, the object is excluded from
 // the aggregation; if none of the source object have ready condition, no target conditions is generated.
+// NOTE: Considering that we are aggregating Ready conditions with positive polarity, also the resulting condition will have positive polarity.
 func aggregate(from []Getter, targetCondition clusterv1.ConditionType, options ...MergeOption) *clusterv1.Condition {
 	conditionsInScope := make([]localizedCondition, 0, len(from))
 	for i := range from {
 		condition := Get(from[i], clusterv1.ReadyCondition)
 
 		conditionsInScope = append(conditionsInScope, localizedCondition{
 			Condition: condition,
+			Polarity:  PositivePolarity,
 			Getter:    from[i],
 		})
 	}

util/conditions/getter_test.go

@@ -20,6 +20,7 @@ import (
 	"testing"
 
 	. "github.com/onsi/gomega"
+	"k8s.io/apimachinery/pkg/util/sets"
 
 	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 )
@@ -31,6 +32,13 @@ var (
 	falseInfo1    = FalseCondition("falseInfo1", "reason falseInfo1", clusterv1.ConditionSeverityInfo, "message falseInfo1")
 	falseWarning1 = FalseCondition("falseWarning1", "reason falseWarning1", clusterv1.ConditionSeverityWarning, "message falseWarning1")
 	falseError1   = FalseCondition("falseError1", "reason falseError1", clusterv1.ConditionSeverityError, "message falseError1")
+
+	negativePolarityConditions = sets.New("positive-false1", "negative-unknown1", "negative-trueInfo1", "negative-trueWarning1", "negative-trueError1")
+	positiveFalse1             = PositiveFalseCondition("positive-false1")
+	negativeUnknown1           = UnknownCondition("negative-unknown1", "reason negative-unknown1", "message negative-unknown1")
+	negativeTrueInfo1          = NegativeTrueCondition("negative-trueInfo1", "reason negative-trueInfo1", clusterv1.ConditionSeverityInfo, "message negative-trueInfo1")
+	negativeTrueWarning1       = NegativeTrueCondition("negative-trueWarning1", "reason negative-trueWarning1", clusterv1.ConditionSeverityWarning, "message negative-trueWarning1")
+	negativeTrueError1         = NegativeTrueCondition("negative-trueError1", "reason negative-trueError1", clusterv1.ConditionSeverityError, "message negative-trueError1")
 )
 
 func TestGetAndHas(t *testing.T) {
@@ -50,41 +58,54 @@ func TestGetAndHas(t *testing.T) {
 func TestIsMethods(t *testing.T) {
 	g := NewWithT(t)
 
-	obj := getterWithConditions(nil1, true1, unknown1, falseInfo1, falseWarning1, falseError1)
+	obj := getterWithConditions(nil1, true1, unknown1, falseInfo1, falseWarning1, falseError1, positiveFalse1, negativeUnknown1, negativeTrueInfo1, negativeTrueWarning1, negativeTrueError1)
 
 	// test isTrue
 	g.Expect(IsTrue(obj, "nil1")).To(BeFalse())
 	g.Expect(IsTrue(obj, "true1")).To(BeTrue())
 	g.Expect(IsTrue(obj, "falseInfo1")).To(BeFalse())
 	g.Expect(IsTrue(obj, "unknown1")).To(BeFalse())
+	g.Expect(IsTrue(obj, "positive-false1")).To(BeFalse())
+	g.Expect(IsTrue(obj, "negative-trueInfo1")).To(BeTrue())
+	g.Expect(IsTrue(obj, "negative-unknown1")).To(BeFalse())
 
 	// test isFalse
 	g.Expect(IsFalse(obj, "nil1")).To(BeFalse())
 	g.Expect(IsFalse(obj, "true1")).To(BeFalse())
 	g.Expect(IsFalse(obj, "falseInfo1")).To(BeTrue())
 	g.Expect(IsFalse(obj, "unknown1")).To(BeFalse())
+	g.Expect(IsFalse(obj, "positive-false1")).To(BeTrue())
+	g.Expect(IsFalse(obj, "negative-trueInfo1")).To(BeFalse())
+	g.Expect(IsFalse(obj, "negative-unknown1")).To(BeFalse())
 
 	// test isUnknown
 	g.Expect(IsUnknown(obj, "nil1")).To(BeTrue())
 	g.Expect(IsUnknown(obj, "true1")).To(BeFalse())
 	g.Expect(IsUnknown(obj, "falseInfo1")).To(BeFalse())
 	g.Expect(IsUnknown(obj, "unknown1")).To(BeTrue())
+	g.Expect(IsUnknown(obj, "positive-false1")).To(BeFalse())
+	g.Expect(IsUnknown(obj, "negative-trueInfo1")).To(BeFalse())
+	g.Expect(IsUnknown(obj, "negative-unknown1")).To(BeTrue())
 
 	// test GetReason
 	g.Expect(GetReason(obj, "nil1")).To(Equal(""))
 	g.Expect(GetReason(obj, "falseInfo1")).To(Equal("reason falseInfo1"))
+	g.Expect(GetReason(obj, "negative-trueInfo1")).To(Equal("reason negative-trueInfo1"))
 
 	// test GetMessage
 	g.Expect(GetMessage(obj, "nil1")).To(Equal(""))
 	g.Expect(GetMessage(obj, "falseInfo1")).To(Equal("message falseInfo1"))
+	g.Expect(GetMessage(obj, "negative-trueInfo1")).To(Equal("message negative-trueInfo1"))
 
 	// test GetSeverity
+	expectedSeverity := clusterv1.ConditionSeverityInfo
 	g.Expect(GetSeverity(obj, "nil1")).To(BeNil())
 	severity := GetSeverity(obj, "falseInfo1")
-	expectedSeverity := clusterv1.ConditionSeverityInfo
+	g.Expect(severity).To(Equal(&expectedSeverity))
+	severity = GetSeverity(obj, "negative-trueInfo1")
 	g.Expect(severity).To(Equal(&expectedSeverity))
 
-	// test GetMessage
+	// test GetLastTransitionTime
 	g.Expect(GetLastTransitionTime(obj, "nil1")).To(BeNil())
 	g.Expect(GetLastTransitionTime(obj, "falseInfo1")).ToNot(BeNil())
 }
@@ -132,6 +153,9 @@ func TestSummary(t *testing.T) {
 	foo := TrueCondition("foo")
 	bar := FalseCondition("bar", "reason falseInfo1", clusterv1.ConditionSeverityInfo, "message falseInfo1")
 	baz := FalseCondition("baz", "reason falseInfo2", clusterv1.ConditionSeverityInfo, "message falseInfo2")
+	negativeFoo := PositiveFalseCondition("negative-foo")
+	negativeBar := NegativeTrueCondition("negative-bar", "reason negative-falseInfo1", clusterv1.ConditionSeverityInfo, "message negative-falseInfo1")
+	// negativeBaz := NegativeTrueCondition("negative-baz", "reason negative-falseInfo2", clusterv1.ConditionSeverityInfo, "message negative-falseInfo2")
 	existingReady := FalseCondition(clusterv1.ReadyCondition, "reason falseError1", clusterv1.ConditionSeverityError, "message falseError1") // NB. existing ready has higher priority than other conditions
 
 	tests := []struct {
@@ -150,6 +174,18 @@ func TestSummary(t *testing.T) {
 			from: getterWithConditions(foo, bar),
 			want: FalseCondition(clusterv1.ReadyCondition, "reason falseInfo1", clusterv1.ConditionSeverityInfo, "message falseInfo1"),
 		},
+		{
+			name:    "Returns ready condition with the summary of existing conditions with negative polarity (with default options)",
+			from:    getterWithConditions(negativeFoo, negativeBar),
+			options: []MergeOption{WithNegativePolarityConditions("negative-foo", "negative-bar")},
+			want:    FalseCondition(clusterv1.ReadyCondition, "reason negative-falseInfo1", clusterv1.ConditionSeverityInfo, "message negative-falseInfo1"),
+		},
+		{
+			name:    "Returns ready condition with the summary of existing conditions with mixed polarity (with default options)",
+			from:    getterWithConditions(foo, bar, negativeFoo, negativeBar),
+			options: []MergeOption{WithNegativePolarityConditions("negative-foo", "negative-bar")},
+			want:    FalseCondition(clusterv1.ReadyCondition, "reason falseInfo1", clusterv1.ConditionSeverityInfo, "message falseInfo1"), // bar take precedence on negativeBar because it is listed first
+		},
 		{
 			name:    "Returns ready condition with the summary of existing conditions (using WithStepCounter options)",
 			from:    getterWithConditions(foo, bar),
@@ -186,6 +222,18 @@ func TestSummary(t *testing.T) {
 			options: []MergeOption{WithConditions("foo")}, // bar should be ignored
 			want:    TrueCondition(clusterv1.ReadyCondition),
 		},
+		{
+			name:    "Returns ready condition with the summary of selected conditions with negative polarity (using WithConditions options)",
+			from:    getterWithConditions(negativeFoo, negativeBar),
+			options: []MergeOption{WithConditions("negative-foo"), WithNegativePolarityConditions("negative-foo", "negative-bar")}, // negative-bar should be ignored
+			want:    TrueCondition(clusterv1.ReadyCondition),
+		},
+		{
+			name:    "Returns ready condition with the summary of selected conditions with mixed polarity (using WithConditions options)",
+			from:    getterWithConditions(foo, bar, negativeFoo, negativeBar),
+			options: []MergeOption{WithConditions("foo", "negative-foo", "negative-bar"), WithNegativePolarityConditions("negative-foo", "negative-bar")}, // bar should be ignored
+			want:    FalseCondition(clusterv1.ReadyCondition, "reason negative-falseInfo1", clusterv1.ConditionSeverityInfo, "message negative-falseInfo1"),
+		},
 		{
 			name:    "Returns ready condition with the summary of selected conditions (using WithConditions and WithStepCounter options)",
 			from:    getterWithConditions(foo, bar, baz),