Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12833 )
Change subject: [master] introduce SentryPrivilegesFetcher ...................................................................... Patch Set 12: Code-Review+1 (6 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@182 PS9, Line 182: rivileges_cach > Nope, that's left for a purpose: if dropping that here and below, compilati Ah, missed the fact that it'd be recursive naming. http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider-test.cc@426 PS9, Line 426: for (const auto& action : SelectRandomSubset<SentryActionsSet, SentryAction::Action, Random>( : kAllActions, 0, &prng)) { : > All right, it seem it's already taken care of (that bring many conflicts in Sorry for the conflicts. Thanks for being accommodating. 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: s = privileges_branch.privileges(); > Yeah, just another bikeshade-able point of this changelist :) It seems I s I think I understand where you're coming from and I agree that there is no contention that privileges are units that can be granted from a Sentry POV. That said, I think if we don't make the conscious decision to clarify different terms, it might be confusing. In Sentry, you can have "a privilege on a scope" (per those docs), but what this really means is "a privilege to perform an action at a given scope". I don't think those docs do a great job making this distinction because an end-user of Sentry doesn't really need to make that distinction -- the "privilege" _is_ the action, and you can grant a privilege on a scope. When it comes down to these code structs, we want our struct to encapsulate (in terms from the docs) a privilege that has been granted on a scope. A simple way to do this would to have a GrantedPrivilege be a struct<privilege, scope>. But I think this is confusing because this means that "privilege" would refer to both the struct itself and a member internal to the struct. And so a clear way to distinguish these two "privileges" is to call the member "privilege" "action" instead, so a Privilege is a struct<action, scope>. This corresponds roughly to what's in TSentryPrivilege in sentry/sentry_policy_service.thrift. In that way, we care about keeping track of the Privileges (ie. the struct<action, scope>s), and a user can be granted many of these Privileges. And so when I think of "granting a privilege", I think of "granting a struct<action, scope>", and by extension, a "granted action" in the context of these Privileges, is the actions on which the privilege has been granted. But if you're caught up on the "granted" aspect of it, I'd also be fine calling this "actions". http://gerrit.cloudera.org:8080/#/c/12833/9/src/kudu/master/sentry_authz_provider.cc@182 PS9, Line 182: > I moved this away from SentryPrivilegesBranch since I didn't want to bring SGTM http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12833/12/src/kudu/master/sentry_authz_provider.cc@50 PS12, Line 50: // action ('required_action) in the specified scope ('required_scope') nit: missing ' 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: std::string table_name; > Yep, the newly added SentryPrivilegesBranch::Init() method addresses that. Thanks for doing that Alexey. -- 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: 12 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Wed, 10 Apr 2019 18:04:51 +0000 Gerrit-HasComments: Yes
