Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/16837 )
Change subject: IMPALA-10211 (Part 1): Add support for role-related statements ...................................................................... Patch Set 10: Code-Review+1 (5 comments) LGTM. I'll carry on Csaba's +1 to +2 once the minor comments are resolved. Thanks for your patience! http://gerrit.cloudera.org:8080/#/c/16837/10/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/16837/10/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@154 PS10, Line 154: nit: blank line http://gerrit.cloudera.org:8080/#/c/16837/10/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@173 PS10, Line 173: User Could you add requestingUser.getShortName() here? We show the username in Sentry errors: https://github.com/apache/impala/blob/3.4.0/testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test#L357 http://gerrit.cloudera.org:8080/#/c/16837/10/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@186 PS10, Line 186: nit: two blank lines http://gerrit.cloudera.org:8080/#/c/16837/10/testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test File testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test: http://gerrit.cloudera.org:8080/#/c/16837/10/testdata/workloads/functional-query/queries/QueryTest/grant_revoke.test@350 PS10, Line 350: # TODO: Investigate whether the privilege on the provided uri should be verified. Yeah, this also confuses me. We do verify the URI privilege. I think enabling Ranger audit logs can help us answering such questions, i.e. which policy allows the action. http://gerrit.cloudera.org:8080/#/c/16837/10/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/16837/10/tests/authorization/test_ranger.py@534 PS10, Line 534: # TODO(IMPALA-10399): We found that if this test is run after : # test_grant_revoke_with_role() in the exhaustive tests, the test could fail due to an : # empty list returned from the first call to _get_ranger_privileges_db() although a : # list consisting of "lock" and "select" is expected. We suspect there might be : # something wrong with the underlying Ranger API but it requires more thorough : # investigation. > Thanks Quanlong for the detailed suggestion! Thanks! Let's debug it in follow-up works. I created a JIRA for enabling ranger audit logs: IMPALA-10401. -- To view, visit http://gerrit.cloudera.org:8080/16837 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic2b204e62a1d8ae1932d955b4efc28be22202860 Gerrit-Change-Number: 16837 Gerrit-PatchSet: 10 Gerrit-Owner: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Fang-Yu Rao <fangyu....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com> Gerrit-Comment-Date: Mon, 21 Dec 2020 02:09:46 +0000 Gerrit-HasComments: Yes
