You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Fredy Wijaya (Code Review)" <ge...@cloudera.org> on 2018/04/20 16:26:31 UTC
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/10135
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
IMPALA-6802 (part 1): Clean up authorization tests
The first patch of this patch is to introduce a new mechanism of testing
authorization that tests authorization at every hierarchy. This patch
rewrites the authorization tests for select statements.
Testing:
- Added new authorization tests
- Ran all front-end tests
Cherry-picks: not for 2.x
Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
---
A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
1 file changed, 525 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10135/2
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
Patch Set 8:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java:
http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java@654
PS8, Line 654: public void TestPrivilegeRequests() throws ImpalaException {
I think these should go into AuthorizationTestV2
http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java@671
PS8, Line 671: verifyPrivilegeReqs("select alltypes.* from functional.alltypes", Sets.newHashSet(
we should also test a "use functional" and referencing "alltypes"
http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@89
PS8, Line 89: // Test qualified and unqualified names.
We've already tested those elsewhere, so no need.
http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@143
PS8, Line 143: // Unqualified table name.
already tested elsewhere
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Apr 2018 21:41:18 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
Patch Set 11:
(4 comments)
Thanks! Almost there
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@94
PS11, Line 94: verifyPrivilegeReqs("select * from functional.alltypes", Sets.newHashSet(
You can create this set once and save many lines
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@195
PS11, Line 195: ));
create expected set once
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@209
PS11, Line 209: authorize("select id from functional.alltypes")
fix indentation
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@271
PS11, Line 271: .error("User '%s' does not have privileges to execute 'SELECT' on: " +
factor out the err msg here and elsewhere to condense the tests
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 11
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Apr 2018 22:21:37 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
Patch Set 6:
(8 comments)
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@47
PS5, Line 47: import static org.junit.Assert.fail;
> I tried running the test and it feels very slow. Do you know what's making
Done. Avoiding calling catalog_.reset() and making Sentry RPCs makes the whole tests faster in a significant way.
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@75
PS5, Line 75:
> Why do we need this?
To ensure we have existing roles created externally will not interfere with these tests. I'll add some comment in the code.
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@122
PS5, Line 122: // Select a specific column on a table.
> What does this mean? My understanding is that columns can only have SELECT,
Done. It's allowed in Sentry but not in Impala.
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@157
PS5, Line 157: .error("User '%s' does not have privileges to execute 'SELECT' on: " +
> add comment that column-level privs are currently not supported
Done
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@183
PS5, Line 183: TPrivilegeLevel.ALL, TPrivilegeLevel.SELECT)))
> I feel like there's still a lot of redundancy in these tests, could we perh
Done
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@324
PS5, Line 324: "functional.alltypes", onColumn("functional", "alltypes", "id", allExcept(
> also very similar to the other test blocks, can we condense them further?
Done
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@349
PS5, Line 349: "functional.alltypes", onTable("functional", "alltypes", allExcept(
> condense further?
Shouldn't this be a separate test case since this is to test a table with an alias?
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@451
PS5, Line 451: "month"}, allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.SELECT)));
> I feel like these tests could be structured a little more. Not sure if you
Done. I restructured the tests. Let me know what you think.
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Apr 2018 02:47:05 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
Patch Set 12: Code-Review+2
Very nice. Let's keep the cleanup going!
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 12
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Apr 2018 22:59:49 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
IMPALA-6802 (part 1): Clean up authorization tests
The first patch of this patch is to introduce a new mechanism of testing
authorization that tests authorization at every hierarchy. This patch
rewrites the authorization tests for select statements.
Testing:
- Added new authorization tests
- Ran all front-end tests
Cherry-picks: not for 2.x
Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Reviewed-on: http://gerrit.cloudera.org:8080/10135
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
2 files changed, 659 insertions(+), 3 deletions(-)
Approvals:
Alex Behm: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 13
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
Patch Set 11:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java:
http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java@654
PS8, Line 654:
> I think these should go into AuthorizationTestV2
Done
http://gerrit.cloudera.org:8080/#/c/10135/8/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java@671
PS8, Line 671:
> we should also test a "use functional" and referencing "alltypes"
Done
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 11
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Apr 2018 22:16:17 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@95
PS2, Line 95: );
> I don't know if we want to go through every permutation in the error. It ca
Maybe we don't need it for every test, but where is the test to ensure that "REFRESH" or other privileges do not unintentionally allow you to do select? Shouldn't that be somewhere with the select tests?
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@509
PS2, Line 509: )
> Passing a full error string in expectedErrorString is essentially comparing
The problem is, if I expect an error to be "... not authorized on functional", and the error is "... not authorized on functional.alltypes", I have no way to say I got the wrong error, i.e. there's information leakage on the error.
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Apr 2018 15:37:00 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@349
PS5, Line 349: .ok(onColumn("functional", "alltypes", new String[]{"id", "bool_col",
> In my mind the auth tests should check whether the authorization rules are
Done. I created a separate test in the AnalyzerTest to test solely on different SQL variants.
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Apr 2018 18:19:50 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
Patch Set 12:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/2386/
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 12
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Apr 2018 23:00:32 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
Patch Set 2:
(6 comments)
I like the new format. Few first pass comments.
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@95
PS2, Line 95: );
Shouldn't there also be :
.error ("USER....", onServer(TPrivilegeLevel.REFRESH, TPrivilegeLevel.CREATE,TPrivilegeLevel.ALTER,TPrivilegeLevel.DROP))
.error ("USER....", onDatabase("functional", TPrivilegeLevel.REFRESH, TPrivilegeLevel.CREATE,TPrivilegeLevel.ALTER,TPrivilegeLevel.DROP))
.etc....
for onServer, Database, Table, Column, and for the other tests as well to ensure none of the other privileges allow the statement?
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@118
PS2, Line 118: // TODO: IMPALA-6718
: //.ok(onColumn("functional", "alltypes_view", new String[]{"id", "bool_col",
: // "tinyint_col", "smallint_col", "int_col", "bigint_col", "float_col",
: // "double_col", "date_string_col", "string_col", "timestamp_col", "year",
: // "month"}, TPrivilegeLevel.SELECT))
: //.ok(onColumn("functional", "alltypes_view", new String[]{"id", "bool_col",
: // "tinyint_col", "smallint_col", "int_col", "bigint_col", "float_col",
: // "double_col", "date_string_col", "string_col", "timestamp_col", "year",
: // "month"}, TPrivilegeLevel.SELECT))
Take out this todo as it's waiting on future function. When the function is delivered, then appropriate tests should be added. Add a comment to the Impala-6718 Jira.
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@138
PS2, Line 138: // TODO: IMPALA-6718
: //.ok(onColumn("functional", "alltypes_view", "id", TPrivilegeLevel.SELECT))
Remove. Add comment to Jira.
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@325
PS2, Line 325: // TODO: IMPALA-6891
: //.ok(onColumn("functional", "alltypes", new String[]{"id", "bool_col",
: // "tinyint_col", "smallint_col", "int_col", "bigint_col", "float_col",
: // "double_col", "date_string_col", "string_col", "timestamp_col", "year",
: // "month"}, TPrivilegeLevel.SELECT),
: // onColumn("functional", "alltypessmall", new String[]{"id", "bool_col",
: // "tinyint_col", "smallint_col", "int_col", "bigint_col", "float_col",
: // "double_col", "date_string_col", "string_col", "timestamp_col", "year",
: // "month"}, TPrivilegeLevel.SELECT))
Remove. Add a comment to the Jira.
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@388
PS2, Line 388: TPrivilege[]... privileges
There doesn't seem to be any tests that use this. If the other tests from comments above are not needed, then is this?
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@509
PS2, Line 509: )
Since we're changing this anyway, can we add another method that takes a boolean to indicate it should use the entire string instead of just starts with? Some of the authorization errors are important to distinguish that the error returns just the table, not the table and column.
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Apr 2018 06:00:59 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
Patch Set 5:
(8 comments)
Overall the new mechanism seems fine to me. Main comments:
* test execution speed needs to be improved
* still a lot of redundancy in the tests
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@47
PS5, Line 47: public class AuthorizationTestV2 extends FrontendTestBase {
I tried running the test and it feels very slow. Do you know what's making it slow? Might be good to address the low hanging perf fruit if we can.
At this execution speed this new test might ultimately take tens of minutes if we add coverage for the other statements.
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@75
PS5, Line 75: public void before() throws ImpalaException {
Why do we need this?
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@122
PS5, Line 122: .ok(onColumn("functional", "alltypes", "id", TPrivilegeLevel.ALL))
What does this mean? My understanding is that columns can only have SELECT, so this should be illegal.
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@157
PS5, Line 157: allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.SELECT)));
add comment that column-level privs are currently not supported
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@183
PS5, Line 183: authorize(createAnalysisCtx("functional"), "select id from alltypes")
I feel like there's still a lot of redundancy in these tests, could we perhaps eliminate some of it?
These ~20 lines are basically the same test as the ~20 lines in 115.
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@324
PS5, Line 324: authorize("select count(id) from functional.alltypes")
also very similar to the other test blocks, can we condense them further?
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@349
PS5, Line 349: authorize("select t.id from functional.alltypes t")
condense further?
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@451
PS5, Line 451: authorize("select * from functional.alltypes a cross join " +
I feel like these tests could be structured a little more. Not sure if you are following the existing tests or not. We seem to have a few dimensions: view vs. table, qualified vs. unqualified column/table reference.
Then we have tests for specific clauses in a SELECT like the FROM clause, GROUP BY / HAVING clauses, etc.
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Wed, 25 Apr 2018 22:51:14 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Adam Holley (Code Review)" <ge...@cloudera.org>.
Adam Holley has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
Patch Set 5: Code-Review+1
Thanks for the updates.
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Apr 2018 22:20:36 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
IMPALA-6802 (part 1): Clean up authorization tests
The first patch of this patch is to introduce a new mechanism of testing
authorization that tests authorization at every hierarchy. This patch
rewrites the authorization tests for select statements.
Testing:
- Added new authorization tests
- Ran all front-end tests
Cherry-picks: not for 2.x
Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
---
A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
1 file changed, 701 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10135/3
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
IMPALA-6802 (part 1): Clean up authorization tests
The first patch of this patch is to introduce a new mechanism of testing
authorization that tests authorization at every hierarchy. This patch
rewrites the authorization tests for select statements.
Testing:
- Added new authorization tests
- Ran all front-end tests
Cherry-picks: not for 2.x
Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
---
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
3 files changed, 727 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10135/8
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
IMPALA-6802 (part 1): Clean up authorization tests
The first patch of this patch is to introduce a new mechanism of testing
authorization that tests authorization at every hierarchy. This patch
rewrites the authorization tests for select statements.
Testing:
- Added new authorization tests
- Ran all front-end tests
Cherry-picks: not for 2.x
Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
---
A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
2 files changed, 715 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10135/6
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#12). ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
IMPALA-6802 (part 1): Clean up authorization tests
The first patch of this patch is to introduce a new mechanism of testing
authorization that tests authorization at every hierarchy. This patch
rewrites the authorization tests for select statements.
Testing:
- Added new authorization tests
- Ran all front-end tests
Cherry-picks: not for 2.x
Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
---
A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
2 files changed, 659 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10135/12
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 12
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
Patch Set 12:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@94
PS11, Line 94: Set<String> expectedPrivileges = Sets.newHashSet(
> You can create this set once and save many lines
Done
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@195
PS11, Line 195: allExcept(TPrivilegeLevel.ALL, TPrivilegeLevel.SELECT)))
> create expected set once
Done
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@209
PS11, Line 209: .error(selectError("functional_seq_snap.subquery_view"), onServer(
> fix indentation
Done
http://gerrit.cloudera.org:8080/#/c/10135/11/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@271
PS11, Line 271: .ok(onServer(TPrivilegeLevel.SELECT))
> factor out the err msg here and elsewhere to condense the tests
Done
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 12
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Mon, 30 Apr 2018 22:47:45 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Alex Behm (Code Review)" <ge...@cloudera.org>.
Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
Patch Set 6:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@47
PS5, Line 47: import static org.junit.Assert.fail;
> Done. Avoiding calling catalog_.reset() and making Sentry RPCs makes the wh
That's more like it! Fantastic.
http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@349
PS5, Line 349: "functional.alltypes", onTable("functional", "alltypes", allExcept(
> Shouldn't this be a separate test case since this is to test a table with a
In my mind the auth tests should check whether the authorization rules are applied correctly.
A table reference with implicit or explicit aliases still refers to the same underlying table,
so it's a variant of the same test, but not really a different auth test.
Ideally, we should separate testing different SQL variants of referencing the same underlying entity
from the auth tests themselves.
For exampe, we could have a single test that asserts the following things generate the same
PrivilegeRequests during analysis.
- table reference with implicit/explicit aliases
- qualified/unqualified table references
- qualified/unqualified column references
- qualified/unqualified star
Then when we do the auth tests, we can focus on whatever variant is most convenient.
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Fri, 27 Apr 2018 16:51:01 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#11). ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
IMPALA-6802 (part 1): Clean up authorization tests
The first patch of this patch is to introduce a new mechanism of testing
authorization that tests authorization at every hierarchy. This patch
rewrites the authorization tests for select statements.
Testing:
- Added new authorization tests
- Ran all front-end tests
Cherry-picks: not for 2.x
Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
---
A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
2 files changed, 770 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10135/11
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 11
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
IMPALA-6802 (part 1): Clean up authorization tests
The first patch of this patch is to introduce a new mechanism of testing
authorization that tests authorization at every hierarchy. This patch
rewrites the authorization tests for select statements.
Testing:
- Added new authorization tests
- Ran all front-end tests
Cherry-picks: not for 2.x
Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
---
A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
1 file changed, 700 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10135/4
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 4
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
Patch Set 12: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 12
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 01 May 2018 02:53:41 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@95
PS2, Line 95: ,
> Maybe we don't need it for every test, but where is the test to ensure that
Done
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@509
PS2, Line 509:
> The problem is, if I expect an error to be "... not authorized on function
Done. Instead of a boolean, there's a custom Matcher that we can use.
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Apr 2018 18:54:55 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
IMPALA-6802 (part 1): Clean up authorization tests
The first patch of this patch is to introduce a new mechanism of testing
authorization that tests authorization at every hierarchy. This patch
rewrites the authorization tests for select statements.
Testing:
- Added new authorization tests
- Ran all front-end tests
Cherry-picks: not for 2.x
Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
---
A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
1 file changed, 729 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/35/10135/5
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests
Posted by "Fredy Wijaya (Code Review)" <ge...@cloudera.org>.
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/10135 )
Change subject: IMPALA-6802 (part 1): Clean up authorization tests
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@95
PS2, Line 95: );
> Shouldn't there also be :
I don't know if we want to go through every permutation in the error. It can be somewhat too excessive. The ok() ensures we have the minimum requirement. For example:
.ok(onTable("functional", "alltypes", TPrivilegeLevel.SELECT)) means we must have SELECT privilege on functional.alltypes and without it, we will get an authorization error.
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@118
PS2, Line 118: // TODO: IMPALA-6718
: //.ok(onColumn("functional", "alltypes_view", new String[]{"id", "bool_col",
: // "tinyint_col", "smallint_col", "int_col", "bigint_col", "float_col",
: // "double_col", "date_string_col", "string_col", "timestamp_col", "year",
: // "month"}, TPrivilegeLevel.SELECT))
: //.ok(onColumn("functional", "alltypes_view", new String[]{"id", "bool_col",
: // "tinyint_col", "smallint_col", "int_col", "bigint_col", "float_col",
: // "double_col", "date_string_col", "string_col", "timestamp_col", "year",
: // "month"}, TPrivilegeLevel.SELECT))
> Take out this todo as it's waiting on future function. When the function i
I saw some existing code that adds TODO with a ticket number to give more context especially if this is a bug found due to this CR. I'm okay with removing it, though. What do others think?
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@138
PS2, Line 138: // TODO: IMPALA-6718
: //.ok(onColumn("functional", "alltypes_view", "id", TPrivilegeLevel.SELECT))
> Remove. Add comment to Jira.
Same comment as above.
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@325
PS2, Line 325: // TODO: IMPALA-6891
: //.ok(onColumn("functional", "alltypes", new String[]{"id", "bool_col",
: // "tinyint_col", "smallint_col", "int_col", "bigint_col", "float_col",
: // "double_col", "date_string_col", "string_col", "timestamp_col", "year",
: // "month"}, TPrivilegeLevel.SELECT),
: // onColumn("functional", "alltypessmall", new String[]{"id", "bool_col",
: // "tinyint_col", "smallint_col", "int_col", "bigint_col", "float_col",
: // "double_col", "date_string_col", "string_col", "timestamp_col", "year",
: // "month"}, TPrivilegeLevel.SELECT))
> Remove. Add a comment to the Jira.
Same comment as above.
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@388
PS2, Line 388: TPrivilege[]... privileges
> There doesn't seem to be any tests that use this. If the other tests from
It's not used for the first part of this patch. In the next part like if we want to do ALTER DATABASE RENAME, we need a CREATE privilege on a database and an ALTER privilege on a table, this method can be handy.
http://gerrit.cloudera.org:8080/#/c/10135/2/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@509
PS2, Line 509: )
> Since we're changing this anyway, can we add another method that takes a bo
Passing a full error string in expectedErrorString is essentially comparing the entire string. I can overload it, but then we need to overload bunch authzError methods.
--
To view, visit http://gerrit.cloudera.org:8080/10135
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Adam Holley <gi...@holleyism.com>
Gerrit-Reviewer: Alex Behm <al...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Comment-Date: Tue, 24 Apr 2018 06:22:33 +0000
Gerrit-HasComments: Yes