diff --git a/controllers/certificaterequest_controller_test.go b/controllers/certificaterequest_controller_test.go index fc463fe..ef9c220 100644 --- a/controllers/certificaterequest_controller_test.go +++ b/controllers/certificaterequest_controller_test.go @@ -426,12 +426,12 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { }, }, - // If the sign function returns a Pending error, set the Ready condition to Pending (even if + // If the sign function returns a reason for being pending, set the Ready condition to Pending (even if // the MaxRetryDuration has been exceeded). { name: "retry-on-pending-error", sign: func(_ context.Context, cr signer.CertificateRequestObject, _ v1alpha1.Issuer) (signer.PEMBundle, error) { - return signer.PEMBundle{}, signer.PendingError{Err: fmt.Errorf("pending error")} + return signer.PEMBundle{}, signer.PendingError{Err: fmt.Errorf("reason for being pending")} }, objects: []client.Object{ cmgen.CertificateRequestFrom(cr1, @@ -451,14 +451,14 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, - Message: "Failed to sign CertificateRequest, will retry: pending error", + Message: "Signing of CertificateRequest still in progress: reason for being pending", LastTransitionTime: &fakeTimeObj2, }, }, }, - validateError: errormatch.ErrorContains("pending error"), + validateError: errormatch.ErrorContains("reason for being pending"), expectedEvents: []string{ - "Warning RetryableError Failed to sign CertificateRequest, will retry: pending error", + "Warning Pending Signing of CertificateRequest still in progress: reason for being pending", }, }, @@ -496,21 +496,21 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Type: "[condition type]", Status: cmmeta.ConditionTrue, Reason: "[reason]", - Message: "test error", + Message: "condition message", LastTransitionTime: &fakeTimeObj2, }, { Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, - Message: "Failed to sign CertificateRequest, will retry: test error", + Message: "Signing of CertificateRequest still in progress: reason why it is still in progress", LastTransitionTime: &fakeTimeObj2, }, }, }, validateError: errormatch.ErrorContains("terminal error: test error"), expectedEvents: []string{ - "Warning RetryableError Failed to sign CertificateRequest, will retry: test error", + "Warning Pending Signing of CertificateRequest still in progress: test error", }, }, @@ -562,14 +562,14 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, - Message: "Failed to sign CertificateRequest, will retry: test error2", + Message: "Signing of CertificateRequest still in progress: reason for being pending", LastTransitionTime: &fakeTimeObj2, }, }, }, - validateError: errormatch.ErrorContains("test error2"), + validateError: errormatch.ErrorContains("reason for being pending"), expectedEvents: []string{ - "Warning RetryableError Failed to sign CertificateRequest, will retry: test error2", + "Warning Pending Signing of CertificateRequest still in progress: reason for being pending", }, }, @@ -719,21 +719,21 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Type: "[condition type]", Status: cmmeta.ConditionTrue, Reason: "[reason]", - Message: "test error", + Message: "reason for being still pending", LastTransitionTime: &fakeTimeObj2, }, { Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, - Message: "Failed to sign CertificateRequest, will retry: test error", + Message: "Signing of CertificateRequest still in progress: reason for being still pending", LastTransitionTime: &fakeTimeObj2, }, }, }, - validateError: errormatch.ErrorContains("terminal error: test error"), + validateError: errormatch.ErrorContains("terminal error: reason for being still pending"), expectedEvents: []string{ - "Warning RetryableError Failed to sign CertificateRequest, will retry: test error", + "Warning Pending Signing of CertificateRequest still in progress: reason for being still pending", }, }, @@ -844,14 +844,14 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, - Message: "Failed to sign CertificateRequest, will retry: waiting for approval", + Message: "Signing of CertificateRequest still in progress: waiting for approval", LastTransitionTime: &fakeTimeObj2, }, }, }, validateError: errormatch.ErrorContains("waiting for approval"), expectedEvents: []string{ - "Warning RetryableError Failed to sign CertificateRequest, will retry: waiting for approval", + "Warning Pending Signing of CertificateRequest still in progress: waiting for approval", }, }, diff --git a/controllers/request_controller.go b/controllers/request_controller.go index 40570d1..e5b8f68 100644 --- a/controllers/request_controller.go +++ b/controllers/request_controller.go @@ -297,22 +297,43 @@ func (r *RequestController) reconcileStatusPatch( } // Check if we have still time to requeue & retry - isPendingError := errors.As(err, &signer.PendingError{}) + isPending := errors.As(err, &signer.PendingError{}) isPermanentError := errors.As(err, &signer.PermanentError{}) pastMaxRetryDuration := r.Clock.Now().After(requestObject.GetCreationTimestamp().Add(r.MaxRetryDuration)) - if !isPendingError && (isPermanentError || pastMaxRetryDuration) { - // fail permanently + switch { + case isPending: + // 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. + logger.V(1).WithValues("reason", err.Error()).Info("Signing in progress.") + statusPatch.SetPending(fmt.Sprintf("Signing still in progress. Reason: %s", err)) + + // Let's not trigger an unnecessary reconciliation when we know that the + // user-defined condition was changed and will trigger a reconciliation. + if didCustomConditionTransition { + return result, statusPatch, reconcile.TerminalError(err) // apply patch, done + } else { + return result, statusPatch, err // apply patch, requeue with backoff + } + case isPermanentError: logger.V(1).Error(err, "Permanent Request error. Marking as failed.") statusPatch.SetPermanentError(err) - return result, statusPatch, reconcile.TerminalError(err) // apply patch, done - } else { - // retry - logger.V(1).Error(err, "Retryable Request error.") + case pastMaxRetryDuration: + logger.V(1).Error(err, "Request has been retried for too long. Marking as failed.") + statusPatch.SetPermanentError(err) + return result, statusPatch, reconcile.TerminalError(err) // apply patch, done + default: + // We consider all the other errors as being retryable. + logger.V(1).Error(err, "Got an error, will be retried.") statusPatch.SetRetryableError(err) + // Let's not trigger an unnecessary reconciliation when we know that the + // user-defined condition was changed and will trigger a reconciliation. if didCustomConditionTransition { - // the reconciliation loop will be retriggered because of the added/ changed custom condition return result, statusPatch, reconcile.TerminalError(err) // apply patch, done } else { return result, statusPatch, err // apply patch, requeue with backoff diff --git a/controllers/request_objecthelper.go b/controllers/request_objecthelper.go index f3fcc4d..fc634b8 100644 --- a/controllers/request_objecthelper.go +++ b/controllers/request_objecthelper.go @@ -28,7 +28,8 @@ import ( ) const ( - eventRequestIssued = "Issued" + eventRequestIssued = "Issued" + eventRequestRetryable = "Pending" eventRequestUnexpectedError = "UnexpectedError" eventRequestRetryableError = "RetryableError" @@ -66,6 +67,7 @@ type RequestPatchHelper interface { //nolint:interfacebloat conditionStatus metav1.ConditionStatus, conditionReason string, conditionMessage string, ) (didCustomConditionTransition bool) + SetPending(reason string) SetRetryableError(error) SetPermanentError(error) SetUnexpectedError(error) diff --git a/controllers/request_objecthelper_certificaterequest.go b/controllers/request_objecthelper_certificaterequest.go index a006f88..b4691a1 100644 --- a/controllers/request_objecthelper_certificaterequest.go +++ b/controllers/request_objecthelper_certificaterequest.go @@ -210,6 +210,16 @@ func (c *certificateRequestPatchHelper) SetUnexpectedError(err error) { c.eventRecorder.Event(c.readOnlyObj, corev1.EventTypeWarning, eventRequestUnexpectedError, message) } +func (c *certificateRequestPatchHelper) SetPending(reason string) { + message, _ := c.setCondition( + cmapi.CertificateRequestConditionReady, + cmmeta.ConditionFalse, + cmapi.CertificateRequestReasonPending, + fmt.Sprintf("Signing still in progress. Reason: %s", reason), + ) + c.eventRecorder.Event(c.readOnlyObj, corev1.EventTypeWarning, eventRequestRetryableError, message) +} + func (c *certificateRequestPatchHelper) SetRetryableError(err error) { message, _ := c.setCondition( cmapi.CertificateRequestConditionReady, diff --git a/controllers/request_objecthelper_certificatesigningrequest.go b/controllers/request_objecthelper_certificatesigningrequest.go index b06882e..8b6c965 100644 --- a/controllers/request_objecthelper_certificatesigningrequest.go +++ b/controllers/request_objecthelper_certificatesigningrequest.go @@ -143,6 +143,11 @@ func (c *certificatesigningRequestPatchHelper) SetCustomCondition( return didCustomConditionTransition } +func (c *certificatesigningRequestPatchHelper) SetPending(reason string) { + message := fmt.Sprintf("Signing still in progress. Reason: %s", reason) + c.eventRecorder.Event(c.readOnlyObj, corev1.EventTypeWarning, eventRequestRetryable, message) +} + func (c *certificatesigningRequestPatchHelper) SetUnexpectedError(err error) { message := "Got an unexpected error while processing the CertificateSigningRequest" c.eventRecorder.Event(c.readOnlyObj, corev1.EventTypeWarning, eventRequestUnexpectedError, message)