runtime: codifying runtime guarded features

CONTRIBUTING.md

@@ -137,6 +137,56 @@ 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 canonican way to runtime guard a feature is
+```
+if (Runtime::LoaderSingleton::getExisting() &&
+    Runtime::LoaderSingleton::getExisting()->snapshot()->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, for example the buffer rewrite in
+[#5441](https://github.com/envoyproxy/envoy/pull/5441), it may make more sense to
+latch the value in a member variable on class creation, for example:
+
+```
+bool use_new_code_path_ = Runtime::LoaderSingleton::getExisting() &&
+  Runtime::LoaderSingleton::getExisting()->snapshot()->runtimeFeatureEnabled(
+      "envoy.reloadable_features.my_feature_name"));
+```
+
+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 three 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. 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)
+3. 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
 

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

source/common/protobuf/utility.h

@@ -195,7 +195,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,

source/common/runtime/runtime_features.h

@@ -9,29 +9,57 @@
 namespace Envoy {
 namespace Runtime {
 
+// Add additional features here to enable the new code paths by default.
+//
+// These features should not be overridden with run-time guards without a bug
+// being filed on github as once high risk features are true by default, the
+// old code path will be removed with the next release.
+const char* runtime_features[] = {
+    // Enabled
+    "envoy.reloadable_features.test_feature_true",
+    // Disabled
+    // "envoy.reloadable_features.test_feature_false",
+};
+
+// TODO(alyssawilk) handle deprecation of reloadable_features. Ideally runtime
+// override of a deprecated feature will log(warn) on runtime-load if not deprecated
+// and hard-fail once it has been deprecated.
 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 {
+class RuntimeFeatures {
 public:
-  DisallowedFeatures() {
+  RuntimeFeatures() {
     for (auto& feature : disallowed_features) {
       disallowed_features_.insert(feature);
     }
+    for (auto& feature : runtime_features) {
+      enabled_features_.insert(feature);
+    }
   }
 
+  // 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:
   absl::flat_hash_set<std::string> disallowed_features_;
+  absl::flat_hash_set<std::string> enabled_features_;
 };
 
-using DisallowedFeaturesDefaults = ConstSingleton<DisallowedFeatures>;
+using RuntimeFeaturesDefaults = ConstSingleton<RuntimeFeatures>;
 
 } // namespace Runtime
 } // namespace Envoy

source/common/runtime/runtime_impl.cc

@@ -152,7 +152,7 @@ bool SnapshotImpl::deprecatedFeatureEnabled(const std::string& key) const {
   bool stored = getBoolean(key, allowed);
   // If not, the default value is based on disallowedByDefault.
   if (!stored) {
-    allowed = !DisallowedFeaturesDefaults::get().disallowedByDefault(key);
+    allowed = !RuntimeFeaturesDefaults::get().disallowedByDefault(key);
   }
 
   if (!allowed) {
@@ -165,6 +165,18 @@ bool SnapshotImpl::deprecatedFeatureEnabled(const std::string& key) const {
   return true;
 }
 
+bool SnapshotImpl::runtimeFeatureEnabled(const std::string& key) const {
+  bool enabled = false;
+  // See if this value is explicitly set as a runtime boolean.
+  bool stored = getBoolean(key, enabled);
+  // If not, the default value is based on runtime_features.
+  if (!stored) {
+    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);

source/common/runtime/runtime_impl.h

@@ -72,6 +72,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;

test/common/runtime/BUILD

@@ -31,6 +31,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"],

test/common/runtime/runtime_flag_override_test.cc

@@ -0,0 +1,16 @@
+#include "common/runtime/runtime_impl.h"
+
+#include "gmock/gmock.h"
+
+namespace Envoy {
+namespace Runtime {
+
+// In the envoy_cc_test declaration, the flag is set
+// "--runtime-feature-override-for-tests=envoy.reloadable_features.test_feature_false"
+TEST(RuntimeFlagOverrideTest, OverridesWork) {
+  Snapshot& snapshot = Runtime::LoaderSingleton::getExisting()->snapshot();
+  EXPECT_EQ(true, snapshot.runtimeFeatureEnabled("envoy.reloadable_features.test_feature_false"));
+}
+
+} // namespace Runtime
+} // namespace Envoy

test/common/runtime/runtime_impl_test.cc

@@ -119,6 +119,10 @@ TEST_F(DiskBackedLoaderImplTest, All) {
   // File1 is not a boolean.
   EXPECT_EQ(false, snapshot->getBoolean("file1", value));
 
+  // Feature defaults.
+  EXPECT_EQ(false, snapshot->runtimeFeatureEnabled("envoy.reloadable_features.test_feature_false"));
+  EXPECT_EQ(true, snapshot->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"));

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));

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/mocks/server:server_mocks",
     ],
 )
 

test/test_common/environment.cc

@@ -20,19 +20,71 @@
 #include "common/common/compiler_requirements.h"
 #include "common/common/logger.h"
 #include "common/common/macros.h"
+#include "common/runtime/runtime_impl.h"
 #include "common/common/utility.h"
 
 #include "server/options_impl.h"
 
+#include "test/mocks/server/mocks.h"
 #include "test/test_common/network_utility.h"
 
 #include "absl/strings/match.h"
 #include "test/test_common/test_base.h"
 #include "spdlog/spdlog.h"
 
+#include "tclap/CmdLine.h"
+
+#include "gtest/gtest.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, create and register a runtime instance, with this specific feature set to
+  // true.
+  void OnTestStart(const ::testing::TestInfo&) override {
+    if (!runtime_override_.empty()) {
+      runtime_state_ = std::make_unique<RuntimeState>();
+      Runtime::LoaderSingleton::getExisting()->mergeValues({{runtime_override_, "true"}});
+    }
+  }
+
+  // As each test ends, clean up the singleton state.
+  void OnTestEnd(const ::testing::TestInfo&) override { runtime_state_.reset(); }
+
+  struct RuntimeState {
+    NiceMock<ThreadLocal::MockInstance> tls;
+    Stats::IsolatedStoreImpl store;
+    Runtime::MockRandomGenerator rand;
+    Runtime::ScopedLoaderSingleton loader{
+        Runtime::LoaderPtr{new Runtime::LoaderImpl(rand, store, tls)}};
+  };
+
+  std::unique_ptr<RuntimeState> runtime_state_;
+  std::string runtime_override_;
+};
+
 std::string makeTempDir(char* name_template) {
 #ifdef WIN32
   char* dirname = ::_mktemp(name_template);
@@ -129,6 +181,20 @@ std::string TestEnvironment::getCheckedEnvVar(const std::string& var) {
 }
 
 void TestEnvironment::initializeOptions(int argc, char** argv) {
+  // Before latching 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));
+  }
+
   argc_ = argc;
   argv_ = argv;
 }