Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] SentryAuthzProvider ...................................................................... Patch Set 3: (12 comments) http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc File src/kudu/sentry/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@72 PS3, Line 72: if (enable_kerberos) { > I think I've seen this block of code at least 3 times now. Can we consolida +1, pull this out into some Sentry test base and use it here and in sentry_client-test, which has almost identical methods. http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@127 PS3, Line 127: ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); > This is surprising; shouldn't it be NotFound() or something like that? Why is this an error at all? Why not just set `authorized` to false? How does the error make its way back to the user? http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@135 PS3, Line 135: // Authorize create table on a user without required privileges. : nit: extra line http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@137 PS3, Line 137: TCreateSentryRoleRequest role_req; : role_req.__set_requestorUserName("test-admin"); : role_req.__set_roleName("developer"); : ASSERT_OK(sentry_client_->CreateRole(role_req)); : : TSentryGroup group; : group.groupName = "user"; : set<TSentryGroup> groups; : groups.insert(group); : TAlterSentryRoleAddGroupsRequest group_requset; : TAlterSentryRoleAddGroupsResponse group_response; : group_requset.__set_requestorUserName("test-admin"); : group_requset.__set_roleName("developer"); : group_requset.__set_groups(groups); : ASSERT_OK(sentry_client_->AlterRoleAddGroups(group_requset, &group_response)); This is copied in every test, so maybe pull it into its own function (or functions, if there's a clean division of functionality). http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@204 PS3, Line 204: TSentryPrivilege privilege; : privilege.__set_privilegeScope("DATABASE"); : privilege.__set_serverName(FLAGS_server_name); : privilege.__set_dbName("db"); : privilege.__set_action("SELECT"); nit: We're using these _a lot_ so maybe it's worth introducing PrivilegeForDb() and PrivilegeForTable() methods? http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc File src/kudu/sentry/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@45 PS3, Line 45: RPC addresses of the Sentry service(s) nit: Call out that it's a comma-separated list http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@55 PS3, Line 55: server Is this a Sentry-specific thing? Hive-specific? Does it make sense to refer to this as "Sentry server namespace" or "Hive server namespace" instead? http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@79 PS3, Line 79: SentryAuthzProvider::SentryAuthzProvider() { : } Remove http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@185 PS3, Line 185: privilege nit: privileges http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@186 PS3, Line 186: implies nit: imply (the privileges imply the authorizable, right?) http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@187 PS3, Line 187: action nit: actions http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@211 PS3, Line 211: : bool SentryAuthzProvider::ValidateAddresses(const char* flag_name, : const string& addresses) { : vector<HostPort> host_ports; : Status s = ParseAddresses(addresses, &host_ports); : if (!s.ok()) { : LOG(ERROR) << "invalid flag " << flag_name << ": " << s.ToString(); : } : return s.ok(); : } If this is only used as a flag validator, it doesn't need to be a part of this class at all, right? Could just be a regular static method, or put in an anonymous namespace. -- 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: 3 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, 24 Oct 2018 06:10:48 +0000 Gerrit-HasComments: Yes
