Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12833 )
Change subject: [master] introduce SentryPrivilegesFetcher ...................................................................... Patch Set 11: (31 comments) http://gerrit.cloudera.org:8080/#/c/12833/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12833/11//COMMIT_MSG@7 PS11, Line 7: WIP > This is no longer a WIP? Done http://gerrit.cloudera.org:8080/#/c/12833/11//COMMIT_MSG@10 PS11, Line 10: raw responses : received from Sentry > We actually store sanitized response from Sentry (SentryPrivilegesBranch), Done 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: public: > Omit this? Done. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc@182 PS9, Line 182: y() { > Drop, here and below Nope, that's left for a purpose: if dropping that here and below, compilation fails (at least on macOS): /Users/aserbin/Projects/kudu/src/kudu/master/sentry_authz_provider-test.cc:215:19: error: too many arguments to function call, expected 0, have 2; did you mean '::kudu::master::DropRole'? RETURN_NOT_OK(DropRole(sentry_client_.get(), kRoleName)); ^~~~~~~~ ::kudu::master::DropRole http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc@426 PS9, Line 426: SentryPrivilegesBranch privileges_info; : ASSERT_OK(sentry_authz_provider_->fetcher_.GetSentryPrivileges( : > I agree with limiting the test matrix, OTOH if we're limiting it, we might All right, it seem it's already taken care of (that bring many conflicts into this patch, BTW): https://github.com/apache/kudu/commit/16dc218b5a26fa18dee9e36e09e86c9c38f0d55c Also, since this test doesn't depend much on the caching/non-caching behavior of SentryPrivilegesFetcher, I'm leaving it in 'caching enabled' mode. http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.h@98 PS11, Line 98: GetSentryPrivileges > nit: it looks like the comment is removed? This method is moved SentryAuthzProvider's interface, yes. 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 Done 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 Done 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: bleScope(privilege.scope).Implies(scope)) { > I strongly feel that we should keep granted_action. Even if an action, with Yeah, just another bikeshade-able point of this changelist :) It seems I started it a few revisions ago with a request to change naming of one field in SentryPrivilegesBranch and now it continues on. After some consideration I realized that I'm not sure what 'granted action' means. How do you grant an action? I think that semantically 'action' in English is not something that can be granted. Also, I don't know of any notion of 'granting an action', and I could not find any notion of 'granted action' in user-facing Sentry's documentation at https://www.cloudera.com/documentation/enterprise/latest/topics/cm_sg_sentry_service.html What would 'granted action' mean in your terms? Maybe, there is some documentation on Sentry that clarifies on 'granting an action', but I could not find it. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc@182 PS9, Line 182: RETURN_NOT_OK(fetcher_.GetS > This reads weirdly ("if" and "do" never follow in an English sentence) and I moved this away from SentryPrivilegesBranch since I didn't want to bring too much of the implication logic into the Fetcher. It seems code conventions often contradict the rules of composition of sentences in English. In current Kudu codebase you can see names of functions like IsXxx, and that's the prevaling rules of naming such methods/functions. That's why I decided to name this function using the same pattern. I think it's better to have more or less uniform naming rules in the codebase. What do you think? http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.cc@44 PS11, Line 44: DoPrivilegesImplyAction > Maybe rename it to 'PrivilegeBranchImplies' to state that this is 'Whether OK, this seems to be second request to update the name. All right, I renamed it into IsActionAllowed() and updated the comment. http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_authz_provider.cc@182 PS11, Line 182: fetcher_.GetSentryPrivileges > nit: use SentryAuthzProvider::GetSentryPrivileges? Actually, I removed SentryAuthzProvider::GetSentryPrivileges() since it's not used anywhere. http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h File src/kudu/master/sentry_privileges_fetcher.h: http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h@92 PS11, Line 92: granted_privileges > Not your fault, but I think here name it as granted_actions is more appropr Please see the bike-shedding discussions in my replies to Adar and Andrew. BTW, what is 'granted_actions'? I could not find an idea of 'granting an action' in Sentry's documentation so far. http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h@183 PS11, Line 183: Transform > nit: Transforms. This methods has been removed. http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h@189 PS11, Line 189: Set > nit: Sets, and the below. It's no longer relevant and removed. http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h@200 PS11, Line 200: test-only method > Is this still a test-only method, I saw it has been used in Start()? Ah, indeed -- it's now used there as well. I updated the comment. http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.h@207 PS11, Line 207: Broaden the authz scope > Can you comment on how scope_depth_limit_ is used in this method and why it This is no longer relevant. 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@100 PS9, Line 100: // A representation of the Sentry privilege hierarchy branch for a single table > I left some feedback in Andrew's patch about this comment: I think this inv Yep, the newly added SentryPrivilegesBranch::Init() method addresses that. Due to the patterns we use throughout the code, we need to be able to create an empty instance of SentryPrivilegesBranch, so moving that into the constructor is not an option. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.h@174 PS9, Line 174: static const sentry::AuthorizableScopesSet& ExpectedNonEmptyFields( : sentry::SentryAuthorizableScope::Scope scope); : > nit: reword: "Send a request to fetch privileges from Sentry for the given Done http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.h@189 PS9, Line 189: // Set the maximum privilege scope depth. : Status SetAuthzScopeLevelLimit(const std::string& scope_level_limit_str); : : // A utility method: sent request to Sentry using the aggregated : // sentry_client_. > Do we need this? Shouldn't we just CHECK that we don't pass in a column aut I had some doubts some time ago, but it seems this time no more contingencies left in this regard, so I removed this method. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.h@201 PS9, Line 201: // has been updated by the test, the cache needs to be invalidated. > Why is this not some static constant? More generally, why is this configura Yep, it was required by BroadenAuthzScope, but it seems it's no longer needed. 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 ::sentry::TSentryAuthorizable& authorizable); : : c > Is this used anywhere? Not in this changelist, but once splitting information into multiple keys it will be required for the look-up process. As for now, I'll remove it, yep. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.cc@241 PS9, Line 241: // until the cached entry expires. The same is true for requests related : // to privileges granted to 'userU' on objects under 'serverS.databaseD'. : 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 > This would be more obvious as a switch statement IMO, instead of having to This is no longer relevant. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.cc@310 PS9, Line 310: mpty()); > Does sizeof(SentryActionsSet) not work? Indeed, it works. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.cc@384 PS9, Line 384: granted_action != SentryActio > Use boost::optional<SentryAuthorizableScope::Scope>. This is no longer used and removed. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.cc@406 PS9, Line 406: Status SentryPrivilegesFetcher::Start() { : ResetCache(); > Seems like it would be a programmer error indicating they're using this fun Done http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.cc@591 PS9, Line 591: default: > This doesn't seem to produce a useful Status. This could return the SentryP It's migrated into SentryPrivilegesBranch::Init(). http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_privileges_fetcher.cc@663 PS9, Line 663: TListSentryPrivilegesResponse* response) { : TListSentryPrivilegesRequest request; : request.__set_requestorUserName(service_name); : r > DCHECK instead so it's clear that this is a programmer error. Also we don't This piece is gone. http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.cc@224 PS11, Line 224: const boost::optional<SentryAuthorizableScope>& scope_limit = boost::none > It doesn't seem we are using 'scope_limit' in this patch, and I am not sure Done http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.cc@233 PS11, Line 233: futher > nit: further Done http://gerrit.cloudera.org:8080/#/c/12833/11/src/kudu/master/sentry_privileges_fetcher.cc@350 PS11, Line 350: Status SentryPrivilegesBranch::Init( > Looks like this could be void; I don't see any non-OK return. Would that al Unfortunately, that's not an option: the signatures of methods in SentryAuthzProvider assumes it's necessary to create an 'empty' instance of SentryPrivilegesBranch on stack and then use a method returning Status to populate it for future use. However, I added a constructor that accepts the same set of parameters -- it helps in some limited number of cases. -- 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: 11 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: Wed, 10 Apr 2019 16:03:03 +0000 Gerrit-HasComments: Yes
