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 2020/02/13 22:21:18 UTC
[impala] branch master updated (14ae6ea -> d96dab2)
This is an automated email from the ASF dual-hosted git repository.
joemcdonnell pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git.
from 14ae6ea IMPALA-9279: Update the Kudu version to include VARCHAR support
new e7d10df IMPALA-9242: Filter privileges before returning them to Sentry
new aca2215 IMPALA-9340: fix bug where max missed heartbeats is off by one
new 55612f9 IMPALA-9287: Add support for embedded HMS in CDP builds
new 8ddbc18 IMPALA-7002: Throw AuthorizationException when user accesses non-existent table/database in CTE without required privileges.
new d96dab2 IMPALA-9182: Print the socket address of the client closing a session or cancelling a query from the WebUI
The 5 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
be/src/kudu/util/web_callback_registry.h | 3 +
be/src/service/impala-http-handler.cc | 6 +-
be/src/statestore/failure-detector.cc | 4 +-
be/src/util/webserver.cc | 1 +
bin/bootstrap_system.sh | 6 +
fe/pom.xml | 29 ++-
.../org/apache/impala/analysis/WithClause.java | 39 ++--
.../sentry/SentryAuthorizationPolicy.java | 85 ++++++-
.../java/org/apache/impala/catalog/Principal.java | 51 ++++-
.../apache/impala/catalog/PrincipalPrivilege.java | 4 +-
.../impala/catalog/PrincipalPrivilegeTree.java | 248 +++++++++++++++++++++
.../authorization/AuthorizationStmtTest.java | 18 ++
.../impala/catalog/PrincipalPrivilegeTreeTest.java | 134 +++++++++++
fe/src/test/resources/hive-site.xml.py | 4 +
tests/common/impala_test_suite.py | 67 +++---
.../test_kudu_table_create_without_hms.py | 1 -
tests/observability/test_log_fragments.py | 2 -
tests/webserver/test_web_pages.py | 28 ++-
18 files changed, 669 insertions(+), 61 deletions(-)
create mode 100644 fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
create mode 100644 fe/src/test/java/org/apache/impala/catalog/PrincipalPrivilegeTreeTest.java
[impala] 03/05: IMPALA-9287: Add support for embedded HMS in CDP
builds
Posted by jo...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
joemcdonnell pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit 55612f99910414b01627c537203737d8f24971bf
Author: skyyws <sk...@163.com>
AuthorDate: Thu Jan 16 17:53:56 2020 +0800
IMPALA-9287: Add support for embedded HMS in CDP builds
In some situations, an embedded HMS is enough for catalogd server.
And we've already implemented this in IMPALA-8974. But after
setting USE_CDP_HIVE=true and rebuilt impala, the custom cluster
test case test_kudu_table_create_without_hms would failed due to
lacking of related jars. The solution is to add related maven
dependency in $IMPALA_HOME/fe/pom.xml and
$IMPALA_HOME/shaded-deps/pom.xml.
Tests:
* Ran test_kudu_table_create_without_hms.py by setting
USE_CDP_HIVE=true locally
Change-Id: Ibc7d7e30cd560d43bb707dec54f4494355809f66
Reviewed-on: http://gerrit.cloudera.org:8080/15057
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
bin/bootstrap_system.sh | 6 +++++
fe/pom.xml | 29 +++++++++++++++++++---
fe/src/test/resources/hive-site.xml.py | 4 +++
.../test_kudu_table_create_without_hms.py | 1 -
4 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/bin/bootstrap_system.sh b/bin/bootstrap_system.sh
index c6ef1d1..fca2ad6 100755
--- a/bin/bootstrap_system.sh
+++ b/bin/bootstrap_system.sh
@@ -312,6 +312,12 @@ redhat sudo sed -ri 's/local +all +all +ident/local all all trust/g' \
# Accept md5 passwords from localhost
redhat sudo sed -i -e 's,\(host.*\)ident,\1md5,' /var/lib/pgsql/data/pg_hba.conf
+# Modfiy pg max connections to 500 for IMPALA-9287
+ubuntu sudo sed -i 's/\(max_connections = \)\S*/\1500/g' \
+ /etc/postgresql/*/main/postgresql.conf
+redhat sudo sed -i 's/\(max_connections = \)\S*/\1500/g' \
+ /var/lib/pgsql/data/postgresql.conf
+
ubuntu sudo service postgresql start
redhat6 sudo service postgresql start
redhat7 notindocker sudo service postgresql start
diff --git a/fe/pom.xml b/fe/pom.xml
index f861096..da1fdb8 100644
--- a/fe/pom.xml
+++ b/fe/pom.xml
@@ -1226,13 +1226,12 @@ under the License.
</exclusion>
</exclusions>
</dependency>
- <!-- Needed by tests like TestCaseLoader instantiate HMS in embedded mode which
- needs datanucleus as test dependency-->
+ <!-- IMPALA-9287: Needed when catalogd used embedded HMS-->
<dependency>
<groupId>org.datanucleus</groupId>
<artifactId>javax.jdo</artifactId>
<version>3.2.0-m3</version>
- <scope>test</scope>
+ <scope>runtime</scope>
</dependency>
<!-- IMPALA-8766: Include Knox jars on the classpath -->
<dependency>
@@ -1247,6 +1246,30 @@ under the License.
</exclusion>
</exclusions>
</dependency>
+
+ <!-- IMPALA-9287: Needed when catalogd used embedded HMS-->
+ <dependency>
+ <groupId>org.apache.hive</groupId>
+ <artifactId>hive-metastore</artifactId>
+ <version>${hive.version}</version>
+ <scope>runtime</scope>
+ <exclusions>
+ <!-- Impala uses log4j v1; avoid pulling in slf4j handling for log4j2 -->
+ <exclusion>
+ <groupId>org.apache.logging.log4j</groupId>
+ <artifactId>log4j-slf4j-impl</artifactId>
+ </exclusion>
+ <!-- https://issues.apache.org/jira/browse/HADOOP-14903 -->
+ <exclusion>
+ <groupId>net.minidev</groupId>
+ <artifactId>json-smart</artifactId>
+ </exclusion>
+ <exclusion>
+ <groupId>org.apache.hadoop</groupId>
+ <artifactId>hadoop-yarn-server-resourcemanager</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
</dependencies>
</profile>
diff --git a/fe/src/test/resources/hive-site.xml.py b/fe/src/test/resources/hive-site.xml.py
index 1f891fe..eb68401 100644
--- a/fe/src/test/resources/hive-site.xml.py
+++ b/fe/src/test/resources/hive-site.xml.py
@@ -155,6 +155,10 @@ CONFIG.update({
if variant == 'without_hms_config':
CONFIG.clear()
+ if hive_major_version >= 3:
+ CONFIG.update({
+ 'metastore.expression.proxy': 'org.apache.hadoop.hive.metastore.DefaultPartitionExpressionProxy'
+ })
# Database and JDO-related configs:
db_type = os.environ.get('HMS_DB_TYPE', 'postgres')
diff --git a/tests/custom_cluster/test_kudu_table_create_without_hms.py b/tests/custom_cluster/test_kudu_table_create_without_hms.py
index cfe957e..acbd3b1 100644
--- a/tests/custom_cluster/test_kudu_table_create_without_hms.py
+++ b/tests/custom_cluster/test_kudu_table_create_without_hms.py
@@ -29,7 +29,6 @@ TBL_NAME = "test_kudu_table_create_without_hms"
class TestCreatingKuduTableWithoutHMS(CustomClusterTestSuite):
"""Test creating kudu managed table without hms"""
- @SkipIfHive3.without_hms_not_supported
@pytest.mark.execute_serially
@CustomClusterTestSuite.with_args(hive_conf_dir=HIVE_SITE_WITHOUT_HMS_DIR)
def test_kudu_table_create_without_hms(self, unique_database):
[impala] 02/05: IMPALA-9340: fix bug where max missed heartbeats is
off by one
Posted by jo...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
joemcdonnell pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit aca2215c358cbbdc5d460200bc4f78c793d56ffc
Author: Riza Suminto <ri...@cloudera.com>
AuthorDate: Mon Feb 10 17:14:57 2020 -0800
IMPALA-9340: fix bug where max missed heartbeats is off by one
Max missed heartbeats is off by one due to greater than sign ('>')
used in comparison against statestore_max_missed_heartbeats flag in
failure-detector.cc. This commit change the sign to greater than or
equal ('>=').
Testing:
* Manual test by running impala mini cluster, kill one of impalad, and
verify in statestored log that the killed impalad is declared as
failed exactly at statestore_max_missed_heartbeats
* Run and pass core test
Change-Id: I19f6bfa7e08d231896665d85299302a17959fb6f
Reviewed-on: http://gerrit.cloudera.org:8080/15201
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
be/src/statestore/failure-detector.cc | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/be/src/statestore/failure-detector.cc b/be/src/statestore/failure-detector.cc
index 8043bed..0db5a52 100644
--- a/be/src/statestore/failure-detector.cc
+++ b/be/src/statestore/failure-detector.cc
@@ -105,9 +105,9 @@ FailureDetector::PeerState MissedHeartbeatFailureDetector::GetPeerState(
FailureDetector::PeerState MissedHeartbeatFailureDetector::ComputePeerState(
int32_t missed_heatbeat_count) {
- if (missed_heatbeat_count > max_missed_heartbeats_) {
+ if (missed_heatbeat_count >= max_missed_heartbeats_) {
return FAILED;
- } else if (missed_heatbeat_count > suspect_missed_heartbeats_) {
+ } else if (missed_heatbeat_count >= suspect_missed_heartbeats_) {
return SUSPECTED;
}
return OK;
[impala] 05/05: IMPALA-9182: Print the socket address of the client
closing a session or cancelling a query from the WebUI
Posted by jo...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
joemcdonnell pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit d96dab20dd663dfdcdcd2771d2f210cb02ecd710
Author: Vincent Tran <vt...@cloudera.com>
AuthorDate: Mon Feb 10 08:57:46 2020 -0800
IMPALA-9182: Print the socket address of the client closing a session or cancelling a query from the WebUI
This change appends the socket address (HOST:PORT) of the client
who made the request to close a session or cancel a query from
the coordinator's debug WebUI.
Existing statuses:
"Cancelled from Impala's debug web interface"
"Session closed from Impala's debug web interface"
New statuses:
"Cancelled from Impala's debug web interface by client at
<host>:<port>"
"Session closed from Impala's debug web interface by client
at <host>:<port>"
Testing:
-Verified visually that the status message is printed in the impalad
log with the socket address when one cancels a query or closes a session.
-Added a new e2e test to verify that the new status gets printed in
runtime profile and coordinator log when a query is cancelled in this
way.
-Made log asserts more robust by adding a timeout/wait value.
Change-Id: Icf74ad06ce1c40fab4ce37de6b7ca78e3e520b43
Reviewed-on: http://gerrit.cloudera.org:8080/14782
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
be/src/kudu/util/web_callback_registry.h | 3 ++
be/src/service/impala-http-handler.cc | 6 ++-
be/src/util/webserver.cc | 1 +
tests/common/impala_test_suite.py | 67 +++++++++++++++++++------------
tests/observability/test_log_fragments.py | 2 -
tests/webserver/test_web_pages.py | 28 ++++++++++++-
6 files changed, 77 insertions(+), 30 deletions(-)
diff --git a/be/src/kudu/util/web_callback_registry.h b/be/src/kudu/util/web_callback_registry.h
index 3b7ff13..d2e4ecc 100644
--- a/be/src/kudu/util/web_callback_registry.h
+++ b/be/src/kudu/util/web_callback_registry.h
@@ -65,6 +65,9 @@ class WebCallbackRegistry {
// In the case of a POST, the posted data.
std::string post_data;
+
+ // The socket address of the requester, <host>:<port>.
+ std::string source_socket;
};
// A response to an HTTP request whose body is rendered by template.
diff --git a/be/src/service/impala-http-handler.cc b/be/src/service/impala-http-handler.cc
index 865a73b..48d94ce 100644
--- a/be/src/service/impala-http-handler.cc
+++ b/be/src/service/impala-http-handler.cc
@@ -212,7 +212,8 @@ void ImpalaHttpHandler::CancelQueryHandler(const Webserver::WebRequest& req,
document->AddMember("error", error, document->GetAllocator());
return;
}
- Status cause("Cancelled from Impala's debug web interface");
+ Status cause(Substitute("Cancelled from Impala's debug web interface by client at $0"
+ , req.source_socket));
// Web UI doesn't have access to secret so we can't validate it. We assume that
// web UI is allowed to close queries.
status = server_->UnregisterQuery(unique_id, true, &cause);
@@ -234,7 +235,8 @@ void ImpalaHttpHandler::CloseSessionHandler(const Webserver::WebRequest& req,
document->AddMember("error", error, document->GetAllocator());
return;
}
- Status cause("Session closed from Impala's debug web interface");
+ Status cause(Substitute("Session closed from Impala's debug web interface by client at"
+ " $0", req.source_socket));
// Web UI doesn't have access to secret so we can't validate it. We assume that
// web UI is allowed to close sessions.
status = server_->CloseSessionInternal(unique_id,
diff --git a/be/src/util/webserver.cc b/be/src/util/webserver.cc
index 65f7555..c3dc7b7 100644
--- a/be/src/util/webserver.cc
+++ b/be/src/util/webserver.cc
@@ -580,6 +580,7 @@ sq_callback_result_t Webserver::BeginRequestCallback(struct sq_connection* conne
}
WebRequest req;
+ req.source_socket = GetRemoteAddress(request_info).ToString();
if (request_info->query_string != nullptr) {
req.query_string = request_info->query_string;
BuildArgumentMap(request_info->query_string, &req.parsed_args);
diff --git a/tests/common/impala_test_suite.py b/tests/common/impala_test_suite.py
index 5127dcc..0ac21b4 100644
--- a/tests/common/impala_test_suite.py
+++ b/tests/common/impala_test_suite.py
@@ -1147,44 +1147,61 @@ class ImpalaTestSuite(BaseTestSuite):
"Check failed to return True after {0} tries and {1} seconds{2}".format(
count, timeout_s, error_msg_str))
- def assert_impalad_log_contains(self, level, line_regex, expected_count=1):
+ def assert_impalad_log_contains(self, level, line_regex, expected_count=1, timeout_s=6):
"""
Convenience wrapper around assert_log_contains for impalad logs.
"""
- self.assert_log_contains("impalad", level, line_regex, expected_count)
+ self.assert_log_contains("impalad", level, line_regex, expected_count, timeout_s)
- def assert_catalogd_log_contains(self, level, line_regex, expected_count=1):
+ def assert_catalogd_log_contains(self, level, line_regex, expected_count=1,
+ timeout_s=6):
"""
Convenience wrapper around assert_log_contains for catalogd logs.
"""
- self.assert_log_contains("catalogd", level, line_regex, expected_count)
+ self.assert_log_contains("catalogd", level, line_regex, expected_count, timeout_s)
- def assert_log_contains(self, daemon, level, line_regex, expected_count=1):
+ def assert_log_contains(self, daemon, level, line_regex, expected_count=1, timeout_s=6):
"""
Assert that the daemon log with specified level (e.g. ERROR, WARNING, INFO) contains
expected_count lines with a substring matching the regex. When expected_count is -1,
at least one match is expected.
+ Retries until 'timeout_s' has expired. The default timeout is the default minicluster
+ log buffering time (5 seconds) with a one second buffer.
When using this method to check log files of running processes, the caller should
make sure that log buffering has been disabled, for example by adding
- '-logbuflevel=-1' to the daemon startup options.
+ '-logbuflevel=-1' to the daemon startup options or set timeout_s to a value higher
+ than the log flush interval.
"""
pattern = re.compile(line_regex)
- found = 0
- if hasattr(self, "impala_log_dir"):
- log_dir = self.impala_log_dir
- else:
- log_dir = EE_TEST_LOGS_DIR
- log_file_path = os.path.join(log_dir, daemon + "." + level)
- # Resolve symlinks to make finding the file easier.
- log_file_path = os.path.realpath(log_file_path)
- with open(log_file_path) as log_file:
- for line in log_file:
- if pattern.search(line):
- found += 1
- if expected_count == -1:
- assert found > 0, "Expected at least one line in file %s matching regex '%s'"\
- ", but found none." % (log_file_path, line_regex)
- else:
- assert found == expected_count, "Expected %d lines in file %s matching regex '%s'"\
- ", but found %d lines. Last line was: \n%s" %\
- (expected_count, log_file_path, line_regex, found, line)
+ start_time = time.time()
+ while True:
+ try:
+ found = 0
+ if hasattr(self, "impala_log_dir"):
+ log_dir = self.impala_log_dir
+ else:
+ log_dir = EE_TEST_LOGS_DIR
+ log_file_path = os.path.join(log_dir, daemon + "." + level)
+ # Resolve symlinks to make finding the file easier.
+ log_file_path = os.path.realpath(log_file_path)
+ with open(log_file_path) as log_file:
+ for line in log_file:
+ if pattern.search(line):
+ found += 1
+ if expected_count == -1:
+ assert found > 0, "Expected at least one line in file %s matching regex '%s'"\
+ ", but found none." % (log_file_path, line_regex)
+ else:
+ assert found == expected_count, \
+ "Expected %d lines in file %s matching regex '%s', but found %d lines. "\
+ "Last line was: \n%s" %\
+ (expected_count, log_file_path, line_regex, found, line)
+ return
+ except AssertionError as e:
+ # Re-throw the exception to the caller only when the timeout is expired. Otherwise
+ # sleep before retrying.
+ if time.time() - start_time > timeout_s:
+ raise
+ LOG.info("Expected log lines could not be found, sleeping before retrying: %s",
+ str(e))
+ time.sleep(1)
diff --git a/tests/observability/test_log_fragments.py b/tests/observability/test_log_fragments.py
index 81fbf53..f81d79e 100644
--- a/tests/observability/test_log_fragments.py
+++ b/tests/observability/test_log_fragments.py
@@ -40,8 +40,6 @@ class TestLogFragments(ImpalaTestSuite):
query_id = re.search("id=([0-9a-f]+:[0-9a-f]+)",
result.runtime_profile).groups()[0]
self.execute_query("select 1")
- # Logging may be buffered, so sleep to wait out the buffering.
- time.sleep(6)
self.assert_impalad_log_contains('INFO', query_id +
"] Analysis and authorization finished.")
assert query_id.endswith("000")
diff --git a/tests/webserver/test_web_pages.py b/tests/webserver/test_web_pages.py
index 523ff12..e6cbfa6 100644
--- a/tests/webserver/test_web_pages.py
+++ b/tests/webserver/test_web_pages.py
@@ -17,7 +17,7 @@
from tests.common.environ import ImpalaTestClusterFlagsDetector
from tests.common.file_utils import grep_dir
-from tests.common.skip import SkipIfBuildType
+from tests.common.skip import SkipIfBuildType, SkipIfDockerizedCluster
from tests.common.impala_cluster import ImpalaCluster
from tests.common.impala_test_suite import ImpalaTestSuite
import itertools
@@ -762,3 +762,29 @@ class TestWebPage(ImpalaTestSuite):
self.get_and_check_status(self.ROOT_URL,
"href='http://.*:%s/'" % self.IMPALAD_TEST_PORT[0], self.IMPALAD_TEST_PORT,
regex=True, headers={'X-Forwarded-Context': '/gateway'})
+
+ @SkipIfDockerizedCluster.daemon_logs_not_exposed
+ def test_display_src_socket_in_query_cause(self):
+ # Execute a long running query then cancel it from the WebUI.
+ # Check the runtime profile and the INFO logs for the cause message.
+ query = "select sleep(10000)"
+ query_id = self.execute_query_async(query).get_handle().id
+ cancel_query_url = "{0}cancel_query?query_id={1}".format(self.ROOT_URL.format
+ ("25000"), query_id)
+ text_profile_url = "{0}query_profile_plain_text?query_id={1}".format(self.ROOT_URL
+ .format("25000"), query_id)
+ requests.get(cancel_query_url)
+ response = requests.get(text_profile_url)
+ cancel_status = "Cancelled from Impala's debug web interface by client at"
+ assert cancel_status in response.text
+ self.assert_impalad_log_contains("INFO", "Cancelled from Impala\'s debug web "
+ "interface by client at", expected_count=-1)
+ # Session closing from the WebUI does not produce the cause message in the profile,
+ # so we will skip checking the runtime profile.
+ results = self.execute_query("select current_session()")
+ session_id = results.data[0]
+ close_session_url = "{0}close_session?session_id={1}".format(self.ROOT_URL.format
+ ("25000"), session_id)
+ requests.get(close_session_url)
+ self.assert_impalad_log_contains("INFO", "Session closed from Impala\'s debug "
+ "web interface by client at", expected_count=-1)
[impala] 04/05: IMPALA-7002: Throw AuthorizationException when user
accesses non-existent table/database in CTE without required privileges.
Posted by jo...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
joemcdonnell pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit 8ddbc1807a641587a05c2f5327c498f92acb935c
Author: wzhou-code <wz...@cloudera.com>
AuthorDate: Mon Jan 27 16:35:54 2020 -0800
IMPALA-7002: Throw AuthorizationException when user accesses
non-existent table/database in CTE without required privileges.
Currently if a user without required privileges tries to access a
non-existent database or table, then impala returns an analysis
exception instead of authorization exception. This happens because
during analysis of the with clause, the authorization request does
not get registered due to analysis exception being thrown before it.
This patch makes sure that those requests get registered regardless.
Testing:
- Manual test:
- ran CTE with non-existent database/table in impala-shell
without required privilege, verified that it results in
AuthorizationException.
- ran CTE with non-existent database/table in impala-shell
with the required privilege, verified that it results
in AnalysisException.
- Added CTE test cases for non-existent database/table/column
in AuthorizationStmtTest.
- Passed all FE tests.
- Passed all core tests.
Change-Id: Ia6b657a7147a136198a9a97a679c9131ee814577
Reviewed-on: http://gerrit.cloudera.org:8080/15123
Reviewed-by: Bikramjeet Vig <bi...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
.../org/apache/impala/analysis/WithClause.java | 39 +++++++++++++---------
.../authorization/AuthorizationStmtTest.java | 18 ++++++++++
2 files changed, 41 insertions(+), 16 deletions(-)
diff --git a/fe/src/main/java/org/apache/impala/analysis/WithClause.java b/fe/src/main/java/org/apache/impala/analysis/WithClause.java
index 9459ab3..5721065 100644
--- a/fe/src/main/java/org/apache/impala/analysis/WithClause.java
+++ b/fe/src/main/java/org/apache/impala/analysis/WithClause.java
@@ -77,23 +77,30 @@ public class WithClause extends StmtNode {
Analyzer withClauseAnalyzer = Analyzer.createWithNewGlobalState(analyzer);
withClauseAnalyzer.setHasWithClause();
if (analyzer.isExplain()) withClauseAnalyzer.setIsExplain();
- for (View view: views_) {
- Analyzer viewAnalyzer = new Analyzer(withClauseAnalyzer);
- view.getQueryStmt().analyze(viewAnalyzer);
- // Register this view so that the next view can reference it.
- withClauseAnalyzer.registerLocalView(view);
- }
- // Register all local views with the analyzer.
- for (FeView localView: withClauseAnalyzer.getLocalViews().values()) {
- analyzer.registerLocalView(localView);
+ try {
+ for (View view: views_) {
+ Analyzer viewAnalyzer = new Analyzer(withClauseAnalyzer);
+ view.getQueryStmt().analyze(viewAnalyzer);
+ // Register this view so that the next view can reference it.
+ withClauseAnalyzer.registerLocalView(view);
+ }
+ // Register all local views with the analyzer.
+ for (FeView localView: withClauseAnalyzer.getLocalViews().values()) {
+ analyzer.registerLocalView(localView);
+ }
+ // Record audit events because the resolved table references won't generate any
+ // when a view is referenced.
+ analyzer.getAccessEvents().addAll(withClauseAnalyzer.getAccessEvents());
}
- // Record audit events because the resolved table references won't generate any
- // when a view is referenced.
- analyzer.getAccessEvents().addAll(withClauseAnalyzer.getAccessEvents());
-
- // Register all privilege requests made from the root analyzer.
- for (PrivilegeRequest req: withClauseAnalyzer.getPrivilegeReqs()) {
- analyzer.registerPrivReq(req);
+ finally {
+ // Register all privilege requests made from the root analyzer to the input
+ // analyzer so that caller could do authorization for all the requests collected
+ // during analysis and report an authorization error over an analysis error.
+ // We should not accidentally reveal the non-existence of a database/table if
+ // the user is not authorized.
+ for (PrivilegeRequest req : withClauseAnalyzer.getPrivilegeReqs()) {
+ analyzer.registerPrivReq(req);
+ }
}
}
diff --git a/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java b/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java
index de4510e..c220a89 100644
--- a/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java
+++ b/fe/src/test/java/org/apache/impala/authorization/AuthorizationStmtTest.java
@@ -594,6 +594,24 @@ public class AuthorizationStmtTest extends AuthorizationTestBase {
authorize("select 1 from functional.notbl")
.error(selectError("functional.notbl"));
+ // Select from non-existent database in CTE without required privileges
+ authorize("with t as (select id from nodb.alltypes) select * from t")
+ .error(selectError("nodb.alltypes"));
+
+ // Select from non-existent table in CTE without required privileges
+ authorize("with t as (select id from functional.notbl) select * from t")
+ .error(selectError("functional.notbl"));
+
+ // Select from non-existent column in CTE without required privileges
+ authorize("with t as (select nocol from functional.alltypes) select * from t")
+ .error(selectError("functional.alltypes"))
+ .error(selectError("functional.alltypes"), onColumn("functional", "alltypes",
+ ALLTYPES_COLUMNS, TPrivilegeLevel.SELECT));
+
+ // With clause column labels exceeding the number of columns in the query
+ authorize("with t(c1, c2) as (select id from functional.alltypes) select * from t")
+ .error(selectError("functional.alltypes"));
+
// Select with inline view.
authorize("select a.* from (select * from functional.alltypes) a")
.ok(onServer(TPrivilegeLevel.ALL))
[impala] 01/05: IMPALA-9242: Filter privileges before returning
them to Sentry
Posted by jo...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
joemcdonnell pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit e7d10df2ecaf14f244eb32224e2c8099f2f0d8cf
Author: Csaba Ringhofer <cs...@cloudera.com>
AuthorDate: Fri Jan 17 20:23:10 2020 +0100
IMPALA-9242: Filter privileges before returning them to Sentry
This change implements the new FilteredPrivilegeCache, which adds
functions for filtering privileges based on the authorizable and
for returning Privileges directly instead of their String form.
The filtering is based on server + db + table (or just server in
case of URI privileges) to filter out the bulk of unrelated privileges.
Efficient filtering is done by a new class PrincipalPrivilegeTree.
It was tempting to reuse Sentry's TreePrivilegeCache, which has a very
similar role, but it lacks a "remove" function that is needed to keep
this index in sync with the CatalogObjectCache in Principal. I am also
a bit concerned about the possible side effect of Sentry's interning
of names in privileges - we try to avoid using String.intern() on
massive amount of names in Impala.
Other Changes:
- Add the Sentry privilege name as member to PrincipalPrivileges.
Note that the name was a member of TPrivilege till IMPALA-7616.
Storing the name shouldn't consume much extra memory, as it
is already stored as the key of the PrincipalPrivilege in
CatalogObjectCache.
Testing:
- added unit tests based on Sentry / TestTreePrivilegeCache
Change-Id: Iecd4281368d1c9fe88cfe850ea725cd68895712e
Reviewed-on: http://gerrit.cloudera.org:8080/15068
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
.../sentry/SentryAuthorizationPolicy.java | 85 ++++++-
.../java/org/apache/impala/catalog/Principal.java | 51 ++++-
.../apache/impala/catalog/PrincipalPrivilege.java | 4 +-
.../impala/catalog/PrincipalPrivilegeTree.java | 248 +++++++++++++++++++++
.../impala/catalog/PrincipalPrivilegeTreeTest.java | 134 +++++++++++
5 files changed, 513 insertions(+), 9 deletions(-)
diff --git a/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java b/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
index 1adfec8..333dd0e 100644
--- a/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
+++ b/fe/src/main/java/org/apache/impala/authorization/sentry/SentryAuthorizationPolicy.java
@@ -20,15 +20,24 @@ package org.apache.impala.authorization.sentry;
import com.google.common.base.Preconditions;
import com.google.common.collect.Sets;
import org.apache.impala.authorization.AuthorizationPolicy;
+import org.apache.impala.catalog.PrincipalPrivilege;
+import org.apache.impala.catalog.PrincipalPrivilegeTree;
import org.apache.impala.catalog.Role;
import org.apache.impala.catalog.User;
import org.apache.sentry.core.common.ActiveRoleSet;
-import org.apache.sentry.provider.cache.PrivilegeCache;
+import org.apache.sentry.core.common.Authorizable;
+import org.apache.sentry.core.common.utils.SentryConstants;
+import org.apache.sentry.core.model.db.DBModelAuthorizable;
+import org.apache.sentry.policy.common.Privilege;
+import org.apache.sentry.policy.common.PrivilegeFactory;
+import org.apache.sentry.policy.engine.common.CommonPrivilegeFactory;
+import org.apache.sentry.provider.cache.FilteredPrivilegeCache;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import java.util.List;
import java.util.Set;
+import java.util.stream.Collectors;
/**
* The source data this cache is backing is read from the Sentry Policy Service.
@@ -37,15 +46,17 @@ import java.util.Set;
* TODO: Instead of calling into Sentry to perform final authorization checks, we
* should parse/validate the privileges in Impala.
*/
-public class SentryAuthorizationPolicy implements PrivilegeCache {
+public class SentryAuthorizationPolicy implements FilteredPrivilegeCache {
private static final Logger LOG = LoggerFactory.getLogger(
SentryAuthorizationPolicy.class);
private final AuthorizationPolicy authzPolicy_;
+ private final PrivilegeFactory privilegeFactory;
public SentryAuthorizationPolicy(AuthorizationPolicy authzPolicy) {
Preconditions.checkNotNull(authzPolicy);
authzPolicy_ = authzPolicy;
+ this.privilegeFactory = new CommonPrivilegeFactory();
}
/**
@@ -54,6 +65,12 @@ public class SentryAuthorizationPolicy implements PrivilegeCache {
@Override
public Set<String> listPrivileges(Set<String> groups,
ActiveRoleSet roleSet) {
+ return listPrivilegesForGroups(groups, roleSet, null);
+ }
+
+
+ private Set<String> listPrivilegesForGroups(Set<String> groups,
+ ActiveRoleSet roleSet, PrincipalPrivilegeTree.Filter filter) {
Set<String> privileges = Sets.newHashSet();
if (roleSet != ActiveRoleSet.ALL) {
throw new UnsupportedOperationException("Impala does not support role subsets.");
@@ -63,7 +80,7 @@ public class SentryAuthorizationPolicy implements PrivilegeCache {
for (String groupName: groups) {
List<Role> grantedRoles = authzPolicy_.getGrantedRoles(groupName);
for (Role role: grantedRoles) {
- privileges.addAll(role.getPrivilegeNames());
+ privileges.addAll(role.getFilteredPrivilegeNames(filter));
}
}
return privileges;
@@ -75,17 +92,75 @@ public class SentryAuthorizationPolicy implements PrivilegeCache {
@Override
public Set<String> listPrivileges(Set<String> groups, Set<String> users,
ActiveRoleSet roleSet) {
- Set<String> privileges = listPrivileges(groups, roleSet);
+ return listPrivilegesForGroupsAndUsers(groups, users, roleSet, null);
+ }
+
+ private Set<String> listPrivilegesForGroupsAndUsers(Set<String> groups,
+ Set<String> users, ActiveRoleSet roleSet, PrincipalPrivilegeTree.Filter filter) {
+ Set<String> privileges = listPrivilegesForGroups(groups, roleSet, filter);
for (String userName: users) {
User user = authzPolicy_.getUser(userName);
if (user != null) {
- privileges.addAll(user.getPrivilegeNames());
+ privileges.addAll(user.getFilteredPrivilegeNames(filter));
}
}
return privileges;
}
@Override
+ public Set<String> listPrivileges(Set<String> groups, Set<String> users,
+ ActiveRoleSet roleSet, Authorizable... authorizationHierarchy) {
+ PrincipalPrivilegeTree.Filter filter = createPrivilegeFilter(authorizationHierarchy);
+ return listPrivilegesForGroupsAndUsers(groups, users, roleSet, filter);
+ }
+
+ @Override
+ public Set<Privilege> listPrivilegeObjects(Set<String> groups, Set<String> users,
+ ActiveRoleSet roleSet, Authorizable... authorizationHierarchy) {
+ Set<String> privilegeStrings =
+ listPrivileges(groups, users, roleSet, authorizationHierarchy);
+
+ return privilegeStrings.stream()
+ .filter(priString -> priString != null)
+ .map(priString -> getPrivilegeObject(priString))
+ .collect(Collectors.toSet());
+ }
+
+ private Privilege getPrivilegeObject(String priString) {
+ return privilegeFactory.createPrivilege(priString);
+ }
+
+ private PrincipalPrivilegeTree.Filter createPrivilegeFilter(
+ Authorizable... authorizationHierarchy) {
+ PrincipalPrivilegeTree.Filter filter = new PrincipalPrivilegeTree.Filter();
+ for (Authorizable auth : authorizationHierarchy) {
+ String name = auth.getName().toLowerCase();
+ if (name.equals(SentryConstants.RESOURCE_WILDCARD_VALUE) ||
+ name.equals(SentryConstants.RESOURCE_WILDCARD_VALUE_SOME)||
+ name.equals(SentryConstants.RESOURCE_WILDCARD_VALUE_ALL)) {
+ name = null; // null will match with everything
+ }
+ if (!(auth instanceof DBModelAuthorizable)) continue;
+ DBModelAuthorizable dbAuth = (DBModelAuthorizable) auth;
+ switch (dbAuth.getAuthzType()) {
+ case Server:
+ filter.setServer(name);
+ break;
+ case Db:
+ filter.setDb(name);
+ break;
+ case Table:
+ filter.setTable(name);
+ break;
+ case URI:
+ filter.setIsUri(true);
+ // Do not do anything for Column and View
+ }
+ }
+ return filter;
+ }
+
+ @Override
public void close() {
// Nothing to do, but required by PrivilegeCache.
}
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 39aeef1..6d03e9d 100644
--- a/fe/src/main/java/org/apache/impala/catalog/Principal.java
+++ b/fe/src/main/java/org/apache/impala/catalog/Principal.java
@@ -22,6 +22,7 @@ import java.util.HashSet;
import java.util.List;
import java.util.Set;
import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
import org.apache.impala.thrift.TCatalogObject;
import org.apache.impala.thrift.TCatalogObjectType;
@@ -43,6 +44,17 @@ public abstract class Principal extends CatalogObjectImpl {
private final CatalogObjectCache<PrincipalPrivilege> principalPrivileges_ =
new CatalogObjectCache<>(false);
+ // An index that allows efficient filtering of Privileges that are relevant to an
+ // access check. Should contain exactly the same privileges as principalPrivileges_.
+ // Does not support catalog version logic / removal of privileges by privilegeName,
+ // so principalPrivileges_ is still needed.
+ private final PrincipalPrivilegeTree privilegeTree_ = new PrincipalPrivilegeTree();
+
+ // Protects privilegeTree_ and its coherence with principalPrivileges_.
+ // Needs to be taken when accessing privilegeTree_ or when writing principalPrivileges_,
+ // but not when reading principalPrivileges_.
+ private final ReentrantReadWriteLock rwLock_ = new ReentrantReadWriteLock(true);
+
protected Principal(String principalName, TPrincipalType type,
Set<String> grantGroups) {
principal_ = new TPrincipal();
@@ -62,7 +74,14 @@ public abstract class Principal extends CatalogObjectImpl {
* to the principal.
*/
public boolean addPrivilege(PrincipalPrivilege privilege) {
- return principalPrivileges_.add(privilege);
+ try {
+ rwLock_.writeLock().lock();
+ if (!principalPrivileges_.add(privilege)) return false;
+ privilegeTree_.add(privilege);
+ } finally {
+ rwLock_.writeLock().unlock();
+ }
+ return true;
}
/**
@@ -74,7 +93,7 @@ public abstract class Principal extends CatalogObjectImpl {
}
/**
- * Returns all privilege names for this principal, or an empty set of no privileges are
+ * Returns all privilege names for this principal, or an empty set if no privileges are
* granted to the principal.
*/
public Set<String> getPrivilegeNames() {
@@ -82,6 +101,25 @@ public abstract class Principal extends CatalogObjectImpl {
}
/**
+ * Returns all privilege names for this principal that match 'filter'.
+ */
+ public Set<String> getFilteredPrivilegeNames(PrincipalPrivilegeTree.Filter filter) {
+ if (filter == null) return getPrivilegeNames();
+
+ List<PrincipalPrivilege> privileges;
+ try {
+ rwLock_.readLock().lock();
+ privileges = privilegeTree_.getFilteredList(filter);
+ } finally {
+ rwLock_.readLock().unlock();
+ }
+
+ Set<String> results = new HashSet<>();
+ for (PrincipalPrivilege priv: privileges) results.add(priv.getName());
+ return results;
+ }
+
+ /**
* Gets a privilege with the given name from this principal. If no privilege exists
* with this name null is returned.
*/
@@ -94,7 +132,14 @@ public abstract class Principal extends CatalogObjectImpl {
* privilege or null if no privilege exists with this name.
*/
public PrincipalPrivilege removePrivilege(String privilegeName) {
- return principalPrivileges_.remove(privilegeName);
+ try {
+ rwLock_.writeLock().lock();
+ PrincipalPrivilege privilege = principalPrivileges_.remove(privilegeName);
+ if (privilege != null) privilegeTree_.remove(privilege);
+ return privilege;
+ } finally {
+ rwLock_.writeLock().unlock();
+ }
}
/**
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 1b2da26..7d1b90f 100644
--- a/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
+++ b/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilege.java
@@ -37,9 +37,11 @@ public class PrincipalPrivilege extends CatalogObjectImpl {
private static final String AUTHORIZABLE_SEPARATOR = "->";
private static final String KV_SEPARATOR = "=";
private final TPrivilege privilege_;
+ private final String name_;
private PrincipalPrivilege(TPrivilege privilege) {
privilege_ = Preconditions.checkNotNull(privilege);
+ name_ = buildPrivilegeName(privilege_);
}
public TPrivilege toThrift() { return privilege_; }
@@ -153,7 +155,7 @@ public class PrincipalPrivilege extends CatalogObjectImpl {
return TCatalogObjectType.PRIVILEGE;
}
@Override
- public String getName() { return buildPrivilegeName(privilege_); }
+ public String getName() { return name_; }
public int getPrincipalId() { return privilege_.getPrincipal_id(); }
public TPrincipalType getPrincipalType() { return privilege_.getPrincipal_type(); }
diff --git a/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java b/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
new file mode 100644
index 0000000..61d87da
--- /dev/null
+++ b/fe/src/main/java/org/apache/impala/catalog/PrincipalPrivilegeTree.java
@@ -0,0 +1,248 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.impala.catalog;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import org.apache.impala.thrift.TPrivilege;
+import org.apache.impala.thrift.TPrivilegeScope;
+
+import com.google.common.base.Preconditions;
+
+/**
+ * Tree that allows efficient lookup for all privileges that can be relevant in the
+ * authorization of an object, e.g. getting privileges for server1/db1 should only iterate
+ * through privileges in db1, but not in other databases.
+ *
+ * Performance characteristics:
+ * add/remove: O(1), as the depth of the tree is limited
+ * getFilteredList: O(number_of_privileges_that_match_the_filter)
+ *
+ * The primary motivation is to speed up SHOW DATABASES/TABLES calls which do a separate
+ * listing to check access rights for each database/table. For this reason column and URI
+ * privileges do not get their own "level" in the tree - column privileges are merged with
+ * table privileges and URI privileges are stored per-server. This avoids unnecessarily
+ * storing many small hash maps in memory.
+ *
+ * This class is expected to be used in pair with a CatalogObjectCache<PrincipalPrivilege>
+ * which also handles catalog version logic and has efficient "remove" by full name.
+ */
+public class PrincipalPrivilegeTree {
+ // Contains database/table/column privileges grouped by server name + database name
+ // + table name. Column privileges are stored with table privileges to avoid the
+ // memory cost of creating many small hashmaps for columns with privileges.
+ Node<PrincipalPrivilege> tableRoot_ = new Node<>();
+
+ // Contains URI privileges grouped by server name. Storing these separately from table
+ // privileges allows Node to be simpler, as the Server level doesn't need to
+ // differentiate between two kind of privileges.
+ Node<PrincipalPrivilege> uriRoot_ = new Node<>();
+
+ // Contains server privileges grouped by server name. Stored separately from other
+ // privileges, as these should be returned both when listing URI and non-URI
+ // privileges.
+ Node<PrincipalPrivilege> serverRoot_ = new Node<>();
+
+ public void add(PrincipalPrivilege privilege) {
+ TPrivilege priv = privilege.toThrift();
+ List<String> path = toPath(priv);
+ Node<PrincipalPrivilege> root = getRootForScope(priv.getScope());
+ root.add(privilege.getName(), privilege, path);
+ }
+
+ public void remove(PrincipalPrivilege privilege) {
+ TPrivilege priv = privilege.toThrift();
+ List<String> path = toPath(priv);
+ Node<PrincipalPrivilege> root = getRootForScope(priv.getScope());
+ root.remove(privilege.getName(), privilege, path);
+ }
+
+ /**
+ * Collect all privileges that match the filter.
+ * E.g. for server1.db1, it returns privileges on:
+ * server1, server1.db1, server1.db1.table1,
+ * but not privileges on:
+ * server2, server2.db1, server1.db2
+ */
+ public List<PrincipalPrivilege> getFilteredList(Filter filter) {
+ List<String> path = filter.toPath();
+ List<PrincipalPrivilege> results = new ArrayList<>();
+ List<Node<PrincipalPrivilege>> roots = new ArrayList<>();
+ // Server level privileges apply to both URIs and other objects.
+ roots.add(serverRoot_);
+ if (filter.isUri_ || path.size() == 1) roots.add(uriRoot_);
+ if (!filter.isUri_) roots.add(tableRoot_);
+ for (Node<PrincipalPrivilege> root : roots) {
+ root.getAllMatchingValues(results, path);
+ }
+ return results;
+ }
+
+ private Node<PrincipalPrivilege> getRootForScope(TPrivilegeScope scope) {
+ switch(scope) {
+ case URI: return uriRoot_;
+ case SERVER: return serverRoot_;
+ default: return tableRoot_;
+ }
+ }
+
+ /**
+ * Creates a path to the given privilege in the tree like ["server1", "db1", "table1"].
+ */
+ private static List<String> toPath(TPrivilege priv) {
+ List<String> path = new ArrayList<>();
+ String server = priv.getServer_name();
+ String db = priv.getDb_name();
+ String table = priv.getTable_name();
+ if (server == null) return path;
+ path.add(server.toLowerCase());
+ if (db == null) return path;
+ path.add(db.toLowerCase());
+ if (table != null) path.add(table.toLowerCase());
+ return path;
+ }
+
+ /**
+ * Lossy representation of an Authorizable (doesn't contain column/URI name).
+ * Can be used to rule out the bulk of the privileges that can have no effect on an
+ * access check.
+ */
+ public static class Filter {
+ String server_, db_, table_; // must be lower case, null matches everything
+ boolean isUri_ = false;
+
+ public void setServer(String server) { server_ = server; }
+ public void setDb(String db) { db_ = db; }
+ public void setTable(String table) { table_ = table; }
+ public void setIsUri(boolean isUri) { isUri_ = isUri; }
+
+ /**
+ * Creates a path till the first null element of the filter, e.g.
+ * ["server1", "db1"] if table_ is null.
+ */
+ private List<String> toPath() {
+ List<String> path = new ArrayList<>();
+ if (server_ == null) return path;
+ path.add(server_);
+ if (db_ == null) return path;
+ Preconditions.checkState(!isUri_);
+ path.add(db_);
+ if (table_ != null) path.add(table_);
+ return path;
+ }
+ }
+
+ /**
+ * Tree node that holds the privileges for a given object (server, database, table),
+ * and its children objects (e.g. databases for a server). Descendants can be addressed
+ * with paths like ["server1", "db1", "table1"].
+ *
+ * Only used to store privileges, but creating a generic class seemed clearer.
+ */
+ private static class Node<T> {
+ Map<String, T> values_ = null;
+ Map<String, Node<T>> children_= null;
+
+ boolean isEmpty() { return values_ == null && children_ == null; }
+
+ /**
+ * Finds the Node at 'path' (or potentially builds it if it doesn't exist),
+ * and adds 'key' + 'value' to it.
+ */
+ public void add(String key, T value, List<String> path) {
+ add(key, value, path, 0);
+ }
+
+ /**
+ * Finds the Node at 'path' (it is treated as error if it doesn't exist),
+ * and removes 'key' + 'value' from it. If a Node becomes empty, it is removed from
+ * it's parent.
+ */
+ public void remove(String key, T value, List<String> path) {
+ remove(key, value, path, 0);
+ }
+
+ /**
+ * Collect all values in this node and those descendants that match 'path'.
+ */
+ public void getAllMatchingValues(List<T> results, List<String> path) {
+ getAllMatchingValues(results, path, 0);
+ }
+
+ /**
+ * Collect all values in this node and its descendants.
+ */
+ public void getAllValues(List<T> results) {
+ if (values_ != null) results.addAll(values_.values());
+ if (children_ != null) {
+ for (Map.Entry<String, Node<T>> entry : children_.entrySet()) {
+ entry.getValue().getAllValues(results);
+ }
+ }
+ }
+
+ private void add(String key, T value, List<String> path, int depth) {
+ if (path.size() <= depth) {
+ if (values_ == null) {
+ // It is very common to have only a single privilege on an object
+ // (e.g. ownership), so the initial capacity is 1 to avoid wasting memory.
+ values_ = new HashMap<>(1);
+ }
+ values_.put(key, value); // Can update an existing value.
+ } else {
+ if (children_ == null) children_ = new HashMap<>();
+ String child_name = path.get(depth);
+ Node<T> next = children_.computeIfAbsent(child_name, (k) -> new Node<T>());
+ next.add(key, value, path, depth + 1);
+ }
+ }
+
+ private void remove(String key, T value, List<String> path, int depth) {
+ if (path.size() <= depth) {
+ Preconditions.checkNotNull(values_);
+ T found = values_.remove(key);
+ Preconditions.checkNotNull(found);
+ if (values_.isEmpty()) values_ = null;
+ } else {
+ Preconditions.checkNotNull(children_);
+ String child_name = path.get(depth);
+ Node<T> next = children_.get(child_name);
+ Preconditions.checkNotNull(next);
+ next.remove(key, value, path, depth + 1);
+ // Remove the child if it became empty.
+ if (next.isEmpty()) children_.remove(child_name);
+ if (children_.isEmpty()) children_ = null;
+ }
+ }
+
+ private void getAllMatchingValues(List<T> results, List<String> path, int depth) {
+ if (path.size() <= depth) {
+ getAllValues(results);
+ return;
+ }
+ if (values_ != null) results.addAll(values_.values());
+ if (children_ == null) return;
+ String child_name = path.get(depth);
+ Preconditions.checkNotNull(child_name);
+ Node<T> next = children_.get(child_name);
+ if (next != null) next.getAllMatchingValues(results, path, depth + 1);
+ }
+ }
+}
diff --git a/fe/src/test/java/org/apache/impala/catalog/PrincipalPrivilegeTreeTest.java b/fe/src/test/java/org/apache/impala/catalog/PrincipalPrivilegeTreeTest.java
new file mode 100644
index 0000000..dfc38f1
--- /dev/null
+++ b/fe/src/test/java/org/apache/impala/catalog/PrincipalPrivilegeTreeTest.java
@@ -0,0 +1,134 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.impala.catalog;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertSame;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import org.apache.impala.thrift.TPrivilege;
+import org.apache.impala.thrift.TPrivilegeLevel;
+import org.apache.impala.thrift.TPrivilegeScope;
+
+public class PrincipalPrivilegeTreeTest {
+ @Test
+ public void testPrincipalPrivilegeTree() {
+ // This test is mainly based on TestTreePrivilegeCache.testListPrivilegesWildCard in
+ // Sentry.
+ List<PrincipalPrivilege> privs = new ArrayList<>();
+
+ privs.add(createTablePriv("server1", "db1", "t1"));
+ privs.add(createTablePriv("server1", "db2", "t1"));
+ privs.add(createTablePriv("server1", "db1", "t2"));
+ privs.add(createDbPriv("server1", "db1"));
+ privs.add(createServerPriv("server1"));
+ privs.add(createServerPriv("server2"));
+ privs.add(createColumnPriv("server1", "db1", "t1", "c1"));
+ privs.add(createUriPriv("server1", "uri1"));
+
+ PrincipalPrivilegeTree tree = new PrincipalPrivilegeTree();
+ // Run the same tests twice to check if removing privileges works correctly.
+ for (int i = 0; i < 2; i++) {
+ for (PrincipalPrivilege priv : privs) tree.add(priv);
+
+ // Update a privilege and check that the newer object is returned by
+ // getFilteredList.
+ PrincipalPrivilege newServer2Priv = createServerPriv("server2");
+ tree.add(newServer2Priv);
+ List<PrincipalPrivilege> results =
+ tree.getFilteredList(createFilter("server2", null, null));
+ assertEquals(1, results.size());
+ assertSame(results.get(0), newServer2Priv);
+
+ assertEquals(7, tree.getFilteredList(createFilter("server1", null, null)).size());
+ assertEquals(5, tree.getFilteredList(createFilter("server1", "db1", null)).size());
+ assertEquals(2, tree.getFilteredList(createFilter("server1", "db2", null)).size());
+ assertEquals(4, tree.getFilteredList(createFilter("server1", "db1", "t1")).size());
+ assertEquals(2, tree.getFilteredList(createFilter("server1", "db2", "t1")).size());
+ assertEquals(2, tree.getFilteredList(createUriFilter("server1")).size());
+
+ // Check that all privileges are removed successfully.
+ for (PrincipalPrivilege priv : privs) tree.remove(priv);
+ assertEquals(0, tree.getFilteredList(createFilter("server1", null, null)).size());
+ }
+ }
+
+ PrincipalPrivilege createColumnPriv(String server, String db, String table,
+ String column) {
+ TPrivilege priv =
+ new TPrivilege(TPrivilegeLevel.SELECT, TPrivilegeScope.COLUMN, false);
+ priv.setServer_name(server);
+ priv.setDb_name(db);
+ priv.setTable_name(table);
+ priv.setColumn_name(column);
+ return PrincipalPrivilege.fromThrift(priv);
+ }
+
+ PrincipalPrivilege createTablePriv(String server, String db, String table) {
+ TPrivilege priv =
+ new TPrivilege(TPrivilegeLevel.SELECT, TPrivilegeScope.TABLE, false);
+ priv.setServer_name(server);
+ priv.setDb_name(db);
+ priv.setTable_name(table);
+ return PrincipalPrivilege.fromThrift(priv);
+ }
+
+ PrincipalPrivilege createDbPriv(String server, String db) {
+ TPrivilege priv =
+ new TPrivilege(TPrivilegeLevel.SELECT, TPrivilegeScope.DATABASE, false);
+ priv.setServer_name(server);
+ priv.setDb_name(db);
+ return PrincipalPrivilege.fromThrift(priv);
+ }
+
+ PrincipalPrivilege createUriPriv(String server, String uri) {
+ TPrivilege priv =
+ new TPrivilege(TPrivilegeLevel.SELECT, TPrivilegeScope.URI, false);
+ priv.setServer_name(server);
+ priv.setUri(uri);
+ return PrincipalPrivilege.fromThrift(priv);
+ }
+
+ PrincipalPrivilege createServerPriv(String server) {
+ TPrivilege priv =
+ new TPrivilege(TPrivilegeLevel.SELECT, TPrivilegeScope.SERVER, false);
+ priv.setServer_name(server);
+ return PrincipalPrivilege.fromThrift(priv);
+ }
+
+ PrincipalPrivilegeTree.Filter createFilter(String server, String db,
+ String table) {
+ PrincipalPrivilegeTree.Filter filter = new PrincipalPrivilegeTree.Filter();
+ filter.setServer(server);
+ filter.setDb(db);
+ filter.setTable(table);
+ filter.setIsUri(false);
+ return filter;
+ }
+
+ PrincipalPrivilegeTree.Filter createUriFilter(String server) {
+ PrincipalPrivilegeTree.Filter filter = new PrincipalPrivilegeTree.Filter();
+ filter.setServer(server);
+ filter.setIsUri(true);
+ return filter;
+ }
+}