build: adding an option to hard-fail when deprecated config is used. (#…

…7962)

Adding a build option to default all deprecated protos off, and using it on the debug build.

Risk Level: Low
Testing: new UT
Docs Changes: inline
Release Notes: n/a
Fixes #7548

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

CONTRIBUTING.md

@@ -72,7 +72,11 @@ maximize the chances of your PR being merged.
   could convert from the earlier API to the new API. A field may be deprecated
   if this tool would be able to perform the conversion. For example, removing a
   field to describe HTTP/2 window settings is valid if a more comprehensive
-  HTTP/2 protocol options field is being introduced to replace it.
+  HTTP/2 protocol options field is being introduced to replace it. The PR author
+  deprecating the old configuration is responsible for updating all tests and
+  canonical configuration, or guarding them with the DEPRECATED_FEATURE_TEST() macro.
+  This will be validated by the bazel.compile_time_options target, which will hard-fail when
+  deprecated configuration is used.
 * For configuration deprecations that are not covered by the above semantic
   replacement policy, any deprecation will only take place after
   community consultation on mailing lists, Slack and GitHub, over the period of

bazel/BUILD

@@ -120,6 +120,11 @@ config_setting(
     values = {"define": "object_dump_on_signal_trace=disabled"},
 )
 
+config_setting(
+    name = "disable_deprecated_features",
+    values = {"define": "deprecated_features=disabled"},
+)
+
 config_setting(
     name = "disable_hot_restart",
     values = {"define": "hot_restart=disabled"},

bazel/README.md

@@ -407,6 +407,8 @@ The following optional features can be disabled on the Bazel build command-line:
 * Backtracing on signals with `--define signal_trace=disabled`
 * Active stream state dump on signals with `--define signal_trace=disabled` or `--define disable_object_dump_on_signal_trace=disabled`
 * tcmalloc with `--define tcmalloc=disabled`
+* deprecated features with `--define deprecated_features=disabled`
+
 
 ## Enabling optional features
 

bazel/envoy_internal.bzl

@@ -56,6 +56,9 @@ def envoy_copts(repository, test = False):
            }) + select({
                repository + "//bazel:disable_object_dump_on_signal_trace": [],
                "//conditions:default": ["-DENVOY_OBJECT_TRACE_ON_DUMP"],
+           }) + select({
+               repository + "//bazel:disable_deprecated_features": ["-DENVOY_DISABLE_DEPRECATED_FEATURES"],
+               "//conditions:default": [],
            }) + select({
                repository + "//bazel:enable_log_debug_assert_in_release": ["-DENVOY_LOG_DEBUG_ASSERT_IN_RELEASE"],
                "//conditions:default": [],

ci/do_ci.sh

@@ -197,6 +197,7 @@ elif [[ "$CI_TARGET" == "bazel.compile_time_options" ]]; then
     --define log_debug_assert_in_release=enabled \
     --define quiche=enabled \
     --define path_normalization_by_default=true \
+    --define deprecated_features=disabled \
   "
   setup_clang_libcxx_toolchain
   # This doesn't go into CI but is available for developer convenience.

source/common/runtime/runtime_impl.cc

@@ -182,9 +182,14 @@ bool SnapshotImpl::deprecatedFeatureEnabled(const std::string& key) const {
     // If either disallowed by default or configured off, the feature is not enabled.
     return false;
   }
+
   // The feature is allowed. It is assumed this check is called when the feature
   // is about to be used, so increment the feature use stat.
   stats_.deprecated_feature_use_.inc();
+#ifdef ENVOY_DISABLE_DEPRECATED_FEATURES
+  return false;
+#endif
+
   return true;
 }
 

test/common/protobuf/utility_test.cc

@@ -511,7 +511,7 @@ TEST_F(DeprecatedFieldsTest, NoErrorWhenDeprecatedFieldsUnused) {
   EXPECT_EQ(0, runtime_deprecated_feature_use_.value());
 }
 
-TEST_F(DeprecatedFieldsTest, IndividualFieldDeprecated) {
+TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(IndividualFieldDeprecated)) {
   envoy::test::deprecation_test::Base base;
   base.set_is_deprecated("foo");
   // Non-fatal checks for a deprecated field should log rather than throw an exception.
@@ -522,15 +522,16 @@ TEST_F(DeprecatedFieldsTest, IndividualFieldDeprecated) {
 }
 
 // Use of a deprecated and disallowed field should result in an exception.
-TEST_F(DeprecatedFieldsTest, IndividualFieldDisallowed) {
+TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(IndividualFieldDisallowed)) {
   envoy::test::deprecation_test::Base base;
   base.set_is_deprecated_fatal("foo");
   EXPECT_THROW_WITH_REGEX(
       MessageUtil::checkForDeprecation(base), ProtoValidationException,
       "Using deprecated option 'envoy.test.deprecation_test.Base.is_deprecated_fatal'");
 }
 
-TEST_F(DeprecatedFieldsTest, IndividualFieldDisallowedWithRuntimeOverride) {
+TEST_F(DeprecatedFieldsTest,
+       DEPRECATED_FEATURE_TEST(IndividualFieldDisallowedWithRuntimeOverride)) {
   envoy::test::deprecation_test::Base base;
   base.set_is_deprecated_fatal("foo");
 
@@ -552,7 +553,7 @@ TEST_F(DeprecatedFieldsTest, IndividualFieldDisallowedWithRuntimeOverride) {
   EXPECT_EQ(1, runtime_deprecated_feature_use_.value());
 }
 
-TEST_F(DeprecatedFieldsTest, DisallowViaRuntime) {
+TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(DisallowViaRuntime)) {
   envoy::test::deprecation_test::Base base;
   base.set_is_deprecated("foo");
 
@@ -574,7 +575,7 @@ TEST_F(DeprecatedFieldsTest, DisallowViaRuntime) {
 // Note that given how Envoy config parsing works, the first time we hit a
 // 'fatal' error and throw, we won't log future warnings. That said, this tests
 // the case of the warning occurring before the fatal error.
-TEST_F(DeprecatedFieldsTest, MixOfFatalAndWarnings) {
+TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(MixOfFatalAndWarnings)) {
   envoy::test::deprecation_test::Base base;
   base.set_is_deprecated("foo");
   base.set_is_deprecated_fatal("foo");
@@ -587,7 +588,7 @@ TEST_F(DeprecatedFieldsTest, MixOfFatalAndWarnings) {
 }
 
 // Present (unused) deprecated messages should be detected as deprecated.
-TEST_F(DeprecatedFieldsTest, MessageDeprecated) {
+TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(MessageDeprecated)) {
   envoy::test::deprecation_test::Base base;
   base.mutable_deprecated_message();
   EXPECT_LOG_CONTAINS(
@@ -596,7 +597,7 @@ TEST_F(DeprecatedFieldsTest, MessageDeprecated) {
   EXPECT_EQ(1, runtime_deprecated_feature_use_.value());
 }
 
-TEST_F(DeprecatedFieldsTest, InnerMessageDeprecated) {
+TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(InnerMessageDeprecated)) {
   envoy::test::deprecation_test::Base base;
   base.mutable_not_deprecated_message()->set_inner_not_deprecated("foo");
   // Checks for a non-deprecated field shouldn't trigger warnings
@@ -612,7 +613,7 @@ TEST_F(DeprecatedFieldsTest, InnerMessageDeprecated) {
 }
 
 // Check that repeated sub-messages get validated.
-TEST_F(DeprecatedFieldsTest, SubMessageDeprecated) {
+TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(SubMessageDeprecated)) {
   envoy::test::deprecation_test::Base base;
   base.add_repeated_message();
   base.add_repeated_message()->set_inner_deprecated("foo");
@@ -626,7 +627,7 @@ TEST_F(DeprecatedFieldsTest, SubMessageDeprecated) {
 }
 
 // Check that deprecated repeated messages trigger
-TEST_F(DeprecatedFieldsTest, RepeatedMessageDeprecated) {
+TEST_F(DeprecatedFieldsTest, DEPRECATED_FEATURE_TEST(RepeatedMessageDeprecated)) {
   envoy::test::deprecation_test::Base base;
   base.add_deprecated_repeated_message();
 

test/common/runtime/runtime_impl_test.cc

@@ -179,6 +179,15 @@ TEST_F(DiskLoaderImplTest, All) {
   // 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"));
 
+  // Deprecation
+#ifdef ENVOY_DISABLE_DEPRECATED_FEATURES
+  EXPECT_EQ(false, snapshot->deprecatedFeatureEnabled("random_string_should_be_enabled"));
+#else
+  EXPECT_EQ(true, snapshot->deprecatedFeatureEnabled("random_string_should_be_enabled"));
+#endif
+  EXPECT_EQ(false, snapshot->deprecatedFeatureEnabled(
+                       "envoy.deprecated_features.deprecated.proto:is_deprecated_fatal"));
+
   // 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"));

test/extensions/clusters/redis/redis_cluster_integration_test.cc

@@ -36,7 +36,9 @@ const std::string& testConfig() {
         name: envoy.redis_proxy
         config:
           stat_prefix: redis_stats
-          cluster: cluster_0
+          prefix_routes:
+            catch_all_route:
+              cluster: cluster_0
           settings:
             op_timeout: 5s
   clusters:

test/extensions/filters/http/cors/cors_filter_integration_test.cc

@@ -34,7 +34,11 @@ class CorsFilterIntegrationTest : public testing::TestWithParam<Network::Address
             auto* route = virtual_host->add_routes();
             route->mutable_match()->set_prefix("/no-cors");
             route->mutable_route()->set_cluster("cluster_0");
-            route->mutable_route()->mutable_cors()->mutable_enabled()->set_value(false);
+            route->mutable_route()
+                ->mutable_cors()
+                ->mutable_filter_enabled()
+                ->mutable_default_value()
+                ->set_numerator(0);
           }
 
           {

test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc

@@ -56,7 +56,9 @@ const std::string CONFIG = R"EOF(
         name: envoy.redis_proxy
         config:
           stat_prefix: redis_stats
-          cluster: cluster_0
+          prefix_routes:
+            catch_all_route:
+              cluster: cluster_0
           settings:
             op_timeout: 5s
 )EOF";
@@ -149,7 +151,8 @@ const std::string CONFIG_WITH_ROUTES_BASE = R"EOF(
 
 const std::string CONFIG_WITH_ROUTES = CONFIG_WITH_ROUTES_BASE + R"EOF(
           prefix_routes:
-            catch_all_cluster: cluster_0
+            catch_all_route:
+              cluster: cluster_0
             routes:
             - prefix: "foo:"
               cluster: cluster_1
@@ -250,7 +253,8 @@ const std::string CONFIG_WITH_ROUTES_AND_AUTH_PASSWORDS = R"EOF(
           settings:
             op_timeout: 5s
           prefix_routes:
-            catch_all_cluster: cluster_0
+            catch_all_route:
+              cluster: cluster_0
             routes:
             - prefix: "foo:"
               cluster: cluster_1

test/test_common/utility.h

@@ -105,6 +105,19 @@ namespace Envoy {
     }                                                                                              \
   } while (false)
 
+// A convenience macro for testing Envoy deprecated features. This will disable the test when
+// tests are built with --define deprecated_features=disabled to avoid the hard-failure mode for
+// deprecated features. Sample usage is:
+//
+// TEST_F(FixtureName, DEPRECATED_FEATURE_TEST(TestName)) {
+// ...
+// }
+#ifndef ENVOY_DISABLE_DEPRECATED_FEATURES
+#define DEPRECATED_FEATURE_TEST(X) X
+#else
+#define DEPRECATED_FEATURE_TEST(X) DISABLED_##X
+#endif
+
 // Random number generator which logs its seed to stderr. To repeat a test run with a non-zero seed
 // one can run the test with --test_arg=--gtest_random_seed=[seed]
 class TestRandomGenerator {