IMPALA-7835: Role and user catalog objects with the same name can cause INVALIDATE METADATA to hang
Before this patch, enabling object ownership and running INVALIDATE METADATA for the role and user catalog objects with the same principal name could cause INVALIDATE METADATA to hang because a principal name without a type is not guaranteed to be unique. This causes the catalog delta logic to assume principal catalog objects with the same name but with different types are considered the same causing a catalog version inconsistency. This patch makes the principal object key to be unique by appending the principal type information. Testing: - Added E2E authorization test - Ran all FE tests - Ran all E2E authorization tests Change-Id: I516cf72e69e142a1349950cfca91f035c1ed445f Reviewed-on: http://gerrit.cloudera.org:8080/11910 Reviewed-by: Vuk Ercegovac <vercego...@cloudera.com> Tested-by: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Project: http://git-wip-us.apache.org/repos/asf/impala/repo Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/58a469ca Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/58a469ca Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/58a469ca Branch: refs/heads/master Commit: 58a469ca24e0225ce2274af779fbbd344595c5cc Parents: a44aadd Author: Fredy Wijaya <fwij...@cloudera.com> Authored: Thu Nov 8 10:47:05 2018 -0800 Committer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Committed: Fri Nov 9 03:37:51 2018 +0000 ---------------------------------------------------------------------- be/src/catalog/catalog-util.cc | 26 +++++++++- .../java/org/apache/impala/catalog/Catalog.java | 12 ++++- .../org/apache/impala/catalog/Principal.java | 5 +- tests/authorization/test_authorization.py | 4 +- tests/authorization/test_grant_revoke.py | 52 ++++++++++++++++++-- 5 files changed, 88 insertions(+), 11 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/impala/blob/58a469ca/be/src/catalog/catalog-util.cc ---------------------------------------------------------------------- diff --git a/be/src/catalog/catalog-util.cc b/be/src/catalog/catalog-util.cc index 6344d4d..2a3f1ae 100644 --- a/be/src/catalog/catalog-util.cc +++ b/be/src/catalog/catalog-util.cc @@ -151,6 +151,8 @@ TCatalogObjectType::type TCatalogObjectTypeFromName(const string& name) { Status TCatalogObjectFromObjectName(const TCatalogObjectType::type& object_type, const string& object_name, TCatalogObject* catalog_object) { + // See Catalog::toCatalogObjectKey in Catalog.java for more information on the + // catalog object key format. switch (object_type) { case TCatalogObjectType::DATABASE: catalog_object->__set_type(object_type); @@ -202,11 +204,31 @@ Status TCatalogObjectFromObjectName(const TCatalogObjectType::type& object_type, catalog_object->__set_cache_pool(THdfsCachePool()); catalog_object->cache_pool.__set_pool_name(object_name); break; - case TCatalogObjectType::PRINCIPAL: + case TCatalogObjectType::PRINCIPAL: { + // The format is <principal name>.<principal type> + vector<string> split; + boost::split(split, object_name, [](char c) { return c == '.'; }); + if (split.size() != 2) { + stringstream error_msg; + error_msg << "Invalid principal name: " << object_name; + return Status(error_msg.str()); + } + string principal_name = split[0]; + string principal_type = split[1]; catalog_object->__set_type(object_type); catalog_object->__set_principal(TPrincipal()); - catalog_object->principal.__set_principal_name(object_name); + catalog_object->principal.__set_principal_name(principal_name); + if (principal_type == "ROLE") { + catalog_object->principal.__set_principal_type(TPrincipalType::ROLE); + } else if (principal_type == "USER") { + catalog_object->principal.__set_principal_type(TPrincipalType::USER); + } else { + stringstream error_msg; + error_msg << "Invalid principal type: " << principal_type; + return Status(error_msg.str()); + } break; + } case TCatalogObjectType::PRIVILEGE: { // The format is <privilege name>.<principal ID>.<principal type> vector<string> split; http://git-wip-us.apache.org/repos/asf/impala/blob/58a469ca/fe/src/main/java/org/apache/impala/catalog/Catalog.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/catalog/Catalog.java b/fe/src/main/java/org/apache/impala/catalog/Catalog.java index fe49d5b..4a14428 100644 --- a/fe/src/main/java/org/apache/impala/catalog/Catalog.java +++ b/fe/src/main/java/org/apache/impala/catalog/Catalog.java @@ -28,6 +28,7 @@ import org.apache.impala.catalog.MetaStoreClientPool.MetaStoreClient; import org.apache.impala.thrift.TCatalogObject; import org.apache.impala.thrift.TFunction; import org.apache.impala.thrift.TPartitionKeyValue; +import org.apache.impala.thrift.TPrincipalType; import org.apache.impala.thrift.TTable; import org.apache.impala.thrift.TTableName; import org.apache.impala.thrift.TUniqueId; @@ -545,6 +546,7 @@ public abstract class Catalog { * Returns a unique string key of a catalog object. */ public static String toCatalogObjectKey(TCatalogObject catalogObject) { + // TODO (IMPALA-7839): Refactor this method to reduce code repetition. Preconditions.checkNotNull(catalogObject); switch (catalogObject.getType()) { case DATABASE: @@ -558,8 +560,14 @@ public abstract class Catalog { return "FUNCTION:" + catalogObject.getFn().getName() + "(" + catalogObject.getFn().getSignature() + ")"; case PRINCIPAL: - return "PRINCIPAL:" + catalogObject.getPrincipal().getPrincipal_name() - .toLowerCase(); + // It is important to make the principal object key unique since it is possible + // to have the same name for both role and user. + String principalName = catalogObject.getPrincipal().getPrincipal_name(); + if (catalogObject.getPrincipal().getPrincipal_type() == TPrincipalType.ROLE) { + principalName = principalName.toLowerCase(); + } + return "PRINCIPAL:" + principalName + "." + + catalogObject.getPrincipal().getPrincipal_type().name(); case PRIVILEGE: // The combination of privilege name + principal ID + principal type is // guaranteed to be unique. http://git-wip-us.apache.org/repos/asf/impala/blob/58a469ca/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..8f65e95 100644 --- a/fe/src/main/java/org/apache/impala/catalog/Principal.java +++ b/fe/src/main/java/org/apache/impala/catalog/Principal.java @@ -159,8 +159,9 @@ public abstract class Principal extends CatalogObjectImpl { @Override public String getUniqueName() { - return this.getPrincipalType() == TPrincipalType.ROLE ? "ROLE:" : "USER:" - + getName().toLowerCase(); + String principalName = getPrincipalType() == TPrincipalType.ROLE ? + getName().toLowerCase() : getName(); + return "PRINCIPAL:" + principalName + "." + getPrincipalType().name(); } public TCatalogObject toTCatalogObject() { http://git-wip-us.apache.org/repos/asf/impala/blob/58a469ca/tests/authorization/test_authorization.py ---------------------------------------------------------------------- diff --git a/tests/authorization/test_authorization.py b/tests/authorization/test_authorization.py index a036680..e315f6d 100644 --- a/tests/authorization/test_authorization.py +++ b/tests/authorization/test_authorization.py @@ -456,7 +456,7 @@ class TestAuthorization(CustomClusterTestSuite): self.client.execute("grant select on database functional to role %s" % unique_role) for service in [self.cluster.catalogd.service, self.cluster.get_first_impalad().service]: - obj_dump = service.get_catalog_object_dump("PRINCIPAL", unique_role) + obj_dump = service.get_catalog_object_dump("PRINCIPAL", "%s.ROLE" % unique_role) assert "catalog_version" in obj_dump # Get the privilege associated with that principal ID. @@ -468,7 +468,7 @@ class TestAuthorization(CustomClusterTestSuite): assert "catalog_version" in obj_dump # Get the principal that does not exist. - obj_dump = service.get_catalog_object_dump("PRINCIPAL", "doesnotexist") + obj_dump = service.get_catalog_object_dump("PRINCIPAL", "doesnotexist.ROLE") assert "CatalogException" in obj_dump # Get the privilege that does not exist. http://git-wip-us.apache.org/repos/asf/impala/blob/58a469ca/tests/authorization/test_grant_revoke.py ---------------------------------------------------------------------- diff --git a/tests/authorization/test_grant_revoke.py b/tests/authorization/test_grant_revoke.py index f7299bf..d568859 100644 --- a/tests/authorization/test_grant_revoke.py +++ b/tests/authorization/test_grant_revoke.py @@ -28,8 +28,9 @@ from tests.common.sentry_cache_test_suite import SentryCacheTestSuite, TestObjec from tests.util.calculation_util import get_random_id from tests.verifiers.metric_verifier import MetricVerifier -SENTRY_CONFIG_FILE = getenv('IMPALA_HOME') + \ - '/fe/src/test/resources/sentry-site.xml' +SENTRY_CONFIG_DIR = "%s/%s" % (getenv("IMPALA_HOME"), "fe/src/test/resources") +SENTRY_CONFIG_FILE = "%s/sentry-site.xml" % SENTRY_CONFIG_DIR +SENTRY_CONFIG_FILE_OO = "%s/sentry-site_oo.xml" % SENTRY_CONFIG_DIR # Sentry long polling frequency to make Sentry refresh not run. SENTRY_LONG_POLLING_FREQUENCY_S = 3600 @@ -306,7 +307,7 @@ class TestGrantRevoke(SentryCacheTestSuite): 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): + def test_role_name_case_insensitive_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()]: @@ -322,3 +323,48 @@ class TestGrantRevoke(SentryCacheTestSuite): assert self.client.wait_for_finished_timeout(handle, timeout=60) finally: self.client.execute("drop role {0}".format(role_name)) + + @pytest.mark.execute_serially + @SentryCacheTestSuite.with_args( + impalad_args="--server_name=server1 --sentry_config={0} " + "--authorization_policy_provider_class=" + "org.apache.impala.service.CustomClusterResourceAuthorizationProvider" + .format(SENTRY_CONFIG_FILE_OO), + catalogd_args="--sentry_config={0} " + "--authorization_policy_provider_class=" + "org.apache.impala.service.CustomClusterResourceAuthorizationProvider" + .format(SENTRY_CONFIG_FILE_OO), + sentry_config=SENTRY_CONFIG_FILE_OO) + def test_same_name_for_role_and_user_invalidate_metadata(self, testid_checksum): + """IMPALA-7729: Tests running invalidate metadata with for the same name for both + user and role should not cause Impala query to hang.""" + db_prefix = testid_checksum + role_name = "foobar" + # Use two different clients so that the sessions will use two different user names. + foobar_impalad_client = self.create_impala_client() + FOOBAR_impalad_client = self.create_impala_client() + try: + # This will create "foobar" role catalog object. + 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)) + + # User names are case sensitive, so "foobar" and "FOOBAR" users should be treated + # as two different catalog objects. + + # This will create "foobar" user catalog object. + self.user_query(foobar_impalad_client, "create database {0}_db1" + .format(db_prefix, user="foobar")) + # This will create "FOOBAR" user catalog object. + self.user_query(FOOBAR_impalad_client, "create database {0}_db2" + .format(db_prefix, user="FOOBAR")) + + # Verify that running invalidate metadata won't hang due to having the same name + # in both user and role. + handle = self.client.execute_async("invalidate metadata") + assert self.client.wait_for_finished_timeout(handle, timeout=60) + finally: + self.client.execute("drop database {0}_db1".format(db_prefix)) + self.client.execute("drop database {0}_db2".format(db_prefix)) + self.client.execute("drop role {0}".format(role_name))