From fd61e1ccc7f6ca0f4af245da6d21651a5508ffd3 Mon Sep 17 00:00:00 2001 From: irbekrm Date: Tue, 22 Jun 2021 07:07:22 +0100 Subject: [PATCH] Delete 'next' CertificateRequests that failed in last issuance cycle So that the issuance is retried Signed-off-by: irbekrm --- .../certificates/requestmanager/BUILD.bazel | 1 + .../requestmanager_controller.go | 36 +++++++++++++++ .../requestmanager_controller_test.go | 44 +++++++++++++++++++ 3 files changed, 81 insertions(+) diff --git a/pkg/controller/certificates/requestmanager/BUILD.bazel b/pkg/controller/certificates/requestmanager/BUILD.bazel index 0cd338cf5..c350f2af0 100644 --- a/pkg/controller/certificates/requestmanager/BUILD.bazel +++ b/pkg/controller/certificates/requestmanager/BUILD.bazel @@ -66,5 +66,6 @@ go_test( "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", "@io_k8s_apimachinery//pkg/runtime:go_default_library", "@io_k8s_client_go//testing:go_default_library", + "@io_k8s_utils//clock/testing:go_default_library", ], ) diff --git a/pkg/controller/certificates/requestmanager/requestmanager_controller.go b/pkg/controller/certificates/requestmanager/requestmanager_controller.go index fd9af38d0..577e31763 100644 --- a/pkg/controller/certificates/requestmanager/requestmanager_controller.go +++ b/pkg/controller/certificates/requestmanager/requestmanager_controller.go @@ -195,6 +195,11 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { return err } + requests, err = c.deleteCurrentFailedRequests(ctx, requests...) + if err != nil { + return err + } + if len(requests) > 1 { // TODO: we should handle this case better, but for now do nothing to // avoid getting into loops where we keep creating multiple requests @@ -212,6 +217,37 @@ 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) + 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 { + 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 { + 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 +} + func (c *controller) deleteRequestsWithoutRevision(ctx context.Context, reqs ...*cmapi.CertificateRequest) ([]*cmapi.CertificateRequest, error) { log := logf.FromContext(ctx) var remaining []*cmapi.CertificateRequest diff --git a/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go b/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go index 921dcea42..7fc7a66bb 100644 --- a/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go +++ b/pkg/controller/certificates/requestmanager/requestmanager_controller_test.go @@ -21,12 +21,14 @@ import ( "fmt" "reflect" "testing" + "time" "github.com/kr/pretty" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" coretesting "k8s.io/client-go/testing" + fakeclock "k8s.io/utils/clock/testing" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" @@ -76,6 +78,14 @@ func TestProcessItem(t *testing.T) { }, Spec: cmapi.CertificateSpec{CommonName: "test-bundle-2"}}, ) + fixedNow := metav1.NewTime(time.Now()) + fixedClock := fakeclock.NewFakeClock(fixedNow.Time) + failedCRCondition := cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionFalse, + Reason: cmapi.CertificateRequestReasonFailed, + LastTransitionTime: &metav1.Time{Time: fixedNow.Time.Add(-1 * time.Hour)}, + } tests := map[string]struct { // key that should be passed to ProcessItem. // if not set, the 'namespace/name' of the 'Certificate' field will be used. @@ -515,6 +525,39 @@ func TestProcessItem(t *testing.T) { ), }, }, + "should recreate the CertificateRequest if the current 'next' CertificateRequest failed during previous 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}), + gen.SetCertificateRevision(5), + ), + requests: []runtime.Object{ + gen.CertificateRequestFrom(bundle1.certificateRequest, + gen.SetCertificateRequestAnnotations(map[string]string{ + cmapi.CertificateRequestPrivateKeyAnnotationKey: "exists", + cmapi.CertificateRequestRevisionAnnotationKey: "6", + }), + gen.AddCertificateRequestStatusCondition(failedCRCondition), + ), + }, + expectedEvents: []string{`Normal Requested Created new CertificateRequest resource "test-notrandom"`}, + expectedActions: []testpkg.Action{ + testpkg.NewAction(coretesting.NewDeleteAction(cmapi.SchemeGroupVersion.WithResource("certificaterequests"), "testns", "test")), + testpkg.NewCustomMatch(coretesting.NewCreateAction(cmapi.SchemeGroupVersion.WithResource("certificaterequests"), "testns", + gen.CertificateRequestFrom(bundle1.certificateRequest, + gen.SetCertificateRequestAnnotations(map[string]string{ + cmapi.CertificateRequestPrivateKeyAnnotationKey: "exists", + cmapi.CertificateRequestRevisionAnnotationKey: "6", + }), + )), relaxedCertificateRequestMatcher), + }, + }, } for name, test := range tests { t.Run(name, func(t *testing.T) { @@ -524,6 +567,7 @@ func TestProcessItem(t *testing.T) { ExpectedEvents: test.expectedEvents, ExpectedActions: test.expectedActions, StringGenerator: func(i int) string { return "notrandom" }, + Clock: fixedClock, } if test.certificate != nil { builder.CertManagerObjects = append(builder.CertManagerObjects, test.certificate)