envoyproxy · zuercher · Sep 28, 2021 · Sep 22, 2021 · Sep 23, 2021 · Sep 23, 2021
diff --git a/contrib/squash/filters/http/test/config_test.cc b/contrib/squash/filters/http/test/config_test.cc
@@ -36,11 +36,12 @@ TEST(SquashFilterConfigFactoryTest, SquashFilterCorrectYaml) {
   cb(filter_callback);
 }
 
-// Test that the deprecated extension name still functions.
+// Test that the deprecated extension name is disabled by default.
+// TODO(zuercher): remove when envoy.deprecated_features.allow_deprecated_extension_names is removed
 TEST(SquashFilterConfigTest, DEPRECATED_FEATURE_TEST(DeprecatedExtensionFilterName)) {
   const std::string deprecated_name = "envoy.squash";
 
-  ASSERT_NE(
+  ASSERT_EQ(
       nullptr,
       Registry::FactoryRegistry<Server::Configuration::NamedHttpFilterConfigFactory>::getFactory(
           deprecated_name));

diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -27,6 +27,9 @@ Incompatible Behavior Changes
   vendor the corresponding protobuf definitions to ensure that the
   renumbered fields have the types expected by those releases.
 * ext_authz: fixed skipping authentication when returning either a direct response or a redirect. This behavior can be temporarily reverted by setting the ``envoy.reloadable_features.http_ext_authz_do_not_skip_direct_response_and_redirect`` runtime guard to false.
+* extensions: deprecated extension names now default to triggering a configuration error.
+  The previous warning-only behavior may be temporarily reverted by setting the runtime key
+  ``envoy.deprecated_features.allow_deprecated_extension_names`` to true.
 
 Minor Behavior Changes
 ----------------------

diff --git a/examples/dynamic-config-fs/configs/lds.yaml b/examples/dynamic-config-fs/configs/lds.yaml
@@ -7,12 +7,12 @@ resources:
       port_value: 10000
   filter_chains:
   - filters:
-      name: envoy.http_connection_manager
+      name: envoy.filters.network.http_connection_manager
       typed_config:
         "@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
         stat_prefix: ingress_http
         http_filters:
-        - name: envoy.router
+        - name: envoy.filters.http.router
         route_config:
           name: local_route
           virtual_hosts:

diff --git a/source/common/config/utility.h b/source/common/config/utility.h
@@ -300,7 +300,7 @@ class Utility {
    * Get a Factory from the registry with error checking to ensure the name and the factory are
    * valid. And a flag to control return nullptr or throw an exception.
    * @param message proto that contains fields 'name' and 'typed_config'.
-   * @param is_optional an exception will be throw when the value is true and no factory found.
+   * @param is_optional an exception will be throw when the value is false and no factory found.
    * @return factory the factory requested or nullptr if it does not exist.
    */
   template <class Factory, class ProtoMessage>

diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc
@@ -55,7 +55,6 @@ constexpr const char* runtime_features[] = {
     // Enabled
     "envoy.reloadable_features.test_feature_true",
     // Begin alphabetically sorted section.
-    "envoy.deprecated_features.allow_deprecated_extension_names",
     "envoy.reloadable_features.add_and_validate_scheme_header",
     "envoy.reloadable_features.allow_response_for_timeout",
     "envoy.reloadable_features.check_unsupported_typed_per_filter_config",

diff --git a/source/extensions/common/utility.h b/source/extensions/common/utility.h
@@ -32,12 +32,9 @@ class ExtensionNameUtil {
     UNREFERENCED_PARAMETER(runtime);
     return Status::Block;
 #else
-    bool warn_only = true;
-
-    if (runtime && !runtime->snapshot().deprecatedFeatureEnabled(
-                       "envoy.deprecated_features.allow_deprecated_extension_names", true)) {
-      warn_only = false;
-    }
+    const bool warn_only =
+        runtime && runtime->snapshot().deprecatedFeatureEnabled(
+                       "envoy.deprecated_features.allow_deprecated_extension_names", false);
 
     return warn_only ? Status::Warn : Status::Block;
 #endif

diff --git a/test/common/access_log/access_log_impl_test.cc b/test/common/access_log/access_log_impl_test.cc
@@ -1592,42 +1592,37 @@ name: accesslog
   }
 }
 
-// Test that the deprecated extension names still function.
+// Test that the deprecated extension names are disabled by default.
+// TODO(zuercher): remove when envoy.deprecated_features.allow_deprecated_extension_names is removed
 TEST_F(AccessLogImplTest, DEPRECATED_FEATURE_TEST(DeprecatedExtensionFilterName)) {
-  {
-    envoy::config::accesslog::v3::AccessLog config;
-    config.set_name("envoy.access_loggers.file");
-
-    EXPECT_NO_THROW(
-        Config::Utility::getAndCheckFactory<Server::Configuration::AccessLogInstanceFactory>(
-            config));
-  }
-
   {
     envoy::config::accesslog::v3::AccessLog config;
     config.set_name("envoy.file_access_log");
 
-    EXPECT_NO_THROW(
+    EXPECT_THROW(
         Config::Utility::getAndCheckFactory<Server::Configuration::AccessLogInstanceFactory>(
-            config));
+            config),
+        EnvoyException);
   }
 
   {
     envoy::config::accesslog::v3::AccessLog config;
     config.set_name("envoy.http_grpc_access_log");
 
-    EXPECT_NO_THROW(
+    EXPECT_THROW(
         Config::Utility::getAndCheckFactory<Server::Configuration::AccessLogInstanceFactory>(
-            config));
+            config),
+        EnvoyException);
   }
 
   {
     envoy::config::accesslog::v3::AccessLog config;
     config.set_name("envoy.tcp_grpc_access_log");
 
-    EXPECT_NO_THROW(
+    EXPECT_THROW(
         Config::Utility::getAndCheckFactory<Server::Configuration::AccessLogInstanceFactory>(
-            config));
+            config),
+        EnvoyException);
   }
 }
 

diff --git a/test/common/config/registry_test.cc b/test/common/config/registry_test.cc
@@ -91,18 +91,10 @@ class TestWithDeprecatedPublishedFactory : public PublishedFactory {
 REGISTER_FACTORY(TestWithDeprecatedPublishedFactory,
                  PublishedFactory){"testing.published.deprecated_name"};
 
+// TODO(zuercher): remove when envoy.deprecated_features.allow_deprecated_extension_names is removed
 TEST(RegistryTest, DEPRECATED_FEATURE_TEST(WithDeprecatedFactoryPublished)) {
-  EXPECT_EQ("testing.published.instead_name",
-            Envoy::Registry::FactoryRegistry<PublishedFactory>::getFactory(
-                "testing.published.deprecated_name")
-                ->name());
-  EXPECT_LOG_CONTAINS("warn",
-                      fmt::format("Using deprecated extension name '{}' for '{}'.",
-                                  "testing.published.deprecated_name",
-                                  "testing.published.instead_name"),
-                      Envoy::Registry::FactoryRegistry<PublishedFactory>::getFactory(
-                          "testing.published.deprecated_name")
-                          ->name());
+  EXPECT_EQ(nullptr, Envoy::Registry::FactoryRegistry<PublishedFactory>::getFactory(
+                         "testing.published.deprecated_name"));
 }
 
 class NoNamePublishedFactory : public PublishedFactory {
@@ -161,17 +153,9 @@ REGISTER_FACTORY(TestVersionedWithDeprecatedNamesFactory,
 
 // Test registration of versioned factory that also uses deprecated names
 TEST(RegistryTest, DEPRECATED_FEATURE_TEST(VersionedWithDeprecatedNamesFactory)) {
-  EXPECT_EQ("testing.published.versioned.instead_name",
-            Envoy::Registry::FactoryRegistry<PublishedFactory>::getFactory(
-                "testing.published.versioned.deprecated_name")
-                ->name());
-  EXPECT_LOG_CONTAINS("warn",
-                      fmt::format("Using deprecated extension name '{}' for '{}'.",
-                                  "testing.published.versioned.deprecated_name",
-                                  "testing.published.versioned.instead_name"),
-                      Envoy::Registry::FactoryRegistry<PublishedFactory>::getFactory(
-                          "testing.published.versioned.deprecated_name")
-                          ->name());
+  EXPECT_EQ(nullptr, Envoy::Registry::FactoryRegistry<PublishedFactory>::getFactory(
+                         "testing.published.versioned.deprecated_name"));
+
   const auto& factories = Envoy::Registry::FactoryCategoryRegistry::registeredFactories();
   auto version = factories.find("testing.published")
                      ->second->getFactoryVersion("testing.published.versioned.instead_name");

diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc
@@ -5676,7 +5676,8 @@ TEST_F(RouteMatcherTest, TestOpaqueConfig) {
   EXPECT_EQ(opaque_config.find("name2")->second, "value2");
 }
 
-// Test that the deprecated name works for opaque configs.
+// Test that the deprecated name no longer works by default for opaque configs.
+// TODO(zuercher): remove when envoy.deprecated_features.allow_deprecated_extension_names is removed
 TEST_F(RouteMatcherTest, DEPRECATED_FEATURE_TEST(TestOpaqueConfigUsingDeprecatedName)) {
   const std::string yaml = R"EOF(
 virtual_hosts:
@@ -5696,13 +5697,8 @@ TEST_F(RouteMatcherTest, DEPRECATED_FEATURE_TEST(TestOpaqueConfigUsingDeprecated
 )EOF";
 
   factory_context_.cluster_manager_.initializeClusters({"ats"}, {});
-  TestConfigImpl config(parseRouteConfigurationFromYaml(yaml), factory_context_, true);
-
-  const std::multimap<std::string, std::string>& opaque_config =
-      config.route(genHeaders("api.lyft.com", "/api", "GET"), 0)->routeEntry()->opaqueConfig();
-
-  EXPECT_EQ(opaque_config.find("name1")->second, "value1");
-  EXPECT_EQ(opaque_config.find("name2")->second, "value2");
+  EXPECT_THROW(TestConfigImpl(parseRouteConfigurationFromYaml(yaml), factory_context_, true),
+               EnvoyException);
 }
 
 class RoutePropertyTest : public testing::Test, public ConfigImplTestBase {};

diff --git a/test/extensions/common/utility_test.cc b/test/extensions/common/utility_test.cc
@@ -19,9 +19,9 @@ namespace {
 
 // Test that deprecated names indicate warning or block depending on runtime flags.
 TEST(ExtensionNameUtilTest, DEPRECATED_FEATURE_TEST(TestDeprecatedExtensionNameStatus)) {
-  // Validate that no runtime available results in warnings.
+  // Validate that no runtime available results in block.
   {
-    EXPECT_EQ(ExtensionNameUtil::Status::Warn,
+    EXPECT_EQ(ExtensionNameUtil::Status::Block,
               ExtensionNameUtil::deprecatedExtensionNameStatus(nullptr));
   }
 
@@ -54,16 +54,11 @@ TEST(ExtensionNameUtilTest, DEPRECATED_FEATURE_TEST(TestDeprecatedExtensionNameS
 
 // Test that deprecated names trigger an exception.
 TEST(ExtensionNameUtilTest, DEPRECATED_FEATURE_TEST(TestCheckDeprecatedExtensionNameThrows)) {
-  // Validate that no runtime available results in warnings.
+  // Validate that no runtime available results in exception.
   {
-    auto test = []() {
-      ExtensionNameUtil::checkDeprecatedExtensionName("XXX", "deprecated", "canonical", nullptr);
-    };
-
-    EXPECT_NO_THROW(test());
-
-    EXPECT_LOG_CONTAINS("warn", "Using deprecated XXX extension name 'deprecated' for 'canonical'.",
-                        test());
+    EXPECT_THROW_WITH_REGEX(
+        ExtensionNameUtil::checkDeprecatedExtensionName("XXX", "deprecated", "canonical", nullptr),
+        EnvoyException, "Using deprecated XXX extension name 'deprecated' for 'canonical'.*");
   }
 
   // If deprecated feature is enabled, warn.
@@ -101,16 +96,15 @@ TEST(ExtensionNameUtilTest, DEPRECATED_FEATURE_TEST(TestCheckDeprecatedExtension
 
 // Test that deprecated names are reported as allowed or not, with logging.
 TEST(ExtensionNameUtilTest, DEPRECATED_FEATURE_TEST(TestAllowDeprecatedExtensionName)) {
-  // Validate that no runtime available results in warnings and allows deprecated names.
+  // Validate that no runtime available results in a log message and returns false.
   {
     auto test = []() {
       return ExtensionNameUtil::allowDeprecatedExtensionName("XXX", "deprecated", "canonical",
                                                              nullptr);
     };
-    EXPECT_TRUE(test());
+    EXPECT_FALSE(test());
 
-    EXPECT_LOG_CONTAINS("warn", "Using deprecated XXX extension name 'deprecated' for 'canonical'.",
-                        test());
+    EXPECT_LOG_CONTAINS("error", "#using-runtime-overrides-for-deprecated-features", test());
   }
 
   // If deprecated feature is enabled, log and return true.

@@ -100,11 +100,12 @@ TEST(BufferFilterFactoryTest, BufferFilterRouteSpecificConfig) {
   EXPECT_TRUE(inflated);
 }
 
-// Test that the deprecated extension name still functions.
+// Test that the deprecated extension name is disabled by default.
+// TODO(zuercher): remove when envoy.deprecated_features.allow_deprecated_extension_names is removed
 TEST(BufferFilterFactoryTest, DEPRECATED_FEATURE_TEST(DeprecatedExtensionFilterName)) {
   const std::string deprecated_name = "envoy.buffer";
 
-  ASSERT_NE(
+  ASSERT_EQ(
       nullptr,
       Registry::FactoryRegistry<Server::Configuration::NamedHttpFilterConfigFactory>::getFactory(
           deprecated_name));

@@ -705,11 +705,12 @@ TEST_F(CorsFilterTest, OptionsRequestNotMatchingOriginByRegex) {
   EXPECT_EQ(Http::FilterTrailersStatus::Continue, filter_.encodeTrailers(response_trailers_));
 }
 
-// Test that the deprecated extension name still functions.
+// Test that the deprecated extension name is disabled by default.
+// TODO(zuercher): remove when envoy.deprecated_features.allow_deprecated_extension_names is removed
 TEST(CorsFilterConfigTest, DEPRECATED_FEATURE_TEST(DeprecatedExtensionFilterName)) {
   const std::string deprecated_name = "envoy.cors";
 
-  ASSERT_NE(
+  ASSERT_EQ(
       nullptr,
       Registry::FactoryRegistry<Server::Configuration::NamedHttpFilterConfigFactory>::getFactory(
           deprecated_name));

@@ -450,11 +450,12 @@ TEST_F(CsrfFilterTest, RequestFromInvalidAdditionalRegexOrigin) {
   EXPECT_EQ(0U, config_->stats().request_valid_.value());
 }
 
-// Test that the deprecated extension name still functions.
+// Test that the deprecated extension name is disabled by default.
+// TODO(zuercher): remove when envoy.deprecated_features.allow_deprecated_extension_names is removed
 TEST(CsrfFilterConfigTest, DEPRECATED_FEATURE_TEST(DeprecatedExtensionFilterName)) {
   const std::string deprecated_name = "envoy.csrf";
 
-  ASSERT_NE(
+  ASSERT_EQ(
       nullptr,
       Registry::FactoryRegistry<Server::Configuration::NamedHttpFilterConfigFactory>::getFactory(
           deprecated_name));

diff --git a/test/extensions/filters/http/dynamo/config_test.cc b/test/extensions/filters/http/dynamo/config_test.cc
@@ -26,11 +26,12 @@ TEST(DynamoFilterConfigTest, DynamoFilter) {
   cb(filter_callback);
 }
 
-// Test that the deprecated extension name still functions.
+// Test that the deprecated extension name is disabled by default.
+// TODO(zuercher): remove when envoy.deprecated_features.allow_deprecated_extension_names is removed
 TEST(DynamoFilterConfigTest, DEPRECATED_FEATURE_TEST(DeprecatedExtensionFilterName)) {
   const std::string deprecated_name = "envoy.http_dynamo_filter";
 
-  ASSERT_NE(
+  ASSERT_EQ(
       nullptr,
       Registry::FactoryRegistry<Server::Configuration::NamedHttpFilterConfigFactory>::getFactory(
           deprecated_name));

@@ -150,11 +150,12 @@ TEST(HttpExtAuthzConfigTest, CorrectProtoHttp) {
   cb(filter_callback);
 }
 
-// Test that the deprecated extension name still functions.
+// Test that the deprecated extension name is disabled by default.
+// TODO(zuercher): remove when envoy.deprecated_features.allow_deprecated_extension_names is removed
 TEST(HttpExtAuthzConfigTest, DEPRECATED_FEATURE_TEST(DeprecatedExtensionFilterName)) {
   const std::string deprecated_name = "envoy.ext_authz";
 
-  ASSERT_NE(
+  ASSERT_EQ(
       nullptr,
       Registry::FactoryRegistry<Server::Configuration::NamedHttpFilterConfigFactory>::getFactory(
           deprecated_name));

@@ -69,11 +69,12 @@ TEST(FaultFilterConfigTest, FaultFilterEmptyProto) {
   cb(filter_callback);
 }
 
-// Test that the deprecated extension name still functions.
+// Test that the deprecated extension name is disabled by default.
+// TODO(zuercher): remove when envoy.deprecated_features.allow_deprecated_extension_names is removed
 TEST(FaultFilterConfigTest, DEPRECATED_FEATURE_TEST(DeprecatedExtensionFilterName)) {
   const std::string deprecated_name = "envoy.fault";
 
-  ASSERT_NE(
+  ASSERT_EQ(
       nullptr,
       Registry::FactoryRegistry<Server::Configuration::NamedHttpFilterConfigFactory>::getFactory(
           deprecated_name));

@@ -23,11 +23,12 @@ TEST(GrpcHttp1BridgeFilterConfigTest, GrpcHttp1BridgeFilter) {
   cb(filter_callback);
 }
 
-// Test that the deprecated extension name still functions.
+// Test that the deprecated extension name is disabled by default.
+// TODO(zuercher): remove when envoy.deprecated_features.allow_deprecated_extension_names is removed
 TEST(GrpcHttp1BridgeFilterConfigTest, DEPRECATED_FEATURE_TEST(DeprecatedExtensionFilterName)) {
   const std::string deprecated_name = "envoy.grpc_http1_bridge";
 
-  ASSERT_NE(
+  ASSERT_EQ(
       nullptr,
       Registry::FactoryRegistry<Server::Configuration::NamedHttpFilterConfigFactory>::getFactory(
           deprecated_name));

@@ -22,11 +22,12 @@ TEST(GrpcJsonTranscoderFilterConfigTest, ValidateFail) {
                ProtoValidationException);
 }
 
-// Test that the deprecated extension name still functions.
+// Test that the deprecated extension name is disabled by default.
+// TODO(zuercher): remove when envoy.deprecated_features.allow_deprecated_extension_names is removed
 TEST(GrpcJsonTranscoderFilterConfigTest, DEPRECATED_FEATURE_TEST(DeprecatedExtensionFilterName)) {
   const std::string deprecated_name = "envoy.grpc_json_transcoder";
 
-  ASSERT_NE(
+  ASSERT_EQ(
       nullptr,
       Registry::FactoryRegistry<Server::Configuration::NamedHttpFilterConfigFactory>::getFactory(
           deprecated_name));

@@ -23,11 +23,12 @@ TEST(GrpcWebFilterConfigTest, GrpcWebFilter) {
   cb(filter_callback);
 }
 
-// Test that the deprecated extension name still functions.
+// Test that the deprecated extension name is disabled by default.
+// TODO(zuercher): remove when envoy.deprecated_features.allow_deprecated_extension_names is removed
 TEST(GrpcWebFilterConfigTest, DEPRECATED_FEATURE_TEST(DeprecatedExtensionFilterName)) {
   const std::string deprecated_name = "envoy.grpc_web";
 
-  ASSERT_NE(
+  ASSERT_EQ(
       nullptr,
       Registry::FactoryRegistry<Server::Configuration::NamedHttpFilterConfigFactory>::getFactory(
           deprecated_name));

@@ -270,11 +270,12 @@ TEST(HealthCheckFilterConfig, HealthCheckFilterDuplicateNoMatch) {
   testHealthCheckHeaderMatch(config, headers, false);
 }
 
-// Test that the deprecated extension name still functions.
+// Test that the deprecated extension name is disabled by default.
+// TODO(zuercher): remove when envoy.deprecated_features.allow_deprecated_extension_names is removed
 TEST(HealthCheckFilterConfig, DEPRECATED_FEATURE_TEST(DeprecatedExtensionFilterName)) {
   const std::string deprecated_name = "envoy.health_check";
 
-  ASSERT_NE(
+  ASSERT_EQ(
       nullptr,
       Registry::FactoryRegistry<Server::Configuration::NamedHttpFilterConfigFactory>::getFactory(
           deprecated_name));