Skip to content

[SPARK-26929][SQL]fix table owner use user instead of principal when create table through spark-sql or beeline #23952

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
hddong wants to merge 12 commits into from
Closed

[SPARK-26929][SQL]fix table owner use user instead of principal when create table through spark-sql or beeline #23952

Changes from 10 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2609c9c
[SPARK-26929][SQL]fix table owner use user instead of principal when …
hddong Mar 4, 2019
05e54fe
Update HiveClientImpl.scala
hddong Jul 25, 2019
bdcc9fa
Update HiveClientImpl.scala
hddong Jul 25, 2019
c4e3468
Merge branch 'master' into SPARK-26929-fix-table-owner
hddong Jul 25, 2019
e8c0571
Update HiveClientImpl.scala
hddong Jul 25, 2019
c27d4af
Update HiveClientImpl.scala
hddong Jul 25, 2019
f1eb8cf
Update HiveClientImpl.scala
hddong Jul 26, 2019
0c7ddb1
Update HiveClientImpl.scala
hddong Sep 10, 2019
6011598
Update HiveClientImpl.scala
hddong Sep 10, 2019
76192b2
Update HiveClientImpl.scala
hddong Sep 10, 2019
a8d6b1d
Update HiveClientImpl.scala
hddong Sep 11, 2019
2f37513
Update HiveClientImpl.scala
hddong Sep 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import org.apache.hadoop.hive.ql.session.SessionState
import org.apache.hadoop.hive.serde.serdeConstants
import org.apache.hadoop.hive.serde2.MetadataTypedColumnsetSerDe
import org.apache.hadoop.hive.serde2.`lazy`.LazySimpleSerDe
import org.apache.hadoop.security.UserGroupInformation

import org.apache.spark.{SparkConf, SparkException}
import org.apache.spark.internal.Logging
Expand Down Expand Up @@ -220,7 +221,19 @@ private[hive] class HiveClientImpl(
hiveConf
}

private val userName = conf.getUser
private val userName = try {
val doAs = sys.env.get("HADOOP_USER_NAME").orNull
val ugi = if (doAs != null && doAs.length() > 0) {

@vanzin vanzin Sep 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong or, at the very least, backwards: the current UGI should be preferred.

What's the goal of this code? Why can't you just get the current UGI's name?

@HeartSaVioR HeartSaVioR Sep 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uh, at least that is not a regression, as conf.getUser calls Hive side and Hive does it in Utils.getUGI, at least from Hive 1.2.1.spark2.
So the code is copied and pasted to modify calling ugi.getShortUserName() instead of ugi.getUserName() - @hddong left a comment before to explain why the code had to be copied and pasted - #23952 (comment)

@vanzin vanzin Sep 11, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, and there's a huge comment in the Hive method (at least in the branch I'm looking at) that explains why that is done. And if you read that comment you'll see that it does not apply here.

In a way, the HMS API is broken in that it lets the caller set the owner (instead of getting it from the auth info).

But we really should at least try to get the correct information through, and that comes from the UGI, not from env variables.

@HeartSaVioR HeartSaVioR Sep 11, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah actually I just read decompiled code of Utils.getUGI() (missed to read comment in source) and didn't indicate the intention - let other application be able to pass it. Yes I agree it's not needed in Spark side, and it would be weird HADOOP_USER_NAME is only used here and undocumented. Thanks for explaining!

@hddong hddong Sep 12, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for you two, I missed to read comment in source too. Yes, current UGI is ok here, SPARK-22846 Fix table owner is null when creating table through spark sql or thriftserver, but lead to an issue that have occurred(owner is principal in kerberized cluster).

UserGroupInformation.createProxyUser(doAs, UserGroupInformation.getLoginUser())
} else {
UserGroupInformation.getCurrentUser
}
ugi.getShortUserName

@cloud-fan cloud-fan Mar 21, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we prove this is the right fix? What if there is no UGI?

@hddong hddong Mar 25, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cloud-fan , It will return system user information if there is no ugi. Actually, the source code of conf.getUser is:

public String getUser() throws IOException {
        try {
             UserGroupInformation ugi = Utils.getUGI(); 
             return ugi.getUserName();
         } catch (LoginException var2) { 
             throw new IOException(var2); 
         } 
     }

this change just use ugi.getShortUserName instead of ugi.getUserName()

@HeartSaVioR HeartSaVioR Aug 23, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like Utils is accessible from here, so if we just want to leverage Hive code and call getShortUserName instead, we can just do it with one-liner.

private val userName = org.apache.hadoop.hive.shims.Utils.getUGI.getShortUserName

Here I intended to not catch LoginException as we don't do anything but throw it.

@hddong hddong Aug 26, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

org.apache.hadoop.hive.shims.Utils.getUGI.getShortUserName

Yep, but shims.Utils is not compatible with hive0.*, and cannot pass all tests. This file is added after hive1.*, so, copy the source code here.

@HeartSaVioR HeartSaVioR Aug 26, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah OK. Didn't recognize it doesn't match with some versions. Makes sense to me.

@HeartSaVioR HeartSaVioR Aug 26, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f1eb8cf was for this, sorry I had to track down commits as well.

} catch {
case e: Exception =>
logError("Can not get login user.")

@HeartSaVioR HeartSaVioR Sep 10, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here: I'm not sure we really want to log error message here, as I expect thrown exception will make query fail somewhere in caller side, and it will log the exception correctly - stack trace in exception will tell us.

throw e
}

override def getConf(key: String, defaultValue: String): String = {
conf.get(key, defaultValue)
Expand Down