Hello Alexey Serbin, Kudu Jenkins, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13494
to look at the new patch set (#7).
Change subject: sentry: don't send requests for DATABASE/SERVER privileges
......................................................................
sentry: don't send requests for DATABASE/SERVER privileges
The API used to request privileges from Sentry will return more
privileges than required for a given request. For example, if requesting
privileges for a specific database, the API will return privileges for
all ancestors and all descendents of that database, i.e. privileges for
the server, the database, all tables in the database, and all columns
therein.
To remediate this, this patch narrows the scope of requests sent to
Sentry to the table-level. Table-level seems fitting since every request
for privileges Kudu makes to Sentry thus far is naturally scoped to a
table anyway.
Note: this patch doesn't touch any of the caching logic; it only tweaks
the request sent to Sentry.
I wrote a small benchmark[1] to gauge the effects of this patch. The
time spent checking database-level privileges with 20K privilege entries
in the database was 1.989 without this patch, and 0.491s with this
patch.
W0602 07:44:03.110574 13909 sentry_client.cc:188] Time spent alter Sentry role
grant privileges: real 3.861s user 0.000s sys 0.000s
I0602 07:44:03.110697 13909 sentry_authz_provider-test.cc:739] num tables
granted: 19998
W0602 07:44:06.829799 13909 sentry_client.cc:188] Time spent alter Sentry role
grant privileges: real 3.719s user 0.000s sys 0.000s
I0602 07:44:06.829936 13909 sentry_authz_provider-test.cc:739] num tables
granted: 19999
W0602 07:44:10.521934 13909 sentry_client.cc:188] Time spent alter Sentry role
grant privileges: real 3.692s user 0.000s sys 0.000s
W0602 07:44:12.375082 14301 sasl_client_transport.cc:160] Received large Thrift
SASL frame: 2.33M
W0602 07:44:12.438380 14301 sentry_client.cc:170] Time spent list Sentry
privileges by user: real 1.892s user 0.054s sys 0.009s
W0602 07:44:12.509138 13909 sentry_authz_provider.cc:275] Action <CREATE> on
table <dbwithalongname.table> with authorizable scope <DATABASE> is not
permitted for user <test-user>
I0602 07:44:12.510848 13909 sentry_authz_provider-test.cc:744] Time spent
Getting database privileges: real 1.989s user 0.070s sys 0.002s
W0602 07:44:13.002019 13909 sentry_authz_provider.cc:275] Action <CREATE> on
table <dbwithalongname.table> with authorizable scope <DATABASE> is not
permitted for user <test-user>
I0602 07:44:13.002111 13909 sentry_authz_provider-test.cc:750] Time spent
Getting database privileges: real 0.491s user 0.000s sys 0.000s
While the test numbers themselves may not be representative of a real
Sentry deployment (since the test uses a Sentry minicluster instead of a
real cluster) the numbers still point to reasonable improvement.
A version of this benchmark is included in this patch. I opted to not
gate the behavior behind a gflag, so rather than running successive
privileges with different behavior, the included test only checks
privileges once.
[1] https://gist.github.com/andrwng/88f3a760441b4b37b3ff011e656683ab
Change-Id: Ic0025e3bacc8449dfffe99a1fc062a9e6787eb78
---
M src/kudu/master/sentry_authz_provider-test.cc
M src/kudu/master/sentry_privileges_fetcher.cc
M src/kudu/master/sentry_privileges_fetcher.h
3 files changed, 68 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/13494/7
--
To view, visit http://gerrit.cloudera.org:8080/13494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic0025e3bacc8449dfffe99a1fc062a9e6787eb78
Gerrit-Change-Number: 13494
Gerrit-PatchSet: 7
Gerrit-Owner: Andrew Wong <aw...@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)
