Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/15206 )
Change subject: KUDU-2972 Add Ranger client ...................................................................... Patch Set 17: (19 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: // The action type mapping is similar to the one in Sentry which was implemented : // before Ranger and the same privileges have to be enforced with both : // authorization providers. : // : // For more information on fine grained authz check docs/security.adoc : // : // SELECT - select row > Agreed re: the usefulness of linking to existing docs, though we shouldn't Done 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: struct AuthorizedAction { > Hmm, that may not work, but at least you could convert the shared ownership hm SubprocessServer has an atomic member (next_id_) so it's not copy/move constructible. Do you think it's worth adding a move constructor to it so we can use a unique_ptr here? Or can you think of a better approach? 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: } > Again, define a hasher rather than overloading std::hash. Done http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client-test.cc@112 PS16, Line 112: > Could just as easily RETURN_NOT_OK. RETURN_NOT_OK only works for methods that return a Status, this returns a boolean. I can return a runtime error or something, but this should always work, unless an incorrect protobuf message is returned which doesn't seem too likely and would indicate a serious error. http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client-test.cc@128 PS16, Line 128: : void SetUp() override { : server_->next_response_ = &next_response_; > Could maybe do resp->mutable_response()->PackFrom(resp_list)? Done 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@38 PS16, Line 38: eturn > nit: Creates? Done http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@47 PS16, Line 47: > nit: indent. Done http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@54 PS16, Line 54: d::strin > Same here. Done http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@59 PS16, Line 59: > It's not a vector. Done http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@60 PS16, Line 60: > nit: is authorized to access if OK status is returned. Done http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@62 PS16, Line 62: : > Same here. Done http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@69 PS16, Line 69: > nit: should it be named to AuthorizeActions given its functionality? Done http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@70 PS16, Line 70: std::unordered_set<std::string>* column_names) : WARN_UNUSED_RESULT; > And here. Done http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.h@82 PS16, Line 82: // Sends request to the subprocess and parses the response. : Status SendRequest(RangerRequestListPB* req, RangerResponseListPB* resp) WARN_UNUSED_RESULT; : : std::shared_ptr<kudu::subprocess::SubprocessServer> server_; : }; : : } // namespace ranger : } > This is a no-no in the Google Style Guide: https://google.github.io/stylegu Done 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@55 PS16, Line 55: atus RangerClient::Star > Add a TODO here if you are going to have a follow up patch to actually call this will only call init, ranger authz provider sets up the server, do you think that's a good approach? if it is, I don't think it's necessary to add a todo here. http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc@67 PS16, Line 67: RETURN_NOT_OK(server_->Execute(&sreq, > Maybe use DCHECK instead? that was the original approach, but it got optimized out and it segfaulted in release mode. As this needs to run anyway, I think between storing the result in a variable and DCHECK on that and simply CHECK I think it's better to CHECK, but let me know if you disagree. http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc@79 PS16, Line 79: : auto s = ParseRangerTableIdentifier(table_name, &db, &tbl); : if (PREDICT_FALSE(!s.ok())) { > I tend to agree. Like with Sentry (though that was due to HMS integration), Even with Sentry only conflicting tables had to be renamed. Tables with names that are invalid in Sentry are unmodified. OTOH those tables are denied in Sentry too so I'm changing it to deny non-Ranger tables and log a warning. http://gerrit.cloudera.org:8080/#/c/15206/16/src/kudu/ranger/ranger_client.cc@156 PS16, Line 156: return Status::OK(); > BTW, the Sentry provider doesn't provide any detail besides "unauthorized a Done 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_) { > Nit: it's C++, so you can simplify to: Done -- 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: 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:16 +0000 Gerrit-HasComments: Yes
