Merge pull request #59 from maelvls/detail-logs-usage

Controller: PendingError isn't actually an error

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,16 @@ 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 still in progress. Reason: Signing still in progress. Reason: reason for being pending",
 						LastTransitionTime: &fakeTimeObj2,
 					},
 				},
 			},
-			validateError: errormatch.ErrorContains("pending error"),
+			expectedResult: reconcile.Result{
+				Requeue: true,
+			},
 			expectedEvents: []string{
-				"Warning RetryableError Failed to sign CertificateRequest, will retry: pending error",
+				"Warning RetryableError Signing still in progress. Reason: Signing still in progress. Reason: reason for being pending",
 			},
 		},
 
@@ -726,14 +728,16 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) {
 						Type:               cmapi.CertificateRequestConditionReady,
 						Status:             cmmeta.ConditionFalse,
 						Reason:             cmapi.CertificateRequestReasonPending,
-						Message:            "Failed to sign CertificateRequest, will retry: test error",
+						Message:            "Signing still in progress. Reason: Signing still in progress. Reason: test error",
 						LastTransitionTime: &fakeTimeObj2,
 					},
 				},
 			},
-			validateError: errormatch.ErrorContains("terminal error: test error"),
+			expectedResult: reconcile.Result{
+				Requeue: false,
+			},
 			expectedEvents: []string{
-				"Warning RetryableError Failed to sign CertificateRequest, will retry: test error",
+				"Warning RetryableError Signing still in progress. Reason: Signing still in progress. Reason: test error",
 			},
 		},
 

controllers/certificatesigningrequest_controller_test.go

@@ -342,9 +342,11 @@ func TestCertificateSigningRequestReconcilerReconcile(t *testing.T) {
 			expectedStatusPatch: &certificatesv1.CertificateSigningRequestStatus{
 				Conditions: nil,
 			},
-			validateError: errormatch.ErrorContains("pending error"),
+			expectedResult: reconcile.Result{
+				Requeue: true,
+			},
 			expectedEvents: []string{
-				"Warning RetryableError Failed to sign CertificateSigningRequest, will retry: pending error",
+				"Warning Pending Signing still in progress. Reason: Signing still in progress. Reason: pending error",
 			},
 		},
 
@@ -598,9 +600,11 @@ func TestCertificateSigningRequestReconcilerReconcile(t *testing.T) {
 					},
 				},
 			},
-			validateError: errormatch.ErrorContains("terminal error: test error"),
+			expectedResult: reconcile.Result{
+				Requeue: false,
+			},
 			expectedEvents: []string{
-				"Warning RetryableError Failed to sign CertificateSigningRequest, will retry: test error",
+				"Warning Pending Signing still in progress. Reason: Signing still in progress. Reason: test error",
 			},
 		},
 

controllers/request_controller.go

@@ -297,22 +297,44 @@ 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, nil // apply patch, done
+		} else {
+			result.Requeue = true
+			return result, statusPatch, nil // 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)