@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 Report

❌ Patch coverage is64.03061% with141 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.

@@             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

@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?

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 thecertificateSecrets 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.

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 theinterface 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 thecertificateSecrets 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 wayCertManagerCaReconciler andStrimziCaRecocniler? 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 ... 🤔