Austin Nobis 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 8: (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 " Done 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: Li > does this need to be public? Done http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/AuthorizationPolicy.java@529 PS7, Line 529: oThrift(); > This is an unordered set, will this be a problem like the output of the row Done 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: 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; : } : : private static String getOrAll(String resource) { : > can we make these private now? They are used in the RangerImpaladAuthorizationManager as well. 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: upInformation ugi = UserGroupInforma > let's try to use the one from JDK instead, i.e. new HashSet<>(ugi.getGroupN The JDK HashSet constructor doesn't take a String[]. The code would have to be: `return new HashSet<>(Arrays.asList(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; : > add default case to to make the compiler happy Done http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@259 PS7, Line 259: vate > since L257 is sa throw. We can make it just an if instead of else if. Done http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@276 PS7, Line 276: // Server is used by column, function, and URI resources. > instead of making a copy, we should just make List<Map<String, Object>> res Done http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@354 PS7, Line 354: > does this need to be public? Done http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@355 PS7, Line 355: @Override : public void updateTableOwnerPrivilege(String serverName, String databaseName, : String tableName, String oldOwner, PrincipalType oldOwnerType, String newOwner, : PrincipalType newOwnerType, TDdlExecResponse response) throws ImpalaException { : } : : private static class RangerResultRow { : private final TPrincipalType principalType_; : private final String principalName_; : private final String database_ > use _ suffix naming convention Done http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@380 PS7, Line 380: this.column_ = column; : this.uri_ = uri; : this.udf_ = udf; : this.privilege_ = privilege; : this.grantOption_ = grantOption; : this.createTime_ = createTime; : } : : public static TResultSetMetadata getSchema() { : TResultSetMetadata schema = new TResultSetMetadata(); : : schema.addToColumns(new TColumn("principal_type", Type.STRING.toThrift())); : schema.addToColumns(new TColumn("principal_name", Type.STRING.toThrift())); : schema.addToColumns(new TColumn("database", Type.STRING.toThrift())); : schema.addToColumns(new TColumn("table", Type.STRING.toThrift())); : schema.addToColumns(new TColumn("column", Type.STRING.toThrift())); : schema.addToColumns(new TColumn("uri", Type.STRING.toThrift())); : schema.addToColumns(new TColumn("udf", Type.STRING.toThrift())); : schema.addToColumns(new TColumn("privilege", Type.STRING.toThrift())); : schema.addToColumns(new TColumn("grant_option", > if this is a private static inner class, we don't need the getters Done http://gerrit.cloudera.org:8080/#/c/13074/7/fe/src/main/java/org/apache/impala/authorization/ranger/RangerImpaladAuthorizationManager.java@430 PS7, Line 430: : : : : > nit: can be put in one line: Actually, it can't. The `rowBuilder.add()` method is overloaded for different types. When you use the ternary expression it will call the type that takes a primitive long and passing a null value will result in a NullPointerException. Using the if, else statement allows the null case to call the overloaded method that takes a String which results in the correct behavior. 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: # Test ALL privilege hides other privileges > can we add another test case for "show grant user/group" without any "on"? Done http://gerrit.cloudera.org:8080/#/c/13074/7/tests/authorization/test_ranger.py@184 PS7, Line 184: grant > nit: user_group Done http://gerrit.cloudera.org:8080/#/c/13074/7/tests/authorization/test_ranger.py@202 PS7, Line 202: result = self.client.execute("show grant user {0} on database {1}" > we need to add a test case for grant on database as well since we're creati That is tested in the _test_show_grant_basic http://gerrit.cloudera.org:8080/#/c/13074/7/tests/authorization/test_ranger.py@204 PS7, Line 204: TestRanger._check_privileges(result, [ : ["GROUP", user, unique_db, "", "", "", "*", "select", "false"], : ["GROUP", user, unique_db, "*", "*", "", "", "select", "false"]]) : finally: : admin_client.execute("revoke select on database {0} from group {1}" : .format(unique_db, group)) > use for loop to condense this Done http://gerrit.cloudera.org:8080/#/c/13074/7/tests/authorization/test_ranger.py@237 PS7, Line 237: result = self.client.execute("show grant user {0} on server".format(user)) : TestRanger._check_privileges(result, [ : ["USER", user, "", "", "", "*", "", "all", "false"], : ["USER", user, "*", "", "", "", "*", "all", "false"], : ["USER", user, "*", "*", "*", "", "", "all", "false"]]) : finally: : for privilege in privileges: > we can use for loop to condense this Done http://gerrit.cloudera.org:8080/#/c/13074/7/tests/authorization/test_ranger.py@245 PS7, Line 245: > can we also test uri? Done http://gerrit.cloudera.org:8080/#/c/13074/7/tests/authorization/test_ranger.py@284 PS7, Line 284: time.sleep(10) : result = self.client.execute("show grant {0} {1} on database {2}" : .format(kw, id, unique_database)) : TestRanger._check_privileges(result, [ : [kw, id, unique_database, "", "", "", "*", "select", "false"], : [kw, id, unique_database, "*", "*", "", "", "select", "false"]]) : : # Revoke database privileges and verify > i feel like we should also do "show grant" after revoking to make sure the Done http://gerrit.cloudera.org:8080/#/c/13074/7/tests/authorization/test_ranger.py@291 PS7, Line 291: # Revoke database privileges and verify > is L291 a duplicate of L284? Done -- 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: 8 Gerrit-Owner: Austin Nobis <[email protected]> Gerrit-Reviewer: Austin Nobis <[email protected]> Gerrit-Reviewer: Fredy Wijaya <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Tue, 30 Apr 2019 05:52:52 +0000 Gerrit-HasComments: Yes
