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.

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 SSL connection parameter for Schema registry #237

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

Conversation

jlvelez
Copy link

@jlvelez jlvelez commented Feb 1, 2021

Made changes to Kafdrop so that it can connect to a secure Schema Registry endpoint.

Added the following parameters:

  • SCHEMAREGISTRY_PROPERTIES : Additional properties to configure the schema registry connection (base-64 encoded). Provides keystore/trustore location and password.
  • SCHEMAREGISTRY_TRUSTSTORE : Certificate for schema registry authentication (base-64 encoded). Required for TLS/SSL.
  • SCHEMAREGISTRY_KEYSTORE : Private key for mutual TLS authentication (base-64 encoded).

These work similar to

  • KAFKA_PROPERTIES
  • KAFKA_TRUSTSTORE
  • KAFKA_KEYSTORE

@Elpaggio
Copy link

Why hasn't this pull request been merged yet?

Copy link
Collaborator

@davideicardi davideicardi left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

Just some minor comments ...

There are some ways to test the new configuration? maybe adding some unit test or integration tests?

@@ -76,6 +76,16 @@
<groupId>org.projectlombok</groupId>
<artifactId>lombok</artifactId>
</dependency>
<dependency>
<groupId>org.apache.kafka</groupId>
Copy link
Collaborator

Choose a reason for hiding this comment

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

From my understending Kafdrop uses spring-kafka. So I think that to update Kafka clients we have to update spring-kafka.
Is required for this PR to update kafka-clients? If not I suggest to remove this update.

<version>2.6.0</version>
</dependency>
<dependency>
<groupId>org.freemarker</groupId>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you have added this dependency? If not strictly required for the PR I suggest to remove it (and in case create another PR ...).

@@ -274,6 +274,9 @@ docker run -d --rm -p 9000:9000 \
|`KAFKA_PROPERTIES_FILE`|Internal location where the Kafka properties file will be written to (if `KAFKA_PROPERTIES` is set). Defaults to `kafka.properties`.
|`KAFKA_TRUSTSTORE_FILE`|Internal location where the truststore file will be written to (if `KAFKA_TRUSTSTORE` is set). Defaults to `kafka.truststore.jks`.
|`KAFKA_KEYSTORE_FILE` |Internal location where the keystore file will be written to (if `KAFKA_KEYSTORE` is set). Defaults to `kafka.keystore.jks`.
|`SCHEMAREGISTRY_PROPERTIES`|Additional properties to configure the schema registry connection (base-64 encoded). Provides keystore/trustore location and password.
|`SCHEMAREGISTRY_TRUSTSTORE`|Certificate for schema registry authentication (base-64 encoded). Required for TLS/SSL.
|`SCHEMAREGISTRY_KEYSTORE` |Private key for mutual TLS authentication (base-64 encoded).

Copy link
Collaborator

Choose a reason for hiding this comment

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

It could also useful to add a simple documentation on how to configure these properties and when is necessary?

@davideicardi davideicardi added the enhancement New feature or request label Dec 28, 2021
@Bert-R Bert-R changed the title added ssl connetion parameter for Schema registry Add SSL connection parameter for Schema registry Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants