Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12833 )
Change subject: WIP [master] introduced SentryAuthzCache ...................................................................... Patch Set 5: (7 comments) http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider-test.cc File src/kudu/master/sentry_authz_provider-test.cc: http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider-test.cc@341 PS5, Line 341: if (CachingEnabled()) { : EXPECT_TRUE(s.IsNotAuthorized()) << s.ToString(); : } else { : EXPECT_TRUE(s.IsNetworkError()) << s.ToString(); : } > These changes reflect interesting runtime behavior: when Sentry is down, we I think if a user wanted that behavior, they could disable the cache, no? Seems reasonable to expect that caching circumvents access to Sentry. http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc File src/kudu/master/sentry_authz_provider.cc: http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@276 PS5, Line 276: An nit: A Also probably worth adding a small example of the AuthzInfoKey string format. http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@280 PS5, Line 280: subject nit: Maybe name it 'user' instead? Or add a comment that this is used as a prefix to make the same authorizables unique for different 'subjects'? I wasn't sure what this was until L583. http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@307 PS5, Line 307: ? 1 : scope == kAuthzScopeDatabase : ? 2 : scope == kAuthzScopeTable : ? 3 : scope == kAuthzScopeColumn : ? 4 : key_sequence_.size(); : : if (level <= key_sequence_.size()) { : return key_sequence_[level - 1]; : } nit: May be slightly more readable starting at 0, eg: const size_t level = scope = kAuthzScopeDatabase ? 0 : ... key_sequence_.size() - 1; if (level < key_sequence_.size()) { return key_sequence_[level]; } ? http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@464 PS5, Line 464: SentryAuthzProvider::IsSameScopeHierarchyBranch > I think we should try to avoid checking for each privilege in the Sentry re It's probably worth sanitizing anything we get from an external service, even if it is "guaranteed," no? http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@601 PS5, Line 601: FLAGS_sentry_authz_cache_finest_scope); > To avoid the implicit char* -> std::string conversion here, what do you thi You could also use SentryAUthorizableScope::FromString() and that'll be case insensitive. Also, if you do want to keep the runtime behavior, you could do that translation here, just to use a switch instead of that ternary loop. http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@608 PS5, Line 608: authorized = HasPrivilege(handle->value(), authorizable, : required_action, required_scope, : require_grant_option); > Nit: might be cleaner to consolidate the two HasPrivilege calls outside the I don't feel too strongly about it, but I agree and another benefit would be that it might facilitate implementing something like a GetSentryPrivileges() that either checks the cache or goes directly to sentry, after which we'd do the `if(HasPrivilege())` check. Probably doesn't need to be in this patch, but I think that kind of function could be reused to generate authz tokens. -- 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: 5 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, 27 Mar 2019 21:13:28 +0000 Gerrit-HasComments: Yes
