Add support for Kubernetes CSR resources

[inteon]

This PR adds support for singing Kubernetes CSRs using your cluster issuer(s).
This features is enabled for anyone using this library.
The new Issuer interface requires you to implement the GetIssuerTypeIdentifier() function. The value returned by this function is used to determine if a Kubernetes CSR should be signed by the issuer.
eg. GetIssuerTypeIdentifier() returns "simpleissuers.issuer.cert-manager.io" and the CSR's signerName value is "simpleissuers.issuer.cert-manager.io/issuer-name".

CSR support is crucial for supporting integrations with other Cloud Native application that do not want to rely on the cert-manager APIs for their certificate creation (eg. service-meshes)

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [inteon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[inteon]

We don't give the Sign function direct access to the *cmapi.CertificateRequest or *certificatesv1.CertificateSigningRequest object.
Instead, the template *x509.Certificate contains all the decoded values from these resources.
metav1.Object can be used to get the labels/ annotations of the object.

[maelvls]

Question 1: I find the "interface" solution really elegant! I was trying to find another possible solution e.g. pass the data needed by the Sign and Check funcs every time but the interface solution is just better (and easier to document and test too)

My thinking was to pass the X.509 req straight to the Sign and Check funcs:

type Sign func(ctx, template *x509.Certificate, duration time.Duration, csr []byte, conds []cmapi.CertificateRequestCondition, issuer v1alpha1.Issuer) ([]byte, error)
type Check func(ctx, issuer v1alpha1.Issuer) error

[maelvls]

Question 2: with this interface, is it still possible for the user of issuer-lib to update the certificaterequest or certificatesigningrequest with extra conditions or send events with this approach? These two possibilities are important when the external issuer is particularly complex and the implementer wants to give more info at some "intermediate" stages of the Sign and Check func.

[inteon]

SetCertificateRequestConditionError can still be used to set custom conditions (https://github.com/cert-manager/issuer-lib/blob/main/controllers/signer/err_set_condition.go)

[maelvls]

Suggestion: I propose to add a note above the interface to explain that it isn't possible to set conditions "directly", but that it is possible to use SetCertificateRequestConditionError as a work around.

What about events?

[inteon]

You should be able to set/ send events using the eventRecorder that you keep before you pass it to CombinedController

issuer-lib/controllers/combined_controller.go

Line 49 in 2427784

EventRecorder record.EventRecorder

.
You won't be able to create events for the CR resource (because for some reason the event recorder wants a runtime.Object).
However, the issuer-lib will automatically create events. If we want to allow the implementer to create events for CR resources, we can still fix this in a follow-up PR ( without breaking the API ).

[maelvls]

That makes sense. Let's keep this "feature idea" in mind when we try to use issuer-lib in google-cas-issuer.

[maelvls]

Hey, here is a partial review, I haven't finished yet.

[maelvls]

I had not noticed v1alpha1.Issuer before. Why aren't we using the types that already exist in github.com/cert-manager/pkg/apis? Or do they not exist?

If we need these IssuerConditionReason constants, why do we use a version in the type? I though these versions only made sense in the context of CRDs, but we aren't defining the types for CRDs right?

Context:

const (
	// IssuerConditionReasonInitializing is the value assigned to
	// the Reason field of the Ready condition when issuer-lib first
	// reconciles an Issuer which does not already have a Ready
	// condition.
	IssuerConditionReasonInitializing = "Initializing"
	IssuerConditionReasonPending = "Pending"
	IssuerConditionReasonChecked = "Checked"
	IssuerConditionReasonFailed = "Failed"
)

[inteon]

The API does not match what we want to achieve eg. the IssuerStatus has this ACME field: https://github.com/cert-manager/cert-manager/blob/master/pkg/apis/certmanager/v1/types_issuer.go#L319C1-L333

[inteon]

The values you listed are not present in the cert-manager/cert-manager repo I think ( I wasn't able to find them ).

[maelvls]

Great work, Tim!! This PR is very well written. I haven't gone into the weeds of understanding how the new certificatesigningrequest_controller.go works but I am confident that what you have written is correct.

Since the user of the issuer-lib doesn't have access to the actual CertificateRequest or CertificateSigningRequest, let's make sure that everything they need to do is included. Right now, it is not possible to send custom events, but it is possible to set a custom condition thanks to the special error SetCertificateRequestConditionError.

/lgtm
/hold

(holding in case you want to address the non-blocking suggestions above)

[maelvls]

/lgtm

[inteon]

/unhold