Add support for AWS MSK IAM #513
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Dockerfile added Renamed Dockerfile Removed unwanted client.properties. Additional formatting changes Additional formatting changes Pom changes Updated dockerfile Updated pom properties. Updated yml with is secured Updated yml with is secured Updated yml with updated properties file Added warn log
Bert-R
requested changes
Apr 24, 2023
There was a problem hiding this comment.
Good extension!
I've added a couple of comments, including one for @davideicardi.
Can you also extend the README.md with an explanation about how to use this in combination with AWS MSK IAM?
| @@ -28,15 +28,22 @@ public final class KafkaConfiguration { | |||
| private String truststoreFile; | |||
| private String propertiesFile; | |||
| private String keystoreFile; | |||
| private String jaasConfig; | |||
| private String clientCallback; | |||
| private String iamEnabled; | |||
There was a problem hiding this comment.
Make this a boolean, so you don't need to call parseBoolean at line 56.
| @@ -45,6 +52,12 @@ public void applyCommon(Properties properties) { | |||
| LOG.info("Assigning truststore location to {}", truststoreFile); | |||
| properties.put("ssl.truststore.location", truststoreFile); | |||
| } | |||
| LOG.info("Is iam enabled : {}", iamEnabled); | |||
Comment on lines
+82
to
+84
| <groupId>software.amazon.msk</groupId> | ||
| <artifactId>aws-msk-iam-auth</artifactId> | ||
| <version>${msk.auth.version}</version> |
There was a problem hiding this comment.
@davideicardi What's your view on this? I'd rather not become dependent on Amazon JARs. The code doesn't need them, but the dependency exists when sasl.client.callback.handler.class (SASL_CLIENT_CALLBACK_HANDLER_CLASS) is set to software.amazon.msk.auth.iam.IAMClientCallbackHandler.
I propose to resolve this by changing kafdrop.sh to allow mounting a folder with extra classes. I recently did that for another project, see hapifhir/hapi-fhir-jpaserver-starter#514
There was a problem hiding this comment.
Yes, it seems to be a good idea to me. Loading extra classes could be a nice feature also for other cases.
We just have to write a good documentation on this kind of stuff.
Comment on lines
+154
to
+158
| <dependency> | ||
| <groupId>com.amazonaws</groupId> | ||
| <artifactId>aws-java-sdk-sts</artifactId> | ||
| <version>${sts.sdk.version}</version> | ||
| </dependency> |
Comment on lines
+26
to
+27
| <msk.auth.version>1.0.0</msk.auth.version> | ||
| <sts.sdk.version>1.11.704</sts.sdk.version> |
There was a problem hiding this comment.
If next comment is accepted, this can be removed.
| @@ -45,6 +52,12 @@ public void applyCommon(Properties properties) { | |||
| LOG.info("Assigning truststore location to {}", truststoreFile); | |||
| properties.put("ssl.truststore.location", truststoreFile); | |||
| } | |||
| LOG.info("Is iam enabled : {}", iamEnabled); | |||
| if (Boolean.parseBoolean(iamEnabled)) { | |||
| LOG.info("Setting sasl.jaas.config {} and sasl and callback callback properties {}", jaasConfig, clientCallback); | |||
There was a problem hiding this comment.
Suggested change
| LOG.info("Setting sasl.jaas.config {} and sasl and callback callback properties {}", jaasConfig, clientCallback); | |
| LOG.info("Setting SASL client callback to {} and JAAS config to ", clientCallback, jaasConfig); |
src/main/resources/application.yml
Outdated
| truststoreFile: "${KAFKA_TRUSTSTORE_FILE:kafka.truststore.jks}" | ||
| propertiesFile : "${KAFKA_PROPERTIES_FILE:kafka.properties}" | ||
| keystoreFile: "${KAFKA_KEYSTORE_FILE:kafka.keystore.jks}" | ||
| iamEnabled: "${KAFKA_IAM_ENABLED:false}" | ||
| jaasConfig: "${KAFKA_JAAS_CONFIG}" |
There was a problem hiding this comment.
Suggested change
| jaasConfig: "${KAFKA_JAAS_CONFIG}" | |
| saslJaasConfig: "${KAFKA_SASL_JAAS_CONFIG}" |
src/main/resources/application.yml
Outdated
| truststoreFile: "${KAFKA_TRUSTSTORE_FILE:kafka.truststore.jks}" | ||
| propertiesFile : "${KAFKA_PROPERTIES_FILE:kafka.properties}" | ||
| keystoreFile: "${KAFKA_KEYSTORE_FILE:kafka.keystore.jks}" | ||
| iamEnabled: "${KAFKA_IAM_ENABLED:false}" | ||
| jaasConfig: "${KAFKA_JAAS_CONFIG}" | ||
| clientCallback: "software.amazon.msk.auth.iam.IAMClientCallbackHandler" |
There was a problem hiding this comment.
Suggested change
| clientCallback: "software.amazon.msk.auth.iam.IAMClientCallbackHandler" | |
| saslClientCallback: "${KAFKA_SASL_CLIENT_CALLBACK}" |
| @@ -28,15 +28,22 @@ public final class KafkaConfiguration { | |||
| private String truststoreFile; | |||
| private String propertiesFile; | |||
| private String keystoreFile; | |||
| private String jaasConfig; | |||
There was a problem hiding this comment.
Suggested change
| private String jaasConfig; | |
| private String saslJaasConfig; |
| @@ -28,15 +28,22 @@ public final class KafkaConfiguration { | |||
| private String truststoreFile; | |||
| private String propertiesFile; | |||
| private String keystoreFile; | |||
| private String jaasConfig; | |||
| private String clientCallback; | |||
There was a problem hiding this comment.
Suggested change
| private String clientCallback; | |
| private String saslClientCallback; |
|
@richardwu When creating the pull request, did you check the box "Allow edits from maintainers"? If not, can you enable it as described here? Then I can add the code to enable adding classes to the KafDrop class path. |
|
@Bert-R Yup I did Let me know if it works. |
|
Ready for another look! |
|
@richardwu Taking a closer look, I wonder why you need the extra configuration settings. Can't you configure the AWS IAM using the configuration described in the readme?
cat << EOF > kafka.properties
security.protocol=SASL_SSL
sasl.mechanism=SCRAM-SHA-512
sasl.jaas.config=org.apache.kafka.common.security.scram.ScramLoginModule required username="foo" password="bar"
EOF
docker run -d --rm -p 9000:9000 \
-v $(pwd)/kafka.properties:/tmp/kafka.properties:ro \
-v $(pwd)/kafka.truststore.jks:/tmp/kafka.truststore.jks:ro \
-v $(pwd)/kafka.keystore.jks:/tmp/kafka.keystore.jks:ro \
-e KAFKA_BROKERCONNECT=<host:port,host:port> \
-e KAFKA_PROPERTIES_FILE=/tmp/kafka.properties \
-e KAFKA_TRUSTSTORE_FILE=/tmp/kafka.truststore.jks \ # optional
-e KAFKA_KEYSTORE_FILE=/tmp/kafka.keystore.jks \ # optional
obsidiandynamics/kafdropThe missing piece would be how to load the AWS classes, but that can be done with the mechanism I mentioned to Davide |
|
Technically you don't: what you describe works. I took the config changes from the previous example, so no strong opinion if it needs to happy or not. The dynamic loading AWS classes I'm less familiar with, so would be great for you to take it over. |
|
@richardwu Given that the implementation approach is very different, I created a new PR: #516. Please also add the list of JARs you need to add to make it work. |
|
@richardwu Can you respond to my previous comment? |
|
Hey @Bert-R thanks for the PR: I'm having some troubles building the jar: any guidance on what the quickest way to build the image? |
|
@richardwu Can you paste the command you use to run Kafdrop? |
|
Yup, this is when I run to build the JAR |
|
That's strange. This is my output: |
|
I see. Look at this in your output: Apparently, you have a local settings file of locally modified file that contains the SASL client callback handler |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Rebased/resolved conflicts from #287