HIVE-28716: Add owner information in HivePrivilegeObject for create database query - #5616
Conversation
asf-ci-hive
added
tests pending
tests unstable
tests failed
and removed
tests pending
tests unstable
labels
1236730 to
fa1a08e
Compare
asf-ci-hive
added
tests pending
tests failed
and removed
tests failed
tests pending
labels
| if (managedLocationUri != null) { | ||
| database.setManagedLocationUri(managedLocationUri); | ||
| } | ||
| database.setOwnerName(SessionState.getUserFromAuthenticator()); |
There was a problem hiding this comment.
CreateDatabaseOperation sets the ownername both for native and remote db type. I wonder, should we follow the same pattern?
There was a problem hiding this comment.
Thanks for the review @InvisibleProgrammer, actually I was under some other assumption.
I will address this.
…atabase query Check RANGER-5097 for more info
fa1a08e to
cb3b8f6
Compare
asf-ci-hive
added
tests pending
tests failed
and removed
tests failed
tests pending
labels
| database.setType(DatabaseType.REMOTE); | ||
| database.setRemote_dbname(remoteDbName); | ||
| } | ||
| database.setOwnerName(SessionState.getUserFromAuthenticator()); |
There was a problem hiding this comment.
Would it be better or more consistent if we set the owner type? I could be wrong
There was a problem hiding this comment.
Thanks @okumin, for the review and a vaild question but I think there is no need to set owner type. My reasoning is, why set additional info which is not getting utilized by Ranger and post authorization the code flow will enter the CreateDatabaseOperation and there ownerType and other attributes of class will be set. We only need owner name info as it is used by ranger, it doesn't require owner type. In future if Ranger requires ownerType info then we can address it separately.
There was a problem hiding this comment.
Thanks. Your statement makes sense. Let me make the last challenge.
We currently encounter the problem of Apache Ranger, and this minimal fix surely resolves the problem. I agree that this fact supports your opinion.
I'd like to add two perspectives here. First, I think adding owner information is theoretically not only for Ranger, i.e., another authorization system or another plugin such as HiveSemanticAnalyzerHook can use the information. Second, the context would be more explicit and less confusing with a principal type as users don't have to worry that the name is linked to a role or group. Because of the two reasons, I am guessing aligning this with CreateDatabaseOperation is not a bad investment.
At least, this change will likely work and resolve the problem. So, I don't have any strong objections to have this merged. Just I want to make a decision once I get your response as you are more knowledgeable with this part 👍
There was a problem hiding this comment.
Sure @okumin, the first point makes sense if other authorizer or in future ranger starts using it. I will update the PR by adding database.setOwnerType(PrincipalType.USER);
My understanding is when we do kinit ntid/serviceId@Realm , even serviceId is considered as user. So setting PrincipalType.USER from hive should be ok!
|
okumin
left a comment
okumin
left a comment
There was a problem hiding this comment.
+1. I'm waiting for CI and 24 hours
…atabase query (apache#5616) (Raghav Aggarwal reviewed by Zsolt Miskolczi, Shohei Okumiya)
4 participants
What changes were proposed in this pull request?
Refer to HIVE-28716 and RANGER-5097 for the info
Why are the changes needed?
Currently we are not passing the owner information for database in HivePrivilegeObject. This object is passed to ranger and ranger is using this owner information but as it is missing, NoSuchObjectException is thrown HS2 logs. Check the above mentioned JIRA for stacktrace and debugging screenshot.
NOTE: There is no impact on the functionality, table/db is getting created in hive. It more of a noise in the HS2 logs coming from ranger.
Does this PR introduce any user-facing change?
No
Is the change a dependency upgrade?
No
How was this patch tested?
On Cluster testing and monitoring HS2 logs