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 <alyssar@chromium.org>

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
 

docs/root/configuration/runtime.rst

@@ -93,7 +93,7 @@ Users are encouraged to go to :repo:`DEPRECATED.md <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 <source/common/runtime/runtime_features.h>`
+:repo:`runtime_features.cc <source/common/runtime/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

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

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

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",

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

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

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

source/common/runtime/runtime_impl.h

@@ -24,6 +24,8 @@
 namespace Envoy {
 namespace Runtime {
 
+bool runtimeFeatureEnabled(const std::string& feature);
+
 using RuntimeSingleton = ThreadSafeSingleton<Loader>;
 
 /**
@@ -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;

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",

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"],

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

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