controller: add "is pending" as a possible case

Signed-off-by: Maël Valais <mael@vls.dev>

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",
  },
  },
 

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

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)

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,

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)