Skip to content

Commit

Permalink
controller: PendingError isn't actually an error
Browse files Browse the repository at this point in the history
I think we confused ourselves with using an error to mean something
else, and we ended up logging an error with log.V(1).Error which gets
printed regardless of the logging level.

What we really wanted is to only show these messages in debug mode,
similarly as to how we log the "Succeeded signing certificate" messages.
That way, the logging experience is consistent: someone debugging the
logs will be able to see, in debug mode, the progression of the
signing of the certificate as well as when it succeeded.

I also propose to change the message associated with the PendingError
case. It isn't a failure since PendingError isn't an error.

Signed-off-by: Maël Valais <[email protected]>
  • Loading branch information
maelvls committed Sep 11, 2023
1 parent 7755573 commit f8cdd78
Showing 1 changed file with 10 additions and 4 deletions.
14 changes: 10 additions & 4 deletions controllers/certificaterequest_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch(
isPermanentError := errors.As(err, &signer.PermanentError{})
pastMaxRetryDuration := r.Clock.Now().After(cr.CreationTimestamp.Add(r.MaxRetryDuration))
if !isPendingError && (isPermanentError || pastMaxRetryDuration) {
// fail permanently
// Fail permanently.
logger.V(1).Error(err, "Permanent CertificateRequest error. Marking as failed.")
_, failedAt := conditions.SetCertificateRequestStatusCondition(
r.Clock,
Expand All @@ -348,16 +348,22 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch(
r.EventRecorder.Eventf(&cr, corev1.EventTypeWarning, "PermanentError", "Failed permanently to sign CertificateRequest: %s", err)
return result, crStatusPatch, reconcile.TerminalError(err) // done, apply patch
} else {
// retry
logger.V(1).Error(err, "Retryable CertificateRequest error.")
// Signing is pending, wait more.
//
// The PendingError has a misleading name: although it is an error,
// it isn't an error. It just means that we should poll again later.
// Its message gives the reason why the signing process is still in
// progress. Thus, we don't log any error.
reason := err.Error()
logger.V(1).WithValues("reason", reason).Info("Signing of CertificateRequest is in still in progress. ")
conditions.SetCertificateRequestStatusCondition(
r.Clock,
cr.Status.Conditions,
&crStatusPatch.Conditions,
cmapi.CertificateRequestConditionReady,
cmmeta.ConditionFalse,
cmapi.CertificateRequestReasonPending,
fmt.Sprintf("Failed to sign CertificateRequest, will retry: %s", err),
fmt.Sprintf("Signing of CertificateRequest is still in progress, will retry: %s", err),
)

r.EventRecorder.Eventf(&cr, corev1.EventTypeWarning, "RetryableError", "Failed to sign CertificateRequest, will retry: %s", err)
Expand Down

0 comments on commit f8cdd78

Please sign in to comment.