[Zeppelin 2367] Hive JDBC proxy user option should be available even without kerberos #2229
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,6 @@ | |
| import java.util.HashMap; | ||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Properties; | ||
| import java.util.Set; | ||
|
|
||
|
|
@@ -179,10 +178,6 @@ public void open() { | |
| } | ||
| logger.debug("JDBC PropretiesMap: {}", basePropretiesMap); | ||
|
|
||
|
Member
There was a problem hiding this comment. this is also removed?
Contributor
Author
There was a problem hiding this comment.
Contributor
Author
There was a problem hiding this comment. Since it's just a removal of extra call, so no doc change required. |
||
| setMaxLineResults(); | ||
| } | ||
|
|
||
|
|
@@ -358,65 +353,70 @@ public Connection getConnection(String propertyKey, InterpreterContext interpret | |
| } else { | ||
| UserGroupInformation.AuthenticationMethod authType = JDBCSecurityImpl.getAuthtype(property); | ||
|
|
||
| final String connectionUrl = appendProxyUserToURL(url, user, propertyKey); | ||
|
|
||
| JDBCSecurityImpl.createSecureConfiguration(property, authType); | ||
| switch (authType) { | ||
| case KERBEROS: | ||
| if (url.trim().startsWith("jdbc:hive")) { | ||
| connection = getConnectionFromPool(connectionUrl, user, propertyKey, properties); | ||
| } else { | ||
| UserGroupInformation ugi = null; | ||
| try { | ||
| ugi = UserGroupInformation.createProxyUser( | ||
| user, UserGroupInformation.getCurrentUser()); | ||
| } catch (Exception e) { | ||
| logger.error("Error in getCurrentUser", e); | ||
| StringBuilder stringBuilder = new StringBuilder(); | ||
| stringBuilder.append(e.getMessage()).append("\n"); | ||
| stringBuilder.append(e.getCause()); | ||
| throw new InterpreterException(stringBuilder.toString()); | ||
| } | ||
|
|
||
| final String poolKey = propertyKey; | ||
| try { | ||
| connection = ugi.doAs(new PrivilegedExceptionAction<Connection>() { | ||
| @Override | ||
| public Connection run() throws Exception { | ||
| return getConnectionFromPool(connectionUrl, user, poolKey, properties); | ||
| } | ||
| }); | ||
| } catch (Exception e) { | ||
| logger.error("Error in doAs", e); | ||
| StringBuilder stringBuilder = new StringBuilder(); | ||
| stringBuilder.append(e.getMessage()).append("\n"); | ||
| stringBuilder.append(e.getCause()); | ||
| throw new InterpreterException(stringBuilder.toString()); | ||
|
Member
There was a problem hiding this comment. why not just include e as inner exception like
Contributor
Author
There was a problem hiding this comment. Sure just did it, thank you for the hint. |
||
| } | ||
| } | ||
| break; | ||
|
|
||
| default: | ||
| connection = getConnectionFromPool(connectionUrl, user, propertyKey, properties); | ||
| } | ||
| } | ||
|
|
||
| return connection; | ||
| } | ||
|
|
||
| private String appendProxyUserToURL(String url, String user, String propertyKey) { | ||
| StringBuilder connectionUrl = new StringBuilder(url); | ||
|
|
||
| if (user != null && !user.equals("anonymous") && | ||
| basePropretiesMap.get(propertyKey).containsKey("proxy.user.property")) { | ||
|
|
||
| Integer lastIndexOfUrl = connectionUrl.indexOf("?"); | ||
| if (lastIndexOfUrl == -1) { | ||
| lastIndexOfUrl = connectionUrl.length(); | ||
| } | ||
| logger.info("Using hive proxy user as :" + user); | ||
| connectionUrl.insert(lastIndexOfUrl, ";" + | ||
| basePropretiesMap.get(propertyKey).getProperty("proxy.user.property") + "=" + user + ";"); | ||
| } | ||
|
|
||
| return connectionUrl.toString(); | ||
| } | ||
|
|
||
| private String getPassword(Properties properties) throws IOException { | ||
| if (isNotEmpty(properties.getProperty(PASSWORD_KEY))) { | ||
| return properties.getProperty(PASSWORD_KEY); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is
hive.proxy.userremoved 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)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant in above line was the key is
default.proxy.user.propertywith an example valuehive.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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was looking at http://zeppelin.apache.org/docs/0.7.1/interpreter/jdbc.html#apache-hive,
hive.proxy.userhasn't made it to the doc section, so I think in this case no need to add migration steps. However, I have addedlogger.warnas well.