Support kerberos authentication for Kudu connector by liyubin117 · Pull Request #3549 · trinodb/trino

liyubin117 · 2020-04-26T08:15:07Z

Add the following configuration to etc/catalog/kudu.properties to enable kerberos authentication:

   ## Whether to enable kerberos authentication, default is false
   kudu.kerberos-auth.enabled=true

   # whether to output kerberos debug information, default is false
   kudu.kerberos-auth.debug.enabled=true

   # The Kerberos principal that Presto will use when connecting to Kudu
   kudu.kerberos-auth.principal=xxx

   # Kudu client keytab location
   kudu.kerberos-auth.keytab=xxx.keytab

Kudu Changes

support kerberos authentication

Fixes #1237

cla-bot · 2020-04-26T08:15:09Z

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 submit the signed CLA to cla@prestosql.io. For more information, see https://github.com/prestosql/cla.

Praveen2112

Thanks for working on this. Please file a CLA

Praveen2112 · 2020-04-27T06:56:03Z

presto-kudu/src/main/java/io/prestosql/plugin/kudu/KuduClientConfig.java

    private boolean disableStatistics;
    private boolean schemaEmulationEnabled;
    private String schemaEmulationPrefix = "presto::";
+    private boolean kerberosAuthEnabled;


Can we have this in a separate config file ?

Praveen2112 · 2020-04-27T07:14:20Z

presto-kudu/src/main/java/io/prestosql/plugin/kudu/KuduClientSession.java

+    private final boolean kerberosAuthEnabled;

-    public KuduClientSession(KuduClient client, SchemaEmulation schemaEmulation)
+    public KuduClientSession(KuduClient client, SchemaEmulation schemaEmulation, boolean kerberosAuthEnabled)


Can we have it as a wrap of top of KuduClientSession ?

Praveen2112 · 2020-04-27T07:18:44Z

presto-kudu/src/main/java/io/prestosql/plugin/kudu/KuduModule.java

-        builder.defaultSocketReadTimeoutMs(config.getDefaultSocketReadTimeout().toMillis());
-        if (config.isDisableStatistics()) {
-            builder.disableStatistics();
+        KuduClient client;


Can we have separate module for the kerberos based KuduClient ?

Praveen2112 · 2020-04-27T07:34:16Z

presto-kudu/src/main/java/io/prestosql/plugin/kudu/KuduUtil.java

+            if (debugEnabled) {
+                System.setProperty("sun.security.krb5.debug", "true");
+            }
+            UserGroupInformation.setConfiguration(conf);


Do we need to add hadoop dependencies ? How about reusing KerberosAuthentication in presto plugiin toolkit ?

kokosing

Thank you for working on this, this will be a great contribution to Presto.

I left some general comments about implementation and configuration properties (to make in line with other components in Presto that are using KRB).

The biggest challenge is see here is testing. Do you have any idea how to tackle this? I would suggest to you setup product tests that could use https://github.com/prestosql/docker-images/tree/master/prestodev/kerberos. What do you think? I can help you go through this, but please prepare that is not something that can be written over night.

kokosing · 2020-04-27T05:38:22Z

presto-docs/src/main/sphinx/connector/kudu.rst

       ## Disable Kudu client's collection of statistics.
       #kudu.client.disable-statistics = false

+       #######################


I think this should be a regular subsection.

Same applies to ### Advanced Kudu Java client configuration.

@mosabua Any suggestsions?

kokosing · 2020-04-27T05:38:54Z

presto-kudu/pom.xml

+
+        <dependency>
+            <groupId>io.prestosql.hadoop</groupId>
+            <artifactId>hadoop-apache</artifactId>


Do we need entire hadoop dependency for this?

kokosing · 2020-04-27T05:39:16Z

presto-kudu/pom.xml

        </dependency>
+
+        <dependency>
+            <groupId>io.prestosql.hadoop</groupId>


please move it to other compile dependencies.

kokosing · 2020-04-27T05:41:07Z

presto-kudu/src/main/java/io/prestosql/plugin/kudu/KuduClientConfig.java

        return this;
    }
+
+    public boolean isKerberosAuthEnabled()


Please add kudu.authentication.type property that accepts KERBEROS or NONE

kokosing · 2020-04-27T05:41:48Z

presto-kudu/src/main/java/io/prestosql/plugin/kudu/KuduClientConfig.java

+        return kerberosPrincipal;
+    }
+
+    @Config("kudu.kerberos-auth.principal")


kudu.client.principal ?

kokosing · 2020-04-27T07:56:38Z

presto-kudu/src/main/java/io/prestosql/plugin/kudu/KuduClientConfig.java

+        return kerberosKeytab;
+    }
+
+    @Config("kudu.kerberos-auth.keytab")


kudu.client.keytab

kokosing · 2020-04-27T07:56:59Z

presto-kudu/src/main/java/io/prestosql/plugin/kudu/KuduClientConfig.java

+        return kerberosAuthDebugEnabled;
+    }
+
+    @Config("kudu.kerberos-auth.debug.enabled")


Please remove this property. One can add it using jvm.config

kokosing · 2020-04-27T07:58:38Z

presto-kudu/src/main/java/io/prestosql/plugin/kudu/KuduUtil.java

+import java.io.IOException;
+import java.security.PrivilegedExceptionAction;
+
+public class KuduUtil


Please use io.prestosql.plugin.base.authentication.KerberosAuthentication and io.prestosql.plugin.base.authentication.CachingKerberosAuthentication

kokosing · 2020-04-27T08:04:04Z

presto-kudu/src/main/java/io/prestosql/plugin/kudu/KuduClientConfig.java

+        return kerberosPrincipal;
+    }
+
+    @Config("kudu.kerberos-auth.principal")


Kerberos related properties should not be allowed to be configures if kudu.authentication.type=NONE. Please see usages how it typically achieved by look at usages of io.airlift.configuration.ConditionalModule#installModuleIf.

liyubin117 · 2020-04-29T02:16:35Z

@kokosing @Praveen2112 Thanks for your review and suggestions!
I will complete the pull request as soon as possible, in spite of a very urgent and unexpected internal deadline may breaks my later schedule, I will do my best

sumit-gupta-sgt · 2020-10-01T17:10:12Z

Any expected ETA for this functionality ?

grantatspothero · 2022-03-03T16:17:02Z

@sumit-gupta-sgt This feature was just released in Trino 372:
https://trino.io/docs/current/release/release-372.html#kudu-connector

hashhar · 2022-03-03T16:46:04Z

Closing this since it is now obsolete and was merged in #10953

Support kerberos authentication for Kudu connector

01cbc1c

liyubin117 mentioned this pull request Apr 27, 2020

Support Kerberos authentication for Kudu Connector #1237

Closed

findepi requested a review from Praveen2112 April 27, 2020 06:51

Praveen2112 reviewed Apr 27, 2020

View reviewed changes

kokosing reviewed Apr 27, 2020

View reviewed changes

ebyhr mentioned this pull request Oct 1, 2020

Support kerberos authentication for Kudu connector. #5367

Closed

findepi force-pushed the master branch from 8538e49 to 1f896ea Compare July 30, 2021 22:14

hashhar closed this Mar 3, 2022

Conversation

liyubin117 commented Apr 26, 2020 • edited by ebyhr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cla-bot bot commented Apr 26, 2020

Uh oh!

Praveen2112 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kokosing left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liyubin117 commented Apr 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sumit-gupta-sgt commented Oct 1, 2020

Uh oh!

grantatspothero commented Mar 3, 2022

Uh oh!

hashhar commented Mar 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

6 participants

liyubin117 commented Apr 26, 2020 •

edited by ebyhr

Loading

Praveen2112 left a comment •

edited

Loading

liyubin117 commented Apr 29, 2020 •

edited

Loading