Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )
Change subject: [WIP] Ranger client ...................................................................... Patch Set 5: (2 comments) http://gerrit.cloudera.org:8080/#/c/15206/3/src/kudu/ranger/ranger_action.cc File src/kudu/ranger/ranger_action.cc: http://gerrit.cloudera.org:8080/#/c/15206/3/src/kudu/ranger/ranger_action.cc@45 PS3, Line 45: case Action::METADATA: : return ActionPB::METADATA; > According to Google C++ style guide[1] we're not using exceptions. I could Ah, sorry that I wasn't clear, I meant to log a fatal error for unknown actions similar to what we do in sentry_action. https://github.com/apache/kudu/blob/master/src/kudu/sentry/sentry_action.cc#L50 http://gerrit.cloudera.org:8080/#/c/15206/3/src/kudu/ranger/ranger_client.h File src/kudu/ranger/ranger_client.h: http://gerrit.cloudera.org:8080/#/c/15206/3/src/kudu/ranger/ranger_client.h@44 PS3, Line 44: // Authorizes listing tables. If there is at least one table that user is : // authorized to access metadata of, it returns OK and sets 'table_names' to : // the tables the user is authorized to list. Otherwise it returns : // NotAuthorized and it doesn't modify 'table_names'. : Status AuthorizeList(const std::string& user_name, : std::unordered_set<std::string>* table_names) WARN_UNUSED_RESULT; : : // Authorizes an action on the table. Returns OK if 'user_name' is authorized : // to perform 'action' on 'table_name', NotAuthorized otherwise. : Status AuthorizeAction(const std::string& user_name, const Action& action, : const std::string& table_name) WARN_UNUSED_RESULT; : : // Authorizes a scan on a table. Returns OK if 'user_name' is authorized to : // scan the whole table or at least one of the specified columns, : // NotAuthorized otherwise. If the user isn't authorized to scan the whole : // table, 'column_names' is changed to contain only the columns the user is : // authorized to scan. : Status AuthorizeScan(const std::string& user_name, const std::string& table_name, : std::unordered_set<std::string>* column_names) WARN_UNUSED_RESULT; What is the reasoning to have duplicate code here and just to keep ranger_authz_provider simpler? You can check sentry_authz_provider and sentry_privilege_fetcher, which I think is the opposite, that sentry_privilege_fetcher only has one 'generic' method for fetching privileges and methods in sentry_authz_provider implement necessary logic to use it. Again, this should also simplify the ranger protobuf definition to make it more clean and easier to use as (https://gerrit.cloudera.org/c/15074/4/src/kudu/ranger/ranger.proto). > I think it would be wasteful to convert the unordered set to vector and back, > especially as the communication between the ranger client and the subprocess > is on the pipe and we don't need to save bandwidth on the wire. You don't need to covert the unordered set to vector and back, I am suggesting to create a structure to hold all authz request related info and you can create a list of it. All you need is to have a mapping between the requests and the resources (tables or columns). -- To view, visit http://gerrit.cloudera.org:8080/15206 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie2e1ec19ed3aeb4d82ad38fe1fb655f57021c1a4 Gerrit-Change-Number: 15206 Gerrit-PatchSet: 5 Gerrit-Owner: Attila Bukor <[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: Thu, 13 Feb 2020 18:03:15 +0000 Gerrit-HasComments: Yes
