Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15207 )
Change subject: [WIP] Ranger authorization provider ...................................................................... Patch Set 17: (6 comments) http://gerrit.cloudera.org:8080/#/c/15207/16/src/kudu/master/ranger_authz_provider.h File src/kudu/master/ranger_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/15207/16/src/kudu/master/ranger_authz_provider.h@84 PS16, Line 84: // Returns the location of kudu-ranger.jar > Should doc what this does. Done http://gerrit.cloudera.org:8080/#/c/15207/16/src/kudu/master/ranger_authz_provider.cc File src/kudu/master/ranger_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/15207/16/src/kudu/master/ranger_authz_provider.cc@41 PS16, Line 41: DEFINE_string(ranger_config_path, "", > Shouldn't we link to a specific filename? If not, we should indicate what f We need to add this directory to the Java classpath. Should I list all files the subprocess will need, including optional ones? Or should I just point to the Ranger docs maybe? http://gerrit.cloudera.org:8080/#/c/15207/16/src/kudu/master/ranger_authz_provider.cc@151 PS16, Line 151: : unordered_set<ActionPB, ActionHash> actions = { : ActionPB::DELETE, : ActionPB::INSERT, : ActionPB::UPDATE, > Can use an initializer_list? Done http://gerrit.cloudera.org:8080/#/c/15207/16/src/kudu/master/ranger_authz_provider.cc@161 PS16, Line 161: auto s = client_.AuthorizeActions(user, &actions, table_name); : if (s.ok()) { : for (const ActionPB& action : actions) { : switch (action) { : case ActionPB::DELETE: : pb->set_delete_privilege(true); : break; : case ActionPB::UPDATE: > I think it'd be more performant to iterate over 'actions' rather than do fo Done http://gerrit.cloudera.org:8080/#/c/15207/16/src/kudu/master/ranger_authz_provider.cc@184 PS16, Line 184: } > So it seems as if we are making two distinct kinds of authz checks here: We could, but that would be a lot of extra complexity and I'm not sure it's worth it. This way we don't need to check column-level privileges if SELECT is allowed on the table, so basically we are optimizing for the case where the whole table is readable and not only specific columns by a user. Let me know what you think, I can rewrite it. http://gerrit.cloudera.org:8080/#/c/15207/16/src/kudu/master/ranger_authz_provider.cc@206 PS16, Line 206: if (ContainsKey(column_names, col.name())) { > Return value should be checked. Done -- To view, visit http://gerrit.cloudera.org:8080/15207 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6e7672a5947d6406e0cad83a0c900bf5b2c03012 Gerrit-Change-Number: 15207 Gerrit-PatchSet: 17 Gerrit-Owner: Attila Bukor <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 03 Mar 2020 13:18:13 +0000 Gerrit-HasComments: Yes
