weiguoqiang/cert-manager
1
0
Fork 0
Code Issues Pull Requests Actions 2 Packages Projects Releases Wiki Activity

trigger-controller: reissue on mismatch using NextRevisionRequest

Browse Source 
Signed-off-by: Maël Valais <mael@vls.dev>
Co-authored-by: Josh Soref <jsoref@users.noreply.github.com>
This commit is contained in:
Maël Valais 2021-03-21 16:03:03 +01:00
parent eb6d1399fc
commit 05c1fb9fc2
3 changed files with 133 additions and 79 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
pkg/controller/certificates/trigger/BUILD.bazel
View File

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

27
pkg/controller/certificates/trigger/trigger_controller.go
View File

@ -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
}

184
pkg/controller/certificates/trigger/trigger_controller_test.go
View File

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