[Zeppelin 2367] Hive JDBC proxy user option should be available even without kerberos #2229

prabhjyotsingh · 2017-04-06T11:46:31Z

What is this PR for?

Hive JDBC proxy user option should be available generically.

What type of PR is it?

[Improvement]

What is the Jira issue?

Zeppelin 2367

How should this be tested?

Enable Shiro authentication and set zeppelin.jdbc.auth.type as SIMPLE in the interpreter setting, and observe the connection string for the Hive.

Screenshots (if appropriate)

Questions:

Does the licenses files need an update? N/A
Is there breaking changes for older versions? N/A
Does this needs documentation? Yes

…ut kerberos

r-kamath · 2017-04-07T12:18:05Z

Tested. LGTM.

felixcheung · 2017-04-08T07:37:50Z

docs/interpreter/hive.md

 ```
+
+### Impersonation
+When Zeppelin server is running with authentication enabled, then this interpreter utilizes Hive's user proxy feature i.e. sends extra parameter for creating and running a session ("hive.server2.proxy.user=": "${loggedInUser}"). This is particularly useful when multi users are sharing a Notebook server.


multi users -> multiple users?

does this affect resource allocation, such as each user could be on different YARN queue and so on?

Yes, that could happen, but it was never documented, hence thought to document it here.

felixcheung · 2017-04-08T07:39:37Z

jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java

  }

+  private void checkAndAppendHiveProxyUser(StringBuilder connectionUrl, String user) {
+    if (connectionUrl.toString().trim().startsWith("jdbc:hive")) {


hmm.. is this the right approach and abstraction, to have specialize type in a general JDBC interpreter?

I thought about other approaches, like using propertyKey, but even in that case name could be anything including default.

felixcheung · 2017-04-08T07:40:40Z

jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java

+
+      if (user != null && !user.equals("anonymous") &&
+          !"false".equalsIgnoreCase(property.getProperty("hive.proxy.user"))) {
+        logger.debug("Using hive proxy user");


logger.info?
also, log the user value?

AhyoungRyu · 2017-04-08T13:44:41Z

docs/interpreter/hive.md

    <td></td>
    <td><b>( Optional ) </b>Other properties used by the driver of <code>%hive(${prefix})</code> </td>
  </tr>
+  <tr>


@prabhjyotsingh Hive docs is deprecated: https://github.com/apache/zeppelin/blob/master/docs/interpreter/hive.md#important-notice

So it'll be better to add this info to https://github.com/apache/zeppelin/blob/master/docs/interpreter/jdbc.md#apache-hive instead.

Sure, then I'll revert my changes from this file, and apply it on jdbc.md

AhyoungRyu · 2017-04-08T13:46:57Z

docs/interpreter/hive.md

+  <tr>
+    <td>hive.proxy.user</td>
+    <td>true</td>
+    <td><b>( Optional ) </b>If want to use `hive.server2.proxy.user`</td>


These back quoters won't work properly in <table> tag. So please use <code> tag like this:
<code>hive.server2.proxy.user</code>

Thank you @AhyoungRyu , have taken care of this in jdbc.md.

prabhjyotsingh · 2017-04-11T18:19:23Z

@AhyoungRyu @felixcheung thank you for the review, have made the suggested changes.

prabhjyotsingh · 2017-04-12T16:53:26Z

@AhyoungRyu, @felixcheung ping

prabhjyotsingh · 2017-04-14T03:23:24Z

Merging this to master and branch-0.7 if no more discussion.

felixcheung · 2017-04-14T17:25:27Z

From my earlier comment, I'm interested in others thought on having very Hive specific logic in the generic JDBC interpreter? https://github.com/apache/zeppelin/pull/2229/files#r110512765

prabhjyotsingh · 2017-04-14T17:51:16Z

Yes, I thought of few options but this was bit convenient. Any suggestions?

# Conflicts: # jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java

prabhjyotsingh · 2017-04-20T06:42:20Z

@felixcheung any specific change that you are looking for?

felixcheung · 2017-04-20T06:55:24Z

Perhaps generalize this to something non-Hive specific? Like a property for userStringToAppend? Add user to JDBC URL should be common place - seems like in this case we just like a way to change the name and value of the property to add on the JDBC URL.

prabhjyotsingh · 2017-04-20T07:20:28Z

Sure, got it, have renamed it to appendProxyUserToURL.

felixcheung · 2017-04-23T01:15:47Z

actually, I wasn't referring to the function name at all. instead of having this fairly hive specific, how about a function like this to take the user name property name and value

// Untested!
 private void appendProxyUserToURL(StringBuilder connectionUrl, String user) {
     if ("true".equalsIgnoreCase(property.getProperty("jdbc.url.set.user") {
       Integer lastIndexOfUrl = connectionUrl.indexOf("?");
       if (lastIndexOfUrl == -1) {
         lastIndexOfUrl = connectionUrl.length();
       }
 
       if (user != null && !user.equals("anonymous") &&
           property.getProperty("jdbc.url.user.property").length() > 1)  { // check not empty or whitespace instead
         logger.info("Using jdbc user as :" + user);
         connectionUrl.insert(lastIndexOfUrl, ";" + property.getProperty("jdbc.url.user.property") + "=" + user + ";");
       }
     }
   }

prabhjyotsingh · 2017-04-24T11:42:37Z

@felixcheung Sure what you have recommended does make a lot of sense, have implemented what you have suggested.

prabhjyotsingh · 2017-04-25T06:06:55Z

CI is green;
https://travis-ci.org/prabhjyotsingh/zeppelin/builds/225173700
https://travis-ci.org/prabhjyotsingh/zeppelin/builds/225214327

felixcheung

looks reasonable. why is zeppelin.jdbc.auth.kerberos.proxy.enable removed?
also is there a way not to hardcode hive jdbc here

felixcheung · 2017-04-26T05:22:23Z

jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java

+                StringBuilder stringBuilder = new StringBuilder();
+                stringBuilder.append(e.getMessage()).append("\n");
+                stringBuilder.append(e.getCause());
+                throw new InterpreterException(stringBuilder.toString());


why not just include e as inner exception like InterpreterException(message, e)?

Sure just did it, thank you for the hint.

prabhjyotsingh · 2017-04-26T07:02:34Z

why is zeppelin.jdbc.auth.kerberos.proxy.enable removed?

My bad, while fixing the above issues, I thought this is redundant, but I forgot about the case where KERBEROS is enabled and the user does not want to use proxy behavior. Have reverted that change.

also is there a way not to hardcode hive jdbc here

Hive doesn't work with this (UserGroupInformation.createProxyUser), and since we allow user to configure multiple JDBC sources using the same interpreter setting; there can be a case where user has configured say Phoenix (requires UserGroupInformation.createProxyUser to proxy user) and Hive (fails if UserGroupInformation.createProxyUser is used), we could introduce new parameter like {profile}.kerberos.proxy.enable but this will increase the documentation part.

prabhjyotsingh · 2017-04-28T13:25:42Z

@felixcheung have removed hard coded string "hive" as well.

CI fails for; not relavent for this change.

Tests in error: 
  ParagraphActionsIT.testClearOutputButton:335->AbstractZeppelinIT.clickAndWait:126 Â» ElementNotVisible

prabhjyotsingh · 2017-04-29T03:53:14Z

@felixcheung ping

felixcheung

I like it! a few questions

felixcheung · 2017-04-29T17:44:04Z

docs/interpreter/jdbc.md

+  </tr>
 </table>

 Connection to Hive JDBC with a proxy user can be disabled with `hive.proxy.user` property (set to true by default)


is hive.proxy.user removed then? do we need to doc to explain the migration?
also you have this line here still with hive.proxy.user property (set to true by default)

I guess the preference would be to have a way to migrate existing users and config by having it automatically converted, add a log warning etc.

but it's your call in the specific case

What I meant in above line was the key is default.proxy.user.property with an example value hive.server2.proxy.user; fair point, let me tweak this documentation.

Yes, adding a log (logger.warn) does make sense, let me do it right away.

I was looking at http://zeppelin.apache.org/docs/0.7.1/interpreter/jdbc.html#apache-hive, hive.proxy.user hasn't made it to the doc section, so I think in this case no need to add migration steps. However, I have added logger.warn as well.

felixcheung · 2017-04-29T17:44:52Z

jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java

    }
    logger.debug("JDBC PropretiesMap: {}", basePropretiesMap);

-    if (!isEmpty(property.getProperty("zeppelin.jdbc.auth.type"))) {


this is also removed? zeppelin.jdbc.auth.type?

JDBCSecurityImpl.createSecureConfiguration call is moved to line#358 (https://github.com/apache/zeppelin/pull/2229/files/9fee9d2a9e476769b5d28a50d17f3a7ad38fc453#diff-ecdae8ee9594a5c4b21a3c217a3f130cR358). Instead of making this object JDBCSecurityImpl.getAuthtype(property) every time on open() do it only if required.

ah ok, doc change?

Since it's just a removal of extra call, so no doc change required.

felixcheung · 2017-04-29T17:48:08Z

docs/interpreter/jdbc.md

+
+To enable this set following:
+  - `zeppelin.jdbc.auth.type` as `SIMPLE` or `KERBEROS` (if required) in the interpreter setting.
+  - `default.proxy.user.property` as `hive.server2.proxy.user`


should we clarify, default here could also be the name of the hive interpreter instance like hive.proxy.user.property?

Sure, let me change this to {propertyKey}.proxy.user.property and post a sample configuration.

Thank you for reviewing this. Have handled your review comments.

prabhjyotsingh · 2017-05-02T07:02:54Z

@felixcheung Have handled your review comments. Let me know if I missed any.

felixcheung

LGTM

prabhjyotsingh · 2017-05-03T02:59:14Z

Thank you @felixcheung. Will merge this to master.

@AhyoungRyu

### What is this PR for? Fix JDBC doc after #2229. ### What type of PR is it? [Bug Fix] ### Screenshots (if appropriate) ### Questions: * Does the licenses files need update? * Is there breaking changes for older versions? * Does this needs documentation? Author: Prabhjyot Singh <[email protected]> Closes #2314 from prabhjyotsingh/minor/jdbc-doc-zeppelin-2367 and squashes the following commits: e54a3a2 [Prabhjyot Singh] @AhyoungRyu review comments 0f396ac [Prabhjyot Singh] fix doc for zeppelin-2367

@AhyoungRyu

### What is this PR for? Fix JDBC doc after apache#2229. ### What type of PR is it? [Bug Fix] ### Screenshots (if appropriate) ### Questions: * Does the licenses files need update? * Is there breaking changes for older versions? * Does this needs documentation? Author: Prabhjyot Singh <[email protected]> Closes apache#2314 from prabhjyotsingh/minor/jdbc-doc-zeppelin-2367 and squashes the following commits: e54a3a2 [Prabhjyot Singh] @AhyoungRyu review comments 0f396ac [Prabhjyot Singh] fix doc for zeppelin-2367

…without kerberos ### What is this PR for? Hive JDBC proxy user option should be available generically. ### What type of PR is it? [Improvement] ### What is the Jira issue? * [Zeppelin 2367](https://issues.apache.org/jira/browse/ZEPPELIN-2367) ### How should this be tested? Enable Shiro authentication and set `zeppelin.jdbc.auth.type` as `SIMPLE` in the interpreter setting, and observe the connection string for the Hive. ### Screenshots (if appropriate) ### Questions: * Does the licenses files need an update? N/A * Is there breaking changes for older versions? N/A * Does this needs documentation? Yes Author: Prabhjyot Singh <[email protected]> Closes apache#2229 from prabhjyotsingh/ZEPPELIN-2367 and squashes the following commits: 84b5e55 [Prabhjyot Singh] add logger.warn for hive and impersonation 45c90a8 [Prabhjyot Singh] improve doc 9fee9d2 [Prabhjyot Singh] replace hive with generic method a348e96 [Prabhjyot Singh] revert "zeppelin.jdbc.auth.kerberos.proxy.enable" behaviour e2bdbb2 [Prabhjyot Singh] include e as inner exception c180f5c [Prabhjyot Singh] Merge remote-tracking branch 'origin/master' into ZEPPELIN-2367 1802b45 [Prabhjyot Singh] remove hive string from logger 513987a [Prabhjyot Singh] apply genric logic to appendProxyUserToURL 3fa2b1e [Prabhjyot Singh] change name to appendProxyUserToURL a751674 [Prabhjyot Singh] Merge remote-tracking branch 'origin/master' into ZEPPELIN-2367 4c382ee [Prabhjyot Singh] log user details as well d51e770 [Prabhjyot Singh] add doc in jdbc.md 01b18b9 [Prabhjyot Singh] add doc (reverted from commit ee8a6b5) 40489c8 [Prabhjyot Singh] Merge remote-tracking branch 'origin/master' into ZEPPELIN-2367 ee8a6b5 [Prabhjyot Singh] add doc 8999d93 [Prabhjyot Singh] ZEPPELIN-2367: Hive JDBC proxy user option should be avail even without kerberos

prabhjyotsingh added 2 commits April 6, 2017 17:12

ZEPPELIN-2367: Hive JDBC proxy user option should be avail even witho…

8999d93

…ut kerberos

add doc

ee8a6b5

prabhjyotsingh changed the title ~~[Zeppelin 2367] Hive JDBC proxy user option should be avail even without kerberos~~ [Zeppelin 2367] Hive JDBC proxy user option should be available even without kerberos Apr 6, 2017

felixcheung reviewed Apr 8, 2017

View reviewed changes

AhyoungRyu reviewed Apr 8, 2017

View reviewed changes

prabhjyotsingh added 4 commits April 9, 2017 09:50

Merge remote-tracking branch 'origin/master' into ZEPPELIN-2367

40489c8

add doc (reverted from commit ee8a6b5)

01b18b9

add doc in jdbc.md

d51e770

log user details as well

4c382ee

Merge remote-tracking branch 'origin/master' into ZEPPELIN-2367

a751674

# Conflicts: # jdbc/src/main/java/org/apache/zeppelin/jdbc/JDBCInterpreter.java

change name to appendProxyUserToURL

3fa2b1e

apply genric logic to appendProxyUserToURL

513987a

prabhjyotsingh force-pushed the ZEPPELIN-2367 branch from c0fd7d4 to 513987a Compare April 24, 2017 11:36

remove hive string from logger

1802b45

Merge remote-tracking branch 'origin/master' into ZEPPELIN-2367

c180f5c

prabhjyotsingh closed this Apr 24, 2017

prabhjyotsingh reopened this Apr 24, 2017

felixcheung reviewed Apr 26, 2017

View reviewed changes

prabhjyotsingh added 2 commits April 26, 2017 12:06

include e as inner exception

e2bdbb2

revert "zeppelin.jdbc.auth.kerberos.proxy.enable" behaviour

a348e96

prabhjyotsingh force-pushed the ZEPPELIN-2367 branch from 962e610 to a348e96 Compare April 26, 2017 06:53

replace hive with generic method

9fee9d2

felixcheung reviewed Apr 29, 2017

View reviewed changes

prabhjyotsingh added 2 commits April 30, 2017 00:01

improve doc

45c90a8

add logger.warn for hive and impersonation

84b5e55

felixcheung approved these changes May 2, 2017

View reviewed changes

asfgit closed this in 08a9814 May 3, 2017

prabhjyotsingh mentioned this pull request May 3, 2017

[minor] Fix JDBC doc after #2229 #2314

Closed

prabhjyotsingh deleted the ZEPPELIN-2367 branch February 25, 2018 03:50

[Zeppelin 2367] Hive JDBC proxy user option should be available even without kerberos #2229

[Zeppelin 2367] Hive JDBC proxy user option should be available even without kerberos #2229

Uh oh!

Conversation

prabhjyotsingh commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is this PR for?

What type of PR is it?

What is the Jira issue?

How should this be tested?

Screenshots (if appropriate)

Questions:

Uh oh!

r-kamath commented Apr 7, 2017

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!

Choose a reason for hiding this comment

Uh oh!

prabhjyotsingh commented Apr 11, 2017

Uh oh!

prabhjyotsingh commented Apr 12, 2017

Uh oh!

prabhjyotsingh commented Apr 14, 2017

Uh oh!

felixcheung commented Apr 14, 2017 via email

Uh oh!

prabhjyotsingh commented Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prabhjyotsingh commented Apr 20, 2017

Uh oh!

felixcheung commented Apr 20, 2017 via email

Uh oh!

prabhjyotsingh commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

felixcheung commented Apr 23, 2017

Uh oh!

prabhjyotsingh commented Apr 24, 2017

Uh oh!

prabhjyotsingh commented Apr 25, 2017

Uh oh!

felixcheung 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!

prabhjyotsingh commented Apr 26, 2017

Uh oh!

prabhjyotsingh commented Apr 28, 2017

Uh oh!

prabhjyotsingh commented Apr 29, 2017

Uh oh!

felixcheung 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

prabhjyotsingh commented Apr 6, 2017 •

edited

Loading

prabhjyotsingh commented Apr 14, 2017 •

edited

Loading

prabhjyotsingh commented Apr 20, 2017 •

edited

Loading

prabhjyotsingh Apr 29, 2017 •

edited

Loading