Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12833 )
Change subject: WIP [master] introduced SentryPrivilegesFetcher ...................................................................... Patch Set 9: (17 comments) http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc@157 PS9, Line 157: FLAGS_sentry_service_security_mode = KerberosEnabled() ? "kerberos" : "none"; Omit this? http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc@182 PS9, Line 182: kudu::master:: Drop, here and below http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc@426 PS9, Line 426: bool KerberosEnabled() const override { : return std::get<1>(GetParam()); : } I chatted with Hao about this briefly, and it seems like non-Kerberos isn't something we expect to be very common or interesting, so perhaps we should just pick KerberoseEnabled() = true for all of these? Maybe define it in the base class as true? http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.h@57 PS9, Line 57: privielge privilege http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.h@98 PS9, Line 98: Status GetSentryPrivileges(sentry::SentryAuthorizableScope::Scope scope, doc http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc@78 PS9, Line 78: granted_privilege : privilege.granted_privileges I strongly feel that we should keep granted_action. Even if an action, with the combination of a scope, is a privilege, the underlying object in question here is an action that has been granted at a given scope, not a privilege. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc@182 PS9, Line 182: if (DoPrivilegesImplyAction This reads weirdly ("if" and "do" never follow in an English sentence) and privileges don't imply actions -- they imply other privileges (ie a scope and an action). How about: if (PrivilegesImply(privileges, scope, action, require_grant_option)) Alternatively, just leave it as it was as a function call. Is there a benefit to pulling it out? http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.h File src/kudu/master/sentry_privileges_fetcher.h: http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.h@174 PS9, Line 174: // A utility method: sent request to Sentry using the aggregated : // sentry_client_. : Status FetchPrivilegeInfoFromSentry( nit: reword: "Send a request to fetch privileges from Sentry for the given authorizable." Also maybe rename it FetchSentryPrivileges or FetchPrivilegesFromSentry. It's not clear whether "privilege info" and "privileges" are different; would prefer not to introduce new terminology if they do mean the same thing. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.h@189 PS9, Line 189: // Broaden the authz scope, e.g. for COLUMN input authz scope the broaden : // result scope might be TABLE. : Status BroadenAuthzScope( : const sentry::SentryAuthorizableScope& scope, : sentry::SentryAuthorizableScope* broader_scope) const; Do we need this? Shouldn't we just CHECK that we don't pass in a column authorizable, since that would be a programmer error (not a bug, but flat out incorrect usage). I don't foresee us wanting to send a request for column privileges. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.h@201 PS9, Line 201: boost::optional<sentry::SentryAuthorizableScope> scope_depth_limit_; Why is this not some static constant? More generally, why is this configurable? It doesn't seem like it should be. Then BroadenAuthzScope() could be static too. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.cc@217 PS9, Line 217: const vector<string>& key_sequence() const { : return key_sequence_; : } Is this used anywhere? http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.cc@241 PS9, Line 241: const auto scope = scope_limit->scope(); : const size_t level = scope == SentryAuthorizableScope::Scope::SERVER : ? 0 : scope == SentryAuthorizableScope::Scope::DATABASE : ? 1 : scope == SentryAuthorizableScope::Scope::TABLE : ? 2 : scope == SentryAuthorizableScope::Scope::COLUMN : ? 3 : key_sequence_.size() - 1; : : if (level < key_sequence_.size()) { : return key_sequence_[level]; : } : return key_sequence_.back(); This would be more obvious as a switch statement IMO, instead of having to reason about all the ternaries or levels. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.cc@310 PS9, Line 310: uint64_t Does sizeof(SentryActionsSet) not work? http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.cc@384 PS9, Line 384: const string& scope_level_limit_str Use boost::optional<SentryAuthorizableScope::Scope>. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.cc@406 PS9, Line 406: // TODO(aserbin): maybe, return Status::InvalidParameters to requests : // with COLUMN scope instead? Seems like it would be a programmer error indicating they're using this function wrong, so D/CHECK is more appropriate. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.cc@591 PS9, Line 591: Status SentryPrivilegesFetcher::SentryResponseToPrivileges( This doesn't seem to produce a useful Status. This could return the SentryPrivilegesBranch directly. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.cc@663 PS9, Line 663: if (scope.scope() == SentryAuthorizableScope::Scope::UNINITIALIZED) { : return Status::InvalidArgument(Substitute("invalid authz scope: '$0'"), : sentry::ScopeToString(scope.scope())); : } DCHECK instead so it's clear that this is a programmer error. Also we don't need to return a Status. -- To view, visit http://gerrit.cloudera.org:8080/12833 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Idaefacd50736f1f152dae34e76778e17b2e84cbe Gerrit-Change-Number: 12833 Gerrit-PatchSet: 9 Gerrit-Owner: Alexey Serbin <aser...@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: Sat, 06 Apr 2019 08:24:11 +0000 Gerrit-HasComments: Yes
