Generalize the certificate generation configuration

[giorio94]

Currently, certgen has detailed knowledge of the specific certificates used by Cilium, hence requiring any changes (e.g., introduction of a new certificate) to be manually applied to this tool as well. Moreover, it strongly relies on default values, making it difficult to predict the final output looking at the Cilium's configuration only.

Let's instead introduce a generic configuration, which can be used to express the list of certificates to be generated, and their characteristics. The configuration shall be provided in yaml format through the dedicated flags (--config or --config-file), and an example is presented in the following:

certs:
- name: foo
  namespace: kube-system
  commonName: foo.cilium.io
  hosts:
  - foo.cilium.io
  - qux.cilium.io
  - 192.0.2.237
  usage:
  - signing
  - key encipherment
  - server auth
  validity: 72h
- name: bar
  ...

WIP changes on the Cilium's side, as an example of how the proposed configuration could be used: giorio94/cilium@8583b14

Note: this obviously introduces backward incompatibility changes if merged, and we should decide whether to backport the Cilium's changes to stable versions, which is more risky, or keep using the old version there (hence needing to support two parallel versions for a while).

[kaworu]

First of all, the flag cleanup is impressive and functionally certgen is way more flexible after this patch, so thank you @giorio94 for tackling this 🙏

we should decide whether to backport the Cilium's changes to stable versions

In any case, we're still on v0.1.x so the first version with this change should be v0.2.x. IMO there is so little maintenance on cergen that we shouldn't bother backporting, but no strong opinion here. If we do not backport, then we should consider branching a v0.1 branch in case we need to do patch releases for previous cilium versions, hence not approving the PR until this question has reached a conclusion (but patch LGTM).

Finally, it's a tad scary to see such a huge change without any related test change, we definitely should consider adding at least a couple of functional tests (i.e. one for the initial cert /CA creation and one for renew), but of course not necessarily on this PR.

[giorio94]

Rebased onto main to fix a conflict.

In any case, we're still on v0.1.x so the first version with this change should be v0.2.x. IMO there is so little maintenance on cergen that we shouldn't bother backporting, but no strong opinion here. If we do not backport, then we should consider branching a v0.1 branch in case we need to do patch releases for previous cilium versions, hence not approving the PR until this question has reached a conclusion (but patch LGTM).

👍 I personally agree on starting using the new version on Cilium v1.16, and keep the previous one for all the other stable branches. And I also agree about branching a dedicated branch for dependency updates for the old version.

Finally, it's a tad scary to see such a huge change without any related test change, we definitely should consider adding at least a couple of functional tests (i.e. one for the initial cert /CA creation and one for renew), but of course not necessarily on this PR.

Agreed. To be fair certgen doesn't include any tests at the moment, hence adding tests requires definitely some extra effort (and likely push this to Cilium v1.17). In a certain sense, the new configuration is also arguably simpler compared to the previous one (~ 1/10 LoCs).

[marseel]

lgtm vendor changes.

I personally agree on starting using the new version on Cilium v1.16, and keep the previous one for all the other stable branches. And I also agree about branching a dedicated branch for dependency updates for the old version.

+1

[giorio94]

Converting back to draft while I'm putting together an E2E test workflow.

[giorio94]

Back reviewable, I've additionally dropped the now superfluous cilium-namespace option, and added an E2E test to validate certificate generation. The test runs the certgen binary once and verifies that both the CA and the leaf certificates specified through the configuration are generated correctly. Then, it reruns the tool a second time, and it asserts that
only the leaf certificates are regenerated, while the CA is not modified.

@kaworu PTAL 🙏

[brlbil]

I love the assert functions.

[rolinh]

Thanks for the addition of the e2e test! I left a couple suggestions below but nothing blocking. Great work, thanks, making certgen flexible is a really nice improvement! And it shaves many lines of code as an added benefit 🙂

[rolinh]

Huh, looks like one of my review comment mysteriously disappeared. I was suggesting to replace references to "Cilium CA" by simply "CA" in the remaining flags, logs and error messages now that the tool is actually generic.

[giorio94]

Huh, looks like one of my review comment mysteriously disappeared. I was suggesting to replace references to "Cilium CA" by simply "CA" in the remaining flags, logs and error messages now that the tool is actually generic.

I should have removed all the remaining references. PTAL and let me know if that's what you had in mind.

[rolinh]

Yes exactly what I had in mind 🚀

[giorio94]

Opened cilium/cilium#32998 to prevent renovate from upgrading the certgen version in stable Cilium branches.

[kaworu]

Awesome work @giorio94 thanks for adding the test! 🙏