refactor(trigger): move backoff logic to a unit-tested func

The trigger_controller_test.go has many unrelated test cases and I
thought it would be good to have more tightly scoped functions that are
easy to review (and most importantly, the unit tests are easy to
review).

Signed-off-by: Maël Valais <mael.valais@gmail.com>

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",

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.

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)
+		})
+	}
+}