Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13069 )
Change subject: [authz] updated SentryAuthzProvider caching strategy ...................................................................... Patch Set 2: (3 comments) I need to head out now, but jotted down some notes. http://gerrit.cloudera.org:8080/#/c/13069/2/src/kudu/master/sentry_privileges_fetcher.cc File src/kudu/master/sentry_privileges_fetcher.cc: http://gerrit.cloudera.org:8080/#/c/13069/2/src/kudu/master/sentry_privileges_fetcher.cc@541 PS2, Line 541: auto handle = cache_->Get(e); : if (!handle) { : continue; : } : handles.emplace_back(std::move(handle)); We're now doing Get() in a loop. Does this mean that we might go to Sentry multiple times? http://gerrit.cloudera.org:8080/#/c/13069/2/src/kudu/master/sentry_privileges_fetcher.cc@548 PS2, Line 548: if (!handles.empty()) { : SentryPrivilegesBranch result; : for (const auto& e : handles) { : DCHECK(e); : result.Merge(e.value()); : } : *privileges = std::move(result); : return Status::OK(); : } > I suspect there might be a bug here. Imagine that information on privilege Yeah this is a bug: - the user has privileges on "db" and we have METADATA ON DATABASE - our privileges for "db.table" expired, but the user _should_ have SELECT ON TABLE - we go through these checks and end up with only METADATA ON DATABASE when we should have really gone to Sentry This means that if _any_ of the Get()s don't return a handle, we need to go to Sentry. http://gerrit.cloudera.org:8080/#/c/13069/2/src/kudu/master/sentry_privileges_fetcher.cc@610 PS2, Line 610: requested_scope >= SentryAuthorizableScope::TABLE This seems to be tied to the implementation/order of the enums instead of the logical scopes themselves. `!SentryAuthorizableScope(requested_scop).Implies(DATABASE)` would be clearer. -- To view, visit http://gerrit.cloudera.org:8080/13069 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id96181345e357a104e28314d8d8d88633dcf9608 Gerrit-Change-Number: 13069 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin <aser...@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-Comment-Date: Sat, 20 Apr 2019 01:03:26 +0000 Gerrit-HasComments: Yes
