diff --git a/pkg/controller/certificates/issuing/issuing_controller.go b/pkg/controller/certificates/issuing/issuing_controller.go index ab0fc05bf..92f66d4ef 100644 --- a/pkg/controller/certificates/issuing/issuing_controller.go +++ b/pkg/controller/certificates/issuing/issuing_controller.go @@ -236,6 +236,24 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { return nil } + certIssuingCond := apiutil.GetCertificateCondition(crt, cmapi.CertificateConditionIssuing) + crReadyCond := apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionReady) + if certIssuingCond == nil { + // This should never happen + log.V(logf.ErrorLevel).Info("Certificate does not have an issuing condition") + return nil + } + // If the CertificateRequest for this revision has failed, but before the + // Issuing condition was set on the Certificate, then it must be a + // failed CertificateRequest from previous issuance for the same + // revision. Leave it to certificate-requests controller to delete the + // CertificateRequest and create a new one. + if req.Status.FailureTime != nil && + req.Status.FailureTime.Before(certIssuingCond.LastTransitionTime) && crReadyCond.Reason == cmapi.CertificateRequestReasonFailed { + log.V(logf.InfoLevel).Info("Found a failed CertificateRequest from previous issuance, waiting for it to be deleted...") + return nil + } + // Some issuers won't honor the "Denied=True" condition, and we don't want // to break these issuers. To avoid breaking these issuers, we skip bubbling // up the "Denied=True" condition from the certificate request object to the @@ -246,8 +264,7 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { // certificate request is "Denied=True" and that the issuer still proceeds // to adding the "Ready" condition (to either true or false), then we // consider that this issuer has ignored the "Denied" state. - cond := apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionReady) - if cond == nil { + if crReadyCond == nil { if apiutil.CertificateRequestIsDenied(req) { return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionDenied)) } @@ -258,7 +275,7 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { // If the certificate request has failed, set the last failure time to now, // and set the Issuing status condition to False with reason. - if cond.Reason == cmapi.CertificateRequestReasonFailed { + if crReadyCond.Reason == cmapi.CertificateRequestReasonFailed { return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionReady)) } @@ -278,7 +295,7 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { // If the CertificateRequest is valid and ready, verify its status and issue // accordingly. - if cond.Reason == cmapi.CertificateRequestReasonIssued { + if crReadyCond.Reason == cmapi.CertificateRequestReasonIssued { return c.issueCertificate(ctx, nextRevision, crt, req, pk) } @@ -290,7 +307,7 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { } // CertificateRequest is not in a final state so do nothing. - log.V(logf.DebugLevel).Info("CertificateRequest not in final state, waiting...", "reason", cond.Reason) + log.V(logf.DebugLevel).Info("CertificateRequest not in final state, waiting...", "reason", crReadyCond.Reason) return nil } diff --git a/pkg/controller/certificates/issuing/issuing_controller_test.go b/pkg/controller/certificates/issuing/issuing_controller_test.go index 1c2f4fe88..fd47f8523 100644 --- a/pkg/controller/certificates/issuing/issuing_controller_test.go +++ b/pkg/controller/certificates/issuing/issuing_controller_test.go @@ -71,17 +71,17 @@ func TestIssuingController(t *testing.T) { exampleBundle := internaltest.MustCreateCryptoBundle(t, baseCert.DeepCopy(), fixedClock) exampleBundleAlt := internaltest.MustCreateCryptoBundle(t, baseCert.DeepCopy(), fixedClock) + metaFixedClockStart := metav1.NewTime(fixedClockStart) issuingCert := gen.CertificateFrom(baseCert.DeepCopy(), gen.SetCertificateStatusCondition(cmapi.CertificateCondition{ Type: cmapi.CertificateConditionIssuing, Status: cmmeta.ConditionTrue, ObservedGeneration: 3, + LastTransitionTime: &metaFixedClockStart, }), ) - metaFixedClockStart := metav1.NewTime(fixedClockStart) - tests := map[string]testT{ "if certificate is not in Issuing state, then do nothing": { certificate: exampleBundle.Certificate, @@ -213,7 +213,38 @@ func TestIssuingController(t *testing.T) { }, expectedErr: false, }, - + "if certificate is in Issuing state, one CertificateRequest that has failed during previous issuance, do nothing": { + certificate: exampleBundle.Certificate, + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{ + gen.CertificateFrom(issuingCert), + gen.CertificateRequestFrom(exampleBundle.CertificateRequestFailed, + gen.AddCertificateRequestAnnotations(map[string]string{ + cmapi.CertificateRequestRevisionAnnotationKey: "2", // Current Certificate revision=1 + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionFalse, + Reason: cmapi.CertificateRequestReasonFailed, + Message: "The certificate request failed because of reasons", + }), + gen.SetCertificateRequestFailureTime(metav1.Time{Time: metaFixedClockStart.Time.Add(time.Hour * -1)}), + )}, + KubeObjects: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: nextPrivateKeySecretName, + Namespace: exampleBundle.Certificate.Namespace, + }, + Data: map[string][]byte{ + corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes, + }, + }, + }, + ExpectedActions: []testpkg.Action{}, + }, + expectedErr: false, + }, "if certificate is in Issuing state, one CertificateRequest, but has failed, set failed state and log event": { certificate: exampleBundle.Certificate, builder: &testpkg.Builder{