From 81f232aa9d58d9e4986b408e2b4c82f2a1beb4f5 Mon Sep 17 00:00:00 2001 From: Danail Branekov Date: Wed, 2 Oct 2024 13:32:52 +0000 Subject: [PATCH] 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 | 52 -------------------------- tools/k8s/reconcile.go | 65 ++++++++++++++++++++++++++++++++- tools/k8s/reconcile_test.go | 67 +++++++++++++++++++++++++++++++--- 3 files changed, 125 insertions(+), 59 deletions(-) diff --git a/tools/k8s/condition_builder.go b/tools/k8s/condition_builder.go index bd15dfbff..cf957331b 100644 --- a/tools/k8s/condition_builder.go +++ b/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 -} diff --git a/tools/k8s/reconcile.go b/tools/k8s/reconcile.go index c7fb6d43b..b30154e84 100644 --- a/tools/k8s/reconcile.go +++ b/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, ¬ReadyErr) { - 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 +} diff --git a/tools/k8s/reconcile_test.go b/tools/k8s/reconcile_test.go index 5bfcaed6c..dccf79d9d 100644 --- a/tools/k8s/reconcile_test.go +++ b/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")), ))) })