Support connecting to Kerberos secured Kafka #7990, updated to Kafka 0.10.1.2 #8394

madhurkumar · 2017-06-29T03:20:21Z

Fix for #7990

Merge from master

…Kafka Consumer API

…o Kafka 0.10.1.2

facebook-github-bot · 2017-06-29T03:20:44Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

electrum · 2017-06-29T03:26:43Z

@cawallin could someone on your team review this?

electrum · 2017-06-29T05:38:41Z

pom.xml

@@ -56,6 +56,7 @@
        <dep.testng.version>6.10</dep.testng.version>
        <dep.nifty.version>0.15.1</dep.nifty.version>
        <dep.swift.version>0.15.2</dep.swift.version>
+        <dep.kafka-clients.version>0.10.2.1</dep.kafka-clients.version>


Don't add a property for a version that is only used in one place. We use properties for related artifacts that need to be versioned together.

Thanks, modified as per suggestion.

electrum · 2017-06-29T05:39:46Z

presto-kafka/pom.xml

            <groupId>joda-time</groupId>
            <artifactId>joda-time</artifactId>
        </dependency>
-
+        <!--


Please delete rather than commenting out. The old version is available in the Git history. Commenting out code quickly clutters the code base.

Done. Thanks

electrum · 2017-06-29T05:43:14Z

presto-kafka/pom.xml

@@ -68,14 +68,20 @@
        </dependency>

        <dependency>
+            <groupId>org.apache.kafka</groupId>
+            <artifactId>kafka-clients</artifactId>
+            <version>${dep.kafka-clients.version}</version>


Add this to the dependencyManagement section in the root POM, then remove the version here. In almost all cases, versions should only be in the root POM, ensuring versions are consistent.

(Technically, this is not necessary here, because this dependency is only used in this one module, and different plugins can actually have different versions of the same library due to class loader isolation. But we follow the rule as much as possible so it's easy for everyone to understand and not wonder why there are exceptions.)

Thanks, changes made per suggestion.

facebook-github-bot · 2017-06-29T06:05:36Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

…o Kafka 0.10.1.2

madhurkumar · 2017-06-29T16:02:37Z

Thanks for review comments, changes made as per suggestion.

cawallin · 2017-06-29T17:22:11Z

@electrum sure

gaohao · 2017-07-08T00:45:06Z

@madhurkumar after all these changes, will it still work with kafka 0.8?

madhurkumar · 2017-07-15T02:20:47Z

No, Kafka client versions are forward compatible not fully backward compatible. In my tests, message consumption from 0.8.2.2 broker did not work.

cawallin

Thanks for your contribution! And sorry for the delay.

A few questions: how does this patch get the Kerberos credentials? Is it as in here http://kafka.apache.org/documentation.html#security_client_staticjaas? For Presto, we avoid setting JAAS config files; instead, we handle the security stuff using Java code. The Hive connector does this (see https://github.com/prestodb/presto/blob/master/presto-hive/src/main/java/com/facebook/presto/hive/authentication/KerberosAuthentication.java that allows you to get a Subject; you should use that Subject and wrap all calls to Kafka with doAs -- see https://github.com/airlift/airlift/blob/be20a24b5bba2c5d674a4a7280e80cf61cf3e1cc/http-client/src/main/java/io/airlift/http/client/spnego/SpnegoAuthentication.java#L111). Also, the Kafka service name should be configurable; it isn't necessarily always going to be kafka (which I'm assuming is the default).

Lastly, since you're going to update the minimum supported version of Kafka, please send an email to presto-users@googlegroups.com to see if anyone is using Presto to connect to Kafka below the new minimum supported version.

cawallin · 2017-07-28T17:20:58Z

presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaConnectorConfig.java

@@ -168,4 +175,28 @@ private static HostAddress toHostAddress(String value)
    {
        return HostAddress.fromString(value).withDefaultPort(KAFKA_DEFAULT_PORT);
    }
+
+    @Config("kafka.security-protocol")
+    public KafkaConnectorConfig setSecurityProtocol(String securityProtocol)


This should be an enum, so that the possible values can be validated

cawallin · 2017-07-28T21:06:18Z

presto-main/etc/catalog/kafka.properties

+kafka.table-names=test
+kafka.hide-internal-columns=false
+kafka.security-protocol=PLAINTEXT
+kafka.auto-commit=false


add a newline, please

gaohao · 2017-07-29T00:11:48Z

I am using kafka 0.8 & 0.10
The current kafka plugin works well on 0.10. So Is that possible to just keep two kafka plugins.

cawallin · 2017-07-31T14:00:26Z

@gaohao It is possible to have two Kafka plugins (the Hive plugin was similar until recently), though it does add an additional maintenance cost. The more code that can be shared, the better.

ironchefpython · 2017-09-08T18:34:37Z

@cawallin It would be possible to support multiple versions with the same plugin, however the plugin would need to dynamically create a classloader for the requested kafka client version.

Is there another plugin that does this as an example?

electrum · 2017-09-09T15:02:23Z

The way to do this is to split up the plugin, similar to how we used to have multiple variants of the Hive connector for different Hadoop versions. The same structure still exists today.

madhurkumar · 2018-04-17T16:13:51Z

@cawallin sorry for the delay, I checked Kafka client but it doesn't support Kerberos without JAAS file please see https://github.com/apache/kafka/blob/0.10.0/clients/src/main/java/org/apache/kafka/common/security/authenticator/AbstractLogin.java
This PR will allow users to have an option to run presto against secured Kafka using JAAS file until Kafka client is modified to support alternatives.
I have fixed other review comments, please let me know if this can be merged now.

ashinohara · 2018-10-27T23:46:11Z

Hello @madhurkumar - I stumbled across this PR as I am attempting to use presto against a kafka cluster with SASL enabled. It looks like the most recent version of the KafkaConsumer allows use of a property sasl.jaas.config where you can pass in the jaas configuration as text. This would allow usage without a jaas file. I would be happy to help contribute in order to get this PR moving again.

madhurkumar · 2018-11-09T15:55:23Z

Hello @madhurkumar - I stumbled across this PR as I am attempting to use presto against a kafka cluster with SASL enabled. It looks like the most recent version of the KafkaConsumer allows use of a property sasl.jaas.config where you can pass in the jaas configuration as text. This would allow usage without a jaas file. I would be happy to help contribute in order to get this PR moving again.

Sure, make sense to remove jaas file and allow configurations through property map.

nitenA · 2018-11-15T08:55:39Z

Hello @madhurkumar,

I was trying connecting presto to my secured Kafka which uses Sasl protocol instead plaintext. Seems I am on old version so it didn’t find all such properties mentioned in Kafka.properties

Just FYI, I am currently using both jaas config and direct properties map to established secured connection. Let me know if you need example using Kafka client code.

Btw, Is there any presto version for now which I can use immediate to connect secure Kafka and I won’t mind providing jaas config for temporary purpose.

nitenA · 2018-11-16T20:40:51Z

pls let me know

nitenA · 2018-11-16T20:41:25Z

Please let me know

madhurkumar · 2018-11-20T01:43:20Z

Please let me know

@nitenA you can use my fork

https://github.com/madhurkumar/presto

Conflicts: pom.xml presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaInternalFieldDescription.java presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaRecordSet.java presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaRecordSetProvider.java presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaSimpleConsumerManager.java presto-kafka/src/main/java/com/facebook/presto/kafka/KafkaSplitManager.java

rja1 · 2019-01-22T23:14:23Z

@madhurkumar,

Thanks for all your work on this. Do you have an ETA on when these changes will be merged to master?

sopel39 · 2019-02-12T09:55:28Z

@madhurkumar Can you also create a PR to https://github.com/prestosql/presto? We’re interested in this.

stale · 2019-08-11T10:31:53Z

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions!

madhurkumar and others added 4 commits June 27, 2017 16:33

Merge pull request #1 from prestodb/master

dcc11c9

Merge from master

Updated to Kafka 0.10.1.2, support for security protocol through new …

f5a6606

…Kafka Consumer API

Support connecting to Kerberos secured Kafka prestodb#7990, updated t…

a90dea9

…o Kafka 0.10.1.2

Support connecting to Kerberos secured Kafka prestodb#7990, updated t…

031cd2c

…o Kafka 0.10.1.2

facebook-github-bot added the CLA Signed label Jun 29, 2017

electrum reviewed Jun 29, 2017

View reviewed changes

Support connecting to Kerberos secured Kafka prestodb#7990, updated t…

71b8c68

…o Kafka 0.10.1.2

cawallin self-requested a review June 29, 2017 17:22

Support for message timestamp @ 0.10.1.1.

bb9976f

cawallin suggested changes Jul 28, 2017

View reviewed changes

madhurkumar added 4 commits March 6, 2018 23:57

Merge branch 'master' of https://github.com/prestodb/presto

6a1db5c

fix style check and split manager interface change

2c99cf1

Security protocol values as enum

64c68ee

Fixed test for security protocol enum values

a0c036a

newline added.

72c6691

kokosing mentioned this pull request Jul 25, 2019

Support connecting to Kerberos secured Kafka trinodb/trino#1184

Open

stale bot added the stale label Aug 11, 2019

stale bot closed this Aug 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support connecting to Kerberos secured Kafka #7990, updated to Kafka 0.10.1.2 #8394

Support connecting to Kerberos secured Kafka #7990, updated to Kafka 0.10.1.2 #8394

madhurkumar commented Jun 29, 2017

facebook-github-bot commented Jun 29, 2017

electrum commented Jun 29, 2017

electrum Jun 29, 2017

madhurkumar Jun 29, 2017

electrum Jun 29, 2017

madhurkumar Jun 29, 2017

electrum Jun 29, 2017

madhurkumar Jun 29, 2017

facebook-github-bot commented Jun 29, 2017

madhurkumar commented Jun 29, 2017

cawallin commented Jun 29, 2017

gaohao commented Jul 8, 2017

madhurkumar commented Jul 15, 2017

cawallin left a comment

cawallin Jul 28, 2017

cawallin Jul 28, 2017

madhurkumar Apr 17, 2018

gaohao commented Jul 29, 2017

cawallin commented Jul 31, 2017

ironchefpython commented Sep 8, 2017

electrum commented Sep 9, 2017 via email

madhurkumar commented Apr 17, 2018

ashinohara commented Oct 27, 2018

madhurkumar commented Nov 9, 2018

nitenA commented Nov 15, 2018

nitenA commented Nov 16, 2018

nitenA commented Nov 16, 2018

madhurkumar commented Nov 20, 2018

rja1 commented Jan 22, 2019

sopel39 commented Feb 12, 2019

stale bot commented Aug 11, 2019

Support connecting to Kerberos secured Kafka #7990, updated to Kafka 0.10.1.2 #8394

Support connecting to Kerberos secured Kafka #7990, updated to Kafka 0.10.1.2 #8394

Conversation

madhurkumar commented Jun 29, 2017

facebook-github-bot commented Jun 29, 2017

electrum commented Jun 29, 2017

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

facebook-github-bot commented Jun 29, 2017

madhurkumar commented Jun 29, 2017

cawallin commented Jun 29, 2017

gaohao commented Jul 8, 2017

madhurkumar commented Jul 15, 2017

cawallin left a 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

gaohao commented Jul 29, 2017

cawallin commented Jul 31, 2017

ironchefpython commented Sep 8, 2017

electrum commented Sep 9, 2017 via email

madhurkumar commented Apr 17, 2018

ashinohara commented Oct 27, 2018

madhurkumar commented Nov 9, 2018

nitenA commented Nov 15, 2018

nitenA commented Nov 16, 2018

nitenA commented Nov 16, 2018

madhurkumar commented Nov 20, 2018

rja1 commented Jan 22, 2019

sopel39 commented Feb 12, 2019

stale bot commented Aug 11, 2019