diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index 86c59606a80e..befad79c2b4c 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -122,6 +122,18 @@ bool SnapshotImpl::featureEnabled(absl::string_view key, percent = default_value; } + // When numerator > denominator condition is always evaluates to TRUE + // It becomes hard to debug why configuration does not work in case of wrong numerator. + // Log debug message that numerator is invalid. + uint64_t denominator_value = + ProtobufPercentHelper::fractionalPercentDenominatorToInt(percent.denominator()); + if (percent.numerator() > denominator_value) { + ENVOY_LOG(debug, + "WARNING runtime key '{}': numerator ({}) > denominator ({}), condition always " + "evaluates to true", + key, percent.numerator(), denominator_value); + } + return ProtobufPercentHelper::evaluateFractionalPercent(percent, random_value); } diff --git a/test/common/runtime/BUILD b/test/common/runtime/BUILD index 8cb4a424daa7..151b3d071eb7 100644 --- a/test/common/runtime/BUILD +++ b/test/common/runtime/BUILD @@ -57,6 +57,7 @@ envoy_cc_test( "//test/mocks/thread_local:thread_local_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:environment_lib", + "//test/test_common:logging_lib", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", "@envoy_api//envoy/service/discovery/v3:pkg_cc_proto", diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index dad18c5d2bf8..d300bd634e48 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -23,6 +23,7 @@ #include "test/mocks/thread_local/mocks.h" #include "test/mocks/upstream/mocks.h" #include "test/test_common/environment.h" +#include "test/test_common/logging.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -684,6 +685,25 @@ TEST_F(StaticLoaderImplTest, ProtoParsing) { EXPECT_EQ(2, store_.gauge("runtime.num_layers", Stats::Gauge::ImportMode::NeverImport).value()); } +TEST_F(StaticLoaderImplTest, InvalidNumerator) { + base_ = TestUtility::parseYaml(R"EOF( + invalid_numerator: + numerator: 111 + denominator: HUNDRED + )EOF"); + setup(); + + envoy::type::v3::FractionalPercent fractional_percent; + + // There is no assertion here - when numerator is invalid + // featureEnabled() will just drop debug log line. + EXPECT_CALL(generator_, random()).WillOnce(Return(500000)); + EXPECT_LOG_CONTAINS("debug", + "runtime key 'invalid_numerator': numerator (111) > denominator (100), " + "condition always evaluates to true", + loader_->snapshot().featureEnabled("invalid_numerator", fractional_percent)); +} + TEST_F(StaticLoaderImplTest, RuntimeFromNonWorkerThreads) { // Force the thread to be considered a non-worker thread. tls_.registered_ = false;