Repository: incubator-sentry Updated Branches: refs/heads/master 02041c56d -> 9da6a6733
SENTRY-560: Sentry HDFS Syncup applies duplicate ACLs for the same scope, group and type (Reviewed by Lenni Kuff) Project: http://git-wip-us.apache.org/repos/asf/incubator-sentry/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-sentry/commit/9da6a673 Tree: http://git-wip-us.apache.org/repos/asf/incubator-sentry/tree/9da6a673 Diff: http://git-wip-us.apache.org/repos/asf/incubator-sentry/diff/9da6a673 Branch: refs/heads/master Commit: 9da6a67330f179636d20f5719cd514e6fd8fe892 Parents: 02041c5 Author: Arun Suresh <[email protected]> Authored: Tue Dec 2 22:42:15 2014 -0800 Committer: Arun Suresh <[email protected]> Committed: Tue Dec 2 22:42:15 2014 -0800 ---------------------------------------------------------------------- .../sentry/hdfs/SentryAuthorizationInfo.java | 10 +++- .../hdfs/SentryAuthorizationProvider.java | 48 ++++++++++++++------ .../hdfs/TestSentryAuthorizationProvider.java | 1 - .../tests/e2e/hdfs/TestHDFSIntegration.java | 18 ++++++++ 4 files changed, 61 insertions(+), 16 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/9da6a673/sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java ---------------------------------------------------------------------- diff --git a/sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java b/sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java index 08807ca..2415890 100644 --- a/sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java +++ b/sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationInfo.java @@ -17,6 +17,7 @@ */ package org.apache.sentry.hdfs; +import java.util.ArrayList; import java.util.Collections; import java.util.List; import java.util.concurrent.Executors; @@ -37,6 +38,7 @@ import org.slf4j.LoggerFactory; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.collect.Lists; public class SentryAuthorizationInfo implements Runnable { private static Logger LOG = @@ -264,8 +266,12 @@ public class SentryAuthorizationInfo implements Runnable { lock.readLock().lock(); try { String authzObj = authzPaths.findAuthzObject(pathElements); - return (authzObj != null) ? authzPermissions.getAcls(authzObj) - : Collections.EMPTY_LIST; + // Apparently setFAcl throws error if 'group::---' is not present + AclEntry noGroup = AclEntry.parseAclEntry("group::---", true); + ArrayList<AclEntry> retList = Lists.newArrayList(noGroup); + retList.addAll((authzObj != null) ? authzPermissions.getAcls(authzObj) + : Collections.EMPTY_LIST); + return retList; } finally { lock.readLock().unlock(); } http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/9da6a673/sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java ---------------------------------------------------------------------- diff --git a/sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java b/sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java index cd96cc4..001da65 100644 --- a/sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java +++ b/sentry-hdfs/sentry-hdfs-namenode-plugin/src/main/java/org/apache/sentry/hdfs/SentryAuthorizationProvider.java @@ -20,6 +20,9 @@ package org.apache.sentry.hdfs; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Set; @@ -296,10 +299,6 @@ public class SentryAuthorizationProvider builder.setPermission(fsPerm.getGroupAction()); list.add(builder.build()); builder.setName(null); - builder.setType(AclEntryType.OTHER); - builder.setScope(AclEntryScope.ACCESS); - builder.setPermission(fsPerm.getOtherAction()); - list.add(builder.build()); return list; } @@ -311,46 +310,69 @@ public class SentryAuthorizationProvider boolean isManaged = false; boolean isStale = false; boolean hasAuthzObj = false; + Map<String, AclEntry> aclMap = null; if (!authzInfo.isManaged(pathElements)) { isManaged = false; f = defaultAuthzProvider.getAclFeature(node, snapshotId); } else { isManaged = true; - List<AclEntry> list = new ArrayList<AclEntry>(); + aclMap = new HashMap<String, AclEntry>(); if (originalAuthzAsAcl) { String user = defaultAuthzProvider.getUser(node, snapshotId); String group = getDefaultProviderGroup(node, snapshotId); FsPermission perm = defaultAuthzProvider.getFsPermission(node, snapshotId); - list.addAll(createAclEntries(user, group, perm)); + addToACLMap(aclMap, createAclEntries(user, group, perm)); } else { - list.addAll(createAclEntries(this.user, this.group, this.permission)); + addToACLMap(aclMap, + createAclEntries(this.user, this.group, this.permission)); } if (!authzInfo.isStale()) { isStale = false; if (authzInfo.doesBelongToAuthzObject(pathElements)) { hasAuthzObj = true; - list.addAll(authzInfo.getAclEntries(pathElements)); - f = new SentryAclFeature(ImmutableList.copyOf(list)); + addToACLMap(aclMap, authzInfo.getAclEntries(pathElements)); + f = new SentryAclFeature(ImmutableList.copyOf(aclMap.values())); } else { hasAuthzObj = false; f = defaultAuthzProvider.getAclFeature(node, snapshotId); } } else { isStale = true; - f = new SentryAclFeature(ImmutableList.copyOf(list)); + f = new SentryAclFeature(ImmutableList.copyOf(aclMap.values())); } } if (LOG.isDebugEnabled()) { - LOG.debug("### getAclEntry [" + (p == null ? "null" : p) + "] : [" + LOG.debug("### getAclEntry \n[" + (p == null ? "null" : p) + "] : [" + "isManaged=" + isManaged + ", isStale=" + isStale + ", hasAuthzObj=" + hasAuthzObj - + ", origAuthzAsAcl=" + originalAuthzAsAcl + "]" - + "[" + (f == null ? "null" : f.getEntries()) + "]"); + + ", origAuthzAsAcl=" + originalAuthzAsAcl + "]\n" + + "[" + (aclMap == null ? "null" : aclMap) + "]\n" + + "[" + (f == null ? "null" : f.getEntries()) + "]\n"); } return f; } + private void addToACLMap(Map<String, AclEntry> map, + Collection<AclEntry> entries) { + for (AclEntry ent : entries) { + String key = (ent.getName() == null ? "" : ent.getName()) + + ent.getScope() + ent.getType(); + AclEntry aclEntry = map.get(key); + if (aclEntry == null) { + map.put(key, ent); + } else { + map.put(key, + new AclEntry.Builder(). + setName(ent.getName()). + setScope(ent.getScope()). + setType(ent.getType()). + setPermission(ent.getPermission().or(aclEntry.getPermission())). + build()); + } + } + } + private String getDefaultProviderGroup(INodeAuthorizationInfo node, int snapshotId) { String group = defaultAuthzProvider.getGroup(node, snapshotId); http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/9da6a673/sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java ---------------------------------------------------------------------- diff --git a/sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java b/sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java index b766a8f..767c8f6 100644 --- a/sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java +++ b/sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/TestSentryAuthorizationProvider.java @@ -136,7 +136,6 @@ public class TestSentryAuthorizationProvider { List<AclEntry> acls = new ArrayList<AclEntry>(); acls.add(new AclEntry.Builder().setName(sysUser).setType(AclEntryType.USER).setScope(AclEntryScope.ACCESS).setPermission(FsAction.ALL).build()); acls.add(new AclEntry.Builder().setName("supergroup").setType(AclEntryType.GROUP).setScope(AclEntryScope.ACCESS).setPermission(FsAction.READ_EXECUTE).build()); - acls.add(new AclEntry.Builder().setName(null).setType(AclEntryType.OTHER).setScope(AclEntryScope.ACCESS).setPermission(FsAction.READ_EXECUTE).build()); acls.add(new AclEntry.Builder().setName("user-authz").setType(AclEntryType.USER).setScope(AclEntryScope.ACCESS).setPermission(FsAction.ALL).build()); Assert.assertEquals(new LinkedHashSet<AclEntry>(acls), new LinkedHashSet<AclEntry>(fs.getAclStatus(path).getEntries())); http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/9da6a673/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java ---------------------------------------------------------------------- diff --git a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java index c640db5..a33cc15 100644 --- a/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java +++ b/sentry-tests/sentry-tests-hive/src/test/java/org/apache/sentry/tests/e2e/hdfs/TestHDFSIntegration.java @@ -500,6 +500,24 @@ public class TestHDFSIntegration { verifyOnPath("/user/hive/warehouse", null, "hbase", false); verifyOnAllSubDirs("/user/hive/warehouse/p1", FsAction.READ_EXECUTE, "hbase", true); + adminUgi.doAs(new PrivilegedExceptionAction<Void>() { + @Override + public Void run() throws Exception { + // Simulate hdfs dfs -setfacl -m <aclantry> <path> + AclStatus existing = + miniDFS.getFileSystem() + .getAclStatus(new Path("/user/hive/warehouse/p1")); + ArrayList<AclEntry> newEntries = + new ArrayList<AclEntry>(existing.getEntries()); + newEntries.add(AclEntry.parseAclEntry("user::---", true)); + newEntries.add(AclEntry.parseAclEntry("group:bla:rwx", true)); + newEntries.add(AclEntry.parseAclEntry("other::---", true)); + miniDFS.getFileSystem().setAcl(new Path("/user/hive/warehouse/p1"), + newEntries); + return null; + } + }); + stmt.execute("revoke select on table p1 from role p1_admin"); verifyOnAllSubDirs("/user/hive/warehouse/p1", null, "hbase", false);
