Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry ...................................................................... Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.h@42 PS5, Line 42: nit: use http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.h@73 PS5, Line 73: LOG(FATAL) << "not reachable"; : } : #endif I am not sure why grant option has to work with privilege 'ALL'? I believe in Sentry grant option can work with any actions. It looks like you are doing this because it is hard to wire grant option with each action, and we only requires grant option for CreateTable which requires 'ALL on Database'? If it is the case, would you mind add a comment here. And we may need to revisit this if there is some operation requires action other than 'ALL' with grant. http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.h@92 PS5, Line 92: It would be more clear to name it as SentryPrivilegesInSameBranch or something alike? To clarify this is Sentry privileges from the same hierarchy branch. http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@176 PS5, Line 176: (SentryAuthorizableScope(privilege.scope).Implies(scope)) { : for (const auto& granted_action : privilege.granted_privileges) { : if (SentryAction(granted_action).Implies(action)) { : return true; : As we are using FixedBitSet to store all actions granted to an authorizable, do you think it makes sense to have a bulk version of 'Implies' method, where check if a SentryActionsSet can imply a single action (using the util of FixedBitSet, e.g 'contains') without iteration through each action? The motivation of this is to have a faster lookup. http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@362 PS5, Line 362: SentryAuthorizableScope::Scope* scope, : SentryAction::Action* action) { : DCHECK_EQ(FLAGS_server_name, requested_authori Should these be moved to L488 to avoid duplicated checks? http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@406 PS5, Line 406: nit: Granted? http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@513 PS5, Line 513: cop Since OWNER is considered as equivalent to ALL, we may need to add OWNER as well. Would you mind adding test such case in TestTableAuthorization.TestAuthorizeCreateTable? -- To view, visit http://gerrit.cloudera.org:8080/12919 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib6de6814f99abfbee4f030298b74f21f4e7c729b Gerrit-Change-Number: 12919 Gerrit-PatchSet: 7 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 04 Apr 2019 21:27:54 +0000 Gerrit-HasComments: Yes
