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

Merge pull request #4688 from irbekrm/renew_failed

Browse Source 
Fixes a bug where a previous failed CertificateRequest was picked up during next issuance
This commit is contained in:
jetstack-bot 2022-01-04 15:08:31 +00:00 committed by GitHub
commit 019d64edcf
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 111 additions and 25 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

27
pkg/controller/certificates/issuing/issuing_controller.go
View File

@ -236,6 +236,24 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error {
return nil
}
certIssuingCond := apiutil.GetCertificateCondition(crt, cmapi.CertificateConditionIssuing)
crReadyCond := apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionReady)
if certIssuingCond == nil {
// This should never happen
log.V(logf.ErrorLevel).Info("Certificate does not have an issuing condition")
return nil
}
// If the CertificateRequest for this revision failed before the
// Issuing condition was last updated on the Certificate, then it must be a
// failed CertificateRequest from the previous issuance for the same
// revision. Leave it to the certificate-requests controller to delete the
// CertificateRequest and create a new one.
if req.Status.FailureTime != nil &&
req.Status.FailureTime.Before(certIssuingCond.LastTransitionTime) && crReadyCond.Reason == cmapi.CertificateRequestReasonFailed {
log.V(logf.InfoLevel).Info("Found a failed CertificateRequest from previous issuance, waiting for it to be deleted...")
return nil
}
// Some issuers won't honor the "Denied=True" condition, and we don't want
// to break these issuers. To avoid breaking these issuers, we skip bubbling
// up the "Denied=True" condition from the certificate request object to the
@ -246,8 +264,7 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error {
// certificate request is "Denied=True" and that the issuer still proceeds
// to adding the "Ready" condition (to either true or false), then we
// consider that this issuer has ignored the "Denied" state.
cond := apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionReady)
if cond == nil {
if crReadyCond == nil {
if apiutil.CertificateRequestIsDenied(req) {
return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionDenied))
}
@ -258,7 +275,7 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error {
// If the certificate request has failed, set the last failure time to now,
// and set the Issuing status condition to False with reason.
if cond.Reason == cmapi.CertificateRequestReasonFailed {
if crReadyCond.Reason == cmapi.CertificateRequestReasonFailed {
return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionReady))
}
@ -278,7 +295,7 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error {
// If the CertificateRequest is valid and ready, verify its status and issue
// accordingly.
if cond.Reason == cmapi.CertificateRequestReasonIssued {
if crReadyCond.Reason == cmapi.CertificateRequestReasonIssued {
return c.issueCertificate(ctx, nextRevision, crt, req, pk)
}
@ -290,7 +307,7 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error {
}
// CertificateRequest is not in a final state so do nothing.
log.V(logf.DebugLevel).Info("CertificateRequest not in final state, waiting...", "reason", cond.Reason)
log.V(logf.DebugLevel).Info("CertificateRequest not in final state, waiting...", "reason", crReadyCond.Reason)
return nil
}

37
pkg/controller/certificates/issuing/issuing_controller_test.go
View File

@ -71,17 +71,17 @@ func TestIssuingController(t *testing.T) {
exampleBundle := internaltest.MustCreateCryptoBundle(t, baseCert.DeepCopy(), fixedClock)
exampleBundleAlt := internaltest.MustCreateCryptoBundle(t, baseCert.DeepCopy(), fixedClock)
metaFixedClockStart := metav1.NewTime(fixedClockStart)
issuingCert := gen.CertificateFrom(baseCert.DeepCopy(),
gen.SetCertificateStatusCondition(cmapi.CertificateCondition{
Type: cmapi.CertificateConditionIssuing,
Status: cmmeta.ConditionTrue,
ObservedGeneration: 3,
LastTransitionTime: &metaFixedClockStart,
}),
)
metaFixedClockStart := metav1.NewTime(fixedClockStart)
tests := map[string]testT{
"if certificate is not in Issuing state, then do nothing": {
certificate: exampleBundle.Certificate,
@ -213,7 +213,38 @@ func TestIssuingController(t *testing.T) {
},
expectedErr: false,
},
"if certificate is in Issuing state, one CertificateRequest that has failed during previous issuance, do nothing": {
certificate: exampleBundle.Certificate,
builder: &testpkg.Builder{
CertManagerObjects: []runtime.Object{
gen.CertificateFrom(issuingCert),
gen.CertificateRequestFrom(exampleBundle.CertificateRequestFailed,
gen.AddCertificateRequestAnnotations(map[string]string{
cmapi.CertificateRequestRevisionAnnotationKey: "2", // Current Certificate revision=1
}),
gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{
Type: cmapi.CertificateRequestConditionReady,
Status: cmmeta.ConditionFalse,
Reason: cmapi.CertificateRequestReasonFailed,
Message: "The certificate request failed because of reasons",
}),
gen.SetCertificateRequestFailureTime(metav1.Time{Time: metaFixedClockStart.Time.Add(time.Hour * -1)}),
)},
KubeObjects: []runtime.Object{
&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: nextPrivateKeySecretName,
Namespace: exampleBundle.Certificate.Namespace,
},
Data: map[string][]byte{
corev1.TLSPrivateKeyKey: exampleBundle.PrivateKeyBytes,
},
},
},
ExpectedActions: []testpkg.Action{},
},
expectedErr: false,
},
"if certificate is in Issuing state, one CertificateRequest, but has failed, set failed state and log event": {
certificate: exampleBundle.Certificate,
builder: &testpkg.Builder{

32
pkg/controller/certificates/requestmanager/requestmanager_controller.go
View File

@ -198,7 +198,7 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error {
return err
}
requests, err = c.deleteCurrentFailedRequests(ctx, requests...)
requests, err = c.deleteCurrentFailedRequests(ctx, crt, requests...)
if err != nil {
return err
}
@ -220,33 +220,43 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error {
return c.createNewCertificateRequest(ctx, crt, pk, nextRevision, nextPrivateKeySecret.Name)
}
func (c *controller) deleteCurrentFailedRequests(ctx context.Context, reqs ...*cmapi.CertificateRequest) ([]*cmapi.CertificateRequest, error) {
log := logf.FromContext(ctx)
func (c *controller) deleteCurrentFailedRequests(ctx context.Context, crt *cmapi.Certificate, reqs ...*cmapi.CertificateRequest) ([]*cmapi.CertificateRequest, error) {
log := logf.FromContext(ctx).WithValues("Certificate", crt.Name)
var remaining []*cmapi.CertificateRequest
for _, req := range reqs {
log = logf.WithRelatedResource(log, req)
// Check if there are any 'current' CertificateRequests that
// failed during the previous issuance cycle. Those should be
// deleted so that a new one gets created and the issuance is
// re-tried. In practice no more than one CertificateRequest is
// expected at this point.
cond := apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionReady)
if cond == nil || cond.Status != cmmeta.ConditionFalse || cond.Reason != cmapi.CertificateRequestReasonFailed {
crReadyCond := apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionReady)
if crReadyCond == nil || crReadyCond.Status != cmmeta.ConditionFalse || crReadyCond.Reason != cmapi.CertificateRequestReasonFailed {
remaining = append(remaining, req)
continue
}
// TODO: once we have implemented exponential back off for
// Certificate failures, this should be changed accordingly.
now := c.clock.Now()
durationSinceFailure := now.Sub(cond.LastTransitionTime.Time)
if durationSinceFailure >= certificates.RetryAfterLastFailure {
certIssuingCond := apiutil.GetCertificateCondition(crt, cmapi.CertificateConditionIssuing)
if certIssuingCond == nil {
// This should never happen
log.V(logf.ErrorLevel).Info("Certificate does not have Issuing condition")
return nil, nil
}
// If the Issuing condition on the Certificate is newer than the
// failure time on CertificateRequest, it means that the
// CertificateRequest failed during the previous issuance (for the
// same revision). If it is a CertificateRequest that failed
// during the previous issuance, then it should be deleted so
// that we create a new one for this issuance.
if req.Status.FailureTime.Before(certIssuingCond.LastTransitionTime) {
log.V(logf.DebugLevel).Info("Found a failed CertificateRequest for previous issuance of this revision, deleting...")
if err := c.client.CertmanagerV1().CertificateRequests(req.Namespace).Delete(ctx, req.Name, metav1.DeleteOptions{}); err != nil {
return nil, err
}
continue
}
remaining = append(remaining, req)
}
return remaining, nil
}

40
pkg/controller/certificates/requestmanager/requestmanager_controller_test.go
View File

@ -80,12 +80,18 @@ func TestProcessItem(t *testing.T) {
)
fixedNow := metav1.NewTime(time.Now())
fixedClock := fakeclock.NewFakeClock(fixedNow.Time)
failedCRCondition := cmapi.CertificateRequestCondition{
failedCRConditionPreviousIssuance := cmapi.CertificateRequestCondition{
Type: cmapi.CertificateRequestConditionReady,
Status: cmmeta.ConditionFalse,
Reason: cmapi.CertificateRequestReasonFailed,
LastTransitionTime: &metav1.Time{Time: fixedNow.Time.Add(-1 * time.Hour)},
}
failedCRConditionThisIssuance := cmapi.CertificateRequestCondition{
Type: cmapi.CertificateRequestConditionReady,
Status: cmmeta.ConditionFalse,
Reason: cmapi.CertificateRequestReasonFailed,
LastTransitionTime: &metav1.Time{Time: fixedNow.Time.Add(1 * time.Minute)},
}
tests := map[string]struct {
// key that should be passed to ProcessItem.
// if not set, the 'namespace/name' of the 'Certificate' field will be used.
@ -534,7 +540,7 @@ func TestProcessItem(t *testing.T) {
},
certificate: gen.CertificateFrom(bundle1.certificate,
gen.SetCertificateNextPrivateKeySecretName("exists"),
gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionIssuing, Status: cmmeta.ConditionTrue}),
gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionIssuing, Status: cmmeta.ConditionTrue, LastTransitionTime: &fixedNow}),
gen.SetCertificateRevision(5),
),
requests: []runtime.Object{
@ -543,7 +549,8 @@ func TestProcessItem(t *testing.T) {
cmapi.CertificateRequestPrivateKeyAnnotationKey: "exists",
cmapi.CertificateRequestRevisionAnnotationKey: "6",
}),
gen.AddCertificateRequestStatusCondition(failedCRCondition),
gen.AddCertificateRequestStatusCondition(failedCRConditionPreviousIssuance),
gen.SetCertificateRequestFailureTime(metav1.Time{Time: fixedNow.Time.Add(time.Hour * -1)}),
),
},
expectedEvents: []string{`Normal Requested Created new CertificateRequest resource "test-notrandom"`},
@ -558,6 +565,29 @@ func TestProcessItem(t *testing.T) {
)), relaxedCertificateRequestMatcher),
},
},
"should do nothing if the CertificateRequest that is valid for spec has failed during this issuance cycle": {
secrets: []runtime.Object{
&corev1.Secret{
ObjectMeta: metav1.ObjectMeta{Namespace: "testns", Name: "exists"},
Data: map[string][]byte{corev1.TLSPrivateKeyKey: bundle1.privateKeyBytes},
},
},
certificate: gen.CertificateFrom(bundle1.certificate,
gen.SetCertificateNextPrivateKeySecretName("exists"),
gen.SetCertificateStatusCondition(cmapi.CertificateCondition{Type: cmapi.CertificateConditionIssuing, Status: cmmeta.ConditionTrue, LastTransitionTime: &fixedNow}),
gen.SetCertificateRevision(5),
),
requests: []runtime.Object{
gen.CertificateRequestFrom(bundle1.certificateRequest,
gen.SetCertificateRequestAnnotations(map[string]string{
cmapi.CertificateRequestPrivateKeyAnnotationKey: "exists",
cmapi.CertificateRequestRevisionAnnotationKey: "6",
}),
gen.AddCertificateRequestStatusCondition(failedCRConditionThisIssuance),
gen.SetCertificateRequestFailureTime(metav1.Time{Time: fixedNow.Time.Add(1 * time.Minute)}),
),
},
},
}
for name, test := range tests {
t.Run(name, func(t *testing.T) {
@ -575,9 +605,7 @@ func TestProcessItem(t *testing.T) {
if test.secrets != nil {
builder.KubeObjects = append(builder.KubeObjects, test.secrets...)
}
for _, req := range test.requests {
builder.CertManagerObjects = append(builder.CertManagerObjects, req)
}
builder.CertManagerObjects = append(builder.CertManagerObjects, test.requests...)
builder.Init()
// Register informers used by the controller using the registration wrapper