Add Helm options to extend auto-approval or disable it

[inteon]

This PR adds Helm options to simplify configuring the build-in cert-manager auto-approver.

1. It adds the option to approve external issuers (including an "approve all external issuers" options)
2. It adds the option to easily disable the auto-approver

This will simplify installing cert-manager combined with an external issuer or in combination with approver-policy (https://cert-manager.io/docs/policy/approval/approver-policy/installation/).

Fixes #6521

NOTE: We will keep the default "only approve cert-manager.io Issuer and ClusterIssuer" rule. In approver-policy and csi-driver-spiffe, we removed this constraint. The reason we keep the constraint here is because cert-manager's auto-approver will automatically approve all CertificateRequests that it can approve (which is not the case for approver-policy and csi-driver-spiffe). To prevent breaking existing setups (prevent two approvers from racing to approve the same resource), we will not expand the default set of issuers.

Kind

/kind feature

Release Note

If the `--controllers` flag only specifies disabled controllers, the default controllers are now enabled implicitly.
Added `disableAutoApproval` and `approveSignerNames` Helm chart options.

[ThatsMrTalbot]

/lgtm

[SgtCoDFish]

/test pull-cert-manager-master-make-test

[SgtCoDFish]

Couple of bits - what do you think?

[SgtCoDFish]

suggestion: I think this could use manual testing to confirm that it does what's expected. Notably this looks different to how we set this in docs which is usually --controllers=*\,-certificaterequests-approver'

I'm not saying this is wrong, but I think it would be helpful to show that it's right in the PR description!

[SgtCoDFish]

Also, how does this interact with someone setting the --controllers flag through extraArgs in the chart?

[inteon]

I was just testing this and there seems to be an issue still:

This table describes the current behavior:

input	controllers
flags:
	*
--controllers=
--controllers=-certificaterequests-approver
--controllers= --controllers=-certificaterequests-approver
--controllers=test --controllers=-certificaterequests-approver	test
--controllers=* --controllers=-certificaterequests-approver	* - certificaterequests-approver
--controllers=* --controllers=test --controllers=-certificaterequests-approver	* + test - certificaterequests-approver
configfile:
{}	*
{controllers: []}	*
{controllers: [ "*" ]}	*
{controllers: [ "-certificaterequests-approver" ]}
{controllers: [ "test", "-certificaterequests-approver" ]}	test

It is clear that there is no way to specify that the certificaterequests-approver controller should be disabled regardless of the other values passed to --controllers (if nothing is passed to --controllers, setting --controllers=-certificaterequests-approver disables all controllers).

I wonder if we should change the logic behind --controllers and make it behave as follows:

input	controllers
flags:
	*
--controllers=
--controllers=-certificaterequests-approver	*
--controllers= --controllers=-certificaterequests-approver	*
--controllers=test --controllers=-certificaterequests-approver	test
--controllers=* --controllers=-certificaterequests-approver	* - certificaterequests-approver
--controllers=* --controllers=test --controllers=-certificaterequests-approver	* + test - certificaterequests-approver
configfile:
{}	*
{controllers: []}	*
{controllers: [ "*" ]}	*
{controllers: [ "-certificaterequests-approver" ]}	*
{controllers: [ "test", "-certificaterequests-approver" ]}	test

This would allow us to specify flags more independently.
Additionally, this should cause minimal breakage since it is currently not useful to specify just --controllers=-certificaterequests-approver.

[SgtCoDFish]

I think what you propose is pretty unsurprising for a user, and it wouldn't be bad. In a different place you said that our current logic is mirroring k8s - it'd be good to know if there's a reason they chose to do it the way they did.

I think an alternative approach (given that the only controller regularly disabled this way) is to add an entirely new flag, --disable-default-approver.

Obviously that's making a special case out of the default approver, but I think it is a special case.

I'd envision that first we'd parse the --controllers flag, and then afterwards if --disable-default-approver is set, remove the defeault approver from the list of controllers if it's present.

[inteon]

The upstream implementation has not changed much since its introduction in 2017: kubernetes/kubernetes#39740.
I don't think they specifically looked at this specific situation (a setup with only negative controller selectors).

[inteon]

I implemented the proposed change in this commit: 985607b

Fixed/ addressed

[ThatsMrTalbot]

I think the new option behaviour makes sense. I think a dedicated flag would be nice long term, but this is a good start for install UX.
/lgtm

[inteon]

/approve

[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

[inteon]

/unhold

[inteon]

/cherrypick release-1.15

[cert-manager-bot]

@inteon: failed to push cherry-picked changes in GitHub: error pushing "cherry-pick-7049-to-release-1.15": exit status 1 To https://github.com/cert-manager-bot/cert-manager
! [remote rejected] cherry-pick-7049-to-release-1.15 -> cherry-pick-7049-to-release-1.15 (refusing to allow a Personal Access Token to create or update workflow .github/workflows/govulncheck.yaml without workflow scope)
error: failed to push some refs to 'https://github.com/cert-manager-bot/cert-manager'

In response to this:

/cherrypick release-1.15

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[inteon]

/cherrypick release-1.15

[cert-manager-bot]

@inteon: new pull request created: #7054

In response to this:

/cherrypick release-1.15

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.