From commits-return-7500-archive-asf-public=cust-asf.ponee.io@kudu.apache.org Thu Jun 6 23:57:30 2019 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id 4758118062B for ; Fri, 7 Jun 2019 01:57:30 +0200 (CEST) Received: (qmail 63223 invoked by uid 500); 6 Jun 2019 23:57:29 -0000 Mailing-List: contact commits-help@kudu.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@kudu.apache.org Delivered-To: mailing list commits@kudu.apache.org Received: (qmail 63197 invoked by uid 99); 6 Jun 2019 23:57:29 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Thu, 06 Jun 2019 23:57:29 +0000 Received: by gitbox.apache.org (ASF Mail Server at gitbox.apache.org, from userid 33) id 888FB8AC3A; Thu, 6 Jun 2019 23:57:29 +0000 (UTC) Date: Thu, 06 Jun 2019 23:57:31 +0000 To: "commits@kudu.apache.org" Subject: [kudu] 02/03: sentry: don't send requests for DATABASE/SERVER privileges MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit From: granthenke@apache.org In-Reply-To: <155986544942.30826.1769855920276561111@gitbox.apache.org> References: <155986544942.30826.1769855920276561111@gitbox.apache.org> X-Git-Host: gitbox.apache.org X-Git-Repo: kudu X-Git-Refname: refs/heads/master X-Git-Reftype: branch X-Git-Rev: 60cc7f3c83e52bc58b9a31d222a2f86e3ccc4059 X-Git-NotificationType: diff X-Git-Multimail-Version: 1.5.dev Auto-Submitted: auto-generated Message-Id: <20190606235729.888FB8AC3A@gitbox.apache.org> This is an automated email from the ASF dual-hosted git repository. granthenke pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/kudu.git commit 60cc7f3c83e52bc58b9a31d222a2f86e3ccc4059 Author: Andrew Wong AuthorDate: Wed May 29 13:42:42 2019 -0700 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 on table with authorizable scope is not permitted for 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 on table with authorizable scope is not permitted for 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 Reviewed-on: http://gerrit.cloudera.org:8080/13494 Tested-by: Kudu Jenkins Reviewed-by: Hao Hao --- src/kudu/master/sentry_authz_provider-test.cc | 33 ++++++++++++++++++ src/kudu/master/sentry_privileges_fetcher.cc | 50 ++++++++++++++++++--------- src/kudu/master/sentry_privileges_fetcher.h | 2 +- 3 files changed, 68 insertions(+), 17 deletions(-) diff --git a/src/kudu/master/sentry_authz_provider-test.cc b/src/kudu/master/sentry_authz_provider-test.cc index e7304ab..1b37927 100644 --- a/src/kudu/master/sentry_authz_provider-test.cc +++ b/src/kudu/master/sentry_authz_provider-test.cc @@ -27,6 +27,7 @@ #include #include +#include #include #include #include @@ -52,17 +53,24 @@ #include "kudu/sentry/sentry_client.h" #include "kudu/sentry/sentry_policy_service_types.h" #include "kudu/util/barrier.h" +#include "kudu/util/flag_tags.h" #include "kudu/util/hdr_histogram.h" +#include "kudu/util/logging.h" #include "kudu/util/metrics.h" #include "kudu/util/net/net_util.h" #include "kudu/util/pb_util.h" #include "kudu/util/random.h" #include "kudu/util/random_util.h" #include "kudu/util/status.h" +#include "kudu/util/stopwatch.h" #include "kudu/util/test_macros.h" #include "kudu/util/test_util.h" #include "kudu/util/ttl_cache.h" +DEFINE_int32(num_table_privileges, 100, + "Number of table privileges to use in testing"); +TAG_FLAG(num_table_privileges, hidden); + DECLARE_int32(sentry_service_recv_timeout_seconds); DECLARE_int32(sentry_service_send_timeout_seconds); DECLARE_uint32(sentry_privileges_cache_capacity_mb); @@ -339,6 +347,31 @@ constexpr const char* kColumn = "column"; } // anonymous namespace +// Benchmark to test the time it takes to evaluate privileges when requesting +// privileges for braod authorization scopes (e.g. SERVER, DATABASE). +TEST_F(SentryAuthzProviderTest, BroadAuthzScopeBenchmark) { + const char* kLongDb = "DbWithLongName"; + const char* kLongTable = "TableWithLongName"; + ASSERT_OK(CreateRoleAndAddToGroups()); + + // Create a database with a bunch tables in it. + int kNumTables = FLAGS_num_table_privileges; + for (int i = 0; i < kNumTables; i++) { + KLOG_EVERY_N_SECS(INFO, 3) << Substitute("num tables granted: $0", i); + ASSERT_OK(AlterRoleGrantPrivilege( + GetTablePrivilege(kLongDb, Substitute("$0_$1", kLongTable, i), "OWNER"))); + } + + // Time how long it takes to get the database privileges via authorizing a + // create table request. + Status s; + LOG_TIMING(INFO, "Getting database privileges") { + s = sentry_authz_provider_->AuthorizeCreateTable( + Substitute("$0.$1_$2", kLongDb, kLongTable, 0) , kTestUser, kTestUser); + } + ASSERT_TRUE(s.IsNotAuthorized()); +} + class SentryAuthzProviderFilterPrivilegesTest : public SentryAuthzProviderTest { public: SentryAuthzProviderFilterPrivilegesTest() diff --git a/src/kudu/master/sentry_privileges_fetcher.cc b/src/kudu/master/sentry_privileges_fetcher.cc index 5d54999..ff04a1f 100644 --- a/src/kudu/master/sentry_privileges_fetcher.cc +++ b/src/kudu/master/sentry_privileges_fetcher.cc @@ -208,28 +208,37 @@ GROUP_FLAG_VALIDATOR(sentry_service_rpc_addresses, namespace { -// Returns an authorizable based on the table identifier (in the format -// .) and the given scope. -Status GetAuthorizable(const string& table_ident, +// Fetching privileges from Sentry gets more expensive the broader the scope of +// the authorizable is, since the API used in a fetch returns all ancestors and +// all descendents of an authorizable in its hierarchy tree. +// +// Even if requesting privileges at a relatively broad scope, e.g. DATABASE, +// fill in the authorizable to request a narrower scope, since the broader +// privileges (i.e. the ancestors) will be returned from Sentry anyway. +void NarrowAuthzScopeForFetch(const string& db, const string& table, + TSentryAuthorizable* authorizable) { + if (authorizable->db.empty()) { + authorizable->__set_db(db); + } + if (authorizable->table.empty()) { + authorizable->__set_table(table); + } +} + +// Returns an authorizable based on the given database and table name and the +// given scope. +Status GetAuthorizable(const string& db, const string& table, SentryAuthorizableScope::Scope scope, TSentryAuthorizable* authorizable) { - Slice database; - Slice table; // We should only ever request privileges from Sentry for authorizables of // scope equal to or higher than 'TABLE'. DCHECK_NE(scope, SentryAuthorizableScope::Scope::COLUMN); switch (scope) { case SentryAuthorizableScope::Scope::TABLE: - RETURN_NOT_OK(ParseHiveTableIdentifier(table_ident, &database, &table)); - DCHECK(!table.empty()); - authorizable->__set_table(table.ToString()); + authorizable->__set_table(table); FALLTHROUGH_INTENDED; case SentryAuthorizableScope::Scope::DATABASE: - if (database.empty() && table.empty()) { - RETURN_NOT_OK(ParseHiveTableIdentifier(table_ident, &database, &table)); - } - DCHECK(!database.empty()); - authorizable->__set_db(database.ToString()); + authorizable->__set_db(db); FALLTHROUGH_INTENDED; case SentryAuthorizableScope::Scope::SERVER: authorizable->__set_server(FLAGS_server_name); @@ -585,11 +594,19 @@ Status SentryPrivilegesFetcher::ResetCache() { Status SentryPrivilegesFetcher::GetSentryPrivileges( SentryAuthorizableScope::Scope requested_scope, - const string& table_name, + const string& table_ident, const string& user, SentryPrivilegesBranch* privileges) { + Slice db_slice; + Slice table_slice; + RETURN_NOT_OK(ParseHiveTableIdentifier(table_ident, &db_slice, &table_slice)); + DCHECK(!table_slice.empty()); + DCHECK(!db_slice.empty()); + const string table = table_slice.ToString(); + const string db = db_slice.ToString(); + TSentryAuthorizable authorizable; - RETURN_NOT_OK(GetAuthorizable(table_name, requested_scope, &authorizable)); + RETURN_NOT_OK(GetAuthorizable(db, table, requested_scope, &authorizable)); if (PREDICT_FALSE(requested_scope == SentryAuthorizableScope::SERVER && !IsGTest())) { @@ -599,7 +616,7 @@ Status SentryPrivilegesFetcher::GetSentryPrivileges( // of the SERVER scope unless this method is called from test code. LOG(DFATAL) << Substitute( "requesting privileges of the SERVER scope from Sentry " - "on authorizable '$0' for user '$1'", table_name, user); + "on authorizable '$0' for user '$1'", table_ident, user); } // Not expecting requests for authorizables of the scope narrower than TABLE, @@ -673,6 +690,7 @@ Status SentryPrivilegesFetcher::GetSentryPrivileges( return Status::OK(); } + NarrowAuthzScopeForFetch(db, table, &authorizable); TRACE("Fetching privileges from Sentry"); const auto s = FetchPrivilegesFromSentry(FLAGS_kudu_service_name, user, authorizable, diff --git a/src/kudu/master/sentry_privileges_fetcher.h b/src/kudu/master/sentry_privileges_fetcher.h index 05b5f3a..a1b704c 100644 --- a/src/kudu/master/sentry_privileges_fetcher.h +++ b/src/kudu/master/sentry_privileges_fetcher.h @@ -169,7 +169,7 @@ class SentryPrivilegesFetcher { // in the cache. Status GetSentryPrivileges( sentry::SentryAuthorizableScope::Scope requested_scope, - const std::string& table_name, + const std::string& table_ident, const std::string& user, SentryPrivilegesBranch* privileges);