Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11657 )
Change subject: [sentry] Fill out more sentry client API ...................................................................... Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/11657/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11657/3//COMMIT_MSG@9 PS3, Line 9: This commit adds more sentry client APIs, e.g list/grant sentry : privileges, for the upcoming sentry authorization provider. nit: please also add "add group" to this list http://gerrit.cloudera.org:8080/#/c/11657/3/src/kudu/sentry/sentry_client-test.cc File src/kudu/sentry/sentry_client-test.cc: http://gerrit.cloudera.org:8080/#/c/11657/3/src/kudu/sentry/sentry_client-test.cc@85 PS3, Line 85: ASSERT_OK(sentry_->Stop()); : ASSERT_OK(sentry_client_->Stop()); Does the order here matter? If so, I would've thought that we should stop the client first, in case there are requests in flight that might result in an error if sentry were stopped. http://gerrit.cloudera.org:8080/#/c/11657/3/src/kudu/sentry/sentry_client-test.cc@98 PS3, Line 98: if (!KerberosEnabled()) { Does this test not work with Kerberos enabled? http://gerrit.cloudera.org:8080/#/c/11657/3/src/kudu/sentry/sentry_client-test.cc@106 PS3, Line 106: SentryClient *client nit: SentryClient* client http://gerrit.cloudera.org:8080/#/c/11657/3/src/kudu/sentry/sentry_client-test.cc@221 PS3, Line 221: group_requset nit: typo http://gerrit.cloudera.org:8080/#/c/11657/3/src/kudu/sentry/sentry_client-test.cc@216 PS3, Line 216: ::sentry::TSentryGroup group; : group.groupName = "user"; : set<::sentry::TSentryGroup> groups; : groups.insert(group); : : ::sentry::TAlterSentryRoleAddGroupsRequest group_requset; : ::sentry::TAlterSentryRoleAddGroupsResponse group_response; : group_requset.requestorUserName = "joe-interloper"; : group_requset.roleName = "viewer"; : group_requset.groups = groups; : : Status s = sentry_client_->AlterRoleAddGroups(group_requset, &group_response); : ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString(); : : // Attempt to alter a non-exist role. : group_requset.requestorUserName = "test-admin"; : s = sentry_client_->AlterRoleAddGroups(group_requset, &group_response); : ASSERT_TRUE(s.IsNotFound()) << s.ToString(); : : // Alter role 'viewer' to add group 'user'. : ::sentry::TCreateSentryRoleRequest role_request; : role_request.requestorUserName = "test-admin"; : role_request.roleName = "viewer"; : ASSERT_OK(sentry_client_->CreateRole(role_request)); : ASSERT_OK(sentry_client_->AlterRoleAddGroups(group_requset, &group_response)); nit: Should this be its own test case? It seems pretty disconnected from testing grant privilege functionality. http://gerrit.cloudera.org:8080/#/c/11657/3/src/kudu/sentry/sentry_client-test.cc@262 PS3, Line 262: privilege_request.requestorUserName = "test-admin"; Not needed? -- To view, visit http://gerrit.cloudera.org:8080/11657 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I34695cd4cc6723b70617164d58f8681cefd09ddd Gerrit-Change-Number: 11657 Gerrit-PatchSet: 3 Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Comment-Date: Mon, 15 Oct 2018 18:23:46 +0000 Gerrit-HasComments: Yes
