You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Huaisi Xu (Code Review)" <ge...@cloudera.org> on 2016/06/11 00:38:08 UTC
[Impala-CR](cdh5-trunk) IMPALA-3711: Only check privilege for databases in getSchemas()
Huaisi Xu has uploaded a new change for review.
http://gerrit.cloudera.org:8080/3371
Change subject: IMPALA-3711: Only check privilege for databases in getSchemas()
......................................................................
IMPALA-3711: Only check privilege for databases in getSchemas()
Previously getSchemas() checks privileges for all databases
and tables, which can lead to bad performance.
Given that it only returns databases, we are save just change
its privilege checking behavior to only check databases. Also
apply filter on database names before checking.
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
1 file changed, 5 insertions(+), 8 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/1
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3371
to look at the new patch set (#14).
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Previously all code paths using getDbsMetadata() sufferred
unnecessary privilege checks:
1. Impala checked privilege of all databases, tables before
applying user provided JDBC pattern filters.
2. Impala passed a null pattern to getDbsMetadata() when
user did not provide one. However, null pattern is treated
as "%", which matches everything thereby causing unnecessary
privilege checks for catalog objects that are not in the
result set.
This patch modifies the behavior of getDbsMetadata() to
avoid retrieving and checking for privileges of catalog
entities that are of no interest to the user.
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
M fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/com/cloudera/impala/catalog/Db.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
M fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java
M fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
12 files changed, 377 insertions(+), 119 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/14
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 14
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Only check privilege for databases in getSchemas()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Only check privilege for databases in getSchemas()
......................................................................
Patch Set 1:
> I don't think these changes are correct. I believe the HS2 API
> specifies that getSchemas(), getTables(), etc, should be able to
> accept a search pattern as an input parameter. You seem to remove
> that functionality altogether, but this is not what we want.
Could you elaborate?
Our currently API for getSchamas() is:
getSchemas(Frontend fe, String catalogName, String schemaName, User user)
It does not take table name or column name at all as input parameter. My patch only changes the inner behavior of getSchemas(), which I believe is invisible.? right?
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 17:
not ready for review, will update soon
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 17
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 17:
(2 comments)
there are lot of changes to test. so just make sure the new API looks good to you... Henry. thank you!
http://gerrit.cloudera.org:8080/#/c/3371/16/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
File fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java:
PS16, Line 74: or (String pattern: Arrays.asList(hivePattern.split("\\|")))
> I really, really think it would be better to have class-wide constants that
Done
http://gerrit.cloudera.org:8080/#/c/3371/17/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
File fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java:
PS17, Line 48: public static final String JDBC_MATCH_ALL_PATTERN = "%";
: public static final String HIVE_MATCH_ALL_PATTERN = null;
: public static final String MATCH_NONE_PATTERN = "";
:
: public boolean hasPattern() { return patterns_ != null; }
:
: public static String getJDBCPattern(String pattern) {
: if (pattern == null || pattern.isEmpty()) return JDBC_MATCH_ALL_PATTERN;
: return pattern;
does this change look good? I can have another getHivePattern(String) to return valid String so that I can have hivePattern in createHivePatternMatcher() to be not null(include in a precondition check).
User has to call this getxxxxPattern() in metadataOp.java in order to convert pattern string early.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 17
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has uploaded a new patch set (#6).
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
IMPALA-3711: Check privileges when necessary
Previously
1. impala checks all columns privileges when user
only request table name
2. impala checks all table+column privileges when
user only request database name
This patch prevent checking unnecessary privileges.
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/catalog/Db.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
6 files changed, 311 insertions(+), 49 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/6
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 6
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 21:
It looks like GVM was run against the commit hash from patch set 20, which is why you got the +1 on it, instead of patch set 21 along with a submit.
http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge/2575/
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 21
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3371/10//COMMIT_MSG
Commit Message:
PS10, Line 9: useing
using
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 10
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
Patch Set 5:
Note: my patch does not change the result of any test. I kept the same behavior. The test may be strange but it was there already. see IMPALA-3744.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 5
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has uploaded a new patch set (#10).
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Previously all code paths useing getDbsMetadata() suffers
unnecessary privilege checks:
1. Impala checked privilege of all databases, tables before
applying user provided JDBC pattern filters.
2. Impala passed a null pattern to getDbsMetadata() when
user did not provide one. However, null pattern is treated
as "%", which matches everything. As a result, even though
user wants nothing, we ended up checking everything.
This patch changes getDbsMetadata()'s interface such that
caller can pass addition information and it knows whether
pattern is provided or not. This patch also applies filters
before checks any privilege.
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
M fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/com/cloudera/impala/catalog/Db.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
M fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java
M fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
12 files changed, 382 insertions(+), 112 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/10
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 10
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
Patch Set 6:
bump
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 6
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 13:
(11 comments)
thanks Henry and Dimitris
http://gerrit.cloudera.org:8080/#/c/3371/13/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java:
PS13, Line 175: if (matcher == null) return Collections.emptyList();
> rather than dealing with this case everywhere, push the logic into PatternM
Setting the pattern of the matcher to null is the problem we are trying to solve.
I can push the logic to filterCatalogObjectsByPattern and filterStringsByPattern though. what do you think
PS13, Line 226: See filterStringsByPattern
: * for details of the pattern match semantics.
> is this comment stale now?
Done
PS13, Line 231: getDataSourceNames
> Is this version really necessary? We don't have this specialisation for ge
what do you mean by version? I changed this code because I changed the interface of filterStringsByPattern to take a pattern matcher?
PS13, Line 237: aal
> all
Done
PS13, Line 237: the pattern of 'matcher'.
> nit, just say 'that match 'matcher''
Done
PS13, Line 238: @see PatternMatcher for supported matchers
> this seems redundant, and can be removed.
Done, and I changed all occurrence.
PS13, Line 327: If
: * 'matcher' does not have a pattern, all catalog objects in 'candidates' are returned.
> This comment leaks through the abstraction of PatternMatcher - i.e. it shou
Done
PS13, Line 334: private
> probably should live in PatternMatcher itself.
then the same may apply to filterCatalogObjectsByPattern(), but it accesses getName(). So I guess it is better to take a filterKey functor as input? so it can call getName() accordingly? But I think that is unnecessary refactor? what do you think?
PS13, Line 360: f
: * 'matcher' does not have a pattern, all catalog objects in 'candidates' are returne
> same comment as above
Done
PS13, Line 365: @see PatternMatcher#matches(String) for matching mechanisms.
> I don't think this is necessary. Callers of this method will have construct
Done
http://gerrit.cloudera.org:8080/#/c/3371/13/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
PS13, Line 274: public boolean hasDbPattern() { return isDbPatternSet_; }
: public boolean hasTablePattern() { return isTablePatternSet_; }
: public boolean hasColumnPattern() { return isColumnPatternSet_; }
: public boolean hasFunctionPattern() { return isFunctionPatternSet_; }
> it would be cleaner to make it so that the default values (maybe null?) do
null is a valid pattern. so we cannot tell if a null pattern is set by the caller or it is just unset. This is one of the problem we are trying to solve.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 13
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3371/6/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
PS6, Line 350: fe.getTableNames(
: db.getName(), "*", tablePatternMatcher, user)
> why? that way There will be a lot of duplicated code? we cannot call getTab
I don't think there will be any duplicate code. getTableNames that takes as input a string for the pattern will simply create a new HivePatternMatcher and pass it to the second getTableNames function.
Line 547: MetadataOpParams getDbsMetadataParams = MetadataOpParams.MetadataOpParamsBuilder()
> if someone call this with tableName to be null, then we cannot differentiat
ignoreAllColumns(true) is equivalent to hasColumnPattern(false). Irrespective of the actual pattern used (null, "", etc), we set the hasDbPattern and hasTablePattern to true by virtue of calling the equivalent set functions. So in the getDbsMetadata() you can check if you have a column pattern and if not, don't make any calls to Catalog.getColumns().
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 6
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has uploaded a new patch set (#11).
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Previously all code paths using getDbsMetadata() suffers
unnecessary privilege checks:
1. Impala checked privilege of all databases, tables before
applying user provided JDBC pattern filters.
2. Impala passed a null pattern to getDbsMetadata() when
user did not provide one. However, null pattern is treated
as "%", which matches everything. As a result, even though
user wants nothing, we ended up checking everything.
This patch changes getDbsMetadata()'s interface such that
caller can pass addition information and it knows whether
pattern is provided or not. This patch also applies filters
before checks any privilege.
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
M fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/com/cloudera/impala/catalog/Db.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
M fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java
M fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
12 files changed, 380 insertions(+), 115 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/11
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 11
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 9:
(10 comments)
Minor comment. Getting there.
http://gerrit.cloudera.org:8080/#/c/3371/9/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java:
PS9, Line 235: using matcher
"that match the pattern of 'matcher'."
PS9, Line 238: matcher
nit: 'matcher'; good practice to use single quotes when referring to input params.
PS9, Line 328: candidats
typo: candidates
http://gerrit.cloudera.org:8080/#/c/3371/9/fe/src/main/java/com/cloudera/impala/service/Frontend.java
File fe/src/main/java/com/cloudera/impala/service/Frontend.java:
PS9, Line 614: columnPatternMatcher
update
PS9, Line 615: columnPatternMatcher
'matcher'
http://gerrit.cloudera.org:8080/#/c/3371/9/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
File fe/src/main/java/com/cloudera/impala/service/JniFrontend.java:
PS9, Line 210: Implement Hive's pattern-matching semantics for "SHOW TABLE [[LIKE] 'pattern']".
: * The only metacharacters are '*' which matches any string of characters, and '|'
: * which denotes choice. Doing the work here saves loading tables or databases from the
: * metastore (which Hive would do if we passed the call through to the metastore
: * client). If the pattern is null, all strings are considered to match. If it is an
: * empty string, no strings match.
Can you move this below L220? The first sentence of the function comment should describe the main role of a function.
PS9, Line 265: Implement Hive's pattern-matching semantics for "SHOW DATABASES [[LIKE] 'pattern']".
: * The only metacharacters are '*' which matches any string of characters, and '|' which
: * denotes choice. Doing the work here saves loading tables or databases from the
: * metastore (which Hive would do if we passed the call through to the metastore
: * client). If the pattern is null, all strings are considered to match. If it is an
: * empty string, no strings match.
This is kind of repetitive. You may want to keep the first sentence, move it below L275 and remove the rest. If you want you may point to the function comment of getTableNames above.
http://gerrit.cloudera.org:8080/#/c/3371/9/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
PS9, Line 226: getMetadataOp
There is no such thing :)
PS9, Line 302: If metadataParams.tablePattern_ is null, then result.tableNames and result.columns
: * will not be populated.
: * If metadataParams.columnPattern_ is null, then result.columns will not be populated.
That's not correct. tablePattern_ could be set to null which indicates that everything matches. Same for columnPattern_, if it is null it means that all columns match for the matching dbs and tables.
http://gerrit.cloudera.org:8080/#/c/3371/9/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
File fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java:
PS9, Line 34: Returns true if pattern_ is null or the candidate matches.
: // Returns false if pattern_ is empty or the candidate mismatches.
Needless to say, these semantics are really weird. Btw, you have a typo, it's patterns_ not pattern_.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 9
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 13:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/3371/13/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java:
PS13, Line 175: if (matcher == null) return Collections.emptyList();
rather than dealing with this case everywhere, push the logic into PatternMatcher. This is probably easiest achieved by setting the pattern of the matcher to null.
PS13, Line 226: See filterStringsByPattern
: * for details of the pattern match semantics.
is this comment stale now?
PS13, Line 231: getDataSourceNames
Is this version really necessary? We don't have this specialisation for getTableNames or getDbNames I think.
PS13, Line 237: the pattern of 'matcher'.
nit, just say 'that match 'matcher''
PS13, Line 237: aal
all
PS13, Line 238: @see PatternMatcher for supported matchers
this seems redundant, and can be removed.
PS13, Line 327: If
: * 'matcher' does not have a pattern, all catalog objects in 'candidates' are returned.
This comment leaks through the abstraction of PatternMatcher - i.e. it should be sufficient to say "Return all members of 'candidates' that match 'matches'".
PS13, Line 334: private
probably should live in PatternMatcher itself.
PS13, Line 360: f
: * 'matcher' does not have a pattern, all catalog objects in 'candidates' are returne
same comment as above
PS13, Line 365: @see PatternMatcher#matches(String) for matching mechanisms.
I don't think this is necessary. Callers of this method will have constructed a PatternMatcher, so know that that's where they should look for matching semantics.
http://gerrit.cloudera.org:8080/#/c/3371/13/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
PS13, Line 274: public boolean hasDbPattern() { return isDbPatternSet_; }
: public boolean hasTablePattern() { return isTablePatternSet_; }
: public boolean hasColumnPattern() { return isColumnPatternSet_; }
: public boolean hasFunctionPattern() { return isFunctionPatternSet_; }
it would be cleaner to make it so that the default values (maybe null?) do the right thing, so you don't have to write any logic to handle the special case where the pattern is not set.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 13
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 13:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3371/13/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java:
PS13, Line 175: if (matcher == null) return Collections.emptyList();
> I believe Henry's comment has broader implications than simply pushing the
Yes, that's exactly right, thanks! I'm sorry if this undoes some of the previous work you've done. The benefit is that we would not need this logic here to check for a null matcher. Special cases like that are always more risky, and harder for the reader to reason about.
PS13, Line 334: private
> then the same may apply to filterCatalogObjectsByPattern(), but it accesses
The same doesn't apply to filterCatalogObjectsByPattern(), because a CatalogObject is a concept that lives in the catalog. This method operates only on strings.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 13
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Only check privilege for databases in getSchemas()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Only check privilege for databases in getSchemas()
......................................................................
Patch Set 1:
need some polish.
tested by backporting this to a 5.4.10 cluster, which has many roles/privileges/tables, etc.. getScehma() performance increases at least 50x+. I did not count in detail, got to go.. will do on monday.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3371
to look at the new patch set (#15).
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Previously all code paths using getDbsMetadata() sufferred
unnecessary privilege checks:
1. Impala checked privilege of all databases, tables before
applying user provided JDBC pattern filters.
2. Impala passed a null pattern to getDbsMetadata() when
user did not provide one. However, null pattern is treated
as "%", which matches everything thereby causing unnecessary
privilege checks for catalog objects that are not in the
result set.
This patch translates null pattern to "match all" early so
that after change JDBC pattern matcher, it is able to safely
treat all nulls as "match nothing".
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
M fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/com/cloudera/impala/catalog/Db.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
M fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java
M fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
12 files changed, 288 insertions(+), 105 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/15
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 15
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
Patch Set 6:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/3371/6/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
PS6, Line 229: catalogName
> private member should end with a '_'.
Done
PS6, Line 230: private boolean ignoreAllDbs;
: private boolean ignoreAllTables;
: private boolean ignoreAllColumns;
: private boolean ignoreAllFunctions;
> Instead of having the ignore* members, wouldn't be simpler if you had has[d
I think we must have this see example at line547
PS6, Line 246: this.
> No need for 'this'. Plz remove.
Done
PS6, Line 316: getDbsMetadataParams
> I think we can just name it metadataParams for simplicity
Done
Line 327: PatternMatcher schemaPatternMatcher = getDbsMetadataParams.ignoreAllDbs
> I should use accesser.
Done
PS6, Line 342: functionName != null
> Why don't you just check if fnPatternMatcher is not null and remove L326?
see comment above.
PS6, Line 350: fe.getTableNames(
: db.getName(), "*", tablePatternMatcher, user)
> I thought the interface would be:
why? that way There will be a lot of duplicated code? we cannot call getTableNames(String dbName, String pattern, Sring user); in getTableNames(String dbName, PatternMatcher matcher, String user). we have to use most of the code in one another?
Line 360:
> Also, why can't we check here if we have a non-null columnPatternMatcher an
will do
Line 547: MetadataOpParams getDbsMetadataParams = MetadataOpParams.MetadataOpParamsBuilder()
if someone call this with tableName to be null, then we cannot differentiate that after we call getDbsMetadata if we do not explicit has those ignore*s.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 6
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 19: Code-Review+2
(3 comments)
thanks. carry Henry's +2
http://gerrit.cloudera.org:8080/#/c/3371/19/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
File fe/src/main/java/com/cloudera/impala/service/JniFrontend.java:
PS19, Line 210: Implement Hive's pattern-matching semantics for "SHOW TABLE [[LIKE] 'pattern']", and
: * return a list of table names matching an optional pattern.
: * The only metacharacters are '*' which matches any string of characters, and '|'
: * which denotes choice. Doing the work here saves loading tables or databases from the
: * metastore (which Hive would do if we passed the call through to the metastore
: * client). If the pattern is null, a
> This comment seems like it would be better on PatternMatcher.createHivePatt
this is exactly what "show table ..." uses. createHivePatternMatcher() can be called for many places, i.e. "show functions ..", "show databases ...". I think it is better to have an 1:1 matching to the corresponding sql statement. actually I think we should do this kind of comment on all such thrift api?
http://gerrit.cloudera.org:8080/#/c/3371/19/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
PS19, Line 227: matcheres
> matchers
Done
http://gerrit.cloudera.org:8080/#/c/3371/19/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
File fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java:
PS19, Line 1544: assertEquals(matchAllMatcher1, matchAllMatcher2);
> this test should confirm that the matchers have the same behaviour, not tha
Done. changed all occurrence to check behavior.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 19
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 21:
> It looks like GVM was run against the commit hash from patch set
> 20, which is why you got the +1 on it, instead of patch set 21
> along with a submit.
>
> http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge/2575/
I cannot access sandbox.jenkins.cloudera.com/* now. but according to my shell history, I submitted two gvms. both of them were against 'refs/changes/71/3371/21'. Did I miss anything?
GVM messages:
huaisi@huaisi-Desktop:~/Impala$ gerrit-verify-merge
Running: ssh -p 29418 HuaisiXu@gerrit.cloudera.org gerrit query --format JSON --current-patch-set c1a1762d00c14fa9d623a1d85918fbd0e1dc3c77
Parsing gerrit refspec from commit: c1a1762d00c14fa9d623a1d85918fbd0e1dc3c77
Launching Jenkins Job using GERRIT_REF_SPEC: 'refs/changes/71/3371/21' TARGET_BRANCH: 'cdh5-trunk'
Job submitted successfully. E-mail will be sent to: hxu@cloudera.com upon completion
Not sure why this failed to get merged by gvm.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 21
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 21: Code-Review+2
carry +2
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 21
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3371
to look at the new patch set (#16).
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Previously all code paths using getDbsMetadata() sufferred
unnecessary privilege checks:
1. Impala checked privilege of all databases, tables before
applying user provided JDBC pattern filters.
2. Impala passed a null pattern to getDbsMetadata() when
user did not provide one. However, null pattern is treated
as "%", which matches everything thereby causing unnecessary
privilege checks for catalog objects that are not in the
result set.
This patch translates null pattern to "match all" early so
that after change JDBC pattern matcher, it is able to safely
treat all nulls as "match nothing".
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
M fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/com/cloudera/impala/catalog/Db.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
M fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java
M fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
12 files changed, 287 insertions(+), 105 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/16
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 16
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 11:
(9 comments)
Almost done.
http://gerrit.cloudera.org:8080/#/c/3371/11//COMMIT_MSG
Commit Message:
PS11, Line 9: suffers
suffered
PS11, Line 15: . As a result, even though
: user wants nothing, we ended up checking everything.
"thereby causing unnecessary privilege checks for catalog objects that aren't in the result set."
PS11, Line 18: This patch changes getDbsMetadata()'s interface such that
: caller can pass addition information and it knows whether
: pattern is provided or not. This patch also applies filters
: before checks any privilege.
Maybe we can refine it a bit. How about something like "This patch modifies the behavior of getDbsMetadata() to avoid retrieving and checking for privileges of catalog entities that are of no interest to the user."
http://gerrit.cloudera.org:8080/#/c/3371/11/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java:
PS11, Line 325: String
nit: Strings
http://gerrit.cloudera.org:8080/#/c/3371/11/fe/src/main/java/com/cloudera/impala/service/Frontend.java
File fe/src/main/java/com/cloudera/impala/service/Frontend.java:
PS11, Line 597: if (matcher == null) return Collections.emptyList();
Is there a reason why we can't push that check into the catalog.getTableNames() fn? In that way, I believe every function that takes a PatternMatcher as input param will have consistent behavior. Right now, some functions expect a non-null matcher while others simply return an empty result set when the matcher is null.
PS11, Line 642: if (matcher == null) return Collections.emptyList();
Same comment as above.
http://gerrit.cloudera.org:8080/#/c/3371/11/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
Line 226: * Utility Class that represents the input parameters of getDbsMetadata() function call.
"a"
http://gerrit.cloudera.org:8080/#/c/3371/11/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
File fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java:
PS11, Line 34: pattern_
typo: patterns_
http://gerrit.cloudera.org:8080/#/c/3371/11/testdata/workloads/functional-query/queries/QueryTest/show.test
File testdata/workloads/functional-query/queries/QueryTest/show.test:
PS11, Line 157: This behaves different than Hive, see IMPALA-3744
"# Impala only considers '*' and '|' as meta-characters in SHOW statements (see IMPALA-3744). "
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 11
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 21:
What I remember from last night was the GIT_COMMIT_HASH of that build was c1a1762d00c14fa9d623a1d85918fbd0e1dc3c77 which is the commit hash of patch set 20. That's also the commit hash in your pasted output. That is derived from a local git rev-parse of your current IMPALA_HOME. This leads me to believe your IMPALA_HOME was c1a1762d00c14fa9d623a1d85918fbd0e1dc3c77 at the time of running gerrit-verify-merge.
I will investigate the confusing issue of the refspec printed being 21.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 21
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 11:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3371/11/fe/src/main/java/com/cloudera/impala/service/Frontend.java
File fe/src/main/java/com/cloudera/impala/service/Frontend.java:
PS11, Line 597: if (matcher == null) return Collections.emptyList();
> I did not think much about this.. for me it is the same. another reason I j
Yes, let's move it there if you don't mind. I understand that for getColumns we can't do something similar but it's fine.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 11
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3371/6/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
Line 327: PatternMatcher schemaPatternMatcher = getDbsMetadataParams.ignoreAllDbs
I should use accesser.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 6
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has submitted this change and it was merged.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Previously all code paths using getDbsMetadata() sufferred
unnecessary privilege checks:
1. Impala checked privilege of all databases, tables before
applying user provided JDBC pattern filters.
2. Impala passed a null pattern to getDbsMetadata() when
user did not provide one. However, null pattern is treated
as "%", which matches everything thereby causing unnecessary
privilege checks for catalog objects that are not in the
result set.
This patch creates PatternMatcher early so that user specified
null pattern is respected when calling getDbsMetadata().
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Reviewed-on: http://gerrit.cloudera.org:8080/3371
Reviewed-by: Huaisi Xu <hx...@cloudera.com>
Tested-by: Huaisi Xu <hx...@cloudera.com>
---
M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
M fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/com/cloudera/impala/catalog/Db.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
M fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java
M fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
12 files changed, 329 insertions(+), 143 deletions(-)
Approvals:
Huaisi Xu: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 22
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Hello Henry Robinson, Dimitris Tsirogiannis,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3371
to look at the new patch set (#20).
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Previously all code paths using getDbsMetadata() sufferred
unnecessary privilege checks:
1. Impala checked privilege of all databases, tables before
applying user provided JDBC pattern filters.
2. Impala passed a null pattern to getDbsMetadata() when
user did not provide one. However, null pattern is treated
as "%", which matches everything thereby causing unnecessary
privilege checks for catalog objects that are not in the
result set.
This patch creates PatternMatcher early so that user specified
null pattern is respected when calling getDbsMetadata().
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
M fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/com/cloudera/impala/catalog/Db.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
M fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java
M fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
12 files changed, 329 insertions(+), 143 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/20
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 20
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
Patch Set 4:
(7 comments)
I think we can fix the issues in the 2nd patch by modifying the Catalog.filterStringsByPattern() functions. Even though it may be correct, I don't think adding all these extra params in the getDbsMetadata is a good approach. It's really confusing and potentially error prone. Let's talk tomorrow.
http://gerrit.cloudera.org:8080/#/c/3371/3//COMMIT_MSG
Commit Message:
PS3, Line 7: Check privileges when necessary
I think this patch solves a bigger problem which is not accessing catalog objects that are not needed during HS2 API calls.
PS3, Line 9: Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
:
:
:
:
I think we can rephrase that a bit. I think you can simply mention that executing HS2 metadata operations in Impala caused unnecessary accesses of catalog objects, thereby resulting in excess authorization checks. Next, briefly describe how this patch addresses this issue.
http://gerrit.cloudera.org:8080/#/c/3371/3/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java:
PS3, Line 131: /**
: * Returns databases that match dbPattern. See filterStringsByPattern for details of
: * the pattern match semantics.
: *
: * dbPattern may be null (and thus matches everything).
: */
Can you update the comment?
PS3, Line 165: /**
: * Returns a list of tables in the supplied database that match
: * tablePattern. See filterStringsByPattern for details of the pattern match semantics.
: *
: * dbName must not be null, but tablePattern may be null (and thus matches
: * everything).
: *
: * Table names are returned unqualified.
: */
Update comment.
PS3, Line 336: empty string, no strings match.
: */
Update comment and everywhere else there is a change in the function signature.
http://gerrit.cloudera.org:8080/#/c/3371/3/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
PS3, Line 239: If functionName is not null, then only function metadata will be returned.
: * If tableName is null, then DbsTablesColumns.tableNames and DbsTablesColumns.columns
: * will not be populated.
: * If columns is null, then DbsTablesColumns.columns will not be populated.
Are these comments up to date?
PS3, Line 411: schemaNam
nit: database names. I don't think it returns the objects.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3371
to look at the new patch set (#18).
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Previously all code paths using getDbsMetadata() sufferred
unnecessary privilege checks:
1. Impala checked privilege of all databases, tables before
applying user provided JDBC pattern filters.
2. Impala passed a null pattern to getDbsMetadata() when
user did not provide one. However, null pattern is treated
as "%", which matches everything thereby causing unnecessary
privilege checks for catalog objects that are not in the
result set.
This patch creates PatternMatcher early so that user specified
null pattern is respected when calling getDbsMetadata().
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
M fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/com/cloudera/impala/catalog/Db.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
M fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java
M fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
12 files changed, 331 insertions(+), 142 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/18
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 18
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks when accessing catalog object
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks when accessing catalog object
......................................................................
Patch Set 9:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3371/9/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
Line 357: if (columnPatternMatcher == null) continue;
I will just remove this line.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 9
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
Patch Set 8:
(2 comments)
> (2 comments)
>
> Thanks Huaisi, it looks much better now. Can you please update all
> the comments? I will then do a final review.
Sorry I was on PTO yesterday.
http://gerrit.cloudera.org:8080/#/c/3371/7/fe/src/main/java/com/cloudera/impala/service/Frontend.java
File fe/src/main/java/com/cloudera/impala/service/Frontend.java:
PS7, Line 595: * Throw ImpalaException when dbName is null or error calling user.getShortName() in
: * hasAccess()
: */
: public List<String> getTableNames(String dbName, PatternMatcher tablePatternMatcher,
: User user) throws ImpalaException {
: if (tablePatternMatcher == null) return Collections.emptyList();
: List<String> tblNames = impaladCatalog_.ge
> We are not very consistent with our comments. Maybe remove Javadoc for now
Done
PS7, Line 605: String tblName = iter.next();
: PrivilegeRequest privilegeRequest = new Privi
> You can use this pattern instead here and everywhere else. emptyList() is a
Done
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 8
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 21:
> > > What I remember from last night was the GIT_COMMIT_HASH of that
> > > build was c1a1762d00c14fa9d623a1d85918fbd0e1dc3c77 which is the
> > > commit hash of patch set 20. That's also the commit hash in
> your
> > > pasted output. That is derived from a local git rev-parse of
> your
> > > current IMPALA_HOME. This leads me to believe your IMPALA_HOME
> > was
> > > c1a1762d00c14fa9d623a1d85918fbd0e1dc3c77 at the time of running
> > > gerrit-verify-merge.
> > >
> > > I will investigate the confusing issue of the refspec printed
> > being
> > > 21.
> >
> > I just checked the console output. The job was indeed build on
> > commit adea2350e2ad9e8e50db7ae86caa1d2502aeface not my local
> commit
> > since I rebased on Gerrit UI (this is what I used to do).
> >
> > So Jenkins failed to find the correct commit that it actually
> built
> > on. right?
> >
> > + git fetch --depth=1 https://gerrit.cloudera.org/Impala
> > refs/changes/71/3371/21
> > From https://gerrit.cloudera.org/Impala
> > * branch refs/changes/71/3371/21 -> FETCH_HEAD
> > + git rev-parse FETCH_HEAD
> > adea2350e2ad9e8e50db7ae86caa1d2502aeface
>
> and from the console output:
> HEAD is now at adea235... IMPALA-3711: Remove unnecessary privilege
> checks in getDbsMetadata()
Taking the rest of this private to avoid spam.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 21
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 16:
(1 comment)
Getting there.
http://gerrit.cloudera.org:8080/#/c/3371/16/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
File fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java:
PS16, Line 74: null pattern matches nothing while empty pattern matches all.
I really, really think it would be better to have class-wide constants that encode these meanings:
PatternMatcher.MATCH_ALL
PatternMAtcher.MATCH_NONE
They can be the same for both JDBC and Hive pattern matchers, because they can then be translated into the right regex by the builder
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 16
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 21:
> > It looks like GVM was run against the commit hash from patch set
> > 20, which is why you got the +1 on it, instead of patch set 21
> > along with a submit.
> >
> > http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge/2575/
>
> I cannot access sandbox.jenkins.cloudera.com/* now. but according
> to my shell history, I submitted two gvms. both of them were
> against 'refs/changes/71/3371/21'. Did I miss anything?
>
> GVM messages:
> huaisi@huaisi-Desktop:~/Impala$ gerrit-verify-merge
> Running: ssh -p 29418 HuaisiXu@gerrit.cloudera.org gerrit query
> --format JSON --current-patch-set c1a1762d00c14fa9d623a1d85918fbd0e1dc3c77
> Parsing gerrit refspec from commit: c1a1762d00c14fa9d623a1d85918fbd0e1dc3c77
> Launching Jenkins Job using GERRIT_REF_SPEC: 'refs/changes/71/3371/21'
> TARGET_BRANCH: 'cdh5-trunk'
> Job submitted successfully. E-mail will be sent to:
> hxu@cloudera.com upon completion
>
> Not sure why this failed to get merged by gvm.
From the message I got in shell, it ran for patch 21. Not sure why it commented for patch 20.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 21
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 21:
> What I remember from last night was the GIT_COMMIT_HASH of that
> build was c1a1762d00c14fa9d623a1d85918fbd0e1dc3c77 which is the
> commit hash of patch set 20. That's also the commit hash in your
> pasted output. That is derived from a local git rev-parse of your
> current IMPALA_HOME. This leads me to believe your IMPALA_HOME was
> c1a1762d00c14fa9d623a1d85918fbd0e1dc3c77 at the time of running
> gerrit-verify-merge.
>
> I will investigate the confusing issue of the refspec printed being
> 21.
I just checked the console output. The job was indeed build on commit adea2350e2ad9e8e50db7ae86caa1d2502aeface not my local commit since I rebased on Gerrit UI (this is what I used to do).
So Jenkins failed to find the correct commit that it actually built on. right?
+ git fetch --depth=1 https://gerrit.cloudera.org/Impala refs/changes/71/3371/21
From https://gerrit.cloudera.org/Impala
* branch refs/changes/71/3371/21 -> FETCH_HEAD
+ git rev-parse FETCH_HEAD
adea2350e2ad9e8e50db7ae86caa1d2502aeface
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 21
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
Patch Set 5:
Updated tests..
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 5
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 12:
(2 comments)
Thanks Dimitris. you know who can +2 on this?
http://gerrit.cloudera.org:8080/#/c/3371/11/fe/src/main/java/com/cloudera/impala/service/Frontend.java
File fe/src/main/java/com/cloudera/impala/service/Frontend.java:
PS11, Line 597: if (matcher == null) return Collections.emptyList();
> Yes, let's move it there if you don't mind. I understand that for getColumn
Done
http://gerrit.cloudera.org:8080/#/c/3371/12/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
PS12, Line 226: A u
> Oops sorry my bad, comment was misplaced. I meant either "a getDbsMetadata(
Done
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 12
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
Patch Set 4:
will talk offline.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 13:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3371/13/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java:
PS13, Line 175: if (matcher == null) return Collections.emptyList();
> Setting the pattern of the matcher to null is the problem we are trying to
I believe Henry's comment has broader implications than simply pushing the check to a different function. Even with all the changes that we went through in the last patches, the behavior is still a bit confusing, the reason being that in several cases we call some functions with a null PatternMatcher and we have extra logic to handle this case. What Henry suggests is essentially eliminating that case altogether. I.e. we never pass a null PatternMatcher. Instead, we encode that behavior (that corresponds to the null matcher) into the PatternMatcher's implementation. Henry plz correct me if I misunderstood your comment. Even though we may have to revert many of the changes you made in the last patches I believe it's a great idea.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 13
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Only check privilege when needed
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has uploaded a new patch set (#3).
Change subject: IMPALA-3711: Only check privilege when needed
......................................................................
IMPALA-3711: Only check privilege when needed
Previously getSchemas() checks privileges for all databases
and tables; getTables() checks for all columns, which can
lead to bad performance.
This patch avoids checking unnecessary privileges.
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
6 files changed, 52 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/3
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
Patch Set 8:
(25 comments)
http://gerrit.cloudera.org:8080/#/c/3371/8//COMMIT_MSG
Commit Message:
PS8, Line 7: Check privileges when necessary
> Maybe "Remove unnecessary privilege checks when accessing catalog objects"
Done
PS8, Line 13: user only request database name
> The description is a bit misleading. In general, requesting a db from the c
I will update this later. I need to think about this.
http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java:
PS8, Line 132: Returns databases using matcher
> "Returns all databases that match the pattern of 'matcher'." Similar commen
Done
PS8, Line 134: matcher must not be null.
> You don't enforce that. Add a Preconditions.checkNotNull() above L137 or re
Done
PS8, Line 172: tablePatternMatcher
> Just rename to "matcher".
Done
PS8, Line 359: Filter candidates.getName() using matcher
> "Return all catalog objects from 'candidates' that match the pattern of 'ma
Done
http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/catalog/Db.java
File fe/src/main/java/com/cloudera/impala/catalog/Db.java:
PS8, Line 17: *
> Please don't do that. We don't need everything from util.
yeah I noticed this.. my IDE changed this..
PS8, Line 427: Returns all functions using fnPatternMatcher
> "Returns all functions that match the pattern of 'matcher'. if 'matcher' is
Done
http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/service/Frontend.java
File fe/src/main/java/com/cloudera/impala/service/Frontend.java:
PS8, Line 592: Match tables in database dbName with tablePatternMatcher and check user's privileges.
> "Returns all tables in database 'dbName' that match the pattern of 'matcher
Done
PS8, Line 595: Throw ImpalaException when dbName is null
> Where does this happen? Also when describing the conditions during which an
ok. just removed. it happens in getTableNames(dbName, matcher).
PS8, Line 598: tablePatternMatcher
> Let's rename all these variables to 'matcher' unless there are multiple ins
Done
PS8, Line 624: List<Column> columns = Lists.newArrayList();
: if (columnPatternMatcher == null) return columns;
> swap these two lines and return Collections.emptyList();
Done
http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
File fe/src/main/java/com/cloudera/impala/service/JniFrontend.java:
PS8, Line 211: params
> Plz don't reference variable names from the body of this function in the fu
Done
PS8, Line 266: params
> Same comment as above.
Done
http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
PS8, Line 226: Class contains all information needed by getMetadataOp().
> "Utility class that represents the input parameters of getDbsMetadata() fun
Done
PS8, Line 287: JDBC search patterns
> You may want to add a ref to the PatternMatcher class so that the reader ca
Done
PS8, Line 287: metadataParams
> 'metadataParams'.
Done
PS8, Line 286: the
: * search pattern
> "the associated search patterns"
Done
PS8, Line 289: The return value result.dbs contains the list of databases that satisfy
: * the "dbPattern_" search pattern.
: * result.tableNames[i] contains the list of tables inside dbs[i] that satisfy
: * the "tablePattern_" search pattern.
: * result.columns[i][j] contains the list of columns of table[j] in dbs[i]
: * that satisfy the search condition "columnPattern_".
: * result.functions[i] contains the list of functions inside dbs[i] that
: * satisfy the "functionPattern_" search pattern.
> You need to update this comment. e.g. "dbPattern_" doesn't even show up in
I was referring to members in metadataParams. Done.
PS8, Line 348: if (columnPatternMatcher == null) continue;
> I don't think that is correct here. You'll never populate the missingTbls u
Agree.. I changed the behavior of this function. thanks!
http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
File fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java:
Line 1543:
> Maybe also add a call to getDbs where the pattern matches a db for which th
Done
PS8, Line 1626: same as "%"
> Also indicate what that means (.e.g. matches all).
Done
PS8, Line 1641: Pattern ".*" or "." works as well
> Mention what that matches. Here and everywhere else
Done
PS8, Line 1657: "_" should work
> Not very descriptive comment :)
Done
http://gerrit.cloudera.org:8080/#/c/3371/8/testdata/workloads/functional-query/queries/QueryTest/show.test
File testdata/workloads/functional-query/queries/QueryTest/show.test:
PS8, Line 157: # This behaves different than Hive, see IMPALA-3744
: show tables in functional like 'alltypesag.'
: ---- RESULTS
: ---- TYPES
: STRING
: ====
: ---- QUERY
: show tables in functional like 'alltypesag.*'
: ---- RESULTS
: ---- TYPES
: STRING
: ====
: ---- QUERY
: show tables in functional like 'alltypesagg%'
: ---- RESULTS
: ---- TYPES
: STRING
: ====
: ---- QUERY
: show tables in functional like 'alltypesag_'
: ---- RESULTS
: ---- TYPES
: STRING
: ====
: ---- QUERY
> What are we testing with these cases?
I just want to fix the behavior of our end-to-end result. Unit test is only for integrity of those particular methods, and they work as expected. but end-to-end is not necessary. Hive' behavior is different.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 8
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
Patch Set 8:
(25 comments)
http://gerrit.cloudera.org:8080/#/c/3371/8//COMMIT_MSG
Commit Message:
PS8, Line 7: Check privileges when necessary
Maybe "Remove unnecessary privilege checks when accessing catalog objects"
PS8, Line 13: user only request database name
The description is a bit misleading. In general, requesting a db from the catalog didn't have the same effect, no? Let's try to improve the description so that it is clear what problem this commit solves.
http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java:
PS8, Line 132: Returns databases using matcher
"Returns all databases that match the pattern of 'matcher'." Similar comment for all the other functions.
PS8, Line 134: matcher must not be null.
You don't enforce that. Add a Preconditions.checkNotNull() above L137 or remove the comment if that condition is enforced in fileCatalogObjectsByPatterrn. Same for any other function that makes that assumption.
PS8, Line 172: tablePatternMatcher
Just rename to "matcher".
PS8, Line 359: Filter candidates.getName() using matcher
"Return all catalog objects from 'candidates' that match the pattern of 'matcher'. If 'matcher' doesn't have a pattern, all catalog objects in 'candidates' are returned. The results are sorted in CATALOG_OBJECT_ORDER.
http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/catalog/Db.java
File fe/src/main/java/com/cloudera/impala/catalog/Db.java:
PS8, Line 17: *
Please don't do that. We don't need everything from util.
PS8, Line 427: Returns all functions using fnPatternMatcher
"Returns all functions that match the pattern of 'matcher'. if 'matcher' is null, return an empty list."
http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/service/Frontend.java
File fe/src/main/java/com/cloudera/impala/service/Frontend.java:
PS8, Line 592: Match tables in database dbName with tablePatternMatcher and check user's privileges.
"Returns all tables in database 'dbName' that match the pattern of 'matcher' and are accessible to the given user. Returns an empty list If 'matcher' is null."
PS8, Line 595: Throw ImpalaException when dbName is null
Where does this happen? Also when describing the conditions during which an exception is thrown you don't need to be that specific. You may remove this comment altogether, it's not that interesting.
PS8, Line 598: tablePatternMatcher
Let's rename all these variables to 'matcher' unless there are multiple instances that we need to differentiate.
PS8, Line 624: List<Column> columns = Lists.newArrayList();
: if (columnPatternMatcher == null) return columns;
swap these two lines and return Collections.emptyList();
http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
File fe/src/main/java/com/cloudera/impala/service/JniFrontend.java:
PS8, Line 211: params
Plz don't reference variable names from the body of this function in the function comment. I had to look inside the function to figure out what params is.
PS8, Line 266: params
Same comment as above.
http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
PS8, Line 226: Class contains all information needed by getMetadataOp().
"Utility class that represents the input parameters of getDbsMetadata() function calls."
PS8, Line 286: the
: * search pattern
"the associated search patterns"
PS8, Line 287: metadataParams
'metadataParams'.
PS8, Line 287: JDBC search patterns
You may want to add a ref to the PatternMatcher class so that the reader can see what a JDBC pattern matcher does.
PS8, Line 289: The return value result.dbs contains the list of databases that satisfy
: * the "dbPattern_" search pattern.
: * result.tableNames[i] contains the list of tables inside dbs[i] that satisfy
: * the "tablePattern_" search pattern.
: * result.columns[i][j] contains the list of columns of table[j] in dbs[i]
: * that satisfy the search condition "columnPattern_".
: * result.functions[i] contains the list of functions inside dbs[i] that
: * satisfy the "functionPattern_" search pattern.
You need to update this comment. e.g. "dbPattern_" doesn't even show up in this function anymore.
PS8, Line 348: if (columnPatternMatcher == null) continue;
I don't think that is correct here. You'll never populate the missingTbls unless a column pattern is specified.
http://gerrit.cloudera.org:8080/#/c/3371/8/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
File fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java:
Line 1543:
Maybe also add a call to getDbs where the pattern matches a db for which the user doesn't have any permissions on, hence an empty list is returned.
PS8, Line 1626: same as "%"
Also indicate what that means (.e.g. matches all).
PS8, Line 1641: Pattern ".*" or "." works as well
Mention what that matches. Here and everywhere else
PS8, Line 1657: "_" should work
Not very descriptive comment :)
http://gerrit.cloudera.org:8080/#/c/3371/8/testdata/workloads/functional-query/queries/QueryTest/show.test
File testdata/workloads/functional-query/queries/QueryTest/show.test:
PS8, Line 157: # This behaves different than Hive, see IMPALA-3744
: show tables in functional like 'alltypesag.'
: ---- RESULTS
: ---- TYPES
: STRING
: ====
: ---- QUERY
: show tables in functional like 'alltypesag.*'
: ---- RESULTS
: ---- TYPES
: STRING
: ====
: ---- QUERY
: show tables in functional like 'alltypesagg%'
: ---- RESULTS
: ---- TYPES
: STRING
: ====
: ---- QUERY
: show tables in functional like 'alltypesag_'
: ---- RESULTS
: ---- TYPES
: STRING
: ====
: ---- QUERY
What are we testing with these cases?
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 8
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 15:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3371/13/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java:
PS13, Line 175: }
> Yes, that's exactly right, thanks! I'm sorry if this undoes some of the pre
Done
http://gerrit.cloudera.org:8080/#/c/3371/15/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
Line 332: if (schemaName == null) schemaName = "%";
not sure whether/how to comment this.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 15
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 20: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 20
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 20: Verified-1
Build failed: http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge/2574/
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 20
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 19: Code-Review+2
(3 comments)
Looks good, thanks Huaisi.
http://gerrit.cloudera.org:8080/#/c/3371/19/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
File fe/src/main/java/com/cloudera/impala/service/JniFrontend.java:
PS19, Line 210: Implement Hive's pattern-matching semantics for "SHOW TABLE [[LIKE] 'pattern']", and
: * return a list of table names matching an optional pattern.
: * The only metacharacters are '*' which matches any string of characters, and '|'
: * which denotes choice. Doing the work here saves loading tables or databases from the
: * metastore (which Hive would do if we passed the call through to the metastore
: * client). If the pattern is null, a
This comment seems like it would be better on PatternMatcher.createHivePatternMatcher()
http://gerrit.cloudera.org:8080/#/c/3371/19/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
PS19, Line 227: matcheres
matchers
http://gerrit.cloudera.org:8080/#/c/3371/19/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
File fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java:
PS19, Line 1544: assertEquals(matchAllMatcher1, matchAllMatcher2);
this test should confirm that the matchers have the same behaviour, not that they refer to the same object (that's an implementation detail, not a guarantee made by the API).
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 19
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 15:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3371/15/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
File fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java:
PS15, Line 74: * Null pattern's matching is controlled by 'isNullMatchAll', where if it is set then
: * null matches everything; unset otherwise.
> if I understand correctly. this depends on we have a pattern that matches n
So why does this extra parameter exist, if it's never used?
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 15
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
Patch Set 7:
I am aware that some comments are outdated.. I will update all comments once you think the code is ok.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 7
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
Patch Set 6:
(2 comments)
sorry for being slow on this.. :)
http://gerrit.cloudera.org:8080/#/c/3371/6/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
PS6, Line 350: fe.getTableNames(
: db.getName(), "*", tablePatternMatcher, user)
> I don't think there will be any duplicate code. getTableNames that takes as
ok I understand.thx
Line 547: MetadataOpParams getDbsMetadataParams = MetadataOpParams.MetadataOpParamsBuilder()
> ignoreAllColumns(true) is equivalent to hasColumnPattern(false). Irrespecti
I see.. thanks!
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 6
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 10:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/3371/9/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java:
PS9, Line 235: using matcher
> "that match the pattern of 'matcher'."
Done
PS9, Line 238: matcher
> nit: 'matcher'; good practice to use single quotes when referring to input
I will just remove this since it is not enforced here. I missed this part in previous patch.
PS9, Line 328: candidats
> typo: candidates
Done
http://gerrit.cloudera.org:8080/#/c/3371/9/fe/src/main/java/com/cloudera/impala/service/Frontend.java
File fe/src/main/java/com/cloudera/impala/service/Frontend.java:
PS9, Line 614: columnPatternMatcher
> update
Done
PS9, Line 615: columnPatternMatcher
> 'matcher'
Done
http://gerrit.cloudera.org:8080/#/c/3371/9/fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
File fe/src/main/java/com/cloudera/impala/service/JniFrontend.java:
PS9, Line 210: Implement Hive's pattern-matching semantics for "SHOW TABLE [[LIKE] 'pattern']".
: * The only metacharacters are '*' which matches any string of characters, and '|'
: * which denotes choice. Doing the work here saves loading tables or databases from the
: * metastore (which Hive would do if we passed the call through to the metastore
: * client). If the pattern is null, all strings are considered to match. If it is an
: * empty string, no strings match.
> Can you move this below L220? The first sentence of the function comment sh
Done
PS9, Line 265: Implement Hive's pattern-matching semantics for "SHOW DATABASES [[LIKE] 'pattern']".
: * The only metacharacters are '*' which matches any string of characters, and '|' which
: * denotes choice. Doing the work here saves loading tables or databases from the
: * metastore (which Hive would do if we passed the call through to the metastore
: * client). If the pattern is null, all strings are considered to match. If it is an
: * empty string, no strings match.
> This is kind of repetitive. You may want to keep the first sentence, move i
Done
http://gerrit.cloudera.org:8080/#/c/3371/9/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
PS9, Line 226: getMetadataOp
> There is no such thing :)
- -!
PS9, Line 302: If metadataParams.tablePattern_ is null, then result.tableNames and result.columns
: * will not be populated.
: * If metadataParams.columnPattern_ is null, then result.columns will not be populated.
> That's not correct. tablePattern_ could be set to null which indicates that
Done
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 10
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 21:
> > What I remember from last night was the GIT_COMMIT_HASH of that
> > build was c1a1762d00c14fa9d623a1d85918fbd0e1dc3c77 which is the
> > commit hash of patch set 20. That's also the commit hash in your
> > pasted output. That is derived from a local git rev-parse of your
> > current IMPALA_HOME. This leads me to believe your IMPALA_HOME
> was
> > c1a1762d00c14fa9d623a1d85918fbd0e1dc3c77 at the time of running
> > gerrit-verify-merge.
> >
> > I will investigate the confusing issue of the refspec printed
> being
> > 21.
>
> I just checked the console output. The job was indeed build on
> commit adea2350e2ad9e8e50db7ae86caa1d2502aeface not my local commit
> since I rebased on Gerrit UI (this is what I used to do).
>
> So Jenkins failed to find the correct commit that it actually built
> on. right?
>
> + git fetch --depth=1 https://gerrit.cloudera.org/Impala
> refs/changes/71/3371/21
> From https://gerrit.cloudera.org/Impala
> * branch refs/changes/71/3371/21 -> FETCH_HEAD
> + git rev-parse FETCH_HEAD
> adea2350e2ad9e8e50db7ae86caa1d2502aeface
and from the console output:
HEAD is now at adea235... IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 21
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Only check privilege for databases in getSchemas()
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-3711: Only check privilege for databases in getSchemas()
......................................................................
Patch Set 1:
As we discussed offline, the implementation is quite confusing. We seem to be evaluating the search patterns in three different places (MetadataOp, Frontend and Catalog), depending on the catalog object type. Let's try to simplify the code and have consistent ways of getting catalog objects and checking their privileges. Thanks for clearing our mess :)
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
Patch Set 6:
(1 comment)
bump
http://gerrit.cloudera.org:8080/#/c/3371/6/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
Line 226: * Class contains all information needed by getMetadataOp().
Just bump...
I can add more comment once you think this is what you want...
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 6
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has uploaded a new patch set (#5).
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
IMPALA-3711: Check privileges when necessary
Previously
1. impala checks all columns privileges when user
only request table name
2. impala checks all table+column privileges when
user only request database name
This patch prevent checking unnecessary privileges.
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
5 files changed, 203 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/5
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 5
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3371
to look at the new patch set (#17).
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Previously all code paths using getDbsMetadata() sufferred
unnecessary privilege checks:
1. Impala checked privilege of all databases, tables before
applying user provided JDBC pattern filters.
2. Impala passed a null pattern to getDbsMetadata() when
user did not provide one. However, null pattern is treated
as "%", which matches everything thereby causing unnecessary
privilege checks for catalog objects that are not in the
result set.
This patch translates null pattern to "match all" early so
that after change JDBC pattern matcher, it is able to safely
treat all nulls as "match nothing".
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
M fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/com/cloudera/impala/catalog/Db.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
M fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java
M fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
12 files changed, 306 insertions(+), 119 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/17
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 17
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks when accessing catalog object
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has uploaded a new patch set (#9).
Change subject: IMPALA-3711: Remove unnecessary privilege checks when accessing catalog object
......................................................................
IMPALA-3711: Remove unnecessary privilege checks when
accessing catalog object
Previously
1. impala checks all columns privileges when user
only request table name
2. impala checks all table+column privileges when
user only request database name with Get_Schema(),
e.g. Hue.
This patch prevent checking unnecessary privileges.
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
M fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/com/cloudera/impala/catalog/Db.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
M fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java
M fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
12 files changed, 383 insertions(+), 112 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/9
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 9
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 12: Code-Review+1
(1 comment)
Thanks Huaisi. Good job.
http://gerrit.cloudera.org:8080/#/c/3371/12/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
PS12, Line 226: A u
Oops sorry my bad, comment was misplaced. I meant either "a getDbsMetadata()" or "getDbsMetadata function calls.".
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 12
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 15:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3371/15/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
File fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java:
PS15, Line 74: * Null pattern's matching is controlled by 'isNullMatchAll', where if it is set then
: * null matches everything; unset otherwise.
> So why does this extra parameter exist, if it's never used?
ok I will remove it...
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 15
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 11:
(9 comments)
sorry was having a team lunch.
http://gerrit.cloudera.org:8080/#/c/3371/11//COMMIT_MSG
Commit Message:
PS11, Line 9: suffers
> suffered
Done
PS11, Line 15: . As a result, even though
: user wants nothing, we ended up checking everything.
> "thereby causing unnecessary privilege checks for catalog objects that aren
Done
PS11, Line 18: This patch changes getDbsMetadata()'s interface such that
: caller can pass addition information and it knows whether
: pattern is provided or not. This patch also applies filters
: before checks any privilege.
> Maybe we can refine it a bit. How about something like "This patch modifies
Done
http://gerrit.cloudera.org:8080/#/c/3371/11/fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
File fe/src/main/java/com/cloudera/impala/catalog/Catalog.java:
PS11, Line 325: String
> nit: Strings
Done
http://gerrit.cloudera.org:8080/#/c/3371/11/fe/src/main/java/com/cloudera/impala/service/Frontend.java
File fe/src/main/java/com/cloudera/impala/service/Frontend.java:
PS11, Line 597: if (matcher == null) return Collections.emptyList();
> Is there a reason why we can't push that check into the catalog.getTableNam
I did not think much about this.. for me it is the same. another reason I just thought of is that we cannot do that in getColumns() since it matches everything itself. So the idea maybe catalog.java does not take null matcher?
I can definitely move that if you insist.
PS11, Line 642: if (matcher == null) return Collections.emptyList();
> Same comment as above.
see above
http://gerrit.cloudera.org:8080/#/c/3371/11/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
Line 226: * Utility Class that represents the input parameters of getDbsMetadata() function call.
> "a"
Done
http://gerrit.cloudera.org:8080/#/c/3371/11/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
File fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java:
PS11, Line 34: pattern_
> typo: patterns_
Done
http://gerrit.cloudera.org:8080/#/c/3371/11/testdata/workloads/functional-query/queries/QueryTest/show.test
File testdata/workloads/functional-query/queries/QueryTest/show.test:
PS11, Line 157: This behaves different than Hive, see IMPALA-3744
> "# Impala only considers '*' and '|' as meta-characters in SHOW statements
Done
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 11
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 21: Verified+1
Michael and I agree that the gvm was verifying patch set 21 (rebased) and it was a gvm issue that it commented on patch 20.
So I will manually merge this! thanks for looking at this Michael. thanks for review Henry and Dimitris!
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 21
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Only check privilege when needed
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has uploaded a new patch set (#2).
Change subject: IMPALA-3711: Only check privilege when needed
......................................................................
IMPALA-3711: Only check privilege when needed
Previously getSchemas() checks privileges for all databases
and tables; getTables() checks for all columns, which can
lead to bad performance.
This patch avoids checking unnecessary privileges.
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
6 files changed, 53 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/2
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 2
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 18:
(10 comments)
Thanks! used similar implementation as java's immutable Collections.EMPTY_LIST
http://gerrit.cloudera.org:8080/#/c/3371/18/fe/src/main/java/com/cloudera/impala/catalog/Db.java
File fe/src/main/java/com/cloudera/impala/catalog/Db.java:
PS18, Line 432: If matcher is null, return an empty list.
> will do
Done
Line 436: if (matcher == null) return Collections.emptyList();
> will do checknotnull(matcher) after review.
Done
http://gerrit.cloudera.org:8080/#/c/3371/18/fe/src/main/java/com/cloudera/impala/service/Frontend.java
File fe/src/main/java/com/cloudera/impala/service/Frontend.java:
Line 593: * accessible to 'user'. Returns an empty list if 'matcher' is null.
updated
Line 619: if (matcher == null) return Collections.emptyList();
> will do checknotnull(matcher) after review
Done
Line 637: * accessible to 'user'. Returns an empty list if 'matcher' is null.
updated
http://gerrit.cloudera.org:8080/#/c/3371/18/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
Line 22: import org.apache.hadoop.yarn.webapp.hamlet.HamletSpec;
> will remove after review
Done
Line 228: * associated search patterns in 'metadataParams'. All patterns are treated as JDBC
> will update after review.
Done
http://gerrit.cloudera.org:8080/#/c/3371/18/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
File fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java:
PS18, Line 46: public static final PatternMatcher MATCHER_MATCH_ALL = new PatternMatcher();
: public static final PatternMatcher MATCHER_MATCH_NONE;
: static {
: MATCHER_MATCH_NONE = new PatternMatcher();
: MATCHER_MATCH_NONE.patterns_ = Collections.emptyList();
: }
> That's the point: the class implementation needs to stay read-only.
Done
http://gerrit.cloudera.org:8080/#/c/3371/18/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
File fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java:
Line 31: import org.apache.hadoop.yarn.webapp.hamlet.HamletSpec;
> will remove after review.
Done
Line 1581: PatternMatcher.createHivePatternMatcher(null), USER);
> will remove after review.
Done
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 18
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 18:
(3 comments)
thanks. could you answer my reply in patternMatcher.java?
http://gerrit.cloudera.org:8080/#/c/3371/18/fe/src/main/java/com/cloudera/impala/catalog/Db.java
File fe/src/main/java/com/cloudera/impala/catalog/Db.java:
PS18, Line 432: If matcher is null, return an empty list.
> Update comment as well?
will do
http://gerrit.cloudera.org:8080/#/c/3371/18/fe/src/main/java/com/cloudera/impala/service/Frontend.java
File fe/src/main/java/com/cloudera/impala/service/Frontend.java:
PS18, Line 614: If 'matcher' is null, returns an empty list.
> Update comment
will do
http://gerrit.cloudera.org:8080/#/c/3371/18/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
File fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java:
PS18, Line 46: public static final PatternMatcher MATCHER_MATCH_ALL = new PatternMatcher();
: public static final PatternMatcher MATCHER_MATCH_NONE;
: static {
: MATCHER_MATCH_NONE = new PatternMatcher();
: MATCHER_MATCH_NONE.patterns_ = Collections.emptyList();
: }
> One problem here is that this requires that PatternMatcher remains thread s
Why this is not thread safe? this is constructed on process start up right? any access later will refer to the same read-only reference?
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 18
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Only check privilege for databases in getSchemas()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Only check privilege for databases in getSchemas()
......................................................................
Patch Set 1:
The behavior of getDbsMetadata() is not changed right?
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has uploaded a new patch set (#8).
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
IMPALA-3711: Check privileges when necessary
Previously
1. impala checks all columns privileges when user
only request table name
2. impala checks all table+column privileges when
user only request database name
This patch prevent checking unnecessary privileges.
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
M fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/com/cloudera/impala/catalog/Db.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
M fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java
M fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
12 files changed, 380 insertions(+), 122 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/8
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 8
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 21:
forgot switch aux-test
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 21
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3371
to look at the new patch set (#19).
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Previously all code paths using getDbsMetadata() sufferred
unnecessary privilege checks:
1. Impala checked privilege of all databases, tables before
applying user provided JDBC pattern filters.
2. Impala passed a null pattern to getDbsMetadata() when
user did not provide one. However, null pattern is treated
as "%", which matches everything thereby causing unnecessary
privilege checks for catalog objects that are not in the
result set.
This patch creates PatternMatcher early so that user specified
null pattern is respected when calling getDbsMetadata().
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
M fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/com/cloudera/impala/catalog/Db.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
M fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java
M fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
12 files changed, 333 insertions(+), 144 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/19
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 19
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Only check privilege for databases in getSchemas()
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-3711: Only check privilege for databases in getSchemas()
......................................................................
Patch Set 1:
I don't think these changes are correct. I believe the HS2 API specifies that getSchemas(), getTables(), etc, should be able to accept a search pattern as an input parameter. You seem to remove that functionality altogether, but this is not what we want.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 1
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 18:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/3371/18/fe/src/main/java/com/cloudera/impala/catalog/Db.java
File fe/src/main/java/com/cloudera/impala/catalog/Db.java:
Line 436: if (matcher == null) return Collections.emptyList();
will do checknotnull(matcher) after review.
http://gerrit.cloudera.org:8080/#/c/3371/18/fe/src/main/java/com/cloudera/impala/service/Frontend.java
File fe/src/main/java/com/cloudera/impala/service/Frontend.java:
Line 619: if (matcher == null) return Collections.emptyList();
will do checknotnull(matcher) after review
http://gerrit.cloudera.org:8080/#/c/3371/18/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
Line 22: import org.apache.hadoop.yarn.webapp.hamlet.HamletSpec;
will remove after review
Line 228: * associated search patterns in 'metadataParams'. All patterns are treated as JDBC
will update after review.
http://gerrit.cloudera.org:8080/#/c/3371/18/fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
File fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java:
Line 31: import org.apache.hadoop.yarn.webapp.hamlet.HamletSpec;
will remove after review.
Line 1581: PatternMatcher.createHivePatternMatcher(null), USER);
will remove after review.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 18
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
Patch Set 6:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/3371/6/fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
File fe/src/main/java/com/cloudera/impala/service/MetadataOp.java:
PS6, Line 229: catalogName
private member should end with a '_'.
PS6, Line 230: private boolean ignoreAllDbs;
: private boolean ignoreAllTables;
: private boolean ignoreAllColumns;
: private boolean ignoreAllFunctions;
Instead of having the ignore* members, wouldn't be simpler if you had has[db|table|column|function]Pattern members instead? These would be false by default unless the corresponding set* function is called. i.e. setDbPattern wouldn't just set the dbPattern value but also set the hasDbPattern to true.
PS6, Line 246: this.
No need for 'this'. Plz remove.
PS6, Line 316: getDbsMetadataParams
I think we can just name it metadataParams for simplicity
PS6, Line 342: functionName != null
Why don't you just check if fnPatternMatcher is not null and remove L326?
PS6, Line 350: fe.getTableNames(
: db.getName(), "*", tablePatternMatcher, user)
I thought the interface would be:
Frontend.getTableNames(String dbName, PatternMatcher matcher, String user) and
Frontend.getTableNames(String dbName, String pattern, Sring user);
Similarly for the Catalog.getTableNames().
Line 360:
Also, why can't we check here if we have a non-null columnPatternMatcher and avoid block L361-369?
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 6
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has uploaded a new patch set (#4).
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
IMPALA-3711: Check privileges when necessary
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
4 files changed, 74 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/4
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 4
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Hello Dimitris Tsirogiannis,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/3371
to look at the new patch set (#13).
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Previously all code paths using getDbsMetadata() sufferred
unnecessary privilege checks:
1. Impala checked privilege of all databases, tables before
applying user provided JDBC pattern filters.
2. Impala passed a null pattern to getDbsMetadata() when
user did not provide one. However, null pattern is treated
as "%", which matches everything thereby causing unnecessary
privilege checks for catalog objects that are not in the
result set.
This patch modifies the behavior of getDbsMetadata() to
avoid retrieving and checking for privileges of catalog
entities that are of no interest to the user.
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
M fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/com/cloudera/impala/catalog/Db.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
M fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java
M fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
12 files changed, 383 insertions(+), 115 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/13
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 13
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has uploaded a new patch set (#7).
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
IMPALA-3711: Check privileges when necessary
Previously
1. impala checks all columns privileges when user
only request table name
2. impala checks all table+column privileges when
user only request database name
This patch prevent checking unnecessary privileges.
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
M fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/com/cloudera/impala/catalog/Db.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
M fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java
M fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
12 files changed, 343 insertions(+), 65 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/7
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 7
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
[Impala-CR](cdh5-trunk) IMPALA-3711: Only check privilege when needed
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Only check privilege when needed
......................................................................
Patch Set 3:
I will update comments. but code change is ready for review..
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 18:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/3371/18/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
File fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java:
PS18, Line 46: public static final PatternMatcher MATCHER_MATCH_ALL = new PatternMatcher();
: public static final PatternMatcher MATCHER_MATCH_NONE;
: static {
: MATCHER_MATCH_NONE = new PatternMatcher();
: MATCHER_MATCH_NONE.patterns_ = Collections.emptyList();
: }
> Why this is not thread safe? this is constructed on process start up right?
That's the point: the class implementation needs to stay read-only.
If you want to go this route, I would add some more defensive checks: make patterns_ final and an ImmutableList. Add a class comment saying that the implementation must be thread safe.
You might consider, instead, subclassing PatternMatcher with a MatchAllMAtcher class, whose implementation of matches() is "return true".
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 18
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Dimitris Tsirogiannis (Code Review)" <ge...@cloudera.org>.
Dimitris Tsirogiannis has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
Patch Set 7:
(2 comments)
Thanks Huaisi, it looks much better now. Can you please update all the comments? I will then do a final review.
http://gerrit.cloudera.org:8080/#/c/3371/7/fe/src/main/java/com/cloudera/impala/service/Frontend.java
File fe/src/main/java/com/cloudera/impala/service/Frontend.java:
PS7, Line 595: * @param dbName database to search for tablePattern
: * @param tablePatternMatcher matcher used to match tables in dbName
: * @param user user used to check privileges
: * @return a list of tables names filtered by matcher and user has
: * accessing privileges
: * @throws ImpalaException when dbName is null or error calling user.getShortName()
: * in hasAccess()
We are not very consistent with our comments. Maybe remove Javadoc for now since we don't seem to use it anywhere else in this file.
PS7, Line 605: List<String> tblNames = Lists.newArrayList();
: if (tablePatternMatcher == null) return tblNames;
You can use this pattern instead here and everywhere else. emptyList() is a bit cheaper to construct and is immutable.
if (tablePatternMatcher == null) return List.<String>emptyList();
List<String> tblNames = Lists.newArrayList();
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 7
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 13: Code-Review+1
carry Dimitris' +1...
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 13
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Check privileges when necessary
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Check privileges when necessary
......................................................................
Patch Set 5:
New patch with builder pattern.(not sure if this is what you want @Dimitris)
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 5
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: No
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 15:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3371/15/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
File fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java:
PS15, Line 74: * Null pattern's matching is controlled by 'isNullMatchAll', where if it is set then
: * null matches everything; unset otherwise.
> We should be able to eliminate this parameter. Users of this API should alw
if I understand correctly. this depends on we have a pattern that matches none, but we do not before this patch. there is simply no pattern string that matches none in the old code: both null and empty are treated as match all.
PS15, Line 78: isNullMatchAll
> can you point out where this is set to true?
It is not set to true in any place. You think we can just remove this option? I do not know if this is a standard in JDBC pattern.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 15
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 15:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/3371/15/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
File fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java:
PS15, Line 74: * Null pattern's matching is controlled by 'isNullMatchAll', where if it is set then
: * null matches everything; unset otherwise.
We should be able to eliminate this parameter. Users of this API should always pass in a correct pattern. Since the caller will know if nulls should match all or not, they can do something like:
// If nulls should match all
createJdbcPatternMatcher(pattern == null ? : PatternMatcher.MATCH_ALL_PATTERN : pattern);
// If nulls should match none
createJdbcPatternMatcher(pattern = null ? PatternMatcher.MATCH_NONE_PATTERN : pattern);
PS15, Line 78: isNullMatchAll
can you point out where this is set to true?
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 15
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
Patch Set 18:
(3 comments)
Getting very close. Please make the changes you've highlighted as well.
http://gerrit.cloudera.org:8080/#/c/3371/18/fe/src/main/java/com/cloudera/impala/catalog/Db.java
File fe/src/main/java/com/cloudera/impala/catalog/Db.java:
PS18, Line 432: If matcher is null, return an empty list.
Update comment as well?
http://gerrit.cloudera.org:8080/#/c/3371/18/fe/src/main/java/com/cloudera/impala/service/Frontend.java
File fe/src/main/java/com/cloudera/impala/service/Frontend.java:
PS18, Line 614: If 'matcher' is null, returns an empty list.
Update comment
http://gerrit.cloudera.org:8080/#/c/3371/18/fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
File fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java:
PS18, Line 46: public static final PatternMatcher MATCHER_MATCH_ALL = new PatternMatcher();
: public static final PatternMatcher MATCHER_MATCH_NONE;
: static {
: MATCHER_MATCH_NONE = new PatternMatcher();
: MATCHER_MATCH_NONE.patterns_ = Collections.emptyList();
: }
One problem here is that this requires that PatternMatcher remains thread safe (as many threads may use these objects).
I think it's better to have:
static PatternMatcher createMatchAllMatcher();
OR
static string PatternMatcher MATCH_ALL_PATTERN = "<...something...>";
given this patch, I think the first is better.
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 18
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-CR](cdh5-trunk) IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Posted by "Huaisi Xu (Code Review)" <ge...@cloudera.org>.
Huaisi Xu has uploaded a new patch set (#12).
Change subject: IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
......................................................................
IMPALA-3711: Remove unnecessary privilege checks in getDbsMetadata()
Previously all code paths using getDbsMetadata() sufferred
unnecessary privilege checks:
1. Impala checked privilege of all databases, tables before
applying user provided JDBC pattern filters.
2. Impala passed a null pattern to getDbsMetadata() when
user did not provide one. However, null pattern is treated
as "%", which matches everything thereby causing unnecessary
privilege checks for catalog objects that are not in the
result set.
This patch modifies the behavior of getDbsMetadata() to
avoid retrieving and checking for privileges of catalog
entities that are of no interest to the user.
Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
---
M fe/src/main/java/com/cloudera/impala/catalog/Catalog.java
M fe/src/main/java/com/cloudera/impala/catalog/CatalogServiceCatalog.java
M fe/src/main/java/com/cloudera/impala/catalog/Db.java
M fe/src/main/java/com/cloudera/impala/service/Frontend.java
M fe/src/main/java/com/cloudera/impala/service/JniCatalog.java
M fe/src/main/java/com/cloudera/impala/service/JniFrontend.java
M fe/src/main/java/com/cloudera/impala/service/MetadataOp.java
M fe/src/main/java/com/cloudera/impala/util/PatternMatcher.java
M fe/src/test/java/com/cloudera/impala/analysis/AuthorizationTest.java
M fe/src/test/java/com/cloudera/impala/testutil/BlockIdGenerator.java
M fe/src/test/java/com/cloudera/impala/testutil/ImpaladTestCatalog.java
M testdata/workloads/functional-query/queries/QueryTest/show.test
12 files changed, 382 insertions(+), 115 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala refs/changes/71/3371/12
--
To view, visit http://gerrit.cloudera.org:8080/3371
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I17d8c5b9fb12483e4b01b819fba48b6849311a14
Gerrit-PatchSet: 12
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Huaisi Xu <hx...@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dt...@cloudera.com>
Gerrit-Reviewer: Huaisi Xu <hx...@cloudera.com>