Allow use of all signers by default

[SgtCoDFish]

This commit changes signer allowlisting, to default to allowing approval for all signers.

This makes csi-driver-spiffe much simpler to use for external issuers such as aws-privateca-issuer, and is justified by the same change being made recently in approver-policy

[inteon]

@SgtCoDFish Do you know how we prevent the csi-driver-spiffe approver from conflicting with other approvers (like approver-policy)?
I think in approver-policy the policies are bound specifically to a set of CSRs using RBAC and selectors in the policy.
Does this project check for an annotation before it performs approval?

Does removing restrictions on signers increase the chance of conflicts between approvers?

[SgtCoDFish]

Do you know how we prevent the csi-driver-spiffe approver from conflicting with other approvers (like approver-policy)?

Mostly the logic is here:

csi-driver-spiffe/internal/approver/controller/controller.go

Lines 84 to 106 in abc426a

	WithEventFilter(predicate.NewPredicateFuncs(func(obj client.Object) bool {
	var req cmapi.CertificateRequest
	err := a.lister.Get(ctx, client.ObjectKeyFromObject(obj), &req)
	if apierrors.IsNotFound(err) {
	// Ignore CertificateRequests that have been deleted.
	return false
	}
	// If an error happens here and we do nothing, we run the risk of not
	// processing CertificateRequests.
	// Exiting error is the safest option, as it will force a resync on all
	// CertificateRequests on start.
	if err != nil {
	a.log.Error(err, "failed to list all CertificateRequests, exiting error")
	os.Exit(1)
	}
	// Ignore requests that already have an Approved or Denied condition.
	if apiutil.CertificateRequestIsApproved(&req) || apiutil.CertificateRequestIsDenied(&req) {
	return false
	}
	return req.Spec.IssuerRef == a.issuerRef

We filter out CRs that don't look like they were created by csi-driver-spiffe. Currently, we look for the issuerRef to match. That'll change in #125 hopefully to check for an annotation but that'll only be ready to merge after this one goes in.

In any case, that's not really related to the changes in this PR.

Does this project check for an annotation before it performs approval?

It will after #125 merges but doesn't yet.

Does removing restrictions on signers increase the chance of conflicts between approvers?

No, this change just makes it easier to configure an external issuer and has no impact on how we select which CRs to reconcile or approve. #125 will change that a bit.

Ultimately the current design of approval as a concept is always going to create approval race conditions. If one approver would approve a CR but another would deny, it's a race to see which goes first. We rely on users configuring things correctly so that approvers don't conflict and there's almost nothing an individual approver can do to prevent that as far as I can see.

[inteon]

Ok, I think with #125 we will better prevent conflicts and thus it is ok to allow the approver to approve all issuer types.
Thanks for explaining.
/approve
/lgtm

[cert-manager-prow]

[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