From 71c6c3a0c1528f3662ff09fd08b31dcce409c2c8 Mon Sep 17 00:00:00 2001 From: Nicolas Flacco Date: Thu, 29 Aug 2019 12:16:24 -0700 Subject: [PATCH 1/4] Redis mirroring should be >=, not > as otherwise it will not read runtime settings unless default is > 0 Signed-off-by: Nicolas Flacco --- source/extensions/filters/network/redis_proxy/router_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/filters/network/redis_proxy/router_impl.cc b/source/extensions/filters/network/redis_proxy/router_impl.cc index 6cfa59022041..c18f0e46c277 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.cc +++ b/source/extensions/filters/network/redis_proxy/router_impl.cc @@ -23,7 +23,7 @@ bool MirrorPolicyImpl::shouldMirror(const std::string& command) const { return false; } - if (default_value_.numerator() > 0) { + if (default_value_.numerator() >= 0) { return runtime_.snapshot().featureEnabled(runtime_key_, default_value_); } From 2f7e5ef1be99b26eff94136bd1fd43d0e70aa41f Mon Sep 17 00:00:00 2001 From: Nicolas Flacco Date: Tue, 10 Sep 2019 17:11:05 -0700 Subject: [PATCH 2/4] Check if default value for mirroring is set, and test zero case does not mirror traffic Signed-off-by: Nicolas Flacco --- .../filters/network/redis_proxy/router_impl.cc | 3 ++- .../filters/network/redis_proxy/router_impl.h | 1 + .../network/redis_proxy/router_impl_test.cc | 16 ++++++++++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/network/redis_proxy/router_impl.cc b/source/extensions/filters/network/redis_proxy/router_impl.cc index c18f0e46c277..9a6b33dc87fc 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.cc +++ b/source/extensions/filters/network/redis_proxy/router_impl.cc @@ -10,6 +10,7 @@ MirrorPolicyImpl::MirrorPolicyImpl(const envoy::config::filter::network::redis_p const ConnPool::InstanceSharedPtr upstream, Runtime::Loader& runtime) : runtime_key_(config.runtime_fraction().runtime_key()), + has_default_value_(config.has_runtime_fraction()), default_value_(config.runtime_fraction().default_value()), exclude_read_commands_(config.exclude_read_commands()), upstream_(upstream), runtime_(runtime) {} @@ -23,7 +24,7 @@ bool MirrorPolicyImpl::shouldMirror(const std::string& command) const { return false; } - if (default_value_.numerator() >= 0) { + if (has_default_value_) { return runtime_.snapshot().featureEnabled(runtime_key_, default_value_); } diff --git a/source/extensions/filters/network/redis_proxy/router_impl.h b/source/extensions/filters/network/redis_proxy/router_impl.h index 26963adb1898..f4a8cdfcba99 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.h +++ b/source/extensions/filters/network/redis_proxy/router_impl.h @@ -37,6 +37,7 @@ class MirrorPolicyImpl : public MirrorPolicy { private: const std::string runtime_key_; + const bool has_default_value_; const envoy::type::FractionalPercent default_value_; const bool exclude_read_commands_; ConnPool::InstanceSharedPtr upstream_; diff --git a/test/extensions/filters/network/redis_proxy/router_impl_test.cc b/test/extensions/filters/network/redis_proxy/router_impl_test.cc index 28f66bbd871b..b4d12c41942a 100644 --- a/test/extensions/filters/network/redis_proxy/router_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/router_impl_test.cc @@ -225,6 +225,22 @@ TEST(MirrorPolicyImplTest, ExcludeReadCommands) { EXPECT_EQ(true, policy.shouldMirror("set")); } +TEST(MirrorPolicyImplTest, DefaultValueZero) { + envoy::config::filter::network::redis_proxy::v2::RedisProxy::PrefixRoutes::Route:: + RequestMirrorPolicy config; + auto* runtime_fraction = config.mutable_runtime_fraction(); + auto* percentage = runtime_fraction->mutable_default_value(); + percentage->set_numerator(0); + percentage->set_denominator(envoy::type::FractionalPercent::HUNDRED); + auto upstream = std::make_shared(); + NiceMock runtime; + + MirrorPolicyImpl policy(config, upstream, runtime); + + EXPECT_EQ(false, policy.shouldMirror("get")); + EXPECT_EQ(false, policy.shouldMirror("set")); +} + TEST(MirrorPolicyImplTest, DeterminedByRuntimeFraction) { envoy::config::filter::network::redis_proxy::v2::RedisProxy::PrefixRoutes::Route:: RequestMirrorPolicy config; From 846d286c97ea0a7c861bbdfc7894fec4e5824e0e Mon Sep 17 00:00:00 2001 From: Nicolas Flacco Date: Tue, 10 Sep 2019 17:13:35 -0700 Subject: [PATCH 3/4] use has_runtime_fraction_ instead of has_default_value_ Signed-off-by: Nicolas Flacco --- source/extensions/filters/network/redis_proxy/router_impl.cc | 4 ++-- source/extensions/filters/network/redis_proxy/router_impl.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/source/extensions/filters/network/redis_proxy/router_impl.cc b/source/extensions/filters/network/redis_proxy/router_impl.cc index 9a6b33dc87fc..ceada8a998ca 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.cc +++ b/source/extensions/filters/network/redis_proxy/router_impl.cc @@ -10,7 +10,7 @@ MirrorPolicyImpl::MirrorPolicyImpl(const envoy::config::filter::network::redis_p const ConnPool::InstanceSharedPtr upstream, Runtime::Loader& runtime) : runtime_key_(config.runtime_fraction().runtime_key()), - has_default_value_(config.has_runtime_fraction()), + has_runtime_fraction_(config.has_runtime_fraction()), default_value_(config.runtime_fraction().default_value()), exclude_read_commands_(config.exclude_read_commands()), upstream_(upstream), runtime_(runtime) {} @@ -24,7 +24,7 @@ bool MirrorPolicyImpl::shouldMirror(const std::string& command) const { return false; } - if (has_default_value_) { + if (has_runtime_fraction_) { return runtime_.snapshot().featureEnabled(runtime_key_, default_value_); } diff --git a/source/extensions/filters/network/redis_proxy/router_impl.h b/source/extensions/filters/network/redis_proxy/router_impl.h index f4a8cdfcba99..144758f9c1c3 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.h +++ b/source/extensions/filters/network/redis_proxy/router_impl.h @@ -37,7 +37,7 @@ class MirrorPolicyImpl : public MirrorPolicy { private: const std::string runtime_key_; - const bool has_default_value_; + const bool has_runtime_fraction_; const envoy::type::FractionalPercent default_value_; const bool exclude_read_commands_; ConnPool::InstanceSharedPtr upstream_; From 5f4b164e9e6a73cec6d8844a835300ab7dbbc71a Mon Sep 17 00:00:00 2001 From: Nicolas Flacco Date: Wed, 11 Sep 2019 12:48:45 -0700 Subject: [PATCH 4/4] use ternary operator with nullopt as fallback Signed-off-by: Nicolas Flacco --- .../filters/network/redis_proxy/router_impl.cc | 9 +++++---- .../extensions/filters/network/redis_proxy/router_impl.h | 3 +-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/network/redis_proxy/router_impl.cc b/source/extensions/filters/network/redis_proxy/router_impl.cc index ceada8a998ca..1cb86f68762e 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.cc +++ b/source/extensions/filters/network/redis_proxy/router_impl.cc @@ -10,8 +10,9 @@ MirrorPolicyImpl::MirrorPolicyImpl(const envoy::config::filter::network::redis_p const ConnPool::InstanceSharedPtr upstream, Runtime::Loader& runtime) : runtime_key_(config.runtime_fraction().runtime_key()), - has_runtime_fraction_(config.has_runtime_fraction()), - default_value_(config.runtime_fraction().default_value()), + default_value_(config.has_runtime_fraction() ? absl::optional( + config.runtime_fraction().default_value()) + : absl::nullopt), exclude_read_commands_(config.exclude_read_commands()), upstream_(upstream), runtime_(runtime) {} @@ -24,8 +25,8 @@ bool MirrorPolicyImpl::shouldMirror(const std::string& command) const { return false; } - if (has_runtime_fraction_) { - return runtime_.snapshot().featureEnabled(runtime_key_, default_value_); + if (default_value_.has_value()) { + return runtime_.snapshot().featureEnabled(runtime_key_, default_value_.value()); } return true; diff --git a/source/extensions/filters/network/redis_proxy/router_impl.h b/source/extensions/filters/network/redis_proxy/router_impl.h index 144758f9c1c3..fdacf760891b 100644 --- a/source/extensions/filters/network/redis_proxy/router_impl.h +++ b/source/extensions/filters/network/redis_proxy/router_impl.h @@ -37,8 +37,7 @@ class MirrorPolicyImpl : public MirrorPolicy { private: const std::string runtime_key_; - const bool has_runtime_fraction_; - const envoy::type::FractionalPercent default_value_; + const absl::optional default_value_; const bool exclude_read_commands_; ConnPool::InstanceSharedPtr upstream_; Runtime::Loader& runtime_;