Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11797 )
Change subject: [sentry] Integrate AuthzProvider into CatalogManager ...................................................................... Patch Set 6: (17 comments) Just noticed a new patch set has been uploaded, so I decided to publish whatever I have right now and continue further with the fresh version. http://gerrit.cloudera.org:8080/#/c/11797/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11797/5//COMMIT_MSG@10 PS5, Line 10: SentryAuthzProvider Is that still SentryAuthzProvider or some generic authz provider interface? http://gerrit.cloudera.org:8080/#/c/11797/5//COMMIT_MSG@12 PS5, Line 12: DDLs DDL operations http://gerrit.cloudera.org:8080/#/c/11797/5//COMMIT_MSG@14 PS5, Line 14: Note that the coarse-grained : access control is still applied to these endpoints. A --trusted_user_acl : flag is introduced to allow the trusted user, e.g. 'impala', to skip the : authorization enforcement. Why would the mix of coarse and fine authz be preferred to the pure fine authz scheme and adding the trusted users as super-users in the authz provider? http://gerrit.cloudera.org:8080/#/c/11797/5//COMMIT_MSG@20 PS5, Line 20: exposedp exposed http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/authz_provider.h File src/kudu/master/authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/authz_provider.h@75 PS5, Line 75: static Why is this enforced to be static? If this cannot/should not be customizable per authz provider, why to put it into the AuthzProvider interface at all? http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/catalog_manager.h@542 PS5, Line 542: rpc::RpcContext* Nit for here and elsewhere: if only constant methods are called on RpcContext, why not to add 'const' specifier? I.e., 'const rpc::RpcContext* ctx'? http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/catalog_manager.h@542 PS5, Line 542: rpc I think 'rpc' is a misnomer here, since it's more about a context than PRC. Maybe, 'ctx' or 'context'? http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/catalog_manager.h@859 PS5, Line 859: Alice drops table B and renames table B to A Alice first drops table B and then renames table A to B http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/catalog_manager.h@865 PS5, Line 865: One exception is when the user is authorized to operate on table, : // then TABLE_NOT_FOUND error status is returned. How is this possible if first the lock is taken on the table prior to checking for authz? I.e., if the table is gone already, then the lock cannot be held, right? If so, how is it possible to tell that the user is authorized to operate on the table? Or that's more about ALL privilege on the whole database? http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/catalog_manager.cc@1410 PS5, Line 1410: string nit: const string& ? http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/catalog_manager.cc@1411 PS5, Line 1411: string ditto http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/sentry_authz_provider-test-base.h File src/kudu/master/sentry_authz_provider-test-base.h: PS5: Why not to extract the implementation of the methods into a .cc file? Also, if doing that, maybe put it into a separate patch? http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/sentry_authz_provider.h@91 PS5, Line 91: Note that the authorization process is case insensitive for the : // authorizables. nit: replace with 'Note that authorizables are case insensitive.' http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/sentry_authz_provider.cc@200 PS5, Line 200: AuthzProvider:: nit: drop since SentryAuthzProvider is a sub-class of AuthzProvider? http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/sentry_authz_provider.cc@225 PS5, Line 225: AuthzProvider:: ditto http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/sentry_authz_provider.cc@239 PS5, Line 239: AuthzProvider:: ditto http://gerrit.cloudera.org:8080/#/c/11797/5/src/kudu/master/sentry_authz_provider.cc@267 PS5, Line 267: AuthzProvider:: ditto -- To view, visit http://gerrit.cloudera.org:8080/11797 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iab4aa027ae6eb4520db48ce348db552c9feec2a8 Gerrit-Change-Number: 11797 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, 07 Feb 2019 22:10:30 +0000 Gerrit-HasComments: Yes
