Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add SentryAuthzProvider ...................................................................... Patch Set 3: (38 comments) http://gerrit.cloudera.org:8080/#/c/11659/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11659/3//COMMIT_MSG@7 PS3, Line 7: [sentry] SentryAuthzProvider > Nit: this is a little sparse; maybe convert into a sentence (or sentence fr Done http://gerrit.cloudera.org:8080/#/c/11659/3//COMMIT_MSG@9 PS3, Line 9: the Sentry > Nit: "above Sentry" Done http://gerrit.cloudera.org:8080/#/c/11659/3//COMMIT_MSG@13 PS3, Line 13: SentryAuthzPrivider > SentryAuthzProvider Done 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 Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@72 PS3, Line 72: if (enable_kerberos) { > +1, pull this out into some Sentry test base and use it here and in sentry_ Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@109 PS3, Line 109: ASSERT_OK(sentry_client_->Stop()); > Shouldn't we stop the client before the server? Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@121 PS3, Line 121: SentryAuthzProviderTest > Is it worth adding a few scenarios to verify how the provider works if Sent Done 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(); > Why is this an error at all? Why not just set `authorized` to false? How do This is an error returned from Sentry service. I think these errors should return it back to users. 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? Yeah, it is strange Sentry is returning as AccessDenied error. I filed a jira https://issues.apache.org/jira/browse/SENTRY-2434 to track this. 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 The extra line is to indicate the comment is for the following blocks. http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider-test.cc@146 PS3, Line 146: group_requset > group_request Done 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 fu Done 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 PrivilegeFor Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h File src/kudu/sentry/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@41 PS3, Line 41: the Sentry > just Sentry, not "the Sentry" Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@60 PS3, Line 60: // Check if the table creation operation on a given table is authorized > Nit: Start/Stop use present tense, so the rest of these methods should too. Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@61 PS3, Line 61: Return the authorization result in out parameter 'authorized', : // and only set it on success. > Why don't we merge 'authorized' directly into the returned Status? Isn't th Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@65 PS3, Line 65: fail > fails Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@71 PS3, Line 71: creation > deletion Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.h@93 PS3, Line 93: // Check if the table alteration with rename operation on a given table is > It's worth adding here (or above) why rename is split out of the rest of Au After a second thought, I think it would be better/more clean to combine these two methods together. Updated. 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: the > Nit: drop this Done 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 Done 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 This is sentry-specific, though I don't know what does it mean to call it 'Sentry server namespace'. I will rephrase the sentence to see if I can make it more clear. http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@55 PS3, Line 55: Configure > Configures Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@79 PS3, Line 79: SentryAuthzProvider::SentryAuthzProvider() { : } > Remove Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@121 PS3, Line 121: Status SentryAuthzProvider::AuthorizeAlterTable(const string& table_name, : const string& user, : bool* authorized) > nit: consider either setting '*authorized' to 'false' in the beginning or a Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@135 PS3, Line 135: // Table alteration (without table rename) requires: > But this is with rename... Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@157 PS3, Line 157: Slice database; : Slice table; : RETURN_NOT_OK(ParseHiveTableIdentifier(table_name, &database, &table)); > This isn't necessary for::SERVER. Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@170 PS3, Line 170: return Status::InvalidArgument("invalid authorizable scope"); > Seems like this should CHECK fail, no? The scopes are statically defined in Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@185 PS3, Line 185: privilege > nit: privileges Done 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?) Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@186 PS3, Line 186: valid > validate Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@187 PS3, Line 187: action > nit: actions Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@202 PS3, Line 202: Imply > The usage here suggests this should be called "Implies" rather than "Imply" Done 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 t Adding tests for this, so keep it as a class method. http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@222 PS3, Line 222: Status SentryAuthzProvider::ParseAddresses > nit: it would be nice if the order of methods in the .cc file matched the o Done http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_authz_provider.cc@234 PS3, Line 234: auto scheme_idx = uri.find(kSchemeSeparator, 1); > If the URL starts with "://", that'll fail in ParseString below? yeah, it should fail. http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc File src/kudu/sentry/sentry_client-test.cc: http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc@218 PS3, Line 218: request.__set_principalName("test-user"); > I'm curious what would be the response if not setting (or clearing/resettin It will return an 'Invalid Input' error. http://gerrit.cloudera.org:8080/#/c/11659/3/src/kudu/sentry/sentry_client-test.cc@223 PS3, Line 223: alter roles add groups > We're trying to write this in plain English, so why not "...Sentry service 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: 3 Gerrit-Owner: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 25 Oct 2018 23:46:44 +0000 Gerrit-HasComments: Yes
