You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ar...@apache.org on 2018/07/18 21:35:40 UTC
[3/5] impala git commit: IMPALA-6881: COMPUTE STATS should require
SELECT privilege at analysis
IMPALA-6881: COMPUTE STATS should require SELECT privilege at analysis
In order to do COMPUTE STATS, Impala performs several SELECT queries.
However, in the COMPUTE STATS analysis phase, we only check for the
ALTER privilege. Although the SELECT privilege is eventually checked
in the target table by the SELECT child queries, it is better to
check the SELECT privilege in the COMPUTE STATS analysis phase and
fail early if the privilege requirements are not met instead of failing
later in the SELECT child queries due to insufficient privileges.
Testing:
- Updated the authorization tests
- Ran all FE tests
Change-Id: Icead43e689404a286859344e309427eb6c68646a
Reviewed-on: http://gerrit.cloudera.org:8080/10918
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/c0e00cad
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/c0e00cad
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/c0e00cad
Branch: refs/heads/master
Commit: c0e00cad96338ca006638c58453627109b4415e0
Parents: 4f2a815
Author: Fredy Wijaya <fw...@cloudera.com>
Authored: Tue Jul 10 20:09:48 2018 -0700
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Wed Jul 18 03:33:29 2018 +0000
----------------------------------------------------------------------
.../org/apache/impala/analysis/Analyzer.java | 34 ++++---
.../impala/analysis/ComputeStatsStmt.java | 2 +-
.../impala/analysis/DropTableOrViewStmt.java | 2 +-
.../apache/impala/analysis/PartitionDef.java | 3 +-
.../impala/analysis/PartitionSpecBase.java | 2 +-
.../apache/impala/analysis/AuditingTest.java | 7 +-
.../impala/analysis/AuthorizationStmtTest.java | 94 +++++++++++++-------
7 files changed, 93 insertions(+), 51 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/c0e00cad/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
index 242df27..1a4ea13 100644
--- a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
+++ b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
@@ -2397,21 +2397,25 @@ public class Analyzer {
* Returns the Table with the given name from the 'loadedTables' map in the global
* analysis state. Throws an AnalysisException if the table or the db does not exist.
* Throws a TableLoadingException if the registered table failed to load.
- * Always registers a privilege request for the table at the given privilege level,
+ * Always registers privilege request(s) for the table at the given privilege level(s),
* regardless of the state of the table (i.e. whether it exists, is loaded, etc.).
- * If addAccessEvent is true adds an access event for successfully loaded tables.
+ * If addAccessEvent is true adds access event(s) for successfully loaded tables. When
+ * multiple privileges are specified, all those privileges will be required for the
+ * authorization check.
*/
- public FeTable getTable(TableName tableName, Privilege privilege, boolean addAccessEvent)
- throws AnalysisException, TableLoadingException {
+ public FeTable getTable(TableName tableName, boolean addAccessEvent,
+ Privilege... privilege) throws AnalysisException, TableLoadingException {
Preconditions.checkNotNull(tableName);
Preconditions.checkNotNull(privilege);
tableName = getFqTableName(tableName);
- if (privilege == Privilege.ANY) {
- registerPrivReq(new PrivilegeRequestBuilder()
- .any().onAnyColumn(tableName.getDb(), tableName.getTbl()).toRequest());
- } else {
- registerPrivReq(new PrivilegeRequestBuilder()
- .allOf(privilege).onTable(tableName.getDb(), tableName.getTbl()).toRequest());
+ for (Privilege priv : privilege) {
+ if (priv == Privilege.ANY) {
+ registerPrivReq(new PrivilegeRequestBuilder()
+ .any().onAnyColumn(tableName.getDb(), tableName.getTbl()).toRequest());
+ } else {
+ registerPrivReq(new PrivilegeRequestBuilder()
+ .allOf(priv).onTable(tableName.getDb(), tableName.getTbl()).toRequest());
+ }
}
FeTable table = getTable(tableName.getDb(), tableName.getTbl());
Preconditions.checkNotNull(table);
@@ -2419,8 +2423,10 @@ public class Analyzer {
// Add an audit event for this access
TCatalogObjectType objectType = TCatalogObjectType.TABLE;
if (table instanceof FeView) objectType = TCatalogObjectType.VIEW;
- globalState_.accessEvents.add(new TAccessEvent(
- tableName.toString(), objectType, privilege.toString()));
+ for (Privilege priv : privilege) {
+ globalState_.accessEvents.add(new TAccessEvent(
+ tableName.toString(), objectType, priv.toString()));
+ }
}
return table;
}
@@ -2433,10 +2439,10 @@ public class Analyzer {
* AuthorizationException is thrown.
* If the table or the db does not exist in the Catalog, an AnalysisError is thrown.
*/
- public FeTable getTable(TableName tableName, Privilege privilege)
+ public FeTable getTable(TableName tableName, Privilege... privilege)
throws AnalysisException {
try {
- return getTable(tableName, privilege, true);
+ return getTable(tableName, true, privilege);
} catch (TableLoadingException e) {
throw new AnalysisException(e);
}
http://git-wip-us.apache.org/repos/asf/impala/blob/c0e00cad/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java b/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
index 7743120..4f1173e 100644
--- a/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
+++ b/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
@@ -343,7 +343,7 @@ public class ComputeStatsStmt extends StatementBase {
throw new AnalysisException(String.format(
"COMPUTE STATS not supported for nested collection: %s", tableName_));
}
- table_ = analyzer.getTable(tableName_, Privilege.ALTER);
+ table_ = analyzer.getTable(tableName_, Privilege.ALTER, Privilege.SELECT);
if (!(table_ instanceof FeFsTable)) {
if (partitionSet_ != null) {
http://git-wip-us.apache.org/repos/asf/impala/blob/c0e00cad/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java b/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
index 3e43a01..e38078a 100644
--- a/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
+++ b/fe/src/main/java/org/apache/impala/analysis/DropTableOrViewStmt.java
@@ -96,7 +96,7 @@ public class DropTableOrViewStmt extends StatementBase {
public void analyze(Analyzer analyzer) throws AnalysisException {
dbName_ = analyzer.getTargetDbName(tableName_);
try {
- FeTable table = analyzer.getTable(tableName_, Privilege.DROP, true);
+ FeTable table = analyzer.getTable(tableName_, true, Privilege.DROP);
Preconditions.checkNotNull(table);
if (table instanceof FeView && dropTable_) {
throw new AnalysisException(String.format(
http://git-wip-us.apache.org/repos/asf/impala/blob/c0e00cad/fe/src/main/java/org/apache/impala/analysis/PartitionDef.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/PartitionDef.java b/fe/src/main/java/org/apache/impala/analysis/PartitionDef.java
index c8e0a4f..7b4f837 100644
--- a/fe/src/main/java/org/apache/impala/analysis/PartitionDef.java
+++ b/fe/src/main/java/org/apache/impala/analysis/PartitionDef.java
@@ -83,8 +83,7 @@ public class PartitionDef implements ParseNode {
FeTable table;
try {
- table = analyzer.getTable(partitionSpec_.getTableName(), Privilege.ALTER,
- false);
+ table = analyzer.getTable(partitionSpec_.getTableName(), false, Privilege.ALTER);
} catch (TableLoadingException e) {
throw new AnalysisException(e.getMessage(), e);
}
http://git-wip-us.apache.org/repos/asf/impala/blob/c0e00cad/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java b/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java
index 4d432e2..ede438b 100644
--- a/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java
+++ b/fe/src/main/java/org/apache/impala/analysis/PartitionSpecBase.java
@@ -74,7 +74,7 @@ public abstract class PartitionSpecBase implements ParseNode {
// be audited outside of the PartitionSpec.
FeTable table;
try {
- table = analyzer.getTable(tableName_, privilegeRequirement_, false);
+ table = analyzer.getTable(tableName_, false, privilegeRequirement_);
} catch (TableLoadingException e) {
throw new AnalysisException(e.getMessage(), e);
}
http://git-wip-us.apache.org/repos/asf/impala/blob/c0e00cad/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java b/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
index cc91188..7735d3f 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AuditingTest.java
@@ -280,7 +280,9 @@ public class AuditingTest extends FrontendTestBase {
"COMPUTE STATS functional_seq_snap.alltypes");
Assert.assertEquals(accessEvents, Sets.newHashSet(
new TAccessEvent(
- "functional_seq_snap.alltypes", TCatalogObjectType.TABLE, "ALTER")));
+ "functional_seq_snap.alltypes", TCatalogObjectType.TABLE, "ALTER"),
+ new TAccessEvent(
+ "functional_seq_snap.alltypes", TCatalogObjectType.TABLE, "SELECT")));
}
@Test
@@ -443,7 +445,8 @@ public class AuditingTest extends FrontendTestBase {
// Compute stats
accessEvents = AnalyzeAccessEvents("compute stats functional_kudu.testtbl");
Assert.assertEquals(accessEvents, Sets.newHashSet(
- new TAccessEvent("functional_kudu.testtbl", TCatalogObjectType.TABLE, "ALTER")));
+ new TAccessEvent("functional_kudu.testtbl", TCatalogObjectType.TABLE, "ALTER"),
+ new TAccessEvent("functional_kudu.testtbl", TCatalogObjectType.TABLE, "SELECT")));
// Describe
accessEvents = AnalyzeAccessEvents("describe functional_kudu.testtbl");
http://git-wip-us.apache.org/repos/asf/impala/blob/c0e00cad/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java b/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
index df46899..3095ff8 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
@@ -1150,38 +1150,72 @@ public class AuthorizationStmtTest extends FrontendTestBase {
@Test
public void testStats() throws ImpalaException {
- for (String statsType: new String[]{"compute", "drop"}) {
- authorize(String.format("%s stats functional.alltypes", statsType))
- .ok(onServer(TPrivilegeLevel.ALL))
- .ok(onServer(TPrivilegeLevel.ALTER))
- .ok(onDatabase("functional", TPrivilegeLevel.ALL))
- .ok(onDatabase("functional", TPrivilegeLevel.ALTER))
- .ok(onTable("functional", "alltypes", TPrivilegeLevel.ALL))
- .ok(onTable("functional", "alltypes", TPrivilegeLevel.ALTER))
- .error(alterError("functional.alltypes"))
- .error(alterError("functional.alltypes"), onServer(allExcept(
- TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER)))
- .error(alterError("functional.alltypes"), onDatabase("functional",
- allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER)))
- .error(alterError("functional.alltypes"), onTable("functional", "alltypes",
- allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER)));
+ // Compute stats.
+ authorize("compute stats functional.alltypes")
+ .ok(onServer(TPrivilegeLevel.ALL))
+ .ok(onServer(TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT))
+ .ok(onDatabase("functional", TPrivilegeLevel.ALL))
+ .ok(onDatabase("functional", TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT))
+ .ok(onTable("functional", "alltypes", TPrivilegeLevel.ALL))
+ .ok(onTable("functional", "alltypes", TPrivilegeLevel.ALTER,
+ TPrivilegeLevel.SELECT))
+ .error(alterError("functional.alltypes"))
+ .error(alterError("functional.alltypes"), onServer(allExcept(
+ TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT)))
+ .error(alterError("functional.alltypes"), onDatabase("functional",
+ allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER,
+ TPrivilegeLevel.SELECT)))
+ .error(alterError("functional.alltypes"), onTable("functional", "alltypes",
+ allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER,
+ TPrivilegeLevel.SELECT)));
+
+ // Compute stats on database that does not exist.
+ authorize("compute stats nodb.notbl")
+ .error(alterError("nodb.notbl"))
+ .error(alterError("nodb.notbl"), onServer(allExcept(
+ TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT)))
+ .error(alterError("nodb.notbl"), onDatabase("nodb", allExcept(
+ TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT)));
+
+ // Compute stats on table that does not exist.
+ authorize("compute stats functional.notbl")
+ .error(alterError("functional.notbl"))
+ .error(alterError("functional.notbl"), onServer(allExcept(
+ TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT)))
+ .error(alterError("functional.notbl"), onDatabase("functional", allExcept(
+ TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER, TPrivilegeLevel.SELECT)));
- // Database does not exist.
- authorize(String.format("%s stats nodb.notbl", statsType))
- .error(alterError("nodb.notbl"))
- .error(alterError("nodb.notbl"), onServer(allExcept(
- TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER)))
- .error(alterError("default.nodb"), onDatabase("nodb", allExcept(
- TPrivilegeLevel.ALL, TPrivilegeLevel.ALL)));
+ // Drop stats.
+ authorize("drop stats functional.alltypes")
+ .ok(onServer(TPrivilegeLevel.ALL))
+ .ok(onServer(TPrivilegeLevel.ALTER))
+ .ok(onDatabase("functional", TPrivilegeLevel.ALL))
+ .ok(onDatabase("functional", TPrivilegeLevel.ALTER))
+ .ok(onTable("functional", "alltypes", TPrivilegeLevel.ALL))
+ .ok(onTable("functional", "alltypes", TPrivilegeLevel.ALTER))
+ .error(alterError("functional.alltypes"))
+ .error(alterError("functional.alltypes"), onServer(allExcept(
+ TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER)))
+ .error(alterError("functional.alltypes"), onDatabase("functional",
+ allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER)))
+ .error(alterError("functional.alltypes"), onTable("functional", "alltypes",
+ allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER)));
- // Table does not exist.
- authorize(String.format("%s stats functional.notbl", statsType))
- .error(alterError("functional.notbl"))
- .error(alterError("functional.notbl"), onServer(allExcept(
- TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER)))
- .error(alterError("default.functional"), onDatabase("functional", allExcept(
- TPrivilegeLevel.ALL, TPrivilegeLevel.ALL)));
- }
+ // Drop stats on database that does not exist.
+ authorize("drop stats nodb.notbl")
+ .error(alterError("nodb.notbl"))
+ .error(alterError("nodb.notbl"), onServer(allExcept(
+ TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER)))
+ .error(alterError("nodb.notbl"), onDatabase("nodb", allExcept(
+ TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER)));
+
+ // Drop stats on table that does not exist.
+ authorize("drop stats functional.notbl")
+ .error(alterError("functional.notbl"))
+ .error(alterError("functional.notbl"), onServer(allExcept(
+ TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER)))
+ .error(alterError("functional.notbl"), onDatabase("functional", allExcept(
+ TPrivilegeLevel.ALL, TPrivilegeLevel.ALTER)));
}
@Test