diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go index 64ec4ffc1..a84ad931d 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go +++ b/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) { diff --git a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go b/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go index f80dad113..eacddfc4c 100644 --- a/internal/controller/authenticator/webhookcachefiller/webhookcachefiller.go +++ b/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) { diff --git a/internal/controller/conditionsutil/conditions_util.go b/internal/controller/conditionsutil/conditions_util.go index fc90c3986..ee2f2434f 100644 --- a/internal/controller/conditionsutil/conditions_util.go +++ b/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 + }) } diff --git a/internal/controller/conditionsutil/conditions_util_test.go b/internal/controller/conditionsutil/conditions_util_test.go index ceb4aee53..7c34a81b4 100644 --- a/internal/controller/conditionsutil/conditions_util_test.go +++ b/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) }) } } diff --git a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go b/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go index 0026126be..4511b8407 100644 --- a/internal/controller/supervisorconfig/activedirectoryupstreamwatcher/active_directory_upstream_watcher.go +++ b/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 { diff --git a/internal/controller/supervisorconfig/federation_domain_watcher.go b/internal/controller/supervisorconfig/federation_domain_watcher.go index 715b2b36f..8d60306c3 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher.go +++ b/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 diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go index 1f8236cac..e90c1fc66 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go +++ b/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 diff --git a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go b/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go index f97fa77a9..5c8c23b9f 100644 --- a/internal/controller/supervisorconfig/ldapupstreamwatcher/ldap_upstream_watcher.go +++ b/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 { diff --git a/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go b/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go index 4086f3532..a9573ba42 100644 --- a/internal/controller/supervisorconfig/oidcclientwatcher/oidc_client_watcher.go +++ b/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 { diff --git a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go b/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go index 3c01188e8..e6ab2f448 100644 --- a/internal/controller/supervisorconfig/oidcupstreamwatcher/oidc_upstream_watcher.go +++ b/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 {