From aa4880ccbbe50d10992fb103aada6402694745b2 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Tue, 30 May 2023 06:45:22 +0200 Subject: [PATCH 1/5] improve setting conditions & fix wrong LastTransitionTime Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- conditions/certificaterequest.go | 52 ++++++++++--- conditions/issuer.go | 77 ++++++++++--------- controllers/certificaterequest_controller.go | 63 ++++++++------- ...caterequest_controller_integration_test.go | 2 + .../certificaterequest_controller_test.go | 2 + controllers/issuer_controller.go | 56 +++++++------- controllers/predicates.go | 2 +- .../testsetups/simple/testutil/api_gen.go | 2 + 8 files changed, 155 insertions(+), 101 deletions(-) diff --git a/conditions/certificaterequest.go b/conditions/certificaterequest.go index 5b565ab..7682580 100644 --- a/conditions/certificaterequest.go +++ b/conditions/certificaterequest.go @@ -17,30 +17,60 @@ limitations under the License. package conditions import ( - cmutil "github.com/cert-manager/cert-manager/pkg/api/util" + "github.com/cert-manager/cert-manager/pkg/api/util" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) // Update the status with the provided condition details & return // the added condition. -// NOTE: this code is just a workaround for cmutil only accepting the certificaterequest object func SetCertificateRequestStatusCondition( - conditions *[]cmapi.CertificateRequestCondition, + existingConditions []cmapi.CertificateRequestCondition, + patchConditions *[]cmapi.CertificateRequestCondition, conditionType cmapi.CertificateRequestConditionType, status cmmeta.ConditionStatus, reason, message string, ) *cmapi.CertificateRequestCondition { - cr := cmapi.CertificateRequest{ - Status: cmapi.CertificateRequestStatus{ - Conditions: *conditions, - }, + newCondition := cmapi.CertificateRequestCondition{ + Type: conditionType, + Status: status, + Reason: reason, + Message: message, } - cmutil.SetCertificateRequestCondition(&cr, conditionType, status, reason, message) - condition := cmutil.GetCertificateRequestCondition(&cr, conditionType) + nowTime := metav1.NewTime(util.Clock.Now()) + newCondition.LastTransitionTime = &nowTime - *conditions = cr.Status.Conditions + // Reset the LastTransitionTime if the status hasn't changed + for _, cond := range existingConditions { + if cond.Type != conditionType { + continue + } - return condition + // If this update doesn't contain a state transition, we don't update + // the conditions LastTransitionTime to Now() + if cond.Status == status { + newCondition.LastTransitionTime = cond.LastTransitionTime + } + } + + // Search through existing conditions + for idx, cond := range *patchConditions { + // Skip unrelated conditions + if cond.Type != conditionType { + continue + } + + // Overwrite the existing condition + (*patchConditions)[idx] = newCondition + + return &newCondition + } + + // If we've not found an existing condition of this type, we simply insert + // the new condition into the slice. + *patchConditions = append(*patchConditions, newCondition) + + return &newCondition } diff --git a/conditions/issuer.go b/conditions/issuer.go index cc40d2c..eef20f3 100644 --- a/conditions/issuer.go +++ b/conditions/issuer.go @@ -17,59 +17,64 @@ limitations under the License. package conditions import ( - cmutil "github.com/cert-manager/cert-manager/pkg/api/util" + "github.com/cert-manager/cert-manager/pkg/api/util" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" ) -// private struct; only used to implement the GenericIssuer interface -// for use with the cmutil.SetIssuerCondition function -type genericIssuer struct { - metav1.TypeMeta `json:",inline"` - metav1.ObjectMeta `json:"metadata,omitempty"` - - Status cmapi.IssuerStatus `json:"status"` -} - -func (g *genericIssuer) DeepCopyObject() runtime.Object { - panic("[HACK]: this function should not get called") -} - -func (g *genericIssuer) GetSpec() *cmapi.IssuerSpec { - panic("[HACK]: this function should not get called") -} - -func (g *genericIssuer) GetObjectMeta() *metav1.ObjectMeta { - return &g.ObjectMeta -} - -func (g *genericIssuer) GetStatus() *cmapi.IssuerStatus { - return &g.Status -} - // Update the status with the provided condition details & return // the added condition. -// NOTE: this code is just a workaround for cmutil only accepting a GenericIssuer interface func SetIssuerStatusCondition( - conditions *[]cmapi.IssuerCondition, + existingConditions []cmapi.IssuerCondition, + patchConditions *[]cmapi.IssuerCondition, observedGeneration int64, conditionType cmapi.IssuerConditionType, status cmmeta.ConditionStatus, reason, message string, ) *cmapi.IssuerCondition { - gi := genericIssuer{ - Status: cmapi.IssuerStatus{ - Conditions: *conditions, - }, + newCondition := cmapi.IssuerCondition{ + Type: conditionType, + Status: status, + Reason: reason, + Message: message, + ObservedGeneration: observedGeneration, } - cmutil.SetIssuerCondition(&gi, observedGeneration, conditionType, status, reason, message) + nowTime := metav1.NewTime(util.Clock.Now()) + newCondition.LastTransitionTime = &nowTime + + // Reset the LastTransitionTime if the status hasn't changed + for _, cond := range existingConditions { + if cond.Type != conditionType { + continue + } + + // If this update doesn't contain a state transition, we don't update + // the conditions LastTransitionTime to Now() + if cond.Status == status { + newCondition.LastTransitionTime = cond.LastTransitionTime + } + } + + // Search through existing conditions + for idx, cond := range *patchConditions { + // Skip unrelated conditions + if cond.Type != conditionType { + continue + } + + // Overwrite the existing condition + (*patchConditions)[idx] = newCondition + + return &newCondition + } - *conditions = gi.Status.Conditions + // If we've not found an existing condition of this type, we simply insert + // the new condition into the slice. + *patchConditions = append(*patchConditions, newCondition) - return GetIssuerStatusCondition(*conditions, conditionType) + return &newCondition } func GetIssuerStatusCondition( diff --git a/controllers/certificaterequest_controller.go b/controllers/certificaterequest_controller.go index 63fd77a..d9ca11f 100644 --- a/controllers/certificaterequest_controller.go +++ b/controllers/certificaterequest_controller.go @@ -73,10 +73,10 @@ func (r *CertificateRequestReconciler) Reconcile(ctx context.Context, req ctrl.R logger.V(2).Info("Starting reconcile loop", "name", req.Name, "namespace", req.Namespace) - result, crsPatch, returnedError := r.reconcileStatusPatch(logger, ctx, req) - logger.V(2).Info("Got StatusPatch result", "result", result, "patch", crsPatch, "error", returnedError) - if crsPatch != nil { - cr, patch, err := ssaclient.GenerateCertificateRequestStatusPatch(req.Name, req.Namespace, crsPatch) + result, crStatusPatch, returnedError := r.reconcileStatusPatch(logger, ctx, req) + logger.V(2).Info("Got StatusPatch result", "result", result, "patch", crStatusPatch, "error", returnedError) + if crStatusPatch != nil { + cr, patch, err := ssaclient.GenerateCertificateRequestStatusPatch(req.Name, req.Namespace, crStatusPatch) if err != nil { returnedError = utilerrors.NewAggregate([]error{err, returnedError}) result = ctrl.Result{} @@ -105,7 +105,7 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( logger logr.Logger, ctx context.Context, req ctrl.Request, -) (result ctrl.Result, crsPatch *cmapi.CertificateRequestStatus, returnedError error) { +) (result ctrl.Result, crStatusPatch *cmapi.CertificateRequestStatus, returnedError error) { var cr cmapi.CertificateRequest if err := r.Client.Get(ctx, req.NamespacedName, &cr); err != nil && apierrors.IsNotFound(err) { logger.V(1).Info("Not found. Ignoring.") @@ -161,14 +161,15 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( // We now have a CertificateRequest that belongs to us so we are responsible // for updating its Status. - crsPatch = &cmapi.CertificateRequestStatus{} + crStatusPatch = &cmapi.CertificateRequestStatus{} // Add a Ready condition if one does not already exist. Set initial Status // to Unknown. if ready := cmutil.GetCertificateRequestCondition(&cr, cmapi.CertificateRequestConditionReady); ready == nil { logger.V(1).Info("Initializing Ready condition") conditions.SetCertificateRequestStatusCondition( - &crsPatch.Conditions, + cr.Status.Conditions, + &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, cmmeta.ConditionUnknown, v1alpha1.CertificateRequestConditionReasonInitializing, @@ -177,34 +178,36 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( // To continue reconciling this CertificateRequest, we must re-run the reconcile loop // after adding the Unknown Ready condition. This update will trigger a // new reconcile loop, so we don't need to requeue here. - return result, crsPatch, nil // apply patch, done + return result, crStatusPatch, nil // apply patch, done } if cmutil.CertificateRequestIsDenied(&cr) { logger.V(1).Info("CertificateRequest has been denied. Marking as failed.") condition := conditions.SetCertificateRequestStatusCondition( - &crsPatch.Conditions, + cr.Status.Conditions, + &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, cmmeta.ConditionFalse, cmapi.CertificateRequestReasonDenied, "The CertificateRequest was denied by an approval controller", ) - crsPatch.FailureTime = condition.LastTransitionTime.DeepCopy() + crStatusPatch.FailureTime = condition.LastTransitionTime.DeepCopy() r.EventRecorder.Eventf(&cr, corev1.EventTypeNormal, "DetectedDenied", "Detected that the CR is denied, will update Ready condition") - return result, crsPatch, nil // done, apply patch + return result, crStatusPatch, nil // done, apply patch } if err := r.Client.Get(ctx, issuerName, issuerObject); err != nil && apierrors.IsNotFound(err) { logger.V(1).Info("Issuer not found. Waiting for it to be created") conditions.SetCertificateRequestStatusCondition( - &crsPatch.Conditions, + cr.Status.Conditions, + &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, cmmeta.ConditionFalse, cmapi.CertificateRequestReasonPending, fmt.Sprintf("%s. Waiting for it to be created.", err), ) r.EventRecorder.Eventf(&cr, corev1.EventTypeNormal, "WaitingForIssuerExist", "Waiting for the issuer to exist") - return result, crsPatch, nil // done, apply patch + return result, crStatusPatch, nil // done, apply patch } else if err != nil { r.EventRecorder.Eventf(&cr, corev1.EventTypeWarning, "UnexpectedError", "Got an unexpected error while processing the CR") return result, nil, fmt.Errorf("unexpected get error: %v", err) // retry @@ -229,14 +232,15 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( logger.V(1).Info("Issuer is not Ready yet. Waiting for it to become ready.", "issuer ready condition", readyCondition) conditions.SetCertificateRequestStatusCondition( - &crsPatch.Conditions, + cr.Status.Conditions, + &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, cmmeta.ConditionFalse, cmapi.CertificateRequestReasonPending, message, ) r.EventRecorder.Eventf(&cr, corev1.EventTypeNormal, "WaitingForIssuerReady", "Waiting for the issuer to become ready") - return result, crsPatch, nil // done, apply patch + return result, crStatusPatch, nil // done, apply patch } signedCertificate, err := r.Sign(log.IntoContext(ctx, logger), &cr, issuerObject) @@ -253,13 +257,14 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( logger.V(1).Error(err, "Temporary CertificateRequest error.") conditions.SetCertificateRequestStatusCondition( - &crsPatch.Conditions, + cr.Status.Conditions, + &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, cmmeta.ConditionFalse, cmapi.CertificateRequestReasonPending, "Issuer is not Ready yet. Current ready condition is outdated. Waiting for it to become ready.", ) - return result, crsPatch, nil // done, apply patch + return result, crStatusPatch, nil // done, apply patch } didCustomConditionTransition := false @@ -267,7 +272,8 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( if targetCustom := new(signer.SetCertificateRequestConditionError); errors.As(err, targetCustom) { logger.V(1).Info("Set CertificateRequestCondition error. Setting condition.", "error", err) conditions.SetCertificateRequestStatusCondition( - &crsPatch.Conditions, + cr.Status.Conditions, + &crStatusPatch.Conditions, targetCustom.ConditionType, targetCustom.Status, targetCustom.Reason, @@ -287,20 +293,22 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( // fail permanently logger.V(1).Error(err, "Permanent CertificateRequest error. Marking as failed.") condition := conditions.SetCertificateRequestStatusCondition( - &crsPatch.Conditions, + cr.Status.Conditions, + &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, cmmeta.ConditionFalse, cmapi.CertificateRequestReasonFailed, fmt.Sprintf("CertificateRequest has failed permanently: %s", err), ) - crsPatch.FailureTime = condition.LastTransitionTime.DeepCopy() + crStatusPatch.FailureTime = condition.LastTransitionTime.DeepCopy() r.EventRecorder.Eventf(&cr, corev1.EventTypeWarning, "PermanentError", "Failed permanently to sign CertificateRequest: %s", err) - return result, crsPatch, nil // done, apply patch + return result, crStatusPatch, nil // done, apply patch } else { // retry logger.V(1).Error(err, "Retryable CertificateRequest error.") conditions.SetCertificateRequestStatusCondition( - &crsPatch.Conditions, + cr.Status.Conditions, + &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, cmmeta.ConditionFalse, cmapi.CertificateRequestReasonPending, @@ -310,7 +318,7 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( r.EventRecorder.Eventf(&cr, corev1.EventTypeWarning, "RetryableError", "Failed to sign CertificateRequest, will retry: %s", err) if didCustomConditionTransition { // the reconciliation loop will be retriggered because of the added/ changed custom condition - return result, crsPatch, nil // done, apply patch + return result, crStatusPatch, nil // done, apply patch } else { // 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 @@ -321,14 +329,15 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( // 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, crsPatch, nil // requeue with backoff, apply patch + return result, crStatusPatch, nil // requeue with backoff, apply patch } } } - crsPatch.Certificate = signedCertificate + crStatusPatch.Certificate = signedCertificate conditions.SetCertificateRequestStatusCondition( - &crsPatch.Conditions, + cr.Status.Conditions, + &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, cmmeta.ConditionTrue, cmapi.CertificateRequestReasonIssued, @@ -337,7 +346,7 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( logger.V(1).Info("Successfully finished the reconciliation.") r.EventRecorder.Eventf(&cr, corev1.EventTypeNormal, "Issued", "Succeeded signing the CertificateRequest") - return result, crsPatch, nil // done, apply patch + return result, crStatusPatch, nil // done, apply patch } func (r *CertificateRequestReconciler) setIssuersGroupVersionKind(scheme *runtime.Scheme) error { diff --git a/controllers/certificaterequest_controller_integration_test.go b/controllers/certificaterequest_controller_integration_test.go index 2eac425..3d3b765 100644 --- a/controllers/certificaterequest_controller_integration_test.go +++ b/controllers/certificaterequest_controller_integration_test.go @@ -360,6 +360,7 @@ func createApprovedCR(t *testing.T, ctx context.Context, kc client.Client, cr *c require.NoError(t, kc.Create(ctx, cr)) conditions.SetCertificateRequestStatusCondition( + cr.Status.Conditions, &cr.Status.Conditions, cmapi.CertificateRequestConditionApproved, cmmeta.ConditionTrue, @@ -399,6 +400,7 @@ func markIssuerReady(t *testing.T, ctx context.Context, kc client.Client, fieldO issuerStatus := &v1alpha1.IssuerStatus{} conditions.SetIssuerStatusCondition( + issuerStatus.Conditions, &issuerStatus.Conditions, issuer.GetGeneration(), cmapi.IssuerConditionReady, diff --git a/controllers/certificaterequest_controller_test.go b/controllers/certificaterequest_controller_test.go index 89f4d6e..9155601 100644 --- a/controllers/certificaterequest_controller_test.go +++ b/controllers/certificaterequest_controller_test.go @@ -99,6 +99,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { }), func(cr *cmapi.CertificateRequest) { conditions.SetCertificateRequestStatusCondition( + cr.Status.Conditions, &cr.Status.Conditions, cmapi.CertificateRequestConditionReady, cmmeta.ConditionUnknown, @@ -106,6 +107,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { fieldOwner+" has begun reconciling this CertificateRequest", ) conditions.SetCertificateRequestStatusCondition( + cr.Status.Conditions, &cr.Status.Conditions, cmapi.CertificateRequestConditionApproved, cmmeta.ConditionTrue, diff --git a/controllers/issuer_controller.go b/controllers/issuer_controller.go index 68f03a7..e941869 100644 --- a/controllers/issuer_controller.go +++ b/controllers/issuer_controller.go @@ -67,10 +67,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) - result, isPatch, returnedError := r.reconcileStatusPatch(logger, ctx, req) - logger.V(2).Info("Got StatusPatch result", "result", result, "patch", isPatch, "error", returnedError) - if isPatch != nil { - cr, patch, err := ssaclient.GenerateIssuerStatusPatch(r.ForObject, req.Name, req.Namespace, isPatch) + 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) if err != nil { returnedError = utilerrors.NewAggregate([]error{err, returnedError}) result = ctrl.Result{} @@ -99,27 +99,27 @@ func (r *IssuerReconciler) reconcileStatusPatch( logger logr.Logger, ctx context.Context, req ctrl.Request, -) (result ctrl.Result, isPatch *v1alpha1.IssuerStatus, returnedError error) { +) (result ctrl.Result, issuerStatusPatch *v1alpha1.IssuerStatus, returnedError error) { // Get the ClusterIssuer - vi := r.ForObject.DeepCopyObject().(v1alpha1.Issuer) + 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, vi); err != nil && apierrors.IsNotFound(err) { + if err := r.Client.Get(ctx, req.NamespacedName, issuer); err != nil && apierrors.IsNotFound(err) { logger.V(1).Info("Not found. Ignoring.") return result, nil, nil // done } else if err != nil { return result, nil, fmt.Errorf("unexpected get error: %v", err) // retry } - readyCondition := conditions.GetIssuerStatusCondition(vi.GetStatus().Conditions, cmapi.IssuerConditionReady) + readyCondition := conditions.GetIssuerStatusCondition(issuer.GetStatus().Conditions, cmapi.IssuerConditionReady) // Ignore Issuer if it is already permanently Failed isFailed := (readyCondition != nil) && (readyCondition.Status == cmmeta.ConditionFalse) && (readyCondition.Reason == v1alpha1.IssuerConditionReasonFailed) && - (readyCondition.ObservedGeneration >= vi.GetGeneration()) + (readyCondition.ObservedGeneration >= issuer.GetGeneration()) if isFailed { logger.V(1).Info("Issuer is Failed. Ignoring.") return result, nil, nil // done @@ -127,15 +127,16 @@ func (r *IssuerReconciler) reconcileStatusPatch( // We now have a Issuer that belongs to us so we are responsible // for updating its Status. - isPatch = &v1alpha1.IssuerStatus{} + 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( - &isPatch.Conditions, - vi.GetGeneration(), + issuer.GetStatus().Conditions, + &issuerStatusPatch.Conditions, + issuer.GetGeneration(), cmapi.IssuerConditionReady, cmmeta.ConditionUnknown, v1alpha1.IssuerConditionReasonInitializing, @@ -144,7 +145,7 @@ func (r *IssuerReconciler) reconcileStatusPatch( // To continue reconciling this Issuer, we must re-run the reconcile loop // after adding the Unknown Ready condition. This update will trigger a // new reconcile loop, so we don't need to requeue here. - return result, isPatch, nil // apply patch, done + return result, issuerStatusPatch, nil // apply patch, done } var err error @@ -153,7 +154,7 @@ func (r *IssuerReconciler) reconcileStatusPatch( // update the ready state of the issuer to reflect the error. err = reportedError } else { - err = r.Check(log.IntoContext(ctx, logger), vi) + err = r.Check(log.IntoContext(ctx, logger), issuer) } if err != nil { isPermanentError := errors.As(err, &signer.PermanentError{}) @@ -161,27 +162,29 @@ func (r *IssuerReconciler) reconcileStatusPatch( // fail permanently logger.V(1).Error(err, "Permanent Issuer error. Marking as failed.") conditions.SetIssuerStatusCondition( - &isPatch.Conditions, - vi.GetGeneration(), + issuer.GetStatus().Conditions, + &issuerStatusPatch.Conditions, + issuer.GetGeneration(), cmapi.IssuerConditionReady, cmmeta.ConditionFalse, v1alpha1.IssuerConditionReasonFailed, fmt.Sprintf("Issuer has failed permanently: %s", err), ) - r.EventRecorder.Eventf(vi, corev1.EventTypeWarning, "PermanentError", "Failed permanently to check issuer: %s", err) - return result, isPatch, nil // apply patch, retry + 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( - &isPatch.Conditions, - vi.GetGeneration(), + 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(vi, corev1.EventTypeWarning, "RetryableError", "Failed to check issuer, will retry: %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 @@ -191,13 +194,14 @@ func (r *IssuerReconciler) reconcileStatusPatch( // 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, isPatch, nil // apply patch, retry + return result, issuerStatusPatch, nil // apply patch, retry } } conditions.SetIssuerStatusCondition( - &isPatch.Conditions, - vi.GetGeneration(), + issuer.GetStatus().Conditions, + &issuerStatusPatch.Conditions, + issuer.GetGeneration(), cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, @@ -205,8 +209,8 @@ func (r *IssuerReconciler) reconcileStatusPatch( ) logger.V(1).Info("Successfully finished the reconciliation.") - r.EventRecorder.Eventf(vi, corev1.EventTypeNormal, "Checked", "Succeeded checking the issuer") - return result, isPatch, nil // done, apply patch + r.EventRecorder.Eventf(issuer, corev1.EventTypeNormal, "Checked", "Succeeded checking the issuer") + return result, issuerStatusPatch, nil // done, apply patch } // SetupWithManager sets up the controller with the Manager. diff --git a/controllers/predicates.go b/controllers/predicates.go index d10582a..5e9eef4 100644 --- a/controllers/predicates.go +++ b/controllers/predicates.go @@ -57,7 +57,7 @@ func (CertificateRequestPredicate) Update(e event.UpdateEvent) bool { } for _, oldCond := range oldCr.Status.Conditions { - if oldCond.Type == "Ready" { + if oldCond.Type == cmapi.CertificateRequestConditionReady { // we can skip the Ready conditions continue } diff --git a/internal/testsetups/simple/testutil/api_gen.go b/internal/testsetups/simple/testutil/api_gen.go index 51d7cff..d4adc33 100644 --- a/internal/testsetups/simple/testutil/api_gen.go +++ b/internal/testsetups/simple/testutil/api_gen.go @@ -70,6 +70,7 @@ func SetSimpleIssuerStatusCondition( ) SimpleIssuerModifier { return func(si *api.SimpleIssuer) { conditions.SetIssuerStatusCondition( + si.Status.Conditions, &si.Status.Conditions, si.Generation, conditionType, @@ -119,6 +120,7 @@ func SetSimpleClusterIssuerStatusCondition( ) SimpleClusterIssuerModifier { return func(si *api.SimpleClusterIssuer) { conditions.SetIssuerStatusCondition( + si.Status.Conditions, &si.Status.Conditions, si.Generation, conditionType, From 6c8d01eed0228105094c0649df8c941c8f4822b2 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Tue, 30 May 2023 13:53:31 +0200 Subject: [PATCH 2/5] improve fake clock testing Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- conditions/certificaterequest.go | 5 +- conditions/issuer.go | 5 +- controllers/certificaterequest_controller.go | 17 +++- ...caterequest_controller_integration_test.go | 17 ++-- .../certificaterequest_controller_test.go | 82 +++++++++++-------- controllers/combined_controller.go | 12 ++- .../combined_controller_integration_test.go | 4 +- controllers/issuer_controller.go | 10 ++- controllers/issuer_controller_test.go | 34 ++++++-- controllers/predicates_test.go | 17 ++++ internal/tests/cmtime/cm_time.go | 29 ------- .../testsetups/simple/testutil/api_gen.go | 5 ++ 12 files changed, 149 insertions(+), 88 deletions(-) delete mode 100644 internal/tests/cmtime/cm_time.go diff --git a/conditions/certificaterequest.go b/conditions/certificaterequest.go index 7682580..bbc135f 100644 --- a/conditions/certificaterequest.go +++ b/conditions/certificaterequest.go @@ -17,15 +17,16 @@ limitations under the License. package conditions import ( - "github.com/cert-manager/cert-manager/pkg/api/util" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/clock" ) // Update the status with the provided condition details & return // the added condition. func SetCertificateRequestStatusCondition( + clock clock.PassiveClock, existingConditions []cmapi.CertificateRequestCondition, patchConditions *[]cmapi.CertificateRequestCondition, conditionType cmapi.CertificateRequestConditionType, @@ -39,7 +40,7 @@ func SetCertificateRequestStatusCondition( Message: message, } - nowTime := metav1.NewTime(util.Clock.Now()) + nowTime := metav1.NewTime(clock.Now()) newCondition.LastTransitionTime = &nowTime // Reset the LastTransitionTime if the status hasn't changed diff --git a/conditions/issuer.go b/conditions/issuer.go index eef20f3..b62d94b 100644 --- a/conditions/issuer.go +++ b/conditions/issuer.go @@ -17,15 +17,16 @@ limitations under the License. package conditions import ( - "github.com/cert-manager/cert-manager/pkg/api/util" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/clock" ) // Update the status with the provided condition details & return // the added condition. func SetIssuerStatusCondition( + clock clock.PassiveClock, existingConditions []cmapi.IssuerCondition, patchConditions *[]cmapi.IssuerCondition, observedGeneration int64, @@ -41,7 +42,7 @@ func SetIssuerStatusCondition( ObservedGeneration: observedGeneration, } - nowTime := metav1.NewTime(util.Clock.Now()) + nowTime := metav1.NewTime(clock.Now()) newCondition.LastTransitionTime = &nowTime // Reset the LastTransitionTime if the status hasn't changed diff --git a/controllers/certificaterequest_controller.go b/controllers/certificaterequest_controller.go index d9ca11f..dd11d70 100644 --- a/controllers/certificaterequest_controller.go +++ b/controllers/certificaterequest_controller.go @@ -33,6 +33,7 @@ import ( "k8s.io/apimachinery/pkg/types" utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/tools/record" + "k8s.io/utils/clock" "k8s.io/utils/pointer" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" @@ -62,9 +63,12 @@ type CertificateRequestReconciler struct { // Sign connects to a CA and returns a signed certificate for the supplied CertificateRequest. signer.Sign - // recorder is used for creating Kubernetes events on resources. + // EventRecorder is used for creating Kubernetes events on resources. EventRecorder record.EventRecorder + // Clock is used to mock condition transition times in tests. + Clock clock.PassiveClock + PostSetupWithManager func(context.Context, schema.GroupVersionKind, ctrl.Manager, controller.Controller) error } @@ -168,6 +172,7 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( if ready := cmutil.GetCertificateRequestCondition(&cr, cmapi.CertificateRequestConditionReady); ready == nil { logger.V(1).Info("Initializing Ready condition") conditions.SetCertificateRequestStatusCondition( + r.Clock, cr.Status.Conditions, &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, @@ -184,6 +189,7 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( if cmutil.CertificateRequestIsDenied(&cr) { logger.V(1).Info("CertificateRequest has been denied. Marking as failed.") condition := conditions.SetCertificateRequestStatusCondition( + r.Clock, cr.Status.Conditions, &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, @@ -199,6 +205,7 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( if err := r.Client.Get(ctx, issuerName, issuerObject); err != nil && apierrors.IsNotFound(err) { logger.V(1).Info("Issuer not found. Waiting for it to be created") conditions.SetCertificateRequestStatusCondition( + r.Clock, cr.Status.Conditions, &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, @@ -232,6 +239,7 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( logger.V(1).Info("Issuer is not Ready yet. Waiting for it to become ready.", "issuer ready condition", readyCondition) conditions.SetCertificateRequestStatusCondition( + r.Clock, cr.Status.Conditions, &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, @@ -257,6 +265,7 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( logger.V(1).Error(err, "Temporary CertificateRequest error.") conditions.SetCertificateRequestStatusCondition( + r.Clock, cr.Status.Conditions, &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, @@ -272,6 +281,7 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( if targetCustom := new(signer.SetCertificateRequestConditionError); errors.As(err, targetCustom) { logger.V(1).Info("Set CertificateRequestCondition error. Setting condition.", "error", err) conditions.SetCertificateRequestStatusCondition( + r.Clock, cr.Status.Conditions, &crStatusPatch.Conditions, targetCustom.ConditionType, @@ -288,11 +298,12 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( // Check if we have still time to requeue & retry isPendingError := errors.As(err, &signer.PendingError{}) isPermanentError := errors.As(err, &signer.PermanentError{}) - pastMaxRetryDuration := cmutil.Clock.Now().After(cr.CreationTimestamp.Add(r.MaxRetryDuration)) + pastMaxRetryDuration := r.Clock.Now().After(cr.CreationTimestamp.Add(r.MaxRetryDuration)) if !isPendingError && (isPermanentError || pastMaxRetryDuration) { // fail permanently logger.V(1).Error(err, "Permanent CertificateRequest error. Marking as failed.") condition := conditions.SetCertificateRequestStatusCondition( + r.Clock, cr.Status.Conditions, &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, @@ -307,6 +318,7 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( // retry logger.V(1).Error(err, "Retryable CertificateRequest error.") conditions.SetCertificateRequestStatusCondition( + r.Clock, cr.Status.Conditions, &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, @@ -336,6 +348,7 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( crStatusPatch.Certificate = signedCertificate conditions.SetCertificateRequestStatusCondition( + r.Clock, cr.Status.Conditions, &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, diff --git a/controllers/certificaterequest_controller_integration_test.go b/controllers/certificaterequest_controller_integration_test.go index 3d3b765..68c3783 100644 --- a/controllers/certificaterequest_controller_integration_test.go +++ b/controllers/certificaterequest_controller_integration_test.go @@ -35,6 +35,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/tools/record" + "k8s.io/utils/clock" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -88,6 +89,7 @@ func TestCertificateRequestControllerIntegrationIssuerInitiallyNotFoundAndNotRea return []byte("ok"), nil }, EventRecorder: record.NewFakeRecorder(100), + Clock: clock.RealClock{}, } }, ) @@ -141,7 +143,7 @@ func TestCertificateRequestControllerIntegrationIssuerInitiallyNotFoundAndNotRea checkComplete := kubeClients.StartObjectWatch(t, ctx, cr) t.Log("Creating & approving the CertificateRequest") - createApprovedCR(t, ctx, kubeClients.Client, cr) + createApprovedCR(t, ctx, kubeClients.Client, clock.RealClock{}, cr) t.Log("Waiting for controller to mark the CertificateRequest as IssuerNotFound") err := checkComplete(func(obj runtime.Object) error { readyCondition := cmutil.GetCertificateRequestCondition(obj.(*cmapi.CertificateRequest), cmapi.CertificateRequestConditionReady) @@ -177,7 +179,7 @@ func TestCertificateRequestControllerIntegrationIssuerInitiallyNotFoundAndNotRea checkComplete = kubeClients.StartObjectWatch(t, ctx, cr) t.Log("Marking the Issuer as ready to trigger the controller to re-reconcile the CertificateRequest") - markIssuerReady(t, ctx, kubeClients.Client, fieldOwner, issuer) + markIssuerReady(t, ctx, kubeClients.Client, clock.RealClock{}, fieldOwner, issuer) t.Log("Waiting for the controller to marks the CertificateRequest as Ready") err = checkComplete(func(obj runtime.Object) error { readyCondition := cmutil.GetCertificateRequestCondition(obj.(*cmapi.CertificateRequest), cmapi.CertificateRequestConditionReady) @@ -227,6 +229,7 @@ func TestCertificateRequestControllerIntegrationSetCondition(t *testing.T) { } }, EventRecorder: record.NewFakeRecorder(100), + Clock: clock.RealClock{}, } }, ) @@ -255,7 +258,7 @@ func TestCertificateRequestControllerIntegrationSetCondition(t *testing.T) { checkComplete := kubeClients.StartObjectWatch(t, ctx, cr) t.Log("Creating & approving the CertificateRequest") - createApprovedCR(t, ctx, kubeClients.Client, cr) + createApprovedCR(t, ctx, kubeClients.Client, clock.RealClock{}, cr) t.Log("Waiting for controller to mark the CertificateRequest as IssuerNotFound") err := checkComplete(func(obj runtime.Object) error { readyCondition := cmutil.GetCertificateRequestCondition(obj.(*cmapi.CertificateRequest), cmapi.CertificateRequestConditionReady) @@ -291,7 +294,7 @@ func TestCertificateRequestControllerIntegrationSetCondition(t *testing.T) { t.Log("Marking the Issuer as ready to trigger the controller to re-reconcile the CertificateRequest") - markIssuerReady(t, ctx, kubeClients.Client, fieldOwner, issuer) + markIssuerReady(t, ctx, kubeClients.Client, clock.RealClock{}, fieldOwner, issuer) checkComplete = kubeClients.StartObjectWatch(t, ctx, cr) signResult <- signer.SetCertificateRequestConditionError{ @@ -355,11 +358,12 @@ func TestCertificateRequestControllerIntegrationSetCondition(t *testing.T) { require.Equal(t, uint64(3), atomic.LoadUint64(&counter)) } -func createApprovedCR(t *testing.T, ctx context.Context, kc client.Client, cr *cmapi.CertificateRequest) { +func createApprovedCR(t *testing.T, ctx context.Context, kc client.Client, clock clock.PassiveClock, cr *cmapi.CertificateRequest) { t.Helper() require.NoError(t, kc.Create(ctx, cr)) conditions.SetCertificateRequestStatusCondition( + clock, cr.Status.Conditions, &cr.Status.Conditions, cmapi.CertificateRequestConditionApproved, @@ -395,11 +399,12 @@ func createIssuerForCR(t *testing.T, ctx context.Context, kc client.Client, cr * return issuer } -func markIssuerReady(t *testing.T, ctx context.Context, kc client.Client, fieldOwner string, issuer v1alpha1.Issuer) { +func markIssuerReady(t *testing.T, ctx context.Context, kc client.Client, clock clock.PassiveClock, fieldOwner string, issuer v1alpha1.Issuer) { t.Helper() issuerStatus := &v1alpha1.IssuerStatus{} conditions.SetIssuerStatusCondition( + clock, issuerStatus.Conditions, &issuerStatus.Conditions, issuer.GetGeneration(), diff --git a/controllers/certificaterequest_controller_test.go b/controllers/certificaterequest_controller_test.go index 9155601..effc4e4 100644 --- a/controllers/certificaterequest_controller_test.go +++ b/controllers/certificaterequest_controller_test.go @@ -33,6 +33,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" + clocktesting "k8s.io/utils/clock/testing" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -41,7 +42,6 @@ import ( "github.com/cert-manager/issuer-lib/conditions" "github.com/cert-manager/issuer-lib/controllers/signer" "github.com/cert-manager/issuer-lib/internal/kubeutil" - "github.com/cert-manager/issuer-lib/internal/tests/cmtime" "github.com/cert-manager/issuer-lib/internal/tests/errormatch" "github.com/cert-manager/issuer-lib/internal/tests/ptr" "github.com/cert-manager/issuer-lib/internal/testsetups/simple/api" @@ -63,13 +63,20 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { expectedEvents []string } - fakeTimeObj := metav1.NewTime(cmtime.FakeTime) + fakeTime1 := time.Now().Truncate(time.Second) + fakeTimeObj1 := metav1.NewTime(fakeTime1) + fakeClock1 := clocktesting.NewFakeClock(fakeTime1) + + fakeTime2 := time.Now().Add(4 * time.Hour).Truncate(time.Second) + fakeTimeObj2 := metav1.NewTime(fakeTime2) + fakeClock2 := clocktesting.NewFakeClock(fakeTime2) issuer1 := testutil.SimpleIssuer( "issuer-1", testutil.SetSimpleIssuerNamespace("ns1"), testutil.SetSimpleIssuerGeneration(70), testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, @@ -81,6 +88,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { "cluster-issuer-1", testutil.SetSimpleClusterIssuerGeneration(70), testutil.SetSimpleClusterIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, @@ -92,13 +100,14 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { "cr1", cmgen.SetCertificateRequestNamespace("ns1"), func(cr *cmapi.CertificateRequest) { - cr.CreationTimestamp = fakeTimeObj + cr.CreationTimestamp = fakeTimeObj1 }, cmgen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ Group: api.SchemeGroupVersion.Group, }), func(cr *cmapi.CertificateRequest) { conditions.SetCertificateRequestStatusCondition( + fakeClock1, cr.Status.Conditions, &cr.Status.Conditions, cmapi.CertificateRequestConditionReady, @@ -107,6 +116,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { fieldOwner+" has begun reconciling this CertificateRequest", ) conditions.SetCertificateRequestStatusCondition( + fakeClock1, cr.Status.Conditions, &cr.Status.Conditions, cmapi.CertificateRequestConditionApproved, @@ -221,7 +231,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionUnknown, Reason: v1alpha1.CertificateRequestConditionReasonInitializing, Message: fieldOwner + " has started reconciling this CertificateRequest", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -244,10 +254,10 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonDenied, Message: "The CertificateRequest was denied by an approval controller", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, - FailureTime: &fakeTimeObj, + FailureTime: &fakeTimeObj2, }, expectedEvents: []string{ "Normal DetectedDenied Detected that the CR is denied, will update Ready condition", @@ -270,7 +280,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, Message: "simpleissuers.testing.cert-manager.io \"issuer-1\" not found. Waiting for it to be created.", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -303,7 +313,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, Message: "Issuer is not Ready yet. No ready condition found. Waiting for it to become ready.", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -324,6 +334,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { ), testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionFalse, "[REASON]", @@ -338,7 +349,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, Message: "Issuer is not Ready yet. Current ready condition is \"[REASON]\": [MESSAGE]. Waiting for it to become ready.", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -369,7 +380,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, Message: "Issuer is not Ready yet. Current ready condition is outdated. Waiting for it to become ready.", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -404,10 +415,10 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonFailed, Message: "CertificateRequest has failed permanently: a specific error", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, - FailureTime: &fakeTimeObj, + FailureTime: &fakeTimeObj2, }, expectedEvents: []string{ "Warning PermanentError Failed permanently to sign CertificateRequest: a specific error", @@ -445,7 +456,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, Message: "CertificateRequest is not ready yet: pending error", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -489,14 +500,14 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: "[reason]", Message: "test error", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, { Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, Message: "CertificateRequest is not ready yet: test error", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -532,7 +543,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: "[reason]", Message: "test error", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }), ), testutil.SimpleIssuerFrom(issuer1), @@ -549,14 +560,14 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: "[reason]", Message: "test error2", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, { Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, Message: "CertificateRequest is not ready yet: test error2", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -603,17 +614,17 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: "[reason]", Message: "test error", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, { Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonFailed, Message: "CertificateRequest has failed permanently: test error", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, - FailureTime: &fakeTimeObj, + FailureTime: &fakeTimeObj2, }, expectedEvents: []string{ "Warning PermanentError Failed permanently to sign CertificateRequest: test error", @@ -650,7 +661,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: "[reason]", Message: "test error", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj1, }), ), testutil.SimpleIssuerFrom(issuer1), @@ -664,17 +675,17 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: "[reason]", Message: "test error2", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj1, // since the status is not updated, the LastTransitionTime is not updated either }, { Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonFailed, Message: "CertificateRequest has failed permanently: test error2", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, - FailureTime: &fakeTimeObj, + FailureTime: &fakeTimeObj2, }, expectedEvents: []string{ "Warning PermanentError Failed permanently to sign CertificateRequest: test error2", @@ -718,14 +729,14 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: "[reason]", Message: "test error", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, { Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, Message: "CertificateRequest is not ready yet: test error", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -768,17 +779,17 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: "[reason]", Message: "test error", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, { Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonFailed, Message: "CertificateRequest has failed permanently: test error", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, - FailureTime: &fakeTimeObj, + FailureTime: &fakeTimeObj2, }, expectedEvents: []string{ "Warning PermanentError Failed permanently to sign CertificateRequest: test error", @@ -807,10 +818,10 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonFailed, Message: "CertificateRequest has failed permanently: a specific error", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, - FailureTime: &fakeTimeObj, + FailureTime: &fakeTimeObj2, }, expectedEvents: []string{ "Warning PermanentError Failed permanently to sign CertificateRequest: a specific error", @@ -843,7 +854,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, Message: "CertificateRequest is not ready yet: waiting for approval", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -870,7 +881,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: cmapi.CertificateRequestReasonIssued, Message: "issued", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -897,7 +908,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: cmapi.CertificateRequestReasonIssued, Message: "issued", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -943,6 +954,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Client: fakeClient, Sign: tc.sign, EventRecorder: fakeRecorder, + Clock: fakeClock2, } err = controller.setIssuersGroupVersionKind(scheme) diff --git a/controllers/combined_controller.go b/controllers/combined_controller.go index 6266d6f..e51e32b 100644 --- a/controllers/combined_controller.go +++ b/controllers/combined_controller.go @@ -23,6 +23,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/tools/record" + "k8s.io/utils/clock" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/controller" @@ -44,9 +45,12 @@ type CombinedController struct { // Sign connects to a CA and returns a signed certificate for the supplied CertificateRequest. signer.Sign - // recorder is used for creating Kubernetes events on resources. + // EventRecorder is used for creating Kubernetes events on resources. EventRecorder record.EventRecorder + // Clock is used to mock condition transition times in tests. + Clock clock.PassiveClock + PostSetupWithManager func(context.Context, schema.GroupVersionKind, ctrl.Manager, controller.Controller) error } @@ -55,6 +59,10 @@ func (r *CombinedController) SetupWithManager(ctx context.Context, mgr ctrl.Mana cl := mgr.GetClient() eventSource := kubeutil.NewEventStore() + if r.Clock == nil { + r.Clock = clock.RealClock{} + } + for _, issuerType := range append(r.IssuerTypes, r.ClusterIssuerTypes...) { if err = (&IssuerReconciler{ ForObject: issuerType, @@ -65,6 +73,7 @@ func (r *CombinedController) SetupWithManager(ctx context.Context, mgr ctrl.Mana Client: cl, Check: r.Check, EventRecorder: r.EventRecorder, + Clock: r.Clock, PostSetupWithManager: r.PostSetupWithManager, }).SetupWithManager(ctx, mgr); err != nil { @@ -83,6 +92,7 @@ func (r *CombinedController) SetupWithManager(ctx context.Context, mgr ctrl.Mana Client: cl, Sign: r.Sign, EventRecorder: r.EventRecorder, + Clock: r.Clock, PostSetupWithManager: r.PostSetupWithManager, }).SetupWithManager(ctx, mgr); err != nil { diff --git a/controllers/combined_controller_integration_test.go b/controllers/combined_controller_integration_test.go index c196d5b..a4636be 100644 --- a/controllers/combined_controller_integration_test.go +++ b/controllers/combined_controller_integration_test.go @@ -30,6 +30,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/tools/record" + "k8s.io/utils/clock" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -139,6 +140,7 @@ func TestCombinedControllerTemporaryFailedCertificateRequestRetrigger(t *testing testutil.SetSimpleIssuerNamespace(namespace), testutil.SetSimpleIssuerGeneration(70), testutil.SetSimpleIssuerStatusCondition( + clock.RealClock{}, cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, @@ -177,7 +179,7 @@ func TestCombinedControllerTemporaryFailedCertificateRequestRetrigger(t *testing }, watch.Added, watch.Modified) require.NoError(t, err) - createApprovedCR(t, ctx, kubeClients.Client, cr) + createApprovedCR(t, ctx, kubeClients.Client, clock.RealClock{}, cr) checkCr1Complete := kubeClients.StartObjectWatch(t, ctx, cr) checkCr2Complete := kubeClients.StartObjectWatch(t, ctx, cr) diff --git a/controllers/issuer_controller.go b/controllers/issuer_controller.go index e941869..c36f3fa 100644 --- a/controllers/issuer_controller.go +++ b/controllers/issuer_controller.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/tools/record" + "k8s.io/utils/clock" "k8s.io/utils/pointer" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" @@ -56,9 +57,12 @@ type IssuerReconciler struct { // Check connects to a CA and checks if it is available signer.Check - // recorder is used for creating Kubernetes events on resources. + // EventRecorder is used for creating Kubernetes events on resources. EventRecorder record.EventRecorder + // Clock is used to mock condition transition times in tests. + Clock clock.PassiveClock + PostSetupWithManager func(context.Context, schema.GroupVersionKind, ctrl.Manager, controller.Controller) error } @@ -134,6 +138,7 @@ func (r *IssuerReconciler) reconcileStatusPatch( if readyCondition == nil { logger.V(1).Info("Initializing Ready condition") conditions.SetIssuerStatusCondition( + r.Clock, issuer.GetStatus().Conditions, &issuerStatusPatch.Conditions, issuer.GetGeneration(), @@ -162,6 +167,7 @@ func (r *IssuerReconciler) reconcileStatusPatch( // fail permanently logger.V(1).Error(err, "Permanent Issuer error. Marking as failed.") conditions.SetIssuerStatusCondition( + r.Clock, issuer.GetStatus().Conditions, &issuerStatusPatch.Conditions, issuer.GetGeneration(), @@ -176,6 +182,7 @@ func (r *IssuerReconciler) reconcileStatusPatch( // retry logger.V(1).Error(err, "Retryable Issuer error.") conditions.SetIssuerStatusCondition( + r.Clock, issuer.GetStatus().Conditions, &issuerStatusPatch.Conditions, issuer.GetGeneration(), @@ -199,6 +206,7 @@ func (r *IssuerReconciler) reconcileStatusPatch( } conditions.SetIssuerStatusCondition( + r.Clock, issuer.GetStatus().Conditions, &issuerStatusPatch.Conditions, issuer.GetGeneration(), diff --git a/controllers/issuer_controller_test.go b/controllers/issuer_controller_test.go index 58c7b0a..72901d9 100644 --- a/controllers/issuer_controller_test.go +++ b/controllers/issuer_controller_test.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "testing" + "time" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" @@ -31,6 +32,7 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" + clocktesting "k8s.io/utils/clock/testing" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -38,7 +40,6 @@ import ( "github.com/cert-manager/issuer-lib/api/v1alpha1" "github.com/cert-manager/issuer-lib/controllers/signer" - "github.com/cert-manager/issuer-lib/internal/tests/cmtime" "github.com/cert-manager/issuer-lib/internal/tests/errormatch" "github.com/cert-manager/issuer-lib/internal/tests/ptr" "github.com/cert-manager/issuer-lib/internal/testsetups/simple/api" @@ -61,7 +62,13 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { expectedEvents []string } - fakeTimeObj := metav1.NewTime(cmtime.FakeTime) + fakeTime1 := time.Now().Truncate(time.Second) + fakeTimeObj1 := metav1.NewTime(fakeTime1) + fakeClock1 := clocktesting.NewFakeClock(fakeTime1) + + fakeTime2 := time.Now().Add(4 * time.Hour).Truncate(time.Second) + fakeTimeObj2 := metav1.NewTime(fakeTime2) + fakeClock2 := clocktesting.NewFakeClock(fakeTime2) issuer1 := testutil.SimpleIssuer( "issuer-1", @@ -91,6 +98,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerGeneration(80), testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, @@ -106,7 +114,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { Reason: v1alpha1.IssuerConditionReasonChecked, Message: "checked", ObservedGeneration: 80, - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj1, // since the status is not updated, the LastTransitionTime is not updated either }, }, }, @@ -123,6 +131,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerGeneration(80), testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionFalse, v1alpha1.IssuerConditionReasonFailed, @@ -141,6 +150,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerGeneration(80), testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionFalse, v1alpha1.IssuerConditionReasonFailed, @@ -160,6 +170,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerGeneration(80), testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, @@ -176,7 +187,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { Reason: v1alpha1.IssuerConditionReasonPending, Message: "Issuer is not ready yet: [specific error]", ObservedGeneration: 80, - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -199,6 +210,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerGeneration(80), testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, @@ -214,7 +226,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: v1alpha1.IssuerConditionReasonChecked, Message: "checked", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj1, // since the status is not updated, the LastTransitionTime is not updated either ObservedGeneration: 81, }, }, @@ -238,7 +250,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionUnknown, Reason: v1alpha1.IssuerConditionReasonInitializing, Message: fieldOwner + " has started reconciling this Issuer", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -251,6 +263,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { objects: []client.Object{ testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionUnknown, v1alpha1.IssuerConditionReasonInitializing, @@ -271,7 +284,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionFalse, Reason: v1alpha1.IssuerConditionReasonPending, Message: "Issuer is not ready yet: a specific error", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -290,6 +303,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { objects: []client.Object{ testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionUnknown, v1alpha1.IssuerConditionReasonInitializing, @@ -304,7 +318,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: v1alpha1.IssuerConditionReasonChecked, Message: "checked", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -321,6 +335,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerGeneration(80), testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionFalse, v1alpha1.IssuerConditionReasonInitializing, @@ -336,7 +351,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: v1alpha1.IssuerConditionReasonChecked, Message: "checked", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, ObservedGeneration: 81, }, }, @@ -382,6 +397,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { Client: fakeClient, Check: tc.check, EventRecorder: fakeRecorder, + Clock: fakeClock2, } res, crsPatch, err := controller.reconcileStatusPatch(logger, context.TODO(), req) diff --git a/controllers/predicates_test.go b/controllers/predicates_test.go index a133e09..0ba8c06 100644 --- a/controllers/predicates_test.go +++ b/controllers/predicates_test.go @@ -18,6 +18,7 @@ package controllers_test import ( "testing" + "time" cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" @@ -27,6 +28,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + clocktesting "k8s.io/utils/clock/testing" "sigs.k8s.io/controller-runtime/pkg/event" "github.com/cert-manager/issuer-lib/api/v1alpha1" @@ -230,6 +232,9 @@ func TestLinkedIssuerPredicate(t *testing.T) { issuer1 := testutil.SimpleIssuer("issuer-1") + fakeTime := time.Now() + fakeClock := clocktesting.NewFakeClock(fakeTime) + type testcase struct { name string event event.UpdateEvent @@ -251,6 +256,7 @@ func TestLinkedIssuerPredicate(t *testing.T) { event: event.UpdateEvent{ ObjectOld: testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerStatusCondition( + fakeClock, "random", cmmeta.ConditionFalse, "test1", @@ -259,6 +265,7 @@ func TestLinkedIssuerPredicate(t *testing.T) { ), ObjectNew: testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerStatusCondition( + fakeClock, "random", cmmeta.ConditionTrue, "test2", @@ -274,6 +281,7 @@ func TestLinkedIssuerPredicate(t *testing.T) { ObjectOld: &testissuer{Status: nil}, ObjectNew: testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerStatusCondition( + fakeClock, cmapi.IssuerConditionReady, cmmeta.ConditionFalse, "reason", @@ -289,6 +297,7 @@ func TestLinkedIssuerPredicate(t *testing.T) { ObjectOld: testutil.SimpleIssuerFrom(issuer1), ObjectNew: testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerStatusCondition( + fakeClock, cmapi.IssuerConditionReady, cmmeta.ConditionFalse, "reason", @@ -303,6 +312,7 @@ func TestLinkedIssuerPredicate(t *testing.T) { event: event.UpdateEvent{ ObjectOld: testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerStatusCondition( + fakeClock, cmapi.IssuerConditionReady, cmmeta.ConditionFalse, "reason1", @@ -311,6 +321,7 @@ func TestLinkedIssuerPredicate(t *testing.T) { ), ObjectNew: testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerStatusCondition( + fakeClock, cmapi.IssuerConditionReady, cmmeta.ConditionFalse, "reason2", @@ -325,6 +336,7 @@ func TestLinkedIssuerPredicate(t *testing.T) { event: event.UpdateEvent{ ObjectOld: testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerStatusCondition( + fakeClock, cmapi.IssuerConditionReady, cmmeta.ConditionFalse, "reason", @@ -333,6 +345,7 @@ func TestLinkedIssuerPredicate(t *testing.T) { ), ObjectNew: testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerStatusCondition( + fakeClock, cmapi.IssuerConditionReady, cmmeta.ConditionTrue, "reason", @@ -357,6 +370,9 @@ func TestIssuerPredicate(t *testing.T) { issuer1 := testutil.SimpleIssuer("issuer-1") + fakeTime := time.Now() + fakeClock := clocktesting.NewFakeClock(fakeTime) + type testcase struct { name string event event.UpdateEvent @@ -428,6 +444,7 @@ func TestIssuerPredicate(t *testing.T) { ObjectOld: testutil.SimpleIssuerFrom(issuer1), ObjectNew: testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerStatusCondition( + fakeClock, cmapi.IssuerConditionReady, cmmeta.ConditionFalse, "reason", diff --git a/internal/tests/cmtime/cm_time.go b/internal/tests/cmtime/cm_time.go deleted file mode 100644 index 1337c8c..0000000 --- a/internal/tests/cmtime/cm_time.go +++ /dev/null @@ -1,29 +0,0 @@ -/* -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 cmtime - -import ( - "time" - - cmutil "github.com/cert-manager/cert-manager/pkg/api/util" - clocktesting "k8s.io/utils/clock/testing" -) - -var FakeTime = time.Now() - -// We use an init function to set the fake clock, so that it is set only once -// and the tests don't interfere with each other. -func init() { - cmutil.Clock = clocktesting.NewFakeClock(FakeTime) -} diff --git a/internal/testsetups/simple/testutil/api_gen.go b/internal/testsetups/simple/testutil/api_gen.go index d4adc33..c73ea0a 100644 --- a/internal/testsetups/simple/testutil/api_gen.go +++ b/internal/testsetups/simple/testutil/api_gen.go @@ -20,6 +20,7 @@ import ( cmapi "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/clock" "github.com/cert-manager/issuer-lib/conditions" "github.com/cert-manager/issuer-lib/internal/testsetups/simple/api" @@ -64,12 +65,14 @@ func SetSimpleIssuerGeneration(generation int64) SimpleIssuerModifier { } func SetSimpleIssuerStatusCondition( + clock clock.PassiveClock, conditionType cmapi.IssuerConditionType, status cmmeta.ConditionStatus, reason, message string, ) SimpleIssuerModifier { return func(si *api.SimpleIssuer) { conditions.SetIssuerStatusCondition( + clock, si.Status.Conditions, &si.Status.Conditions, si.Generation, @@ -114,12 +117,14 @@ func SetSimpleClusterIssuerGeneration(generation int64) SimpleClusterIssuerModif } func SetSimpleClusterIssuerStatusCondition( + clock clock.PassiveClock, conditionType cmapi.IssuerConditionType, status cmmeta.ConditionStatus, reason, message string, ) SimpleClusterIssuerModifier { return func(si *api.SimpleClusterIssuer) { conditions.SetIssuerStatusCondition( + clock, si.Status.Conditions, &si.Status.Conditions, si.Generation, From 44d57d4f4c2b5df83d77baa8accc53ce592bca9c Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Tue, 30 May 2023 13:53:39 +0200 Subject: [PATCH 3/5] fix FailureTime value Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- conditions/certificaterequest.go | 6 +++--- conditions/issuer.go | 6 +++--- controllers/certificaterequest_controller.go | 8 ++++---- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/conditions/certificaterequest.go b/conditions/certificaterequest.go index bbc135f..66ee964 100644 --- a/conditions/certificaterequest.go +++ b/conditions/certificaterequest.go @@ -32,7 +32,7 @@ func SetCertificateRequestStatusCondition( conditionType cmapi.CertificateRequestConditionType, status cmmeta.ConditionStatus, reason, message string, -) *cmapi.CertificateRequestCondition { +) (*cmapi.CertificateRequestCondition, *metav1.Time) { newCondition := cmapi.CertificateRequestCondition{ Type: conditionType, Status: status, @@ -66,12 +66,12 @@ func SetCertificateRequestStatusCondition( // Overwrite the existing condition (*patchConditions)[idx] = newCondition - return &newCondition + return &newCondition, &nowTime } // If we've not found an existing condition of this type, we simply insert // the new condition into the slice. *patchConditions = append(*patchConditions, newCondition) - return &newCondition + return &newCondition, &nowTime } diff --git a/conditions/issuer.go b/conditions/issuer.go index b62d94b..56cf230 100644 --- a/conditions/issuer.go +++ b/conditions/issuer.go @@ -33,7 +33,7 @@ func SetIssuerStatusCondition( conditionType cmapi.IssuerConditionType, status cmmeta.ConditionStatus, reason, message string, -) *cmapi.IssuerCondition { +) (*cmapi.IssuerCondition, *metav1.Time) { newCondition := cmapi.IssuerCondition{ Type: conditionType, Status: status, @@ -68,14 +68,14 @@ func SetIssuerStatusCondition( // Overwrite the existing condition (*patchConditions)[idx] = newCondition - return &newCondition + return &newCondition, &nowTime } // If we've not found an existing condition of this type, we simply insert // the new condition into the slice. *patchConditions = append(*patchConditions, newCondition) - return &newCondition + return &newCondition, &nowTime } func GetIssuerStatusCondition( diff --git a/controllers/certificaterequest_controller.go b/controllers/certificaterequest_controller.go index dd11d70..a2c2f15 100644 --- a/controllers/certificaterequest_controller.go +++ b/controllers/certificaterequest_controller.go @@ -188,7 +188,7 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( if cmutil.CertificateRequestIsDenied(&cr) { logger.V(1).Info("CertificateRequest has been denied. Marking as failed.") - condition := conditions.SetCertificateRequestStatusCondition( + _, failedAt := conditions.SetCertificateRequestStatusCondition( r.Clock, cr.Status.Conditions, &crStatusPatch.Conditions, @@ -197,7 +197,7 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( cmapi.CertificateRequestReasonDenied, "The CertificateRequest was denied by an approval controller", ) - crStatusPatch.FailureTime = condition.LastTransitionTime.DeepCopy() + crStatusPatch.FailureTime = failedAt.DeepCopy() r.EventRecorder.Eventf(&cr, corev1.EventTypeNormal, "DetectedDenied", "Detected that the CR is denied, will update Ready condition") return result, crStatusPatch, nil // done, apply patch } @@ -302,7 +302,7 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( if !isPendingError && (isPermanentError || pastMaxRetryDuration) { // fail permanently logger.V(1).Error(err, "Permanent CertificateRequest error. Marking as failed.") - condition := conditions.SetCertificateRequestStatusCondition( + _, failedAt := conditions.SetCertificateRequestStatusCondition( r.Clock, cr.Status.Conditions, &crStatusPatch.Conditions, @@ -311,7 +311,7 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( cmapi.CertificateRequestReasonFailed, fmt.Sprintf("CertificateRequest has failed permanently: %s", err), ) - crStatusPatch.FailureTime = condition.LastTransitionTime.DeepCopy() + crStatusPatch.FailureTime = failedAt.DeepCopy() r.EventRecorder.Eventf(&cr, corev1.EventTypeWarning, "PermanentError", "Failed permanently to sign CertificateRequest: %s", err) return result, crStatusPatch, nil // done, apply patch } else { From a6fa208815cbbb4de214c3d1d02403e82e481f41 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Tue, 30 May 2023 14:22:56 +0200 Subject: [PATCH 4/5] update fake creationtime logic Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- .../certificaterequest_controller_test.go | 32 ++++++++++++------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/controllers/certificaterequest_controller_test.go b/controllers/certificaterequest_controller_test.go index effc4e4..716b58f 100644 --- a/controllers/certificaterequest_controller_test.go +++ b/controllers/certificaterequest_controller_test.go @@ -99,9 +99,6 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { cr1 := cmgen.CertificateRequest( "cr1", cmgen.SetCertificateRequestNamespace("ns1"), - func(cr *cmapi.CertificateRequest) { - cr.CreationTimestamp = fakeTimeObj1 - }, cmgen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ Group: api.SchemeGroupVersion.Group, }), @@ -403,7 +400,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Group: api.SchemeGroupVersion.Group, }), func(cr *cmapi.CertificateRequest) { - cr.CreationTimestamp.Time = cr.CreationTimestamp.Time.Add(-2 * time.Minute) + cr.CreationTimestamp = metav1.NewTime(fakeTimeObj2.Add(-2 * time.Minute)) }, ), testutil.SimpleIssuerFrom(issuer1), @@ -439,7 +436,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Group: api.SchemeGroupVersion.Group, }), func(cr *cmapi.CertificateRequest) { - cr.CreationTimestamp.Time = cr.CreationTimestamp.Time.Add(-2 * time.Minute) + cr.CreationTimestamp = metav1.NewTime(fakeTimeObj2.Add(-2 * time.Minute)) }, ), testutil.SimpleIssuerFrom(issuer1), @@ -483,6 +480,9 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { }, objects: []client.Object{ cmgen.CertificateRequestFrom(cr1, + func(cr *cmapi.CertificateRequest) { + cr.CreationTimestamp = fakeTimeObj2 + }, cmgen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ Name: issuer1.Name, Group: api.SchemeGroupVersion.Group, @@ -534,6 +534,9 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { }, objects: []client.Object{ cmgen.CertificateRequestFrom(cr1, + func(cr *cmapi.CertificateRequest) { + cr.CreationTimestamp = fakeTimeObj2 + }, cmgen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ Name: issuer1.Name, Group: api.SchemeGroupVersion.Group, @@ -599,7 +602,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Group: api.SchemeGroupVersion.Group, }), func(cr *cmapi.CertificateRequest) { - cr.CreationTimestamp.Time = cr.CreationTimestamp.Time.Add(-2 * time.Minute) + cr.CreationTimestamp = metav1.NewTime(fakeTimeObj2.Add(-2 * time.Minute)) }, ), testutil.SimpleIssuerFrom(issuer1), @@ -654,7 +657,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Group: api.SchemeGroupVersion.Group, }), func(cr *cmapi.CertificateRequest) { - cr.CreationTimestamp.Time = cr.CreationTimestamp.Time.Add(-2 * time.Minute) + cr.CreationTimestamp = metav1.NewTime(fakeTimeObj2.Add(-2 * time.Minute)) }, cmgen.AddCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ Type: "[condition type]", @@ -714,7 +717,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Group: api.SchemeGroupVersion.Group, }), func(cr *cmapi.CertificateRequest) { - cr.CreationTimestamp.Time = cr.CreationTimestamp.Time.Add(-2 * time.Minute) + cr.CreationTimestamp = metav1.NewTime(fakeTimeObj2.Add(-2 * time.Minute)) }, ), testutil.SimpleIssuerFrom(issuer1), @@ -836,10 +839,15 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { return nil, errors.New("waiting for approval") }, objects: []client.Object{ - cmgen.CertificateRequestFrom(cr1, func(cr *cmapi.CertificateRequest) { - cr.Spec.IssuerRef.Name = issuer1.Name - cr.Spec.IssuerRef.Kind = issuer1.Kind - }), + cmgen.CertificateRequestFrom(cr1, + func(cr *cmapi.CertificateRequest) { + cr.CreationTimestamp = fakeTimeObj2 + }, + func(cr *cmapi.CertificateRequest) { + cr.Spec.IssuerRef.Name = issuer1.Name + cr.Spec.IssuerRef.Kind = issuer1.Kind + }, + ), testutil.SimpleIssuerFrom(issuer1), }, // instead of returning an error, we trigger a new reconciliation by setting requeue=true From 642ff1721821861b708d77c41bb90a5ad5c8c623 Mon Sep 17 00:00:00 2001 From: Tim Ramlot <42113979+inteon@users.noreply.github.com> Date: Tue, 30 May 2023 16:20:25 +0200 Subject: [PATCH 5/5] use randomTime Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com> --- .../certificaterequest_controller_test.go | 6 ++++-- controllers/issuer_controller_test.go | 21 +++++++++++++++++-- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/controllers/certificaterequest_controller_test.go b/controllers/certificaterequest_controller_test.go index 716b58f..c6555dd 100644 --- a/controllers/certificaterequest_controller_test.go +++ b/controllers/certificaterequest_controller_test.go @@ -63,11 +63,13 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { expectedEvents []string } - fakeTime1 := time.Now().Truncate(time.Second) + randTime := randomTime() + + fakeTime1 := randTime.Truncate(time.Second) fakeTimeObj1 := metav1.NewTime(fakeTime1) fakeClock1 := clocktesting.NewFakeClock(fakeTime1) - fakeTime2 := time.Now().Add(4 * time.Hour).Truncate(time.Second) + fakeTime2 := randTime.Add(4 * time.Hour).Truncate(time.Second) fakeTimeObj2 := metav1.NewTime(fakeTime2) fakeClock2 := clocktesting.NewFakeClock(fakeTime2) diff --git a/controllers/issuer_controller_test.go b/controllers/issuer_controller_test.go index 72901d9..4252d87 100644 --- a/controllers/issuer_controller_test.go +++ b/controllers/issuer_controller_test.go @@ -19,6 +19,7 @@ package controllers import ( "context" "fmt" + "math/rand" "testing" "time" @@ -46,6 +47,20 @@ import ( "github.com/cert-manager/issuer-lib/internal/testsetups/simple/testutil" ) +// We are using a random time generator to generate random times for the +// fakeClock. This will result in different times for each test run and +// should make sure we don't incorrectly rely on `time.Now()` in the code. +// WARNING: This approach does not guarantee that incorrect use of `time.Now()` +// is always detected, but after a few test runs it should be very unlikely. +func randomTime() time.Time { + min := time.Date(1970, 1, 0, 0, 0, 0, 0, time.UTC).Unix() + max := time.Date(2070, 1, 0, 0, 0, 0, 0, time.UTC).Unix() + delta := max - min + + sec := rand.Int63n(delta) + min + return time.Unix(sec, 0) +} + func TestSimpleIssuerReconcilerReconcile(t *testing.T) { t.Parallel() @@ -62,11 +77,13 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { expectedEvents []string } - fakeTime1 := time.Now().Truncate(time.Second) + randTime := randomTime() + + fakeTime1 := randTime.Truncate(time.Second) fakeTimeObj1 := metav1.NewTime(fakeTime1) fakeClock1 := clocktesting.NewFakeClock(fakeTime1) - fakeTime2 := time.Now().Add(4 * time.Hour).Truncate(time.Second) + fakeTime2 := randTime.Add(4 * time.Hour).Truncate(time.Second) fakeTimeObj2 := metav1.NewTime(fakeTime2) fakeClock2 := clocktesting.NewFakeClock(fakeTime2)