Merge pull request #31 from ecordell/unpause

pause: support unpausing object by removing the label

Author: ecordell

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"`

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

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)

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"`

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()

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)
+		})
+	}
+}