Skip to content
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.

Sign up for GitHub

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

Add support for AWS MSK IAM #513

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add support for AWS MSK IAM #513

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bc1ef2c
Added support for iam enabled msk using custom role attached to k8s pod
Jun 15, 2021
4d8e5d1
Add sts version.
richardwu Apr 21, 2023
98fab17
Comments.
richardwu Apr 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
<protobuf.version>3.22.3</protobuf.version>
<testcontainers.version>1.18.0</testcontainers.version>
<kafka-libs.version>7.3.3</kafka-libs.version>
<msk.auth.version>1.0.0</msk.auth.version>
<sts.sdk.version>1.11.704</sts.sdk.version>
Comment on lines +26 to +27
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If next comment is accepted, this can be removed.

</properties>

<scm>
Expand Down Expand Up @@ -76,6 +78,11 @@
<groupId>org.projectlombok</groupId>
<artifactId>lombok</artifactId>
</dependency>
<dependency>
<groupId>software.amazon.msk</groupId>
<artifactId>aws-msk-iam-auth</artifactId>
<version>${msk.auth.version}</version>
Comment on lines +82 to +84
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</dependency>
<dependency>
<groupId>io.confluent</groupId>
<artifactId>kafka-avro-serializer</artifactId>
Expand Down Expand Up @@ -143,6 +150,12 @@
<groupId>org.springframework.kafka</groupId>
<artifactId>spring-kafka</artifactId>
</dependency>

<dependency>
<groupId>com.amazonaws</groupId>
<artifactId>aws-java-sdk-sts</artifactId>
<version>${sts.sdk.version}</version>
</dependency>
Comment on lines +154 to +158
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above.

<dependency>
<groupId>org.springframework.boot</groupId>
<artifactId>spring-boot-starter-undertow</artifactId>
Expand Down
17 changes: 15 additions & 2 deletions src/main/java/kafdrop/config/KafkaConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,15 +28,22 @@ public final class KafkaConfiguration {
private String truststoreFile;
private String propertiesFile;
private String keystoreFile;
private String jaasConfig;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private String jaasConfig;
private String saslJaasConfig;

private String clientCallback;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private String clientCallback;
private String saslClientCallback;

private String iamEnabled;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this a boolean, so you don't need to call parseBoolean at line 56.


public void applyCommon(Properties properties) {
properties.setProperty(CommonClientConfigs.BOOTSTRAP_SERVERS_CONFIG, brokerConnect);

if (isSecured) {
LOG.warn("The 'isSecured' property is deprecated; consult README.md on the preferred way to configure security");
properties.put(SaslConfigs.SASL_MECHANISM, saslMechanism);
}

if (isSecured || securityProtocol.equals("SSL")) {
LOG.info("Setting sasl mechanism to {}", saslMechanism);
properties.put(SaslConfigs.SASL_MECHANISM, saslMechanism);

if (isSecured || securityProtocol.equals("SSL") || securityProtocol.equals("SASL_SSL")) {
LOG.info("Setting security protocol to {}", securityProtocol);
properties.put(CommonClientConfigs.SECURITY_PROTOCOL_CONFIG, securityProtocol);
}

Expand All @@ -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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spell IAM in uppercase

if (Boolean.parseBoolean(iamEnabled)) {
LOG.info("Setting sasl.jaas.config {} and sasl and callback callback properties {}", jaasConfig, clientCallback);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

properties.put(SaslConfigs.SASL_CLIENT_CALLBACK_HANDLER_CLASS, clientCallback);
properties.put(SaslConfigs.SASL_JAAS_CONFIG, jaasConfig);
}

LOG.info("Checking keystore file {}", keystoreFile);
if (new File(keystoreFile).isFile()) {
Expand Down
9 changes: 6 additions & 3 deletions src/main/resources/application.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,12 @@ kafdrop.monitor:

kafka:
brokerConnect: localhost:9092
isSecured: false
saslMechanism: "PLAIN"
securityProtocol: "SASL_PLAINTEXT"
isSecured: "${KAFKA_IS_SECURED:false}"
saslMechanism: "${KAFKA_SASL_MECHANISM:PLAIN}"
securityProtocol: "${KAFKA_SECURITY_PROTOCOL:SASL_PLAINTEXT}"
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}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
jaasConfig: "${KAFKA_JAAS_CONFIG}"
saslJaasConfig: "${KAFKA_SASL_JAAS_CONFIG}"

clientCallback: "software.amazon.msk.auth.iam.IAMClientCallbackHandler"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
clientCallback: "software.amazon.msk.auth.iam.IAMClientCallbackHandler"
saslClientCallback: "${KAFKA_SASL_CLIENT_CALLBACK}"