Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/13074 )
Change subject: IMPALA-8280, IMPALA-8281: Add support for show grant user/group with Ranger ...................................................................... Patch Set 7: (20 comments) http://gerrit.cloudera.org:8080/#/c/13074/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13074/7//COMMIT_MSG@9 PS7, Line 9: Add support for SHOW GRANT statements for Apache Ranger. This patch also : adds the RangerImpaladAuthorizationManager as the show grant statement : is called from impalad. mention the list of new syntax and also mention that "show grant" without "on" is not supported. http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java File fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java: http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java@526 PS7, Line 526: public does this need to be public? http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java@529 PS7, Line 529: new HashSet<>() This is an unordered set, will this be a problem like the output of the row will be indeterministic? Maybe we should use LinkedHashSet instead. http://gerrit.cloudera.org:8080/#/c/13074/7/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/13074/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerCatalogdAuthorizationManager.java@260 PS7, Line 260: public static Map<String, String> createColumnResource(TPrivilege privilege) { : Map<String, String> resource = new HashMap<>(); : : resource.put(RangerImpalaResourceBuilder.DATABASE, getOrAll(privilege.getDb_name())); : resource.put(RangerImpalaResourceBuilder.TABLE, getOrAll(privilege.getTable_name())); : resource.put(RangerImpalaResourceBuilder.COLUMN, : getOrAll(privilege.getColumn_name())); : : return resource; : } : : public static Map<String, String> createUriResource(TPrivilege privilege) { : Map<String, String> resource = new HashMap<>(); : String uri = privilege.getUri(); : resource.put(RangerImpalaResourceBuilder.URL, uri == null ? "*" : uri); : : return resource; : } : : public static Map<String, String> createFunctionResource(TPrivilege privilege) { : Map<String, String> resource = new HashMap<>(); : : resource.put(RangerImpalaResourceBuilder.DATABASE, getOrAll(privilege.getDb_name())); : resource.put(RangerImpalaResourceBuilder.UDF, "*"); : : return resource; : } can we make these private now? http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java File fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java: http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@162 PS7, Line 162: Sets.newHashSet(ugi.getGroupNames()) let's try to use the one from JDK instead, i.e. new HashSet<>(ugi.getGroupNames()) http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@236 PS7, Line 236: switch (privilege.getScope()) { : case COLUMN: : if (!column.isPresent() || column.get().equals("*")) return null; : case TABLE: : if (!table.isPresent() || table.get().equals("*")) return null; : case DATABASE: : if (!database.isPresent() || database.get().equals("*")) return null; : break; : case URI: : if (!uri.isPresent() || uri.get().equals("*")) return null; : break; : } add default case to to make the compiler happy http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@259 PS7, Line 259: else since L257 is sa throw. We can make it just an if instead of else if. http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@276 PS7, Line 276: Map<String, Object> tmpResource = new HashMap<>(resource); instead of making a copy, we should just make List<Map<String, Object>> resources http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@354 PS7, Line 354: public does this need to be public? http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@355 PS7, Line 355: private final TPrincipalType principalType; : private final String principalName; : private final String database; : private final String table; : private final String column; : private final String uri; : private final String udf; : private final TPrivilegeLevel privilege; : private final boolean grantOption; : private final Long createTime; use _ suffix naming convention http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@380 PS7, Line 380: : public TPrincipalType getPrincipalType() { return principalType; } : : public String getPrincipalName() { return principalName; } : : public String getDatabase() { return database; } : : public String getTable() { return table; } : : public String getColumn() { return column; } : : public String getUri() { return uri; } : : public String getUdf() { return udf; } : : public TPrivilegeLevel getPrivilege() { return privilege; } : : public boolean isGrantOption() { return grantOption; } : : public long getCreateTime() { return createTime; } if this is a private static inner class, we don't need the getters http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@430 PS7, Line 430: if (createTime == null) { : rowBuilder.add(null); : } else { : rowBuilder.add(createTime); : } nit: can be put in one line: rowBuilder.add(createTime == null ? nul : createTime) http://gerrit.cloudera.org:8080/#/c/13074/7/tests/authorization/test_ranger.py File tests/authorization/test_ranger.py: http://gerrit.cloudera.org:8080/#/c/13074/7/tests/authorization/test_ranger.py@178 PS7, Line 178: self._test_show_grant_usrgrp(admin_client, user, group, unique_db) can we add another test case for "show grant user/group" without any "on"? http://gerrit.cloudera.org:8080/#/c/13074/7/tests/authorization/test_ranger.py@184 PS7, Line 184: usrgrp nit: user_group i don't think we saved that many chars :) http://gerrit.cloudera.org:8080/#/c/13074/7/tests/authorization/test_ranger.py@202 PS7, Line 202: def _test_show_grant_mask(self, admin_client, user): we need to add a test case for grant on database as well since we're creating two resources, i.e. database;udf and database;table;column resource sets. http://gerrit.cloudera.org:8080/#/c/13074/7/tests/authorization/test_ranger.py@204 PS7, Line 204: admin_client.execute("grant select on server to user {0}".format(user)) : admin_client.execute("grant insert on server to user {0}".format(user)) : admin_client.execute("grant create on server to user {0}".format(user)) : admin_client.execute("grant alter on server to user {0}".format(user)) : admin_client.execute("grant drop on server to user {0}".format(user)) : admin_client.execute("grant refresh on server to user {0}".format(user)) use for loop to condense this http://gerrit.cloudera.org:8080/#/c/13074/7/tests/authorization/test_ranger.py@237 PS7, Line 237: admin_client.execute("revoke select on server from user {0}".format(user)) : admin_client.execute("revoke insert on server from user {0}".format(user)) : admin_client.execute("revoke create on server from user {0}".format(user)) : admin_client.execute("revoke alter on server from user {0}".format(user)) : admin_client.execute("revoke drop on server from user {0}".format(user)) : admin_client.execute("revoke refresh on server from user {0}".format(user)) : admin_client.execute("revoke all on server from user {0}".format(user)) we can use for loop to condense this http://gerrit.cloudera.org:8080/#/c/13074/7/tests/authorization/test_ranger.py@245 PS7, Line 245: def _test_show_grant_basic(self, admin_client, kw, id, unique_database, unique_table): can we also test uri? http://gerrit.cloudera.org:8080/#/c/13074/7/tests/authorization/test_ranger.py@291 PS7, Line 291: admin_client.execute("revoke all on server from {0} {1}".format(kw is L291 a duplicate of L284? http://gerrit.cloudera.org:8080/#/c/13074/7/tests/authorization/test_ranger.py@284 PS7, Line 284: admin_client.execute("revoke all on server from {0} {1}".format(kw, id)) : admin_client.execute("revoke select(x) on table {0}.{1} from {2} {3}" : .format(unique_database, unique_table, kw, id)) : admin_client.execute("revoke select on table {0}.{1} from {2} {3}" : .format(unique_database, unique_table, kw, id)) : admin_client.execute("revoke select on database {0} from {1} {2}" : .format(unique_database, kw, id)) : admin_client.execute("revoke all on server from {0} {1}".format(kw, id)) i feel like we should also do "show grant" after revoking to make sure the output looks correct (the some rows should be removed) -- To view, visit http://gerrit.cloudera.org:8080/13074 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic46fb9fc36c9e11ec78d5840d22eb0668150c2a4 Gerrit-Change-Number: 13074 Gerrit-PatchSet: 7 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, 30 Apr 2019 00:11:13 +0000 Gerrit-HasComments: Yes
