Unnecessary patch for CA injection

[lentzi90]

What broke? What's expected?

The CA injection patch is not necessary after the switch to replacements instead of vars. To be clear, it is not breaking anything either, it just doesn't add anything on top of what the replacements already does. The reason is that the replacements have create: true, which means they will create the annotation if it doesn't exist.

Since the replacements adds the annotations, it also means that ALL CRDs will get the CA injected instead of just those that have the patch (when uncommenting only some of them, if you have multiple). Is this intentional? I guess it will not cause problems but wanted to double check.

Reproducing this issue

Simply following the quick start: https://book.kubebuilder.io/quick-start

mkdir -p ~/projects/guestbook
cd ~/projects/guestbook
kubebuilder init --domain my.domain --repo my.domain/guestbook
kubebuilder create api --group webapp --version v1 --kind Guestbook --controller --resource
make manifests

After this you will see that config/crd/patches contains cainjection_in_guestbooks.yaml, but we need to add a webhook to make it useful.

Create a webhook:

kubebuilder create webhook --group webapp --version v1 --kind Guestbook --defaulting --programmatic-validation --conversion
make manifests

Now uncomment the [WEBHOOK] and [CERTMANAGER] in config/default/kustomization.yaml. The comments in the file explains that one should uncomment the same sections in config/crd/kustomization.yaml. However, commenting/uncommenting the following lines does not affect the outcome:

# In default/kustomization.yaml
# [CERTMANAGER] To enable cert-manager, uncomment all sections with 'CERTMANAGER'.
# Uncomment 'CERTMANAGER' sections in crd/kustomization.yaml to enable the CA injection in the admission webhooks.
# 'CERTMANAGER' needs to be enabled to use ca injection
# - webhookcainjection_patch.yaml

# In crd/kustomization.yaml
# [CERTMANAGER] To enable cert-manager, uncomment all the sections with [CERTMANAGER] prefix.
# patches here are for enabling the CA injection for each CRD
# - path: patches/cainjection_in_guestbooks.yaml
#+kubebuilder:scaffold:crdkustomizecainjectionpatch

We can check like this:

kustomize build config/default | grep "cert-manager.io/inject-ca-from"

Try running the command with the lines commented/uncommented and see that there is no difference in the output.

The output is as follows:

# With all uncommented:
    cert-manager.io/inject-ca-from: guestbook-system/guestbook-serving-cert
    cert-manager.io/inject-ca-from: guestbook-system/guestbook-serving-cert
    cert-manager.io/inject-ca-from: guestbook-system/guestbook-serving-cert

# With the two mentioned lines commented:
    cert-manager.io/inject-ca-from: guestbook-system/guestbook-serving-cert
    cert-manager.io/inject-ca-from: guestbook-system/guestbook-serving-cert
    cert-manager.io/inject-ca-from: guestbook-system/guestbook-serving-cert

# With the two mentioned lines uncommented, and the replacements commented:
    cert-manager.io/inject-ca-from: CERTIFICATE_NAMESPACE/CERTIFICATE_NAME
    cert-manager.io/inject-ca-from: CERTIFICATE_NAMESPACE/CERTIFICATE_NAME
    cert-manager.io/inject-ca-from: CERTIFICATE_NAMESPACE/CERTIFICATE_NAME

From this it should be clear that the replacements in config/default/kustomization.yaml is enough to inject the CA and the patches are not needed.

KubeBuilder (CLI) Version

Version: main.version{KubeBuilderVersion:"3.11.1", KubernetesVendor:"1.27.1", GitCommit:"1dc8ed95f7cc55fef3151f749d3d541bec3423c9", BuildDate:"2023-07-03T13:10:56Z", GoOs:"linux", GoArch:"amd64"}

PROJECT version

3

Plugin versions

layout:
- go.kubebuilder.io/v4

Other versions

No response

Extra Labels

No response

[camilamacedo86]

HI @lentzi90,

Thank you for raise that. By looking the description it seems make sense.
Could you please push a PR for we change it with your suggestion so that we can discuss from there?

note that the change would only be made for the plugin kustomize/v2 here: https://github.com/kubernetes-sigs/kubebuilder/tree/master/pkg/plugins/common/kustomize/v2/scaffolds/internal/templates/config

You can change the plugin and its template
Then, run make generate to ensure that all samples under testdata and from docs will be properly updated.

[camilamacedo86]

Hi @lentzi90,

Thank you for pointing this out and for the PR. Upon reviewing your PR (#3555), it seems that the root of the issue might lie in the replacements. We must consider genuine scenarios where users might want to decide which resource should have the cert-manager injected.

Rather than removing the injections altogether, it might be more appropriate to address the replacements. Our goal should be to ensure that the certmanager is only injected into the uncommented files.

Would you like to help us to sorting it out by changing the replacement to ensure that we will ONLY inject the cert-manager in the CRDs and/or webhooks which are uncommented?

Following example scenarios where we might not inject the cert-manager in all:

Use Case Scenarios for Selectively Using cert-manager on CRD Webhooks

1. Public vs. Internal Exposure

• CRD A: Interacts with external systems. Its webhook requires public accessibility.

  apiVersion: cert-manager.io/v1
  kind: Certificate
  metadata:
    name: crd-a-cert
  spec:
    # ... specify certificate details for public CA ...

• CRD B: Strictly for internal operations. Uses self-signed or cluster-internal CA.

  # Using Kubernetes to generate self-signed certificate

2. Complex Multi-Cluster Scenario

• CRD A: Webhook needs to be trusted across multiple clusters. cert-manager manages a common certificate.

  # ... specify certificate details for cross-cluster trust ...

• CRD B: Webhook is cluster-local. A local certificate mechanism suffices.

3. Different Compliance Requirements

• CRD A: Processes sensitive data. Requires stringent compliance and auditability.

  # ... specify certificate details with CA that meets compliance needs ...

• CRD B: No stringent compliance needs. Simple certificate mechanism suffices.

4. Differing Availability Needs

• CRD A: Critical webhook. cert-manager ensures automatic, timely renewal.

  # ... specify certificate details ...

• CRD B: Non-critical webhook. Temporary unavailability due to certificate issues is acceptable.

5. Legacy vs. New Systems

• CRD A: Modern tooling and practices. Using cert-manager aligns with this approach.

  # ... specify modern certificate details ...

• CRD B: Legacy system. Uses an older mechanism for certificate management.

6. Development/Test vs. Production

• CRD A: Used in both development and production. Different strategies for each

[lentzi90]

Thanks for the thorough explanation, very appreciated! This is exactly why I brought the issue upstream instead of just addressing it in our downstream. Now hopefully we can find a proper solution that will work for all cases.

[lentzi90]

Alright I have a suggestion! First some background: Kustomize does not accept selecting a resource and then finding out that it is impossible to apply the replacement. So if we select it, it must have the annotation in our case, unless we tell kustomize to create it. That is where we are today.

There are a couple of alternatives I see here:

1. Make the replacements more specific. Instead of selecting all MutatingWebhookConfigurations and ValidatingWebhookConfigurations, select by name and list all of them. The user can then decide which to uncomment. We can keep the create: true and the CA injection annotation patch is not needed.
2. Same as the above, but keep the CA injection annotation patch and set create: false.

I would prefer option 1 since the replacements then become the source of truth and we reduce the number of files and patches, but honestly they are very similar.

Here is an example of what this would look like. Check

kubebuilder/pkg/plugins/common/kustomize/v2/scaffolds/internal/templates/config/kdefault/kustomization.go

Lines 105 to 112 in d2fa79c

	# - select:
	# kind: ValidatingWebhookConfiguration
	# fieldPaths:
	# - .metadata.annotations.[cert-manager.io/inject-ca-from]
	# options:
	# delimiter: '/'
	# index: 0
	# create: true

In here we have a select for kind: ValidatingWebhookConfiguration which will match all of them. We would change this to include name: foo and then list all names that we know about. The list would be the same as the list of patches we currently produce here: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/common/kustomize/v2/scaffolds/internal/templates/config/crd/kustomization.go

Now I'm not quite sure how to implement this so if someone wants to pick it up, I would be happy to hand it over. Otherwise I guess I will try to figure out a way. 🙂

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale