Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider ...................................................................... Patch Set 6: (27 comments) http://gerrit.cloudera.org:8080/#/c/11659/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11659/5//COMMIT_MSG@13 PS5, Line 13: is placed in th > is placed in the Done http://gerrit.cloudera.org:8080/#/c/11659/5//COMMIT_MSG@14 PS5, Line 14: the fu > nit: ...in the future, other... Done http://gerrit.cloudera.org:8080/#/c/11659/5//COMMIT_MSG@14 PS5, Line 14: in the fu > in the future Done 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*/, > The parameter is required, but the preferred way to add arguments that aren Done 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@78 PS5, Line 78: : Status StopSentry() { : RETURN_NOT_OK(sentry_client_->Stop()); : > If that's all this does, you don't need it. Done http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@122 PS5, Line 122: const TSentryGrantOption::type& grant_option) { : TSentryPrivilege privilege; : privilege.__set_privilegeScope("DATABASE"); : privilege.__set_serverName(FLAGS_server_name); > nit: Any reason to not have this return the TSentryPrivilege directly? Same Done http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@95 PS5, Line 95: 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); : } : : // Returns a database level TSentryPrivilege with the given database name, action : // and grant option. : TSentryPrivilege GetDatabasePrivilege(const string& db_name, : const string& action, : const TSentryGrantOption::type& grant_option) { : TSentryPrivilege privilege; : privilege.__set_privilegeScope("DATABASE"); : privilege.__set_serverName(FLAGS_server_name); : privilege.__set_dbName(db_name); : privilege.__set_action(action); : privilege.__set_grantOption(TSentryGrantOption::TRUE); : return privilege; : } : : // Returns a table level TSentryPrivilege with the given table name, database name, : // action and grant option. : TSentryPrivilege GetTablePrivilege(const string& db_name, : const stri > nit: docs for these? Done http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@140 PS5, Line 140: return privilege; : } : : protected: : unique_ptr<SentryAuthzProvider> sentry_authz_provider_; : }; : : INSTA > nit: maybe Done http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@177 PS5, Line 177: , "AL > nit: for boolean params, it's generally easier to read coming back to it as Done http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@181 PS5, Line 181: > nit: comma not needed Done http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@270 PS5, Line 270: > nit: spacing I thought wrapped function has 4 space indent? http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@288 PS5, Line 288: > Here and below, the expected value should come first. Done http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider-test.cc@295 PS5, Line 295: > nit: spacing Done 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: // user in Sentry, or Sentry fai > Probably worth doc'ing this, if its behavior is specific to Sentry Done http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.h@62 PS5, Line 62: const std::string& user, : const std::string& owner) override WARN_UNUSED_RESULT; : > Rather than repeating this in every comment, can you put this in the block Right, it corresponds to the config 'sentry.service.admin.group' in Sentry Server. After a second though I think it might be good to expose it as a gflag as well. http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.h@96 PS5, Line 96: const std::string& user, > Could this use ValidateAddresses instead? How useful are the Statuses provi Remove this to https://gerrit.cloudera.org/#/c/11843/. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.h@111 PS4, Line 111: : > I'm not Dan but I wanted to make sure this was chased down. Is the API you' Currently, connection to Sentry server is guarded by 'sentry.service.allow.connect' config, that only the trusted service users can connect to the Sentry service. Therefore, I fail to see the reasons this is less safe than other Sentry APIs. http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/4/src/kudu/master/sentry_authz_provider.cc@187 PS4, Line 187: > I'm inclined to agree with Hao: reuse the bulk of ParseStrings() to share c Put it to https://gerrit.cloudera.org/#/c/11843/. 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: Server2 configuration, and the value of the --server_name " : "in Impala configuration."); : TAG_FLAG(server_name, experimental); > How will this fail if these statements aren't true? Different server name implies different privilege is checked. That means the authorization might fail or the user might be able to access databases/tables belong to other server that she/he doesn't have privileges. Though I don't see a way Kudu can detect that without connecting to HiveServer2 and Impala. http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@149 PS5, Line 149: 2. 'CREATE ON DATABASE > Does capitalization matter at this point? Kudu does enforce case sensitivity for table names. Even though with HMS integration enabled Kudu follows HMS table naming rule, the catalog manager has logic to normalize the table name. So I think it makes sense to do a case sensitive comparison. http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@259 PS5, Line 259: return Status::OK(); : } : > Does the user's request have more information than what's presented in this For example, if user Bob has ALL privilege on table B and he wants to check if table A exists. He can alter table B to A. If he gets the error message with 'alter to table A is not permitted' instead of AlreadyPresent, then he knows that table A already does not exist. http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@262 PS5, Line 262: e master : } // namespace kudu > nit: use Substitute I don't see there is a method in auto-gen to convert TSentryAuthorizable to string? http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@264 PS5, Line 264: > Remove Done http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/master/sentry_authz_provider.cc@275 PS5, Line 275: > nit: maybe don't set these until we know we'll return OK? Is there a move c Yeah, I don't think here it matters. Since ParseHiveTableIdentifier in L279 should not be called again the call in L274 succeeds. 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 _? Done 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: : TEST_P(SentryClientTest, TestMiniSentryLifecycle) { : // Create an HA Sentry client and ensure it automatically reconnects after service interruption. : thrift::HaClient<SentryClient> client; : thrift::ClientOptions sentry_client_opts; : if (kerberos_enabled_) { : > Don't need these since they're both the default. Done http://gerrit.cloudera.org:8080/#/c/11659/5/src/kudu/sentry/sentry_client-test.cc@56 PS5, Line 56: : TEST_P(SentryClientTest, TestMiniSentryLifecycle) { : // Create an HA Sentry client and ensure it automatically reconnects after service interruption. : thrift::HaClient<SentryClient> client; : thrift::ClientOptions sentry_client_opts; : if (kerberos_enabled_) { : > Here too; if all this is doing is calling the superclass, it can be omitted Done -- 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: 6 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: Thu, 01 Nov 2018 03:39:25 +0000 Gerrit-HasComments: Yes
