Add support for basic auth to Kafka schema registry #25456

derbeneviv · 2025-03-29T18:27:29Z

Description

This is a clone of #23317
add config options to pass username\password to confluent schema registry for kafka plugin

due to GitHub limitations (see isaacs/github#361) i've re-created the MR

Additional context and related issues

resolves #12195 and #22679

Release notes

Release notes are required. Please propose a release note for me.

cla-bot · 2025-03-29T18:27:32Z

Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: DERBENEV Ivan V.
This is most likely caused by a git client misconfiguration; please make sure to:

check if your git client is configured with an email to sign commits git config --list | grep email
If not, set it up using git config --global user.email [email protected]
Make sure that the git commit email is configured in your GitHub account settings, see https://github.com/settings/emails

mosabua · 2025-03-29T20:23:08Z

Please change commit message and add authored by tags from the original PR

derbeneviv · 2025-03-29T21:56:33Z

@mosabua i've updated commit message, what do you mean by "authored by tags from the original PR"?
working on tests right now

derbeneviv · 2025-03-31T12:20:19Z

So i guess i've passed all the CI checks, can i have a review?
There were people complaining this is crucial for them for trino+kafka
i think @Praveen2112 and @hashhar were reviewers in a previous version of this MR

ebyhr

Can we add a test with dockerized schema registry?

.../src/main/java/io/trino/plugin/kafka/schema/KafkaSchemaRegistryClientPropertiesProvider.java

...-kafka/src/main/java/io/trino/plugin/kafka/schema/confluent/ConfluentSchemaRegistryAuth.java

...a/src/main/java/io/trino/plugin/kafka/schema/confluent/ConfluentSchemaRegistryBasicAuth.java

...afka/src/main/java/io/trino/plugin/kafka/schema/confluent/ConfluentSchemaRegistryConfig.java

...afka/src/main/java/io/trino/plugin/kafka/schema/confluent/ConfluentSchemaRegistryNoAuth.java

derbeneviv · 2025-04-03T12:37:58Z

i've also updated confluent version in TestKafkaLatestConnectorSmokeTest.java to the value set in DEFAULT_CONFLUENT_PLATFORM_VERSION

derbeneviv · 2025-04-03T12:40:03Z

@ebyhr

Can we add a test with dockerized schema registry?

i'm not sure i'm competent enough to do it from scratch. Is there a reference i can look at?

derbeneviv · 2025-04-03T14:32:17Z

also i don't understand - why is CI checking commit which was already overridden?
https://github.com/trinodb/trino/actions/runs/14243784278/job/39919657078?pr=25456

ebyhr · 2025-04-08T05:44:39Z

Is there a reference i can look at?

It seems there is https://hub.docker.com/r/confluentinc/cp-schema-registry. You can refer to TestingPostgreSqlServer for understanding how to use docker container in tests.

derbeneviv · 2025-04-14T17:21:52Z

@ebyhr done, i guess

Add basicauth option to authorize to kafka schema registry Update the code to master upstream

derbeneviv · 2025-04-16T10:50:40Z

Added doc section
@ebyhr would be great if you can review

Praveen2112 · 2025-04-16T13:13:36Z

.../src/main/java/io/trino/plugin/kafka/schema/KafkaSchemaRegistryClientPropertiesProvider.java

+        if (basicAuthConfig.isPresent()) {
+            auth = new ConfluentSchemaRegistryBasicAuth(
+                   basicAuthConfig.get().getConfluentSchemaRegistryUsername(),
+                   basicAuthConfig.get().getConfluentSchemaRegistryPassword());
+        }
+        else {
+            auth = new ConfluentSchemaRegistryNoAuth();
+        }


Can we use guice magic here ? i.e based on the config we could directly inject ConfluentSchemaRegistryBasicAuth or ConfluentSchemaRegistryNoAuth

Praveen2112 · 2025-04-16T13:14:25Z

plugin/trino-kafka/src/main/java/io/trino/plugin/kafka/schema/confluent/BasicAuthConfig.java

+
+    @Config("kafka.confluent-schema-registry.basic-auth.username")
+    @ConfigDescription("The username for the Confluent Schema Registry")
+    public BasicAuthConfig setConfluentSchemaRegistryUsername(String username)


Can we add ConfigSecuritySensitive to ensure it is masked as a part of bootstrap logs ?

Praveen2112 · 2025-04-16T13:15:13Z

...afka/src/main/java/io/trino/plugin/kafka/schema/confluent/ConfluentSchemaRegistryConfig.java

@@ -54,6 +62,19 @@ public ConfluentSchemaRegistryConfig setConfluentSchemaRegistryUrls(Set<String>
        return this;
    }

+    public Optional<ConfluentSchemaRegistryAuthType> getConfluentSchemaRegistryAuthType()


Why do we need it to be Optional ?

Praveen2112 · 2025-04-16T13:15:53Z

.../src/main/java/io/trino/plugin/kafka/schema/KafkaSchemaRegistryClientPropertiesProvider.java

The first commit is not specific to adding tests right ?

Praveen2112 · 2025-04-16T13:17:04Z

...va/io/trino/plugin/kafka/schema/confluent/TestKafkaWithConfluentSchemaRegistryBasicAuth.java

+    }
+
+    @Test
+    public void testBasicTopicBasicAuth2()


testTopicMixedCaste

Praveen2112 · 2025-04-16T13:17:21Z

...va/io/trino/plugin/kafka/schema/confluent/TestKafkaWithConfluentSchemaRegistryBasicAuth.java

Can we apply the trino test guidelines here ?

Praveen2112 · 2025-04-16T13:18:40Z

testing/trino-testing-kafka/src/main/java/io/trino/testing/kafka/TestingKafka.java

-                        schemaRegistryLogTemplate,
-                        "/etc/confluent/docker/log4j.properties.template")
-                .dependsOn(kafka);
+        if (this.withBasicAuth) {


Instead of adding withBasicAuth flag - Can we extract SchemaRegistry container outside TestingKafka or have some sort of abuilder for them ?

Praveen2112 · 2025-04-16T13:19:15Z

testing/trino-testing-kafka/src/main/resources/schema_registry_password-file

Any readme file on how did we create this file would be helpful.

Praveen2112 · 2025-04-16T13:20:57Z

.../test/java/io/trino/plugin/kafka/schema/confluent/KafkaWithConfluentSchemaRegistryUtils.java

+import static org.junit.jupiter.api.parallel.ExecutionMode.SAME_THREAD;
+
+@Execution(SAME_THREAD)
+public class KafkaWithConfluentSchemaRegistryUtils


Is it Utils class or some test class ? In case of Utils only class it should be final in case of test class can we have Test as prefix

github-actions bot added the kafka Kafka connector label Mar 29, 2025

derbeneviv changed the title ~~update to master, fix comments~~ 12195 - add user\pass to kafka schema registry Mar 29, 2025

derbeneviv mentioned this pull request Mar 29, 2025

12195 - add user\pass to kafka schema registry #23317

Closed

derbeneviv force-pushed the 12195-add-credentials-to-kafka-schema-registry branch from 6129124 to 8ca276e Compare March 29, 2025 18:38

cla-bot bot added the cla-signed label Mar 29, 2025

derbeneviv force-pushed the 12195-add-credentials-to-kafka-schema-registry branch from 8ca276e to e91708f Compare March 29, 2025 21:53

derbeneviv force-pushed the 12195-add-credentials-to-kafka-schema-registry branch 2 times, most recently from 270e91f to 2c32e0c Compare March 31, 2025 11:56

ebyhr reviewed Mar 31, 2025

View reviewed changes

ebyhr changed the title ~~12195 - add user\pass to kafka schema registry~~ Add support for basic auth to Kafka schema registry Mar 31, 2025

ebyhr assigned derbeneviv Apr 3, 2025

derbeneviv force-pushed the 12195-add-credentials-to-kafka-schema-registry branch from 710705d to 046ea5e Compare April 3, 2025 12:34

derbeneviv force-pushed the 12195-add-credentials-to-kafka-schema-registry branch 2 times, most recently from 6df04e6 to 4420c09 Compare April 3, 2025 13:08

ebyhr force-pushed the 12195-add-credentials-to-kafka-schema-registry branch from 4420c09 to 54291d4 Compare April 6, 2025 22:15

derbeneviv force-pushed the 12195-add-credentials-to-kafka-schema-registry branch 3 times, most recently from 57e0316 to c4aad06 Compare April 14, 2025 15:55

derbeneviv force-pushed the 12195-add-credentials-to-kafka-schema-registry branch from c4aad06 to 7da2e2e Compare April 15, 2025 15:08

derbeneviv added 2 commits April 16, 2025 13:15

Add tests for confluent schema registry with basic auth

174522f

Add basicauth option to authorize to kafka schema registry Update the code to master upstream

rewrite tests for basic auth confluent schema registry

3ccc2a2

derbeneviv force-pushed the 12195-add-credentials-to-kafka-schema-registry branch from 7da2e2e to b7d0575 Compare April 16, 2025 10:26

github-actions bot added the docs label Apr 16, 2025

add docs for kafka confluent schema registry auth

b721aa6

derbeneviv force-pushed the 12195-add-credentials-to-kafka-schema-registry branch from b7d0575 to b721aa6 Compare April 16, 2025 10:52

Praveen2112 reviewed Apr 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for basic auth to Kafka schema registry #25456

Add support for basic auth to Kafka schema registry #25456

derbeneviv commented Mar 29, 2025

cla-bot bot commented Mar 29, 2025

mosabua commented Mar 29, 2025

derbeneviv commented Mar 29, 2025

derbeneviv commented Mar 31, 2025

ebyhr left a comment

derbeneviv commented Apr 3, 2025

derbeneviv commented Apr 3, 2025

derbeneviv commented Apr 3, 2025

ebyhr commented Apr 8, 2025

derbeneviv commented Apr 14, 2025

derbeneviv commented Apr 16, 2025

Praveen2112 Apr 16, 2025

Praveen2112 Apr 16, 2025

Praveen2112 Apr 16, 2025

Praveen2112 Apr 16, 2025

Praveen2112 Apr 16, 2025

Praveen2112 Apr 16, 2025

Praveen2112 Apr 16, 2025

Praveen2112 Apr 16, 2025

Praveen2112 Apr 16, 2025

Add support for basic auth to Kafka schema registry #25456

Are you sure you want to change the base?

Add support for basic auth to Kafka schema registry #25456

Conversation

derbeneviv commented Mar 29, 2025

Description

Additional context and related issues

Release notes

cla-bot bot commented Mar 29, 2025

mosabua commented Mar 29, 2025

derbeneviv commented Mar 29, 2025

derbeneviv commented Mar 31, 2025

ebyhr left a comment

Choose a reason for hiding this comment

derbeneviv commented Apr 3, 2025

derbeneviv commented Apr 3, 2025

derbeneviv commented Apr 3, 2025

ebyhr commented Apr 8, 2025

derbeneviv commented Apr 14, 2025

derbeneviv commented Apr 16, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment