Patching reconciler ensures non-empty status reason

Status conditions must have non-empty reason, otherwise the patch
operation is rejected by kubernetes.

When constructing the `NotReadyError`, a reconciler might set the cause
but not be able to determine the reason. Instead of trying to adhere to
always setting the reason in the controllers, the patching reconciler
now ensures that it is defaulted to `Unknown`.

While being here, move the `NotReadyError` type into the `reconcile.go`
file - it logically makes sense to be there

tools/k8s/condition_builder.go

@@ -1,11 +1,9 @@
 package k8s
 
 import (
- "fmt"
  "time"
 
  korifiv1alpha1 "code.cloudfoundry.org/korifi/controllers/api/v1alpha1"
- "code.cloudfoundry.org/korifi/tools"
  metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
  "sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -72,53 +70,3 @@ func (b *ReadyConditionBuilder) Build() metav1.Condition {
 
  return result
 }
-
-type NotReadyError struct {
- cause error
- reason string
- message string
- requeueAfter *time.Duration
- requeue bool
- noRequeue bool
-}
-
-func (e NotReadyError) Error() string {
- if e.cause != nil {
- return fmt.Sprintf("%s: %s", e.message, e.cause.Error())
- }
- return e.message
-}
-
-func NewNotReadyError() NotReadyError {
- return NotReadyError{}
-}
-
-func (e NotReadyError) WithCause(cause error) NotReadyError {
- e.cause = cause
- return e
-}
-
-func (e NotReadyError) WithRequeue() NotReadyError {
- e.requeue = true
- return e
-}
-
-func (e NotReadyError) WithRequeueAfter(duration time.Duration) NotReadyError {
- e.requeueAfter = tools.PtrTo(duration)
- return e
-}
-
-func (e NotReadyError) WithNoRequeue() NotReadyError {
- e.noRequeue = true
- return e
-}
-
-func (e NotReadyError) WithReason(reason string) NotReadyError {
- e.reason = reason
- return e
-}
-
-func (e NotReadyError) WithMessage(message string) NotReadyError {
- e.message = message
- return e
-}

tools/k8s/reconcile.go

@@ -5,7 +5,9 @@ import (
  "errors"
  "fmt"
  "reflect"
+ "time"
 
+ "code.cloudfoundry.org/korifi/tools"
  "github.com/go-logr/logr"
  "github.com/google/uuid"
  k8serrors "k8s.io/apimachinery/pkg/api/errors"
@@ -75,7 +77,12 @@ func (r *PatchingReconciler[T, PT]) Reconcile(ctx context.Context, req ctrl.Requ
 
  var notReadyErr NotReadyError
  if errors.As(delegateErr, &notReadyErr) {
- readyConditionBuilder.WithReason(notReadyErr.reason).WithMessage(notReadyErr.message)
+ reason := notReadyErr.reason
+ if reason == "" {
+ reason = "Unknown"
+ }
+
+ readyConditionBuilder.WithReason(reason).WithMessage(notReadyErr.message)
 
  if notReadyErr.noRequeue {
  result = ctrl.Result{}
@@ -104,3 +111,59 @@ func (r *PatchingReconciler[T, PT]) Reconcile(ctx context.Context, req ctrl.Requ
 func (r *PatchingReconciler[T, PT]) SetupWithManager(mgr ctrl.Manager) error {
  return r.objectReconciler.SetupWithManager(mgr).Complete(r)
 }
+
+type NotReadyError struct {
+ cause error
+ reason string
+ message string
+ requeueAfter *time.Duration
+ requeue bool
+ noRequeue bool
+}
+
+func (e NotReadyError) Error() string {
+ if e.cause == nil {
+ return e.message
+ }
+
+ message := e.message
+ if message != "" {
+ message = message + ": "
+ }
+
+ return fmt.Sprintf("%s%s", message, e.cause.Error())
+}
+
+func NewNotReadyError() NotReadyError {
+ return NotReadyError{}
+}
+
+func (e NotReadyError) WithCause(cause error) NotReadyError {
+ e.cause = cause
+ return e
+}
+
+func (e NotReadyError) WithRequeue() NotReadyError {
+ e.requeue = true
+ return e
+}
+
+func (e NotReadyError) WithRequeueAfter(duration time.Duration) NotReadyError {
+ e.requeueAfter = tools.PtrTo(duration)
+ return e
+}
+
+func (e NotReadyError) WithNoRequeue() NotReadyError {
+ e.noRequeue = true
+ return e
+}
+
+func (e NotReadyError) WithReason(reason string) NotReadyError {
+ e.reason = reason
+ return e
+}
+
+func (e NotReadyError) WithMessage(message string) NotReadyError {
+ e.message = message
+ return e
+}

tools/k8s/reconcile_test.go

@@ -222,10 +222,10 @@ var _ = Describe("Reconcile", func() {
 
  When("the object reconciliation fails with NotReady error", func() {
  BeforeEach(func() {
- objectReconciler.reconcileResourceError = k8s.NewNotReadyError().WithReason("TestReason").WithMessage("TestMessage")
+ objectReconciler.reconcileResourceError = k8s.NewNotReadyError()
  })
 
- It("sets the ready condition to false with the reason specified", func() {
+ It("sets the ready condition to false with unknown reason", func() {
  Expect(fakeStatusWriter.PatchCallCount()).To(Equal(1))
  _, updatedObject, _, _ := fakeStatusWriter.PatchArgsForCall(0)
  updatedOrg, ok := updatedObject.(*korifiv1alpha1.CFOrg)
@@ -234,16 +234,72 @@ var _ = Describe("Reconcile", func() {
  Expect(updatedOrg.Status.Conditions).To(ContainElement(SatisfyAll(
  HasType(Equal(korifiv1alpha1.StatusConditionReady)),
  HasStatus(Equal(metav1.ConditionFalse)),
- HasReason(Equal("TestReason")),
- HasMessage(Equal("TestMessage")),
+ HasReason(Equal("Unknown")),
+ HasMessage(BeEmpty()),
  )))
  })
 
+ When("reason is specified", func() {
+ BeforeEach(func() {
+ objectReconciler.reconcileResourceError = k8s.NewNotReadyError().WithReason("TestReason")
+ })
+
+ It("sets the ready condition to false with the reason specified", func() {
+ Expect(fakeStatusWriter.PatchCallCount()).To(Equal(1))
+ _, updatedObject, _, _ := fakeStatusWriter.PatchArgsForCall(0)
+ updatedOrg, ok := updatedObject.(*korifiv1alpha1.CFOrg)
+ Expect(ok).To(BeTrue())
+
+ Expect(updatedOrg.Status.Conditions).To(ContainElement(SatisfyAll(
+ HasType(Equal(korifiv1alpha1.StatusConditionReady)),
+ HasStatus(Equal(metav1.ConditionFalse)),
+ HasReason(Equal("TestReason")),
+ )))
+ })
+ })
+
+ When("message is specified", func() {
+ BeforeEach(func() {
+ objectReconciler.reconcileResourceError = k8s.NewNotReadyError().WithMessage("TestMessage")
+ })
+
+ It("sets the ready condition to false with the message specified", func() {
+ Expect(fakeStatusWriter.PatchCallCount()).To(Equal(1))
+ _, updatedObject, _, _ := fakeStatusWriter.PatchArgsForCall(0)
+ updatedOrg, ok := updatedObject.(*korifiv1alpha1.CFOrg)
+ Expect(ok).To(BeTrue())
+
+ Expect(updatedOrg.Status.Conditions).To(ContainElement(SatisfyAll(
+ HasType(Equal(korifiv1alpha1.StatusConditionReady)),
+ HasStatus(Equal(metav1.ConditionFalse)),
+ HasMessage(Equal("TestMessage")),
+ )))
+ })
+ })
+
  When("cause is specified", func() {
+ BeforeEach(func() {
+ objectReconciler.reconcileResourceError = k8s.NewNotReadyError().WithCause(errors.New("test-err"))
+ })
+
+ It("sets the ready condition to false with the error as message", func() {
+ Expect(fakeStatusWriter.PatchCallCount()).To(Equal(1))
+ _, updatedObject, _, _ := fakeStatusWriter.PatchArgsForCall(0)
+ updatedOrg, ok := updatedObject.(*korifiv1alpha1.CFOrg)
+ Expect(ok).To(BeTrue())
+
+ Expect(updatedOrg.Status.Conditions).To(ContainElement(SatisfyAll(
+ HasType(Equal(korifiv1alpha1.StatusConditionReady)),
+ HasStatus(Equal(metav1.ConditionFalse)),
+ HasMessage(Equal("test-err")),
+ )))
+ })
+ })
+
+ When("cause and message are specified", func() {
  BeforeEach(func() {
  objectReconciler.reconcileResourceError = k8s.NewNotReadyError().
  WithCause(errors.New("test-err")).
- WithReason("TestReason").
  WithMessage("TestMessage")
  })
 
@@ -256,7 +312,6 @@ var _ = Describe("Reconcile", func() {
  Expect(updatedOrg.Status.Conditions).To(ContainElement(SatisfyAll(
  HasType(Equal(korifiv1alpha1.StatusConditionReady)),
  HasStatus(Equal(metav1.ConditionFalse)),
- HasReason(Equal("TestReason")),
  HasMessage(Equal("TestMessage: test-err")),
  )))
  })