Improve comments and variable naming

README.md

@@ -76,7 +76,7 @@ The business logic of the controllers can be provided to the libary through the
 If it returns a normal error, the controller will retry with backoff until the `Check` function succeeds.  
 If the error is of type `signer.PermanentError`, the controller will not retry automatically. Instead, an increase in Generation is required to recheck the issuer.
 
-- The `Sign` function is used by the CertificateRequest controller.
+- The `Sign` function is used by the CertificateRequest controller.  
 If it returns a normal error, the `Sign` function will be retried as long as we have not spent more than the configured `MaxRetryDuration` after the certificate request was created.  
 If the error is of type `signer.IssuerError`, the error is an error that should be set on the issuer instead of the CertificateRequest.  
 If the error is of type `signer.SetCertificateRequestConditionError`, the controller will, additional to setting the ready condition, also set the specified condition. This can be used in case we have to store some additional state in the status.  

controllers/request_controller.go

@@ -165,10 +165,10 @@ func (r *RequestController) reconcileStatusPatch(
 	}
 
 	// Select first matching issuer type and construct an issuerObject and issuerName
-	issuerObject, issuerName, err := r.matchIssuerType(requestObject)
+	issuerObject, issuerName, matchErr := r.matchIssuerType(requestObject)
 	// Ignore Request if issuerRef doesn't match one of our issuer Types
-	if err != nil {
-		logger.V(1).Info("Request has a foreign issuer. Ignoring.", "error", err)
+	if matchErr != nil {
+		logger.V(1).Info("Request has a foreign issuer. Ignoring.", "error", matchErr)
 		return result, nil, nil // done
 	}
 	issuerGvk := issuerObject.GetObjectKind().GroupVersionKind()
@@ -287,8 +287,8 @@ func (r *RequestController) reconcileStatusPatch(
 		return result, statusPatch, nil // apply patch, done
 	}
 
-	signedCertificate, err := r.Sign(log.IntoContext(ctx, logger), requestObjectHelper.RequestObject(), issuerObject)
-	if err == nil {
+	signedCertificate, signErr := r.Sign(log.IntoContext(ctx, logger), requestObjectHelper.RequestObject(), issuerObject)
+	if signErr == nil {
 		logger.V(1).Info("Successfully finished the reconciliation.")
 		statusPatch.SetIssued(signedCertificate)
 
@@ -297,7 +297,7 @@ func (r *RequestController) reconcileStatusPatch(
 
 	// An error in the issuer part of the operator should trigger a reconcile
 	// of the issuer's state.
-	if issuerError := new(signer.IssuerError); errors.As(err, issuerError) {
+	if issuerError := new(signer.IssuerError); errors.As(signErr, issuerError) {
 		if reportError := r.EventSource.ReportError(
 			issuerGvk, client.ObjectKeyFromObject(issuerObject),
 			issuerError.Err,
@@ -312,8 +312,8 @@ func (r *RequestController) reconcileStatusPatch(
 	}
 
 	didCustomConditionTransition := false
-	if targetCustom := new(signer.SetCertificateRequestConditionError); errors.As(err, targetCustom) {
-		logger.V(1).Info("Set RequestCondition error. Setting condition.", "error", err)
+	if targetCustom := new(signer.SetCertificateRequestConditionError); errors.As(signErr, targetCustom) {
+		logger.V(1).Info("Set RequestCondition error. Setting condition.", "error", signErr)
 		didCustomConditionTransition = statusPatch.SetCustomCondition(
 			string(targetCustom.ConditionType),
 			metav1.ConditionStatus(targetCustom.Status),
@@ -324,8 +324,8 @@ func (r *RequestController) reconcileStatusPatch(
 
 	// Check if we have still time to requeue & retry
 	pendingError := &signer.PendingError{}
-	isPending := errors.As(err, pendingError)
-	isPermanentError := errors.As(err, &signer.PermanentError{})
+	isPending := errors.As(signErr, pendingError)
+	isPermanentError := errors.As(signErr, &signer.PermanentError{})
 	pastMaxRetryDuration := r.Clock.Now().After(requestObject.GetCreationTimestamp().Add(r.MaxRetryDuration))
 	switch {
 	case isPending:
@@ -335,8 +335,8 @@ func (r *RequestController) reconcileStatusPatch(
 		// 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))
+		logger.V(1).WithValues("reason", signErr.Error()).Info("Signing in progress.")
+		statusPatch.SetPending(fmt.Sprintf("Signing still in progress. Reason: %s", signErr))
 
 		// Let's not trigger an unnecessary reconciliation when we know that the
 		// user-defined condition was changed and will trigger a reconciliation.
@@ -351,24 +351,24 @@ func (r *RequestController) reconcileStatusPatch(
 			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
+		logger.V(1).Error(signErr, "Permanent Request error. Marking as failed.")
+		statusPatch.SetPermanentError(signErr)
+		return result, statusPatch, reconcile.TerminalError(signErr) // apply patch, done
 	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
+		logger.V(1).Error(signErr, "Request has been retried for too long. Marking as failed.")
+		statusPatch.SetPermanentError(signErr)
+		return result, statusPatch, reconcile.TerminalError(signErr) // 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)
+		logger.V(1).Error(signErr, "Got an error, will be retried.")
+		statusPatch.SetRetryableError(signErr)
 
 		// 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
+			return result, statusPatch, reconcile.TerminalError(signErr) // apply patch, done
 		} else {
-			return result, statusPatch, err // apply patch, requeue with backoff
+			return result, statusPatch, signErr // apply patch, requeue with backoff
 		}
 	}
 }

controllers/signer/certificate_request_object.go

@@ -0,0 +1,166 @@
+/*
+Copyright 2023 The cert-manager Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package signer
+
+import (
+	"crypto/x509"
+	"time"
+
+	apiutil "github.com/cert-manager/cert-manager/pkg/api/util"
+	cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
+	experimentalapi "github.com/cert-manager/cert-manager/pkg/apis/experimental/v1alpha1"
+	"github.com/cert-manager/cert-manager/pkg/util/pki"
+	certificatesv1 "k8s.io/api/certificates/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+// CertificateRequestObject represents either a cert-manager CertificateRequest
+// or a Kubernetes CertificateSigningRequest resource. The interface hides the
+// underlying spec fields and exposes a certificate template and the raw CSR
+// bytes. This lets the signer be agnostic to the underlying resource type and
+// to how spec fields are interpreted (for example, defaulting logic). The
+// signer can still access labels, annotations, or other metadata, and can use
+// `GetConditions` to retrieve the resource's conditions.
+type CertificateRequestObject interface {
+	metav1.Object
+
+	// Return the Certificate details originating from the cert-manager
+	// CertificateRequest or Kubernetes CertificateSigningRequest resources.
+	GetCertificateDetails() (details CertificateDetails, err error)
+
+	GetConditions() []metav1.Condition
+}
+
+type CertificateDetails struct {
+	CSR         []byte
+	Duration    time.Duration
+	IsCA        bool
+	MaxPathLen  *int
+	KeyUsage    x509.KeyUsage
+	ExtKeyUsage []x509.ExtKeyUsage
+}
+
+// CertificateTemplate generates a certificate template for issuance,
+// based on CertificateDetails extracted from the CertificateRequest or
+// CertificateSigningRequest resource.
+//
+// This function internally calls CertificateTemplateFromCSRPEM, which performs
+// additional work such as parsing the CSR and verifying signatures. Since this
+// operation can be expensive, issuer implementations should call this function
+// only when a certificate template is actually needed (e.g., not when proxying
+// the X.509 CSR to a CA).
+func (cd CertificateDetails) CertificateTemplate() (template *x509.Certificate, err error) {
+	return pki.CertificateTemplateFromCSRPEM(
+		cd.CSR,
+		pki.CertificateTemplateOverrideDuration(cd.Duration),
+		pki.CertificateTemplateValidateAndOverrideBasicConstraints(cd.IsCA, cd.MaxPathLen), // Override the basic constraints, but make sure they match the constraints in the CSR if present
+		pki.CertificateTemplateValidateAndOverrideKeyUsages(cd.KeyUsage, cd.ExtKeyUsage),   // Override the key usages, but make sure they match the usages in the CSR if present
+	)
+}
+
+type certificateRequestImpl struct {
+	*cmapi.CertificateRequest
+}
+
+var _ CertificateRequestObject = &certificateRequestImpl{}
+
+func CertificateRequestObjectFromCertificateRequest(cr *cmapi.CertificateRequest) CertificateRequestObject {
+	return &certificateRequestImpl{cr}
+}
+
+func (c *certificateRequestImpl) GetCertificateDetails() (CertificateDetails, error) {
+	duration := apiutil.DefaultCertDuration(c.Spec.Duration)
+
+	keyUsage, extKeyUsage, err := pki.KeyUsagesForCertificateOrCertificateRequest(c.Spec.Usages, c.Spec.IsCA)
+	if err != nil {
+		return CertificateDetails{}, err
+	}
+
+	return CertificateDetails{
+		CSR:         c.Spec.Request,
+		Duration:    duration,
+		IsCA:        c.Spec.IsCA,
+		MaxPathLen:  nil,
+		KeyUsage:    keyUsage,
+		ExtKeyUsage: extKeyUsage,
+	}, nil
+}
+
+func (c *certificateRequestImpl) GetConditions() []metav1.Condition {
+	conditions := make([]metav1.Condition, 0, len(c.Status.Conditions))
+	for _, condition := range c.Status.Conditions {
+		var lastTransition metav1.Time
+		if lt := condition.LastTransitionTime; lt != nil {
+			lastTransition = *lt
+		}
+		conditions = append(conditions, metav1.Condition{
+			Type:               string(condition.Type),
+			Status:             metav1.ConditionStatus(condition.Status),
+			LastTransitionTime: lastTransition,
+			Reason:             condition.Reason,
+			Message:            condition.Message,
+		})
+	}
+	return conditions
+}
+
+type certificateSigningRequestImpl struct {
+	*certificatesv1.CertificateSigningRequest
+}
+
+var _ CertificateRequestObject = &certificateSigningRequestImpl{}
+
+func CertificateRequestObjectFromCertificateSigningRequest(csr *certificatesv1.CertificateSigningRequest) CertificateRequestObject {
+	return &certificateSigningRequestImpl{csr}
+}
+
+func (c *certificateSigningRequestImpl) GetCertificateDetails() (CertificateDetails, error) {
+	duration, err := pki.DurationFromCertificateSigningRequest(c.CertificateSigningRequest)
+	if err != nil {
+		return CertificateDetails{}, err
+	}
+
+	isCA := c.CertificateSigningRequest.Annotations[experimentalapi.CertificateSigningRequestIsCAAnnotationKey] == "true"
+
+	keyUsage, extKeyUsage, err := pki.BuildKeyUsagesKube(c.Spec.Usages)
+	if err != nil {
+		return CertificateDetails{}, err
+	}
+
+	return CertificateDetails{
+		CSR:         c.Spec.Request,
+		Duration:    duration,
+		IsCA:        isCA,
+		MaxPathLen:  nil,
+		KeyUsage:    keyUsage,
+		ExtKeyUsage: extKeyUsage,
+	}, nil
+}
+
+func (c *certificateSigningRequestImpl) GetConditions() []metav1.Condition {
+	conditions := make([]metav1.Condition, 0, len(c.Status.Conditions))
+	for _, condition := range c.Status.Conditions {
+		conditions = append(conditions, metav1.Condition{
+			Type:               string(condition.Type),
+			Status:             metav1.ConditionStatus(condition.Status),
+			LastTransitionTime: condition.LastTransitionTime,
+			Reason:             condition.Reason,
+			Message:            condition.Message,
+		})
+	}
+	return conditions
+}

controllers/signer/err_issuer.go

@@ -16,15 +16,13 @@ limitations under the License.
 
 package signer
 
-// IssuerError is thrown by the CertificateRequest controller
-// to indicate that er was an error in the issuer part of the
-// reconcile process, and that the issuer's reconcile function
-// should be retriggered.
+// IssuerError is returned by the CertificateRequest controller to indicate
+// there was an error in the issuer part of the reconcile process and that
+// the issuer's reconcile function should be retried.
 //
-// This error is useful to indicate that the Sign function got
-// an error for an action that should have been checked by the
-// Check function, and that has appeared after the Check function
-// has been called.
+// This error is useful when the Sign function encounters an error for an
+// action that should have been handled by the Check function, and which
+// surfaced after Check had already succeeded.
 //
 // > This error should be returned only by the Sign function.
 type IssuerError struct {

controllers/signer/err_pending.go

@@ -18,21 +18,19 @@ package signer
 
 import "time"
 
-// PendingError should be returned if we are certain that we will converge to a
-// successful result or another type of error in a finite amount of time by
-// just retrying the same operation.
+// PendingError should be returned when retrying the same operation is expected
+// to result in either success or another error within a finite time.
 //
-// It can be used to circumvent the MaxRetryDuration
-// check, which is useful for example when the signer is waiting for an async
-// answer from an external service that is indicating that the request is still
-// being processed.
+// It can be used to bypass the MaxRetryDuration check, for example when the
+// signer is waiting for an asynchronous response from an external service
+// indicating the request is still being processed.
 //
 // > This error should be returned only by the Sign function.
 type PendingError struct {
 	Err error
 
-	// RequeueAfter can be used to specify how long to wait before retrying. By default
-	// we wait for 1s before retrying.
+	// RequeueAfter can be set to control how long to wait before retrying. By
+	// default the controller waits 1s before retrying.
 	RequeueAfter time.Duration
 }
 

controllers/signer/err_permanent.go

@@ -16,19 +16,18 @@ limitations under the License.
 
 package signer
 
-// PermanentError is returned if it is impossible for the resource to
-// get in a Ready state without being changed. It should not be used
-// if there is any way to fix the error by altering the environment/
-// other resources. The client should not try again after receiving
-// this error.
+// PermanentError is returned when it is impossible for the resource to
+// become Ready without changing the resource itself. It must not be used
+// when the issue can be resolved by modifying the environment or other
+// resources. The controller should not retry after receiving this error.
 //
 // For the Check function, this error is useful when we detected an
 // invalid configuration/ setting in the Issuer or ClusterIssuer resource.
 // This should only happen very rarely, because of webhook validation.
 //
-// For the Sign function, this error is useful when we detected an
-// error that will only get resolved by creating a new CertificateRequest,
-// for example when it is required to craft a new CSR.
+// For the Sign function, this error is useful when the problem can only be
+// resolved by creating a new CertificateRequest (for example, when a new
+// CSR must be generated).
 //
 // > This error should be returned by the Sign or Check function.
 type PermanentError struct {