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/10/20 06:44:11 UTC
[2/5] impala git commit: IMPALA-7721: Fix broken /catalog_object web
API when getting a privilege
IMPALA-7721: Fix broken /catalog_object web API when getting a privilege
Before this patch, /catalog_object web API was broken when getting a
privilege due to an incorrect way of getting a role ID. IMPALA-7616
broke this even more due to a lack of test coverage in /catalog_object
when authorization is enabled. This patch fixes the issue and makes the
/catalog_object web API usable again for getting a privilege.
Testing:
- Added a new BE test
- Added a new E2E test
- Ran all E2E authorization tests
Change-Id: I525149d113a1437c1e1493ad3c25a755e370b0c7
Reviewed-on: http://gerrit.cloudera.org:8080/11721
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/bad49e73
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/bad49e73
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/bad49e73
Branch: refs/heads/master
Commit: bad49e73632f64a386ad1154201f99137af864d8
Parents: 072f3ee
Author: Fredy Wijaya <fw...@cloudera.com>
Authored: Wed Oct 17 16:02:49 2018 -0700
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Sat Oct 20 00:19:18 2018 +0000
----------------------------------------------------------------------
be/src/catalog/catalog-util-test.cc | 90 ++++++++++++-
be/src/catalog/catalog-util.cc | 125 ++++++++++++++++++-
be/src/catalog/catalog-util.h | 4 +-
.../java/org/apache/impala/catalog/Catalog.java | 12 +-
.../impala/catalog/PrincipalPrivilege.java | 2 +-
tests/authorization/test_authorization.py | 40 ++++++
6 files changed, 260 insertions(+), 13 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/bad49e73/be/src/catalog/catalog-util-test.cc
----------------------------------------------------------------------
diff --git a/be/src/catalog/catalog-util-test.cc b/be/src/catalog/catalog-util-test.cc
index d37fc5c..91466f5 100644
--- a/be/src/catalog/catalog-util-test.cc
+++ b/be/src/catalog/catalog-util-test.cc
@@ -15,11 +15,14 @@
// specific language governing permissions and limitations
// under the License.
+#include <gutil/strings/substitute.h>
+
#include "catalog/catalog-util.h"
#include "testutil/gtest-util.h"
using namespace impala;
using namespace std;
+using namespace strings;
void CompressAndDecompress(const std::string& input) {
string compressed;
@@ -32,7 +35,6 @@ void CompressAndDecompress(const std::string& input) {
ASSERT_EQ(input, decompressed);
}
-
TEST(CatalogUtil, TestCatalogCompression) {
CompressAndDecompress("");
CompressAndDecompress("deadbeef");
@@ -45,5 +47,91 @@ TEST(CatalogUtil, TestCatalogCompression) {
CompressAndDecompress(large_string);
}
+TEST(CatalogUtil, TestTPrivilegeFromObjectName) {
+ vector<tuple<string, TPrivilegeLevel::type>> actions = {
+ make_tuple("all", TPrivilegeLevel::ALL),
+ make_tuple("insert", TPrivilegeLevel::INSERT),
+ make_tuple("select", TPrivilegeLevel::SELECT),
+ make_tuple("refresh", TPrivilegeLevel::REFRESH),
+ make_tuple("create", TPrivilegeLevel::CREATE),
+ make_tuple("alter", TPrivilegeLevel::ALTER),
+ make_tuple("drop", TPrivilegeLevel::DROP),
+ make_tuple("owner", TPrivilegeLevel::OWNER)
+ };
+ vector<tuple<string, bool>> grant_options = {
+ make_tuple("true", true),
+ make_tuple("false", false)
+ };
+
+ for (const auto& action: actions) {
+ for (const auto& grant_option: grant_options) {
+ TPrivilege server_privilege;
+ ASSERT_OK(TPrivilegeFromObjectName(Substitute(
+ "server=server1->action=$0->grantoption=$1",
+ get<0>(action), get<0>(grant_option)), &server_privilege));
+ ASSERT_EQ(TPrivilegeScope::SERVER, server_privilege.scope);
+ ASSERT_EQ(get<1>(action), server_privilege.privilege_level);
+ ASSERT_EQ(get<1>(grant_option), server_privilege.has_grant_opt);
+ ASSERT_EQ("server1", server_privilege.server_name);
+
+ TPrivilege uri_privilege;
+ ASSERT_OK(TPrivilegeFromObjectName(Substitute(
+ "server=server1->uri=/test-warehouse->action=$0->grantoption=$1",
+ get<0>(action), get<0>(grant_option)), &uri_privilege));
+ ASSERT_EQ(TPrivilegeScope::URI, uri_privilege.scope);
+ ASSERT_EQ(get<1>(action), uri_privilege.privilege_level);
+ ASSERT_EQ(get<1>(grant_option), uri_privilege.has_grant_opt);
+ ASSERT_EQ("server1", uri_privilege.server_name);
+ ASSERT_EQ("/test-warehouse", uri_privilege.uri);
+
+ TPrivilege db_privilege;
+ ASSERT_OK(TPrivilegeFromObjectName(Substitute(
+ "server=server1->db=functional->action=$0->grantoption=$1",
+ get<0>(action), get<0>(grant_option)), &db_privilege));
+ ASSERT_EQ(TPrivilegeScope::DATABASE, db_privilege.scope);
+ ASSERT_EQ(get<1>(action), db_privilege.privilege_level);
+ ASSERT_EQ(get<1>(grant_option), db_privilege.has_grant_opt);
+ ASSERT_EQ("server1", db_privilege.server_name);
+ ASSERT_EQ("functional", db_privilege.db_name);
+
+ TPrivilege table_privilege;
+ ASSERT_OK(TPrivilegeFromObjectName(Substitute(
+ "server=server1->db=functional->table=alltypes->action=$0->grantoption=$1",
+ get<0>(action), get<0>(grant_option)), &table_privilege));
+ ASSERT_EQ(TPrivilegeScope::TABLE, table_privilege.scope);
+ ASSERT_EQ(get<1>(action), table_privilege.privilege_level);
+ ASSERT_EQ(get<1>(grant_option), table_privilege.has_grant_opt);
+ ASSERT_EQ("server1", table_privilege.server_name);
+ ASSERT_EQ("functional", table_privilege.db_name);
+ ASSERT_EQ("alltypes", table_privilege.table_name);
+
+ TPrivilege column_privilege;
+ ASSERT_OK(TPrivilegeFromObjectName(Substitute(
+ "server=server1->db=functional->table=alltypes->column=id->action=$0->"
+ "grantoption=$1", get<0>(action), get<0>(grant_option)), &column_privilege));
+ ASSERT_EQ(TPrivilegeScope::COLUMN, column_privilege.scope);
+ ASSERT_EQ(get<1>(action), column_privilege.privilege_level);
+ ASSERT_EQ(get<1>(grant_option), column_privilege.has_grant_opt);
+ ASSERT_EQ("server1", column_privilege.server_name);
+ ASSERT_EQ("functional", column_privilege.db_name);
+ ASSERT_EQ("alltypes", column_privilege.table_name);
+ ASSERT_EQ("id", column_privilege.column_name);
+ }
+ }
+
+ TPrivilege privilege;
+ EXPECT_ERROR(TPrivilegeFromObjectName("abc=server1->action=select->grantoption=true",
+ &privilege), TErrorCode::GENERAL);
+ EXPECT_ERROR(TPrivilegeFromObjectName("server=server1->action=foo->grantoption=true",
+ &privilege), TErrorCode::GENERAL);
+ EXPECT_ERROR(TPrivilegeFromObjectName("server=server1->action=select->grantoption=foo",
+ &privilege), TErrorCode::GENERAL);
+ EXPECT_ERROR(TPrivilegeFromObjectName("", &privilege), TErrorCode::GENERAL);
+ EXPECT_ERROR(TPrivilegeFromObjectName("SERVER=server1->action=select->grantoption=true",
+ &privilege), TErrorCode::GENERAL);
+ EXPECT_ERROR(TPrivilegeFromObjectName("server;server1->action=select->grantoption=true",
+ &privilege), TErrorCode::GENERAL);
+}
+
IMPALA_TEST_MAIN();
http://git-wip-us.apache.org/repos/asf/impala/blob/bad49e73/be/src/catalog/catalog-util.cc
----------------------------------------------------------------------
diff --git a/be/src/catalog/catalog-util.cc b/be/src/catalog/catalog-util.cc
index cc7bf49..5084828 100644
--- a/be/src/catalog/catalog-util.cc
+++ b/be/src/catalog/catalog-util.cc
@@ -17,6 +17,7 @@
#include <boost/algorithm/string.hpp>
+#include <boost/algorithm/string_regex.hpp>
#include <sstream>
#include "catalog/catalog-util.h"
@@ -24,6 +25,7 @@
#include "util/compress.h"
#include "util/jni-util.h"
#include "util/debug-util.h"
+#include "util/string-parser.h"
#include "common/names.h"
@@ -36,6 +38,10 @@ jmethodID JniCatalogCacheUpdateIterator::pair_ctor;
jclass JniCatalogCacheUpdateIterator::boolean_cl;
jmethodID JniCatalogCacheUpdateIterator::boolean_ctor;
+/// Populates a TPrivilegeLevel::type based on the given object name string.
+Status TPrivilegeLevelFromObjectName(const std::string& object_name,
+ TPrivilegeLevel::type* privilege_level);
+
Status JniCatalogCacheUpdateIterator::InitJNI() {
JNIEnv* env = getJNIEnv();
if (env == nullptr) return Status("Failed to get/create JVM");
@@ -202,16 +208,40 @@ Status TCatalogObjectFromObjectName(const TCatalogObjectType::type& object_type,
catalog_object->principal.__set_principal_name(object_name);
break;
case TCatalogObjectType::PRIVILEGE: {
- int pos = object_name.find(".");
- if (pos == string::npos || pos >= object_name.size() - 1) {
+ // The format is <privilege name>.<principal ID>.<principal type>
+ vector<string> split;
+ boost::split(split, object_name, [](char c){ return c == '.'; });
+ if (split.size() != 3) {
stringstream error_msg;
error_msg << "Invalid privilege name: " << object_name;
return Status(error_msg.str());
}
+ string privilege_name = split[0];
+ string principal_id = split[1];
+ string principal_type = split[2];
catalog_object->__set_type(object_type);
- catalog_object->__set_privilege(TPrivilege());
- catalog_object->privilege.__set_principal_id(
- atoi(object_name.substr(0, pos).c_str()));
+ TPrivilege privilege;
+ Status status = TPrivilegeFromObjectName(privilege_name, &privilege);
+ if (!status.ok()) return status;
+ catalog_object->__set_privilege(privilege);
+ StringParser::ParseResult result;
+ int32_t pid = StringParser::StringToInt<int32_t>(principal_id.c_str(),
+ principal_id.length(), &result);
+ if (result != StringParser::PARSE_SUCCESS) {
+ stringstream error_msg;
+ error_msg << "Invalid principal ID: " << principal_id;
+ return Status(error_msg.str());
+ }
+ catalog_object->privilege.__set_principal_id(pid);
+ if (principal_type == "ROLE") {
+ catalog_object->privilege.__set_principal_type(TPrincipalType::ROLE);
+ } else if (principal_type == "USER") {
+ catalog_object->privilege.__set_principal_type(TPrincipalType::USER);
+ } else {
+ stringstream error_msg;
+ error_msg << "Invalid principal type: " << principal_type;
+ return Status(error_msg.str());
+ }
break;
}
case TCatalogObjectType::CATALOG:
@@ -224,6 +254,64 @@ Status TCatalogObjectFromObjectName(const TCatalogObjectType::type& object_type,
return Status::OK();
}
+Status TPrivilegeFromObjectName(const string& object_name, TPrivilege* privilege) {
+ DCHECK(privilege != nullptr);
+ // Format:
+ // server=val->action=val->grantoption=[true|false]
+ // server=val->uri=val->action=val->grantoption=[true|false]
+ // server=val->db=val->action=val->grantoption=[true|false]
+ // server=val->db=val->table=val->action=val->grantoption=[true|false]
+ // server=val->db=val->table=val->column=val->action=val->grantoption=[true|false]
+ vector<string> split;
+ boost::algorithm::split_regex(split, object_name, boost::regex("->"));
+ for (const auto& s: split) {
+ vector<string> key_value;
+ boost::split(key_value, s, [](char c){ return c == '='; });
+ if (key_value.size() != 2) {
+ stringstream error_msg;
+ error_msg << "Invalid field name/value format: " << s;
+ return Status(error_msg.str());
+ }
+
+ if (key_value[0] == "server") {
+ privilege->__set_server_name(key_value[1]);
+ privilege->__set_scope(TPrivilegeScope::SERVER);
+ } else if (key_value[0] == "uri") {
+ privilege->__set_uri(key_value[1]);
+ privilege->__set_scope(TPrivilegeScope::URI);
+ } else if (key_value[0] == "db") {
+ privilege->__set_db_name(key_value[1]);
+ privilege->__set_scope(TPrivilegeScope::DATABASE);
+ } else if (key_value[0] == "table") {
+ privilege->__set_table_name(key_value[1]);
+ privilege->__set_scope(TPrivilegeScope::TABLE);
+ } else if (key_value[0] == "column") {
+ privilege->__set_column_name(key_value[1]);
+ privilege->__set_scope(TPrivilegeScope::COLUMN);
+ } else if (key_value[0] == "action") {
+ TPrivilegeLevel::type privilege_level;
+ Status status = TPrivilegeLevelFromObjectName(key_value[1], &privilege_level);
+ if (!status.ok()) return status;
+ privilege->__set_privilege_level(privilege_level);
+ } else if (key_value[0] == "grantoption") {
+ if (key_value[1] == "true") {
+ privilege->__set_has_grant_opt(true);
+ } else if (key_value[1] == "false") {
+ privilege->__set_has_grant_opt(false);
+ } else {
+ stringstream error_msg;
+ error_msg << "Invalid grant option: " << key_value[1];
+ return Status(error_msg.str());
+ }
+ } else {
+ stringstream error_msg;
+ error_msg << "Invalid privilege field name: " << key_value[0];
+ return Status(error_msg.str());
+ }
+ }
+ return Status::OK();
+}
+
Status CompressCatalogObject(const uint8_t* src, uint32_t size, string* dst) {
scoped_ptr<Codec> compressor;
RETURN_IF_ERROR(Codec::CreateCompressor(nullptr, false, THdfsCompression::LZ4,
@@ -252,4 +340,31 @@ Status DecompressCatalogObject(const uint8_t* src, uint32_t size, string* dst) {
return Status::OK();
}
+Status TPrivilegeLevelFromObjectName(const std::string& object_name,
+ TPrivilegeLevel::type* privilege_level) {
+ DCHECK(privilege_level != nullptr);
+ if (object_name == "all") {
+ *privilege_level = TPrivilegeLevel::ALL;
+ } else if (object_name == "insert") {
+ *privilege_level = TPrivilegeLevel::INSERT;
+ } else if (object_name == "select") {
+ *privilege_level = TPrivilegeLevel::SELECT;
+ } else if (object_name == "refresh") {
+ *privilege_level = TPrivilegeLevel::REFRESH;
+ } else if (object_name == "create") {
+ *privilege_level = TPrivilegeLevel::CREATE;
+ } else if (object_name == "alter") {
+ *privilege_level = TPrivilegeLevel::ALTER;
+ } else if (object_name == "drop") {
+ *privilege_level = TPrivilegeLevel::DROP;
+ } else if (object_name == "owner") {
+ *privilege_level = TPrivilegeLevel::OWNER;
+ } else {
+ stringstream error_msg;
+ error_msg << "Invalid privilege level: " << object_name;
+ return Status(error_msg.str());
+ }
+ return Status::OK();
+}
+
}
http://git-wip-us.apache.org/repos/asf/impala/blob/bad49e73/be/src/catalog/catalog-util.h
----------------------------------------------------------------------
diff --git a/be/src/catalog/catalog-util.h b/be/src/catalog/catalog-util.h
index a01e9bb..bc50408 100644
--- a/be/src/catalog/catalog-util.h
+++ b/be/src/catalog/catalog-util.h
@@ -98,6 +98,9 @@ TCatalogObjectType::type TCatalogObjectTypeFromName(const std::string& name);
Status TCatalogObjectFromObjectName(const TCatalogObjectType::type& object_type,
const std::string& object_name, TCatalogObject* catalog_object);
+/// Populates a TPrivilege based on the given object name string.
+Status TPrivilegeFromObjectName(const std::string& object_name, TPrivilege* privilege);
+
/// Compresses a serialized catalog object using LZ4 and stores it back in 'dst'. Stores
/// the size of the uncompressed catalog object in the first sizeof(uint32_t) bytes of
/// 'dst'. The compression fails if the uncompressed data size exceeds 0x7E000000 bytes.
@@ -109,7 +112,6 @@ Status CompressCatalogObject(const uint8_t* src, uint32_t size, std::string* dst
/// catalog object.
Status DecompressCatalogObject(const uint8_t* src, uint32_t size, std::string* dst)
WARN_UNUSED_RESULT;
-
}
#endif
http://git-wip-us.apache.org/repos/asf/impala/blob/bad49e73/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 879195f..fe49d5b 100644
--- a/fe/src/main/java/org/apache/impala/catalog/Catalog.java
+++ b/fe/src/main/java/org/apache/impala/catalog/Catalog.java
@@ -512,11 +512,11 @@ public abstract class Catalog {
break;
case PRIVILEGE:
Principal tmpPrincipal = authPolicy_.getPrincipal(
- objectDesc.getPrincipal().getPrincipal_id(),
- objectDesc.getPrincipal().getPrincipal_type());
+ objectDesc.getPrivilege().getPrincipal_id(),
+ objectDesc.getPrivilege().getPrincipal_type());
if (tmpPrincipal == null) {
throw new CatalogException(String.format("No %s associated with ID: %d",
- Principal.toString(objectDesc.getPrincipal().getPrincipal_type())
+ Principal.toString(objectDesc.getPrivilege().getPrincipal_type())
.toLowerCase(), objectDesc.getPrivilege().getPrincipal_id()));
}
String privilegeName = PrincipalPrivilege.buildPrivilegeName(
@@ -561,11 +561,13 @@ public abstract class Catalog {
return "PRINCIPAL:" + catalogObject.getPrincipal().getPrincipal_name()
.toLowerCase();
case PRIVILEGE:
- // The combination of privilege name + principal ID is guaranteed to be unique.
+ // The combination of privilege name + principal ID + principal type is
+ // guaranteed to be unique.
return "PRIVILEGE:" +
PrincipalPrivilege.buildPrivilegeName(catalogObject.getPrivilege())
.toLowerCase() + "." +
- Integer.toString(catalogObject.getPrivilege().getPrincipal_id());
+ Integer.toString(catalogObject.getPrivilege().getPrincipal_id()) + "." +
+ catalogObject.getPrivilege().getPrincipal_type();
case HDFS_CACHE_POOL:
return "HDFS_CACHE_POOL:" +
catalogObject.getCache_pool().getPool_name().toLowerCase();
http://git-wip-us.apache.org/repos/asf/impala/blob/bad49e73/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java b/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
index 39046f0..c375cfc 100644
--- a/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
+++ b/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
@@ -146,7 +146,7 @@ public class PrincipalPrivilege extends CatalogObjectImpl {
@Override
public String getUniqueName() {
return "PRIVILEGE:" + getName().toLowerCase() + "." + Integer.toString(
- getPrincipalId());
+ getPrincipalId()) + "." + getPrincipalType().toString();
}
public TCatalogObject toTCatalogObject() {
http://git-wip-us.apache.org/repos/asf/impala/blob/bad49e73/tests/authorization/test_authorization.py
----------------------------------------------------------------------
diff --git a/tests/authorization/test_authorization.py b/tests/authorization/test_authorization.py
index cf5b2e9..9da43d3 100644
--- a/tests/authorization/test_authorization.py
+++ b/tests/authorization/test_authorization.py
@@ -23,6 +23,9 @@ import shutil
import tempfile
import json
import grp
+import re
+import urllib
+
from time import sleep, time
from getpass import getuser
from ImpalaService import ImpalaHiveServer2Service
@@ -438,3 +441,40 @@ class TestAuthorization(CustomClusterTestSuite):
cols = row.split("\t")
return cols[0:len(cols) - 1]
assert map(columns, result.data) == expected
+
+ @pytest.mark.execute_serially
+ @CustomClusterTestSuite.with_args(
+ impalad_args="--server_name=server1 --sentry_config=%s" % SENTRY_CONFIG_FILE,
+ catalogd_args="--sentry_config=%s" % SENTRY_CONFIG_FILE,
+ impala_log_dir=tempfile.mkdtemp(prefix="test_catalog_restart_",
+ dir=os.getenv("LOG_DIR")))
+ def test_catalog_object(self, unique_role):
+ """IMPALA-7721: Tests /catalog_object web API for principal and privilege"""
+ self.role_cleanup(unique_role)
+ try:
+ self.client.execute("create role %s" % unique_role)
+ 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)
+ assert "catalog_version" in obj_dump
+
+ # Get the privilege associated with that principal ID.
+ principal_id = re.search(r"principal_id \(i32\) = (\d+)", obj_dump)
+ assert principal_id is not None
+ obj_dump = service.get_catalog_object_dump("PRIVILEGE", urllib.quote(
+ "server=server1->db=functional->action=select->grantoption=false.%s.ROLE" %
+ principal_id.group(1)))
+ assert "catalog_version" in obj_dump
+
+ # Get the principal that does not exist.
+ obj_dump = service.get_catalog_object_dump("PRINCIPAL", "doesnotexist")
+ assert "CatalogException" in obj_dump
+
+ # Get the privilege that does not exist.
+ obj_dump = service.get_catalog_object_dump("PRIVILEGE", urllib.quote(
+ "server=server1->db=doesntexist->action=select->grantoption=false.%s.ROLE" %
+ principal_id.group(1)))
+ assert "CatalogException" in obj_dump
+ finally:
+ self.role_cleanup(unique_role)