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: (6 comments) a few comments, more are coming 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@77 PS3, Line 77: authorized_actions I understand that it's directly related to Sentry's terminology, but I'm not sure we want to continue confusing ourselves and others who might be reading this code... I would expect those to be named somehow that reflects the fact that those are granted privileges, not authorized actions. By my understanding, the AuthzProvider is the authority to authorize actions based on the set of granted privileges. What do you think? https://en.wikipedia.org/wiki/Privilege_(computing) https://www.postgresql.org/docs/9.0/sql-grant.html Maybe, name it 'granted_privileges' instead? 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 change into '... the user is granted privileges at ...'. http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@90 PS3, Line 90: apply to a single table for a single user. Is it going to be bound to the table only? http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/master/sentry_authz_provider.h@95 PS3, Line 95: const std::string& table_ident, I'm not sure I understand why this parameter is necessary given the description of this method. So, given some table 'A' and privileges that apply to it (say, at table level only), it's possible to use this method to deduce whether this set of privileges implies some action at a table 'B', maybe in other database? From the other point, if we are about to include object identifier as 'table_identifier' here to verify it matches with what is passed as a parameter, why not to include username as well? http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h File src/kudu/sentry/sentry_authorizable_scope.h: http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h@91 PS3, Line 91: AuthorizableScopesSet Is it possible to create the result AuthorizableScopesSet once and the just return references to those? E.g., maybe declare those as static inside the function and populate those just once. Also, is it really important to declare this function to be inline and in this header? Maybe, just move the definition into sentry_authz_provider.cc file (the only place it's used now, if I'm not mistaked) and drop the 'inline' specified to allow the compiler do the right thing? http://gerrit.cloudera.org:8080/#/c/12919/3/src/kudu/sentry/sentry_authorizable_scope.h@114 PS3, Line 114: inline AuthorizableScopesSet ExpectedNonEmptyFields(SentryAuthorizableScope::Scope scope) { : AuthorizableScopesSet expected_nonempty_fields; : switch (scope) { : case SentryAuthorizableScope::COLUMN: : InsertOrDie(&expected_nonempty_fields, SentryAuthorizableScope::COLUMN); : FALLTHROUGH_INTENDED; : case SentryAuthorizableScope::TABLE: : InsertOrDie(&expected_nonempty_fields, SentryAuthorizableScope::TABLE); : FALLTHROUGH_INTENDED; : case SentryAuthorizableScope::DATABASE: : InsertOrDie(&expected_nonempty_fields, SentryAuthorizableScope::DATABASE); : FALLTHROUGH_INTENDED; : case SentryAuthorizableScope::SERVER: : InsertOrDie(&expected_nonempty_fields, SentryAuthorizableScope::SERVER); : break; : default: : LOG(DFATAL) << "not reachable"; : } : return expected_nonempty_fields; : } ditto -- 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 <aw...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Thu, 04 Apr 2019 00:50:57 +0000 Gerrit-HasComments: Yes
