Add support for cert-manager - #13017
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
2c82358 to
e645796
Compare
|
FYI: I want to review it, but I'm on an offsite meeting this week and will likely not get to it before Friday. |
|
@scholzj no worries, thanks for letting us know :) |
e645796 to
2dc0ac4
Compare
d817069 to
caa56d3
Compare
ppatierno
left a comment
ppatierno
left a comment
There was a problem hiding this comment.
I had a first pass mostly looking at code and understanding stuff. I haven't ran the code yet, I will do it asap.
| @Description("Reference to the Secret containing the CA certificate (public key) " + | ||
| "that trusts certificates issued by cert-manager. " + | ||
| "Required.") | ||
| public CaCertRef getCaCert() { |
There was a problem hiding this comment.
couldn't it be better to be getCaCertRef as we have for the issuer getIssuerRef (instead of just getIssuer)? If you agree, the set method should be changed accordingly.
There was a problem hiding this comment.
So for the issuer the name of the CRD property is issuerRef and the name of the class is IssuerRef. For the ca cert we agreed to have the field name as caCert in the proposal, but I named the class CaCertRef, so that is the reason for the difference. We could change the name of the CaCertRef class to CaCert, or change the CRD property to caCertRef if you wanted them to all align. As a note the reason we chose issuerRef is to align with the cert-manager property name
There was a problem hiding this comment.
Even the description says it's a "reference", so I would be consistent.
There was a problem hiding this comment.
Ok so we'll update the API to have:
certManager:
issuerRef:
name: <string>
kind: <Issuer|ClusterIssuer>
group: <string> # cert-manager.io by default
caCertRef:
secretName: <string>
certificate: <string>
Happy with that @scholzj @ppatierno?
There was a problem hiding this comment.
I am happy with that, thanks.
There was a problem hiding this comment.
I think we mostly followed naming of CertSecretSource.certificate and CertAndKeySecretSource.certificate. Have we seen the confusion specifically with certificate as well? I wonder if certificateKey could also be confusing, as we normally use key to refer to private keys. We could name it just secretKey but since we use certificate in other places, it might be better to keep consistent.
There was a problem hiding this comment.
You are right that even certificateKey is misleading here. secretKey is better. I do not strongly insist on changing it. But I think we should consider it. But I'm happy to leave the final call to you.
There was a problem hiding this comment.
We are in favour of leaving it as certificate. @ppatierno let us know if you disagree on this.
There was a problem hiding this comment.
sorry @tinaselenge but aren't we discussing something different on this thread? Or am I misleading the question?
There was a problem hiding this comment.
I had an offiline chat with @katheris and got what you meant. I think secretKey would be better here but we can stick with certificate for now. Can we plan to change it as in the other API for the future?
im-konge
left a comment
im-konge
left a comment
There was a problem hiding this comment.
Thanks for the PR, I didn't go through it fully, but I left one comment about the pipelines. Sorry I will have a look at it more once I'm back from PTO..
| } else if (secret.getData().get(caCertSecretKey) == null) { | ||
| throw new InvalidResourceException("CA public certificate Secret " + caCertSecretName + " missing key " + caCertSecretKey); | ||
| } | ||
| CertificateUtils.validateUserCaCertChain(reconciliation, caRole, Map.of(caCertSecretKey, secret.getData().get(caCertSecretKey))); |
There was a problem hiding this comment.
Maybe we can rename validateUserCaCertChain to be just validateCaCertChain because "User" let me think only when the user is providing the CA chain, while in this case we are using the same method for validating what cert-manager is providing.
There was a problem hiding this comment.
For the cert-manager case it's still the user who has provided the CA chain to Strimzi. We don't actually know whether it was created by the user directly or by cert-manager. But happy to generalise the method name if you prefer.
There was a problem hiding this comment.
yes I would generalize if it can be used to validate any cert chain and not just a "user provided" one.
4aed9d6 to
9d115fe
Compare
- Validate certManager property is not null when CA type is certManager - Add a unit test for the validation - Correct the target release Co-authored-by: Kate Stanley 11195226+katheris@users.noreply.github.com Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
* Remove unnecessary 'public' in docs * Add missing null checking * Throw IllegalArgumentException for unknown CertificateManagerType Signed-off-by: Kate Stanley <11195226+katheris@users.noreply.github.com>
Signed-off-by: Kate Stanley <11195226+katheris@users.noreply.github.com> Co-authored-by: Gantigmaa Selenge <tina.selenge@gmail.com>
- Drop .io suffix from CA type values - Rename caCert field to caCertRef in CertManager - Moved api.kafka.model.common.certmanager to api.kafka.model.kafka.certmanager - Enforce required fields in CaCertRef, CertManager, IssuerRef - Env vars for cert-manager renamed to avoid ambigous CM - Merge testNewClusterWithCertManagerClusterCa and testCertManagerClusterCaRenewal into single test - Update the doc to have the right prodecure steps, concept section and information on switching between different types on running cluster Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
…ert-manager rules to the existing installation files, and have EntityOperator.generateRole() conditionally include them when cert-manager is the CA type. Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
230c6ec to
47fd411
Compare
…files Co-authored-by: Kate Stanley <11195226+katheris@users.noreply.github.com> Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
- Add CEL validation rules for CertificateAuthority - Remove redundant checks that are covered by the CEL validation rules - Add a new system test for migration between CA types Co-authored-by: Kate Stanley <11195226+katheris@users.noreply.github.com> Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
47fd411 to
663731f
Compare
|
/gha run pipeline=regression |
|
⏳ System test verification started: link The following 6 job(s) will be executed:
Tests will start after successful build completion. |
|
❌ System test verification failed: link |
| <ignoredNonTestScopedDependencies> | ||
| <!-- Used transitively at runtime via CertManagerCertificateOperator in operator-common; | ||
| maven analysis cannot see the indirect usage through operator-common's method signatures --> | ||
| <ignoredNonTestScopedDependency>io.fabric8:certmanager-model</ignoredNonTestScopedDependency> | ||
| </ignoredNonTestScopedDependencies> |
There was a problem hiding this comment.
Sorry, but if it is used transitively through operator-common, why do we need to add the dependency here? Do you use it in tests?
There was a problem hiding this comment.
This is something I had to add to work around the maven analysis failure:
[ERROR] Used undeclared dependencies found:
[ERROR] io.fabric8:certmanager-model:jar:7.8.0:compile
[ERROR] Non-test scoped test only dependencies found:
[ERROR] io.fabric8:certmanager-model:jar:7.8.0:compile
user-operator does not import/use this directly anywhere. It just constructs a CertManagerCertificateOperator and passes it around. The dependency is needed at compile time only because Certificate appears in CertManagerCertificateOperator's type hierarchy (extends AbstractNamespacedResourceOperator<..., Certificate, CertificateList, ...>).
Apparently, this happens when the dependency is used transitively through method signatures but not imported directly. I'm not sure what else I could do to fix, does anyone have a suggestion?
There was a problem hiding this comment.
We've done another investigate on this, it seems to be due to the use of Mockito to mock the CertManagerCertificateOperator class, which as Tina says has the Certificate class as part of it's type hierarchy. So the options are to either:
a. Remove the dependency and add ignored used dependency for it
b. Update the scope to test for the dependency which means we wouldn't need the ignored part
Not sure which is preferable, what do you think? @scholzj
There was a problem hiding this comment.
Seeing that this was intentional and its not just some leftover, I'm flexible. Adding is as a test dependency seemes the cleanest way as long as it does not affect the transitive runtime dependency part. In any case, please make sure to be detailed in the comment about why is it needed.
Co-authored-by: Kate Stanley <11195226+katheris@users.noreply.github.com> Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
| .End-entity certificate rotation | ||
| End-entity certificates are renewed by cert-manager if: | ||
| * they are due to expire | ||
| * a change to Kafka CR triggers Cluster Operator to update Certificate resource (for example, `validityDays` is updated) |
There was a problem hiding this comment.
I think here we are missing the case when End-entity certs are generated when there is a new CA certificate signed with a new private key. It's related to the second use case of the previous paragraph but while the previous paragraph mentions the pods rolling in terms of trusting the new CA cert, this paragraph should mention that new EE certs are generated because signed by a shiny new private key now.
|
|
||
| [role="_abstract"] | ||
| Perform the steps in this procedure to enable cert-manager to issue certificates on either a | ||
| new or existing cluster. |
There was a problem hiding this comment.
can we move this few words on the previous line?
| For this reason the cert-manager `SelfSigned` issuer is not supported. | ||
|
|
||
| . Create or update the `Kafka` resource, setting `type: cert-manager` on `clusterCa`, `clientsCa`, or both. | ||
| Set `generateCertificateAuthority: false` and provide the `certManager.issuerRef` and `certManager.caCert` configuration. |
There was a problem hiding this comment.
Didn't we agree on certManager.caCertRef ? The same applies to the following YAML.
| group: cert-manager.io | ||
| caCert: | ||
| secretName: _<ca_cert_secret_name>_ | ||
| certificate: ca.crt |
There was a problem hiding this comment.
what about changing the example to have two different CAs for clients and cluster? so just changing the _<ca_cert_secret_name>_ in _<cluster_ca_cert_secret_name>_ and '<clients_ca_cert_secret_name> `.
Of course they can be the same but maybe it's not the usual case?
| -n _<namespace>_ | ||
| ---- | ||
| + | ||
| Replace _<ca_cert_secret_name>_ with a name of your choice, _<path_to_ca_cert_pem>_ with the path to the CA certificate PEM file, and _<namespace>_ with the namespace of the Kafka cluster. |
There was a problem hiding this comment.
I was going through some manual tests but I think the above needs more details.
The user needs:
- cert-manager installed
- a
IssuerorClusterIssuerconfigured - a Secret containing the CA cert and the corresponding key within
tls.crtandtls.keyfields. This is where the issuer expect to find the CA cert/key. - copy the
tls.crtwithin the <ca_cert_secret_name> into theca.crtfield. This is where Strimzi is looking for the CA cert.
So I would not explain how to set up cert-manager but at least going into more details with issuer and the corresponding Secret.
Added a wait for CO cert to be re-issued after switching the CA type to fix the failure. Separate the migration test into 2 separate tests, to simplify the setup it needs. Reduce the number of nodes, to help saving on time, as nodes have to be rolled twice at each transition. Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
|
@tinaselenge @katheris I ran some manual tests with scaling up/down the Kafka cluster. It seems that when the brokers/controllers are scaled down, the corresponding -cm suffixed Secrets and Certificate custom resources are not deleted (as they are created on scale up). It looks like a bug to me. We are leaving orphan resources around, or? |
|
@tinaselenge @katheris I tried renewing the CA cert by using a new private key, it seems the procedure goes through by rolling the pods twice (for trusting new CA and then for using new EE certs) but at the beginning I see this exception in the operator log: 2026-09-08 14:38:04 ERROR CertificateUtils:182 - Reconciliation #35(timer) Kafka(myproject/my-cluster): Certificate chain cannot be validated with supplied CA cert.
java.security.cert.CertPathValidatorException: Path does not chain with any of the trust anchors
at java.base/sun.security.provider.certpath.PKIXCertPathValidator.validate(PKIXCertPathValidator.java:157)
at java.base/sun.security.provider.certpath.PKIXCertPathValidator.engineValidate(PKIXCertPathValidator.java:83)
at java.base/java.security.cert.CertPathValidator.validate(CertPathValidator.java:309)
at io.strimzi.operator.common.ca.CertificateUtils.certIsTrusted(CertificateUtils.java:178)
at io.strimzi.operator.common.ca.CertManagerCa.shouldUpdateCa(CertManagerCa.java:155)
at io.strimzi.operator.common.ca.CertManagerCa.maybeUpdateCa(CertManagerCa.java:103)
at io.strimzi.operator.cluster.operator.assembly.CertManagerCaProvider.lambda$createAndReconcileCa$1(CertManagerCaProvider.java:91)
at java.base/java.util.concurrent.CompletableFuture$UniCompose.tryFire(CompletableFuture.java:1150)
at java.base/java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:510)
at java.base/java.util.concurrent.CompletableFuture$AsyncSupply.run(CompletableFuture.java:1773)
at io.strimzi.operator.cluster.operator.VertxUtil.lambda$asExecutor$0(VertxUtil.java:46)
at io.vertx.core.impl.ExecuteBlocking$1.execute(ExecuteBlocking.java:36)
at io.vertx.core.impl.WorkerTask.run(WorkerTask.java:57)
at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
at java.base/java.lang.Thread.run(Thread.java:1583)
2026-09-08 14:38:04 WARN CaReconciler:272 - Reconciliation #35(timer) Kafka(myproject/my-cluster): Cluster CA needs to be fully trusted across the cluster, keeping current CO secret and certs
2026-09-08 14:38:04 INFO StrimziPodSetController:350 - Reconciliation #36(watch) StrimziPodSet(myproject/my-cluster-controller): StrimziPodSet will be reconciled
2026-09-08 14:38:04 INFO StrimziPodSetController:388 - Reconciliation #36(watch) StrimziPodSet(myproject/my-cluster-controller): reconciled
2026-09-08 14:38:04 INFO StrimziPodSetController:350 - Reconciliation #37(watch) StrimziPodSet(myproject/my-cluster-broker): StrimziPodSet will be reconciled
2026-09-08 14:38:04 INFO StrimziPodSetController:388 - Reconciliation #37(watch) StrimziPodSet(myproject/my-cluster-broker): reconciled
2026-09-08 14:38:04 INFO StrimziPodSetController:350 - Reconciliation #38(watch) StrimziPodSet(myproject/my-cluster-controller): StrimziPodSet will be reconciled
2026-09-08 14:38:04 INFO StrimziPodSetController:388 - Reconciliation #38(watch) StrimziPodSet(myproject/my-cluster-controller): reconciled
2026-09-08 14:38:04 INFO StrimziPodSetController:350 - Reconciliation #39(watch) StrimziPodSet(myproject/my-cluster-broker): StrimziPodSet will be reconciled
2026-09-08 14:38:04 INFO StrimziPodSetController:388 - Reconciliation #39(watch) StrimziPodSet(myproject/my-cluster-broker): reconciled
2026-09-08 14:38:04 INFO KafkaRoller:447 - Reconciliation #35(timer) Kafka(myproject/my-cluster): Rolling Pod my-cluster-controller-5/5 due to [Trust new cluster CA certificate signed by new key]
2026-09-08 14:38:04 INFO PodOperator:73 - Reconciliation #35(timer) Kafka(myproject/my-cluster): Rolling pod my-cluster-controller-5I also ended with the EO going into crash loop and operator not able to connect to nodes anymore with ... 2026-09-08 14:45:28 INFO Ca:633 - Reconciliation #268(watch) Kafka(myproject/my-cluster): cluster-ca: Old CA certificates removed
2026-09-08 14:45:29 ERROR NetworkClient:889 - [AdminClient clientId=adminclient-65] Connection to node -1 (my-cluster-controller-3.my-cluster-kafka-brokers.myproject.svc/10.244.0.37:9090) failed authentication due to: SSL handshake failed
2026-09-08 14:45:29 WARN AdminMetadataManager:280 - [AdminClient clientId=adminclient-65] Fatal error during metadata update
org.apache.kafka.common.errors.SslAuthenticationException: SSL handshake failed
Caused by: javax.net.ssl.SSLHandshakeException: (certificate_unknown) PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target
at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:130)
at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:383)
at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:326)
at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:321)
at java.base/sun.security.ssl.CertificateMessage$T13CertificateConsumer.checkServerCerts(CertificateMessage.java:1287)
at java.base/sun.security.ssl.CertificateMessage$T13CertificateConsumer.onConsumeCertificate(CertificateMessage.java:1164)
at java.base/sun.security.ssl.CertificateMessage$T13CertificateConsumer.consume(CertificateMessage.java:1107)
at java.base/sun.security.ssl.SSLHandshake.consume(SSLHandshake.java:393)
at java.base/sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:477)
at java.base/sun.security.ssl.SSLEngineImpl$DelegatedTask$DelegatedAction.run(SSLEngineImpl.java:1274)
at java.base/sun.security.ssl.SSLEngineImpl$DelegatedTask$DelegatedAction.run(SSLEngineImpl.java:1261)
at java.base/java.security.AccessController.doPrivileged(AccessController.java:714)
at java.base/sun.security.ssl.SSLEngineImpl$DelegatedTask.run(SSLEngineImpl.java:1206)
at org.apache.kafka.common.network.SslTransportLayer.runDelegatedTasks(SslTransportLayer.java:444)
at org.apache.kafka.common.network.SslTransportLayer.handshakeUnwrap(SslTransportLayer.java:533)
at org.apache.kafka.common.network.SslTransportLayer.doHandshake(SslTransportLayer.java:382)
at org.apache.kafka.common.network.SslTransportLayer.handshake(SslTransportLayer.java:302)
at org.apache.kafka.common.network.KafkaChannel.prepare(KafkaChannel.java:178)
at org.apache.kafka.common.network.Selector.pollSelectionKeys(Selector.java:548)
at org.apache.kafka.common.network.Selector.poll(Selector.java:486)
at org.apache.kafka.clients.NetworkClient.poll(NetworkClient.java:645)
at org.apache.kafka.clients.admin.KafkaAdminClient$AdminClientRunnable.processRequests(KafkaAdminClient.java:1541)
at org.apache.kafka.clients.admin.KafkaAdminClient$AdminClientRunnable.run(KafkaAdminClient.java:1472)
at java.base/java.lang.Thread.run(Thread.java:1583)
Caused by: sun.security.validator.ValidatorException: PKIX path building failed: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target
at java.base/sun.security.validator.PKIXValidator.doBuild(PKIXValidator.java:388)
at java.base/sun.security.validator.PKIXValidator.engineValidate(PKIXValidator.java:271)
at java.base/sun.security.validator.Validator.validate(Validator.java:256)
at java.base/sun.security.ssl.X509TrustManagerImpl.checkTrusted(X509TrustManagerImpl.java:284)
at java.base/sun.security.ssl.X509TrustManagerImpl.checkServerTrusted(X509TrustManagerImpl.java:144)
at java.base/sun.security.ssl.CertificateMessage$T13CertificateConsumer.checkServerCerts(CertificateMessage.java:1265)
... 19 more
Caused by: sun.security.provider.certpath.SunCertPathBuilderException: unable to find valid certification path to requested target
at java.base/sun.security.provider.certpath.SunCertPathBuilder.build(SunCertPathBuilder.java:148)
at java.base/sun.security.provider.certpath.SunCertPathBuilder.engineBuild(SunCertPathBuilder.java:129)
at java.base/java.security.cert.CertPathBuilder.build(CertPathBuilder.java:297)
at java.base/sun.security.validator.PKIXValidator.doBuild(PKIXValidator.java:383)
... 24 moreI will try the scenario again. [UPDATE] I tried a couple of times, the EO didn't crash loop but the operator consistently gives me the above exception in the log.
I can see pods rolling twice (for trusting new CA and for new EE certs) but at the end the operator is not able to connect to any nodes anymore. Do you have STs which checks that after such renewal, the operator is still able to connect and work fine? Events:
Type Reason Age From Message
---- ------ ---- ---- -------
Normal Issuing 19m cert-manager-certificates-trigger Issuing certificate as Secret does not exist
Normal Generated 19m cert-manager-certificates-key-manager Stored new private key in temporary Secret resource "my-cluster-broker-0-qnvhc"
Normal Requested 19m cert-manager-certificates-request-manager Created new CertificateRequest resource "my-cluster-broker-0-1"
Normal Issuing 19m cert-manager-certificates-issuing The certificate has been successfully issuedHow the operator is supposed to ask cert-manager to re-issue EE certs? By deleting -cm Secrets or annotating the Certificate somehow? [UPDATE] It seems the agreement was that it has to be the user to trigger cert-manager to generate new EE certs with the private key and only after that they copy the new CA cert to Strimzi Secret to trigger the rolling. |
|
@katheris @tinaselenge just leaving one more comment ... when deleting the Kafka custom resource and cleaning the cluster, the [UPDATE] Reading the cert-manager documentation, it deletes the corresponding Secrets only if the cert-manager installation had the |
| * @return CompletionStage with CertAndKey | ||
| */ | ||
| private CompletionStage<CertAndKey> generateSignedCert(String entityName, StrimziSubject subject) { | ||
| Certificate certificate = buildCertificateResource(entityName, subject, caConfig.getValidityDays(), caConfig.getRenewalDays()); |
There was a problem hiding this comment.
Related to the behaviour I described here #13017 (comment) ... when a new private key is used, the operator should ask somehow cert-manager to issue new EE certs but all the parameters we are passing here are the same, so the Certificate resource doesn't change, it means cert-manager doesn't know that it has to issue new EE certs. How is it supposed to work?
I noticed that by deleting all -cm Secrets, forces cert-manager to re-issue EE certs but then we need anyway operator to copy the EE certs from -cm Secrets to the Strimzi ones. Other than deleting the existing -cm Secrets I can't find a different way within the cert-manager documentation.
| .withNamespace(reconciliation.namespace()) | ||
| .endMetadata() | ||
| .withNewSpec() | ||
| .withCommonName(subject.commonName()) |
There was a problem hiding this comment.
from here to endSpec() everything should have a deeper indentation.
| CertificateBuilder certificateBuilder = new CertificateBuilder() | ||
| .withNewMetadata() | ||
| .withName(entityName) | ||
| .withNamespace(reconciliation.namespace()) |
There was a problem hiding this comment.
I was wondering if we should also add some usual labels we have on other resources to give the possibility to the user to filter specific Certificate related to specific Kafka clusters.
For example, I am thinking about something like:
labels:
app.kubernetes.io/instance: my-cluster
app.kubernetes.io/managed-by: strimzi-cluster-operator
app.kubernetes.io/name: my-cluster-broker-0
app.kubernetes.io/part-of: strimzi-my-cluster
strimzi.io/cluster: my-cluster
strimzi.io/component-type: strimzi-cluster-operator
strimzi.io/kind: Kafka
strimzi.io/name: my-cluster-broker-0Given discussions we had in the past about usage of app.kubernetes.io labels I could be ok if someone disagree on putting them here.
There was a problem hiding this comment.
We should definitely have the strimzi.io labels. Probably also the Kubernetes ones if we have them everywhere else, although they suck.
There was a problem hiding this comment.
For the labels, we did originally have all the labels listed above. However we ran into some issues with the KafkaReconciler and User operator because they incorrectly interpreted the presence of the strimzi.io/kind label to mean that a particular Secret was the Secret containing the certificates that is directly used by the pods/the user secret.
We did actually add some handling for this in KafkaReconciler, but when we hit the problem in User Operator we then decided to remove the labels.
So we have two options:
- Use the standard labels but omit the
strimzi.io/kindlabel. - Add custom handling to the user operator to handle the fact that the cert-manager secrets get returned by the secret informer
Option 1 means we don't have to have custom handling on KafkaReconciler and User Operator for resources that only the cert-manager related code should know about, however everywhere today we assume that everything has those labels.
@ppatierno @scholzj what are your thoughts?
There was a problem hiding this comment.
I doubt the strimzi.io/kind label is the problem as there are many secrets with this label. I understand the issue, but I expect that there are other key labels for it such as strimzi.io/component-type that should be skipped if needed. Definitely not strimzi.io/kind which should be the key to link it to the CR. If strimzi.io/kind is really the problem, then the selector used in Kafka reconciliation is wrong.
There was a problem hiding this comment.
Also, I think the labels make more sense on the CErtificate CR rather than the Secret. But that might be out of your control.
There was a problem hiding this comment.
Also II ... I wonder if the UO integration is not properly thought through (not your fault -> our collective fault). What sense does it make to create a Certificate resource, that creates a Secret, then copy the secrets content into another Secret (which is where the label issue comes up I guess?) ... and then have the user copy that Secret once again into the right namespace 🤔. But it might be too late for that discussion.
There was a problem hiding this comment.
I doubt the strimzi.io/kind label is the problem
The problem is not necessarily the kind label specifically, it's more if we put the exact same labels on the cert-manager Secret as the user Secret then the User Operator doesn't currently have logic to realise the difference. As we said we added logic in the KafkaReconciler to handle it since it already explicitly handles e.g. ignoring the jmx Secret, but it means knowledge of the cert-manager certificate issuing bleeds into KafkaReconciler.
I think the labels make more sense on the Certificate CR rather than the Secret
We can choose whether the labels go on just the Certificate, or the just the Secret, or both, and we can choose which labels should be there, so we could put some custom labels rather than the generic ones used for the other resources.
I wonder if the UO integration is not properly thought through
For the UO in the original proposal I proposed that the User Secret would be the one maintained by cert-manager. The implementation is actually different from that because when we were working on the change we realised that the order of steps in the reconciliation is actually not set up at all to do that. In the user operator it generates all the credentials, then updates the ACLs in Kafka, then finally updates the user Secret as the last step. So the current implementation keeps that flow and allows us to reuse a lot of the code that is used in the cluster-operator, rather than having custom handling.
I don't think it's too late for any discussion about the right way to do any of these changes. The thing I am keen to do though is try to be iterative where we can, rather than continually delaying this feature. So we could look to change this in future to remove the intermediate Secret, since it would have no user facing impact, or consider omitting the UO part of this PR and preventing the clientsCa from having the type cert-manager until we can agree on the approach.
There was a problem hiding this comment.
I doubt the strimzi.io/kind label is the problem
The problem is not necessarily the kind label specifically, it's more if we put the exact same labels on the cert-manager Secret as the user Secret then the User Operator doesn't currently have logic to realise the difference. As we said we added logic in the KafkaReconciler to handle it since it already explicitly handles e.g. ignoring the jmx Secret, but it means knowledge of the cert-manager certificate issuing bleeds into KafkaReconciler.
I think the labels make more sense on the Certificate CR rather than the Secret
We can choose whether the labels go on just the Certificate, or the just the Secret, or both, and we can choose which labels should be there, so we could put some custom labels rather than the generic ones used for the other resources.
Strictly speaking, the Secrets are not ours but Cert Managers. So I think for us it is important to have the expected Strimzi labels on the Cert Manager CRs we create. Not so much on the Secrets Cert Manager creates as those should be selected differently. So maybe that is something to consider as well?
I wonder if the UO integration is not properly thought through
For the UO in the original proposal I proposed that the User Secret would be the one maintained by cert-manager. The implementation is actually different from that because when we were working on the change we realised that the order of steps in the reconciliation is actually not set up at all to do that. In the user operator it generates all the credentials, then updates the ACLs in Kafka, then finally updates the user Secret as the last step. So the current implementation keeps that flow and allows us to reuse a lot of the code that is used in the cluster-operator, rather than having custom handling.
I'm not sure I understand why the order matters for this. I guess the Secret is created last to block the applications from deploying when something fails. E.g. if we create the Secret early but ACL creation fails, Deployments mounting the Secret would get up and running. When the Secret is missing, the Pods using it would be stuck waiting. That said, this works only for new users 🤷. So not sure how much the order matters.
But I'm not sure I understand why the order matters if you do not create the Secret and just check its presence. But obviously you might have the reasons.
That said, I think having only one Secret is much better from a security perspective (only one thing to protect). Any security review that runs into this will raise it. So it might be worth thinking about it if it is somehow reasonably possible.
I don't think it's too late for any discussion about the right way to do any of these changes. The thing I am keen to do though is try to be iterative where we can, rather than continually delaying this feature. So we could look to change this in future to remove the intermediate Secret, since it would have no user facing impact, or consider omitting the UO part of this PR and preventing the clientsCa from having the type cert-manager until we can agree on the approach.
I do not want to prolong the PR either. I want it merged so that we can do a release with the cert manager and cluster security 😄.
The main thing I wonder about is why did we bother with the User Operator at all. Why don't we use type: tls-external authentication in KafkaUser resources and have the users create their certificates on their own. That way - especially when using a ClusterIssuer - users wont need to do any copying to other namespaces etc. Obviously, this is also solution for the two secrets if someone has problem with it.
That said:
- Users can still do this on their own instead of using
type: tls, so having also direct support in UO does not prevent it in any way. - I think I raised it during the proposal and for some reason we did not choose this. I'm just not sure why anymore, as it would have obviously saved a lot of work 🤷.
So I think this part does not impact the PR.
|
@katheris @tinaselenge @scholzj sorry for asking but where we ended with the discussion around unwrapping - lastTransitionTime: "2026-09-09T15:41:17.043859847Z"
message: 'io.strimzi.operator.common.model.InvalidResourceException: CA public
certificate Secret paolo-ca-cert-2 missing.'
reason: CompletionException
status: "True"
type: NotReadyWhile I would expect more something like: - lastTransitionTime: "2026-09-09T15:41:17.043859847Z"
message: CA public certificate Secret paolo-ca-cert-2 missing.'
reason: InvalidResourceException
status: "True"
type: NotReady |
As far as I know, the plan was to not unpack them unless you need to check the exception type for some special handling. |
|
@scholzj As an update from the comments @ppatierno shared above about the steps for key replacement, we have found there is a bug in the code today. If the user follows the expected flow, which is to have cert-manager issue new certificates signed by the new CA key, before updating the CA certificate given to Strimzi, it works as expected. We are intending to make this flow clearer in the docs when we address Paolo's other comments there. However, if the user updates the CA certificate they have given us with a new private key before new end-entity certificates are ready, then Strimzi rolls the pods once as expected, but then incorrectly determines that the CA certificate is used everywhere and removes the old CA certificate, breaking the cluster. The fix for this is we need to add a check in CaReconciler so that we verify the cert generation annotation on the certificate Secrets, not just the Kafka pods. This was noted in the original proposal but we missed it in our implementation. We will start working on this change, but Paolo has said if the other comments are resolved before we address the bug he would be happy for it to be a follow-up PR. Would that be ok with you or would you want us to hold this PR for that change? We would of course fix it before this feature is in a release. On a similar note, Paolo spotted that we are not cleaning up the resources (Certificate, Secret etc). We intend to address this, but again wanted to check if you feel this needs to be part of this PR or if it can go in a follow-up. |
|
To be honest, I'm not sure I completely follow the issues. But they sound like bugs and I cannot say that I like the idea of merging things with a known bugs. It just makes the main branch un-releasable, so one way or another we will need to wait. And TBH it does not look like it is the only thing preventing this PR from being merged anyway. The unit tests failed, there are unresolved conflicts, and the STs run last time last week and failed. |
5 participants
Type of change
Description
This PR implements SIP-100 for adding support for cert-manager
Allows users to delegate TLS certificate issuance to cert-manager instead of Strimzi's built-in CA, by setting
spec.clusterCa.typeand/orspec.clientsCa.typetocert-manager.io.Resolves #929
Summary of the changes:
API
Cluster Operator
User Operator
Feature gate
RBAC
System tests
Doc
Co-authored-by: Kate Stanley 11195226+katheris@users.noreply.github.com
Checklist
Please go through this checklist and make sure all applicable tasks have been done