Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider ...................................................................... Patch Set 5: (21 comments) http://gerrit.cloudera.org:8080/#/c/11659/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11659/5//COMMIT_MSG@14 PS5, Line 14: future nit: ...in the future, other... http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h File src/kudu/master/default_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/default_authz_provider.h@32 PS4, Line 32: virtual Status AuthorizeCreateTable(const std::string& table_name, > Ignored, since those parameters are required in other implementation. The parameter is required, but the preferred way to add arguments that aren't use is to comment out the unused parameters, eg: Status AuthorizeCreateTable(const std::string& /*table_name*/, const std::string& /*user*/, ... http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@122 PS5, Line 122: void GetDatabasePrivilege(const string& db_name, : const string& action, : bool grant_option, : TSentryPrivilege* privilege) { nit: Any reason to not have this return the TSentryPrivilege directly? Same for GetTablePrivilege? http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@95 PS5, Line 95: Status CreateRoleAndAddToGroups(const string& role_name, const string& group_name) { : TCreateSentryRoleRequest role_req; : role_req.__set_requestorUserName(kAdminUser); : role_req.__set_roleName(role_name); : RETURN_NOT_OK(sentry_client_->CreateRole(role_req)); : : TSentryGroup group; : group.groupName = group_name; : set<TSentryGroup> groups; : groups.insert(group); : TAlterSentryRoleAddGroupsRequest group_request; : TAlterSentryRoleAddGroupsResponse group_response; : group_request.__set_requestorUserName(kAdminUser); : group_request.__set_roleName(role_name); : group_request.__set_groups(groups); : return sentry_client_->AlterRoleAddGroups(group_request, &group_response); : } : : Status AlterRoleGrantPrivilege(const string& role_name, const TSentryPrivilege& privilege) { : TAlterSentryRoleGrantPrivilegeRequest privilege_request; : TAlterSentryRoleGrantPrivilegeResponse privilege_response; : privilege_request.__set_requestorUserName(kAdminUser); : privilege_request.__set_roleName(role_name); : privilege_request.__set_privilege(privilege); : return sentry_client_->AlterRoleGrantPrivilege(privilege_request, &privilege_response); : } : : void GetDatabasePrivilege(const string& db_name, : const string& action, : bool grant_option, : TSentryPrivilege* privilege) { : privilege->__set_privilegeScope("DATABASE"); : privilege->__set_serverName(FLAGS_server_name); : privilege->__set_dbName(db_name); : privilege->__set_action(action); : if (grant_option) { : privilege->__set_grantOption(TSentryGrantOption::TRUE); : } : } : : void GetTablePrivilege(const string& db_name, nit: docs for these? http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@140 PS5, Line 140: privilege->__set_privilegeScope("DATABASE"); : privilege->__set_serverName(FLAGS_server_name); : privilege->__set_dbName(db_name); : privilege->__set_tableName(table_name); : privilege->__set_action(action); : if (grant_option) { : privilege->__set_grantOption(TSentryGrantOption::TRUE); : } nit: maybe GetDatabasePrivilege(...); privilege->__set_tableName(table_name); http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@177 PS5, Line 177: false nit: for boolean params, it's generally easier to read coming back to it as either an enum or by doing something like "/*grant_option=*/false", here and elsewhere http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@181 PS5, Line 181: , nit: comma not needed http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@270 PS5, Line 270: nit: spacing http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@288 PS5, Line 288: hostports, vector<HostPort>({ HostPort("foo-bar-baz", 1234) } Here and below, the expected value should come first. http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@295 PS5, Line 295: nit: spacing http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.h@57 PS5, Line 57: virtual Status Start() override; Probably worth doc'ing this, if its behavior is specific to Sentry http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.h@62 PS5, Line 62: Sentry service is unreachable, or 'kudu' (Kudu server name) is a non : // admin user in Sentry, or Sentry fails to resolve the group mapping of : // the user. Rather than repeating this in every comment, can you put this in the block comment at the top? It doesn't seem like these requirements vary from method to method. Also what is "Kudu server name" in this context? Does it correspond to an external constant? nit: "non-admin" with a hyphen http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.h@96 PS5, Line 96: FRIEND_TEST(SentryAuthzProviderStaticTest, TestParseAddresses); Could this use ValidateAddresses instead? How useful are the Statuses provided by ParseAddresses (vs just having the bool) http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@59 PS5, Line 59: Must match the value of the hive.sentry.server option " : "in the HiveServer2 configuration, and the value of the --server_name " : "in Impala configuration." How will this fail if these statements aren't true? http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@149 PS5, Line 149: old_table == new_table Does capitalization matter at this point? http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@259 PS5, Line 259: // Logs an error if the action is not authorized for debugging purpose, and : // only returns generic error back to the users to avoid side channel leak, : // e.g. 'whether table A exists'. Does the user's request have more information than what's presented in this error message? How would a user be able to determine whether table A exists if this message were returned? nit: Also maybe reword "Action <action> on authorizable <authorizable> is not permitted" or somesuch? Since the operation itself didn't fail; it's just the user isn't authorized, right? http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@262 PS5, Line 262: << "Failed to perform action [" << action.action() << "] on authorizable [" : << authorizable << "] for user [" << user << "]"; nit: use Substitute http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@264 PS5, Line 264: Substitute Remove http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@275 PS5, Line 275: authorizable->__set_table(table.ToString()); nit: maybe don't set these until we know we'll return OK? Is there a move constructor for TSentryAuthorizable? http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/sentry/sentry-test-base.h File src/kudu/sentry/sentry-test-base.h: http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/sentry/sentry-test-base.h@76 PS5, Line 76: kerberos_enabled nit: maybe end with a _? http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/sentry/sentry_client-test.cc File src/kudu/sentry/sentry_client-test.cc: http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/sentry/sentry_client-test.cc@56 PS5, Line 56: void SetUp() override { : SentryTestBase::SetUp(); : } : : void TearDown() override { : SentryTestBase::TearDown(); : } Don't need these since they're both the default. -- To view, visit http://gerrit.cloudera.org:8080/11659 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I254828d640cd905e33dbaf0fe100d660bc9e6772 Gerrit-Change-Number: 11659 Gerrit-PatchSet: 5 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 31 Oct 2018 05:08:52 +0000 Gerrit-HasComments: Yes
