Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12919 )
Change subject: sentry: sanitize and parse privileges from Sentry ...................................................................... Patch Set 11: (11 comments) http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: PS11: > With all the new tests, could you double check the NUM_SHARDS value? Does i That has been addressed to have more shards: https://gerrit.cloudera.org/#/c/12833/10/src/kudu/master/CMakeLists.txt http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider-test.cc@439 PS11, Line 439: // Parameterized by whether Kerberos should be enabled. > sentry_authz_provider-test extensively parameterizes with Kerberization, bu It seems this piece hasn't been changed a lot, it's just an artifact of diff-ing. As for the Kerberization, I don't think it's related to the internals of Kudu authz, but rather to the ways of Kudu trusted user (i.e. the OS user under which kudu-master runs) to authenticate with Sentry. By my understanding, the Kerberized parameterisation is a common pattern for the majority of test scenarios in this file. http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@87 PS11, Line 87: sentry::SentryActionsSet granted_privileges; > Might want to name this granted_actions; 'privileges' makes me think of the I'm not sure why privilege should imply authorizable, but action should not. If you don't feel strongly against this, I'm going to keep it as is for now. http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.h@102 PS11, Line 102: the : // given table. > This was a bit confusing as there's no table argument. My takeaway (without As far as I see this is enforced in SentryAuthzProvider::GetSentryPrivileges() where an instance of SentryPrivilegesBranch is populated. http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@371 PS11, Line 371: DCHECK_EQ(FLAGS_server_name, requested_authorizable.server); > IIUC, invariant violations on requested_authorizable trigger DCHECKs, while Done http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@424 PS11, Line 424: if (!privilege.__isset.columnName || privilege.columnName.empty()) { > This is also just defensive. I think having '__isset' set with an empty value in corresponding field is a sign of messed-up TSentryPrivilege for particular context, so I think it makes sense to validate that unless we have some guarantees from Thrift (I'm not sure we have such guarantees from the protocol side: at least I cannot see it from the sources where TSentryPrivilege class is defined). http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@431 PS11, Line 431: !boost::iequals(privilege.tableName, requested_authorizable.table))) { > Sentry authorizable names are case-insensitive? Is there a definitive refer By my understanding Sentry authorizables are case-insensitive (databases, tables, columns): that comes from the properties of the Kudu-HMS integration. I added a comment on that into the code published at https://gerrit.cloudera.org/#/c/12833/ I didn't understand the 'kServer' part of your comment. http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@472 PS11, Line 472: DCHECK(!db.empty() && !table.empty()); > Conjunct DCHECKs are hard to debug when they trigger; it's never clear whic Done http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@501 PS11, Line 501: unordered_map<string, AuthorizablePrivileges> privileges_map; > I think the logic from here on down might be better suited as an "initializ I moved this into SentryPrivilegesBranch::Init() method. http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/master/sentry_authz_provider.cc@524 PS11, Line 524: privilege.all_with_grant = true; > It should be fine to have a grant option on any action; but for Kudu we onl I added VLOG(1) message for that. http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/sentry/sentry_action.h File src/kudu/sentry/sentry_action.h: http://gerrit.cloudera.org:8080/#/c/12919/11/src/kudu/sentry/sentry_action.h@64 PS11, Line 64: static const size_t kMaxAction = Action::OWNER + 1; > You might find the templates and macros in "Enumeration Casts and Tests" (g Ack. -- 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: 11 Gerrit-Owner: Andrew Wong <aw...@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: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 09 Apr 2019 19:07:06 +0000 Gerrit-HasComments: Yes
