diff --git a/pkg/controller/certificates/issuing/issuing_controller.go b/pkg/controller/certificates/issuing/issuing_controller.go index 49816f435..9209d2ad1 100644 --- a/pkg/controller/certificates/issuing/issuing_controller.go +++ b/pkg/controller/certificates/issuing/issuing_controller.go @@ -279,25 +279,31 @@ func (c *controller) ProcessItem(ctx context.Context, key string) error { 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 - // certificate object when the issuer ignores the "Denied" state. - // - // To know whether or not an issuer ignores the "Denied" state, we pay - // attention to the "Ready" condition on the certificate request. If a - // 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. + // Now check if CertificateRequest is in any of the final states so that + // this issuance can be completed as either succeeded or failed. Failed + // issuance will be retried with a delay (the logic for that lives in + // certificates-trigger controller). + // Final states are: + // Denied condition with status True => fail issuance + // InvalidRequest condition with status True => fail issuance + // Ready conidtion with reason Failed => fail issuance + // Ready condition with reason Issued => finalize issuance as succeeded + + // If the certificate request was denied, set the last failure time to + // now, bump the issuance attempts and set the Issuing status condition + // to False. + if apiutil.CertificateRequestIsDenied(req) { + return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionDenied)) + } + + // If the certificate request is invalid, set the last failure time to + // now, bump the issuance attempts and set the Issuing status condition + // to False. + if apiutil.CertificateRequestHasInvalidRequest(req) { + return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionInvalidRequest)) + } + if crReadyCond == nil { - if apiutil.CertificateRequestIsDenied(req) { - return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionDenied)) - } - - if apiutil.CertificateRequestHasInvalidRequest(req) { - return c.failIssueCertificate(ctx, log, crt, apiutil.GetCertificateRequestCondition(req, cmapi.CertificateRequestConditionInvalidRequest)) - } - log.V(logf.DebugLevel).Info("CertificateRequest does not have Ready condition, waiting...") return nil } diff --git a/pkg/controller/certificates/issuing/issuing_controller_test.go b/pkg/controller/certificates/issuing/issuing_controller_test.go index 2b911fed0..859f95986 100644 --- a/pkg/controller/certificates/issuing/issuing_controller_test.go +++ b/pkg/controller/certificates/issuing/issuing_controller_test.go @@ -1010,7 +1010,7 @@ func TestIssuingController(t *testing.T) { }, expectedErr: false, }, - "if certificate is in Issuing state, one CertificateRequest, but has been denied, report denial and set last failed time and issuance attempts": { + "if certificate is in Issuing state, one CertificateRequest without Ready condition, but with Denied condition, report denial and set last failed time and issuance attempts": { certificate: exampleBundle.Certificate, builder: &testpkg.Builder{ CertManagerObjects: []runtime.Object{ @@ -1062,6 +1062,290 @@ func TestIssuingController(t *testing.T) { }, expectedErr: false, }, + "if certificate is in Issuing state, one CertificateRequest with a pending Ready condition and a Denied condition, report denial and set last failed time and issuance attempts": { + certificate: exampleBundle.Certificate, + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{ + gen.CertificateFrom(issuingCert), + gen.CertificateRequestFrom(exampleBundle.CertificateRequest, + gen.AddCertificateRequestAnnotations(map[string]string{ + cmapi.CertificateRequestRevisionAnnotationKey: "2", // Current Certificate revision=1 + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionDenied, + Status: cmmeta.ConditionTrue, + Reason: "DeniedReason", + Message: "The certificate request has been denied", + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionFalse, + Reason: "Pending", + Message: "The certificate request is pending", + }), + )}, + 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{ + testpkg.NewAction(coretesting.NewUpdateSubresourceAction( + cmapi.SchemeGroupVersion.WithResource("certificates"), + "status", + exampleBundle.Certificate.Namespace, + gen.CertificateFrom(exampleBundle.Certificate, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{ + Type: cmapi.CertificateConditionIssuing, + Status: cmmeta.ConditionFalse, + Reason: "DeniedReason", + Message: "The certificate request has failed to complete and will be retried: The certificate request has been denied", + LastTransitionTime: &metaFixedClockStart, + ObservedGeneration: 3, + }), + gen.SetCertificateLastFailureTime(metaFixedClockStart), + gen.SetCertificateIssuanceAttempts(pointer.Int(1)), + ), + )), + }, + ExpectedEvents: []string{ + "Warning DeniedReason The certificate request has failed to complete and will be retried: The certificate request has been denied", + }, + }, + expectedErr: false, + }, + "if certificate is in Issuing state, one CertificateRequest that has been issued, but also has a Denied condition, report denial and set last failed time and issuance attempts": { + certificate: exampleBundle.Certificate, + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{ + gen.CertificateFrom(issuingCert), + gen.CertificateRequestFrom(exampleBundle.CertificateRequest, + gen.AddCertificateRequestAnnotations(map[string]string{ + cmapi.CertificateRequestRevisionAnnotationKey: "2", // Current Certificate revision=1 + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionDenied, + Status: cmmeta.ConditionTrue, + Reason: "DeniedReason", + Message: "The certificate request has been denied", + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionTrue, + Reason: "Issued", + Message: "The certificate request has been issued", + }), + )}, + 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{ + testpkg.NewAction(coretesting.NewUpdateSubresourceAction( + cmapi.SchemeGroupVersion.WithResource("certificates"), + "status", + exampleBundle.Certificate.Namespace, + gen.CertificateFrom(exampleBundle.Certificate, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{ + Type: cmapi.CertificateConditionIssuing, + Status: cmmeta.ConditionFalse, + Reason: "DeniedReason", + Message: "The certificate request has failed to complete and will be retried: The certificate request has been denied", + LastTransitionTime: &metaFixedClockStart, + ObservedGeneration: 3, + }), + gen.SetCertificateLastFailureTime(metaFixedClockStart), + gen.SetCertificateIssuanceAttempts(pointer.Int(1)), + ), + )), + }, + ExpectedEvents: []string{ + "Warning DeniedReason The certificate request has failed to complete and will be retried: The certificate request has been denied", + }, + }, + expectedErr: false, + }, + "if certificate is in Issuing state, one CertificateRequest that has no ready condition and has been marked as invalid, report error and set last failed time and issuance attempts": { + certificate: exampleBundle.Certificate, + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{ + gen.CertificateFrom(issuingCert), + gen.CertificateRequestFrom(exampleBundle.CertificateRequest, + gen.AddCertificateRequestAnnotations(map[string]string{ + cmapi.CertificateRequestRevisionAnnotationKey: "2", // Current Certificate revision=1 + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionInvalidRequest, + Status: cmmeta.ConditionTrue, + Reason: "InvalidRequest", + Message: "The certificate request is invalid", + }), + )}, + 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{ + testpkg.NewAction(coretesting.NewUpdateSubresourceAction( + cmapi.SchemeGroupVersion.WithResource("certificates"), + "status", + exampleBundle.Certificate.Namespace, + gen.CertificateFrom(exampleBundle.Certificate, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{ + Type: cmapi.CertificateConditionIssuing, + Status: cmmeta.ConditionFalse, + Reason: "InvalidRequest", + Message: "The certificate request has failed to complete and will be retried: The certificate request is invalid", + LastTransitionTime: &metaFixedClockStart, + ObservedGeneration: 3, + }), + gen.SetCertificateLastFailureTime(metaFixedClockStart), + gen.SetCertificateIssuanceAttempts(pointer.Int(1)), + ), + )), + }, + ExpectedEvents: []string{ + "Warning InvalidRequest The certificate request has failed to complete and will be retried: The certificate request is invalid", + }, + }, + expectedErr: false, + }, + "if certificate is in Issuing state, one CertificateRequest that has a pending ready condition and has been marked as invalid, report error and set last failed time and issuance attempts": { + certificate: exampleBundle.Certificate, + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{ + gen.CertificateFrom(issuingCert), + gen.CertificateRequestFrom(exampleBundle.CertificateRequest, + gen.AddCertificateRequestAnnotations(map[string]string{ + cmapi.CertificateRequestRevisionAnnotationKey: "2", // Current Certificate revision=1 + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionInvalidRequest, + Status: cmmeta.ConditionTrue, + Reason: "InvalidRequest", + Message: "The certificate request is invalid", + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionFalse, + Reason: "Pending", + Message: "The certificate request is being issued", + }), + )}, + 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{ + testpkg.NewAction(coretesting.NewUpdateSubresourceAction( + cmapi.SchemeGroupVersion.WithResource("certificates"), + "status", + exampleBundle.Certificate.Namespace, + gen.CertificateFrom(exampleBundle.Certificate, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{ + Type: cmapi.CertificateConditionIssuing, + Status: cmmeta.ConditionFalse, + Reason: "InvalidRequest", + Message: "The certificate request has failed to complete and will be retried: The certificate request is invalid", + LastTransitionTime: &metaFixedClockStart, + ObservedGeneration: 3, + }), + gen.SetCertificateLastFailureTime(metaFixedClockStart), + gen.SetCertificateIssuanceAttempts(pointer.Int(1)), + ), + )), + }, + ExpectedEvents: []string{ + "Warning InvalidRequest The certificate request has failed to complete and will be retried: The certificate request is invalid", + }, + }, + expectedErr: false, + }, + "if certificate is in Issuing state, one CertificateRequest that has been issued, but has also been marked as invalid, report error and set last failed time and issuance attempts": { + certificate: exampleBundle.Certificate, + builder: &testpkg.Builder{ + CertManagerObjects: []runtime.Object{ + gen.CertificateFrom(issuingCert), + gen.CertificateRequestFrom(exampleBundle.CertificateRequest, + gen.AddCertificateRequestAnnotations(map[string]string{ + cmapi.CertificateRequestRevisionAnnotationKey: "2", // Current Certificate revision=1 + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionInvalidRequest, + Status: cmmeta.ConditionTrue, + Reason: "InvalidRequest", + Message: "The certificate request is invalid", + }), + gen.SetCertificateRequestStatusCondition(cmapi.CertificateRequestCondition{ + Type: cmapi.CertificateRequestConditionReady, + Status: cmmeta.ConditionTrue, + Reason: "Issued", + Message: "The certificate request has been issued", + }), + )}, + 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{ + testpkg.NewAction(coretesting.NewUpdateSubresourceAction( + cmapi.SchemeGroupVersion.WithResource("certificates"), + "status", + exampleBundle.Certificate.Namespace, + gen.CertificateFrom(exampleBundle.Certificate, + gen.SetCertificateStatusCondition(cmapi.CertificateCondition{ + Type: cmapi.CertificateConditionIssuing, + Status: cmmeta.ConditionFalse, + Reason: "InvalidRequest", + Message: "The certificate request has failed to complete and will be retried: The certificate request is invalid", + LastTransitionTime: &metaFixedClockStart, + ObservedGeneration: 3, + }), + gen.SetCertificateLastFailureTime(metaFixedClockStart), + gen.SetCertificateIssuanceAttempts(pointer.Int(1)), + ), + )), + }, + ExpectedEvents: []string{ + "Warning InvalidRequest The certificate request has failed to complete and will be retried: The certificate request is invalid", + }, + }, + expectedErr: false, + }, } for name, test := range tests {