Repository: impala Updated Branches: refs/heads/master 8c93a4568 -> 7a022cf36
IMPALA-7729: Fix invalidate metadata hang when there is an upper case role name Sentry stores the role names in lower case and Impala stores the role names based on the original input role names. IMPALA-7343 introduced a new bulk API (listAllRolesPrivileges) from Sentry that returns a map of role name to a set of privileges. Since Impala preserves the case sensitivity of the role names based on the original input role names, this causes an issue when trying to retrieve a set of privileges from a role name that is stored in Impala, especially when the role names in Impala differ than the ones returned by listAllRolesPrivileges. This issue will then result in privileges with mismatch role names to never get refreshed in the Catalogd, which causes Impalad to wait indefinitely waiting for the privileges to be updated by Catalogd. The fix is to get a set of privileges using the role names returned by Sentry's listAllRoles instead of using the role names stored in Impala. Testing: - Added a new E2E test - Ran all E2E authorization tests Change-Id: I5aa6f626ad3df4e9321ed18273d045517bc099c2 Reviewed-on: http://gerrit.cloudera.org:8080/11734 Reviewed-by: Impala Public Jenkins <[email protected]> Tested-by: Impala Public Jenkins <[email protected]> Project: http://git-wip-us.apache.org/repos/asf/impala/repo Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/072f3ee9 Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/072f3ee9 Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/072f3ee9 Branch: refs/heads/master Commit: 072f3ee9045d62cceb23f1f416f3052e0024cdcd Parents: 8c93a45 Author: Fredy Wijaya <[email protected]> Authored: Thu Oct 18 22:48:07 2018 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Fri Oct 19 21:48:28 2018 +0000 ---------------------------------------------------------------------- .../org/apache/impala/catalog/Principal.java | 7 +++++- .../java/org/apache/impala/catalog/Role.java | 7 ++++++ .../java/org/apache/impala/catalog/User.java | 7 ++++++ .../org/apache/impala/util/SentryProxy.java | 25 +++++++++++++++----- tests/authorization/test_grant_revoke.py | 22 +++++++++++++++++ 5 files changed, 61 insertions(+), 7 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/impala/blob/072f3ee9/fe/src/main/java/org/apache/impala/catalog/Principal.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/catalog/Principal.java b/fe/src/main/java/org/apache/impala/catalog/Principal.java index d048d10..9f3f6d4 100644 --- a/fe/src/main/java/org/apache/impala/catalog/Principal.java +++ b/fe/src/main/java/org/apache/impala/catalog/Principal.java @@ -39,7 +39,7 @@ public abstract class Principal extends CatalogObjectImpl { private static AtomicInteger principalId_ = new AtomicInteger(0); private final CatalogObjectCache<PrincipalPrivilege> principalPrivileges_ = - new CatalogObjectCache<>(); + new CatalogObjectCache<>(isCaseInsensitiveKeys()); protected Principal(String principalName, TPrincipalType type, Set<String> grantGroups) { @@ -55,6 +55,11 @@ public abstract class Principal extends CatalogObjectImpl { } /** + * Returns true if the keys in the catalog to be treated as case insensitive. + */ + protected abstract boolean isCaseInsensitiveKeys(); + + /** * Adds a privilege to the principal. Returns true if the privilege was added * successfully or false if there was a newer version of the privilege already added * to the principal. http://git-wip-us.apache.org/repos/asf/impala/blob/072f3ee9/fe/src/main/java/org/apache/impala/catalog/Role.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/catalog/Role.java b/fe/src/main/java/org/apache/impala/catalog/Role.java index 9cee7d2..d2e841a 100644 --- a/fe/src/main/java/org/apache/impala/catalog/Role.java +++ b/fe/src/main/java/org/apache/impala/catalog/Role.java @@ -36,4 +36,11 @@ public class Role extends Principal { Preconditions.checkArgument( thriftPrincipal.getPrincipal_type() == TPrincipalType.ROLE); } + + @Override + protected boolean isCaseInsensitiveKeys() { + // If Sentry changes the role name to be case sensitive, make sure to update + // this code to return false. + return true; + } } http://git-wip-us.apache.org/repos/asf/impala/blob/072f3ee9/fe/src/main/java/org/apache/impala/catalog/User.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/catalog/User.java b/fe/src/main/java/org/apache/impala/catalog/User.java index 2845670..9bcd7fb 100644 --- a/fe/src/main/java/org/apache/impala/catalog/User.java +++ b/fe/src/main/java/org/apache/impala/catalog/User.java @@ -36,4 +36,11 @@ public class User extends Principal { Preconditions.checkArgument( thriftPrincipal.getPrincipal_type() == TPrincipalType.USER); } + + @Override + protected boolean isCaseInsensitiveKeys() { + // If Sentry changes the user name to be case sensitive, make sure to update + // this code to return false. + return true; + } } http://git-wip-us.apache.org/repos/asf/impala/blob/072f3ee9/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 f74e033..acad161 100644 --- a/fe/src/main/java/org/apache/impala/util/SentryProxy.java +++ b/fe/src/main/java/org/apache/impala/util/SentryProxy.java @@ -169,6 +169,7 @@ public class SentryProxy { // 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(); + // The keys (role names) in listAllRolesPrivileges here are always in lower case. Map<String, Set<TSentryPrivilege>> allRolesPrivileges = sentryPolicyService_.listAllRolesPrivileges(processUser_); // Read the full policy, adding new/modified roles to "updatedRoles". @@ -195,7 +196,9 @@ public class SentryProxy { } else { role = catalog_.addRole(sentryRole.getRoleName(), grantGroups); } - refreshPrivilegesInCatalog(role, allRolesPrivileges); + // allRolesPrivileges keys and sentryRole.getName() are used here since they both + // come from Sentry so they agree in case. + refreshPrivilegesInCatalog(sentryRole.getRoleName(), role, allRolesPrivileges); } return rolesToRemove; } @@ -211,6 +214,7 @@ public class SentryProxy { // Assume all users should be removed. Then query the Policy Service and remove // users from this set that actually exist. Set<String> usersToRemove = catalog_.getAuthPolicy().getAllUserNames(); + // The keys (user names) in listAllUsersPrivileges here are always in lower case. Map<String, Set<TSentryPrivilege>> allUsersPrivileges = sentryPolicyService_.listAllUsersPrivileges(processUser_); for (Map.Entry<String, Set<TSentryPrivilege>> userPrivilegesEntry: @@ -226,24 +230,33 @@ public class SentryProxy { if (existingUser.getRef() && resetVersions_) { user.setCatalogVersion(catalog_.incrementAndGetCatalogVersion()); } - refreshPrivilegesInCatalog(user, allUsersPrivileges); + // allUsersPrivileges keys and userPrivilegesEntry.getKey() are used here since + // they both come from Sentry so they agree in case. + refreshPrivilegesInCatalog(userPrivilegesEntry.getKey(), user, + allUsersPrivileges); } return usersToRemove; } /** * Updates the privileges for a given principal in the catalog since the last Sentry - * sync update. + * sync update. The sentryPrincipalName is used to match against the key in + * allPrincipalPrivileges, which both come from Sentry, so they should have the + * same case sensitivity. */ - private void refreshPrivilegesInCatalog(Principal principal, - Map<String, Set<TSentryPrivilege>> allPrincipalPrivileges) + private void refreshPrivilegesInCatalog(String sentryPrincipalName, + 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(); + // It is important to get a set of privileges using sentryPrincipalName + // and not principal.getName() because principal.getName() may return a + // principal name with a different case than the principal names stored + // in allPrincipalPrivileges. See IMPALA-7729 for more information. Set<TSentryPrivilege> sentryPrivileges = allPrincipalPrivileges.get( - principal.getName()); + sentryPrincipalName); if (sentryPrivileges == null) return; // Check all the privileges that are part of this principal. for (TSentryPrivilege sentryPriv: sentryPrivileges) { http://git-wip-us.apache.org/repos/asf/impala/blob/072f3ee9/tests/authorization/test_grant_revoke.py ---------------------------------------------------------------------- diff --git a/tests/authorization/test_grant_revoke.py b/tests/authorization/test_grant_revoke.py index 480f2c5..949887b 100644 --- a/tests/authorization/test_grant_revoke.py +++ b/tests/authorization/test_grant_revoke.py @@ -369,3 +369,25 @@ class TestGrantRevoke(SentryCacheTestSuite): assert privileges_before.data == privileges_after.data finally: self.client.execute("drop role {0}".format(role_name)) + + @pytest.mark.execute_serially + @SentryCacheTestSuite.with_args( + impalad_args="--server_name=server1", + catalogd_args="--sentry_config={0}".format(SENTRY_CONFIG_FILE), + sentry_config=SENTRY_CONFIG_FILE) + def test_invalidate_metadata(self, unique_role): + """IMPALA-7729: Tests running invalidate metadata with role names that have different + case sensitivity.""" + for role_name in [unique_role.lower(), unique_role.upper(), unique_role.capitalize()]: + try: + self.client.execute("create role {0}".format(role_name)) + self.client.execute("grant all on server to {0}".format(role_name)) + self.client.execute("grant role {0} to group `{1}`".format( + role_name, grp.getgrnam(getuser()).gr_name)) + + # Verify that running invalidate metadata won't hang due to case sensitivity + # in the role names. + handle = self.client.execute_async("invalidate metadata") + assert self.client.wait_for_finished_timeout(handle, timeout=60) + finally: + self.client.execute("drop role {0}".format(role_name))
