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 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@132 PS3, Line 132: Why just the server? This guarantees that a request for any : // authorizable scope will ignore such invalid privileges. Yes, I understand the reasoning here: the responses are from real Sentry instance, and that's about end-to-end verification. What do you think about adding sort of unit test for SentryPrivilegeIsWellFormed() (currently file-scope-private of sentry_authz_provider.cc) where the fields of TSentryPrivilege are being messed up mercilessly? http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@171 PS3, Line 171: : style nit: it seems the indent should be four spaces; a space after the column is needed http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider-test.cc@264 PS3, Line 264: static const vector<InvalidPrivilege> kInvalidPrivileges = { : InvalidPrivilege::INCORRECT_ACTION, : InvalidPrivilege::INCORRECT_SCOPE, : InvalidPrivilege::INCORRECT_SERVER, : InvalidPrivilege::FLIPPED_FIELD, : }; syntax sugar addict's nit: could it be just static constexpr InvalidPrivilege kInvalidPrivileges[] = { ... }; ? http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@87 PS3, Line 87: the privileges apply. > I'm not sure I understand what 'apply' means in this context. So, maybe ch Whoops, it should have been '... the user is granted privileges on ...' http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.cc@325 PS3, Line 325: SentryPrivilegeIsWellFormed Why not IsSentryPrivilegeWellFormed(...) ? http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.cc@463 PS3, Line 463: continue; Do you think it's worth logging a message in that case? Maybe LOG(INFO) or at least VLOG(1) ? http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_action.h File src/kudu/sentry/sentry_action.h: http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_action.h@53 PS3, Line 53: ALL, Not from this particular changelist, but while you are at it: maybe, move ALL closer to OWNER (e.g. insert it between 'DROP' and 'OWNER')? -- 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: 3 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 04 Apr 2019 02:18:11 +0000 Gerrit-HasComments: Yes
