You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by al...@apache.org on 2019/04/26 04:58:14 UTC
[kudu] branch master updated: [authz] more tests for
SentryPrivilegesFetcher's cache
This is an automated email from the ASF dual-hosted git repository.
alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git
The following commit(s) were added to refs/heads/master by this push:
new 7ac1ed6 [authz] more tests for SentryPrivilegesFetcher's cache
7ac1ed6 is described below
commit 7ac1ed6d30a3053cb436f34b0f7b94084cb8bb31
Author: Alexey Serbin <al...@apache.org>
AuthorDate: Thu Apr 25 18:06:31 2019 -0700
[authz] more tests for SentryPrivilegesFetcher's cache
This changelist adds more coverage for the behavior of the
SentryPrivilegesFetcher's cache. In particular, newly added assertions
verify the behavior of the cache w.r.t. to cache operations exposed
via the metric counters/gauges (inserts, lookups, hits, misses, etc.).
I also updated the key pattern for the cache entries, dropping
the leading '/' symbols since it didn't serve any particular purpose.
This is a follow-up to a10d025dc13012c1a387de0607f1bd554b2038c0.
Change-Id: I7e51f214e6c26eeccf2d2556a036b9fb1e656e45
Reviewed-on: http://gerrit.cloudera.org:8080/13125
Tested-by: Kudu Jenkins
Reviewed-by: Hao Hao <ha...@cloudera.com>
---
src/kudu/master/sentry_authz_provider-test.cc | 169 +++++++++++++++++++++++++-
src/kudu/master/sentry_authz_provider.h | 1 +
src/kudu/master/sentry_privileges_fetcher.cc | 8 +-
src/kudu/master/sentry_privileges_fetcher.h | 1 +
4 files changed, 171 insertions(+), 8 deletions(-)
diff --git a/src/kudu/master/sentry_authz_provider-test.cc b/src/kudu/master/sentry_authz_provider-test.cc
index b70ea24..f06e2a4 100644
--- a/src/kudu/master/sentry_authz_provider-test.cc
+++ b/src/kudu/master/sentry_authz_provider-test.cc
@@ -36,6 +36,7 @@
#include "kudu/common/common.pb.h"
#include "kudu/common/schema.h"
#include "kudu/common/wire_protocol.h"
+#include "kudu/gutil/integral_types.h"
#include "kudu/gutil/macros.h"
#include "kudu/gutil/map-util.h"
#include "kudu/gutil/ref_counted.h"
@@ -60,6 +61,7 @@
#include "kudu/util/status.h"
#include "kudu/util/test_macros.h"
#include "kudu/util/test_util.h"
+#include "kudu/util/ttl_cache.h"
DECLARE_int32(sentry_service_recv_timeout_seconds);
DECLARE_int32(sentry_service_send_timeout_seconds);
@@ -75,6 +77,15 @@ METRIC_DECLARE_counter(sentry_client_reconnections_succeeded);
METRIC_DECLARE_counter(sentry_client_reconnections_failed);
METRIC_DECLARE_histogram(sentry_client_task_execution_time_us);
+METRIC_DECLARE_counter(sentry_privileges_cache_evictions);
+METRIC_DECLARE_counter(sentry_privileges_cache_evictions_expired);
+METRIC_DECLARE_counter(sentry_privileges_cache_hits);
+METRIC_DECLARE_counter(sentry_privileges_cache_hits_expired);
+METRIC_DECLARE_counter(sentry_privileges_cache_inserts);
+METRIC_DECLARE_counter(sentry_privileges_cache_lookups);
+METRIC_DECLARE_counter(sentry_privileges_cache_misses);
+METRIC_DECLARE_gauge_uint64(sentry_privileges_cache_memory_usage);
+
using kudu::pb_util::SecureDebugString;
using kudu::security::ColumnPrivilegePB;
using kudu::security::TablePrivilegePB;
@@ -243,6 +254,29 @@ class SentryAuthzProviderTest : public SentryTestBase {
GET_GAUGE_READINGS(GetReconnectionsFailed, reconnections_failed)
#undef GET_GAUGE_READINGS
+#define GET_GAUGE_READINGS(func_name, counter_name_suffix) \
+ int64_t func_name() { \
+ scoped_refptr<Counter> gauge(metric_entity_->FindOrCreateCounter( \
+ &METRIC_sentry_privileges_##counter_name_suffix)); \
+ CHECK(gauge); \
+ return gauge->value(); \
+ }
+ GET_GAUGE_READINGS(GetCacheEvictions, cache_evictions)
+ GET_GAUGE_READINGS(GetCacheEvictionsExpired, cache_evictions_expired)
+ GET_GAUGE_READINGS(GetCacheHitsExpired, cache_hits_expired)
+ GET_GAUGE_READINGS(GetCacheHits, cache_hits)
+ GET_GAUGE_READINGS(GetCacheInserts, cache_inserts)
+ GET_GAUGE_READINGS(GetCacheLookups, cache_lookups)
+ GET_GAUGE_READINGS(GetCacheMisses, cache_misses)
+#undef GET_GAUGE_READINGS
+
+ int64_t GetCacheUsage() {
+ scoped_refptr<AtomicGauge<uint64>> gauge(metric_entity_->FindOrCreateGauge(
+ &METRIC_sentry_privileges_cache_memory_usage, static_cast<uint64>(0)));
+ CHECK(gauge);
+ return gauge->value();
+ }
+
protected:
MetricRegistry metric_registry_;
scoped_refptr<MetricEntity> metric_entity_;
@@ -704,28 +738,65 @@ TEST_F(SentryAuthzProviderTest, CacheBehaviorScopeHierarchyAdjacentBranches) {
// ALTER TABLE, if not renaming the table itself, requires ALTER privilege
// on the table, but nothing is required on the database that contains
- // the table. METADATA requires corresponding privilege on the table, but
- // nothing is required on the database.
+ // the table.
ASSERT_EQ(0, GetTasksSuccessful());
ASSERT_OK(sentry_authz_provider_->AuthorizeAlterTable(
"db.t0", "db.t0", kTestUser));
ASSERT_EQ(1, GetTasksSuccessful());
+
+ // The cache was empty. The query was for TABLE scope privileges, so the
+ // cache was examined for both DATABASE and TABLE scope entries, and both
+ // were missing. After fetching information on privileges granted to the user
+ // on table 'db.t0', the information received from Sentry was split and put
+ // into DATABASE and TABLE scope entries.
+ ASSERT_EQ(2, GetCacheLookups());
+ ASSERT_EQ(2, GetCacheMisses());
+ ASSERT_EQ(0, GetCacheHits());
+ ASSERT_EQ(2, GetCacheInserts());
+
ASSERT_OK(sentry_authz_provider_->AuthorizeAlterTable(
"db.t1", "db.t1", kTestUser));
+ // Information on the user's privileges granted on 'db.t1' was not present
+ // in the cache: there was an RPC request sent to Sentry.
ASSERT_EQ(2, GetTasksSuccessful());
+
+ // One more cache miss: TABLE scope entry for 'db.t1' was absent.
+ ASSERT_EQ(3, GetCacheMisses());
+ // One more cache hit: DATABASE scope entry was already present.
+ ASSERT_EQ(1, GetCacheHits());
+ // Updated already existing DATABASE and inserted new TABLE entry.
+ ASSERT_EQ(4, GetCacheInserts());
+
+ // METADATA requires corresponding privilege granted on the table, but nothing
+ // is required on the database.
ASSERT_OK(sentry_authz_provider_->AuthorizeGetTableMetadata(
"db.other_table", kTestUser));
ASSERT_EQ(3, GetTasksSuccessful());
+ // One more cache miss: TABLE scope entry for 'db.other_table' was absent.
+ ASSERT_EQ(4, GetCacheMisses());
+ // One more cache hit: DATABASE scope entry was already present.
+ ASSERT_EQ(2, GetCacheHits());
+ // Updated already existing DATABASE and inserted new TABLE entry.
+ ASSERT_EQ(6, GetCacheInserts());
+
// Repeat all the requests above: not a single new RPC to Sentry should be
// sent since all authz queries must hit the cache: that's about repeating
// the same requests.
ASSERT_OK(sentry_authz_provider_->AuthorizeAlterTable(
"db.t0", "db.t0", kTestUser));
+ ASSERT_EQ(4, GetCacheHits());
ASSERT_OK(sentry_authz_provider_->AuthorizeAlterTable(
"db.t1", "db.t1", kTestUser));
+ ASSERT_EQ(6, GetCacheHits());
ASSERT_OK(sentry_authz_provider_->AuthorizeGetTableMetadata(
"db.other_table", kTestUser));
+ ASSERT_EQ(8, GetCacheHits());
+ // No new cache misses.
+ ASSERT_EQ(4, GetCacheMisses());
+ // No additional inserts, of course.
+ ASSERT_EQ(6, GetCacheInserts());
+ // No additional RPC requests to Sentry.
ASSERT_EQ(3, GetTasksSuccessful());
// All the requests below should also hit the cache since the information on
@@ -735,10 +806,18 @@ TEST_F(SentryAuthzProviderTest, CacheBehaviorScopeHierarchyAdjacentBranches) {
// database the table belongs to.
Status s = sentry_authz_provider_->AuthorizeDropTable("db.t0", kTestUser);
ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString();
+ ASSERT_EQ(10, GetCacheHits());
s = sentry_authz_provider_->AuthorizeDropTable("db.t1", kTestUser);
ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString();
+ ASSERT_EQ(12, GetCacheHits());
s = sentry_authz_provider_->AuthorizeDropTable("db.other_table", kTestUser);
ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString();
+ ASSERT_EQ(14, GetCacheHits());
+ // No new cache misses.
+ ASSERT_EQ(4, GetCacheMisses());
+ // No additional inserts, of course.
+ ASSERT_EQ(6, GetCacheInserts());
+ // No additional RPC requests to Sentry.
ASSERT_EQ(3, GetTasksSuccessful());
// A sanity check: verify no failed requests are registered.
@@ -762,6 +841,16 @@ TEST_F(SentryAuthzProviderTest, CacheBehaviorForDatabaseScope) {
"db0.t0", "db0.t0", kTestUser));
ASSERT_EQ(1, GetTasksSuccessful());
+ // The cache was empty. The query was for TABLE scope privileges, so the
+ // cache was examined for both DATABASE and TABLE scope entries, and both
+ // were missing. After fetching information on privileges granted to the user
+ // on table 'db.t0', the information received from Sentry was split and put
+ // into DATABASE and TABLE scope entries.
+ ASSERT_EQ(2, GetCacheLookups());
+ ASSERT_EQ(2, GetCacheMisses());
+ ASSERT_EQ(0, GetCacheHits());
+ ASSERT_EQ(2, GetCacheInserts());
+
// The CREATE privileges is not granted on the 'db0', so the request must
// not be authorized.
auto s = sentry_authz_provider_->AuthorizeCreateTable(
@@ -771,6 +860,13 @@ TEST_F(SentryAuthzProviderTest, CacheBehaviorForDatabaseScope) {
// have been cached already due to the prior request.
ASSERT_EQ(1, GetTasksSuccessful());
+ // One more cache lookup of the corresponding DATABASE scope key.
+ ASSERT_EQ(3, GetCacheLookups());
+ // No new cache misses.
+ ASSERT_EQ(2, GetCacheMisses());
+ // Single cache lookup turned to be a cache hit.
+ ASSERT_EQ(1, GetCacheHits());
+
// No new RPCs to Sentry should be issued: the information on privileges
// on 'db1' authorizable of the DATABASE scope should be fetched and cached
// while fetching the information privileges on 'db1.t0' authorizable of the
@@ -785,6 +881,15 @@ TEST_F(SentryAuthzProviderTest, CacheBehaviorForDatabaseScope) {
// of table "db1.t0". All other requests must hit the cache.
ASSERT_EQ(2, GetTasksSuccessful());
+ // Ten more cache lookups of the corresponding DATABASE scope key: one turned
+ // to be a miss and other nine hits after the information was fetched
+ // from Sentry and inserted into the cache.
+ ASSERT_EQ(13, GetCacheLookups());
+ ASSERT_EQ(3, GetCacheMisses());
+ ASSERT_EQ(10, GetCacheHits());
+ // One more insert: adding an entry for the DATABASE scope key for 'db1'.
+ ASSERT_EQ(3, GetCacheInserts());
+
// Same story for requests for 'db1.t0', ..., 'db1.t19'.
for (int idx = 0; idx < 20; ++idx) {
const auto table_name = Substitute("db1.t$0", idx);
@@ -793,6 +898,11 @@ TEST_F(SentryAuthzProviderTest, CacheBehaviorForDatabaseScope) {
}
ASSERT_EQ(2, GetTasksSuccessful());
+ // All twenty lookups hit the cache, no new misses.
+ ASSERT_EQ(33, GetCacheLookups());
+ ASSERT_EQ(30, GetCacheHits());
+ ASSERT_EQ(3, GetCacheMisses());
+
// A sanity check: verify no failed requests are registered.
ASSERT_EQ(0, GetTasksFailedFatal());
ASSERT_EQ(0, GetTasksFailedNonFatal());
@@ -813,6 +923,16 @@ TEST_F(SentryAuthzProviderTest, CacheBehaviorHybridLookups) {
ASSERT_OK(sentry_authz_provider_->AuthorizeDropTable("db.t", kTestUser));
ASSERT_EQ(1, GetTasksSuccessful());
+ // The cache was empty. The query was for TABLE scope privileges, so the
+ // cache was examined for both DATABASE and TABLE scope entries, and both
+ // were missing. After fetching information on privileges granted to the user
+ // on table 'db.t', the information received from Sentry was split and put
+ // into DATABASE and TABLE scope entries.
+ ASSERT_EQ(0, GetCacheHits());
+ ASSERT_EQ(2, GetCacheLookups());
+ ASSERT_EQ(2, GetCacheMisses());
+ ASSERT_EQ(2, GetCacheInserts());
+
// CREATE TABLE requires privileges only on the database itself. No privileges
// are granted on the database, so the request must not be authorized.
auto s = sentry_authz_provider_->AuthorizeCreateTable(
@@ -822,6 +942,10 @@ TEST_F(SentryAuthzProviderTest, CacheBehaviorHybridLookups) {
// granted on relevant authorizables of the DATABASE scope in corresponding
// branch should have been fetched and cached.
ASSERT_EQ(1, GetTasksSuccessful());
+ // One more lookup in the cache that turned to a cache hit: it was necessary
+ // to lookup only DATABASE scope entry in the cache.
+ ASSERT_EQ(3, GetCacheLookups());
+ ASSERT_EQ(1, GetCacheHits());
// ALTER TABLE, if renaming the table, requires privileges both on the
// database and the table. Even if ALL is granted on the table itself, there
@@ -832,6 +956,10 @@ TEST_F(SentryAuthzProviderTest, CacheBehaviorHybridLookups) {
ASSERT_TRUE(s.IsNotAuthorized()) << s.ToString();
// No extra RPCs are expected in this case.
ASSERT_EQ(1, GetTasksSuccessful());
+ // Three more lookups; three more cache hits: DATABASE and TABLE lookups
+ // are 'db.t'-related, and DATABASE lookup is 'db.t_renamed' related.
+ ASSERT_EQ(6, GetCacheLookups());
+ ASSERT_EQ(4, GetCacheHits());
// A sanity check: verify no failed requests are registered.
ASSERT_EQ(0, GetTasksFailedFatal());
@@ -841,7 +969,7 @@ TEST_F(SentryAuthzProviderTest, CacheBehaviorHybridLookups) {
// Verify that information on TABLE-scope privileges are fetched from Sentry,
// but not cached when SentryPrivilegeFetcher receives a ListPrivilegesByUser
// response for a DATABASE-scope authorizable.
-TEST_F(SentryAuthzProviderTest, CacheBehaviorNotCachingTableInfoOnDatabase) {
+TEST_F(SentryAuthzProviderTest, CacheBehaviorNotCachingTableInfo) {
ASSERT_OK(CreateRoleAndAddToGroups());
ASSERT_OK(AlterRoleGrantPrivilege(GetDatabasePrivilege("db", "CREATE")));
ASSERT_OK(AlterRoleGrantPrivilege(GetTablePrivilege("db", "t0", "ALL")));
@@ -849,10 +977,33 @@ TEST_F(SentryAuthzProviderTest, CacheBehaviorNotCachingTableInfoOnDatabase) {
ASSERT_EQ(0, GetTasksSuccessful());
// In the Sentry's authz model for Kudu, CREATE TABLE requires only privileges
- // only on the database.
+ // on the database.
ASSERT_OK(sentry_authz_provider_->AuthorizeCreateTable(
"db.table", kTestUser, kTestUser));
ASSERT_EQ(1, GetTasksSuccessful());
+ ASSERT_EQ(1, GetCacheInserts());
+ ASSERT_EQ(1, GetCacheLookups());
+ ASSERT_EQ(1, GetCacheMisses());
+
+ // Examine the entry that has just been cached: it should not contain
+ // any information on authorizables of the TABLE scope under the 'db':
+ // the cache chops off everything of the TABLE and narrower scope from
+ // Sentry response before adding corresponding entry into the cache.
+ auto* cache = sentry_authz_provider_->fetcher_.cache_.get();
+ ASSERT_NE(nullptr, cache);
+ {
+ auto handle = cache->Get(
+ Substitute("$0/$1/$2", kTestUser, FLAGS_server_name, "db"));
+ ASSERT_TRUE(handle);
+ ASSERT_EQ(2, GetCacheLookups());
+ ASSERT_EQ(1, GetCacheHits());
+
+ const auto& value = handle.value();
+ for (const auto& privilege : value.privileges()) {
+ ASSERT_NE(SentryAuthorizableScope::TABLE, privilege.scope);
+ ASSERT_NE(SentryAuthorizableScope::COLUMN, privilege.scope);
+ }
+ }
// ALTER TABLE, if not renaming the table, requires privileges only on the
// table itself.
@@ -864,11 +1015,21 @@ TEST_F(SentryAuthzProviderTest, CacheBehaviorNotCachingTableInfoOnDatabase) {
// cached deliberately: that way the cache avoids storing information on
// non-Kudu tables, if any, under an authorizable of the DATABASE scope.
ASSERT_EQ(2, GetTasksSuccessful());
+ ASSERT_EQ(2, GetCacheMisses());
+ // Two more lookups: one DATABASE scope and another TABLE scope lookup.
+ ASSERT_EQ(4, GetCacheLookups());
+ // Inserted DATABASE and TABLE entries.
+ ASSERT_EQ(3, GetCacheInserts());
// The same as above stays valid for the 'db.t1' table.
ASSERT_OK(sentry_authz_provider_->AuthorizeAlterTable(
"db.t1", "db.t1", kTestUser));
ASSERT_EQ(3, GetTasksSuccessful());
+ ASSERT_EQ(3, GetCacheMisses());
+ // Two more lookups: one DATABASE scope and another TABLE scope lookup.
+ ASSERT_EQ(6, GetCacheLookups());
+ // Updated already existing DATABASE and inserted new TABLE entry.
+ ASSERT_EQ(5, GetCacheInserts());
// A sanity check: verify no failed requests are registered.
ASSERT_EQ(0, GetTasksFailedFatal());
diff --git a/src/kudu/master/sentry_authz_provider.h b/src/kudu/master/sentry_authz_provider.h
index 9eff699..a537e00 100644
--- a/src/kudu/master/sentry_authz_provider.h
+++ b/src/kudu/master/sentry_authz_provider.h
@@ -92,6 +92,7 @@ class SentryAuthzProvider : public AuthzProvider {
FRIEND_TEST(TestAuthzHierarchy, TestAuthorizableScope);
FRIEND_TEST(SentryAuthzProviderFilterPrivilegesScopeTest, TestFilterInvalidResponses);
FRIEND_TEST(SentryAuthzProviderFilterPrivilegesScopeTest, TestFilterValidResponses);
+ FRIEND_TEST(SentryAuthzProviderTest, CacheBehaviorNotCachingTableInfo);
// Checks if the user can perform an action on the table identifier (in the format
// <database-name>.<table-name>), based on the given authorizable scope and the
diff --git a/src/kudu/master/sentry_privileges_fetcher.cc b/src/kudu/master/sentry_privileges_fetcher.cc
index bf1ad0a..a497651 100644
--- a/src/kudu/master/sentry_privileges_fetcher.cc
+++ b/src/kudu/master/sentry_privileges_fetcher.cc
@@ -290,24 +290,24 @@ vector<string> AuthzInfoKey::GenerateRawKeySequence(
DCHECK(!authorizable.server.empty());
if (!authorizable.__isset.db || authorizable.db.empty()) {
return {
- Substitute("/$0/$1", user, authorizable.server),
+ Substitute("$0/$1", user, authorizable.server),
};
}
if (!authorizable.__isset.table || authorizable.table.empty()) {
- auto k0 = Substitute("/$0/$1", user, authorizable.server);
+ auto k0 = Substitute("$0/$1", user, authorizable.server);
auto k1 = Substitute("$0/$1", k0, authorizable.db);
return { std::move(k0), std::move(k1), };
}
if (!authorizable.__isset.column || authorizable.column.empty()) {
- auto k0 = Substitute("/$0/$1", user, authorizable.server);
+ auto k0 = Substitute("$0/$1", user, authorizable.server);
auto k1 = Substitute("$0/$1", k0, authorizable.db);
auto k2 = Substitute("$0/$1", k1, authorizable.table);
return { std::move(k0), std::move(k1), std::move(k2), };
}
- auto k0 = Substitute("/$0/$1", user, authorizable.server);
+ auto k0 = Substitute("$0/$1", user, authorizable.server);
auto k1 = Substitute("$0/$1", k0, authorizable.db);
auto k2 = Substitute("$0/$1", k1, authorizable.table);
auto k3 = Substitute("$0/$1", k2, authorizable.column);
diff --git a/src/kudu/master/sentry_privileges_fetcher.h b/src/kudu/master/sentry_privileges_fetcher.h
index 71a7d75..bb17edd 100644
--- a/src/kudu/master/sentry_privileges_fetcher.h
+++ b/src/kudu/master/sentry_privileges_fetcher.h
@@ -174,6 +174,7 @@ class SentryPrivilegesFetcher {
friend class SentryAuthzProviderTest;
friend class SentryPrivilegesBranch;
FRIEND_TEST(SentryPrivilegesFetcherStaticTest, TestPrivilegesWellFormed);
+ FRIEND_TEST(SentryAuthzProviderTest, CacheBehaviorNotCachingTableInfo);
// Utility function to determine whether the given privilege is a well-formed
// possibly Kudu-related privilege describing a descendent or ancestor of the