From 05c1fb9fc2154aebf7b268c5bfe4b54b0ffccd7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Valais?= Date: Sun, 21 Mar 2021 16:03:03 +0100 Subject: [PATCH] trigger-controller: reissue on mismatch using NextRevisionRequest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Maël Valais Co-authored-by: Josh Soref --- .../certificates/trigger/BUILD.bazel | 1 - .../trigger/trigger_controller.go | 27 ++- .../trigger/trigger_controller_test.go | 184 +++++++++++------- 3 files changed, 133 insertions(+), 79 deletions(-) diff --git a/pkg/controller/certificates/trigger/BUILD.bazel b/pkg/controller/certificates/trigger/BUILD.bazel index 02144751b..431bb1719 100644 --- a/pkg/controller/certificates/trigger/BUILD.bazel +++ b/pkg/controller/certificates/trigger/BUILD.bazel @@ -63,7 +63,6 @@ go_test( "@com_github_go_logr_logr//testing:go_default_library", "@com_github_stretchr_testify//assert:go_default_library", "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library", - "@io_k8s_apimachinery//pkg/types: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/trigger/trigger_controller.go b/pkg/controller/certificates/trigger/trigger_controller.go index 2d729e959..25fddcb47 100644 --- a/pkg/controller/certificates/trigger/trigger_controller.go +++ b/pkg/controller/certificates/trigger/trigger_controller.go @@ -163,7 +163,7 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { // 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) + backoff, delay := shouldBackoffReissuingOnFailure(log, c.clock, input.Certificate, input.NextRevisionRequest) 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) @@ -199,20 +199,37 @@ 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) { +// shouldBackoffReissuingOnFailure tells us if this certificate's +// re-issuance should be backed off and for how much time. +// +// The request can be left nil, in which case no back off is required. When +// the certificate doesn't match the request, no back off is required +// either since it means the request will have to be re-issued. +func shouldBackoffReissuingOnFailure(log logr.Logger, c clock.Clock, crt *cmapi.Certificate, nextCR *cmapi.CertificateRequest) (backoff bool, delay time.Duration) { if crt.Status.LastFailureTime == nil { return false, 0 } + if nextCR != nil { + mismatches, err := certificates.RequestMatchesSpec(nextCR, crt.Spec) + if err != nil { + log.V(logf.InfoLevel).Info("next CertificateRequest cannot be decoded, skipping checking if Certificate matches the CertificateRequest") + return false, 0 + } + if len(mismatches) > 0 { + log.V(logf.ExtendedInfoLevel).WithValues("mismatches", mismatches).Info("Certificate is failing but the Certificate differs from CertificateRequest, backoff is not required") + return false, 0 + } + } else { + log.V(logf.InfoLevel).Info("next CertificateRequest not available, skipping checking if Certificate matches the CertificateRequest") + } + 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 } diff --git a/pkg/controller/certificates/trigger/trigger_controller_test.go b/pkg/controller/certificates/trigger/trigger_controller_test.go index 81e591a8d..741ff10c8 100644 --- a/pkg/controller/certificates/trigger/trigger_controller_test.go +++ b/pkg/controller/certificates/trigger/trigger_controller_test.go @@ -159,11 +159,52 @@ func Test_controller_ProcessItem(t *testing.T) { }, "should not set Issuing=True when cert has been failing for 59 minutes": { existingCertificate: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("cert-1-uid"), + gen.SetCertificateRevision(1), + gen.SetCertificateDNSNames("example.com"), gen.SetCertificateLastFailureTime(metav1.NewTime(fixedNow.Add(-59*time.Minute))), ), wantDataForCertificateCalled: true, - mockDataForCertificateReturn: policies.Input{}, - wantShouldReissueCalled: false, + mockDataForCertificateReturn: policies.Input{ + NextRevisionRequest: createCertificateRequestOrPanic(gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("cert-1-uid"), + gen.SetCertificateRevision(2), + gen.SetCertificateDNSNames("example.com"), + )), + }, + wantShouldReissueCalled: false, + }, + "should set Issuing=True when cert has been failing for 59 minutes but cert and next CR are mismatched": { + existingCertificate: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateGeneration(42), + gen.SetCertificateUID("cert-1-uid"), + gen.SetCertificateRevision(1), + gen.SetCertificateDNSNames("example-that-was-updated-by-user.com"), + gen.SetCertificateLastFailureTime(metav1.NewTime(fixedNow.Add(-59*time.Minute))), + ), + wantDataForCertificateCalled: true, + mockDataForCertificateReturn: policies.Input{ + NextRevisionRequest: createCertificateRequestOrPanic(gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("cert-1-uid"), + gen.SetCertificateRevision(2), + gen.SetCertificateDNSNames("example.com"), + )), + }, + wantShouldReissueCalled: true, + mockShouldReissue: func(*testing.T) policies.Func { + return func(policies.Input) (string, string, bool) { + return "ForceTriggered", "Re-issuance forced by unit test case", true + } + }, + wantEvent: "Normal Issuing Re-issuance forced by unit test case", + wantConditions: []cmapi.CertificateCondition{{ + Type: "Issuing", + Status: "True", + Reason: "ForceTriggered", + Message: "Re-issuance forced by unit test case", + LastTransitionTime: &fixedNow, + ObservedGeneration: 42, + }}, }, "should set Issuing=True when cert has been failing for 61 minutes and shouldReissue returns true": { existingCertificate: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), @@ -188,68 +229,6 @@ func Test_controller_ProcessItem(t *testing.T) { ObservedGeneration: 42, }}, }, - "should set Issuing=True when mismatch between cert and next CR and cert is failing": { - existingCertificate: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), - gen.SetCertificateUID("cert-1-uid"), - gen.SetCertificateRevision(1), - gen.SetCertificateDNSNames("example.com"), - gen.SetCertificateLastFailureTime(fixedNow), - ), - wantDataForCertificateCalled: true, - mockDataForCertificateReturn: policies.Input{ - CurrentRevisionRequest: createCertificateRequestOrPanic(gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), - gen.SetCertificateUID("cert-1-uid"), - gen.SetCertificateRevision(1), - gen.SetCertificateDNSNames("example-mismatched.com"), - )), - }, - wantShouldReissueCalled: true, - mockShouldReissue: func(*testing.T) policies.Func { - return func(policies.Input) (string, string, bool) { - return "ForceTriggered", "Re-issuance forced by unit test case", true - } - }, - wantEvent: "Normal Issuing Re-issuance forced by unit test case", - wantConditions: []cmapi.CertificateCondition{{ - Type: "Issuing", - Status: "True", - Reason: "ForceTriggered", - Message: "Re-issuance forced by unit test case", - LastTransitionTime: &fixedNow, - ObservedGeneration: 42, - }}, - }, - "should set Issuing=True when cert is failing and the cert status.revision is nil": { - existingCertificate: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), - gen.SetCertificateUID("cert-1-uid"), - // Revision is nil. - gen.SetCertificateDNSNames("example.com"), - gen.SetCertificateLastFailureTime(fixedNow), - ), - wantDataForCertificateCalled: true, - mockDataForCertificateReturn: policies.Input{ - CurrentRevisionRequest: createCertificateRequestOrPanic(gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), - gen.SetCertificateUID("cert-1-uid"), - gen.SetCertificateRevision(1), - gen.SetCertificateDNSNames("example-mismatch.com"), - )), - }, - wantShouldReissueCalled: true, - mockShouldReissue: func(*testing.T) policies.Func { - return func(policies.Input) (string, string, bool) { - return "ForceTriggered", "Re-issuance forced by unit test case", true - } - }, - wantEvent: "Normal Issuing Re-issuance forced by unit test case", - wantConditions: []cmapi.CertificateCondition{{ - Type: "Issuing", - Status: "True", - Reason: "ForceTriggered", - Message: "Re-issuance forced by unit test case", - LastTransitionTime: &fixedNow, - ObservedGeneration: 42, - }}, - }, } for name, test := range tests { t.Run(name, func(t *testing.T) { @@ -343,49 +322,75 @@ func Test_controller_ProcessItem(t *testing.T) { func Test_shouldBackoffReissuingOnFailure(t *testing.T) { clock := fakeclock.NewFakeClock(time.Date(2020, 11, 20, 16, 05, 00, 0000, time.Local)) + // We don't need to full bundle, just a simple CertificateRequest. + createCertificateRequestOrPanic := func(crt *cmapi.Certificate) *cmapi.CertificateRequest { + return internaltest.MustCreateCryptoBundle(t, crt, clock).CertificateRequest + } + tests := map[string]struct { givenCert *cmapi.Certificate + givenNextCR *cmapi.CertificateRequest wantBackoff bool wantDelay time.Duration }{ - "should not back off from reissuing when the input request is nil": { - givenCert: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns")), + "no need to backoff from reissuing when the input request is nil": { + givenCert: gen.Certificate("test", gen.SetCertificateNamespace("testns")), wantBackoff: false, }, "should not back off from reissuing when cert is not failing": { givenCert: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), gen.SetCertificateUID("cert-1-uid"), gen.SetCertificateRevision(1), - gen.SetCertificateDNSNames("example2.com"), + gen.SetCertificateDNSNames("example.com"), // LastFailureTime is not set here. ), + givenNextCR: createCertificateRequestOrPanic(gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("cert-1-uid"), + gen.SetCertificateDNSNames("example.com"), + gen.SetCertificateRevision(1), + )), wantBackoff: false, }, "should not back off from reissuing when the failure happened 61 minutes ago": { givenCert: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), gen.SetCertificateUID("cert-1-uid"), gen.SetCertificateRevision(1), - gen.SetCertificateDNSNames("example2.com"), + gen.SetCertificateDNSNames("example.com"), gen.SetCertificateLastFailureTime(metav1.NewTime(clock.Now().Add(-61*time.Minute))), ), + givenNextCR: createCertificateRequestOrPanic(gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("cert-1-uid"), + gen.SetCertificateRevision(1), + gen.SetCertificateDNSNames("example.com"), + )), wantBackoff: false, }, "should not back off from reissuing when the failure happened 60 minutes ago": { givenCert: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), gen.SetCertificateUID("cert-1-uid"), gen.SetCertificateRevision(1), - gen.SetCertificateDNSNames("example2.com"), + gen.SetCertificateDNSNames("example.com"), gen.SetCertificateLastFailureTime(metav1.NewTime(clock.Now().Add(-60*time.Minute))), ), + givenNextCR: createCertificateRequestOrPanic(gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("cert-1-uid"), + gen.SetCertificateRevision(1), + gen.SetCertificateDNSNames("example.com"), + )), wantBackoff: false, }, "should back off from reissuing when the failure happened 59 minutes ago": { givenCert: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), - gen.SetCertificateLastFailureTime(metav1.NewTime(clock.Now().Add(-59*time.Minute))), gen.SetCertificateUID("cert-1-uid"), gen.SetCertificateRevision(1), - gen.SetCertificateDNSNames("example2.com"), + gen.SetCertificateDNSNames("example.com"), + gen.SetCertificateLastFailureTime(metav1.NewTime(clock.Now().Add(-59*time.Minute))), ), + givenNextCR: createCertificateRequestOrPanic(gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("cert-1-uid"), + gen.SetCertificateRevision(1), + gen.SetCertificateDNSNames("example.com"), + )), wantBackoff: true, wantDelay: 1 * time.Minute, }, @@ -393,16 +398,49 @@ func Test_shouldBackoffReissuingOnFailure(t *testing.T) { givenCert: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), gen.SetCertificateUID("cert-1-uid"), gen.SetCertificateRevision(1), + gen.SetCertificateDNSNames("example.com"), gen.SetCertificateLastFailureTime(metav1.NewTime(clock.Now())), - gen.SetCertificateDNSNames("example2.com"), ), + givenNextCR: createCertificateRequestOrPanic(gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("cert-1-uid"), + gen.SetCertificateRevision(1), + gen.SetCertificateDNSNames("example.com"), + )), wantBackoff: true, wantDelay: 1 * time.Hour, }, + "should not back off from reissuing when the failure happened 0 minutes ago and cert and next CR are mismatched": { + givenCert: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("cert-1-uid"), + gen.SetCertificateRevision(1), + gen.SetCertificateDNSNames("example-was-changed-by-user.com"), + gen.SetCertificateLastFailureTime(metav1.NewTime(clock.Now())), + ), + givenNextCR: createCertificateRequestOrPanic(gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("cert-1-uid"), + gen.SetCertificateRevision(1), + gen.SetCertificateDNSNames("example.com"), + )), + wantBackoff: false, + }, + "should not back off from reissuing when the failure happened 1 minutes ago and cert and next CR are mismatched": { + givenCert: gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("cert-1-uid"), + gen.SetCertificateRevision(1), + gen.SetCertificateDNSNames("example-was-updated-by-user.com"), + gen.SetCertificateLastFailureTime(metav1.NewTime(clock.Now().Add(-1*time.Minute))), + ), + givenNextCR: createCertificateRequestOrPanic(gen.Certificate("cert-1", gen.SetCertificateNamespace("testns"), + gen.SetCertificateUID("cert-1-uid"), + gen.SetCertificateRevision(1), + gen.SetCertificateDNSNames("example.com"), + )), + wantBackoff: false, + }, } for name, test := range tests { t.Run(name, func(t *testing.T) { - gotBackoff, gotDelay := shouldBackoffReissuingOnFailure(logtest.TestLogger{T: t}, clock, test.givenCert) + gotBackoff, gotDelay := shouldBackoffReissuingOnFailure(logtest.TestLogger{T: t}, clock, test.givenCert, test.givenNextCR) assert.Equal(t, test.wantBackoff, gotBackoff) assert.Equal(t, test.wantDelay, gotDelay) })