Use PEM directly accessed from secrets in KafkaConnect and MirrorMaker2#12735
Use PEM directly accessed from secrets in KafkaConnect and MirrorMaker2#12735tinaselenge wants to merge 5 commits into
Conversation
Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12735 +/- ##
============================================
+ Coverage 74.95% 75.05% +0.10%
- Complexity 6369 6429 +60
============================================
Files 345 346 +1
Lines 24126 24272 +146
Branches 3090 3108 +18
============================================
+ Hits 18083 18217 +134
- Misses 4811 4822 +11
- Partials 1232 1233 +1
🚀 New features to boost your workflow:
|
|
/azp run regression |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/gha run pipeline=regression |
|
⏳ System test verification started: link The following 6 job(s) will be executed:
Tests will start after successful build completion. |
|
All regression tests passed but image build step failed with some error. Kicking them off again to get green build. |
|
🎉 System test verification passed: link |
| if (mirrorMaker2Cluster.getTls() != null && mirrorMaker2Cluster.getTls().getTrustedCertificates() != null) { | ||
| allTrustedCertificates.addAll(mirrorMaker2Cluster.getTls().getTrustedCertificates()); | ||
| } | ||
|
|
||
| if (mirrorMaker2Cluster instanceof KafkaMirrorMaker2Cluster mm2Cluster) { | ||
| for (KafkaMirrorMaker2ClusterSpec cluster : mm2Cluster.clusters()) { | ||
| if (cluster.getTls() != null && cluster.getTls().getTrustedCertificates() != null) { | ||
| allTrustedCertificates.addAll(cluster.getTls().getTrustedCertificates()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
- Do we need to worry about uniqueness?
- I think it is fine to collect them into a single Secret. But it is not fine to collect them into a single
ca.crtfile. That would be a CVE as you would trust certificates you should not trust, or?
(E.g. mirror A trusts CA X and mirror B trusts CA Y. If you mix them into a single file, mirror A would now trust also CA Y and so on.)
There was a problem hiding this comment.
I agree with Jakub I guess we should distinguish the two trusting.
There was a problem hiding this comment.
Good point, I have now updated the method to use cluser alias as the key.
| if (!allTrustedCertificates.isEmpty()) { | ||
| return ReconcilerUtils.trustedCertificates(reconciliation, secretOperations, allTrustedCertificates) | ||
| .compose(certificates -> { | ||
| if (certificates != null) { | ||
| return secretOperations.reconcile( | ||
| reconciliation, | ||
| namespace, | ||
| KafkaConnectResources.internalTlsTrustedCertsSecretName(mirrorMaker2Cluster.getCluster()), | ||
| mirrorMaker2Cluster.generateTlsTrustedCertsSecret( | ||
| Map.of("ca.crt", Util.encodeToBase64(certificates)), | ||
| KafkaConnectResources.internalTlsTrustedCertsSecretName(mirrorMaker2Cluster.getCluster()) | ||
| )) | ||
| .mapEmpty(); | ||
| } else { | ||
| return Future.succeededFuture(); | ||
| } | ||
| }); | ||
| } else { | ||
| return Future.succeededFuture(); | ||
| } |
There was a problem hiding this comment.
Should there be some cleanup in the else branch? E.g. deleting the Secret or something?
There was a problem hiding this comment.
That's good point. so if user has removed the TLS configuration, the internal secret is not left behind. This is something I should fix for KafkaConnect and KafkaBridge in another PR.
✅ 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. |
Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
tinaselenge
force-pushed
the
mirror-connect-pem
branch
from
5ac600d to
6e1440d
Compare
Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
|
/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 |
|
/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 |
|
@tinaselenge some regression tests are failing and they could be related to your changes. I didn't look into them but it seems related to TLS within MM2. |
|
I will take a look, thanks @ppatierno |
Signed-off-by: Gantigmaa Selenge <tina.selenge@gmail.com>
tinaselenge
force-pushed
the
mirror-connect-pem
branch
from
769a96a to
d1553d2
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. |
3 participants
Type of change
Select the type of your PR
Description
Remove PKCS12 usage in MirrorMaker2 so that it uses PEM files from secrets directly accessed using KubernetesSecretConfigProvider. Also authentication clients secrets are accessed directly via KubernetesSecretConfigProvider instead of volume mounting them.
Refactor KafkaConnect to also access authentication clients secrets directly instead of volume mounting them and to use AuthenticationUtils for creating role/rolebinding resources for them.
Closes #12605
Checklist
Please go through this checklist and make sure all applicable tasks have been done