CertificateRequest status extension

[chojnack]

Is your feature request related to a problem? Please describe.
The CertificateRequest status currently supports only these Reason values:

const (
        CertificateRequestReasonPending = "Pending"
        CertificateRequestReasonFailed  = "Failed"
        CertificateRequestReasonIssued  = "Issued"
)

The Failed value is used to indicate errors when issuing the certificate which are always considered to be an intermittent, so the CertificateRequest is always re-tried after 1 hour.

However, when clearing the error requires change in the certificate request (e.g. fixing common name or domain names) then re-trying such request automatically doesn't make sense.

Describe the solution you'd like

I think additional Reason value should be added to indicate problems that require change in the request for which automatic retry will be blocked e.g:

const (
CertificateRequestReasonPending = "Pending"
CertificateRequestReasonFailed = "Failed"
CertificateRequestReasonFailedNoRetry = "FailedNoRetry"
CertificateRequestReasonIssued = "Issued"
)

Additional context
ADCS Issuer implementation

Environment details (if applicable):

• cert-manager version: 0.11.0

/kind feature

[munnerz]

👍 to this in terms of motivation - my only question would be around the terminology we use here for the reason string, although FailedNoRetry is at least clear and explicit for end-users.

cc @JoshVanL

[chojnack]

I agree that readability of the code should be the priority. Other option could be e.g. 'Rejected' but it will not be so clear reg. the expected reaction.
I suspect that there' may be more potential reasons of unsuccessful requests in context of different issuers and protocols and the need of adding each one to the list will be annoying. So better to leave the decision how each failure reason should be interpreted to the issuer implementation, and just inform the cert-manager core what reaction is expected (retry/ noretry/anythingelse? )

Jut my thoughts. I'm leaving the decision to you of course :-)
Marcin

[munnerz]

Another place where this would be useful #2370

[munnerz]

I've also raised this in the upstream discussion around extending the certificates.k8s.io CertificateSigningRequest type to include a signerName field: kubernetes/enhancements#1400 (comment)