search

kubernetes-sigs / kubebuilder

Kubebuilder - SDK for building Kubernetes APIs using CRDs
http://book.kubebuilder.io
Apache License 2.0
7.86k stars 1.44k forks source link

Unnecessary patch for CA injection #3538

Closed lentzi90 closed 2 weeks ago

lentzi90 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Hi @lentzi90,

Thank you for pointing this out and for the PR. Upon reviewing your PR (https://github.com/kubernetes-sigs/kubebuilder/pull/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

2. Complex Multi-Cluster Scenario

3. Different Compliance Requirements

4. Differing Availability Needs

5. Legacy vs. New Systems

6. Development/Test vs. Production

lentzi90 commented 1 year ago

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 commented 1 year ago

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 https://github.com/kubernetes-sigs/kubebuilder/blob/d2fa79c922614cf0edd02f96b4305f1e1bac89ca/pkg/plugins/common/kustomize/v2/scaffolds/internal/templates/config/kdefault/kustomization.go#L105-L112 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. :slightly_smiling_face:

k8s-triage-robot commented 9 months ago

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

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

You can:

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

/lifecycle stale