HIVE-29167: Use access() api to check permission for different filesystems #6047
Conversation
| public class SecurityUtils { | ||
| private static final Logger LOG = LoggerFactory.getLogger(SecurityUtils.class); | ||
|
|
||
| public static UserGroupInformation getUGI() throws LoginException, IOException { |
There was a problem hiding this comment.
The LoginException would never be thrown, I consider to remove all uses of it in a new PR because it's not quite relative to current PR.
asf-ci-hive
added
tests passed
tests pending
and removed
tests pending
tests passed
labels
|
@ayushtkn @deniskuzZ @zhangbutao could you help review this PR? |
| return null; | ||
| }); | ||
| } catch (InterruptedException e) { | ||
| throw new IOException(e); |
There was a problem hiding this comment.
Why not throw AccessControlException?
There was a problem hiding this comment.
AccessControlException will be thrown directly, and InterruptedException is usually not related to permission.
| public class SecurityUtils { | ||
| private static final Logger LOG = LoggerFactory.getLogger(SecurityUtils.class); | ||
|
|
||
| public static UserGroupInformation getUGI() throws LoginException, IOException { |
| return; | ||
| try { | ||
| ugi.doAs((PrivilegedExceptionAction<Void>) () -> { | ||
| fs.access(path, action); |
There was a problem hiding this comment.
In case of hdfs filesystem, If user A belongs to the superuser group fs.getConf().get("dfs.permissions.supergroup", ""), then user A should have all permissions.
Can the fs.access(path, action) determine whether user A has all permissions?
There was a problem hiding this comment.
Yes, hdfs access() API will also skip checkPermission for super user:
https://github.com/apache/hadoop/blob/4d7825309348956336b8f06a08322b78422849b1/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSDirectory.java#L1954-L1960
|
LGTM @wecharyu Please rebase to fix the conflicts. |
|
3 participants
What changes were proposed in this pull request?
Use filesystem's
access()api to check permission.Why are the changes needed?
The permission modes vary in different filesystems, we could not just check permissions based on the group mode.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Removed the
supergrouptest and add a new test inTestHdfsUtils.mvn test -Dtest.groups= -Dtest=org.apache.hadoop.hive.metastore.utils.TestHdfsUtils -pl :hive-standalone-metastore-server