diff --git a/component/ensure_component_test.go b/component/ensure_component_test.go index 580ea4a..53db901 100644 --- a/component/ensure_component_test.go +++ b/component/ensure_component_test.go @@ -31,7 +31,7 @@ type MyObject struct { // this implements the conditions interface for MyObject, but note that // this is not supported by kube codegen at the moment (don't try to use // this in a real controller) - conditions.StatusWithConditions[MyObjectStatus] `json:"-"` + conditions.StatusWithConditions[*MyObjectStatus] `json:"-"` } type MyObjectStatus struct { ObservedGeneration int64 `json:"observedGeneration,omitempty" protobuf:"varint,3,opt,name=observedGeneration"` diff --git a/conditions/conditions.go b/conditions/conditions.go index b497344..0034e8b 100644 --- a/conditions/conditions.go +++ b/conditions/conditions.go @@ -16,11 +16,12 @@ type StatusConditions struct { } // GetStatusConditions returns a pointer to all status conditions. -func (c StatusConditions) GetStatusConditions() *[]metav1.Condition { +func (c *StatusConditions) GetStatusConditions() *[]metav1.Condition { return &c.Conditions } type HasConditions interface { + comparable GetStatusConditions() *[]metav1.Condition } @@ -30,13 +31,20 @@ type StatusWithConditions[S HasConditions] struct { } // GetStatusConditions returns all status conditions. -func (s StatusWithConditions[S]) GetStatusConditions() []metav1.Condition { - return *s.Status.GetStatusConditions() +func (s *StatusWithConditions[S]) GetStatusConditions() *[]metav1.Condition { + var zero S + if s.Status == zero { + return nil + } + return s.Status.GetStatusConditions() } // FindStatusCondition finds the conditionType in conditions. func (s StatusWithConditions[S]) FindStatusCondition(conditionType string) *metav1.Condition { - return meta.FindStatusCondition(s.GetStatusConditions(), conditionType) + if s.GetStatusConditions() == nil { + return nil + } + return meta.FindStatusCondition(*s.GetStatusConditions(), conditionType) } // SetStatusCondition sets the corresponding condition in conditions to newCondition. @@ -56,17 +64,26 @@ func (s StatusWithConditions[S]) RemoveStatusCondition(conditionType string) { // IsStatusConditionTrue returns true when the conditionType is present and set to `metav1.ConditionTrue` func (s StatusWithConditions[S]) IsStatusConditionTrue(conditionType string) bool { - return meta.IsStatusConditionTrue(s.GetStatusConditions(), conditionType) + if s.GetStatusConditions() == nil { + return false + } + return meta.IsStatusConditionTrue(*s.GetStatusConditions(), conditionType) } // IsStatusConditionFalse returns true when the conditionType is present and set to `metav1.ConditionFalse` func (s StatusWithConditions[S]) IsStatusConditionFalse(conditionType string) bool { - return meta.IsStatusConditionFalse(s.GetStatusConditions(), conditionType) + if s.GetStatusConditions() == nil { + return false + } + return meta.IsStatusConditionFalse(*s.GetStatusConditions(), conditionType) } // IsStatusConditionPresentAndEqual returns true when conditionType is present and equal to status. func (s StatusWithConditions[S]) IsStatusConditionPresentAndEqual(conditionType string, status metav1.ConditionStatus) bool { - return meta.IsStatusConditionPresentAndEqual(s.GetStatusConditions(), conditionType, status) + if s.GetStatusConditions() == nil { + return false + } + return meta.IsStatusConditionPresentAndEqual(*s.GetStatusConditions(), conditionType, status) } // IsStatusConditionChanged returns true if the passed in condition is different diff --git a/metrics/condition_metrics.go b/metrics/condition_metrics.go index 97b70db..98a5e18 100644 --- a/metrics/condition_metrics.go +++ b/metrics/condition_metrics.go @@ -103,7 +103,7 @@ func (c *ConditionStatusCollector[K]) CollectWithStability(ch chan<- metrics.Met objectsWithCondition := map[string]uint16{} for _, o := range objs { objectName := types.NamespacedName{Name: o.GetName(), Namespace: o.GetNamespace()}.String() - for _, condition := range o.GetStatusConditions() { + for _, condition := range *o.GetStatusConditions() { objectsWithCondition[condition.Type]++ timeInCondition := collectTime.Sub(condition.LastTransitionTime.Time) diff --git a/metrics/condition_metrics_test.go b/metrics/condition_metrics_test.go index debd058..2b6afdd 100644 --- a/metrics/condition_metrics_test.go +++ b/metrics/condition_metrics_test.go @@ -14,7 +14,7 @@ type MyObject struct { // this implements the conditions interface for MyObject, but note that // this is not supported by kube codegen at the moment (don't try to use // this in a real controller) - conditions.StatusWithConditions[MyObjectStatus] `json:"-"` + conditions.StatusWithConditions[*MyObjectStatus] `json:"-"` } type MyObjectStatus struct { ObservedGeneration int64 `json:"observedGeneration,omitempty" protobuf:"varint,3,opt,name=observedGeneration"` diff --git a/pause/pause.go b/pause/pause.go index f82f775..1fb040b 100644 --- a/pause/pause.go +++ b/pause/pause.go @@ -36,7 +36,8 @@ type HasStatusConditions interface { metav1.Object FindStatusCondition(conditionType string) *metav1.Condition SetStatusCondition(metav1.Condition) - GetStatusConditions() []metav1.Condition + RemoveStatusCondition(conditionType string) + GetStatusConditions() *[]metav1.Condition } func IsPaused(object metav1.Object, pausedLabelKey string) bool { @@ -80,17 +81,32 @@ func (p *Handler[K]) pause(ctx context.Context, object K) { object.SetStatusCondition(NewPausedCondition(p.PausedLabelKey)) object.SetManagedFields(nil) if err := p.PatchStatus(ctx, object); err != nil { - p.ctrls.MustValue(ctx).RequeueErr(err) + p.ctrls.MustValue(ctx).RequeueAPIErr(err) return } p.ctrls.MustValue(ctx).Done() } func (p *Handler[K]) Handle(ctx context.Context) { - if obj := p.Object.MustValue(ctx); IsPaused(obj, p.PausedLabelKey) { + obj := p.Object.MustValue(ctx) + if IsPaused(obj, p.PausedLabelKey) { p.pause(ctx, obj) return } + + // unpaused and no paused condition, continue + if obj.FindStatusCondition(ConditionTypePaused) == nil { + p.Next.Handle(ctx) + return + } + + // remove the paused condition + obj.RemoveStatusCondition(ConditionTypePaused) + obj.SetManagedFields(nil) + if err := p.PatchStatus(ctx, obj); err != nil { + p.ctrls.MustValue(ctx).RequeueAPIErr(err) + return + } p.Next.Handle(ctx) } @@ -135,7 +151,7 @@ func (p *SelfPauseHandler[K]) Handle(ctx context.Context) { object.SetLabels(labels) if err := p.Patch(ctx, object); err != nil { utilruntime.HandleError(err) - p.ctrls.MustValue(ctx).Requeue() + p.ctrls.MustValue(ctx).RequeueAPIErr(err) return } p.ctrls.MustValue(ctx).Done() diff --git a/pause/pause_test.go b/pause/pause_test.go index 0ec4d46..3220c69 100644 --- a/pause/pause_test.go +++ b/pause/pause_test.go @@ -2,12 +2,17 @@ package pause import ( "context" + "fmt" + "testing" + "github.com/stretchr/testify/require" + "golang.org/x/exp/slices" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/authzed/controller-idioms/conditions" "github.com/authzed/controller-idioms/handler" "github.com/authzed/controller-idioms/queue" + "github.com/authzed/controller-idioms/queue/fake" "github.com/authzed/controller-idioms/typedctx" ) @@ -18,7 +23,7 @@ type MyObject struct { // this implements the conditions interface for MyObject, but note that // this is not supported by kube codegen at the moment (don't try to use // this in a real controller) - conditions.StatusWithConditions[MyObjectStatus] `json:"-"` + conditions.StatusWithConditions[*MyObjectStatus] `json:"-"` } type MyObjectStatus struct { ObservedGeneration int64 `json:"observedGeneration,omitempty" protobuf:"varint,3,opt,name=observedGeneration"` @@ -47,3 +52,170 @@ func ExampleNewPauseContextHandler() { pauseHandler.Handle(ctx) // Output: } + +func TestPauseHandler(t *testing.T) { + var nextKey handler.Key = "next" + const PauseLabelKey = "com.my-controller/controller-paused" + tests := []struct { + name string + + obj *MyObject + patchError error + + expectNext handler.Key + expectEvents []string + expectPatchStatus bool + expectConditions []metav1.Condition + expectRequeue bool + expectDone bool + }{ + { + name: "pauses when label found", + obj: &MyObject{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + PauseLabelKey: "", + }, + }, + StatusWithConditions: conditions.StatusWithConditions[*MyObjectStatus]{ + Status: &MyObjectStatus{ + StatusConditions: conditions.StatusConditions{ + Conditions: []metav1.Condition{}, + }, + }, + }, + }, + expectPatchStatus: true, + expectConditions: []metav1.Condition{NewPausedCondition(PauseLabelKey)}, + expectDone: true, + }, + { + name: "requeues on pause patch error", + obj: &MyObject{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + PauseLabelKey: "", + }, + }, + StatusWithConditions: conditions.StatusWithConditions[*MyObjectStatus]{ + Status: &MyObjectStatus{ + StatusConditions: conditions.StatusConditions{ + Conditions: []metav1.Condition{}, + }, + }, + }, + }, + patchError: fmt.Errorf("error patching"), + expectPatchStatus: true, + expectRequeue: true, + }, + { + name: "no-op when label found and status is already paused", + obj: &MyObject{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + PauseLabelKey: "", + }, + }, + StatusWithConditions: conditions.StatusWithConditions[*MyObjectStatus]{ + Status: &MyObjectStatus{ + StatusConditions: conditions.StatusConditions{ + Conditions: []metav1.Condition{NewPausedCondition(PauseLabelKey)}, + }, + }, + }, + }, + expectDone: true, + }, + { + name: "removes condition when label is removed", + obj: &MyObject{ + StatusWithConditions: conditions.StatusWithConditions[*MyObjectStatus]{ + Status: &MyObjectStatus{ + StatusConditions: conditions.StatusConditions{ + Conditions: []metav1.Condition{NewPausedCondition(PauseLabelKey)}, + }, + }, + }, + }, + expectPatchStatus: true, + expectConditions: []metav1.Condition{}, + expectNext: nextKey, + }, + { + name: "removes self-pause condition when label is removed", + obj: &MyObject{ + StatusWithConditions: conditions.StatusWithConditions[*MyObjectStatus]{ + Status: &MyObjectStatus{ + StatusConditions: conditions.StatusConditions{ + Conditions: []metav1.Condition{NewSelfPausedCondition(PauseLabelKey)}, + }, + }, + }, + }, + expectPatchStatus: true, + expectConditions: []metav1.Condition{}, + expectNext: nextKey, + }, + { + name: "requeues on unpause patch error", + obj: &MyObject{ + StatusWithConditions: conditions.StatusWithConditions[*MyObjectStatus]{ + Status: &MyObjectStatus{ + StatusConditions: conditions.StatusConditions{ + Conditions: []metav1.Condition{NewPausedCondition(PauseLabelKey)}, + }, + }, + }, + }, + patchError: fmt.Errorf("error patching"), + expectPatchStatus: true, + expectRequeue: true, + }, + { + name: "no-op, no pause label, no pause status", + obj: &MyObject{}, + expectNext: nextKey, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrls := &fake.FakeInterface{} + patchCalled := false + + patchStatus := func(ctx context.Context, patch *MyObject) error { + patchCalled = true + + if tt.patchError != nil { + return tt.patchError + } + + require.Truef(t, slices.EqualFunc(tt.expectConditions, patch.Status.Conditions, func(a, b metav1.Condition) bool { + return a.Type == b.Type && + a.Status == b.Status && + a.ObservedGeneration == b.ObservedGeneration && + a.Message == b.Message && + a.Reason == b.Reason + }), "conditions not equal:\na: %#v\nb: %#v", tt.expectConditions, patch.Status.Conditions) + + return nil + } + queueOps := queue.NewQueueOperationsCtx() + ctxMyObject := typedctx.WithDefault[*MyObject](nil) + + ctx := context.Background() + ctx = queueOps.WithValue(ctx, ctrls) + ctx = ctxMyObject.WithValue(ctx, tt.obj) + var called handler.Key + + NewPauseContextHandler(queueOps.Key, PauseLabelKey, ctxMyObject, patchStatus, handler.ContextHandlerFunc(func(ctx context.Context) { + called = nextKey + })).Handle(ctx) + + require.Equal(t, tt.expectPatchStatus, patchCalled) + require.Equal(t, tt.expectNext, called) + require.Equal(t, tt.expectRequeue, ctrls.RequeueAPIErrCallCount() == 1) + require.Equal(t, tt.expectDone, ctrls.DoneCallCount() == 1) + }) + } +}