You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by jo...@apache.org on 2018/08/18 20:38:26 UTC
[7/7] impala git commit: IMPALA-7343: Update SentryProxy to use
Sentry bulk API
IMPALA-7343: Update SentryProxy to use Sentry bulk API
Prior to this patch, every Sentry policy update required a Thrift call
to Sentry to get the role privileges per role. This is inefficient.
This patch updates the way Impala update its Sentry policy by using the
new Sentry APIs for getting all roles/users and their associated
privileges in a single Thrift call to Sentry. This patch also updates
SentryProxy to get user privileges.
Before:
- Refreshing 100 roles: 493ms
- Refreshing 1000 roles: 4668ms
- Refreshing 10000 roles: 45636ms
After:
- Refreshing 100 roles: 114ms
- Refreshing 1000 roles: 1021ms
- Refreshing 10000 roles: 10076ms
Testing:
- Ran all FE tests
- Ran all E2E authorization tests
Change-Id: I3ab215f2a5c5111bf1d25eec7fac90506d2f6304
Reviewed-on: http://gerrit.cloudera.org:8080/11250
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/0a901fcb
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/0a901fcb
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/0a901fcb
Branch: refs/heads/master
Commit: 0a901fcb275058fc4c24e243d2a829dfddfef4d5
Parents: 1d4df94
Author: Fredy Wijaya <fw...@cloudera.com>
Authored: Thu Aug 16 15:33:25 2018 -0700
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Sat Aug 18 07:31:27 2018 +0000
----------------------------------------------------------------------
bin/impala-config.sh | 2 +-
.../impala/catalog/CatalogServiceCatalog.java | 10 +
.../apache/impala/util/SentryPolicyService.java | 49 ++++-
.../org/apache/impala/util/SentryProxy.java | 207 ++++++++++++-------
4 files changed, 195 insertions(+), 73 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/0a901fcb/bin/impala-config.sh
----------------------------------------------------------------------
diff --git a/bin/impala-config.sh b/bin/impala-config.sh
index 6a7511d..656861d 100755
--- a/bin/impala-config.sh
+++ b/bin/impala-config.sh
@@ -162,7 +162,7 @@ unset IMPALA_KUDU_URL
: ${CDH_DOWNLOAD_HOST:=native-toolchain.s3.amazonaws.com}
export CDH_DOWNLOAD_HOST
export CDH_MAJOR_VERSION=6
-export CDH_BUILD_NUMBER=517354
+export CDH_BUILD_NUMBER=533940
export IMPALA_HADOOP_VERSION=3.0.0-cdh6.x-SNAPSHOT
export IMPALA_HBASE_VERSION=2.0.0-cdh6.x-SNAPSHOT
export IMPALA_HIVE_VERSION=2.1.1-cdh6.x-SNAPSHOT
http://git-wip-us.apache.org/repos/asf/impala/blob/0a901fcb/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java b/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
index 252bfe1..1fc43f5 100644
--- a/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
+++ b/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
@@ -1628,6 +1628,16 @@ public class CatalogServiceCatalog extends Catalog {
return removePrincipalPrivilege(roleName, thriftPriv, TPrincipalType.ROLE);
}
+ /**
+ * Removes a PrincipalPrivilege from the given user name. Returns the removed
+ * PrincipalPrivilege with an incremented catalog version or null if no matching
+ * privilege was found. Throws a CatalogException if no user exists with this name.
+ */
+ public PrincipalPrivilege removeUserPrivilege(String userName, TPrivilege thriftPriv)
+ throws CatalogException {
+ return removePrincipalPrivilege(userName, thriftPriv, TPrincipalType.USER);
+ }
+
private PrincipalPrivilege removePrincipalPrivilege(String principalName,
TPrivilege privilege, TPrincipalType type) throws CatalogException {
versionLock_.writeLock().lock();
http://git-wip-us.apache.org/repos/asf/impala/blob/0a901fcb/fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/util/SentryPolicyService.java b/fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
index f039ee7..5d3ee33 100644
--- a/fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
+++ b/fe/src/main/java/org/apache/impala/util/SentryPolicyService.java
@@ -18,8 +18,12 @@
package org.apache.impala.util;
import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.impala.catalog.Principal;
import org.apache.impala.catalog.PrincipalPrivilege;
+import org.apache.impala.thrift.TPrincipalType;
import org.apache.sentry.api.service.thrift.SentryPolicyServiceClient;
import org.apache.sentry.api.service.thrift.TSentryGrantOption;
import org.apache.sentry.api.service.thrift.TSentryPrivilege;
@@ -429,9 +433,50 @@ public class SentryPolicyService {
}
/**
+ * Returns a map of all roles with their associated privileges.
+ */
+ public Map<String, Set<TSentryPrivilege>> listAllRolesPrivileges(User requestingUser)
+ throws ImpalaException {
+ return listAllPrincipalsPrivileges(requestingUser, TPrincipalType.ROLE);
+ }
+
+ /**
+ * Returns a map of all users with their associated privileges.
+ */
+ public Map<String, Set<TSentryPrivilege>> listAllUsersPrivileges(User requestingUser)
+ throws ImpalaException {
+ return listAllPrincipalsPrivileges(requestingUser, TPrincipalType.USER);
+ }
+
+ private Map<String, Set<TSentryPrivilege>> listAllPrincipalsPrivileges(
+ User requestingUser, TPrincipalType type) throws ImpalaException {
+ SentryServiceClient client = new SentryServiceClient();
+ try {
+ return type == TPrincipalType.ROLE ?
+ client.get().listAllRolesPrivileges(requestingUser.getShortName()) :
+ client.get().listAllUsersPrivileges(requestingUser.getShortName());
+ } catch (Exception e) {
+ if (SentryUtil.isSentryAccessDenied(e)) {
+ throw new AuthorizationException(String.format(ACCESS_DENIED_ERROR_MSG,
+ requestingUser.getName(),
+ type == TPrincipalType.ROLE ?
+ "LIST_ALL_ROLES_PRIVILEGES" : "LIST_ALL_USERS_PRIVILEGES"));
+ }
+ throw new InternalException(String.format("Error making '%s' RPC to " +
+ "Sentry Service: ",
+ type == TPrincipalType.ROLE ?
+ "listAllRolesPrivileges" :
+ "listAllUsersPrivileges"), e);
+ } finally {
+ client.close();
+ }
+ }
+
+ /**
* Utility function that converts a TSentryPrivilege to an Impala TPrivilege object.
*/
- public static TPrivilege sentryPrivilegeToTPrivilege(TSentryPrivilege sentryPriv) {
+ public static TPrivilege sentryPrivilegeToTPrivilege(TSentryPrivilege sentryPriv,
+ Principal principal) {
TPrivilege privilege = new TPrivilege();
privilege.setServer_name(sentryPriv.getServerName());
if (sentryPriv.isSetDbName()) privilege.setDb_name(sentryPriv.getDbName());
@@ -456,6 +501,8 @@ public class SentryPolicyService {
} else {
privilege.setHas_grant_opt(false);
}
+ privilege.setPrincipal_id(principal.getId());
+ privilege.setPrincipal_type(principal.getPrincipalType());
return privilege;
}
}
http://git-wip-us.apache.org/repos/asf/impala/blob/0a901fcb/fe/src/main/java/org/apache/impala/util/SentryProxy.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/util/SentryProxy.java b/fe/src/main/java/org/apache/impala/util/SentryProxy.java
index fa769b6..4d1fb19 100644
--- a/fe/src/main/java/org/apache/impala/util/SentryProxy.java
+++ b/fe/src/main/java/org/apache/impala/util/SentryProxy.java
@@ -27,6 +27,7 @@ import java.util.concurrent.TimeUnit;
import org.apache.impala.catalog.AuthorizationException;
import org.apache.impala.catalog.CatalogException;
import org.apache.impala.catalog.CatalogServiceCatalog;
+import org.apache.impala.catalog.Principal;
import org.apache.impala.catalog.PrincipalPrivilege;
import org.apache.impala.catalog.Role;
import org.apache.impala.thrift.TPrincipalType;
@@ -117,81 +118,18 @@ public class SentryProxy {
public void run() {
synchronized (SentryProxy.this) {
- // Assume all roles should be removed. Then query the Policy Service and remove
- // roles from this set that actually exist.
- Set<String> rolesToRemove = catalog_.getAuthPolicy().getAllRoleNames();
+ Set<String> rolesToRemove;
+ Set<String> usersToRemove;
+ long startTime = System.currentTimeMillis();
try {
- // Read the full policy, adding new/modified roles to "updatedRoles".
- for (TSentryRole sentryRole:
- sentryPolicyService_.listAllRoles(processUser_)) {
- // This role exists and should not be removed, delete it from the
- // rolesToRemove set.
- rolesToRemove.remove(sentryRole.getRoleName().toLowerCase());
-
- Set<String> grantGroups = Sets.newHashSet();
- for (TSentryGroup group: sentryRole.getGroups()) {
- grantGroups.add(group.getGroupName());
- }
- Role existingRole =
- catalog_.getAuthPolicy().getRole(sentryRole.getRoleName());
- Role role;
- // These roles are the same, use the current role.
- if (existingRole != null &&
- existingRole.getGrantGroups().equals(grantGroups)) {
- role = existingRole;
- if (resetVersions_) {
- role.setCatalogVersion(catalog_.incrementAndGetCatalogVersion());
- }
- } else {
- role = catalog_.addRole(sentryRole.getRoleName(), grantGroups);
- }
- // Assume all privileges should be removed. Privileges that still exist are
- // deleted from this set and we are left with the set of privileges that need
- // to be removed.
- Set<String> privilegesToRemove = role.getPrivilegeNames();
- List<TSentryPrivilege> sentryPrivlist = Lists.newArrayList();
-
- try {
- sentryPrivlist =
- sentryPolicyService_.listRolePrivileges(processUser_, role.getName());
- } catch (ImpalaException e) {
- String roleName = role.getName() != null ? role.getName(): "null";
- LOG.error("Error listing the role name: " + roleName, e);
- }
-
- // Check all the privileges that are part of this role.
- for (TSentryPrivilege sentryPriv: sentryPrivlist) {
- TPrivilege thriftPriv =
- SentryPolicyService.sentryPrivilegeToTPrivilege(sentryPriv);
- thriftPriv.setPrincipal_id(role.getId());
- thriftPriv.setPrincipal_type(TPrincipalType.ROLE);
-
- privilegesToRemove.remove(thriftPriv.getPrivilege_name().toLowerCase());
-
- PrincipalPrivilege existingRolePriv =
- role.getPrivilege(thriftPriv.getPrivilege_name());
- // We already know about this privilege (privileges cannot be modified).
- if (existingRolePriv != null &&
- existingRolePriv.getCreateTimeMs() == sentryPriv.getCreateTime()) {
- if (resetVersions_) {
- existingRolePriv.setCatalogVersion(
- catalog_.incrementAndGetCatalogVersion());
- }
- continue;
- }
- catalog_.addRolePrivilege(role.getName(), thriftPriv);
- }
-
- // Remove the privileges that no longer exist.
- for (String privilegeName: privilegesToRemove) {
- TPrivilege privilege = new TPrivilege();
- privilege.setPrivilege_name(privilegeName);
- catalog_.removeRolePrivilege(role.getName(), privilege);
- }
- }
+ rolesToRemove = refreshRolePrivileges();
+ usersToRemove = refreshUserPrivileges();
} catch (Exception e) {
LOG.error("Error refreshing Sentry policy: ", e);
return;
+ } finally {
+ LOG.debug("Refreshing Sentry policy took " +
+ (System.currentTimeMillis() - startTime) + "ms");
}
// Remove all the roles, incrementing the catalog version to indicate
@@ -199,6 +137,133 @@ public class SentryProxy {
for (String roleName: rolesToRemove) {
catalog_.removeRole(roleName);
}
+ // Remove all the users, incrementing the catalog version to indicate
+ // a change.
+ for (String userName: usersToRemove) {
+ catalog_.removeUser(userName);
+ }
+ }
+ }
+
+ /**
+ * Updates all roles and their associated privileges in the catalog by adding,
+ * removing, and replacing the catalog objects to match those in Sentry since
+ * the last sentry sync update. This method returns a list of roles to be removed.
+ */
+ private Set<String> refreshRolePrivileges() throws ImpalaException {
+ // Assume all roles should be removed. Then query the Policy Service and remove
+ // roles from this set that actually exist.
+ Set<String> rolesToRemove = catalog_.getAuthPolicy().getAllRoleNames();
+ Map<String, Set<TSentryPrivilege>> allRolesPrivileges =
+ sentryPolicyService_.listAllRolesPrivileges(processUser_);
+ // Read the full policy, adding new/modified roles to "updatedRoles".
+ for (TSentryRole sentryRole:
+ sentryPolicyService_.listAllRoles(processUser_)) {
+ // This role exists and should not be removed, delete it from the
+ // rolesToRemove set.
+ rolesToRemove.remove(sentryRole.getRoleName().toLowerCase());
+
+ Set<String> grantGroups = Sets.newHashSet();
+ for (TSentryGroup group: sentryRole.getGroups()) {
+ grantGroups.add(group.getGroupName());
+ }
+ Role existingRole =
+ catalog_.getAuthPolicy().getRole(sentryRole.getRoleName());
+ Role role;
+ // These roles are the same, use the current role.
+ if (existingRole != null &&
+ existingRole.getGrantGroups().equals(grantGroups)) {
+ role = existingRole;
+ if (resetVersions_) {
+ role.setCatalogVersion(catalog_.incrementAndGetCatalogVersion());
+ }
+ } else {
+ role = catalog_.addRole(sentryRole.getRoleName(), grantGroups);
+ }
+ refreshPrivilegesInCatalog(role, allRolesPrivileges);
+ }
+ return rolesToRemove;
+ }
+
+ /**
+ * Updates all users and their associated privileges in the catalog by adding,
+ * removing, and replacing the catalog objects to match those in Sentry since the
+ * last Sentry sync update. Take note that we only store the users with privileges
+ * stored in Sentry and not all available users in the system. This method returns a
+ * list of users to be removed. User privileges do not support grant groups.
+ */
+ private Set<String> refreshUserPrivileges() throws ImpalaException {
+ // Assume all users should be removed. Then query the Policy Service and remove
+ // roles from this set that actually exist.
+ Set<String> usersToRemove = catalog_.getAuthPolicy().getAllUserNames();
+ Map<String, Set<TSentryPrivilege>> allUsersPrivileges =
+ sentryPolicyService_.listAllUsersPrivileges(processUser_);
+ for (Map.Entry<String, Set<TSentryPrivilege>> userPrivilegesEntry:
+ allUsersPrivileges.entrySet()) {
+ String userName = userPrivilegesEntry.getKey();
+ // This user exists and should not be removed so remove it from the
+ // usersToRemove set.
+ usersToRemove.remove(userName);
+
+ org.apache.impala.catalog.User existingUser =
+ catalog_.getAuthPolicy().getUser(userName);
+ org.apache.impala.catalog.User user;
+ if (existingUser != null) {
+ user = existingUser;
+ if (resetVersions_) {
+ user.setCatalogVersion(catalog_.incrementAndGetCatalogVersion());
+ }
+ } else {
+ user = catalog_.addUser(userName);
+ }
+ refreshPrivilegesInCatalog(user, allUsersPrivileges);
+ }
+ return usersToRemove;
+ }
+
+ /**
+ * Updates the privileges for a given principal in the catalog since the last Sentry
+ * sync update.
+ */
+ private void refreshPrivilegesInCatalog(Principal principal,
+ Map<String, Set<TSentryPrivilege>> allPrincipalPrivileges)
+ throws CatalogException {
+ // Assume all privileges should be removed. Privileges that still exist are
+ // deleted from this set and we are left with the set of privileges that need
+ // to be removed.
+ Set<String> privilegesToRemove = principal.getPrivilegeNames();
+ // Check all the privileges that are part of this principal.
+ for (TSentryPrivilege sentryPriv: allPrincipalPrivileges.get(principal.getName())) {
+ TPrivilege thriftPriv =
+ SentryPolicyService.sentryPrivilegeToTPrivilege(sentryPriv, principal);
+ privilegesToRemove.remove(thriftPriv.getPrivilege_name().toLowerCase());
+ PrincipalPrivilege existingPrincipalPriv =
+ principal.getPrivilege(thriftPriv.getPrivilege_name());
+ // We already know about this privilege (privileges cannot be modified).
+ if (existingPrincipalPriv != null &&
+ existingPrincipalPriv.getCreateTimeMs() == sentryPriv.getCreateTime()) {
+ if (resetVersions_) {
+ existingPrincipalPriv.setCatalogVersion(
+ catalog_.incrementAndGetCatalogVersion());
+ }
+ continue;
+ }
+ if (principal.getPrincipalType() == TPrincipalType.ROLE) {
+ catalog_.addRolePrivilege(principal.getName(), thriftPriv);
+ } else {
+ catalog_.addUserPrivilege(principal.getName(), thriftPriv);
+ }
+ }
+
+ // Remove the privileges that no longer exist.
+ for (String privilegeName: privilegesToRemove) {
+ TPrivilege privilege = new TPrivilege();
+ privilege.setPrivilege_name(privilegeName);
+ if (principal.getPrincipalType() == TPrincipalType.ROLE) {
+ catalog_.removeRolePrivilege(principal.getName(), privilege);
+ } else {
+ catalog_.removeUserPrivilege(principal.getName(), privilege);
+ }
}
}
}