http filters: fix filter_metadata when used with router and lua filters (envoyproxy#10285)

zuercher · web-flow · commit 841ad99de494 · 2020-03-06T16:00:13.000-08:00
The HTTP router filter and HTTP LUA filter look up metadata attached to
route entries using their well-known-filter names, which recently changed.
Extend deprecated filter name warnings to these cases so that existing
routes may continue to use the deprecated names.

Risk Level: low
Testing: add unit tests
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Stephan Zuercher &lt;zuercher@gmail.com&gt;
diff --git a/include/envoy/registry/registry.h b/include/envoy/registry/registry.h
@@ -337,13 +337,8 @@ template <class Base> class FactoryRegistry : public Logger::Loggable<Logger::Id
     auto it = deprecatedFactoryNames().find(name);
     const bool deprecated = it != deprecatedFactoryNames().end();
     if (deprecated) {
-      auto status = Extensions::Common::Utility::ExtensionNameUtil::deprecatedExtensionNameStatus();
-      if (status == Extensions::Common::Utility::ExtensionNameUtil::Status::Block) {
-        ENVOY_LOG(error, "{} is deprecated and disabled, use {} instead.", it->first, it->second);
-        return false;
-      }
-
-      ENVOY_LOG(warn, "{} is deprecated, use {} instead.", it->first, it->second);
+      return Extensions::Common::Utility::ExtensionNameUtil::allowDeprecatedExtensionName(
+          "", it->first, it->second);
     }
 
     return true;
diff --git a/source/common/router/config_impl.cc b/source/common/router/config_impl.cc
@@ -57,6 +57,8 @@ convertInternalRedirectAction(const envoy::config::route::v3::RouteAction& route
   }
 }
 
+const std::string DEPRECATED_ROUTER_NAME = "envoy.router";
+
 } // namespace
 
 std::string SslRedirector::newPath(const Http::RequestHeaderMap& headers) const {
@@ -647,10 +649,18 @@ std::multimap<std::string, std::string>
 RouteEntryImplBase::parseOpaqueConfig(const envoy::config::route::v3::Route& route) {
   std::multimap<std::string, std::string> ret;
   if (route.has_metadata()) {
-    const auto filter_metadata = route.metadata().filter_metadata().find(
+    auto filter_metadata = route.metadata().filter_metadata().find(
         Extensions::HttpFilters::HttpFilterNames::get().Router);
     if (filter_metadata == route.metadata().filter_metadata().end()) {
-      return ret;
+      // TODO(zuercher): simply return `ret` when deprecated filter names are removed.
+      filter_metadata = route.metadata().filter_metadata().find(DEPRECATED_ROUTER_NAME);
+      if (filter_metadata == route.metadata().filter_metadata().end()) {
+        return ret;
+      }
+
+      Extensions::Common::Utility::ExtensionNameUtil::checkDeprecatedExtensionName(
+          "http filter", DEPRECATED_ROUTER_NAME,
+          Extensions::HttpFilters::HttpFilterNames::get().Router);
     }
     for (const auto& it : filter_metadata->second.fields()) {
       if (it.second.kind_case() == ProtobufWkt::Value::kStringValue) {
diff --git a/source/extensions/common/utility.h b/source/extensions/common/utility.h
@@ -20,13 +20,14 @@ class ExtensionNameUtil {
   enum class Status { Warn, Block };
 
   /**
-   * Returns the status of deprecated extension names.
+   * Checks the status of deprecated extension names and increments the deprecated feature stats
+   * counter if deprecated names are allowed.
    *
-   * @return Status indicating whether deprecated names trigger warnings or are blocked.
+   * @param runtime Runtime::Loader used to determine if deprecated extension names are allowed.
+   * @return Status::Warn (allowed, warn) or Status::Block (disallowed, error)
    */
   static Status deprecatedExtensionNameStatus(
       Runtime::Loader* runtime = Runtime::LoaderSingleton::getExisting()) {
-
 #ifdef ENVOY_DISABLE_DEPRECATED_FEATURES
     UNREFERENCED_PARAMETER(runtime);
     return Status::Block;
@@ -43,9 +44,42 @@ class ExtensionNameUtil {
   }
 
   /**
-   * Checks the status of deprecated extension and either generates a log message or throws.
-   * The passed strings are used only to generate the log or exception message.
+   * Checks the status of deprecated extension names. If deprecated extension names are allowed,
+   * it increments the deprecated feature stats counter. Generates a warning or error log message
+   * based on whether the name is allowed (warning) or not (error). The string parameters are used
+   * only to generate the log message.
+   *
+   * @param extension_type absl::string_view that contains the extension type, for logging
+   * @param deprecated_name absl::string_view that contains the deprecated name, for logging
+   * @param canonical_name absl::string_view that contains the canonical name, for logging
+   * @param runtime Runtime::Loader used to determine if deprecated extension names are allowed.
+   * @return true if deprecated extensions are allowed, false otherwise.
+   */
+  static bool
+  allowDeprecatedExtensionName(absl::string_view extension_type, absl::string_view deprecated_name,
+                               absl::string_view canonical_name,
+                               Runtime::Loader* runtime = Runtime::LoaderSingleton::getExisting()) {
+    auto status = deprecatedExtensionNameStatus(runtime);
+
+    if (status == Status::Warn) {
+      ENVOY_LOG_MISC(warn, "{}", message(extension_type, deprecated_name, canonical_name));
+      return true;
+    }
+
+    ENVOY_LOG_MISC(error, "{}", fatalMessage(extension_type, deprecated_name, canonical_name));
+    return false;
+  }
+
+  /**
+   * Checks the status of deprecated extension names. If deprecated extension names are allowed,
+   * it increments the deprecated feature stats counter and generates a log message. If not allowed,
+   * an exception is thrown. The passed strings are used only to generate the log or exception
+   * message.
    *
+   * @param extension_type absl::string_view that contains the extension type, for logging
+   * @param deprecated_name absl::string_view that contains the deprecated name, for logging
+   * @param canonical_name absl::string_view that contains the canonical name, for logging
+   * @param runtime Runtime::Loader used to determine if deprecated extension names are allowed.
    * @throw EnvoyException if the use of deprecated extension names is not allowed.
    */
   static void
@@ -54,24 +88,38 @@ class ExtensionNameUtil {
                                Runtime::Loader* runtime = Runtime::LoaderSingleton::getExisting()) {
     auto status = deprecatedExtensionNameStatus(runtime);
 
-    std::string err = fmt::format(
-        "Using deprecated {} extension name '{}' for '{}'. This name will be removed from Envoy "
-        "soon. Please see "
-        "https://www.envoyproxy.io/docs/envoy/latest/intro/deprecated for details.",
-        extension_type, deprecated_name, canonical_name);
-
     if (status == Status::Warn) {
-      ENVOY_LOG_MISC(warn, "{}", err);
+      ENVOY_LOG_MISC(warn, "{}", message(extension_type, deprecated_name, canonical_name));
       return;
     }
 
+    throw EnvoyException(fatalMessage(extension_type, deprecated_name, canonical_name));
+  }
+
+private:
+  static std::string message(absl::string_view extension_type, absl::string_view deprecated_name,
+                             absl::string_view canonical_name) {
+    absl::string_view spacing = extension_type.empty() ? "" : " ";
+
+    return fmt::format(
+        "Using deprecated {}{}extension name '{}' for '{}'. This name will be removed from Envoy "
+        "soon. Please see "
+        "https://www.envoyproxy.io/docs/envoy/latest/intro/deprecated for details.",
+        extension_type, spacing, deprecated_name, canonical_name);
+  }
+
+  static std::string fatalMessage(absl::string_view extension_type,
+                                  absl::string_view deprecated_name,
+                                  absl::string_view canonical_name) {
+    std::string err = message(extension_type, deprecated_name, canonical_name);
+
     const char fatal_error[] =
         " If continued use of this filter name is absolutely necessary, see "
         "https://www.envoyproxy.io/docs/envoy/latest/configuration/operations/runtime"
         "#using-runtime-overrides-for-deprecated-features for how to apply a temporary and "
         "highly discouraged override.";
 
-    throw EnvoyException(err + fatal_error);
+    return err + fatal_error;
   }
 };
 
diff --git a/source/extensions/filters/http/lua/BUILD b/source/extensions/filters/http/lua/BUILD
@@ -25,6 +25,7 @@ envoy_cc_library(
         "//source/common/common:enum_to_int",
         "//source/common/crypto:utility_lib",
         "//source/common/http:message_lib",
+        "//source/extensions/common:utility_lib",
         "//source/extensions/filters/common/lua:lua_lib",
         "//source/extensions/filters/common/lua:wrappers_lib",
         "//source/extensions/filters/http:well_known_names",
diff --git a/source/extensions/filters/http/lua/lua_filter.cc b/source/extensions/filters/http/lua/lua_filter.cc
@@ -1,5 +1,6 @@
 #include "extensions/filters/http/lua/lua_filter.h"
 
+#include <atomic>
 #include <memory>
 
 #include "envoy/http/codes.h"
@@ -16,6 +17,57 @@ namespace HttpFilters {
 namespace Lua {
 
 namespace {
+
+const std::string DEPRECATED_LUA_NAME = "envoy.lua";
+
+std::atomic<bool>& deprecatedNameLogged() {
+  MUTABLE_CONSTRUCT_ON_FIRST_USE(std::atomic<bool>, false);
+}
+
+// Checks if deprecated metadata names are allowed. On the first check only it will log either
+// a warning (indicating the name should be updated) or an error (the feature is off and the
+// name is not allowed). When warning, the deprecated feature stat is incremented. Subsequent
+// checks do not log since this check is done in potentially high-volume request paths.
+bool allowDeprecatedMetadataName() {
+  if (!deprecatedNameLogged().exchange(true)) {
+    // Have not logged yet, so use the logging test.
+    return Extensions::Common::Utility::ExtensionNameUtil::allowDeprecatedExtensionName(
+        "http filter", DEPRECATED_LUA_NAME, Extensions::HttpFilters::HttpFilterNames::get().Lua);
+  }
+
+  // We have logged (or another thread will do so momentarily), so just check whether the
+  // deprecated name is allowed.
+  auto status = Extensions::Common::Utility::ExtensionNameUtil::deprecatedExtensionNameStatus();
+  return status == Extensions::Common::Utility::ExtensionNameUtil::Status::Warn;
+}
+
+const ProtobufWkt::Struct& getMetadata(Http::StreamFilterCallbacks* callbacks) {
+  if (callbacks->route() == nullptr || callbacks->route()->routeEntry() == nullptr) {
+    return ProtobufWkt::Struct::default_instance();
+  }
+  const auto& metadata = callbacks->route()->routeEntry()->metadata();
+
+  {
+    const auto& filter_it = metadata.filter_metadata().find(HttpFilterNames::get().Lua);
+    if (filter_it != metadata.filter_metadata().end()) {
+      return filter_it->second;
+    }
+  }
+
+  // TODO(zuercher): Remove this block when deprecated filter names are removed.
+  {
+    const auto& filter_it = metadata.filter_metadata().find(DEPRECATED_LUA_NAME);
+    if (filter_it != metadata.filter_metadata().end()) {
+      // Use the non-throwing check here because this happens at request time.
+      if (allowDeprecatedMetadataName()) {
+        return filter_it->second;
+      }
+    }
+  }
+
+  return ProtobufWkt::Struct::default_instance();
+}
+
 // Okay to return non-const reference because this doesn't ever get changed.
 NoopCallbacks& noopCallbacks() {
   static NoopCallbacks* callbacks = new NoopCallbacks();
@@ -646,13 +698,21 @@ void Filter::DecoderCallbacks::respond(Http::ResponseHeaderMapPtr&& headers, Buf
   }
 }
 
+const ProtobufWkt::Struct& Filter::DecoderCallbacks::metadata() const {
+  return getMetadata(callbacks_);
+}
+
 void Filter::EncoderCallbacks::respond(Http::ResponseHeaderMapPtr&&, Buffer::Instance*,
                                        lua_State* state) {
   // TODO(mattklein123): Support response in response path if nothing has been continued
   // yet.
   luaL_error(state, "respond not currently supported in the response path");
 }
 
+const ProtobufWkt::Struct& Filter::EncoderCallbacks::metadata() const {
+  return getMetadata(callbacks_);
+}
+
 } // namespace Lua
 } // namespace HttpFilters
 } // namespace Extensions
diff --git a/source/extensions/filters/http/lua/lua_filter.h b/source/extensions/filters/http/lua/lua_filter.h
@@ -5,6 +5,7 @@
 
 #include "common/crypto/utility.h"
 
+#include "extensions/common/utility.h"
 #include "extensions/filters/common/lua/wrappers.h"
 #include "extensions/filters/http/lua/wrappers.h"
 #include "extensions/filters/http/well_known_names.h"
@@ -14,20 +15,6 @@ namespace Extensions {
 namespace HttpFilters {
 namespace Lua {
 
-namespace {
-const ProtobufWkt::Struct& getMetadata(Http::StreamFilterCallbacks* callbacks) {
-  if (callbacks->route() == nullptr || callbacks->route()->routeEntry() == nullptr) {
-    return ProtobufWkt::Struct::default_instance();
-  }
-  const auto& metadata = callbacks->route()->routeEntry()->metadata();
-  const auto& filter_it = metadata.filter_metadata().find(HttpFilterNames::get().Lua);
-  if (filter_it == metadata.filter_metadata().end()) {
-    return ProtobufWkt::Struct::default_instance();
-  }
-  return filter_it->second;
-}
-} // namespace
-
 /**
  * Callbacks used by a stream handler to access the filter.
  */
@@ -391,7 +378,7 @@ class Filter : public Http::StreamFilter, Logger::Loggable<Logger::Id::lua> {
     void respond(Http::ResponseHeaderMapPtr&& headers, Buffer::Instance* body,
                  lua_State* state) override;
 
-    const ProtobufWkt::Struct& metadata() const override { return getMetadata(callbacks_); }
+    const ProtobufWkt::Struct& metadata() const override;
     StreamInfo::StreamInfo& streamInfo() override { return callbacks_->streamInfo(); }
     const Network::Connection* connection() const override { return callbacks_->connection(); }
 
@@ -412,7 +399,7 @@ class Filter : public Http::StreamFilter, Logger::Loggable<Logger::Id::lua> {
     void respond(Http::ResponseHeaderMapPtr&& headers, Buffer::Instance* body,
                  lua_State* state) override;
 
-    const ProtobufWkt::Struct& metadata() const override { return getMetadata(callbacks_); }
+    const ProtobufWkt::Struct& metadata() const override;
     StreamInfo::StreamInfo& streamInfo() override { return callbacks_->streamInfo(); }
     const Network::Connection* connection() const override { return callbacks_->connection(); }
 
diff --git a/test/common/config/registry_test.cc b/test/common/config/registry_test.cc
@@ -95,7 +95,7 @@ TEST(RegistryTest, DEPRECATED_FEATURE_TEST(WithDeprecatedFactoryPublished)) {
                 "testing.published.deprecated_name")
                 ->name());
   EXPECT_LOG_CONTAINS("warn",
-                      fmt::format("{} is deprecated, use {} instead.",
+                      fmt::format("Using deprecated extension name '{}' for '{}'.",
                                   "testing.published.deprecated_name",
                                   "testing.published.instead_name"),
                       Envoy::Registry::FactoryRegistry<PublishedFactory>::getFactory(
@@ -152,7 +152,7 @@ TEST(RegistryTest, DEPRECATED_FEATURE_TEST(VersionedWithDeprecatedNamesFactory))
                 "testing.published.versioned.deprecated_name")
                 ->name());
   EXPECT_LOG_CONTAINS("warn",
-                      fmt::format("{} is deprecated, use {} instead.",
+                      fmt::format("Using deprecated extension name '{}' for '{}'.",
                                   "testing.published.versioned.deprecated_name",
                                   "testing.published.versioned.instead_name"),
                       Envoy::Registry::FactoryRegistry<PublishedFactory>::getFactory(
diff --git a/test/common/router/config_impl_test.cc b/test/common/router/config_impl_test.cc
@@ -4857,6 +4857,34 @@ TEST_F(RouteMatcherTest, TestOpaqueConfig) {
   EXPECT_EQ(opaque_config.find("name2")->second, "value2");
 }
 
+// Test that the deprecated name works for opaque configs.
+TEST_F(RouteMatcherTest, DEPRECATED_FEATURE_TEST(TestOpaqueConfigUsingDeprecatedName)) {
+  const std::string yaml = R"EOF(
+virtual_hosts:
+- name: default
+  domains:
+  - "*"
+  routes:
+  - match:
+      prefix: "/api"
+    route:
+      cluster: ats
+    metadata:
+      filter_metadata:
+        envoy.router:
+          name1: value1
+          name2: value2
+)EOF";
+
+  TestConfigImpl config(parseRouteConfigurationFromV2Yaml(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");
+}
+
 class RoutePropertyTest : public testing::Test, public ConfigImplTestBase {};
 
 TEST_F(RoutePropertyTest, ExcludeVHRateLimits) {
diff --git a/test/extensions/common/utility_test.cc b/test/extensions/common/utility_test.cc
diff --git a/test/extensions/filters/http/lua/BUILD b/test/extensions/filters/http/lua/BUILD
diff --git a/test/extensions/filters/http/lua/lua_filter_test.cc b/test/extensions/filters/http/lua/lua_filter_test.cc
diff --git a/test/extensions/filters/http/lua/lua_integration_test.cc b/test/extensions/filters/http/lua/lua_integration_test.cc
