Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15207 )
Change subject: [WIP] Ranger authorization provider ...................................................................... Patch Set 16: (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: static std::string GetJar(); Should doc what this does. 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 files we'll be looking for in this directory. http://gerrit.cloudera.org:8080/#/c/15207/16/src/kudu/master/ranger_authz_provider.cc@151 PS16, Line 151: unordered_set<ActionPB> actions; : actions.emplace(ActionPB::DELETE); : actions.emplace(ActionPB::INSERT); : actions.emplace(ActionPB::UPDATE); : actions.emplace(ActionPB::SELECT); Can use an initializer_list? unoredered_set<ActionPB> actions = { ActionPB::DELETE, ActionPB::INSERT, ... }; http://gerrit.cloudera.org:8080/#/c/15207/16/src/kudu/master/ranger_authz_provider.cc@161 PS16, Line 161: pb->set_delete_privilege(ContainsKey(actions, ActionPB::DELETE)); : pb->set_update_privilege(ContainsKey(actions, ActionPB::UPDATE)); : pb->set_insert_privilege(ContainsKey(actions, ActionPB::INSERT)); : if (ContainsKey(actions, ActionPB::SELECT)) { : pb->set_scan_privilege(true); : return Status::OK(); : } : pb->set_scan_privilege(false); I think it'd be more performant to iterate over 'actions' rather than do four discrete lookups. http://gerrit.cloudera.org:8080/#/c/15207/16/src/kudu/master/ranger_authz_provider.cc@184 PS16, Line 184: if (!client_.AuthorizeAction(user, ActionPB::SELECT, table_name, So it seems as if we are making two distinct kinds of authz checks here: 1. One table, N actions, no columns. --> tell me which of the N actions I'm allowed to perform on this table. 2. One table, one action, N columns. --> tell me which of the table's N columns I'm allowed to SELECT. Can we combine them into one request to Ranger? http://gerrit.cloudera.org:8080/#/c/15207/16/src/kudu/master/ranger_authz_provider.cc@206 PS16, Line 206: env->GetExecutablePath(&exe); Return value should be checked. -- 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: 16 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 07:26:22 +0000 Gerrit-HasComments: Yes
