Repository: incubator-sentry Updated Branches: refs/heads/master 4b33ad929 -> 06688cee6
SENTRY-988: It's better to let SentryAuthorization setter path always fall through and update HDFS (Yongjun Zhang, Reviewed by: Sravya Tirukkovalur) Change-Id: I6f868ad1b3f72ff0e8bdc06e945b56b406dbf062 Project: http://git-wip-us.apache.org/repos/asf/incubator-sentry/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-sentry/commit/06688cee Tree: http://git-wip-us.apache.org/repos/asf/incubator-sentry/tree/06688cee Diff: http://git-wip-us.apache.org/repos/asf/incubator-sentry/diff/06688cee Branch: refs/heads/master Commit: 06688cee637a86c99c6bf9c8ec22534e1d628b80 Parents: 4b33ad9 Author: Sravya Tirukkovalur <[email protected]> Authored: Thu Dec 17 15:01:10 2015 -0800 Committer: Sravya Tirukkovalur <[email protected]> Committed: Thu Dec 17 15:01:10 2015 -0800 ---------------------------------------------------------------------- .../sentry/hdfs/SentryAuthorizationInfo.java | 17 +- .../hdfs/SentryAuthorizationProvider.java | 166 ++++++++++--------- .../sentry/hdfs/SentryAuthorizationInfoX.java | 2 +- 3 files changed, 103 insertions(+), 82 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/06688cee/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 c8d56be..def34a4 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 @@ -251,7 +251,7 @@ public class SentryAuthorizationInfo implements Runnable { return stale; } - public boolean isManaged(String[] pathElements) { + public boolean isUnderPrefix(String[] pathElements) { lock.readLock().lock(); try { return authzPaths.isUnderPrefix(pathElements); @@ -260,6 +260,11 @@ public class SentryAuthorizationInfo implements Runnable { } } + @Deprecated + public boolean isManaged(String[] pathElements) { + return isUnderPrefix(pathElements); + } + public boolean doesBelongToAuthzObject(String[] pathElements) { lock.readLock().lock(); try { @@ -269,6 +274,16 @@ public class SentryAuthorizationInfo implements Runnable { } } + public boolean isSentryManaged(final String[] pathElements) { + lock.readLock().lock(); + try { + return authzPaths.isUnderPrefix(pathElements) && + authzPaths.findAuthzObject(pathElements) != null; + } finally { + lock.readLock().unlock(); + } + } + @SuppressWarnings("unchecked") public List<AclEntry> getAclEntries(String[] pathElements) { lock.readLock().lock(); http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/06688cee/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 4d03ba3..b7e94f3 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 @@ -21,7 +21,6 @@ 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; @@ -38,6 +37,7 @@ import org.apache.hadoop.fs.permission.FsPermission; import org.apache.hadoop.hdfs.DFSConfigKeys; import org.apache.hadoop.hdfs.server.namenode.AclFeature; import org.apache.hadoop.hdfs.server.namenode.AuthorizationProvider; +import org.apache.hadoop.hdfs.server.namenode.snapshot.Snapshot; import org.apache.hadoop.security.AccessControlException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -66,6 +66,9 @@ public class SentryAuthorizationProvider private boolean originalAuthzAsAcl; private SentryAuthorizationInfo authzInfo; + private static String WARN_VISIBILITY = + " The result won't be visible when the path is managed by Sentry"; + public SentryAuthorizationProvider() { this(null); } @@ -193,80 +196,61 @@ public class SentryAuthorizationProvider return paths; } - @Override - public void setUser(INodeAuthorizationInfo node, String user) { + private boolean isSentryManaged(final String[] pathElements) { + return authzInfo.isSentryManaged(pathElements); + } + + private boolean isSentryManaged(INodeAuthorizationInfo node) { String[] pathElements = getPathElements(node); + return isSentryManaged(pathElements); + } - // For the non sentry managed paths, set the user based on - // the requests. Otherwise should be a no op. - if (!authzInfo.isManaged(pathElements) - || !authzInfo.doesBelongToAuthzObject(pathElements)) { - defaultAuthzProvider.setUser(node, user); - } else { - if (LOG.isErrorEnabled()) { - LOG.error("### setUser is a no op for the sentry managed path.\n"); - } + @Override + public void setUser(INodeAuthorizationInfo node, String user) { + // always fall through to defaultAuthZProvider, + // issue warning when the path is sentry managed + if (isSentryManaged(node)) { + LOG.warn("### setUser {} (sentry managed path) to {}, update HDFS." + + WARN_VISIBILITY, + node.getFullPathName(), user); } + defaultAuthzProvider.setUser(node, user); } @Override public String getUser(INodeAuthorizationInfo node, int snapshotId) { - String user; - String[] pathElements = getPathElements(node); - if (!authzInfo.isManaged(pathElements)) { - user = defaultAuthzProvider.getUser(node, snapshotId); - } else if (!authzInfo.doesBelongToAuthzObject(pathElements)) { - user = defaultAuthzProvider.getUser(node, snapshotId); - } else { - user = this.user; - } - return user; + return isSentryManaged(node)? + this.user : defaultAuthzProvider.getUser(node, snapshotId); } @Override public void setGroup(INodeAuthorizationInfo node, String group) { - String[] pathElements = getPathElements(node); - - // For the non sentry managed paths, set the group based on - // the requests. Otherwise should be a no op. - if (!authzInfo.isManaged(pathElements) - || !authzInfo.doesBelongToAuthzObject(pathElements)) { - defaultAuthzProvider.setGroup(node, group); - } else { - if (LOG.isErrorEnabled()) { - LOG.error("### setGroup is a no op for the sentry managed path.\n"); - } + // always fall through to defaultAuthZProvider, + // issue warning when the path is sentry managed + if (isSentryManaged(node)) { + LOG.warn("### setGroup {} (sentry managed path) to {}, update HDFS." + + WARN_VISIBILITY, + node.getFullPathName(), group); } + defaultAuthzProvider.setGroup(node, group); } @Override public String getGroup(INodeAuthorizationInfo node, int snapshotId) { - String group; - String[] pathElements = getPathElements(node); - if (!authzInfo.isManaged(pathElements)) { - group = getDefaultProviderGroup(node, snapshotId); - } else if (!authzInfo.doesBelongToAuthzObject(pathElements)) { - group = getDefaultProviderGroup(node, snapshotId); - } else { - group = this.group; - } - return group; + return isSentryManaged(node)? + this.group : defaultAuthzProvider.getGroup(node, snapshotId); } @Override public void setPermission(INodeAuthorizationInfo node, FsPermission permission) { - String[] pathElements = getPathElements(node); - - // For the non sentry managed paths, set the permission based on - // the requests. Otherwise should be a no op. - if (!authzInfo.isManaged(pathElements) - || !authzInfo.doesBelongToAuthzObject(pathElements)) { - defaultAuthzProvider.setPermission(node, permission); - } else { - if (LOG.isErrorEnabled()) { - LOG.error("### setPermission is a no op for the sentry managed path.\n"); - } + // always fall through to defaultAuthZProvider, + // issue warning when the path is sentry managed + if (isSentryManaged(node)) { + LOG.warn("### setPermission {} (sentry managed path) to {}, update HDFS." + + WARN_VISIBILITY, + node.getFullPathName(), permission.toString()); } + defaultAuthzProvider.setPermission(node, permission); } @Override @@ -274,12 +258,9 @@ public class SentryAuthorizationProvider INodeAuthorizationInfo node, int snapshotId) { FsPermission permission; String[] pathElements = getPathElements(node); - if (!authzInfo.isManaged(pathElements)) { - permission = defaultAuthzProvider.getFsPermission(node, snapshotId); - } else if (!authzInfo.doesBelongToAuthzObject(pathElements)) { + if (!isSentryManaged(pathElements)) { permission = defaultAuthzProvider.getFsPermission(node, snapshotId); - } - else { + } else { FsPermission returnPerm = this.permission; // Handle case when prefix directory is itself associated with an // authorizable object (default db directory in hive) @@ -329,18 +310,18 @@ public class SentryAuthorizationProvider AclFeature f = null; String[] pathElements = getPathElements(node); String p = Arrays.toString(pathElements); - boolean isManaged = false; + boolean isPrefixed = false; boolean isStale = false; boolean hasAuthzObj = false; Map<String, AclEntry> aclMap = null; - if (!authzInfo.isManaged(pathElements)) { - isManaged = false; + if (!authzInfo.isUnderPrefix(pathElements)) { + isPrefixed = false; f = defaultAuthzProvider.getAclFeature(node, snapshotId); } else if (!authzInfo.doesBelongToAuthzObject(pathElements)) { - isManaged = true; + isPrefixed = true; f = defaultAuthzProvider.getAclFeature(node, snapshotId); } else { - isManaged = true; + isPrefixed = true; hasAuthzObj = true; aclMap = new HashMap<String, AclEntry>(); if (originalAuthzAsAcl) { @@ -363,7 +344,7 @@ public class SentryAuthorizationProvider } if (LOG.isDebugEnabled()) { LOG.debug("### getAclEntry \n[" + (p == null ? "null" : p) + "] : [" - + "isManaged=" + isManaged + + "isPreifxed=" + isPrefixed + ", isStale=" + isStale + ", hasAuthzObj=" + hasAuthzObj + ", origAuthzAsAcl=" + originalAuthzAsAcl + "]\n" @@ -404,30 +385,55 @@ public class SentryAuthorizationProvider return group; } - @Override - public void removeAclFeature(INodeAuthorizationInfo node) { - AclFeature aclFeature = node.getAclFeature(CURRENT_STATE_ID); - String[] pathElements = getPathElements(node); - - // For non sentry managed paths, remove the ACLs based on - // the requests. Otherwise should be a no op. - if (aclFeature.getClass() != SentryAclFeature.class - && !authzInfo.isManaged(pathElements)) { + /* + * Check if the given node has ACL, remove the ACL if so. Issue a warning + * message when the node doesn't have ACL and warn is true. + * TODO: We need this to maintain backward compatibility (not throw error in + * some cases). We may remove this when we release sentry major version. + */ + private void checkAndRemoveHdfsAcl(INodeAuthorizationInfo node, + boolean warn) { + AclFeature f = defaultAuthzProvider.getAclFeature(node, + Snapshot.CURRENT_STATE_ID); + if (f != null) { defaultAuthzProvider.removeAclFeature(node); } else { - if (LOG.isErrorEnabled()) { - LOG.error("### removeAclFeature is a no op for " + - "the path under prefix.\n"); + if (warn) { + LOG.warn("### removeAclFeature is requested on {}, but it doesn't " + + "have any acl.", node); } } } @Override + public void removeAclFeature(INodeAuthorizationInfo node) { + // always fall through to defaultAuthZProvider, + // issue warning when the path is sentry managed + if (isSentryManaged(node)) { + LOG.warn("### removeAclFeature {} (sentry managed path), update HDFS." + + WARN_VISIBILITY, + node.getFullPathName()); + // For Sentry-managed paths, client code may try to remove a + // non-existing ACL, ignore the request with a warning if the ACL + // doesn't exist + checkAndRemoveHdfsAcl(node, true); + } else { + defaultAuthzProvider.removeAclFeature(node); + } + } + + @Override public void addAclFeature(INodeAuthorizationInfo node, AclFeature f) { - String[] pathElements = getPathElements(node); - if (!authzInfo.isManaged(pathElements)) { - defaultAuthzProvider.addAclFeature(node, f); + // always fall through to defaultAuthZProvider, + // issue warning when the path is sentry managed + if (isSentryManaged(node)) { + LOG.warn("### addAclFeature {} (sentry managed path) {}, update HDFS." + + WARN_VISIBILITY, + node.getFullPathName(), f.toString()); + // For Sentry-managed path, remove ACL silently before adding new ACL + checkAndRemoveHdfsAcl(node, false); } + defaultAuthzProvider.addAclFeature(node, f); } } http://git-wip-us.apache.org/repos/asf/incubator-sentry/blob/06688cee/sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/SentryAuthorizationInfoX.java ---------------------------------------------------------------------- diff --git a/sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/SentryAuthorizationInfoX.java b/sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/SentryAuthorizationInfoX.java index 0ed290d..a81f7ab 100644 --- a/sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/SentryAuthorizationInfoX.java +++ b/sentry-hdfs/sentry-hdfs-namenode-plugin/src/test/java/org/apache/sentry/hdfs/SentryAuthorizationInfoX.java @@ -67,7 +67,7 @@ public class SentryAuthorizationInfoX extends SentryAuthorizationInfo { } @Override - public boolean isManaged(String[] pathElements) { + public boolean isUnderPrefix(String[] pathElements) { return hasPrefix(MANAGED, pathElements); }
