fix: condition updater (#93)

* fix bug in condition updater (wrongfully updated condition)

* enforce reason

* add GenerateCreateConditionFunc function to controller package

* feat: release v0.13.1

Author: Diaphteiros

VERSION

@@ -1 +1 @@
-v0.13.0-dev
+v0.13.1

go.mod

@@ -11,7 +11,7 @@ require (
 	github.com/onsi/ginkgo v1.16.5
 	github.com/onsi/ginkgo/v2 v2.23.4
 	github.com/onsi/gomega v1.37.0
-	github.com/openmcp-project/controller-utils/api v0.13.0
+	github.com/openmcp-project/controller-utils/api v0.13.1
 	github.com/spf13/pflag v1.0.6
 	github.com/stretchr/testify v1.10.0
 	go.uber.org/zap v1.27.0

pkg/conditions/updater.go

@@ -88,6 +88,10 @@ func (c *conditionUpdater) WithEventRecorder(recorder record.EventRecorder, verb
 // All fields of the condition are updated with the values given in the arguments, but the condition's LastTransitionTime is only updated (with the timestamp contained in the receiver struct) if the status changed.
 // Returns the receiver for easy chaining.
 func (c *conditionUpdater) UpdateCondition(conType string, status metav1.ConditionStatus, observedGeneration int64, reason, message string) *conditionUpdater {
+	if reason == "" {
+		// the metav1.Condition type requires a reason, so let's add a dummy if none is given
+		reason = conType + string(status)
+	}
 	con := metav1.Condition{
 		Type:               conType,
 		Status:             status,
@@ -96,11 +100,6 @@ func (c *conditionUpdater) UpdateCondition(conType string, status metav1.Conditi
 		ObservedGeneration: observedGeneration,
 		LastTransitionTime: c.Now,
 	}
-	old, ok := c.conditions[conType]
-	if ok && old.Status == con.Status {
-		// update LastTransitionTime only if status changed
-		con.LastTransitionTime = old.LastTransitionTime
-	}
 	c.updates[conType] = status
 	c.conditions[conType] = con
 	return c
@@ -134,7 +133,13 @@ func (c *conditionUpdater) RemoveCondition(conType string) *conditionUpdater {
 // The conditions are returned sorted by their type.
 // The second return value indicates whether the condition list has actually changed.
 func (c *conditionUpdater) Conditions() ([]metav1.Condition, bool) {
-	res := c.updatedConditions()
+	res := collections.ProjectSlice(c.updatedConditions(), func(con metav1.Condition) metav1.Condition {
+		if c.original[con.Type].Status == con.Status {
+			// if the status has not changed, reset the LastTransitionTime to the original value
+			con.LastTransitionTime = c.original[con.Type].LastTransitionTime
+		}
+		return con
+	})
 	slices.SortStableFunc(res, func(a, b metav1.Condition) int {
 		return strings.Compare(a.Type, b.Type)
 	})

pkg/conditions/updater_test.go

@@ -161,6 +161,16 @@ var _ = Describe("Conditions", func() {
 			Expect(newCon.LastTransitionTime.After(oldCon.LastTransitionTime.Time)).To(BeTrue())
 		})
 
+		It("should not change the last transition time if the same condition is updated multiple times with the last update setting it to the original value again", func() {
+			cons := testConditionSet()
+			oldCon := conditions.GetCondition(cons, "true")
+			updater := conditions.ConditionUpdater(cons, false)
+			updated, changed := updater.UpdateCondition(oldCon.Type, invert(oldCon.Status), oldCon.ObservedGeneration+1, "newReason", "newMessage").
+				UpdateCondition(oldCon.Type, oldCon.Status, oldCon.ObservedGeneration, oldCon.Reason, oldCon.Message).Conditions()
+			Expect(changed).To(BeFalse())
+			Expect(updated).To(ConsistOf(cons))
+		})
+
 		It("should sort the conditions by type", func() {
 			cons := []metav1.Condition{
 				TestConditionFromValues("c", conditions.FromBool(true), 0, "reason", "message", metav1.Now()).ToCondition(),

pkg/controller/status_updater.go

@@ -231,7 +231,11 @@ func (s *statusUpdater[Obj]) UpdateStatus(ctx context.Context, c client.Client,
 		}
 		cu.Now = now
 		for _, con := range rr.Conditions {
-			cu.UpdateConditionFromTemplate(con)
+			gen := con.ObservedGeneration
+			if gen == 0 {
+				gen = rr.Object.GetGeneration()
+			}
+			cu.UpdateCondition(con.Type, con.Status, gen, con.Reason, con.Message)
 		}
 		newCons, _ := cu.Record(rr.Object).Conditions()
 		SetField(status, s.fieldNames[STATUS_FIELD_CONDITIONS], newCons)
@@ -366,3 +370,21 @@ type ReconcileResult[Obj client.Object] struct {
 	// The lastTransition timestamp of the condition will be overwritten with the current time while updating.
 	Conditions []metav1.Condition
 }
+
+// GenerateCreateConditionFunc returns a function that can be used to add a condition to the given ReconcileResult.
+// If the ReconcileResult's Object is not nil, the condition's ObservedGeneration is set to the object's generation.
+func GenerateCreateConditionFunc[Obj client.Object](rr *ReconcileResult[Obj]) func(conType string, status metav1.ConditionStatus, reason, message string) {
+	var gen int64 = 0
+	if any(rr.Object) != nil {
+		gen = rr.Object.GetGeneration()
+	}
+	return func(conType string, status metav1.ConditionStatus, reason, message string) {
+		rr.Conditions = append(rr.Conditions, metav1.Condition{
+			Type:               conType,
+			Status:             status,
+			ObservedGeneration: gen,
+			Reason:             reason,
+			Message:            message,
+		})
+	}
+}

pkg/controller/status_updater_test.go

@@ -194,6 +194,47 @@ var _ = Describe("Status Updater", func() {
 		}
 	})
 
+	Context("GenerateCreateConditionFunc", func() {
+
+		It("should add the condition to the given ReconcileResult", func() {
+			rr := controller.ReconcileResult[*CustomObject]{
+				Object: &CustomObject{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:       "test",
+						Namespace:  "default",
+						Generation: 15,
+					},
+				},
+				Conditions: dummyConditions(),
+			}
+			createCon := controller.GenerateCreateConditionFunc(&rr)
+			createCon("TestConditionFoo", metav1.ConditionTrue, "TestReasonFoo", "TestMessageFoo")
+			Expect(rr.Conditions).To(ConsistOf(
+				MatchCondition(TestConditionFromCondition(dummyConditions()[0])),
+				MatchCondition(TestConditionFromCondition(dummyConditions()[1])),
+				MatchCondition(TestConditionFromValues("TestConditionFoo", metav1.ConditionTrue, 15, "TestReasonFoo", "TestMessageFoo", metav1.Time{})),
+			))
+		})
+
+		It("should create the condition list, if it is nil", func() {
+			rr := controller.ReconcileResult[*CustomObject]{
+				Object: &CustomObject{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:       "test",
+						Namespace:  "default",
+						Generation: 15,
+					},
+				},
+			}
+			createCon := controller.GenerateCreateConditionFunc(&rr)
+			createCon("TestConditionFoo", metav1.ConditionTrue, "TestReasonFoo", "TestMessageFoo")
+			Expect(rr.Conditions).To(ConsistOf(
+				MatchCondition(TestConditionFromValues("TestConditionFoo", metav1.ConditionTrue, 15, "TestReasonFoo", "TestMessageFoo", metav1.Time{})),
+			))
+		})
+
+	})
+
 })
 
 func preconfiguredStatusUpdaterBuilder() *controller.StatusUpdaterBuilder[*CustomObject] {