Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12833 )
Change subject: WIP [master] introduced SentryAuthzCache ...................................................................... Patch Set 5: (24 comments) http://gerrit.cloudera.org:8080/#/c/12833/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/12833/5//COMMIT_MSG@21 PS5, Line 21: * to add tests specific to SentryAuthzCache: I'd also recommend testing the metrics (i.e. manufacturing "cache hits" and "cache misses", then verifying that they really behave that way). 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@211 PS5, Line 211: FLAGS_sentry_authz_cache_capacity_bytes = CachingEnabled() ? 512 * 1024 : 0; Doesn't the superclass do this? 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 can continue to authorize using cached information. Is that a good thing? Or should we "fail hard" and not authorize anything while Sentry is down? http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider-test.cc@503 PS5, Line 503: // Verification of basic functionality of the : // SentryAuthzProvider::IsSameScopeHierarchyBranch() method. Do you intend to extend this with more test cases? http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.h File src/kudu/master/sentry_authz_provider.h: http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.h@30 PS5, Line 30: #include "kudu/sentry/sentry_policy_service_types.h" With some minor rework you can avoid this inclusion, which would be nice because it's generated code and isn't useful for includers: 1. Move the private static functions into an anonymous namespace in the .cc file. 2. Convert SendRequestToSentry into a static function and deal with it similarly. Oh, IsSameScopeHierarchyBranch being used in tests defeats that. Also, is this impossible because of the TTLCache declaration? http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.h@102 PS5, Line 102: funciton function 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@118 PS5, Line 118: Value Nit: "A value" http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@130 PS5, Line 130: DEFINE_string(sentry_authz_cache_finest_scope, "", The explanation here doesn't match the runtime behavior. AFAICT, finest_scope only informs the scope level to use when _inserting to the cache_, not when _sending requests to Sentry_. http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@133 PS5, Line 133: about Nit: "for a" http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@134 PS5, Line 134: and the cache " : "doesn't contain any relevant entry Nit: "and the query led to a cache miss," http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@141 PS5, Line 141: meaning the cache is fetching " : "whatever fine/deep authz scope level it is asked for if it does " : "not contain any relevant entry at the time of the request." Nit: "which means the cache will fetch exactly the authz scope level it was asked for on a cache miss." http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@294 PS5, Line 294: is enforced to Nit: will http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@295 PS5, Line 295: the Nit: drop 'the' http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@304 PS5, Line 304: Similar Nit: "The same" http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@305 PS5, Line 305: privleges privileges http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@367 PS5, Line 367: if (obj) { This check seems unnecessary. http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@368 PS5, Line 368: res += obj->privileges.size() * sizeof(TSentryPrivilege); Should note that this is an approximation; we're trying to gauge the size of an std::set and we can't do that accurately without overriding its Allocator. http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@369 PS5, Line 369: for (const auto& p : obj->privileges) { Do we need to add sizeof(p) as well? http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@492 PS5, Line 492: bool SentryAuthzProvider::IsSameScopeHierarchyBranch( Note that these checks are being applied both to the cached and non-cached code branches, and previously we weren't doing checks like this in the non-cached branch (which is the only branch that existed). Why is that? http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@524 PS5, Line 524: DCHECK_EQ(a.size(), p.size()); Isn't this trivially true by reading the 10 lines just above? http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@525 PS5, Line 525: size_t idx = 0; : while (idx < a.size()) { : if (!boost::iequals(a[idx], p[idx])) { : break; : } : ++idx; : } Nit: more concise as a for loop: size_t idx; for (idx = 0; idx < a.size() && boost::iequals(a[idx], p[idx]); ++idx); Maybe the declaration of idx can be folded into the loop too? Not sure. 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 think about converting FLAGS_sentry_authz_cache_finest_scope into an enum at cache construction time? You could then switch on the enum in GetFlattenedKey instead of the nested if statement. It'd mean losing the 'runtime' aspect of the flag though. http://gerrit.cloudera.org:8080/#/c/12833/5/src/kudu/master/sentry_authz_provider.cc@603 PS5, Line 603: handle = cache_->Put(key, std::move(res_ptr), res_footprint); Did you explore having Put() usage mirror Get()? That is, rather than using finest_scope to choose one scope with which to cache, inserting up to 4 keys all mapped to the same value (shared via ref counting)? 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 if/else. Would need to rescope 'handle' and 'response' to the function itself, and to have the if/else set up a pointer into the appropriate one accordingly. But I think it'd be worth it as then the required_action/required_scope declarations could also move down. -- 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 18:34:42 +0000 Gerrit-HasComments: Yes
