Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry ...................................................................... Patch Set 10: (9 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 No, this is correct. I.e. "this is preferred instead of using..." or "this is preferred over using..." 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 Done http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.h@92 PS5, Line 92: string column_na > It would be more clear to name it as SentryPrivilegesInSameBranch or someth A bit verbose, I went with SentryPrivilegesBranch, since it's the privileges of a branch of the Sentry privilege tree. 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: 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 Maybe, but not as a part of this patch. I doubt this will be the bottleneck; maybe if we begin to see a lot of this similar code around, or if we see that this is a slow point, then sure. I'm doubtful that will be the case though. http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@362 PS5, Line 362: } : return kColumnFields; : } > Should these be moved to L488 to avoid duplicated checks? No, I want these checks to happen for every function that may use this function (e.g. in tests when calling it directly). It wont show up in release build anyway. http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@406 PS5, Line 406: > nit: Granted? Done http://gerrit.cloudera.org:8080/#/c/12919/5/src/kudu/master/sentry_authz_provider.cc@513 PS5, Line 513: > Since OWNER is considered as equivalent to ALL, we may need to add OWNER as Interesting, seems like OWNER could use some more test coverage in general. http://gerrit.cloudera.org:8080/#/c/12919/7/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12919/7/src/kudu/master/sentry_authz_provider.cc@319 PS7, Line 319: static const AuthorizableScopesSet kDbFields{ SentryAuthorizableScope::TABLE, : SentryAuthorizableScope::COLUMN }; : static const AuthorizableScopesSet kTableFields > This is going to be created every time this function called, no? Done http://gerrit.cloudera.org:8080/#/c/12919/7/src/kudu/master/sentry_authz_provider.cc@335 PS7, Line 335: return kColumnField > Why not to return const reference here? Done -- 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: 10 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 22:53:26 +0000 Gerrit-HasComments: Yes
