From ff67b2a9a0bc5e1520c875496fbd728b915ca738 Mon Sep 17 00:00:00 2001 From: irbekrm Date: Wed, 22 Dec 2021 14:51:25 +0000 Subject: [PATCH 1/3] Ignore failed CRs for previous issuance in certificates-issuing controller Issuing controller should only look at 'current' CertificateRequests Signed-off-by: irbekrm --- .../issuing/issuing_controller.go | 27 +++++++++++--- .../issuing/issuing_controller_test.go | 37 +++++++++++++++++-- 2 files changed, 56 insertions(+), 8 deletions(-) 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{ From fac6622f5e5b0308dec79734e6dfc6a8412e71c7 Mon Sep 17 00:00:00 2001 From: irbekrm Date: Wed, 22 Dec 2021 14:54:55 +0000 Subject: [PATCH 2/3] Delete CertificateRequest that failed during previous issuance if we are re-issuing for the same revision Signed-off-by: irbekrm --- .../requestmanager_controller.go | 32 +++++++++++------ .../requestmanager_controller_test.go | 36 +++++++++++++++++-- 2 files changed, 54 insertions(+), 14 deletions(-) diff --git a/pkg/controller/certificates/requestmanager/requestmanager_controller.go b/pkg/controller/certificates/requestmanager/requestmanager_controller.go index 9a11c1151..3fbafda39 100644 --- a/pkg/controller/certificates/requestmanager/requestmanager_controller.go +++ b/pkg/controller/certificates/requestmanager/requestmanager_controller.go @@ -198,7 +198,7 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { return err } - requests, err = c.deleteCurrentFailedRequests(ctx, requests...) + requests, err = c.deleteCurrentFailedRequests(ctx, crt, requests...) if err != nil { return err } @@ -220,33 +220,43 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { return c.createNewCertificateRequest(ctx, crt, pk, nextRevision, nextPrivateKeySecret.Name) } -func (c *controller) deleteCurrentFailedRequests(ctx context.Context, reqs ...*cmapi.CertificateRequest) ([]*cmapi.CertificateRequest, error) { - log := logf.FromContext(ctx) +func (c *controller) deleteCurrentFailedRequests(ctx context.Context, crt *cmapi.Certificate, reqs ...*cmapi.CertificateRequest) ([]*cmapi.CertificateRequest, error) { + log := logf.FromContext(ctx).WithValues("Certificate", crt.Name) var remaining []*cmapi.CertificateRequest for _, req := range reqs { log = logf.WithRelatedResource(log, req) + // Check if there are any 'current' CertificateRequests that // failed during the previous issuance cycle. Those should be // deleted so that a new one gets created and the issuance is // re-tried. In practice no more than one CertificateRequest is // expected at this point. - cond := apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionReady) - if cond == nil || cond.Status != cmmeta.ConditionFalse || cond.Reason != cmapi.CertificateRequestReasonFailed { + crReadyCond := apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionReady) + if crReadyCond == nil || crReadyCond.Status != cmmeta.ConditionFalse || crReadyCond.Reason != cmapi.CertificateRequestReasonFailed { remaining = append(remaining, req) continue } - // TODO: once we have implemented exponential back off for - // Certificate failures, this should be changed accordingly. - now := c.clock.Now() - durationSinceFailure := now.Sub(cond.LastTransitionTime.Time) - if durationSinceFailure >= certificates.RetryAfterLastFailure { + + certIssuingCond := apiutil.GetCertificateCondition(crt, cmapi.CertificateConditionIssuing) + if certIssuingCond == nil { + // This should never happen + log.V(logf.ErrorLevel).Info("Certificate does not have Issuing condition") + return nil, nil + } + // If the Issuing condition on the Certificate is newer than the + // failure time on CertificateRequest it means that the + // CertificateRequest failed during previous issuance (for the + // same revision). If it is a CertificateRequest that failed + // during the previous issuance, then it should be deleted so + // that we create a new one for this issuance. + if req.Status.FailureTime.Before(certIssuingCond.LastTransitionTime) { + log.V(logf.DebugLevel).Info("Found a failed CertificateRequest for previous issuance of this revision, deleting...") if err := c.client.CertmanagerV1().CertificateRequests(req.Namespace).Delete(ctx, req.Name, metav1.DeleteOptions{}); err != nil { return nil, err } continue } remaining = append(remaining, req) - } return remaining, nil } diff --git a/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go b/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go index 7fc7a66bb..6c6485db9 100644 --- a/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go +++ b/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go @@ -80,12 +80,18 @@ func TestProcessItem(t *testing.T) { ) fixedNow := metav1.NewTime(time.Now()) fixedClock := fakeclock.NewFakeClock(fixedNow.Time) - failedCRCondition := cmapi.CertificateRequestCondition{ + failedCRConditionPreviousIssuance := cmapi.CertificateRequestCondition{ Type: cmapi.CertificateRequestConditionReady, Status: cmmeta.ConditionFalse, Reason: cmapi.CertificateRequestReasonFailed, LastTransitionTime: &metav1.Time{Time: fixedNow.Time.Add(-1 * time.Hour)}, } + failedCRConditionThisIssuance := cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionFalse, + Reason: cmapi.CertificateRequestReasonFailed, + LastTransitionTime: &metav1.Time{Time: fixedNow.Time.Add(1 * time.Minute)}, + } tests := map[string]struct { // key that should be passed to ProcessItem. // if not set, the 'namespace/name' of the 'Certificate' field will be used. @@ -534,7 +540,7 @@ func TestProcessItem(t *testing.T) { }, certificate: gen.CertificateFrom(bundle1.certificate, gen.SetCertificateNextPrivateKeySecretName("exists"), - gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionIssuing, Status: cmmeta.ConditionTrue}), + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionIssuing, Status: cmmeta.ConditionTrue, LastTransitionTime: &fixedNow}), gen.SetCertificateRevision(5), ), requests: []runtime.Object{ @@ -543,7 +549,8 @@ func TestProcessItem(t *testing.T) { cmapi.CertificateRequestPrivateKeyAnnotationKey: "exists", cmapi.CertificateRequestRevisionAnnotationKey: "6", }), - gen.AddCertificateRequestStatusCondition(failedCRCondition), + gen.AddCertificateRequestStatusCondition(failedCRConditionPreviousIssuance), + gen.SetCertificateRequestFailureTime(metav1.Time{Time: fixedNow.Time.Add(time.Hour * -1)}), ), }, expectedEvents: []string{`Normal Requested Created new CertificateRequest resource "test-notrandom"`}, @@ -558,6 +565,29 @@ func TestProcessItem(t *testing.T) { )), relaxedCertificateRequestMatcher), }, }, + "should do nothing if the CertificateRequest that is valid for spec has failed during this issuance cycle": { + secrets: []runtime.Object{ + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "exists"}, + Data: map[string][]byte{corev1.TLSPrivateKeyKey: bundle1.privateKeyBytes}, + }, + }, + certificate: gen.CertificateFrom(bundle1.certificate, + gen.SetCertificateNextPrivateKeySecretName("exists"), + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionIssuing, Status: cmmeta.ConditionTrue, LastTransitionTime: &fixedNow}), + gen.SetCertificateRevision(5), + ), + requests: []runtime.Object{ + gen.CertificateRequestFrom(bundle1.certificateRequest, + gen.SetCertificateRequestAnnotations(map[string]string{ + cmapi.CertificateRequestPrivateKeyAnnotationKey: "exists", + cmapi.CertificateRequestRevisionAnnotationKey: "6", + }), + gen.AddCertificateRequestStatusCondition(failedCRConditionThisIssuance), + gen.SetCertificateRequestFailureTime(metav1.Time{Time: fixedNow.Time.Add(1 * time.Minute)}), + ), + }, + }, } for name, test := range tests { t.Run(name, func(t *testing.T) { From 0a4617e58226024a5d7fefa9ccbae165ea49332c Mon Sep 17 00:00:00 2001 From: irbekrm Date: Wed, 22 Dec 2021 14:55:42 +0000 Subject: [PATCH 3/3] Fix staticcheck error Signed-off-by: irbekrm --- pkg/controller/certificates/issuing/issuing_controller.go | 8 ++++---- .../requestmanager/requestmanager_controller.go | 4 ++-- .../requestmanager/requestmanager_controller_test.go | 4 +--- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/pkg/controller/certificates/issuing/issuing_controller.go b/pkg/controller/certificates/issuing/issuing_controller.go index 92f66d4ef..611bbe024 100644 --- a/pkg/controller/certificates/issuing/issuing_controller.go +++ b/pkg/controller/certificates/issuing/issuing_controller.go @@ -243,10 +243,10 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { 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 + // If the CertificateRequest for this revision failed before the + // Issuing condition was last updated on the Certificate, then it must be a + // failed CertificateRequest from the previous issuance for the same + // revision. Leave it to the 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 { diff --git a/pkg/controller/certificates/requestmanager/requestmanager_controller.go b/pkg/controller/certificates/requestmanager/requestmanager_controller.go index 3fbafda39..6c2119f74 100644 --- a/pkg/controller/certificates/requestmanager/requestmanager_controller.go +++ b/pkg/controller/certificates/requestmanager/requestmanager_controller.go @@ -244,8 +244,8 @@ func (c *controller) deleteCurrentFailedRequests(ctx context.Context, crt *cmapi return nil, nil } // If the Issuing condition on the Certificate is newer than the - // failure time on CertificateRequest it means that the - // CertificateRequest failed during previous issuance (for the + // failure time on CertificateRequest, it means that the + // CertificateRequest failed during the previous issuance (for the // same revision). If it is a CertificateRequest that failed // during the previous issuance, then it should be deleted so // that we create a new one for this issuance. diff --git a/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go b/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go index 6c6485db9..40decde6c 100644 --- a/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go +++ b/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go @@ -605,9 +605,7 @@ func TestProcessItem(t *testing.T) { if test.secrets != nil { builder.KubeObjects = append(builder.KubeObjects, test.secrets...) } - for _, req := range test.requests { - builder.CertManagerObjects = append(builder.CertManagerObjects, req) - } + builder.CertManagerObjects = append(builder.CertManagerObjects, test.requests...) builder.Init() // Register informers used by the controller using the registration wrapper