diff --git a/controllers/certificaterequest_controller_integration_test.go b/controllers/certificaterequest_controller_integration_test.go index 78fde78..a28558a 100644 --- a/controllers/certificaterequest_controller_integration_test.go +++ b/controllers/certificaterequest_controller_integration_test.go @@ -421,7 +421,7 @@ func markIssuerReady(t *testing.T, ctx context.Context, kc client.Client, clock cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, - "checked", + "Succeeded checking the issuer", ) err := kubeutil.SetGroupVersionKind(kc.Scheme(), issuer) diff --git a/controllers/certificaterequest_controller_test.go b/controllers/certificaterequest_controller_test.go index 574293b..7e1b30c 100644 --- a/controllers/certificaterequest_controller_test.go +++ b/controllers/certificaterequest_controller_test.go @@ -82,7 +82,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, - "checked", + "Succeeded checking the issuer", ), ) @@ -94,7 +94,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, - "checked", + "Succeeded checking the issuer", ), ) diff --git a/controllers/certificatesigningrequest_controller_test.go b/controllers/certificatesigningrequest_controller_test.go index 394be41..446b37f 100644 --- a/controllers/certificatesigningrequest_controller_test.go +++ b/controllers/certificatesigningrequest_controller_test.go @@ -84,7 +84,7 @@ func TestCertificateSigningRequestReconcilerReconcile(t *testing.T) { cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, - "checked", + "Succeeded checking the issuer", ), ) @@ -96,7 +96,7 @@ func TestCertificateSigningRequestReconcilerReconcile(t *testing.T) { cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, - "checked", + "Succeeded checking the issuer", ), ) diff --git a/controllers/combined_controller_integration_test.go b/controllers/combined_controller_integration_test.go index 0072795..1325d4e 100644 --- a/controllers/combined_controller_integration_test.go +++ b/controllers/combined_controller_integration_test.go @@ -150,7 +150,7 @@ func TestCombinedControllerTemporaryFailedCertificateRequestRetrigger(t *testing cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, - "checked", + "Succeeded checking the issuer", ), ) @@ -177,7 +177,7 @@ func TestCombinedControllerTemporaryFailedCertificateRequestRetrigger(t *testing (readyCondition.ObservedGeneration != issuer.Generation) || (readyCondition.Status != cmmeta.ConditionTrue) || (readyCondition.Reason != v1alpha1.IssuerConditionReasonChecked) || - (readyCondition.Message != "checked") { + (readyCondition.Message != "Succeeded checking the issuer") { return fmt.Errorf("incorrect ready condition: %v", readyCondition) } @@ -250,7 +250,7 @@ func TestCombinedControllerTemporaryFailedCertificateRequestRetrigger(t *testing (readyCondition.ObservedGeneration != issuer.Generation) || (readyCondition.Status != cmmeta.ConditionTrue) || (readyCondition.Reason != v1alpha1.IssuerConditionReasonChecked) || - (readyCondition.Message != "checked") { + (readyCondition.Message != "Succeeded checking the issuer") { return fmt.Errorf("incorrect ready condition: %v", readyCondition) } diff --git a/controllers/issuer_controller.go b/controllers/issuer_controller.go index e0c3a07..d0eec4d 100644 --- a/controllers/issuer_controller.go +++ b/controllers/issuer_controller.go @@ -37,6 +37,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/controller" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" + "sigs.k8s.io/controller-runtime/pkg/reconcile" v1alpha1 "github.com/cert-manager/issuer-lib/api/v1alpha1" "github.com/cert-manager/issuer-lib/conditions" @@ -45,6 +46,12 @@ import ( "github.com/cert-manager/issuer-lib/internal/ssaclient" ) +const ( + eventIssuerChecked = "Checked" + eventIssuerRetryableError = "RetryableError" + eventIssuerPermanentError = "PermanentError" +) + // IssuerReconciler reconciles a SimpleIssuer object type IssuerReconciler struct { ForObject v1alpha1.Issuer @@ -74,7 +81,10 @@ func (r *IssuerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res logger.V(2).Info("Starting reconcile loop", "name", req.Name, "namespace", req.Namespace) + // The error returned by `reconcileStatusPatch` is meant for controller-runtime, + // not for us. That's why we aren't checking `returnedError != nil` . result, issuerStatusPatch, returnedError := r.reconcileStatusPatch(logger, ctx, req) + logger.V(2).Info("Got StatusPatch result", "result", result, "patch", issuerStatusPatch, "error", returnedError) if issuerStatusPatch != nil { cr, patch, err := ssaclient.GenerateIssuerStatusPatch(r.ForObject, req.Name, req.Namespace, issuerStatusPatch) @@ -88,9 +98,10 @@ func (r *IssuerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res Force: ptr.To(true), }, }); err != nil { - if err := client.IgnoreNotFound(err); err != nil { + if !apierrors.IsNotFound(err) { return ctrl.Result{}, utilerrors.NewAggregate([]error{err, returnedError}) } + logger.V(1).Info("Not found. Ignoring.") } } @@ -98,11 +109,19 @@ func (r *IssuerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (res return result, returnedError } +// reconcileStatusPatch is responsible for reconciling the issuer. It will return the +// result and reconcileError to be returned by the Reconcile function. It also returns +// an issuerStatusPatch that the Reconcile function will apply to the issuer's status. +// This function is split out from the Reconcile function to allow for easier testing. +// +// The error returned by `reconcileStatusPatch` is meant for controller-runtime, +// not for the caller. The caller must not check the error (i.e., they must not +// do `if err != nil...`). func (r *IssuerReconciler) reconcileStatusPatch( logger logr.Logger, ctx context.Context, req ctrl.Request, -) (result ctrl.Result, issuerStatusPatch *v1alpha1.IssuerStatus, returnedError error) { +) (result ctrl.Result, issuerStatusPatch *v1alpha1.IssuerStatus, reconcileError error) { // Get the ClusterIssuer issuer := r.ForObject.DeepCopyObject().(v1alpha1.Issuer) forObjectGvk := r.ForObject.GetObjectKind().GroupVersionKind() @@ -110,10 +129,10 @@ func (r *IssuerReconciler) reconcileStatusPatch( reportedError := r.EventSource.HasReportedError(forObjectGvk, req.NamespacedName) if err := r.Client.Get(ctx, req.NamespacedName, issuer); err != nil && apierrors.IsNotFound(err) { - logger.V(1).Info("Not found. Ignoring.") + logger.V(1).Info("Issuer not found. Ignoring.") return result, nil, nil // done } else if err != nil { - return result, nil, fmt.Errorf("unexpected get error: %v", err) // retry + return result, nil, fmt.Errorf("unexpected get error: %v", err) // requeue with backoff } readyCondition := conditions.GetIssuerStatusCondition(issuer.GetStatus().Conditions, cmapi.IssuerConditionReady) @@ -124,17 +143,17 @@ func (r *IssuerReconciler) reconcileStatusPatch( (readyCondition.Reason == v1alpha1.IssuerConditionReasonFailed) && (readyCondition.ObservedGeneration >= issuer.GetGeneration()) if isFailed { - logger.V(1).Info("Issuer is Failed. Ignoring.") + logger.V(1).Info("Issuer is Failed Permanently. Ignoring.") return result, nil, nil // done } if r.IgnoreIssuer != nil { ignore, err := r.IgnoreIssuer(ctx, issuer) if err != nil { - return result, nil, fmt.Errorf("failed to check if issuer should be ignored: %v", err) // retry + return result, nil, fmt.Errorf("failed to check if issuer should be ignored: %v", err) // requeue with backoff } if ignore { - logger.V(1).Info("Ignoring issuer") + logger.V(1).Info("IgnoreIssuer() returned true. Ignoring.") return result, nil, nil // done } } @@ -143,15 +162,27 @@ func (r *IssuerReconciler) reconcileStatusPatch( // for updating its Status. issuerStatusPatch = &v1alpha1.IssuerStatus{} - // Add a Ready condition if one does not already exist. Set initial Status - // to Unknown. - if readyCondition == nil { - logger.V(1).Info("Initializing Ready condition") - conditions.SetIssuerStatusCondition( + setCondition := func( + conditionType cmapi.IssuerConditionType, + status cmmeta.ConditionStatus, + reason, message string, + ) string { + condition, _ := conditions.SetIssuerStatusCondition( r.Clock, issuer.GetStatus().Conditions, &issuerStatusPatch.Conditions, issuer.GetGeneration(), + conditionType, status, + reason, message, + ) + return condition.Message + } + + // Add a Ready condition if one does not already exist. Set initial Status + // to Unknown. + if readyCondition == nil { + logger.V(1).Info("Initializing Ready condition") + setCondition( cmapi.IssuerConditionReady, cmmeta.ConditionUnknown, v1alpha1.IssuerConditionReasonInitializing, @@ -171,64 +202,43 @@ func (r *IssuerReconciler) reconcileStatusPatch( } else { err = r.Check(log.IntoContext(ctx, logger), issuer) } - if err != nil { - isPermanentError := errors.As(err, &signer.PermanentError{}) - if isPermanentError { - // fail permanently - logger.V(1).Error(err, "Permanent Issuer error. Marking as failed.") - conditions.SetIssuerStatusCondition( - r.Clock, - issuer.GetStatus().Conditions, - &issuerStatusPatch.Conditions, - issuer.GetGeneration(), - cmapi.IssuerConditionReady, - cmmeta.ConditionFalse, - v1alpha1.IssuerConditionReasonFailed, - fmt.Sprintf("Issuer has failed permanently: %s", err), - ) - r.EventRecorder.Eventf(issuer, corev1.EventTypeWarning, "PermanentError", "Failed permanently to check issuer: %s", err) - return result, issuerStatusPatch, nil // apply patch, retry - } else { - // retry - logger.V(1).Error(err, "Retryable Issuer error.") - conditions.SetIssuerStatusCondition( - r.Clock, - issuer.GetStatus().Conditions, - &issuerStatusPatch.Conditions, - issuer.GetGeneration(), - cmapi.IssuerConditionReady, - cmmeta.ConditionFalse, - v1alpha1.IssuerConditionReasonPending, - fmt.Sprintf("Issuer is not ready yet: %s", err), - ) - r.EventRecorder.Eventf(issuer, corev1.EventTypeWarning, "RetryableError", "Failed to check issuer, will retry: %s", err) - // We trigger a reconciliation here. Controller-runtime will use exponential backoff to requeue - // the request. We don't return an error here because we don't want controller-runtime to log an - // additional error message and we want the metrics to show a requeue instead of an error to be - // consistent with the other cases (see Permanent error above). - // - // Important: This means that the ReconcileErrors metric will only be incremented in case of a - // apiserver failure (see "unexpected get error" above). The ReconcileTotal labelRequeue metric - // can be used instead to get some estimate of the number of requeues. - result.Requeue = true - return result, issuerStatusPatch, nil // apply patch, retry - } + if err == nil { + logger.V(1).Info("Successfully finished the reconciliation.") + message := setCondition( + cmapi.IssuerConditionReady, + cmmeta.ConditionTrue, + v1alpha1.IssuerConditionReasonChecked, + "Succeeded checking the issuer", + ) + r.EventRecorder.Event(issuer, corev1.EventTypeNormal, eventIssuerChecked, message) + + return result, issuerStatusPatch, nil // apply patch, done } - conditions.SetIssuerStatusCondition( - r.Clock, - issuer.GetStatus().Conditions, - &issuerStatusPatch.Conditions, - issuer.GetGeneration(), - cmapi.IssuerConditionReady, - cmmeta.ConditionTrue, - v1alpha1.IssuerConditionReasonChecked, - "checked", - ) - - logger.V(1).Info("Successfully finished the reconciliation.") - r.EventRecorder.Eventf(issuer, corev1.EventTypeNormal, "Checked", "Succeeded checking the issuer") - return result, issuerStatusPatch, nil // done, apply patch + isPermanentError := errors.As(err, &signer.PermanentError{}) + if isPermanentError { + // fail permanently + logger.V(1).Error(err, "Permanent Issuer error. Marking as failed.") + message := setCondition( + cmapi.IssuerConditionReady, + cmmeta.ConditionFalse, + v1alpha1.IssuerConditionReasonFailed, + fmt.Sprintf("Issuer has failed permanently: %s", err), + ) + r.EventRecorder.Event(issuer, corev1.EventTypeWarning, eventIssuerPermanentError, message) + return result, issuerStatusPatch, reconcile.TerminalError(err) // apply patch, done + } else { + // retry + logger.V(1).Error(err, "Retryable Issuer error.") + message := setCondition( + cmapi.IssuerConditionReady, + cmmeta.ConditionFalse, + v1alpha1.IssuerConditionReasonPending, + fmt.Sprintf("Issuer is not ready yet: %s", err), + ) + r.EventRecorder.Event(issuer, corev1.EventTypeWarning, eventIssuerRetryableError, message) + return result, issuerStatusPatch, err // apply patch, requeue with backoff + } } // SetupWithManager sets up the controller with the Manager. diff --git a/controllers/issuer_controller_test.go b/controllers/issuer_controller_test.go index 5f78da7..ead4ee9 100644 --- a/controllers/issuer_controller_test.go +++ b/controllers/issuer_controller_test.go @@ -119,7 +119,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, - "checked", + "Succeeded checking the issuer", ), ), }, @@ -129,7 +129,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { Type: cmapi.IssuerConditionReady, Status: cmmeta.ConditionTrue, Reason: v1alpha1.IssuerConditionReasonChecked, - Message: "checked", + Message: "Succeeded checking the issuer", ObservedGeneration: 80, LastTransitionTime: &fakeTimeObj1, // since the status is not updated, the LastTransitionTime is not updated either }, @@ -191,7 +191,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, - "checked", + "Succeeded checking the issuer", ), ), }, @@ -208,14 +208,9 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { }, }, }, - // instead of an error, a new reconcile request is triggered by the - // requeue=true return value - validateError: errormatch.NoError(), - expectedResult: reconcile.Result{ - Requeue: true, - }, + validateError: errormatch.ErrorContains("[specific error]"), expectedEvents: []string{ - "Warning RetryableError Failed to check issuer, will retry: [specific error]", + "Warning RetryableError Issuer is not ready yet: [specific error]", }, }, @@ -231,7 +226,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, - "checked", + "Succeeded checking the issuer", ), testutil.SetSimpleIssuerGeneration(81), ), @@ -242,7 +237,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { Type: cmapi.IssuerConditionReady, Status: cmmeta.ConditionTrue, Reason: v1alpha1.IssuerConditionReasonChecked, - Message: "checked", + Message: "Succeeded checking the issuer", LastTransitionTime: &fakeTimeObj1, // since the status is not updated, the LastTransitionTime is not updated either ObservedGeneration: 81, }, @@ -259,7 +254,6 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { objects: []client.Object{ issuer1, }, - expectedResult: reconcile.Result{}, expectedStatusPatch: &v1alpha1.IssuerStatus{ Conditions: []cmapi.IssuerCondition{ { @@ -276,7 +270,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { // Retry if the check function returns an error { name: "retry-on-error", - check: staticChecker(fmt.Errorf("a specific error")), + check: staticChecker(fmt.Errorf("[specific error]")), objects: []client.Object{ testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerStatusCondition( @@ -288,25 +282,52 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { ), ), }, - // instead of an error, a new reconcile request is triggered by the - // requeue=true return value - validateError: errormatch.NoError(), - expectedResult: reconcile.Result{ - Requeue: true, - }, expectedStatusPatch: &v1alpha1.IssuerStatus{ Conditions: []cmapi.IssuerCondition{ { Type: cmapi.IssuerConditionReady, Status: cmmeta.ConditionFalse, Reason: v1alpha1.IssuerConditionReasonPending, - Message: "Issuer is not ready yet: a specific error", + Message: "Issuer is not ready yet: [specific error]", + LastTransitionTime: &fakeTimeObj2, + }, + }, + }, + validateError: errormatch.ErrorContains("[specific error]"), + expectedEvents: []string{ + "Warning RetryableError Issuer is not ready yet: [specific error]", + }, + }, + + // Don't retry if the check function returns a permanent error + { + name: "dont-retry-on-permanent-error", + check: staticChecker(signer.PermanentError{Err: fmt.Errorf("[specific error]")}), + objects: []client.Object{ + testutil.SimpleIssuerFrom(issuer1, + testutil.SetSimpleIssuerStatusCondition( + fakeClock1, + cmapi.IssuerConditionReady, + cmmeta.ConditionUnknown, + v1alpha1.IssuerConditionReasonInitializing, + fieldOwner+" has started reconciling this Issuer", + ), + ), + }, + expectedStatusPatch: &v1alpha1.IssuerStatus{ + Conditions: []cmapi.IssuerCondition{ + { + Type: cmapi.IssuerConditionReady, + Status: cmmeta.ConditionFalse, + Reason: v1alpha1.IssuerConditionReasonFailed, + Message: "Issuer has failed permanently: [specific error]", LastTransitionTime: &fakeTimeObj2, }, }, }, + validateError: errormatch.ErrorContains("terminal error: [specific error]"), expectedEvents: []string{ - "Warning RetryableError Failed to check issuer, will retry: a specific error", + "Warning PermanentError Issuer has failed permanently: [specific error]", }, }, @@ -334,7 +355,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { Type: cmapi.IssuerConditionReady, Status: cmmeta.ConditionTrue, Reason: v1alpha1.IssuerConditionReasonChecked, - Message: "checked", + Message: "Succeeded checking the issuer", LastTransitionTime: &fakeTimeObj2, }, }, @@ -367,7 +388,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { Type: cmapi.IssuerConditionReady, Status: cmmeta.ConditionTrue, Reason: v1alpha1.IssuerConditionReasonChecked, - Message: "checked", + Message: "Succeeded checking the issuer", LastTransitionTime: &fakeTimeObj2, ObservedGeneration: 81, }, @@ -417,11 +438,11 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { Clock: fakeClock2, } - res, crsPatch, err := controller.reconcileStatusPatch(logger, context.TODO(), req) + res, issuerStatusPatch, reconcileErr := controller.reconcileStatusPatch(logger, context.TODO(), req) assert.Equal(t, tc.expectedResult, res) - assert.Equal(t, tc.expectedStatusPatch, crsPatch) - ptr.Deref(tc.validateError, *errormatch.NoError())(t, err) + assert.Equal(t, tc.expectedStatusPatch, issuerStatusPatch) + ptr.Deref(tc.validateError, *errormatch.NoError())(t, reconcileErr) allEvents := chanToSlice(fakeRecorder.Events) if len(tc.expectedEvents) == 0 {