Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )
Change subject: KUDU-2972 Add Ranger client ...................................................................... Patch Set 16: (11 comments) http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger.proto File src/kudu/ranger/ranger.proto: http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger.proto@27 PS16, Line 27: // SELECT - select rows in a table : // INSERT - insert to a table : // UPDATE - update existing rows in a table : // DELETE - delete rows in a table : // ALTER - alter schema : // CREATE - creating a table : // DROP - drop a table > It's not your fault, as this is introduced in the plugin patch. But if you Agreed re: the usefulness of linking to existing docs, though we shouldn't link to vendor-specific documentation. Is there an Apache Kudu page we could use? http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client-test.cc File src/kudu/ranger/ranger_client-test.cc: http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client-test.cc@55 PS12, Line 55: return user_name == rhs.user_name && > Partially done, but I think I'm missing something. How would I inject the m Hmm, that may not work, but at least you could convert the shared ownership into exclusive ownership. http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client-test.cc File src/kudu/ranger/ranger_client-test.cc: http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client-test.cc@68 PS16, Line 68: struct hash<kudu::ranger::AuthorizedAction> { Again, define a hasher rather than overloading std::hash. http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client-test.cc@112 PS16, Line 112: CHECK(req->request().UnpackTo(&req_list)); Could just as easily RETURN_NOT_OK. http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client-test.cc@128 PS16, Line 128: auto packed_resp = new Any(); : packed_resp->PackFrom(resp_list); : resp->set_allocated_response(packed_resp); Could maybe do resp->mutable_response()->PackFrom(resp_list)? http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h File src/kudu/ranger/ranger_client.h: http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@59 PS16, Line 59: vector It's not a vector. Given how similar this is to AuthorizeAction, perhaps restyle the doc to resemble it, so the differences are more stark? I'd also rename all of these to clarify their use; there are subtle differences between each AuthorizeAction overload and there's no real reason for them to be overloads. http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@82 PS16, Line 82: : namespace std { : template<> : struct hash<kudu::ranger::ActionPB> { : int operator()(const kudu::ranger::ActionPB action) const { : return action; : } : }; This is a no-no in the Google Style Guide: https://google.github.io/styleguide/cppguide.html#std_hash Instead, just define your own hasher and use it in the various ActionPB unordered_sets. http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc File src/kudu/ranger/ranger_client.cc: http://gerrit.cloudera.org:8080/#/c/15206/12/src/kudu/ranger/ranger_client.cc@67 PS12, Line 67: CHECK(sresp.response().UnpackTo(resp)); > added a DCHECK, do you think that's enough? Curious what would be the problem with RETURN_NOT_OK, given we're OK doing that on L65? http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc File src/kudu/ranger/ranger_client.cc: http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc@79 PS16, Line 79: As these tables cannot be : // whitelisted in Ranger we should allow access to them to avoid making tables : // inaccessible with Ranger integration enabled > I think we should disallow the access to such tables to follow the deny by I tend to agree. Like with Sentry (though that was due to HMS integration), aren't we assuming that in order to set up Ranger integration, one must run CLI tools to massage existing tables into shape? http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc@156 PS16, Line 156: return Status::NotAuthorized(Substitute("User $0 is not authorized to " BTW, the Sentry provider doesn't provide any detail besides "unauthorized action" on purpose: // Log a warning if the action is not authorized for debugging purpose, and // only return a generic error to users to avoid a side channel leak, e.g. // whether table A exists. LOG(WARNING) << Substitute("Action <$0> on table <$1> with authorizable scope " "<$2> is not permitted for user <$3>", sentry::ActionToString(action), table_ident, sentry::ScopeToString(scope), user); return Status::NotAuthorized("unauthorized action"); I presume we'd want to follow the same policy here. http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/subprocess/server.cc File src/kudu/subprocess/server.cc: http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/subprocess/server.cc@155 PS16, Line 155: if (write_thread_ != nullptr) { Nit: it's C++, so you can simplify to: if (write_thread_) { ... } -- 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: 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:11:38 +0000 Gerrit-HasComments: Yes
