-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Use PEM certificates loaded from secrets for Kafka #11447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
e5eaea1
to
eb7a6d6
Compare
eb7a6d6
to
8ffd37f
Compare
@ppatierno @katheris can you please review this PR when you get a chance? Thank you :) |
8ffd37f
to
91eaf20
Compare
...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaReconciler.java
Outdated
Show resolved
Hide resolved
reconciliation.namespace(), | ||
oauthSecret, | ||
kafka.generateSecret(Map.of(oauthSecret + ".crt", mergeAndEncodeCerts(certs)), oauthSecret)) | ||
.mapEmpty()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the core part of this method is exactly the same as the authzTrustedCertsSecret
. Is there any way to factor out a common method here? wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They look similar but quite different with the way we join all the secrets. For cluster trusted certs, we just add them under individual keys in the secret, so it's simple. For OAuth, we are collecting the certs as a list of strings first and then decode and merge them into a single string before encoding and putting it under a single key in the secret.
However, these two methods are getting used in several places in KafkaConnect, Kafka and KafkaBridge. So I think if I should move them in AuthenticationUtils, instead of them repeating them in their assembly operator classes.
sslContextFactory.setTrustStorePath(sslTruststorePath); | ||
sslContextFactory.setTrustStorePassword(sslTruststorePassword); | ||
sslContextFactory.setKeyStore(KafkaAgentUtils.jksKeyStore(nodeCertSecret)); | ||
sslContextFactory.setKeyStorePassword("changeit"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about this "changeit"? We had a random generated password when using the script.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had followed what KafkaAgentClient had, but you are right, we should do the same as the script. I added a method to generate random password with 32 chars like the script did.
3d7de64
to
3363f8a
Compare
f67eec1
to
595b61b
Compare
Thank @ppatierno so much for reviewing the PR. I have now addressed your comments. Could you also please kick off the regression tests? |
/azp run regression |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run regression |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look pretty good to me, I just had a couple of questions and suggestions that I added.
writer.println(CruiseControlConfigurationParameters.METRICS_REPORTER_SSL_TRUSTSTORE_LOCATION + "=/tmp/kafka/cluster.truststore.p12"); | ||
writer.println(CruiseControlConfigurationParameters.METRICS_REPORTER_SSL_TRUSTSTORE_PASSWORD + "=" + PLACEHOLDER_CERT_STORE_PASSWORD_CONFIG_PROVIDER_ENV_VAR); | ||
writer.println(CruiseControlConfigurationParameters.METRICS_REPORTER_SSL_KEYSTORE_TYPE + "=PEM"); | ||
writer.println(CruiseControlConfigurationParameters.METRICS_REPORTER_SSL_KEYSTORE_CERTIFICATE_CHAIN + "=" + String.format(PLACEHOLDER_SECRET_TEMPLATE_KUBE_CONFIG_PROVIDER, reconciliation.namespace(), node.podName(), node.podName() + ".crt")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether the code would read better if we had a method, e.g.
writer.println(CruiseControlConfigurationParameters.METRICS_REPORTER_SSL_KEYSTORE_CERTIFICATE_CHAIN + "=" + String.format(PLACEHOLDER_SECRET_TEMPLATE_KUBE_CONFIG_PROVIDER, reconciliation.namespace(), node.podName(), node.podName() + ".crt")); | |
writer.println(CruiseControlConfigurationParameters.METRICS_REPORTER_SSL_KEYSTORE_CERTIFICATE_CHAIN + "=" + secretConfigProvider(reconciliation.namespace(), node.podName(), node.podName() + ".crt")); |
I can't remember what we've done for the other similar changes though, so maybe this is something we could look at as a follow up PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a good idea but yeah maybe I can do it in a follow up PR.
...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/ReconcilerUtils.java
Outdated
Show resolved
Hide resolved
...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/ReconcilerUtils.java
Outdated
Show resolved
Hide resolved
...er-operator/src/main/java/io/strimzi/operator/cluster/operator/assembly/ReconcilerUtils.java
Outdated
Show resolved
Hide resolved
/** | ||
* Class with various utility methods for generating KeyStore and TrustStore for KafkaAgent | ||
*/ | ||
public class KafkaAgentUtils { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of these methods are duplicates of ones we have elsewhere, but I assume you put them here so we didn't have to pull in any other Strimzi modules and create cyclic dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was the idea. I didn't think it was good idea to pull in any Strimzi modules into the agent running in the Kafka process.
@tinaselenge I restarted failed regression tests, not sure if they were related to the PR but there were quite a few. Let's see the next run. |
They failed even for the previous runs, so I guess they are related to the PR. |
yes, they are definitely related as they failed locally for me as well. I fixed OAuth related failures but still trying to fix some failures in ListenersST that tests listeners with custom certificates. I will update the PR once I have it passing locally. |
/azp run regression |
Azure Pipelines successfully started running 1 pipeline(s). |
bfede7c
to
8ae072a
Compare
@strimzi/maintainers can someone please kick off the regression tests? I believe I have fixed them now. Thank you. |
/azp run regression |
Azure Pipelines successfully started running 1 pipeline(s). |
b493973
to
6650758
Compare
There is one remaining system test failure:
They fail because of the changes I made to KafkaAgent is missing from kafka-agent-3 (agent for Kafka 3.x versions). To fix this test, I would have to duplicate all the changes in kafka-agent-3. However Kafka 4.1.0 is currently in the process of getting released and will soon be supported in the next Strimzi release. This means the next Strimzi version will not support Kafka 3.9.x version, therefore kafka-agent-3 will be removed. Given that, we will likely to remove it in the next 2,3 weeks, I decided not to fix this test and wait for it to get removed before merging this PR. While we wait for that to happen, I think this PR can be reviewed again since all the review comments were addressed and other system tests were fixed. |
@tinaselenge could you please solve the conflicts we have now :-( |
Signed-off-by: Gantigmaa Selenge <[email protected]>
Signed-off-by: Gantigmaa Selenge <[email protected]>
…rets into ReconcilerUtils Signed-off-by: Gantigmaa Selenge <[email protected]>
Signed-off-by: Gantigmaa Selenge <[email protected]>
Signed-off-by: Gantigmaa Selenge <[email protected]>
Signed-off-by: Gantigmaa Selenge <[email protected]>
Signed-off-by: Gantigmaa Selenge <[email protected]>
6650758
to
c604c5b
Compare
@ppatierno done :) |
Type of change
Description
ssl.truststore.certificates
is because Jaas configuration cannot parse multiline of certificates. (This might get improved in a future PR but for now going with a simpler approach to keep the refactoring minimal).Resolves part of #11294
Checklist
Please go through this checklist and make sure all applicable tasks have been done