Merge pull request #42 from inteon/cleanup_issuer_controller

Cleanup issuer controller

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)

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

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

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)
  }
 

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,32 +98,41 @@ 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.")
  }
  }
 
  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()
  // calling IsInvalidated early to make sure the map is always cleared
  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.