Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )
Change subject: KUDU-2972 Add Ranger client ...................................................................... Patch Set 23: (5 comments) http://gerrit.cloudera.org:8080/#/c/15206/23/src/kudu/ranger/ranger_client.cc File src/kudu/ranger/ranger_client.cc: http://gerrit.cloudera.org:8080/#/c/15206/23/src/kudu/ranger/ranger_client.cc@118 PS23, Line 118: string db; : Slice tbl; : : auto s = ParseRangerTableIdentifier(table_name, &db, &tbl); : if (PREDICT_FALSE(!s.ok())) { : LOG(WARNING) << Substitute(kDenyNonRangerTableTemplate, table_name); : return Status::NotAuthorized(kUnauthorizedAction); : } : : RangerRequestListPB req_list; : RangerResponseListPB resp_list; : req_list.set_user(user_name); : : RangerRequestPB* req = req_list.add_requests(); : : req->set_action(action); : req->set_database(db); : req->set_table(tbl.ToString()); : : RETURN_NOT_OK(subprocess_.Execute(req_list, &resp_list)); : : CHECK_EQ(1, resp_list.responses_size()); : if (resp_list.responses().begin()->allowed()) { : return Status::OK(); : } : : LOG(WARNING) << Substitute("User $0 is not authorized to perform $1 on $2", : user_name, ActionPB_Name(action), table_name); : return Status::NotAuthorized(kUnauthorizedAction); nit: Could this be: return AuthorizeActions(user_name, table_name, { action }); or return AuthorizeActionMultipleTables(user_name, action, { table_name }); ? It'd reduce code complexity to reduce the number of implementations of roughly the same thing, assuming there isn't a huge perf cost. http://gerrit.cloudera.org:8080/#/c/15206/23/src/kudu/ranger/ranger_client.cc@234 PS23, Line 234: bool any_allowed = false; : for (auto i = 0; i < orig_table_names.size(); i++) { : if (resp_list.responses(i).allowed()) { : if (!any_allowed) { : any_allowed = true; : table_names->clear(); : } : table_names->emplace(orig_table_names[i]); : } : } : : if (!any_allowed) { : LOG(WARNING) << Substitute("User $0 is not authorized to perform $1 on $2 tables", : user_name, ActionPB_Name(action), table_names->size()); : return Status::NotAuthorized(kUnauthorizedAction); : } nit: may be simpler as unordered_set<string> allowed_tables; for (int i = 0; i < orig_table_names.size(); i++) { if (resp_list.responses(i).allowed()) { EmplaceOrDie(&allowed_tables, std::move(orig_table_names[i])); } } if (allowed_tables.empty()) { // return error } *table_names = std::move(allowed_tables); Same elsewhere. http://gerrit.cloudera.org:8080/#/c/15206/23/src/kudu/ranger/ranger_client.cc@295 PS23, Line 295: LOG(WARNING) << Substitute("User $0 is not authorized to perform $1 actions on table $2", nit: consider using JoinMapped() to output the actions instead of just the count? http://gerrit.cloudera.org:8080/#/c/15206/23/src/kudu/ranger/ranger_client.cc@308 PS23, Line 308: !FLAGS_ranger_jar_path.empty() ? : FLAGS_ranger_jar_path : : bin_dir + "/kudu-subprocess.jar"; nit: it'd be cleaner to read this without the negative. Also consider using JoinPathSegments? E.g. string jar_path = FLAGS_ranger_jar_path.empty() ? JoinPathSegments(bin_dir, "kudu-subprocess.jar") : FLAGS_ranger_jar_path; http://gerrit.cloudera.org:8080/#/c/15206/23/src/kudu/subprocess/subprocess_proxy.h File src/kudu/subprocess/subprocess_proxy.h: http://gerrit.cloudera.org:8080/#/c/15206/23/src/kudu/subprocess/subprocess_proxy.h@83 PS23, Line 83: void ReplaceServerForTests(SubprocessServer* server) { : server_ = std::unique_ptr<SubprocessServer>(server); nit: It'd probably be less error-prone to pass a unique_ptr<> and std::move() here. That way callers aren't able to misuse the with raw pointers. -- 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: 23 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: Wed, 04 Mar 2020 22:12:57 +0000 Gerrit-HasComments: Yes
