Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/11659 )
Change subject: [sentry] add AuthzProvider ...................................................................... Patch Set 7: (9 comments) http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h File src/kudu/master/authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/authz_provider.h@33 PS7, Line 33: virtual Status Start() { return Status::OK(); } : : // Stops the AuthzProvider instance. : virtual void Stop() {} > nit: if you have DefaultAuthzProvider with dummy implementation of all auth Sure, but any specific reasons for doing this? 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@270 PS5, Line 270: > That's what we do for wrapping long lines. Ah, thanks! http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h@55 PS7, Line 55: This following methods will fail if the operation is not authorized, or the : // 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. > How about we split these out so it's a bit easier to read. Done http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.h@90 PS7, Line 90: // : // This method will fail if the Sentry service is unreachable, or 'kudu' : // (Kudu server name) is a non admin user in Sentry, or Sentry fail to : // resolve the group mapping of the user. > nit: not needed given the comment up top? Done 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@259 PS5, Line 259: return Status::OK(); : } : > Ah, thanks for the explanation. So based on where the Authorize() fails, a Yeah, which we should be careful about what to return in the caller of AuthzProvider. http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@50 PS7, Line 50: sentry.service.client.server.rpc-addresses > This seems confusing: ...service.client.server.... Is there any typo in th No... this is the name defined in Sentry. http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@56 PS7, Line 56: server_name > If that's to describe server namespace in Sentry terms, why not 'sentry_ser Sentry has server level privilege and this is used to configure that. The idea is to define server namespace in Kudu to support authorization on multi clusters. Thus, 'sentry_server_namespace' sounds misleading to me. http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@115 PS7, Line 115: on > super nit: elsewhere we seem to favor capitalizing the whole of "ALL ON DAT Done http://gerrit.cloudera.org:8080/#/c/11659/7/src/kudu/master/sentry_authz_provider.cc@209 PS7, Line 209: > nit: spacing here too 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: 7 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, 01 Nov 2018 17:58:49 +0000 Gerrit-HasComments: Yes
