cert-manager support

[katheris]

Type of change

Select the type of your PR

• Enhancement / new feature

Description

Add support for cert-manager issued certificates

Checklist

Please go through this checklist and make sure all applicable tasks have been done

• Write tests
• Make sure all tests pass
• Update documentation
• Check RBAC rights for Kubernetes / OpenShift roles
• Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
• Reference relevant issue(s) and close them after merging
• Update CHANGELOG.md
• Supply screenshots for visual changes, such as Grafana dashboards

[katheris]

@ppatierno @scholzj I'm not ready to open a final PR yet, but would be interested in some initial feedback on how I'm integrating cert-manager. At the moment we have a lot of if/else checks for Strimzi managed vs cert-manager managed. I could put in further abstractions but wanted to see if 1 you thought that was needed or could be done later, and 2 if you are happy with roughly how I've laid things out before adding the abstraction code.

The things I'm still working on:

• tests for the KafkaReconciler
• Entity operator and CC use of cert-manager
• tests for Clients CA (I haven't manually tested this part yet either)
• Some kind of system tests

[codecov]

Codecov Report

❌ Patch coverage is 64.03061% with 141 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.71%. Comparing base (062e41f) to head (765cbeb).
⚠️ Report is 69 commits behind head on main.

Files with missing lines	Patch %	Lines
...main/java/io/strimzi/operator/common/model/Ca.java	18.34%	80 Missing and 9 partials ⚠️
...rimzi/operator/cluster/model/CertManagerUtils.java	65.38%	15 Missing and 3 partials ⚠️
...rce/kubernetes/CertManagerCertificateOperator.java	14.28%	12 Missing ⚠️
...a/io/strimzi/operator/cluster/model/CertUtils.java	70.83%	7 Missing ⚠️
...va/io/strimzi/operator/common/model/ClientsCa.java	0.00%	7 Missing ⚠️
...erator/cluster/operator/assembly/CaReconciler.java	96.77%	2 Missing and 1 partial ⚠️
...a/io/strimzi/operator/cluster/model/ClusterCa.java	94.28%	2 Missing ⚠️
...o/strimzi/operator/cluster/model/KafkaCluster.java	94.28%	2 Missing ⚠️
...tor/cluster/operator/assembly/KafkaReconciler.java	95.23%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #12188      +/-   ##
============================================
- Coverage     74.77%   74.71%   -0.07%     
- Complexity     6618     6698      +80     
============================================
  Files           377      379       +2     
  Lines         25329    25659     +330     
  Branches       3394     3441      +47     
============================================
+ Hits          18940    19170     +230     
- Misses         5003     5099      +96     
- Partials       1386     1390       +4     

Files with missing lines	Coverage Δ
...er/operator/resource/ResourceOperatorSupplier.java	92.30% <100.00%> (+0.15%)	⬆️
...io/strimzi/operator/user/model/KafkaUserModel.java	84.13% <ø> (ø)
...tor/cluster/operator/assembly/KafkaReconciler.java	94.61% <95.23%> (+<0.01%)	⬆️
...a/io/strimzi/operator/cluster/model/ClusterCa.java	91.36% <94.28%> (+0.54%)	⬆️
...o/strimzi/operator/cluster/model/KafkaCluster.java	92.26% <94.28%> (-0.15%)	⬇️
...erator/cluster/operator/assembly/CaReconciler.java	90.81% <96.77%> (+1.32%)	⬆️
...a/io/strimzi/operator/cluster/model/CertUtils.java	77.41% <70.83%> (-0.68%)	⬇️
...va/io/strimzi/operator/common/model/ClientsCa.java	0.00% <0.00%> (ø)
...rce/kubernetes/CertManagerCertificateOperator.java	14.28% <14.28%> (ø)
...rimzi/operator/cluster/model/CertManagerUtils.java	65.38% <65.38%> (ø)
... and 1 more

... and 12 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[scholzj]

Is there a way to change this into the regular typed fields as we use for example for listeners etc.? Or do we need to keep this strange shape because type is not required and is not there by default?

[katheris]

I'm not sure, I will investigate

[scholzj]

I'm not sure I fully follow the logic of these separate RBAC files. Does it cause errors when cert-manager is not installed? If yes, maybe we should have two full sets of files. If not, maybe it should be merged into the main installation files?

[katheris]

My thought process was that the operator should only have access to do the things it needs. Since this is an optional feature if users don't want to use it they might prefer the operator isn't granted any permissions related to cert-manager. So it was more just allowing for tighter control over what is being granted

[scholzj]

I think we have other optional resources all included in the main files (e.g. Routes, OCP Build, Ingress). So I would probably stick with keep it all in one and leave it to the users to remove things should they not want them.

[scholzj]

This will likely need to be in common and not be Vert.x based as the UO will need it as well?

[scholzj]

@ppatierno @scholzj I'm not ready to open a final PR yet, but would be interested in some initial feedback on how I'm integrating cert-manager. At the moment we have a lot of if/else checks for Strimzi managed vs cert-manager managed. I could put in further abstractions but wanted to see if 1 you thought that was needed or could be done later, and 2 if you are happy with roughly how I've laid things out before adding the abstraction code.

The things I'm still working on:

• tests for the KafkaReconciler
• Entity operator and CC use of cert-manager
• tests for Clients CA (I haven't manually tested this part yet either)
• Some kind of system tests

I haven't done an in depth review of the code. But some thoughts:

• The User Operator integration seems unfinished. That makes it a bit hard to understand what does and does not need to be in the common module and what should be only in the CO module.
• I personally would probably prefer a cleaner separation of the two implementations:
  • I think it would help the code cleanliness and testing
  • I was not obsessed with pluggability when the proposal was written, but it would help towards it as well
  • Something along this line:

    classDiagram
        class Ca
        class CaProvider
        <<interface>> CaProvider
        Ca <|-- ClientsCa
        Ca <|-- ClusterCa
        CaProvider <|-- StrimziCaProvider
        CaProvider <|-- CertManagerCaProvider
        Ca ..* CaProvider : uses
        class Ca {
          +provider CaProvider
        } 
    

    Loading
    Where the specific implementation logic is encapsulated in some kind of CaProvider classes. These are used by the Ca classes which in turn are used by CO/UO. That might make sure that the logic for the individual Ca implementation are in a single place (in the CaProvider implementation). And the Ca classes are mostly independent on it and are just configured at construct time with the correct provider. Obviously, I did not wrote the code. But I obviously had just a quick look through the code. So maybe there are specific reasons to not do it?

[katheris]

I haven't done an in depth review of the code. But some thoughts:

Thanks for your review @scholzj. As you've noticed there was an oversight in my outline of what was left, which I discovered on Monday when working on the Clients CA tests, which is I haven't implemented the logic to have cert-manager issue the User certificates 🤦‍♀️ So that is something I'm working on now.

Your suggestions around the abstraction with having a CaProvider makes sense and were along the lines I was thinking, I have been wondering about even having that abstraction added as a separate PR upfront, similar to how I've already raised PRs to refactor the CaReconciler and Ca classes to make the final PR easier to review.

One area I wanted to check your thoughts on was the reconcile loop. In all the reconcilers I am currently following the approach of having something like:

...
.compose(i -> maybeReconcileCertManagerCertificates())
.compose(cmSecret -> certificateSecrets(clock, cmSecret))
...

Are you happy with this approach? The alternative which I had originally was to add the reconciling of the cert-manager Certificate resources within the certificateSecrets method, but I switched to the current approach to keep the interactions with Kubernetes at the reconciler level. For example in the KafkaReconciler it's actually the KafkaCluster class which called the clusterCa to generate the certificates, and it seemed counter-intuitive to have the KafkaCluster class interact with Kubernetes in the cert-manager case.

[scholzj]

Your suggestions around the abstraction with having a CaProvider makes sense and were along the lines I was thinking, I have been wondering about even having that abstraction added as a separate PR upfront, similar to how I've already raised PRs to refactor the CaReconciler and Ca classes to make the final PR easier to review.

I'm happy for more PRs if that is simple to do for you. I guess the question is how well can you actually define the interface while the cert manager work is still in progress. Sometimes it's simple, sometimes it's not. So I'm happy to leave it up to you and your judgment.

One area I wanted to check your thoughts on was the reconcile loop. In all the reconcilers I am currently following the approach of having something like:

...
.compose(i -> maybeReconcileCertManagerCertificates())
.compose(cmSecret -> certificateSecrets(clock, cmSecret))
...

Are you happy with this approach? The alternative which I had originally was to add the reconciling of the cert-manager Certificate resources within the certificateSecrets method, but I switched to the current approach to keep the interactions with Kubernetes at the reconciler level. For example in the KafkaReconciler it's actually the KafkaCluster class which called the clusterCa to generate the certificates, and it seemed counter-intuitive to have the KafkaCluster class interact with Kubernetes in the cert-manager case.

Honestly, this is not one of the things I noticed while going through it.

I think this is a bit strange way to do it as it does not encapsulate the logic and leaves it spread across the reconcilers. So when I look at the reconciled, I would definitely say that it should be all in one call, which does different things depending on how the certs are managed. Ideally, I would expect that for example the KafkaReconciler does not need to know what is used for certifciate management.

That said, we spent a lot of time to make the CA classes as independent on any Kube details as possible. So, how would that work? If I follow up on the diagram I shared above, maybe the CA classes should be independent on Kubernetes, but the provider should have the Kubernetes logic? E.g. be in a way CertManagerCaReconciler and StrimziCaRecocniler? But that would mean the Kubernetes tooling needs to pass through the Ca classes ... so should maybe the CaProvider own the Ca class and not the other way around?

Not an easy question ... 🤔

[ppatierno]

I finally came back to this, sorry to be so late.
I think that the Jakub's proposal (depicted in the diagram) makes a lot of sense.
Following my thoughts:

• I think we all agree that the if/then branching should be avoided as much as possible. By instantiating the desired CaProvider (Strimzi or cert-manager one) which contains the logic for "low level" certificate/key handling, we should be able to reach this goal.
• We should keep the reconcilers handling Kubernetes things while the Ca class and its implementations "understanding" and making decisions about the certificates handling (without Kubernetes knowledge) and relying on the configured provider to run operations.
• The fact that the specific CertManagerCaProvider uses Kubernetes doesn't break the previous point imho. The provider hides the implementation details of the certificates handling and it's just by chance that this specific provider uses ... Kubernetes. I mean, the StrimziCaProvider would use openssl for certs handling while the CertManagerCaProvider would use Kubernetes but it's a "low level" or a different usage of Kubernetes compared with what we have at upper level in the reconcilers. It uses Kubernetes API as a way to communicate with a tool (cert-manager) compared to the Strimzi provider which uses openssl as a tool via a command line. It doesn't mean that we need to pass some Kubernetes stuff to Ca to initiliaze the provider. It could be initialized at higher level (in the reconciler?) and the injected into the Ca (which is still not aware of how the provider works).