Handle missing secret/secret key for custom certificates #12274
Conversation
Signed-off-by: Gantigmaa Selenge <[email protected]>
|
|
||
| private static Future<CertAndKey> getCertificateAndKeyAsync(SecretOperator secretOperator, String namespace, KafkaClientAuthenticationTls auth) { | ||
| return getValidatedSecret(secretOperator, namespace, auth.getCertificateAndKey().getSecretName(), auth.getCertificateAndKey().getCertificate(), auth.getCertificateAndKey().getKey()) | ||
| //TODO: shouldn't this be decoded since we encode again? Otherwise encoded value gets encoded again for calculating the hash. |
There was a problem hiding this comment.
@scholzj can I please clarify something here
I was going to make this method public and then use it for extracting CertAndKey from the secret, but realised that it returns it with secret data values that are not decoded (stays encoded in base64). Then the method that calls this method authTlsHash(), encodes the values again, meaning that certificates and keys end up encoded in base64 twice before calculating the hash for it. I'm not sure if that makes sense or was intentional so wonder if we should decode the extracted values before constructing CertAndKey object to return, just like I did in the new method below. In that case we wouldn't need 2 separate methods for the purpose.
There was a problem hiding this comment.
I do not think there is any intention of having the hash calculated from data that are twice encoded to Base64 (that said, doing the base64 encoding twice is wasteful, but it should still produce a stable hash, so it should not really cause major issues).
But I think that if you can do it easily, you can definitely remove it. I guess this was either done by mistake or because some of the values being fed there are decoded and some are encoded.
I do not remember the details of how the CertAndKey is used, so not 100% sure if it is better to keep the values encoded or decoded inside it. Normally, the base64 encoding matters for us only when interacting with Kubernetes Secrets. But the certificates themself should be anyway strings even without the base64 encoding.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12274 +/- ##
============================================
+ Coverage 74.81% 74.84% +0.03%
- Complexity 6629 6639 +10
============================================
Files 376 376
Lines 25348 25366 +18
Branches 3401 3402 +1
============================================
+ Hits 18963 18985 +22
+ Misses 4997 4993 -4
Partials 1388 1388
🚀 New features to boost your workflow:
|
2 participants
Type of change
Select the type of your PR
Description
This PR adds a logic to copy the secrets for custom broker certificates provided by users into the per broker secret generated by the operator. This secret will now contain internal TLS listener certificates generated by the operator as well as the custom broker certificate user has provided in the Kafka CR. The internal TLS certificates is stored with podname as the key for example,
my-cluster-b-abcms-0.crt. The custom broker certificates will be stored with the listener name and port as the key for example,external-9104.crt, so the listener name and port will be used to configure the listeners when brokers get restarted.This way if the custom broker certificate secret or secret key is deleted/changed and brokers got restarted for some reason, they would not go into Pending or CrashLoopBackOff as the per broker secrets would still have the copy of the secret data. However, the operator will fail the reconciliation when trying to reconcile the per broker secrets since the custom broker certificate secret/secret key to copy is missing. This would notify user to fix the issue without risking the cluster availability.
Closes #12000
Checklist
Please go through this checklist and make sure all applicable tasks have been done