fix: Inject custom ca and certificates

[romachalm]

What this PR does / why we need it:

Currently, the certificate is automatically created and rotated by the controller, and we cannot use custom certificates.
This PR implements :

• Helm parameter to deactivate the creation and rotation of the certificate by the controller
• Ability to add annotations in webhooks (to use for instance ca-injector by cert-manager)
• Helm parameter to pass custom certificate secret in controller
• Documentation to inject cert-manager certificates

Which issue(s) this PR fixes:
Fixes #520

Special notes for your reviewer:
I have chosen not to inject specific cert-manager annotations to keep the helm chart open. The usage of annotations in webhooks may be interesting for other integrations.

Activating both the certRotation (default) and the ca-injection (annotation) produces a race condition between the controller and the ca-injector : it loops creating generations of webhooks. I haven't found an obvious way to prevent this situation. This is documented as a warning in the doc.

I have tested on my dev platform using cert-manager 1.3 and kubernetes 1.19

[romachalm]

I signed it

[maxsmythe]

Thank you for the PR!

Unfortunately we cannot support third-party integrations with our Helm chart for support scalability reasons:

• We cannot take on the responsibility of keeping track of any changes to the cert-manager (or any other) API
• We cannot know what version of cert-manager (or any other project) users have installed on their cluster (if any), or the impact of version skew
• We cannot maintain the ability to field support requests for problems with cert-manager (or any other third-party project).
• We do not know how many other, similar integrations people may want
• We do not know people's clusters and what configurations make sense for all use cases.

I wonder if there is a home we could give this third party integrations that doesn't imply we are are committing to supporting these integrations? I think a contrib repo or directory was mentioned in the past.

[maxsmythe]

It looks like a large part of this PR is exposing individual annotations, which I think does make sense and gives people flexibility, rather than a hard coupling with cert-manager.

It looks like the other part of the PR toggles the creation of the secret and makes the name configurable?

If we remove the cert-manager-specific support docs, I think this makes sense.

[maxsmythe]

Apologies, I assumed this would be more vendor-specific than it turned out to be when I dug in.

I added some suggestions for how to make this more generic. Mostly docs changes, the Helm changes are already fairly generic.

[maxsmythe]

We should make this more generic: "Managing certificates outside of Gatekeeper"

[maxsmythe]

Or similar, not in love with that title.

[romachalm]

I understand the concern.
However, I use here the ca-injector from cert-manager to change the CABundle in the webhook (through annotations). It is actually this precise functionality I am more interested in.
Without this CAInjector, and so to be more generic, it would require either the controller code to manage this injection (through the CA contained in the certificate passed in parameter) or to make the caBundle a helm parameter. This second option must be a simpler implementation.

[maxsmythe]

This assumes the use of Helm, which is not necessarily true. We should have a section that highlights both the helm chart parameters and the manifest.

E.g. for helm, set these fields, for the manifest, these flags must be set, and the certificates are read from these other mount points"

Context for how GK reads certs:

gatekeeper/main.go

Lines 98 to 99 in bf94eb3

	certDir = flag.String("cert-dir", "/certs", "The directory where certs are stored, defaults to /certs")
	disableCertRotation = flag.Bool("disable-cert-rotation", false, "disable automatic generation and rotation of webhook TLS certificates/keys")

[romachalm]

It makes sense

[maxsmythe]

I think it's fine to reference cert-manager as a thing that can generate certificates, but we should avoid coupling our docs with their UI.

To make this more agnostic, maybe use shell commands to generate the certificate?

[romachalm]

👍

[romachalm]

It looks like a large part of this PR is exposing individual annotations, which I think does make sense and gives people flexibility, rather than a hard coupling with cert-manager.

It looks like the other part of the PR toggles the creation of the secret and makes the name configurable?

If we remove the cert-manager-specific support docs, I think this makes sense.

Indeed. I have opened the PR following the issue #520 that is clearly cert-manager oriented because I had faced a similar need (ie signing certificates with a custom CA). But I wanted to keep the helm chart generic enough to cover any future needs. Only the doc is cert-manager oriented mainly due to ca-injector capability that cert-manager offers to dynamically injects a CABundle into an admission webhook.

[codecov-commenter]

Codecov Report

Merging #1359 (bf94eb3) into master (bf94eb3) will not change coverage.
The diff coverage is n/a.

❗ Current head bf94eb3 differs from pull request most recent head c48d90a. Consider uploading reports for the commit c48d90a to get more accurate results

@@           Coverage Diff           @@
##           master    #1359   +/-   ##
=======================================
  Coverage   49.69%   49.69%           
=======================================
  Files          68       68           
  Lines        4926     4926           
=======================================
  Hits         2448     2448           
  Misses       2134     2134           
  Partials      344      344           

Flag	Coverage Δ
unittests	49.69% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bf94eb3...c48d90a. Read the comment docs.

[maxsmythe]

Curious what others' thoughts our on what we should do WRT the documentation issue. @ritazh @shomron @sozercan, thoughts?

[ritazh]

I do see values in adding docs and sample scripts for users who want to bring their own certs with cert-manager or some openssl script. I have similar concerns about the manageability and supportability of including cert-manager as part of the chart. This PR seems to strike a good balance between these two as it exposes the knob for users to provide their certs without locking in any specific cert provider.

This PR however is lacking tests. How do we test this knob without having to maintain cert-manager? would an openssl script help validate this while not adding more maintenance burden on the maintainers?

[maxsmythe]

What tests would you like @ritazh ? The chart is just passing through annotations and setting the disable-cert-rotation command line flag? Is there some way to test that the manifest a chart generates looks correct without a full e2e test? That seems too heavy-weight given the install/uninstall cycle.

[ritazh]

I guess my point is without any tests, how do we know if this PR actually works and if something breaks this integration we have no signal of knowing that a commit broke it.

[romachalm]

Do I understand correctly that the goal of the test is to ensure that we can inject a certificate in the controller and a CA in the webhook without breaking the comm between the two ?

To do so, I have to make caBundle a parmater too. That way, I can inject a custom CA in the webhook, create a cert from it and mount it in the controller. If I can run it kind without failure, this will prove that the controller and the webhook can communicate, and therefore that the custom certificate works.

For the annotations, I do not see how to test them (only rendering). I will add in the doc as examples that the annotations can be used to inject caBundle such as cert-manager or openshift propose.

[maxsmythe]

@ritazh I don't think we need to test this all the way through to GK, it sounds like you want an assertion on the Helm chart. I don't know what tooling Helm does or does not offer for that.

All we are promising is:

"if you specify these annotations as helm parameters, they will show up here"
"if you specify --disable-cert-rotation, we will set that flag"

[ritazh]

I guess I have the same concern you have

Unfortunately we cannot support third-party integrations with our Helm chart for support scalability reasons

If a user tries this and it doesn't work, they open an issue, how do we triage and fix it?

Without any tests, we don't even know if this PR works?

[ritazh]

Do I understand correctly that the goal of the test is to ensure that we can inject a certificate in the controller and a CA in the webhook without breaking the comm between the two ?

Yes.

To do so, I have to make caBundle a parmater too. That way, I can inject a custom CA in the webhook, create a cert from it and mount it in the controller. If I can run it kind without failure, this will prove that the controller and the webhook can communicate, and therefore that the custom certificate works.

The added bonus here is it will give users another way to provide their own cert, in addition to the self generated one (by default) and the one from cert-manager.

[maxsmythe]

Found another comment (relocating the HELMSUBST_ for the mutation webhook), and needs DCO.

[maxsmythe]

HELMSUBST_ stuff should go under /helmify/

[sozercan]

Closing as this PR went stale. Please feel free to re-open and rebase it if it's still applicable.