From 23da112334733e371b2b2f19e35f1ee3c78730c5 Mon Sep 17 00:00:00 2001 From: alyssawilk Date: Wed, 13 Mar 2019 13:54:03 -0400 Subject: [PATCH] runtime: codifying runtime guarded features (#6134) Documenting best practices and adding some tooling for guarding high risk features Risk Level: Low (mostly new code) Testing: Added new unit tests Docs Changes: updated CONTRIBUTING.md Release Notes: n/a Part of #5693 (TODO: handling deprecating and adding tooling scripts) Signed-off-by: Alyssa Wilk --- CONTRIBUTING.md | 55 ++++++++++++++++ docs/root/configuration/runtime.rst | 2 +- include/envoy/runtime/runtime.h | 7 +++ source/common/protobuf/utility.h | 2 +- source/common/runtime/BUILD | 5 +- source/common/runtime/runtime_features.cc | 62 +++++++++++++++++++ source/common/runtime/runtime_features.h | 29 +++++---- source/common/runtime/runtime_impl.cc | 30 +++++++-- source/common/runtime/runtime_impl.h | 3 + test/BUILD | 1 + test/common/runtime/BUILD | 23 +++++++ .../runtime/runtime_flag_override_test.cc | 17 +++++ test/common/runtime/runtime_impl_test.cc | 19 ++++++ test/common/runtime/utility.h | 22 +++++++ test/mocks/runtime/mocks.h | 1 + test/test_common/BUILD | 1 + test/test_runner.h | 62 +++++++++++++++++++ 17 files changed, 320 insertions(+), 21 deletions(-) create mode 100644 source/common/runtime/runtime_features.cc create mode 100644 test/common/runtime/runtime_flag_override_test.cc create mode 100644 test/common/runtime/utility.h diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fb22eeb737ff..a952e80e0bf9 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -137,6 +137,61 @@ maximize the chances of your PR being merged. [envoy-filter-example](https://github.com/envoyproxy/envoy-filter-example) (for example making a new branch so that CI can pass) it is your responsibility to follow through with merging those changes back to master once the CI dance is done. +* If your PR is a high risk change, the reviewer may ask that you runtime guard + it. See the section on runtime guarding below. + + +# Runtime guarding + +Some high risk changes in Envoy are deemed worthy of runtime guarding. Instead of just replacing +old code with new code, both code paths are supported for between one Envoy release (if it is +guarded due to performance concerns) and a full deprecation cycle (if it is a high risk behavioral +change). + +The canonical way to runtime guard a feature is +``` +if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.my_feature_name")) { + [new code path] +} else { + [old_code_path] +} +``` +Runtime guarded features named with the "envoy.reloadable_features." prefix must be safe to flip +true or false on running Envoy instances. In some situations it may make more sense to +latch the value in a member variable on class creation, for example: + +``` +bool use_new_code_path_ = + Runtime::runtimeFeatureEnabled("envoy.reloadable_features.my_feature_name") +``` + +This should only be done if the lifetime of the object in question is relatively short compared to +the lifetime of most Envoy instances, i.e. latching state on creation of the +Http::ConnectionManagerImpl or all Network::ConnectionImpl classes, to ensure that the new behavior +will be exercised as the runtime value is flipped, and that the old behavior will trail off over +time. + +Runtime guarded features may either set true (running the new code by default) in the initial PR, +after a testing interval, or during the next release cycle, at the PR author's and reviewing +maintainer's discretion. Generally all runtime guarded features will be set true when a +release is cut, and the old code path will be deprecated at that time. Runtime features +are set true by default by inclusion in +[source/common/runtime/runtime_features.h](https://github.com/envoyproxy/envoy/blob/master/source/common/runtime/runtime_features.h) + +There are four suggested options for testing new runtime features: + +1. Create a per-test Runtime::LoaderSingleton as done in [DeprecatedFieldsTest.IndividualFieldDisallowedWithRuntimeOverride](https://github.com/envoyproxy/envoy/blob/master/test/common/protobuf/utility_test.cc) +2. Create a [parameterized test](https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#how-to-write-value-parameterized-tests) + where the set up of the test sets the new runtime value explicitly to + GetParam() as outlined in (1). +3. Set up integration tests with custom runtime defaults as documented in the + [integration test README](https://github.com/envoyproxy/envoy/blob/master/test/integration/README.md) +4. Run a given unit test with the new runtime value explicitly set true as done + for [runtime_flag_override_test](https://github.com/envoyproxy/envoy/blob/master/test/common/runtime/BUILD) + +Runtime code is held to the same standard as regular Envoy code, so both the old +path and the new should have 100% coverage both with the feature defaulting true +and false. # PR review policy for maintainers diff --git a/docs/root/configuration/runtime.rst b/docs/root/configuration/runtime.rst index 0c2dce87e4bb..7d3ced54614c 100644 --- a/docs/root/configuration/runtime.rst +++ b/docs/root/configuration/runtime.rst @@ -93,7 +93,7 @@ Users are encouraged to go to :repo:`DEPRECATED.md ` to see how t migrate to the new code path and make sure it is suitable for their use case. In the second phase the message and filename will be added to -:repo:`runtime_features.h ` +:repo:`runtime_features.cc ` and use of that configuration field will cause the config to be rejected by default. This fail-by-default mode can be overridden in runtime configuration by setting envoy.deprecated_features.filename.proto:fieldname to true. For example, for a deprecated field diff --git a/include/envoy/runtime/runtime.h b/include/envoy/runtime/runtime.h index 6b424e74907a..499fd48e5ee6 100644 --- a/include/envoy/runtime/runtime.h +++ b/include/envoy/runtime/runtime.h @@ -83,6 +83,13 @@ class Snapshot { // configuration of "false" in runtime config. virtual bool deprecatedFeatureEnabled(const std::string& key) const PURE; + // Returns true if a runtime feature is enabled. + // + // Runtime features are used to easily allow switching between old and new code paths for high + // risk changes. The intent is for the old code path to be short lived - the old code path is + // deprecated as the feature is defaulted true, and removed with the following Envoy release. + virtual bool runtimeFeatureEnabled(const std::string& key) const PURE; + /** * Test if a feature is enabled using the built in random generator. This is done by generating * a random number in the range 0-99 and seeing if this number is < the value stored in the diff --git a/source/common/protobuf/utility.h b/source/common/protobuf/utility.h index 04961bbd2d6d..c5ae9635755a 100644 --- a/source/common/protobuf/utility.h +++ b/source/common/protobuf/utility.h @@ -205,7 +205,7 @@ class MessageUtil { * @param message message to validate. * @param loader optional a pointer to the runtime loader for live deprecation status. * @throw ProtoValidationException if deprecated fields are used and listed - * Runtime::DisallowedFeatures + * in disallowed_features in runtime_features.h */ static void checkForDeprecation(const Protobuf::Message& message, diff --git a/source/common/runtime/BUILD b/source/common/runtime/BUILD index 6cb7f339b36c..91874797b024 100644 --- a/source/common/runtime/BUILD +++ b/source/common/runtime/BUILD @@ -10,7 +10,10 @@ envoy_package() envoy_cc_library( name = "runtime_lib", - srcs = ["runtime_impl.cc"], + srcs = [ + "runtime_features.cc", + "runtime_impl.cc", + ], hdrs = [ "runtime_features.h", "runtime_impl.h", diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc new file mode 100644 index 000000000000..7e63701b1c88 --- /dev/null +++ b/source/common/runtime/runtime_features.cc @@ -0,0 +1,62 @@ +#include "common/runtime/runtime_features.h" + +namespace Envoy { +namespace Runtime { + +// Add additional features here to enable the new code paths by default. +// +// Per documentation in CONTRIBUTING.md is is expected that new high risk code paths be guarded +// by runtime feature guards, i.e +// +// if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.my_feature_name")) { +// [new code path] +// else { +// [old_code_path] +// } +// +// Runtime features are false by default, so the old code path is exercised. +// To make a runtime feature true by default, add it to the array below. +// New features should be true-by-default for an Envoy release cycle before the +// old code path is removed. +// +// If issues are found that that require a runtime feature to be disabled, it should be reported +// ASAP by filing a bug on github. Overriding non-buggy code is strongly discouraged to avoid the +// problem of the bugs being found after the old code path has been removed. +constexpr const char* runtime_features[] = { + // Enabled + "envoy.reloadable_features.test_feature_true", +}; + +// This is a list of configuration fields which are disallowed by default in Envoy +// +// By default, use of proto fields marked as deprecated in their api/.../*.proto file will result +// in a logged warning, so that Envoy users have a warning that they are using deprecated fields. +// +// During the Envoy release cycle, the maintainer team runs a script which will upgrade currently +// deprecated features to be disallowed (adding them to the list below) at which point use of said +// feature will cause a hard-failure (ProtoValidationException) instead of a logged warning. +// +// The release cycle after a feature has been marked disallowed, it is officially removable, and +// the maintainer team will run a script creating a tracking issue for proto and code clean up. +// +// TODO(alyssawilk) handle deprecation of reloadable_features and update the above comment. Ideally +// runtime override of a deprecated feature will log(warn) on runtime-load if not deprecated +// and hard-fail once it has been deprecated. + +constexpr const char* disallowed_features[] = { + // Acts as both a test entry for deprecated.proto and a marker for the Envoy + // deprecation scripts. + "envoy.deprecated_features.deprecated.proto:is_deprecated_fatal", +}; + +RuntimeFeatures::RuntimeFeatures() { + for (auto& feature : disallowed_features) { + disallowed_features_.insert(feature); + } + for (auto& feature : runtime_features) { + enabled_features_.insert(feature); + } +} + +} // namespace Runtime +} // namespace Envoy diff --git a/source/common/runtime/runtime_features.h b/source/common/runtime/runtime_features.h index d57aab19c6d2..340eae0d5b4f 100644 --- a/source/common/runtime/runtime_features.h +++ b/source/common/runtime/runtime_features.h @@ -9,29 +9,32 @@ namespace Envoy { namespace Runtime { -const char* disallowed_features[] = { - // Acts as both a test entry for deprecated.proto and a marker for the Envoy - // deprecation scripts. - "envoy.deprecated_features.deprecated.proto:is_deprecated_fatal", -}; - -class DisallowedFeatures { +// TODO(alyssawilk) convert these to string view. +class RuntimeFeatures { public: - DisallowedFeatures() { - for (auto& feature : disallowed_features) { - disallowed_features_.insert(feature); - } - } + RuntimeFeatures(); + // This tracks proto configured features, to determine if a given deprecated + // feature is still allowed, or has been made fatal-by-default per the Envoy + // deprecation process. bool disallowedByDefault(const std::string& feature) const { return disallowed_features_.find(feature) != disallowed_features_.end(); } + // This tracks config-guarded code paths, to determine if a given + // runtime-guarded-code-path has the new code run by default or the old code. + bool enabledByDefault(const std::string& feature) const { + return enabled_features_.find(feature) != enabled_features_.end(); + } + private: + friend class RuntimeFeaturesPeer; + absl::flat_hash_set disallowed_features_; + absl::flat_hash_set enabled_features_; }; -using DisallowedFeaturesDefaults = ConstSingleton; +using RuntimeFeaturesDefaults = ConstSingleton; } // namespace Runtime } // namespace Envoy diff --git a/source/common/runtime/runtime_impl.cc b/source/common/runtime/runtime_impl.cc index b8e79b07c24c..da0bab28fe60 100644 --- a/source/common/runtime/runtime_impl.cc +++ b/source/common/runtime/runtime_impl.cc @@ -22,6 +22,16 @@ namespace Envoy { namespace Runtime { +bool runtimeFeatureEnabled(const std::string& feature) { + ASSERT(absl::StartsWith(feature, "envoy.reloadable_features")); + if (Runtime::LoaderSingleton::getExisting()) { + return Runtime::LoaderSingleton::getExisting()->snapshot().runtimeFeatureEnabled(feature); + } + ENVOY_LOG_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::runtime), warn, + "Unable to use runtime singleton for feature {}", feature); + return RuntimeFeaturesDefaults::get().enabledByDefault(feature); +} + const size_t RandomGeneratorImpl::UUID_LENGTH = 36; uint64_t RandomGeneratorImpl::random() { @@ -148,11 +158,10 @@ std::string RandomGeneratorImpl::uuid() { bool SnapshotImpl::deprecatedFeatureEnabled(const std::string& key) const { bool allowed = false; - // See if this value is explicitly set as a runtime boolean. - bool stored = getBoolean(key, allowed); - // If not, the default value is based on disallowedByDefault. - if (!stored) { - allowed = !DisallowedFeaturesDefaults::get().disallowedByDefault(key); + // If the value is not explicitly set as a runtime boolean, the default value is based on + // disallowedByDefault. + if (!getBoolean(key, allowed)) { + allowed = !RuntimeFeaturesDefaults::get().disallowedByDefault(key); } if (!allowed) { @@ -165,6 +174,17 @@ bool SnapshotImpl::deprecatedFeatureEnabled(const std::string& key) const { return true; } +bool SnapshotImpl::runtimeFeatureEnabled(const std::string& key) const { + bool enabled = false; + // If the value is not explicitly set as a runtime boolean, the default value is based on + // disallowedByDefault. + if (!getBoolean(key, enabled)) { + enabled = RuntimeFeaturesDefaults::get().enabledByDefault(key); + } + + return enabled; +} + bool SnapshotImpl::featureEnabled(const std::string& key, uint64_t default_value, uint64_t random_value, uint64_t num_buckets) const { return random_value % num_buckets < std::min(getInteger(key, default_value), num_buckets); diff --git a/source/common/runtime/runtime_impl.h b/source/common/runtime/runtime_impl.h index edbed0bbba4a..02876f336d2c 100644 --- a/source/common/runtime/runtime_impl.h +++ b/source/common/runtime/runtime_impl.h @@ -24,6 +24,8 @@ namespace Envoy { namespace Runtime { +bool runtimeFeatureEnabled(const std::string& feature); + using RuntimeSingleton = ThreadSafeSingleton; /** @@ -72,6 +74,7 @@ class SnapshotImpl : public Snapshot, // Runtime::Snapshot bool deprecatedFeatureEnabled(const std::string& key) const override; + bool runtimeFeatureEnabled(const std::string& key) const override; bool featureEnabled(const std::string& key, uint64_t default_value, uint64_t random_value, uint64_t num_buckets) const override; bool featureEnabled(const std::string& key, uint64_t default_value) const override; diff --git a/test/BUILD b/test/BUILD index 6cde7f42fbf1..9e194a76dcf2 100644 --- a/test/BUILD +++ b/test/BUILD @@ -31,6 +31,7 @@ envoy_cc_test_library( "//source/common/event:libevent_lib", "//source/common/http/http2:codec_lib", "//source/common/thread:thread_factory_singleton_lib", + "//test/common/runtime:utility_lib", "//test/mocks/access_log:access_log_mocks", "//test/test_common:environment_lib", "//test/test_common:global_lib", diff --git a/test/common/runtime/BUILD b/test/common/runtime/BUILD index a16dbba3e368..a970d50e543e 100644 --- a/test/common/runtime/BUILD +++ b/test/common/runtime/BUILD @@ -3,6 +3,7 @@ licenses(["notice"]) # Apache 2 load( "//bazel:envoy_build_system.bzl", "envoy_cc_test", + "envoy_cc_test_library", "envoy_package", ) @@ -15,6 +16,16 @@ filegroup( srcs = glob(["test_data/**"]), ) +envoy_cc_test_library( + name = "utility_lib", + hdrs = [ + "utility.h", + ], + deps = [ + "//source/common/runtime:runtime_lib", + ], +) + envoy_cc_test( name = "runtime_impl_test", srcs = ["runtime_impl_test.cc"], @@ -31,6 +42,18 @@ envoy_cc_test( ], ) +envoy_cc_test( + name = "runtime_flag_override_test", + srcs = ["runtime_flag_override_test.cc"], + args = [ + "--runtime-feature-override-for-tests=envoy.reloadable_features.test_feature_false", + ], + coverage = False, + deps = [ + "//source/common/runtime:runtime_lib", + ], +) + envoy_cc_test( name = "uuid_util_test", srcs = ["uuid_util_test.cc"], diff --git a/test/common/runtime/runtime_flag_override_test.cc b/test/common/runtime/runtime_flag_override_test.cc new file mode 100644 index 000000000000..a083e723bad8 --- /dev/null +++ b/test/common/runtime/runtime_flag_override_test.cc @@ -0,0 +1,17 @@ +#include "common/runtime/runtime_impl.h" + +#include "gmock/gmock.h" + +namespace Envoy { +namespace Runtime { + +// Features not in runtime_features.cc are false by default (and this particular one is verified to +// be false in runtime_impl_test.cc). However in in the envoy_cc_test declaration, the flag is set +// "--runtime-feature-override-for-tests=envoy.reloadable_features.test_feature_false" +// to override the return value of runtimeFeatureEnabled to true. +TEST(RuntimeFlagOverrideTest, OverridesWork) { + EXPECT_TRUE(Runtime::runtimeFeatureEnabled("envoy.reloadable_features.test_feature_false")); +} + +} // namespace Runtime +} // namespace Envoy diff --git a/test/common/runtime/runtime_impl_test.cc b/test/common/runtime/runtime_impl_test.cc index 6e26b4893b10..5ec499348291 100644 --- a/test/common/runtime/runtime_impl_test.cc +++ b/test/common/runtime/runtime_impl_test.cc @@ -120,6 +120,16 @@ TEST_F(DiskBackedLoaderImplTest, All) { // File1 is not a boolean. EXPECT_EQ(false, snapshot->getBoolean("file1", value)); + // Feature defaults. + // test_feature_true is explicitly set true in runtime_features.cc + EXPECT_EQ(true, snapshot->runtimeFeatureEnabled("envoy.reloadable_features.test_feature_true")); + // test_feature_false is not in runtime_features.cc and so is false by default. + EXPECT_EQ(false, snapshot->runtimeFeatureEnabled("envoy.reloadable_features.test_feature_false")); + + // Feature defaults via helper function. + EXPECT_EQ(false, runtimeFeatureEnabled("envoy.reloadable_features.test_feature_false")); + EXPECT_EQ(true, runtimeFeatureEnabled("envoy.reloadable_features.test_feature_true")); + // Files with comments. EXPECT_EQ(123UL, loader->snapshot().getInteger("file5", 1)); EXPECT_EQ("/home#about-us", loader->snapshot().get("file6")); @@ -340,6 +350,15 @@ TEST_F(DiskLayerTest, Loop) { EnvoyException, "Walk recursion depth exceeded 16"); } +TEST(NoRuntime, FeatureEnabled) { + // Make sure the registry is not set up. + ASSERT_TRUE(Runtime::LoaderSingleton::getExisting() == nullptr); + + // Feature defaults should still work. + EXPECT_EQ(false, runtimeFeatureEnabled("envoy.reloadable_features.test_feature_false")); + EXPECT_EQ(true, runtimeFeatureEnabled("envoy.reloadable_features.test_feature_true")); +} + } // namespace } // namespace Runtime } // namespace Envoy diff --git a/test/common/runtime/utility.h b/test/common/runtime/utility.h new file mode 100644 index 000000000000..49b91f137e71 --- /dev/null +++ b/test/common/runtime/utility.h @@ -0,0 +1,22 @@ +#pragma once + +#include "common/runtime/runtime_features.h" + +namespace Envoy { +namespace Runtime { + +class RuntimeFeaturesPeer { +public: + static bool addFeature(const std::string& feature) { + return const_cast(&Runtime::RuntimeFeaturesDefaults::get()) + ->enabled_features_.insert(feature) + .second; + } + static void removeFeature(const std::string& feature) { + const_cast(&Runtime::RuntimeFeaturesDefaults::get()) + ->enabled_features_.erase(feature); + } +}; + +} // namespace Runtime +} // namespace Envoy diff --git a/test/mocks/runtime/mocks.h b/test/mocks/runtime/mocks.h index fe70e3e56f99..2753c09593b6 100644 --- a/test/mocks/runtime/mocks.h +++ b/test/mocks/runtime/mocks.h @@ -28,6 +28,7 @@ class MockSnapshot : public Snapshot { ~MockSnapshot() override; MOCK_CONST_METHOD1(deprecatedFeatureEnabled, bool(const std::string& key)); + MOCK_CONST_METHOD1(runtimeFeatureEnabled, bool(const std::string& key)); MOCK_CONST_METHOD2(featureEnabled, bool(const std::string& key, uint64_t default_value)); MOCK_CONST_METHOD3(featureEnabled, bool(const std::string& key, uint64_t default_value, uint64_t random_value)); diff --git a/test/test_common/BUILD b/test/test_common/BUILD index f41fe1ad31be..b54f4f282686 100644 --- a/test/test_common/BUILD +++ b/test/test_common/BUILD @@ -35,6 +35,7 @@ envoy_cc_test_library( "//source/common/json:json_loader_lib", "//source/common/network:utility_lib", "//source/server:options_lib", + "//test/common/runtime:utility_lib", ], ) diff --git a/test/test_runner.h b/test/test_runner.h index 2e65e604d92c..25b4e19faef5 100644 --- a/test/test_runner.h +++ b/test/test_runner.h @@ -5,7 +5,9 @@ #include "common/common/thread.h" #include "common/event/libevent.h" #include "common/http/http2/codec_impl.h" +#include "common/runtime/runtime_features.h" +#include "test/common/runtime/utility.h" #include "test/mocks/access_log/mocks.h" #include "test/test_common/environment.h" #include "test/test_listener.h" @@ -13,6 +15,52 @@ #include "gmock/gmock.h" namespace Envoy { +namespace { + +std::string findAndRemove(const std::regex& pattern, int& argc, char**& argv) { + std::smatch matched; + std::string return_value; + for (int i = 0; i < argc; ++i) { + if (return_value.empty()) { + std::string argument = std::string(argv[i]); + if (regex_search(argument, matched, pattern)) { + return_value = matched[1]; + argc--; + } + } + if (!return_value.empty() && i < argc) { + argv[i] = argv[i + 1]; + } + } + return return_value; +} + +// This class is created iff a test is run with the special runtime override flag. +class RuntimeManagingListener : public ::testing::EmptyTestEventListener { +public: + RuntimeManagingListener(std::string& runtime_override) : runtime_override_(runtime_override) {} + + // On each test start, edit RuntimeFeaturesDefaults with our custom runtime defaults. + void OnTestStart(const ::testing::TestInfo&) override { + if (!runtime_override_.empty()) { + if (!Runtime::RuntimeFeaturesPeer::addFeature(runtime_override_)) { + // If the entry was already in the hash map, don't remove it OnTestEnd. + runtime_override_.clear(); + } + } + } + + // As each test ends, clean up the RuntimeFeaturesDefaults state. + void OnTestEnd(const ::testing::TestInfo&) override { + if (!runtime_override_.empty()) { + Runtime::RuntimeFeaturesPeer::removeFeature(runtime_override_); + } + } + std::string runtime_override_; +}; + +} // namespace + class TestRunner { public: static int RunTests(int argc, char** argv) { @@ -40,6 +88,20 @@ class TestRunner { TestEnvironment::setEnvVar("TEST_UDSDIR", TestEnvironment::unixDomainSocketDirectory(), 1); + // Before letting TestEnvironment latch argv and argc, remove any runtime override flag. + // This allows doing test overrides of Envoy runtime features without adding + // test flags to the Envoy production command line. + const std::regex PATTERN{"--runtime-feature-override-for-tests=(.*)", std::regex::optimize}; + std::string runtime_override = findAndRemove(PATTERN, argc, argv); + if (!runtime_override.empty()) { + ENVOY_LOG_TO_LOGGER(Logger::Registry::getLog(Logger::Id::testing), info, + "Running with runtime feature override {}", runtime_override); + // Set up a listener which will create a global runtime and set the feature + // to true for the duration of each test instance. + ::testing::TestEventListeners& listeners = ::testing::UnitTest::GetInstance()->listeners(); + listeners.Append(new RuntimeManagingListener(runtime_override)); + } + TestEnvironment::initializeOptions(argc, argv); Thread::MutexBasicLockable lock; Logger::Context logging_state(TestEnvironment::getOptions().logLevel(),