Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12769 )
Change subject: IMPALA-8225: Add Ranger support for grant/revoke privilege to/from user ...................................................................... Patch Set 5: (18 comments) http://gerrit.cloudera.org:8080/#/c/12769/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12769/4//COMMIT_MSG@14 PS4, Line 14: ROLE via GRANT/REVOKE statements even : when the ROLE keyword was omitted. some example would be good http://gerrit.cloudera.org:8080/#/c/12769/4//COMMIT_MSG@30 PS4, Line 30: works properly nit: works properly with Ranger? http://gerrit.cloudera.org:8080/#/c/12769/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12769/5//COMMIT_MSG@26 PS5, Line 26: An additional end to end test, test_ranger.py, was added. A single test : was added that grants and revokes for a user and asserts permissions on : a table. The test uses sleep statements to work with Ranger's polling : interval for policy changes. More end to end tests will be added in the : future when the refresh authorization statement works properly. nit: use a bullet point to be consistent with the other two bullet points http://gerrit.cloudera.org:8080/#/c/12769/5//COMMIT_MSG@26 PS5, Line 26: An additional end to end test, test_ranger.py, was added. A single test : was added that grants and revokes for a user and asserts permissions on : a table. The test uses sleep statements to work with Ranger's polling : interval for policy changes. More end to end tests will be added in the : future when the refresh authorization statement works properly. : mention that AuthorizationStmtTest has been refactored to use the grant/revoke methods from the AuthorizationManager to provide greater code coverage. http://gerrit.cloudera.org:8080/#/c/12769/4/fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java File fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java: http://gerrit.cloudera.org:8080/#/c/12769/4/fe/src/main/java/org/apache/impala/authorization/NoneAuthorizationFactory.java@127 PS4, Line 127: @Override : public void grantPrivilegeToUser(User requestingUser, TGrantRevokePrivParams params, : TDdlExecResponse response) throws ImpalaException { : } : : @Override : public void revokePrivilegeFromUser(User requestingUser, : TGrantRevokePrivParams params, TDdlExecResponse response) throws ImpalaException { : } throw UnsupportedOperationException http://gerrit.cloudera.org:8080/#/c/12769/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/12769/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationManager.java@50 PS4, Line 50: RangerAuthorizationManager I believe this is meant for Catalogd. We should rename this to RangerCatalogdAuthorizationManager. http://gerrit.cloudera.org:8080/#/c/12769/4/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationManager.java@53 PS4, Line 53: : public RangerAuthorizationManager( : Supplier<? extends AuthorizationChecker> authzChecker) { : Preconditions.checkNotNull(authzChecker); : Preconditions.checkArgument(authzChecker.get() instanceof RangerAuthorizationChecker); : auditHandler_ = new RangerDefaultAuditHandler(); : plugin_ = () -> ((RangerAuthorizationChecker) authzChecker.get()) : .getRangerImpalaPlugin(); : } : : public RangerAuthorizationManager(RangerImpalaPlugin plugin) { : auditHandler_ = new RangerDefaultAuditHandler(); : plugin_ = () -> plugin; : } I think we can just have one constructor. public RangerAuthorizationManager(Supplier<RangerImpalaPlugin> plugin) http://gerrit.cloudera.org:8080/#/c/12769/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/12769/5/fe/src/main/java/org/apache/impala/authorization/ranger/RangerAuthorizationManager.java@43 PS5, Line 43: An implementation of {@link AuthorizationManager} using Ranger. update the comment that saying that it's used by catalogd http://gerrit.cloudera.org:8080/#/c/12769/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java: http://gerrit.cloudera.org:8080/#/c/12769/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3266 PS4, Line 3266: nit: add : http://gerrit.cloudera.org:8080/#/c/12769/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3294 PS4, Line 3294: same as above http://gerrit.cloudera.org:8080/#/c/12769/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java File fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java: http://gerrit.cloudera.org:8080/#/c/12769/4/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java@42 PS4, Line 42: import org.apache.impala.authorization.ranger.*; no wildcard import http://gerrit.cloudera.org:8080/#/c/12769/4/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java: http://gerrit.cloudera.org:8080/#/c/12769/4/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1495 PS4, Line 1495: identities nit: principal type is a better name http://gerrit.cloudera.org:8080/#/c/12769/4/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java@1670 PS4, Line 1670: : @Test : public void testGrantRevoke() { : String defaultDb = System.getProperty("user.name"); : AnalysisContext context = createAnalysisCtx(createAuthorizationFactory()); : testToSql(context, "CREATE ROLE testrole"); : testToSql(context, "GRANT ALL ON SERVER server1 TO USER anobis"); : testToSql(context, "GRANT ALL ON SERVER server1 TO ROLE testrole"); : testToSql(context, "GRANT ALL ON SERVER server1 TO testrole", : "GRANT ALL ON SERVER server1 TO ROLE testrole"); : } what's the difference between this test and testGrantRevokePrivStmt? http://gerrit.cloudera.org:8080/#/c/12769/4/fe/src/test/resources/xasecure-audit.xml File fe/src/test/resources/xasecure-audit.xml: http://gerrit.cloudera.org:8080/#/c/12769/4/fe/src/test/resources/xasecure-audit.xml@1 PS4, Line 1: <?xml version="1.0"?> do we need this file? I believe we already have hive-ranger-audit.xml. http://gerrit.cloudera.org:8080/#/c/12769/4/tests/authorization/test_grant_revoke.py File tests/authorization/test_grant_revoke.py: http://gerrit.cloudera.org:8080/#/c/12769/4/tests/authorization/test_grant_revoke.py@369 PS4, Line 369: > Done What is this? http://gerrit.cloudera.org:8080/#/c/12769/4/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/12769/4/tests/authorization/test_ranger.py@26 PS4, Line 26: class TestRanger(CustomClusterTestSuite): add pydoc class comment. http://gerrit.cloudera.org:8080/#/c/12769/4/tests/authorization/test_ranger.py@45 PS4, Line 45: anobis you need to use getuser() since the user created will vary. http://gerrit.cloudera.org:8080/#/c/12769/4/tests/authorization/test_ranger.py@43 PS4, Line 43: # TODO: Use refresh authorization : time.sleep(30) : anobis_client.execute("show tables in functional", user="anobis") : admin_client.execute("revoke select on database functional from user anobis", : user="admin") : # TODO: Use refresh authorization add the ticket number IMPALA-8293 for the TODO -- To view, visit http://gerrit.cloudera.org:8080/12769 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6ee97bf41546d63385026c0e2b19545565402462 Gerrit-Change-Number: 12769 Gerrit-PatchSet: 5 Gerrit-Owner: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Austin Nobis <ano...@cloudera.com> Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Tue, 26 Mar 2019 18:42:44 +0000 Gerrit-HasComments: Yes
