kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From granthe...@apache.org
Subject [kudu] 02/03: sentry: don't send requests for DATABASE/SERVER privileges
Date Thu, 06 Jun 2019 23:57:31 GMT
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 <awong@apache.org>
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 <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
    Reviewed-on: http://gerrit.cloudera.org:8080/13494
    Tested-by: Kudu Jenkins
    Reviewed-by: Hao Hao <hao.hao@cloudera.com>
---
 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 <unordered_set>
 #include <vector>
 
+#include <gflags/gflags.h>
 #include <gflags/gflags_declare.h>
 #include <glog/logging.h>
 #include <google/protobuf/stubs/port.h>
@@ -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
-// <database-name>.<table-name>) 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);
 


Mime
View raw message