Merge pull request #2037 from vmware-tanzu/jtc/refactor-conditions-util

Refactor conditions util

internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go

@@ -735,10 +735,10 @@ func (c *jwtCacheFillerController) updateStatus(
 
 	_ = conditionsutil.MergeConditions(
 		conditions,
-		original.Generation,
 		&updated.Status.Conditions,
-		logger,
+		original.Generation,
 		metav1.NewTime(c.clock.Now()),
+		logger,
 	)
 
 	if equality.Semantic.DeepEqual(original, updated) {

internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go

@@ -527,10 +527,10 @@ func (c *webhookCacheFillerController) updateStatus(
 
 	_ = conditionsutil.MergeConditions(
 		conditions,
-		original.Generation,
 		&updated.Status.Conditions,
-		logger,
+		original.Generation,
 		metav1.NewTime(c.clock.Now()),
+		logger,
 	)
 
 	if equality.Semantic.DeepEqual(original, updated) {

internal/controller/conditionsutil/conditions_util.go

@@ -4,9 +4,10 @@
 package conditionsutil
 
 import (
+	"slices"
 	"sort"
 
-	"k8s.io/apimachinery/pkg/api/equality"
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"go.pinniped.dev/internal/plog"
@@ -23,70 +24,66 @@ const (
 	MessageUnableToValidate = "unable to validate; see other conditions for details"
 )
 
-// MergeConditions merges conditions into conditionsToUpdate.
-// Note that LastTransitionTime refers to the time when the status changed,
-// but ObservedGeneration should be the current generation for all conditions, since Pinniped should always check every condition.
+// MergeConditions merges newConditions into existingConditionsToUpdate.
+// Note that lastTransitionTime refers to the time when the status changed,
+// but observedGeneration should be the current generation for all conditions,
+// since Pinniped should always check every condition.
 // It returns true if any resulting condition has non-true status.
 func MergeConditions(
-	conditions []*metav1.Condition,
+	newConditions []*metav1.Condition,
+	existingConditionsToUpdate *[]metav1.Condition,
 	observedGeneration int64,
-	conditionsToUpdate *[]metav1.Condition,
-	log plog.MinLogger,
 	lastTransitionTime metav1.Time,
+	log plog.MinLogger,
 ) bool {
-	for i := range conditions {
-		cond := conditions[i].DeepCopy()
-		cond.LastTransitionTime = lastTransitionTime
-		cond.ObservedGeneration = observedGeneration
-		if mergeCondition(conditionsToUpdate, cond) {
-			log.Info("updated condition", "type", cond.Type, "status", cond.Status, "reason", cond.Reason, "message", cond.Message)
+	for i := range newConditions {
+		newCondition := newConditions[i].DeepCopy()
+		newCondition.LastTransitionTime = lastTransitionTime
+		newCondition.ObservedGeneration = observedGeneration
+		if mergeCondition(existingConditionsToUpdate, newCondition) {
+			log.Info("updated condition",
+				"type", newCondition.Type,
+				"status", newCondition.Status,
+				"reason", newCondition.Reason,
+				"message", newCondition.Message)
 		}
 	}
-	sort.SliceStable(*conditionsToUpdate, func(i, j int) bool {
-		return (*conditionsToUpdate)[i].Type < (*conditionsToUpdate)[j].Type
+	sort.SliceStable(*existingConditionsToUpdate, func(i, j int) bool {
+		return (*existingConditionsToUpdate)[i].Type < (*existingConditionsToUpdate)[j].Type
 	})
-	return HadErrorCondition(conditions)
+	return HadErrorCondition(newConditions)
 }
 
 // mergeCondition merges a new metav1.Condition into a slice of existing conditions. It returns true
-// if the condition has meaningfully changed.
-func mergeCondition(existing *[]metav1.Condition, new *metav1.Condition) bool {
+// if something other than the LastTransitionTime has been updated.
+func mergeCondition(existingConditionsToUpdate *[]metav1.Condition, newCondition *metav1.Condition) bool {
 	// Find any existing condition with a matching type.
-	var old *metav1.Condition
-	for i := range *existing {
-		if (*existing)[i].Type == new.Type {
-			old = &(*existing)[i]
-			continue
-		}
-	}
+	index := slices.IndexFunc(*existingConditionsToUpdate, func(condition metav1.Condition) bool {
+		return newCondition.Type == condition.Type
+	})
 
-	// If there is no existing condition of this type, append this one and we're done.
-	if old == nil {
-		*existing = append(*existing, *new)
+	var existingCondition *metav1.Condition
+	if index < 0 {
+		// If there is no existing condition of this type, append this one and we're done.
+		*existingConditionsToUpdate = append(*existingConditionsToUpdate, *newCondition)
 		return true
 	}
 
-	// Set the LastTransitionTime depending on whether the status has changed.
-	new = new.DeepCopy()
-	if old.Status == new.Status {
-		new.LastTransitionTime = old.LastTransitionTime
+	// Get a pointer to the existing condition
+	existingCondition = &(*existingConditionsToUpdate)[index]
+
+	// If the status has not changed, preserve the original lastTransitionTime
+	if newCondition.Status == existingCondition.Status {
+		newCondition.LastTransitionTime = existingCondition.LastTransitionTime
 	}
 
-	// If anything has actually changed, update the entry and return true.
-	if !equality.Semantic.DeepEqual(old, new) {
-		*old = *new
-		return true
-	}
-
-	// Otherwise the entry is already up-to-date.
-	return false
+	changed := !apiequality.Semantic.DeepEqual(existingCondition, newCondition)
+	*existingCondition = *newCondition
+	return changed
 }
 
 func HadErrorCondition(conditions []*metav1.Condition) bool {
-	for _, c := range conditions {
-		if c.Status != metav1.ConditionTrue {
-			return true
-		}
-	}
-	return false
+	return slices.ContainsFunc(conditions, func(condition *metav1.Condition) bool {
+		return condition.Status != metav1.ConditionTrue
+	})
 }

internal/controller/conditionsutil/conditions_util_test.go

@@ -21,13 +21,13 @@ func TestMergeIDPConditions(t *testing.T) {
 	testTime := metav1.Now()
 
 	tests := []struct {
-		name               string
-		newConditions      []*metav1.Condition
-		conditionsToUpdate *[]metav1.Condition
-		observedGeneration int64
-		wantResult         bool
-		wantLogSnippets    []string
-		wantConditions     []metav1.Condition
+		name                       string
+		newConditions              []*metav1.Condition
+		existingConditionsToUpdate *[]metav1.Condition
+		observedGeneration         int64
+		wantResult                 bool
+		wantLogSnippets            []string
+		wantConditions             []metav1.Condition
 	}{
 		{
 			name: "Adding a new condition with status=True returns false",
@@ -39,8 +39,8 @@ func TestMergeIDPConditions(t *testing.T) {
 					Message: "new message",
 				},
 			},
-			observedGeneration: int64(999),
-			conditionsToUpdate: &[]metav1.Condition{},
+			observedGeneration:         int64(999),
+			existingConditionsToUpdate: &[]metav1.Condition{},
 			wantLogSnippets: []string{
 				`"message":"updated condition","type":"NewType","status":"True"`,
 			},
@@ -78,7 +78,7 @@ func TestMergeIDPConditions(t *testing.T) {
 					Message: "new message",
 				},
 			},
-			conditionsToUpdate: &[]metav1.Condition{
+			existingConditionsToUpdate: &[]metav1.Condition{
 				{
 					Type:               "UnchangedType",
 					Status:             metav1.ConditionTrue,
@@ -140,7 +140,7 @@ func TestMergeIDPConditions(t *testing.T) {
 					Message: "unchanged message",
 				},
 			},
-			conditionsToUpdate: &[]metav1.Condition{
+			existingConditionsToUpdate: &[]metav1.Condition{
 				{
 					Type:               "UnchangedType",
 					Status:             metav1.ConditionFalse,
@@ -165,26 +165,100 @@ func TestMergeIDPConditions(t *testing.T) {
 		},
 	}
 
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
 			var log bytes.Buffer
 			logger := plog.TestLogger(t, &log)
 
 			result := MergeConditions(
-				tt.newConditions,
-				tt.observedGeneration,
-				tt.conditionsToUpdate,
-				logger,
+				test.newConditions,
+				test.existingConditionsToUpdate,
+				test.observedGeneration,
 				testTime,
+				logger,
 			)
 
 			logString := log.String()
-			require.Equal(t, len(tt.wantLogSnippets), strings.Count(logString, "\n"))
-			for _, wantLog := range tt.wantLogSnippets {
+			require.Equal(t, len(test.wantLogSnippets), strings.Count(logString, "\n"))
+			for _, wantLog := range test.wantLogSnippets {
 				require.Contains(t, logString, wantLog)
 			}
-			require.Equal(t, tt.wantResult, result)
-			require.Equal(t, tt.wantConditions, *tt.conditionsToUpdate)
+			require.Equal(t, test.wantResult, result)
+			require.Equal(t, test.wantConditions, *test.existingConditionsToUpdate)
+		})
+	}
+}
+
+func TestHadErrorCondition(t *testing.T) {
+	tests := []struct {
+		name       string
+		conditions []*metav1.Condition
+		wantResult bool
+	}{
+		{
+			name: "Returns false when all conditions have status true",
+			conditions: []*metav1.Condition{
+				{
+					Status: metav1.ConditionTrue,
+				},
+				{
+					Status: metav1.ConditionTrue,
+				},
+			},
+			wantResult: false,
+		},
+		{
+			name:       "Returns false when input is nil",
+			conditions: nil,
+			wantResult: false,
+		},
+		{
+			name:       "Returns false when input is empty",
+			conditions: []*metav1.Condition{},
+			wantResult: false,
+		},
+		{
+			name: "Returns true when any condition has status unknown",
+			conditions: []*metav1.Condition{
+				{
+					Status: metav1.ConditionTrue,
+				},
+				{
+					Status: metav1.ConditionUnknown,
+				},
+			},
+			wantResult: true,
+		},
+		{
+			name: "Returns true when any condition has status false",
+			conditions: []*metav1.Condition{
+				{
+					Status: metav1.ConditionTrue,
+				},
+				{
+					Status: metav1.ConditionFalse,
+				},
+			},
+			wantResult: true,
+		},
+		{
+			name: "Returns true when any condition has invalid status",
+			conditions: []*metav1.Condition{
+				{
+					Status: metav1.ConditionTrue,
+				},
+				{
+					Status: "not a valid status",
+				},
+			},
+			wantResult: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			actual := HadErrorCondition(test.conditions)
+			require.Equal(t, test.wantResult, actual)
 		})
 	}
 }

internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go

@@ -382,7 +382,13 @@ func (c *activeDirectoryWatcherController) updateStatus(ctx context.Context, ups
 	log := plog.WithValues("namespace", upstream.Namespace, "name", upstream.Name)
 	updated := upstream.DeepCopy()
 
-	hadErrorCondition := conditionsutil.MergeConditions(conditions, upstream.Generation, &updated.Status.Conditions, log, metav1.Now())
+	hadErrorCondition := conditionsutil.MergeConditions(
+		conditions,
+		&updated.Status.Conditions,
+		upstream.Generation,
+		metav1.Now(),
+		log,
+	)
 
 	updated.Status.Phase = idpv1alpha1.ActiveDirectoryPhaseReady
 	if hadErrorCondition {

internal/controller/supervisorconfig/federation_domain_watcher.go

@@ -829,8 +829,13 @@ func (c *federationDomainWatcherController) updateStatus(
 		})
 	}
 
-	_ = conditionsutil.MergeConditions(conditions,
-		federationDomain.Generation, &updated.Status.Conditions, plog.New().WithName(controllerName), metav1.NewTime(c.clock.Now()))
+	_ = conditionsutil.MergeConditions(
+		conditions,
+		&updated.Status.Conditions,
+		federationDomain.Generation,
+		metav1.NewTime(c.clock.Now()),
+		plog.New().WithName(controllerName),
+	)
 
 	if equality.Semantic.DeepEqual(federationDomain, updated) {
 		return nil

internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go

@@ -569,10 +569,10 @@ func (c *gitHubWatcherController) updateStatus(
 
 	hadErrorCondition := conditionsutil.MergeConditions(
 		conditions,
-		upstream.Generation,
 		&updated.Status.Conditions,
-		log,
+		upstream.Generation,
 		metav1.NewTime(c.clock.Now()),
+		log,
 	)
 
 	updated.Status.Phase = idpv1alpha1.GitHubPhaseReady

internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go

@@ -268,7 +268,13 @@ func (c *ldapWatcherController) updateStatus(ctx context.Context, upstream *idpv
 	log := plog.WithValues("namespace", upstream.Namespace, "name", upstream.Name)
 	updated := upstream.DeepCopy()
 
-	hadErrorCondition := conditionsutil.MergeConditions(conditions, upstream.Generation, &updated.Status.Conditions, log, metav1.Now())
+	hadErrorCondition := conditionsutil.MergeConditions(
+		conditions,
+		&updated.Status.Conditions,
+		upstream.Generation,
+		metav1.Now(),
+		log,
+	)
 
 	updated.Status.Phase = idpv1alpha1.LDAPPhaseReady
 	if hadErrorCondition {

internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go

@@ -133,8 +133,13 @@ func (c *oidcClientWatcherController) updateStatus(
 ) error {
 	updated := upstream.DeepCopy()
 
-	hadErrorCondition := conditionsutil.MergeConditions(conditions,
-		upstream.Generation, &updated.Status.Conditions, plog.New(), metav1.Now())
+	hadErrorCondition := conditionsutil.MergeConditions(
+		conditions,
+		&updated.Status.Conditions,
+		upstream.Generation,
+		metav1.Now(),
+		plog.New(),
+	)
 
 	updated.Status.Phase = supervisorconfigv1alpha1.OIDCClientPhaseReady
 	if hadErrorCondition {

internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go

@@ -460,7 +460,13 @@ func (c *oidcWatcherController) updateStatus(
 	log := c.log.WithValues("namespace", upstream.Namespace, "name", upstream.Name)
 	updated := upstream.DeepCopy()
 
-	hadErrorCondition := conditionsutil.MergeConditions(conditions, upstream.Generation, &updated.Status.Conditions, log, metav1.Now())
+	hadErrorCondition := conditionsutil.MergeConditions(
+		conditions,
+		&updated.Status.Conditions,
+		upstream.Generation,
+		metav1.Now(),
+		log,
+	)
 
 	updated.Status.Phase = idpv1alpha1.PhaseReady
 	if hadErrorCondition {