You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by bo...@apache.org on 2018/11/19 10:45:38 UTC
[31/33] impala git commit: IMPALA-7835: Role and user catalog objects
with the same name can cause INVALIDATE METADATA to hang
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 <ve...@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/87c2ad05
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/87c2ad05
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/87c2ad05
Branch: refs/heads/branch-3.1.0
Commit: 87c2ad0531d66ac7c253f340301b5782f3afe7fd
Parents: 4a10096
Author: Fredy Wijaya <fw...@cloudera.com>
Authored: Thu Nov 8 10:47:05 2018 -0800
Committer: Zoltan Borok-Nagy <bo...@cloudera.com>
Committed: Tue Nov 13 12:52:36 2018 +0100
----------------------------------------------------------------------
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/87c2ad05/be/src/catalog/catalog-util.cc
----------------------------------------------------------------------
diff --git a/be/src/catalog/catalog-util.cc b/be/src/catalog/catalog-util.cc
index 5084828..8cd2e2c 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/87c2ad05/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/87c2ad05/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/87c2ad05/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/87c2ad05/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))