diff --git a/pkg/controller/certificates/trigger/BUILD.bazel b/pkg/controller/certificates/trigger/BUILD.bazel index 3dbc1217f..0ed7bd5b3 100644 --- a/pkg/controller/certificates/trigger/BUILD.bazel +++ b/pkg/controller/certificates/trigger/BUILD.bazel @@ -59,6 +59,9 @@ go_test( "//pkg/controller:go_default_library", "//pkg/controller/certificates/trigger/policies:go_default_library", "//pkg/controller/test:go_default_library", + "//test/unit/gen:go_default_library", + "@com_github_go_logr_logr//testing:go_default_library", + "@com_github_stretchr_testify//assert:go_default_library", "@io_k8s_api//core/v1:go_default_library", "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", "@io_k8s_client_go//testing:go_default_library", diff --git a/pkg/controller/certificates/trigger/trigger_controller.go b/pkg/controller/certificates/trigger/trigger_controller.go index 3f97bbdb8..07bf68873 100644 --- a/pkg/controller/certificates/trigger/trigger_controller.go +++ b/pkg/controller/certificates/trigger/trigger_controller.go @@ -153,16 +153,18 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { return nil } - // check if we have had a recent failure, and if so do not trigger a - // re-issuance immediately - if crt.Status.LastFailureTime != nil { - now := c.clock.Now() - retryAfter := crt.Status.LastFailureTime.Add(retryAfterLastFailure) - if now.Before(retryAfter) { - log.V(logf.InfoLevel).Info("Not re-issuing certificate as an attempt has been made in the last hour", "retry_after", retryAfter) - c.scheduleRecheckOfCertificateIfRequired(log, key, retryAfter.Sub(now)) - return nil - } + input, err := c.gatherer.DataForCertificate(ctx, crt) + if err != nil { + return err + } + + // Back off from re-issuing immediately when the certificate has been + // in failing mode for less than 1 hour. + backoff, delay := shouldBackoffReissuingOnFailure(log, c.clock, input.Certificate) + if backoff { + log.V(logf.InfoLevel).Info("Not re-issuing certificate as an attempt has been made in the last hour", "retry_delay", delay) + c.scheduleRecheckOfCertificateIfRequired(log, key, delay) + return nil } if crt.Status.RenewalTime != nil { @@ -171,11 +173,6 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { c.scheduleRecheckOfCertificateIfRequired(log, key, crt.Status.RenewalTime.Time.Sub(c.clock.Now())) } - input, err := c.gatherer.DataForCertificate(ctx, crt) - if err != nil { - return err - } - reason, message, reissue := c.policyChain.Evaluate(input) if !reissue { // no re-issuance required, return early @@ -193,6 +190,23 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { return nil } +// shouldBackoffReissuingOnFailure tells us if we should back off from +// reissuing the certificate and for how much time. +func shouldBackoffReissuingOnFailure(log logr.Logger, c clock.Clock, crt *cmapi.Certificate) (backoff bool, delay time.Duration) { + if crt.Status.LastFailureTime == nil { + return false, 0 + } + + now := c.Now() + durationSinceFailure := now.Sub(crt.Status.LastFailureTime.Time) + if durationSinceFailure > retryAfterLastFailure { + log.V(logf.ExtendedInfoLevel).WithValues("since_failure", durationSinceFailure).Info("Certificate has been in failure mode long enough, no need to back off") + return false, 0 + } + + return true, retryAfterLastFailure - durationSinceFailure +} + // scheduleRecheckOfCertificateIfRequired will schedule the resource with the // given key to be re-queued for processing after the given amount of time // has elapsed. diff --git a/pkg/controller/certificates/trigger/trigger_controller_test.go b/pkg/controller/certificates/trigger/trigger_controller_test.go index 191b18002..6ddd0ca4c 100644 --- a/pkg/controller/certificates/trigger/trigger_controller_test.go +++ b/pkg/controller/certificates/trigger/trigger_controller_test.go @@ -26,17 +26,20 @@ import ( coretesting "k8s.io/client-go/testing" fakeclock "k8s.io/utils/clock/testing" + logtest "github.com/go-logr/logr/testing" cmapi "github.com/jetstack/cert-manager/pkg/apis/certmanager/v1" cmmeta "github.com/jetstack/cert-manager/pkg/apis/meta/v1" controllerpkg "github.com/jetstack/cert-manager/pkg/controller" "github.com/jetstack/cert-manager/pkg/controller/certificates/trigger/policies" testpkg "github.com/jetstack/cert-manager/pkg/controller/test" + "github.com/jetstack/cert-manager/test/unit/gen" + "github.com/stretchr/testify/assert" ) // policyFuncBuilder wraps a policies.Func to allow injecting a testing.T type policyFuncBuilder func(t *testing.T) policies.Func -func TestProcessItem(t *testing.T) { +func Test_controller_ProcessItem(t *testing.T) { // now time is the current time at the start of the test (the clock is fixed) now := time.Now() metaNow := metav1.NewTime(now) @@ -424,3 +427,55 @@ func buildTestPolicyChain(t *testing.T, funcs ...policyFuncBuilder) policies.Cha } return c } + +func Test_shouldBackoffReissuingOnFailure(t *testing.T) { + clock := fakeclock.NewFakeClock(time.Date(2020, 11, 20, 16, 05, 00, 0000, time.Local)) + tests := []struct { + name string + givenCert *cmapi.Certificate + wantBackoff bool + wantDelay time.Duration + }{ + { + name: "no need to back off from reissuing when there is no previous failure", + givenCert: gen.Certificate("test", + gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("test-uid"), + gen.SetCertificateDNSNames("example2.com"), + gen.SetCertificateRevision(1), + // LastFailureTime is not set here. + ), + wantBackoff: false, + }, + { + name: "no need to back off from reissuing when the failure is more than an hour ago, reissuance can happen now", + givenCert: gen.Certificate("test", + gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("test-uid"), + gen.SetCertificateDNSNames("example2.com"), + gen.SetCertificateRevision(1), + gen.SetCertificateLastFailureTime(metav1.NewTime(clock.Now().Add(-61*time.Minute))), + ), + wantBackoff: false, + }, + { + name: "needs to back off from reissuing when the failure happened less than an hour ago", + givenCert: gen.Certificate("test", + gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("test-uid"), + gen.SetCertificateDNSNames("example2.com"), + gen.SetCertificateRevision(1), + gen.SetCertificateLastFailureTime(metav1.NewTime(clock.Now().Add(-59*time.Minute))), + ), + wantBackoff: true, + wantDelay: 1 * time.Minute, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotBackoff, gotDelay := shouldBackoffReissuingOnFailure(logtest.TestLogger{T: t}, clock, tt.givenCert) + assert.Equal(t, tt.wantBackoff, gotBackoff) + assert.Equal(t, tt.wantDelay, gotDelay) + }) + } +}