From d76e9b390082828e9b9047a71c3eb96faca8be0d Mon Sep 17 00:00:00 2001 From: Henry Yang <4411287+HenryYYang@users.noreply.github.com> Date: Mon, 30 Sep 2019 13:46:48 -0700 Subject: [PATCH 1/3] Enforce hashtagging for Redis clusters (#8418) Signed-off-by: Henry Yang --- docs/root/intro/version_history.rst | 1 + .../clusters/redis/redis_cluster_lb.cc | 6 ++--- .../clusters/redis/redis_cluster_lb.h | 14 ++++++++++- .../network/redis_proxy/conn_pool_impl.cc | 6 ++--- .../clusters/redis/redis_cluster_lb_test.cc | 23 +++++++++++++++++++ 5 files changed, 43 insertions(+), 7 deletions(-) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 88005526b9cb..3a7afd519c07 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -49,6 +49,7 @@ Version history * redis: added :ref:`enable_command_stats ` to enable :ref:`per command statistics ` for upstream clusters. * redis: added :ref:`read_policy ` to allow reading from redis replicas for Redis Cluster deployments. * redis: fix a bug where the redis health checker ignored the upstream auth password. +* redis: enable_hashtaging is always enabled when the upstream uses open source Redis cluster protocol. * regex: introduce new :ref:`RegexMatcher ` type that provides a safe regex implementation for untrusted user input. This type is now used in all configuration that processes user provided input. See :ref:`deprecated configuration details diff --git a/source/extensions/clusters/redis/redis_cluster_lb.cc b/source/extensions/clusters/redis/redis_cluster_lb.cc index b4f3b1d06a2c..442602e8529d 100644 --- a/source/extensions/clusters/redis/redis_cluster_lb.cc +++ b/source/extensions/clusters/redis/redis_cluster_lb.cc @@ -176,11 +176,11 @@ bool isReadRequest(const NetworkFilters::Common::Redis::RespValue& request) { } // namespace RedisLoadBalancerContextImpl::RedisLoadBalancerContextImpl( - const std::string& key, bool enabled_hashtagging, bool use_crc16, + const std::string& key, bool enabled_hashtagging, bool is_redis_cluster, const NetworkFilters::Common::Redis::RespValue& request, NetworkFilters::Common::Redis::Client::ReadPolicy read_policy) - : hash_key_(use_crc16 ? Crc16::crc16(hashtag(key, enabled_hashtagging)) - : MurmurHash::murmurHash2_64(hashtag(key, enabled_hashtagging))), + : hash_key_(is_redis_cluster ? Crc16::crc16(hashtag(key, true)) + : MurmurHash::murmurHash2_64(hashtag(key, enabled_hashtagging))), is_read_(isReadRequest(request)), read_policy_(read_policy) {} // Inspired by the redis-cluster hashtagging algorithm diff --git a/source/extensions/clusters/redis/redis_cluster_lb.h b/source/extensions/clusters/redis/redis_cluster_lb.h index 3ad30cf76074..1ee23c11ed18 100644 --- a/source/extensions/clusters/redis/redis_cluster_lb.h +++ b/source/extensions/clusters/redis/redis_cluster_lb.h @@ -67,7 +67,19 @@ class RedisLoadBalancerContext { class RedisLoadBalancerContextImpl : public RedisLoadBalancerContext, public Upstream::LoadBalancerContextBase { public: - RedisLoadBalancerContextImpl(const std::string& key, bool enabled_hashtagging, bool use_crc16, + /** + * The load balancer context for Redis requests. Note that is_redis_cluster implies using Redis + * cluster which require us to always enable hashtagging. + * @param key specify the key for the Redis request. + * @param enabled_hashtagging specify whether to enable hashtagging, this will always be true if + * is_redis_cluster is true. + * @param is_redis_cluster specify whether this is a request for redis cluster, if true the key + * will be hashed using crc16. + * @param request specify the Redis request. + * @param read_policy specify the read policy. + */ + RedisLoadBalancerContextImpl(const std::string& key, bool enabled_hashtagging, + bool is_redis_cluster, const NetworkFilters::Common::Redis::RespValue& request, NetworkFilters::Common::Redis::Client::ReadPolicy read_policy = NetworkFilters::Common::Redis::Client::ReadPolicy::Master); diff --git a/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc b/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc index 7304ef902325..24c8caf7a2ab 100644 --- a/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc +++ b/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc @@ -208,9 +208,9 @@ InstanceImpl::ThreadLocalPool::makeRequest(const std::string& key, return nullptr; } - const bool use_crc16 = is_redis_cluster_; - Clusters::Redis::RedisLoadBalancerContextImpl lb_context( - key, parent_.config_.enableHashtagging(), use_crc16, request, parent_.config_.readPolicy()); + Clusters::Redis::RedisLoadBalancerContextImpl lb_context(key, parent_.config_.enableHashtagging(), + is_redis_cluster_, request, + parent_.config_.readPolicy()); Upstream::HostConstSharedPtr host = cluster_->loadBalancer().chooseHost(&lb_context); if (!host) { return nullptr; diff --git a/test/extensions/clusters/redis/redis_cluster_lb_test.cc b/test/extensions/clusters/redis/redis_cluster_lb_test.cc index 5e0ae78de9b2..8e0d8214966b 100644 --- a/test/extensions/clusters/redis/redis_cluster_lb_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_lb_test.cc @@ -423,6 +423,29 @@ TEST_F(RedisLoadBalancerContextImplTest, UnsupportedCommand) { EXPECT_EQ(NetworkFilters::Common::Redis::Client::ReadPolicy::Master, context3.readPolicy()); } +TEST_F(RedisLoadBalancerContextImplTest, EnforceHashTag) { + std::vector set_foo(3); + set_foo[0].type(NetworkFilters::Common::Redis::RespType::BulkString); + set_foo[0].asString() = "set"; + set_foo[1].type(NetworkFilters::Common::Redis::RespType::BulkString); + set_foo[1].asString() = "{foo}bar"; + set_foo[2].type(NetworkFilters::Common::Redis::RespType::BulkString); + set_foo[2].asString() = "bar"; + + NetworkFilters::Common::Redis::RespValue set_request; + set_request.type(NetworkFilters::Common::Redis::RespType::Array); + set_request.asArray().swap(set_foo); + + // Enable_hash tagging should be override when is_redis_cluster is true. This is treated like + // "foo" + RedisLoadBalancerContextImpl context2("{foo}bar", false, true, set_request, + NetworkFilters::Common::Redis::Client::ReadPolicy::Master); + + EXPECT_EQ(absl::optional(44950), context2.computeHashKey()); + EXPECT_EQ(false, context2.isReadCommand()); + EXPECT_EQ(NetworkFilters::Common::Redis::Client::ReadPolicy::Master, context2.readPolicy()); +} + } // namespace Redis } // namespace Clusters } // namespace Extensions From b72f38a1770b9d18afba6d65ac229e308ada4341 Mon Sep 17 00:00:00 2001 From: Yi Tang Date: Tue, 1 Oct 2019 14:55:44 +0800 Subject: [PATCH 2/3] tools: proto sync shebang line for portability (#8430) Description: use /usr/bin/env python3 for portability in proto_sync.py. Risk Level: low Signed-off-by: Yi Tang --- tools/proto_sync.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/proto_sync.py b/tools/proto_sync.py index 4911ac767215..809126eff520 100755 --- a/tools/proto_sync.py +++ b/tools/proto_sync.py @@ -1,4 +1,4 @@ -#!/usr/bin/python3 +#!/usr/bin/env python3 # Diff or copy protoxform artifacts from Bazel cache back to the source tree. From 27c77491d676d98bf6ab7eb539ad6662034d8ae0 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Tue, 1 Oct 2019 08:18:15 -0400 Subject: [PATCH 3/3] stats: Recent lookups implementation (#8376) * recent-lookups class impl & tests. Signed-off-by: Joshua Marantz --- source/common/stats/BUILD | 9 ++ source/common/stats/recent_lookups.cc | 60 ++++++++++ source/common/stats/recent_lookups.h | 77 +++++++++++++ test/common/stats/BUILD | 24 ++++ .../common/stats/recent_lookups_speed_test.cc | 82 +++++++++++++ test/common/stats/recent_lookups_test.cc | 108 ++++++++++++++++++ 6 files changed, 360 insertions(+) create mode 100644 source/common/stats/recent_lookups.cc create mode 100644 source/common/stats/recent_lookups.h create mode 100644 test/common/stats/recent_lookups_speed_test.cc create mode 100644 test/common/stats/recent_lookups_test.cc diff --git a/source/common/stats/BUILD b/source/common/stats/BUILD index ed5efa052b47..224c006fc395 100644 --- a/source/common/stats/BUILD +++ b/source/common/stats/BUILD @@ -87,6 +87,15 @@ envoy_cc_library( ], ) +envoy_cc_library( + name = "recent_lookups_lib", + srcs = ["recent_lookups.cc"], + hdrs = ["recent_lookups.h"], + deps = [ + "//source/common/common:assert_lib", + ], +) + envoy_cc_library( name = "store_impl_lib", hdrs = ["store_impl.h"], diff --git a/source/common/stats/recent_lookups.cc b/source/common/stats/recent_lookups.cc new file mode 100644 index 000000000000..994441cccb02 --- /dev/null +++ b/source/common/stats/recent_lookups.cc @@ -0,0 +1,60 @@ +#include "common/stats/recent_lookups.h" + +#include "common/common/assert.h" + +namespace Envoy { +namespace Stats { + +void RecentLookups::lookup(absl::string_view str) { + ++total_; + if (capacity_ == 0) { + return; + } + auto map_iter = map_.find(str); + if (map_iter != map_.end()) { + // The item is already in the list. Bump its reference-count and move it to + // the front of the list. + auto list_iter = map_iter->second; + ++list_iter->count_; + if (list_iter != list_.begin()) { + list_.splice(list_.begin(), list_, list_iter); + } + } else { + ASSERT(list_.size() <= capacity_); + // Evict oldest item if needed. + if (list_.size() >= capacity_) { + evictOne(); + } + + // The string storage is in the list entry. + list_.push_front(ItemCount{std::string(str), 1}); + auto list_iter = list_.begin(); + map_[list_iter->item_] = list_iter; + } + ASSERT(list_.size() == map_.size()); +} + +void RecentLookups::forEach(const IterFn& fn) const { + for (const ItemCount& item_count : list_) { + fn(item_count.item_, item_count.count_); + } +} + +void RecentLookups::setCapacity(uint64_t capacity) { + capacity_ = capacity; + while (capacity_ < list_.size()) { + evictOne(); + } +} + +void RecentLookups::evictOne() { + ASSERT(!list_.empty()); + ASSERT(!map_.empty()); + const ItemCount& item_count = list_.back(); + int erased = map_.erase(item_count.item_); + ASSERT(erased == 1); + list_.pop_back(); +} + +} // namespace Stats +} // namespace Envoy diff --git a/source/common/stats/recent_lookups.h b/source/common/stats/recent_lookups.h new file mode 100644 index 000000000000..3e633bca8dbd --- /dev/null +++ b/source/common/stats/recent_lookups.h @@ -0,0 +1,77 @@ +#pragma once + +#include +#include +#include + +#include "absl/container/flat_hash_map.h" +#include "absl/strings/string_view.h" + +namespace Envoy { +namespace Stats { + +// Remembers the last 'Capacity' items passed to lookup(). +class RecentLookups { +public: + /** + * Records a lookup of a string. Only the last 'Capacity' lookups are remembered. + * + * @param str the item being looked up. + */ + void lookup(absl::string_view str); + + using IterFn = std::function; + + /** + * Calls fn(item, count) for each of the remembered lookups. + * + * @param fn The function to call for every recently looked up item. + */ + void forEach(const IterFn& fn) const; + + /** + * @return the total number of lookups since tracking began. + */ + uint64_t total() const { return total_; } + + /** + * Clears out all contents. + */ + void clear() { + total_ = 0; + map_.clear(); + list_.clear(); + } + + /** + * Controls the maximum number of recent lookups to remember. If set to 0, + * then only lookup counts is tracked. + * @param capacity The number of lookups to remember. + */ + void setCapacity(uint64_t capacity); + + /** + * @return The configured capacity. + */ + uint64_t capacity() const { return capacity_; } + +private: + void evictOne(); + + struct ItemCount { + std::string item_; + uint64_t count_; + }; + using List = std::list; + List list_; + + // TODO(jmarantz): we could make this more compact by making this a set of + // list-iterators with heterogeneous hash/compare functors. + using Map = absl::flat_hash_map; + Map map_; + uint64_t total_{0}; + uint64_t capacity_{0}; +}; + +} // namespace Stats +} // namespace Envoy diff --git a/test/common/stats/BUILD b/test/common/stats/BUILD index b4b528194103..65ed7bf5fd45 100644 --- a/test/common/stats/BUILD +++ b/test/common/stats/BUILD @@ -39,6 +39,30 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "recent_lookups_test", + srcs = ["recent_lookups_test.cc"], + deps = [ + "//source/common/common:utility_lib", + "//source/common/stats:recent_lookups_lib", + "//test/test_common:logging_lib", + "//test/test_common:simulated_time_system_lib", + ], +) + +envoy_cc_test_binary( + name = "recent_lookups_speed_test", + srcs = ["recent_lookups_speed_test.cc"], + external_deps = [ + "benchmark", + ], + deps = [ + "//source/common/common:utility_lib", + "//source/common/runtime:runtime_lib", + "//source/common/stats:recent_lookups_lib", + ], +) + envoy_cc_test( name = "stat_merger_test", srcs = ["stat_merger_test.cc"], diff --git a/test/common/stats/recent_lookups_speed_test.cc b/test/common/stats/recent_lookups_speed_test.cc new file mode 100644 index 000000000000..15224a7c569d --- /dev/null +++ b/test/common/stats/recent_lookups_speed_test.cc @@ -0,0 +1,82 @@ +// Note: this should be run with --compilation_mode=opt, and would benefit from a +// quiescent system with disabled cstate power management. +// +// NOLINT(namespace-envoy) +// +// Running bazel-bin/test/common/stats/recent_lookups_speed_test +// Run on (12 X 4500 MHz CPU s) +// CPU Caches: +// L1 Data 32K (x6) +// L1 Instruction 32K (x6) +// L2 Unified 1024K (x6) +// L3 Unified 8448K (x1) +// Load Average: 1.32, 7.40, 10.21 +// ***WARNING*** CPU scaling is enabled, the benchmark real time measurements may be noisy and will +// incur extra overhead. +// ----------------------------------------------------------------- +// Benchmark Time CPU Iterations +// ----------------------------------------------------------------- +// BM_LookupsMixed 87068 ns 87068 ns 6955 +// BM_LookupsNoEvictions 45662 ns 45662 ns 15329 +// BM_LookupsAllEvictions 83015 ns 83015 ns 8435 + +#include "common/runtime/runtime_impl.h" +#include "common/stats/recent_lookups.h" + +#include "absl/strings/str_cat.h" +#include "benchmark/benchmark.h" + +class RecentLookupsSpeedTest { +public: + RecentLookupsSpeedTest(uint64_t lookup_variants, uint64_t capacity) { + recent_lookups_.setCapacity(capacity); + Envoy::Runtime::RandomGeneratorImpl random; + lookups_.reserve(lookup_variants); + for (size_t i = 0; i < lookup_variants; ++i) { + lookups_.push_back(absl::StrCat("lookup #", random.random())); + } + } + + void test(benchmark::State& state) { + for (auto _ : state) { + Envoy::Runtime::RandomGeneratorImpl random; + for (uint64_t i = 0; i < lookups_.size(); ++i) { + recent_lookups_.lookup(lookups_[random.random() % lookups_.size()]); + } + } + } + +private: + std::vector lookups_; + Envoy::Stats::RecentLookups recent_lookups_; +}; + +static void BM_LookupsMixed(benchmark::State& state) { + RecentLookupsSpeedTest speed_test(1000, 500); + speed_test.test(state); +} +BENCHMARK(BM_LookupsMixed); + +static void BM_LookupsNoEvictions(benchmark::State& state) { + RecentLookupsSpeedTest speed_test(1000, 1000); + speed_test.test(state); +} +BENCHMARK(BM_LookupsNoEvictions); + +static void BM_LookupsAllEvictions(benchmark::State& state) { + RecentLookupsSpeedTest speed_test(1000, 10); + speed_test.test(state); +} +BENCHMARK(BM_LookupsAllEvictions); + +int main(int argc, char** argv) { + Envoy::Thread::MutexBasicLockable lock; + Envoy::Logger::Context logger_context(spdlog::level::warn, + Envoy::Logger::Logger::DEFAULT_LOG_FORMAT, lock); + benchmark::Initialize(&argc, argv); + + if (benchmark::ReportUnrecognizedArguments(argc, argv)) { + return 1; + } + benchmark::RunSpecifiedBenchmarks(); +} diff --git a/test/common/stats/recent_lookups_test.cc b/test/common/stats/recent_lookups_test.cc new file mode 100644 index 000000000000..e8be8f9d90be --- /dev/null +++ b/test/common/stats/recent_lookups_test.cc @@ -0,0 +1,108 @@ +#include +#include + +#include "common/common/utility.h" +#include "common/stats/recent_lookups.h" + +#include "test/test_common/logging.h" + +#include "absl/strings/str_cat.h" +#include "gtest/gtest.h" + +namespace Envoy { +namespace Stats { +namespace { + +class RecentLookupsTest : public testing::Test { +protected: + std::string joinLookups() { + using ItemCount = std::pair; + std::vector items; + recent_lookups_.forEach([&items](absl::string_view item, uint64_t count) { + items.emplace_back(ItemCount(std::string(item), count)); + }); + std::sort(items.begin(), items.end(), [](const ItemCount& a, const ItemCount& b) -> bool { + if (a.second == b.second) { + return a.first < b.first; + } + return a.second < b.second; + }); + std::vector accum; + accum.reserve(items.size()); + for (const auto& item : items) { + accum.push_back(absl::StrCat(item.second, ": ", item.first)); + } + return StringUtil::join(accum, " "); + } + + RecentLookups recent_lookups_; +}; + +TEST_F(RecentLookupsTest, Empty) { EXPECT_EQ("", joinLookups()); } + +TEST_F(RecentLookupsTest, One) { + recent_lookups_.lookup("Hello"); + EXPECT_EQ("", joinLookups()); + recent_lookups_.setCapacity(10); + EXPECT_EQ(1, recent_lookups_.total()); + recent_lookups_.lookup("Hello"); + EXPECT_EQ(2, recent_lookups_.total()); + EXPECT_EQ("1: Hello", joinLookups()); + + recent_lookups_.clear(); + EXPECT_EQ("", joinLookups()); + EXPECT_EQ(0, recent_lookups_.total()); + recent_lookups_.lookup("Hello"); + EXPECT_EQ(1, recent_lookups_.total()); + EXPECT_EQ("1: Hello", joinLookups()); + recent_lookups_.setCapacity(0); + EXPECT_EQ("", joinLookups()); + EXPECT_EQ(1, recent_lookups_.total()); +} + +TEST_F(RecentLookupsTest, DropOne) { + recent_lookups_.setCapacity(10); + for (int i = 0; i < 11; ++i) { + recent_lookups_.lookup(absl::StrCat("lookup", i)); + } + EXPECT_EQ("1: lookup1 " + "1: lookup10 " + "1: lookup2 " + "1: lookup3 " + "1: lookup4 " + "1: lookup5 " + "1: lookup6 " + "1: lookup7 " + "1: lookup8 " + "1: lookup9", + joinLookups()); + recent_lookups_.clear(); + EXPECT_EQ("", joinLookups()); +} + +TEST_F(RecentLookupsTest, RepeatDrop) { + recent_lookups_.setCapacity(10); + recent_lookups_.lookup("drop_early"); + for (int i = 0; i < 11; ++i) { + recent_lookups_.lookup(absl::StrCat("lookup", i)); + recent_lookups_.lookup(absl::StrCat("lookup", i)); + } + recent_lookups_.lookup("add_late"); + EXPECT_EQ("1: add_late " + "2: lookup10 " + "2: lookup2 " + "2: lookup3 " + "2: lookup4 " + "2: lookup5 " + "2: lookup6 " + "2: lookup7 " + "2: lookup8 " + "2: lookup9", + joinLookups()); + recent_lookups_.clear(); + EXPECT_EQ("", joinLookups()); +} + +} // namespace +} // namespace Stats +} // namespace Envoy