Fetch Kafka Connect, MM2, Bridge trusted certificates Secret only once for each reference#12241
Fetch Kafka Connect, MM2, Bridge trusted certificates Secret only once for each reference#12241venkatesh2090 wants to merge 8 commits intostrimzi:mainfrom
Conversation
Refactored tlsTrustedCertsSecret to return a list of secrets instead of the concatenated version. Signed-off-by: Venkatesh Kannan <[email protected]>
Also fixed auth hash in MM2. Signed-off-by: Venkatesh Kannan <[email protected]>
Signed-off-by: Venkatesh Kannan <[email protected]>
Signed-off-by: Venkatesh Kannan <[email protected]>
Signed-off-by: Venkatesh Kannan <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12241 +/- ##
============================================
+ Coverage 74.76% 74.89% +0.12%
- Complexity 6626 6643 +17
============================================
Files 377 376 -1
Lines 25362 25361 -1
Branches 3402 3401 -1
============================================
+ Hits 18962 18994 +32
+ Misses 5011 4983 -28
+ Partials 1389 1384 -5
🚀 New features to boost your workflow:
|
Signed-off-by: Venkatesh Kannan <[email protected]>
Signed-off-by: Venkatesh Kannan <[email protected]>
|
/gha run pipeline=upgrade,regression |
|
⏳ System test verification started: link The following 10 job(s) will be executed:
Tests will start after successful build completion. |
tinaselenge
left a comment
tinaselenge
left a comment
There was a problem hiding this comment.
Thanks for the PR. I left some comments. I think we need to leave KafkaBridge related changes out of this PR as mentioned below.
| * @return Future which completes when the reconciliation is done | ||
| */ | ||
| protected Future<Void> tlsTrustedCertsSecret(Reconciliation reconciliation, String namespace, KafkaConnectCluster connect) { | ||
| protected Future<List<String>> tlsTrustedCertsSecret(Reconciliation reconciliation, String namespace, KafkaConnectCluster connect) { |
There was a problem hiding this comment.
java doc for @return needs updating
| return Future.succeededFuture(); | ||
| } | ||
| }); | ||
| .compose(certificates -> tlsTrustedCertsSecret(reconciliation, namespace, connect, certificates)); |
There was a problem hiding this comment.
Do we really need this to be broken down to another method? The only changes here seem to be encoding a list of certificates instead of String and then mapping the result with the list instead of empty.
There was a problem hiding this comment.
I had to split it so I can use it here https://github.com/strimzi/strimzi-kafka-operator/pull/12241/files#diff-c6d9b371d64b172f4e174d61f4a6b81ed129010c41a016f0bbf29dddc62df009R138. More details in the other comment
| }) | ||
| .compose(i -> isPodDisruptionBudgetGeneration ? podDisruptionBudgetOperator.reconcile(reconciliation, namespace, bridge.getComponentName(), bridge.generatePodDisruptionBudget()) : Future.succeededFuture()) | ||
| .compose(i -> ReconcilerUtils.authTlsHash(secretOperations, namespace, auth, trustedCertificates)) | ||
| .compose(i -> ReconcilerUtils.trustedCertificates(reconciliation, secretOperations, trustedCertificates)) |
There was a problem hiding this comment.
KafkaBridge resource currently doesn't use this internal secret so I don't think we should create one yet . The primary reason for this secret is to configure truststore directly from a secret instead of using generated certificates based on volume mounted secrets. It should be done in a separate PR like in #11549 which is closed for now.
There was a problem hiding this comment.
I'll undo KafkaBridge changes as mentioned in the parent comment.
There was a problem hiding this comment.
Actually reverting KafkaBridge changes is not straightforward because I changed authTlsHash. I have changed the function parameters to require a list of certificates which I fetch here.
| KafkaConnectBuild build; | ||
| KafkaConnectStatus kafkaConnectStatus = new KafkaConnectStatus(); | ||
| List<String> tlsCertificates; | ||
| List<String> oauthCertificates; |
There was a problem hiding this comment.
Committed by mistake, will remove
| .compose(i -> serviceOperations.reconcile(reconciliation, namespace, mirrorMaker2Cluster.getServiceName(), mirrorMaker2Cluster.generateService())) | ||
| .compose(i -> serviceOperations.reconcile(reconciliation, namespace, mirrorMaker2Cluster.getComponentName(), mirrorMaker2Cluster.generateHeadlessService())) | ||
| .compose(i -> tlsTrustedCertsSecret(reconciliation, namespace, mirrorMaker2Cluster)) | ||
| .compose(i -> updateMM2ClusterCertificateMap(reconciliation, mirrorMaker2Cluster, clusterCerts)) |
There was a problem hiding this comment.
I'm not sure if I understand these changes? Can you please provide some explanation?
There was a problem hiding this comment.
The updateMM2ClusterCertificateMap updates the clusterCerts map that's in the scope of a single reconcilation. The key is the alias of the cluster as specified in the CR and the value is a list of certs required for that cluster.
I fetch all the clusters beforehand and use them in the next stage.
I use the target cluster's alias kafkaMirrorMaker2.getSpec().getTarget().getAlias() to get the target cluster's CA cert and use that to generate the connect's internal tls secret. This just replicates the behaviour that already existed.
The original tlsTrustedCertsSecret gets certificate using connect.getTls().getTrustedCertificates() and fetches it. I didn't want it to fetch again in this context because it is already fetched in the updateMM2ClusterCertificateMap method. So I split tlsTrustedCertsSecret to be able to pass in the list of certificates from the map. The connect.getTls() method resolves to spec.target.tls from KafkaMirrorMaker2Cluster.fromCrd and eventually KafkaMirrorMaker2Cluster.buildKafkaConnectSpec usingspec.getTarget().getTls(). That's why I'm using the target's alias for tlsTrustedCertsSecret. The alias remains unique at this stage because KafkaMirrorMaker2Cluster.validateAndUpdateToNewAPI has a check for it
Later on, the map is used in generateAuthHash to generate the hash as well.
There was a problem hiding this comment.
Thank you for explaining, I understand it now. So if we are getting the target cluster's TLS config to reconcile the internal truststore secret, why do we need this map with source alias and its certs? Source cluster certs seem to be used for generating the auth hash but we previously just used target's clusters' certs to generate the hash. Source cluster's certs would be only used when configuring Connectors, rather the Connect cluster (which is the target cluster) but this part is not implemented yet. I guess for that we would need to both clusters certs but not sure, if we need to do it for this PR's purpose.
There was a problem hiding this comment.
I think I implemented the auth hash method incorrectly. I meant to use the map like
clusterCert.get(cluster.getAlias())and then pass it to ReconcilerUtils.authTlsHash. I meant to replicate this behaviour https://github.com/strimzi/strimzi-kafka-operator/pull/12241/files/63d40adaf074ca0d50be8e7b4bc294a58f8b3d73#diff-c6d9b371d64b172f4e174d61f4a6b81ed129010c41a016f0bbf29dddc62df009L204-L207
.map(cluster -> {
List<CertSecretSource> trustedCertificates = cluster.getTls() == null ? List.of() : cluster.getTls().getTrustedCertificates();
return ReconcilerUtils.authTlsHash(secretOperations, namespace, cluster.getAuthentication(), trustedCertificates);
}).collect(Collectors.toList())
)by doing this instead
- private Future<Integer> generateAuthHash(String namespace, KafkaMirrorMaker2Cluster mirrorMaker2Cluster) {
+ private Future<Integer> generateAuthHash(String namespace, KafkaMirrorMaker2Cluster mirrorMaker2Cluster, Map<String, List<String>> clusterCert) {
Promise<Integer> authHash = Promise.promise();
Future.join(mirrorMaker2Cluster
.clusters()
.stream()
.map(cluster -> {
- List<CertSecretSource> trustedCertificates = cluster.getTls() == null ? List.of() : cluster.getTls().getTrustedCertificates();
- return ReconcilerUtils.authTlsHash(secretOperations, namespace, cluster.getAuthentication(), trustedCertificates);
- }).collect(Collectors.toList())
+ KafkaClientAuthentication auth = cluster.getAuthentication();
+ List<String> certificates = clusterCert.getOrDefault(cluster.getAlias(), , Collections.emptyList());
+ return ReconcilerUtils.authTlsHash(secretOperations, namespace, auth, certificates);
+ })
+ .collect(Collectors.toList())
)
but we previously just used target's clusters' certs to generate the hash
I'm not sure about this part. Didn't it use all the clusters - source and target, to generate the auth hash via the KafkaMirrorMaker2Cluster.clusters() method
There was a problem hiding this comment.
I'm not sure about this part. Didn't it use all the clusters - source and target, to generate the auth hash via the KafkaMirrorMaker2Cluster.clusters() method
You are right, I misunderstood it. We do need both clusters to generate the auth hash.
| * @return Certificates extracted from the Secrets | ||
| */ | ||
| public static Future<String> trustedCertificates(Reconciliation reconciliation, SecretOperator secretOperations, List<CertSecretSource> certificateSources) { | ||
| public static Future<List<String>> trustedCertificates(Reconciliation reconciliation, SecretOperator secretOperations, List<CertSecretSource> certificateSources) { |
There was a problem hiding this comment.
Does this really need to be changed to a List? Can the hash not be computed from the final String instead of a List?
There was a problem hiding this comment.
the auth hash method sums the hashes of each cert, but the trustedCertificates method returns the string list concatenated with "\n" as the delimiter. I believe it isn't possible to split it after it is concatenated. It would also not produce the same hash as before I assume if I just use hashCode of the concatenated string.
But if it is ok to use the hashCode of the concatenated string, it makes the implementation much simpler. I don't mind changing it if that is better in your opinion.
There was a problem hiding this comment.
If it doesn't bring any clear advantage I would leave the previous implementation. Unless as I said you need the List of certificates somewhere else.
There was a problem hiding this comment.
Cool. I will refactor to make this simpler. The only place this list of certs is used is for the auth hash calculation.
|
🎉 System test verification passed: link |
Signed-off-by: Venkatesh Kannan <[email protected]>
venkatesh2090
force-pushed
the
fix-trusted-certs
branch
from
f1b8201 to
c615a6a
Compare
4 participants
Type of change
Select the type of your PR
Enhancement / new feature
Description
Fixes #11972
Updated when Secrets for trusted certificates are fetched and used in KafkaConnect, KafkaMirrorMaker2, and KafkaBridge assembly operators. It was fetched twice - once when the combined CA cert secret is generated, and again when the hash is calculated with certs and auth.
Refactored the reconcile loop such that it is only fetched once per reference.
KafkaConnect
It is fetched in
tlsTrustedCertsSecretand returned with the method, which is passed into thegenerateAuthHashmethod.KafkaBridge
A similar pattern is used here - but since a new secret isn't generated, it is fetched directly in the loop and passed to the hash method.
KafkaMirrorMaker2
Since there are multiple certs for each mirror's target and source, all the certs are fetched and stored in a
HashMap. It relies on the fact that each cluster in MM2 has a unique alias. It is then passed to both secret generation and auth hash methods when needed.I have also removed 2 tests in
ReconcilerUtilsTestwhich checked if theauthTlsHashmethod fetched the secrets correctly. That method doesn't fetch trusted cert secrets anymore. It expects certs to be passed in as parameter.Disclosure: The operator tests were generated using AI and modified. I have reviewed it thoroughly before committing.
Checklist
Please go through this checklist and make sure all applicable tasks have been done