diff --git a/conditions/certificaterequest.go b/conditions/certificaterequest.go index 5b565ab..66ee964 100644 --- a/conditions/certificaterequest.go +++ b/conditions/certificaterequest.go @@ -17,30 +17,61 @@ limitations under the License. package conditions import ( - cmutil "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. -// NOTE: this code is just a workaround for cmutil only accepting the certificaterequest object func SetCertificateRequestStatusCondition( - conditions *[]cmapi.CertificateRequestCondition, + clock clock.PassiveClock, + 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, - }, +) (*cmapi.CertificateRequestCondition, *metav1.Time) { + 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(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, &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, &nowTime } diff --git a/conditions/issuer.go b/conditions/issuer.go index cc40d2c..56cf230 100644 --- a/conditions/issuer.go +++ b/conditions/issuer.go @@ -17,59 +17,65 @@ limitations under the License. package conditions import ( - cmutil "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" + "k8s.io/utils/clock" ) -// 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, + clock clock.PassiveClock, + 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, - }, +) (*cmapi.IssuerCondition, *metav1.Time) { + newCondition := cmapi.IssuerCondition{ + Type: conditionType, + Status: status, + Reason: reason, + Message: message, + ObservedGeneration: observedGeneration, } - cmutil.SetIssuerCondition(&gi, observedGeneration, conditionType, status, reason, message) + nowTime := metav1.NewTime(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, &nowTime + } - *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, &nowTime } func GetIssuerStatusCondition( diff --git a/controllers/certificaterequest_controller.go b/controllers/certificaterequest_controller.go index 63fd77a..a2c2f15 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 } @@ -73,10 +77,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 +109,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 +165,16 @@ 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, + r.Clock, + cr.Status.Conditions, + &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, cmmeta.ConditionUnknown, v1alpha1.CertificateRequestConditionReasonInitializing, @@ -177,34 +183,38 @@ 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, + _, failedAt := conditions.SetCertificateRequestStatusCondition( + r.Clock, + 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 = failedAt.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, + r.Clock, + 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 +239,16 @@ 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, + r.Clock, + 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 +265,15 @@ func (r *CertificateRequestReconciler) reconcileStatusPatch( logger.V(1).Error(err, "Temporary CertificateRequest error.") conditions.SetCertificateRequestStatusCondition( - &crsPatch.Conditions, + r.Clock, + 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 +281,9 @@ 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, + r.Clock, + cr.Status.Conditions, + &crStatusPatch.Conditions, targetCustom.ConditionType, targetCustom.Status, targetCustom.Reason, @@ -282,25 +298,29 @@ 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( - &crsPatch.Conditions, + _, failedAt := conditions.SetCertificateRequestStatusCondition( + r.Clock, + 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 = failedAt.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, + r.Clock, + cr.Status.Conditions, + &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, cmmeta.ConditionFalse, cmapi.CertificateRequestReasonPending, @@ -310,7 +330,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 +341,16 @@ 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, + r.Clock, + cr.Status.Conditions, + &crStatusPatch.Conditions, cmapi.CertificateRequestConditionReady, cmmeta.ConditionTrue, cmapi.CertificateRequestReasonIssued, @@ -337,7 +359,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..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,13 @@ 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, cmmeta.ConditionTrue, @@ -394,11 +399,13 @@ 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(), cmapi.IssuerConditionReady, diff --git a/controllers/certificaterequest_controller_test.go b/controllers/certificaterequest_controller_test.go index 89f4d6e..c6555dd 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,22 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { expectedEvents []string } - fakeTimeObj := metav1.NewTime(cmtime.FakeTime) + randTime := randomTime() + + fakeTime1 := randTime.Truncate(time.Second) + fakeTimeObj1 := metav1.NewTime(fakeTime1) + fakeClock1 := clocktesting.NewFakeClock(fakeTime1) + + fakeTime2 := randTime.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 +90,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { "cluster-issuer-1", testutil.SetSimpleClusterIssuerGeneration(70), testutil.SetSimpleClusterIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, @@ -91,14 +101,13 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { cr1 := cmgen.CertificateRequest( "cr1", cmgen.SetCertificateRequestNamespace("ns1"), - func(cr *cmapi.CertificateRequest) { - cr.CreationTimestamp = fakeTimeObj - }, cmgen.SetCertificateRequestIssuer(cmmeta.ObjectReference{ Group: api.SchemeGroupVersion.Group, }), func(cr *cmapi.CertificateRequest) { conditions.SetCertificateRequestStatusCondition( + fakeClock1, + cr.Status.Conditions, &cr.Status.Conditions, cmapi.CertificateRequestConditionReady, cmmeta.ConditionUnknown, @@ -106,6 +115,8 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { fieldOwner+" has begun reconciling this CertificateRequest", ) conditions.SetCertificateRequestStatusCondition( + fakeClock1, + cr.Status.Conditions, &cr.Status.Conditions, cmapi.CertificateRequestConditionApproved, cmmeta.ConditionTrue, @@ -219,7 +230,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionUnknown, Reason: v1alpha1.CertificateRequestConditionReasonInitializing, Message: fieldOwner + " has started reconciling this CertificateRequest", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -242,10 +253,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", @@ -268,7 +279,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, }, }, }, @@ -301,7 +312,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, }, }, }, @@ -322,6 +333,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { ), testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionFalse, "[REASON]", @@ -336,7 +348,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, }, }, }, @@ -367,7 +379,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, }, }, }, @@ -390,7 +402,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), @@ -402,10 +414,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", @@ -426,7 +438,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), @@ -443,7 +455,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonPending, Message: "CertificateRequest is not ready yet: pending error", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -470,6 +482,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, @@ -487,14 +502,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, }, }, }, @@ -521,6 +536,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, @@ -530,7 +548,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: "[reason]", Message: "test error", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }), ), testutil.SimpleIssuerFrom(issuer1), @@ -547,14 +565,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, }, }, }, @@ -586,7 +604,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), @@ -601,17 +619,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", @@ -641,14 +659,14 @@ 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]", Status: cmmeta.ConditionTrue, Reason: "[reason]", Message: "test error", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj1, }), ), testutil.SimpleIssuerFrom(issuer1), @@ -662,17 +680,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", @@ -701,7 +719,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), @@ -716,14 +734,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, }, }, }, @@ -766,17 +784,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", @@ -805,10 +823,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", @@ -823,10 +841,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 @@ -841,7 +864,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, }, }, }, @@ -868,7 +891,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: cmapi.CertificateRequestReasonIssued, Message: "issued", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -895,7 +918,7 @@ func TestCertificateRequestReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: cmapi.CertificateRequestReasonIssued, Message: "issued", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -941,6 +964,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 68f03a7..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 } @@ -67,10 +71,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 +103,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 +131,17 @@ 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(), + r.Clock, + issuer.GetStatus().Conditions, + &issuerStatusPatch.Conditions, + issuer.GetGeneration(), cmapi.IssuerConditionReady, cmmeta.ConditionUnknown, v1alpha1.IssuerConditionReasonInitializing, @@ -144,7 +150,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 +159,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 +167,31 @@ func (r *IssuerReconciler) reconcileStatusPatch( // fail permanently logger.V(1).Error(err, "Permanent Issuer error. Marking as failed.") conditions.SetIssuerStatusCondition( - &isPatch.Conditions, - vi.GetGeneration(), + r.Clock, + issuer.GetStatus().Conditions, + &issuerStatusPatch.Conditions, + issuer.GetGeneration(), cmapi.IssuerConditionReady, cmmeta.ConditionFalse, v1alpha1.IssuerConditionReasonFailed, fmt.Sprintf("Issuer has failed permanently: %s", err), ) - r.EventRecorder.Eventf(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(), + r.Clock, + issuer.GetStatus().Conditions, + &issuerStatusPatch.Conditions, + issuer.GetGeneration(), cmapi.IssuerConditionReady, cmmeta.ConditionFalse, v1alpha1.IssuerConditionReasonPending, fmt.Sprintf("Issuer is not ready yet: %s", err), ) - r.EventRecorder.Eventf(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 +201,15 @@ 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(), + r.Clock, + issuer.GetStatus().Conditions, + &issuerStatusPatch.Conditions, + issuer.GetGeneration(), cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, @@ -205,8 +217,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/issuer_controller_test.go b/controllers/issuer_controller_test.go index 58c7b0a..4252d87 100644 --- a/controllers/issuer_controller_test.go +++ b/controllers/issuer_controller_test.go @@ -19,7 +19,9 @@ package controllers import ( "context" "fmt" + "math/rand" "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 +33,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,13 +41,26 @@ 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" "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() @@ -61,7 +77,15 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { expectedEvents []string } - fakeTimeObj := metav1.NewTime(cmtime.FakeTime) + randTime := randomTime() + + fakeTime1 := randTime.Truncate(time.Second) + fakeTimeObj1 := metav1.NewTime(fakeTime1) + fakeClock1 := clocktesting.NewFakeClock(fakeTime1) + + fakeTime2 := randTime.Add(4 * time.Hour).Truncate(time.Second) + fakeTimeObj2 := metav1.NewTime(fakeTime2) + fakeClock2 := clocktesting.NewFakeClock(fakeTime2) issuer1 := testutil.SimpleIssuer( "issuer-1", @@ -91,6 +115,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerGeneration(80), testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, @@ -106,7 +131,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 +148,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerGeneration(80), testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionFalse, v1alpha1.IssuerConditionReasonFailed, @@ -141,6 +167,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerGeneration(80), testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionFalse, v1alpha1.IssuerConditionReasonFailed, @@ -160,6 +187,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerGeneration(80), testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, @@ -176,7 +204,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 +227,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerGeneration(80), testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionTrue, v1alpha1.IssuerConditionReasonChecked, @@ -214,7 +243,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 +267,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 +280,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { objects: []client.Object{ testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionUnknown, v1alpha1.IssuerConditionReasonInitializing, @@ -271,7 +301,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 +320,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { objects: []client.Object{ testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionUnknown, v1alpha1.IssuerConditionReasonInitializing, @@ -304,7 +335,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: v1alpha1.IssuerConditionReasonChecked, Message: "checked", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, }, }, }, @@ -321,6 +352,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { testutil.SimpleIssuerFrom(issuer1, testutil.SetSimpleIssuerGeneration(80), testutil.SetSimpleIssuerStatusCondition( + fakeClock1, cmapi.IssuerConditionReady, cmmeta.ConditionFalse, v1alpha1.IssuerConditionReasonInitializing, @@ -336,7 +368,7 @@ func TestSimpleIssuerReconcilerReconcile(t *testing.T) { Status: cmmeta.ConditionTrue, Reason: v1alpha1.IssuerConditionReasonChecked, Message: "checked", - LastTransitionTime: &fakeTimeObj, + LastTransitionTime: &fakeTimeObj2, ObservedGeneration: 81, }, }, @@ -382,6 +414,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.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/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 51d7cff..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,15 @@ 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, conditionType, @@ -113,12 +117,15 @@ 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, conditionType,