From ec1bdc4983a1a91ad381f356135e2f93b79aa505 Mon Sep 17 00:00:00 2001 From: irbekrm Date: Mon, 23 Aug 2021 15:00:57 +0100 Subject: [PATCH] Adds a test case for renewal time skew and a comment Signed-off-by: irbekrm --- pkg/controller/certificates/util.go | 10 ++++++++++ pkg/controller/certificates/util_test.go | 9 +++++++++ 2 files changed, 19 insertions(+) diff --git a/pkg/controller/certificates/util.go b/pkg/controller/certificates/util.go index 2935ff0f4..4f33f9e44 100644 --- a/pkg/controller/certificates/util.go +++ b/pkg/controller/certificates/util.go @@ -307,6 +307,16 @@ func RenewalTime(notBefore, notAfter time.Time, renewBeforeOverride *metav1.Dura // 2. Calculate when a cert should be renewed + // Truncate the renewal time to nearest second. This is important + // because the renewal time also gets stored on Certificate's status + // where it is truncated to the nearest second. We use the renewal time + // from Certificate's status to determine when the Certificate will be + // added to the queue to be renewed, but then re-calculate whether it + // needs to be renewed _now_ using this function- so returning a + // non-truncated value here would potentially cause Certificates to be + // re-queued for renewal earlier than the calculated renewal time thus + // causing Certificates to not be automatically renewed. See + // https://github.com/jetstack/cert-manager/pull/4399. rt := metav1.NewTime(notAfter.Add(-1 * renewBefore).Truncate(time.Second)) return &rt } diff --git a/pkg/controller/certificates/util_test.go b/pkg/controller/certificates/util_test.go index 8951914b2..400dfbbe0 100644 --- a/pkg/controller/certificates/util_test.go +++ b/pkg/controller/certificates/util_test.go @@ -343,6 +343,15 @@ func TestRenewalTime(t *testing.T) { renewBeforeOverride: &metav1.Duration{Duration: time.Hour * 24}, expectedRenewalTime: &metav1.Time{Time: now.Add(time.Minute * 3)}, // renew in 3 minutes }, + // This test case is here to guard against an earlier bug where + // a non-truncated renewal time returned from this function + // caused certs to not be renewed. + // See https://github.com/jetstack/cert-manager/pull/4399 + "certificate's duration is skewed by a second": { + notBefore: now, + notAfter: now.Add(time.Hour * 24).Add(time.Second * -1), + expectedRenewalTime: &metav1.Time{Time: now.Add(time.Hour * 16).Add(time.Second * -1)}, + }, } for n, s := range tests { t.Run(n, func(t *testing.T) {