cleanup: Scope mutating webhook to only certificaterequest resources

[hawksight]

Pull Request Motivation

Relating to the certificate-preset investigations we have been exploring ideas on how provide users of the Certificate resource without baking a new feature into cert-manager code. (See #2239 for back story).

As suggested by @wallrj in a slack thread, it seems that cert-manager only uses this mutating webhook for CertificateRequest resources, and not for actual certificates. (NOTE this may not be true and we should absolutely validate this).

The impact of this webhook was that it applied to Certificate resources when cert-manager does nothing useful with them at admission time. In fact as @erikgb has pointed out in slack, the required fields issuerRef and secretName do not set omitempty on the resources here and here.

• So an empty value is applied to these fields when they are not present in the Certificate resource supplied.
• This means that other mutation policies may well skip injecting a default because the value, although blank, has already been provided.
• The empty value is then rejected by the cert-manager validating webhook as it needs a value.

This change basically makes enables other mutating webhooks to apply a default value to the field before validation. Because now the mutating webhook does not pickup Certificate resources and cause the small change of setting blanks.

Kind

claeanup?

Release Note

cleanup: Restrict mutatingwebhookconfiguration to only CertificateRequest resources

[jetstack-bot]

Hi @hawksight. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

[hawksight]

I should add here that I will look through again at the cert-manager code around webhooks. The only reason to catch all cert-manager / acme resources would be to convert the API version. I think in previous discussions we don't do that anymore as everyone should not be on v1 versions of all the core CRDs. We are also 12 minor versions ins (soon 13), so even if we did convert them still, it might be time to remove that anyway as we support the last 4 minor versions from latest.

[maelvls]

/ok-to-test

[inteon]

/ok-to-test

[SgtCoDFish]

/kind cleanup

[erikgb]

The only reason to catch all cert-manager / acme resources would be to convert the API version.

@hawksight I think the conversion webhooks are not relevant here if that's what you mean. They are configured on the CRDs.

[hawksight]

@hawksight I think the conversion webhooks are not relevant here if that's what you mean. They are configured on the CRDs.

I was thinking that perhaps the use of Equivalent in the mutating webhook might be the trigger here that is catching non v1 versions of all cert-manager resources and prompts a mutation of the object?

But are we confident that cert-manager only converts CRDs themselves and not instances of Certificates for example?
Is there another place where the CRD conversion is triggered?

[erikgb]

@hawksight I think the conversion webhooks are not relevant here if that's what you mean. They are configured on the CRDs.

I was thinking that perhaps the use of Equivalent in the mutating webhook might be the trigger here that is catching non v1 versions of all cert-manager resources and prompts a mutation of the object?

But are we confident that cert-manager only converts CRDs themselves and not instances of Certificates for example? Is there another place where the CRD conversion is triggered?

Any conversion webhooks should be defined on the CRD (but apply to the resources of that kind). See here for reference: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#configure-customresourcedefinition-to-use-conversion-webhooks. And I don't think we have any conversion webhooks defined in cert-manager at present.

[erikgb]

/lgtm (which I should not be eligible for in this project)

[erikgb]

/lgtm

[jetstack-bot]

@erikgb: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

/lgtm

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/test-infra repository.

[hawksight]

@munnerz - talking with @SgtCoDFish this morning he suggested you might have a good understanding in the area of cert-manager's mutating webhooks. Might you be able to take a look at this relatively small change please? I'm mostly checking there is no unintended consequences from reducing the scope of the mutating webhook here.

[munnerz]

One thing to note: this does mean we won't mutate any subresource requests for certificaterequests anymore. I think this is totally fine - the only time we need to inspect requests to certificaterequests/status is in the approval admission plugin: https://github.com/cert-manager/cert-manager/blob/master/internal/plugin/admission/certificaterequest/approval/certificaterequest_approval.go

This is a validating plugin only however, not mutating, so this is fine 👍

[munnerz]

This LGTM and I think is quite a big win overall - thanks for doing this investigation!

I am a little concerned that some clients may have come to rely on this value being default to "" rather than not set at all, however given mutating webhooks are called in a non-deterministic order, this is already quite a grey area for our users and I imagine most webhooks will be asserting on the length of the value being zero, which it still is.

Looking at our defaulting functions registered with our scheme (which get applied at the point the webhook decodes the json object), they are empty across all API groups too: https://github.com/cert-manager/cert-manager/blob/master/internal/apis/certmanager/v1/defaults.go

You can also see what kind of admission plugins we actually have by looking here: https://github.com/cert-manager/cert-manager/tree/master/internal/plugin/admission - as noted above, only one of these is mutating and it only applies on 'create' operations :)

I think we could probably also go a step further and remove the mutating webhook for UPDATE operations specifically too (though we do still need the validating webhook for updates, to prevent people changing the userInfo fields).

Our tests are passing here, and we don't require mutation for any other resource types...

/lgtm
/approve
/hold (to give anyone else time to respond/review :))

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: erikgb, munnerz

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 [munnerz]

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

[hawksight]

Thank you @munnerz for looking over that. I've removed the UPDATE operation as well. Might as well fully scope it down now whilst thinking about it.

The links where particularly helpful for my understanding.

[munnerz]

Awesome, looks great! 🚀

/lgtm

[munnerz]

Sounds like everyone is broadly in favour here :) let's see how this goes 🚀

/hold cancel

[hawksight]

Adding some documentation here as to other potential workarounds to the problem if you happen to be running cert-manager v1.14.x and are unable to upgrade.

Extract from notes

If you are running a cert-manager installation prior to v1.14.x you should first consider upgrading.
If upgrading is not feasible right now then you will need to consider one of the following potential fixes for this issue:

1. Rename the 'cert-manager-webhook' mutating and validating webhooks with z-<existing name> so that they execute last, after the Kyverno webhooks.

2. Fix the cert-manager mutating webhook not trigger for Certificate resource changes, as in PR #6311.

3. Use enforcement in your policy to explicitly override the value regardless of what the user sets.

Option one seems to work but generally should be avoided as order cannot be counted on. Mutating webhooks should also be idempotent.
Option three defeats the point of allowing a user to override when needed, so discounted on the basis that I am looking at defaulting solutions as opposed to enforcement solutions.

Option two is recommended, as this is the fix that is implemented in later versions of cert-manager, based on this PR #6311.

To manually patch the webhook we have a YAML patch resource:

webhooks:
- name: webhook.cert-manager.io
  namespaceSelector: {}
  objectSelector: {}
  reinvocationPolicy: Never
  rules:
  - apiGroups:
    - cert-manager.io
    apiVersions:
    - v1
    operations:
    - CREATE
    resources:
    - 'certificaterequests'
    scope: '*'

To validate exactly what this patch will do, simply run the following to see a kubectl diff:

kubectl patch mutatingwebhookconfigurations.admissionregistration.k8s.io cert-manager-webhook --patch-file patches/mutatingwebhookconfiguration-patch.yaml --dry-run=server -o yaml | kubectl diff -f -

And to apply the patch so it takes effect:

kubectl patch mutatingwebhookconfigurations.admissionregistration.k8s.io cert-manager-webhook --patch-file patches/mutatingwebhookconfiguration-patch.yaml