Support dynamic_action in composite filter (#33511)

--------- Signed-off-by: Yanjun Xiang <yanjunxiang@google.com>
envoyproxy · Apr 15, 2024 · b5c7e51 · b5c7e51
1 parent 5219730
commit b5c7e51
Show file tree

Hide file tree

Showing 10 changed files with 342 additions and 232 deletions.
diff --git a/api/envoy/extensions/filters/http/composite/v3/composite.proto b/api/envoy/extensions/filters/http/composite/v3/composite.proto
@@ -55,9 +55,6 @@ message ExecuteFilterAction {
 
   // Dynamic configuration of filter obtained via extension configuration discovery service.
   // Only one of ``typed_config`` or ``dynamic_config`` can be set.
-  // When composite filter is in upstream filter chain, the ``dynamic_config``
-  // can not be set. TBD: Refactor cluster manager init sequence to Support this.
-  // Please check https://github.com/envoyproxy/envoy/issues/33218 for details.
   DynamicConfig dynamic_config = 2
       [(udpa.annotations.field_migrate).oneof_promotion = "config_type"];
 }
diff --git a/envoy/upstream/cluster_manager.h b/envoy/upstream/cluster_manager.h
@@ -506,7 +506,7 @@ class ClusterManagerFactory {
 
   /**
    * Allocate a cluster manager from configuration proto.
-   * The cluster manager init() method needs to be called right after this method.
+   * The cluster manager initialize() method needs to be called right after this method.
    * Please check https://github.com/envoyproxy/envoy/issues/33218 for details.
    */
   virtual ClusterManagerPtr

diff --git a/source/extensions/filters/http/common/factory_base.h b/source/extensions/filters/http/common/factory_base.h
@@ -161,7 +161,7 @@ class DualFactoryBase : public CommonFactoryBase<ConfigProto, RouteConfigProto>,
                                     Server::Configuration::ServerFactoryContext& context) PURE;
 
   // This method is for dual filter to create filter from server context when it is configured
-  // in downstream. If a dual filter is in upstream, it must not call this method.
+  // in downstream. It won't be called if a dual filter is in upstream.
   Envoy::Http::FilterFactoryCb createFilterFactoryFromProtoWithServerContext(
       const Protobuf::Message& proto_config, const std::string& stats_prefix,
       Server::Configuration::ServerFactoryContext& server_context) override {
@@ -176,7 +176,7 @@ class DualFactoryBase : public CommonFactoryBase<ConfigProto, RouteConfigProto>,
   createFilterFactoryFromProtoWithServerContextTyped(const ConfigProto&, const std::string&,
                                                      Server::Configuration::ServerFactoryContext&) {
     ExceptionUtil::throwEnvoyException(
-        "Creating filter factory from server factory context is not supported");
+        "DualFactoryBase: creating filter factory from server factory context is not supported");
     return nullptr;
   }
 };

diff --git a/source/extensions/filters/http/composite/action.cc b/source/extensions/filters/http/composite/action.cc
@@ -5,9 +5,6 @@ namespace Extensions {
 namespace HttpFilters {
 namespace Composite {
 
-using HttpExtensionConfigProviderSharedPtr = std::shared_ptr<
-    Config::DynamicExtensionConfigProvider<Envoy::Filter::NamedHttpFilterFactoryCb>>;
-
 void ExecuteFilterAction::createFilters(Http::FilterChainFactoryCallbacks& callbacks) const {
   cb_(callbacks);
 }
@@ -25,47 +22,51 @@ Matcher::ActionFactoryCb ExecuteFilterActionFactory::createActionFactoryCb(
   }
 
   if (composite_action.has_dynamic_config()) {
-    if (!context.is_downstream_) {
-      throw EnvoyException(fmt::format("When composite filter is in upstream, the composite action "
-                                       "config must not be dynamic."));
-    }
-
-    if (!context.factory_context_.has_value() || !context.server_factory_context_.has_value()) {
-      throw EnvoyException(fmt::format("Failed to get factory context or server factory context."));
+    if (context.is_downstream_) {
+      return createDynamicActionFactoryCbDownstream(composite_action, context);
+    } else {
+      return createDynamicActionFactoryCbUpstream(composite_action, context);
     }
-    std::string name = composite_action.dynamic_config().name();
-    // Create a dynamic filter config provider and register it with the server factory context.
-    auto config_discovery = composite_action.dynamic_config().config_discovery();
-    Server::Configuration::FactoryContext& factory_context = context.factory_context_.value();
-    Server::Configuration::ServerFactoryContext& server_factory_context =
-        context.server_factory_context_.value();
-    auto provider_manager =
-        Envoy::Http::FilterChainUtility::createSingletonDownstreamFilterConfigProviderManager(
-            server_factory_context);
-    HttpExtensionConfigProviderSharedPtr provider =
-        provider_manager->createDynamicFilterConfigProvider(
-            config_discovery, composite_action.dynamic_config().name(), server_factory_context,
-            factory_context, server_factory_context.clusterManager(), false, "http", nullptr);
-    return [provider = std::move(provider), n = std::move(name)]() -> Matcher::ActionPtr {
-      auto config_value = provider->config();
-      if (config_value.has_value()) {
-        auto factory_cb = config_value.value().get().factory_cb;
-        return std::make_unique<ExecuteFilterAction>(factory_cb, n);
-      }
-      // There is no dynamic config available. Apply missing config filter.
-      auto factory_cb = Envoy::Http::MissingConfigFilterFactory;
-      return std::make_unique<ExecuteFilterAction>(factory_cb, n);
-    };
   }
 
   if (context.is_downstream_) {
-    return createActionFactoryCbDownstream(composite_action, context, validation_visitor);
+    return createStaticActionFactoryCbDownstream(composite_action, context, validation_visitor);
   } else {
-    return createActionFactoryCbUpstream(composite_action, context, validation_visitor);
+    return createStaticActionFactoryCbUpstream(composite_action, context, validation_visitor);
+  }
+}
+
+Matcher::ActionFactoryCb ExecuteFilterActionFactory::createDynamicActionFactoryCbDownstream(
+    const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action,
+    Http::Matching::HttpFilterActionContext& context) {
+  if (!context.factory_context_.has_value() || !context.server_factory_context_.has_value()) {
+    throw EnvoyException(
+        fmt::format("Failed to get downstream factory context or server factory context."));
+  }
+  auto provider_manager =
+      Envoy::Http::FilterChainUtility::createSingletonDownstreamFilterConfigProviderManager(
+          context.server_factory_context_.value());
+  return createDynamicActionFactoryCbTyped<Server::Configuration::FactoryContext>(
+      composite_action, context, "http", context.factory_context_.value(), provider_manager);
+}
+
+Matcher::ActionFactoryCb ExecuteFilterActionFactory::createDynamicActionFactoryCbUpstream(
+    const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action,
+    Http::Matching::HttpFilterActionContext& context) {
+  if (!context.upstream_factory_context_.has_value() ||
+      !context.server_factory_context_.has_value()) {
+    throw EnvoyException(
+        fmt::format("Failed to get upstream factory context or server factory context."));
   }
+  auto provider_manager =
+      Envoy::Http::FilterChainUtility::createSingletonUpstreamFilterConfigProviderManager(
+          context.server_factory_context_.value());
+  return createDynamicActionFactoryCbTyped<Server::Configuration::UpstreamFactoryContext>(
+      composite_action, context, "router upstream http", context.upstream_factory_context_.value(),
+      provider_manager);
 }
 
-Matcher::ActionFactoryCb ExecuteFilterActionFactory::createActionFactoryCbDownstream(
+Matcher::ActionFactoryCb ExecuteFilterActionFactory::createStaticActionFactoryCbDownstream(
     const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action,
     Http::Matching::HttpFilterActionContext& context,
     ProtobufMessage::ValidationVisitor& validation_visitor) {
@@ -93,7 +94,7 @@ Matcher::ActionFactoryCb ExecuteFilterActionFactory::createActionFactoryCbDownst
   }
 
   if (callback == nullptr) {
-    throw EnvoyException("Failed to get filter factory creation function");
+    throw EnvoyException("Failed to get downstream filter factory creation function");
   }
   std::string name = composite_action.typed_config().name();
 
@@ -102,7 +103,7 @@ Matcher::ActionFactoryCb ExecuteFilterActionFactory::createActionFactoryCbDownst
   };
 }
 
-Matcher::ActionFactoryCb ExecuteFilterActionFactory::createActionFactoryCbUpstream(
+Matcher::ActionFactoryCb ExecuteFilterActionFactory::createStaticActionFactoryCbUpstream(
     const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action,
     Http::Matching::HttpFilterActionContext& context,
     ProtobufMessage::ValidationVisitor& validation_visitor) {
@@ -124,7 +125,7 @@ Matcher::ActionFactoryCb ExecuteFilterActionFactory::createActionFactoryCbUpstre
   }
 
   if (callback == nullptr) {
-    throw EnvoyException("Failed to get filter factory creation function");
+    throw EnvoyException("Failed to get upstream filter factory creation function");
   }
   std::string name = composite_action.typed_config().name();
 

diff --git a/source/extensions/filters/http/composite/action.h b/source/extensions/filters/http/composite/action.h
@@ -11,6 +11,9 @@ namespace Extensions {
 namespace HttpFilters {
 namespace Composite {
 
+using HttpExtensionConfigProviderSharedPtr = std::shared_ptr<
+    Config::DynamicExtensionConfigProvider<Envoy::Filter::NamedHttpFilterFactoryCb>>;
+
 class ExecuteFilterAction
     : public Matcher::ActionBase<
           envoy::extensions::filters::http::composite::v3::ExecuteFilterAction> {
@@ -43,12 +46,46 @@ class ExecuteFilterActionFactory
   }
 
 private:
-  Matcher::ActionFactoryCb createActionFactoryCbDownstream(
+  template <class FactoryCtx, class FilterCfgProviderMgr>
+  Matcher::ActionFactoryCb createDynamicActionFactoryCbTyped(
+      const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action,
+      Http::Matching::HttpFilterActionContext& context, const std::string& filter_chain_type,
+      FactoryCtx& factory_context, std::shared_ptr<FilterCfgProviderMgr>& provider_manager) {
+    std::string name = composite_action.dynamic_config().name();
+    // Create a dynamic filter config provider and register it with the server factory context.
+    auto config_discovery = composite_action.dynamic_config().config_discovery();
+    Server::Configuration::ServerFactoryContext& server_factory_context =
+        context.server_factory_context_.value();
+    HttpExtensionConfigProviderSharedPtr provider =
+        provider_manager->createDynamicFilterConfigProvider(
+            config_discovery, name, server_factory_context, factory_context,
+            server_factory_context.clusterManager(), false, filter_chain_type, nullptr);
+    return [provider = std::move(provider), n = std::move(name)]() -> Matcher::ActionPtr {
+      auto config_value = provider->config();
+      if (config_value.has_value()) {
+        auto factory_cb = config_value.value().get().factory_cb;
+        return std::make_unique<ExecuteFilterAction>(factory_cb, n);
+      }
+      // There is no dynamic config available. Apply missing config filter.
+      auto factory_cb = Envoy::Http::MissingConfigFilterFactory;
+      return std::make_unique<ExecuteFilterAction>(factory_cb, n);
+    };
+  }
+
+  Matcher::ActionFactoryCb createDynamicActionFactoryCbDownstream(
+      const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action,
+      Http::Matching::HttpFilterActionContext& context);
+
+  Matcher::ActionFactoryCb createDynamicActionFactoryCbUpstream(
+      const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action,
+      Http::Matching::HttpFilterActionContext& context);
+
+  Matcher::ActionFactoryCb createStaticActionFactoryCbDownstream(
       const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action,
       Http::Matching::HttpFilterActionContext& context,
       ProtobufMessage::ValidationVisitor& validation_visitor);
 
-  Matcher::ActionFactoryCb createActionFactoryCbUpstream(
+  Matcher::ActionFactoryCb createStaticActionFactoryCbUpstream(
       const envoy::extensions::filters::http::composite::v3::ExecuteFilterAction& composite_action,
       Http::Matching::HttpFilterActionContext& context,
       ProtobufMessage::ValidationVisitor& validation_visitor);