filters: instantiate filters on a per-stream basis (#967)

Description: Changes filter instantiation to be a per-stream basis, across the core, bridge, and iOS platform layers. Also improves usage of CFBridge* functions to be better in line with their intent. Risk Level: Moderate Testing: Local end-to-end Signed-off-by: Mike Schore <[email protected]> Signed-off-by: JP Simard <[email protected]>
envoyproxy · Nov 28, 2022 · d564190 · d564190
1 parent 51a18d9
commit d564190
Show file tree

Hide file tree

Showing 13 changed files with 145 additions and 58 deletions.
diff --git a/mobile/examples/swift/hello_world/DemoFilter.swift b/mobile/examples/swift/hello_world/DemoFilter.swift
@@ -3,7 +3,7 @@ import Foundation
 
 struct DemoFilter: ResponseFilter {
   // TODO(goaway): Update once dynamic registration is in place.
-  let name = "PlatformStub"
+  static let name = "PlatformStub"
 
   func onResponseHeaders(_ headers: ResponseHeaders, endStream: Bool)
     -> FilterHeaderStatus<ResponseHeaders>

diff --git a/mobile/examples/swift/hello_world/ViewController.swift b/mobile/examples/swift/hello_world/ViewController.swift
@@ -15,7 +15,7 @@ final class ViewController: UITableViewController {
     super.viewDidLoad()
     do {
       NSLog("starting Envoy...")
-      self.client = try StreamClientBuilder().addFilter(DemoFilter()).build()
+      self.client = try StreamClientBuilder().addFilter(DemoFilter.self).build()
     } catch let error {
       NSLog("starting Envoy failed: \(error)")
     }

diff --git a/mobile/library/common/extensions/filters/http/platform_bridge/BUILD b/mobile/library/common/extensions/filters/http/platform_bridge/BUILD
@@ -25,6 +25,7 @@ envoy_cc_library(
         "//library/common/http:header_utility_lib",
         "//library/common/types:c_types_lib",
         "@envoy//include/envoy/http:filter_interface",
+        "@envoy//source/common/common:minimal_logger_lib",
         "@envoy//source/extensions/filters/http/common:pass_through_filter_lib",
     ],
 )

diff --git a/mobile/library/common/extensions/filters/http/platform_bridge/c_types.h b/mobile/library/common/extensions/filters/http/platform_bridge/c_types.h
@@ -57,23 +57,35 @@ typedef struct {
 extern "C" { // function pointers
 #endif
 
+/**
+ * Function signature for filter factory. Implementations must return a instance_context
+ * capable of dispatching envoy_http_filter calls (below) to a platform filter instance.
+ */
+typedef const void* (*envoy_filter_init_f)(const void* context);
+
 /**
  * Function signature for on-headers filter invocations.
  */
 typedef envoy_filter_headers_status (*envoy_filter_on_headers_f)(envoy_headers headers,
-                                                                 bool end_stream, void* context);
+                                                                 bool end_stream,
+                                                                 const void* context);
 
 /**
  * Function signature for on-data filter invocations.
  */
 typedef envoy_filter_data_status (*envoy_filter_on_data_f)(envoy_data data, bool end_stream,
-                                                           void* context);
+                                                           const void* context);
 
 /**
  * Function signature for on-trailers filter invocations.
  */
 typedef envoy_filter_trailers_status (*envoy_filter_on_trailers_f)(envoy_headers trailers,
-                                                                   void* context);
+                                                                   const void* context);
+
+/**
+ * Function signature to release a filter instance once the filter chain is finished with it.
+ */
+typedef void (*envoy_filter_release_f)(const void* context);
 
 #ifdef __cplusplus
 } // function pointers
@@ -84,11 +96,14 @@ typedef envoy_filter_trailers_status (*envoy_filter_on_trailers_f)(envoy_headers
  * PlatformBridgeFilter.
  */
 typedef struct {
+  envoy_filter_init_f init_filter;
   envoy_filter_on_headers_f on_request_headers;
   envoy_filter_on_data_f on_request_data;
   envoy_filter_on_trailers_f on_request_trailers;
   envoy_filter_on_headers_f on_response_headers;
   envoy_filter_on_data_f on_response_data;
   envoy_filter_on_trailers_f on_response_trailers;
-  void* context;
+  envoy_filter_release_f release_filter;
+  const void* static_context;
+  const void* instance_context;
 } envoy_http_filter;
diff --git a/mobile/library/common/extensions/filters/http/platform_bridge/filter.cc b/mobile/library/common/extensions/filters/http/platform_bridge/filter.cc
@@ -17,22 +17,54 @@ namespace PlatformBridge {
 
 PlatformBridgeFilterConfig::PlatformBridgeFilterConfig(
     const envoymobile::extensions::filters::http::platform_bridge::PlatformBridge& proto_config)
-    : platform_filter_(static_cast<envoy_http_filter*>(
+    : filter_name_(proto_config.platform_filter_name()),
+      platform_filter_(static_cast<envoy_http_filter*>(
           Api::External::retrieveApi(proto_config.platform_filter_name()))) {}
 
 PlatformBridgeFilter::PlatformBridgeFilter(PlatformBridgeFilterConfigSharedPtr config)
-    : platform_filter_(config->platform_filter()) {}
+    : filter_name_(config->filter_name()), platform_filter_(*config->platform_filter()) {
+  // The initialization above sets platform_filter_ to a copy of the struct stored on the config.
+  // In the typical case, this will represent a filter implementation that needs to be intantiated.
+  // static_context will contain the necessary platform-specific mechanism to produce a filter
+  // instance. instance_context will initially be null, but after initialization, set to the
+  // context needed for actual filter invocations.
+
+  // If init_filter is missing, zero out the rest of the struct for safety.
+  if (platform_filter_.init_filter == nullptr) {
+    ENVOY_LOG(debug, "platform bridge filter: missing initializer for {}", filter_name_);
+    platform_filter_ = {};
+    return;
+  }
+
+  // Set the instance_context to the result of the initialization call. Cleanup will ultimately
+  // occur during in the onDestroy() invocation below.
+  platform_filter_.instance_context = platform_filter_.init_filter(platform_filter_.static_context);
+  ASSERT(platform_filter_.instance_context,
+         fmt::format("init_filter unsuccessful for {}", filter_name_));
+}
+
+void PlatformBridgeFilter::onDestroy() {
+  // Allow nullptr as no-op only if nothing was initialized.
+  if (platform_filter_.release_filter == nullptr) {
+    ASSERT(!platform_filter_.instance_context,
+           fmt::format("release_filter required for {}", filter_name_));
+    return;
+  }
+
+  platform_filter_.release_filter(platform_filter_.instance_context);
+  platform_filter_.instance_context = nullptr;
+}
 
 Http::FilterHeadersStatus PlatformBridgeFilter::onHeaders(Http::HeaderMap& headers, bool end_stream,
                                                           envoy_filter_on_headers_f on_headers) {
-  // Allow nullptr to act as (optimized) no-op.
+  // Allow nullptr to act as no-op.
   if (on_headers == nullptr) {
     return Http::FilterHeadersStatus::Continue;
   }
 
   envoy_headers in_headers = Http::Utility::toBridgeHeaders(headers);
   envoy_filter_headers_status result =
-      on_headers(in_headers, end_stream, platform_filter_->context);
+      on_headers(in_headers, end_stream, platform_filter_.instance_context);
   Http::FilterHeadersStatus status = static_cast<Http::FilterHeadersStatus>(result.status);
   // TODO(goaway): Current platform implementations expose immutable headers, thus any modification
   // necessitates a full copy. Add 'modified' bit to determine when we can elide the copy. See also
@@ -50,35 +82,32 @@ Http::FilterHeadersStatus PlatformBridgeFilter::onHeaders(Http::HeaderMap& heade
 
 Http::FilterDataStatus PlatformBridgeFilter::onData(Buffer::Instance& data, bool end_stream,
                                                     envoy_filter_on_data_f on_data) {
-  // Allow nullptr to act as (optimized) no-op.
+  // Allow nullptr to act as no-op.
   if (on_data == nullptr) {
     return Http::FilterDataStatus::Continue;
   }
 
   envoy_data in_data = Buffer::Utility::toBridgeData(data);
-  envoy_filter_data_status result = on_data(in_data, end_stream, platform_filter_->context);
+  envoy_filter_data_status result = on_data(in_data, end_stream, platform_filter_.instance_context);
   Http::FilterDataStatus status = static_cast<Http::FilterDataStatus>(result.status);
-  // Current platform implementations expose immutable data, thus any modification necessitates a
-  // full copy. If the returned buffer is identical, we assume no modification was made and elide
-  // the copy here. See also https://github.com/lyft/envoy-mobile/issues/949 for potential future
-  // optimization.
-  if (in_data.bytes != result.data.bytes) {
-    data.drain(data.length());
-    data.addBufferFragment(*Buffer::BridgeFragment::createBridgeFragment(result.data));
-  }
+  // TODO(goaway): Current platform implementations expose immutable data, thus any modification
+  // necessitates a full copy. Add 'modified' bit to determine when we can elide the copy. See also
+  // https://github.com/lyft/envoy-mobile/issues/949 for potential future optimization.
+  data.drain(data.length());
+  data.addBufferFragment(*Buffer::BridgeFragment::createBridgeFragment(result.data));
 
   return status;
 }
 
 Http::FilterHeadersStatus PlatformBridgeFilter::decodeHeaders(Http::RequestHeaderMap& headers,
                                                               bool end_stream) {
   // Delegate to shared implementation for request and response path.
-  return onHeaders(headers, end_stream, platform_filter_->on_request_headers);
+  return onHeaders(headers, end_stream, platform_filter_.on_request_headers);
 }
 
 Http::FilterDataStatus PlatformBridgeFilter::decodeData(Buffer::Instance& data, bool end_stream) {
   // Delegate to shared implementation for request and response path.
-  return onData(data, end_stream, platform_filter_->on_request_data);
+  return onData(data, end_stream, platform_filter_.on_request_data);
 }
 
 Http::FilterTrailersStatus
@@ -98,12 +127,12 @@ PlatformBridgeFilter::encode100ContinueHeaders(Http::ResponseHeaderMap& /*header
 Http::FilterHeadersStatus PlatformBridgeFilter::encodeHeaders(Http::ResponseHeaderMap& headers,
                                                               bool end_stream) {
   // Delegate to shared implementation for request and response path.
-  return onHeaders(headers, end_stream, platform_filter_->on_response_headers);
+  return onHeaders(headers, end_stream, platform_filter_.on_response_headers);
 }
 
 Http::FilterDataStatus PlatformBridgeFilter::encodeData(Buffer::Instance& data, bool end_stream) {
   // Delegate to shared implementation for request and response path.
-  return onData(data, end_stream, platform_filter_->on_response_data);
+  return onData(data, end_stream, platform_filter_.on_response_data);
 }
 
 Http::FilterTrailersStatus

diff --git a/mobile/library/common/extensions/filters/http/platform_bridge/filter.h b/mobile/library/common/extensions/filters/http/platform_bridge/filter.h
@@ -2,6 +2,8 @@
 
 #include "envoy/http/filter.h"
 
+#include "common/common/logger.h"
+
 #include "extensions/filters/http/common/pass_through_filter.h"
 
 #include "library/common/extensions/filters/http/platform_bridge/c_types.h"
@@ -17,9 +19,11 @@ class PlatformBridgeFilterConfig {
   PlatformBridgeFilterConfig(
       const envoymobile::extensions::filters::http::platform_bridge::PlatformBridge& proto_config);
 
+  const std::string& filter_name() { return filter_name_; }
   const envoy_http_filter* platform_filter() const { return platform_filter_; }
 
 private:
+  const std::string filter_name_;
   const envoy_http_filter* platform_filter_;
 };
 
@@ -28,10 +32,14 @@ typedef std::shared_ptr<PlatformBridgeFilterConfig> PlatformBridgeFilterConfigSh
 /**
  * Harness to bridge Envoy filter invocations up to the platform layer.
  */
-class PlatformBridgeFilter final : public Http::PassThroughFilter {
+class PlatformBridgeFilter final : public Http::PassThroughFilter,
+                                   Logger::Loggable<Logger::Id::filter> {
 public:
   PlatformBridgeFilter(PlatformBridgeFilterConfigSharedPtr config);
 
+  // StreamFilterBase
+  void onDestroy() override;
+
   // StreamDecoderFilter
   Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap& headers,
                                           bool end_stream) override;
@@ -52,7 +60,8 @@ class PlatformBridgeFilter final : public Http::PassThroughFilter {
                                       envoy_filter_on_headers_f on_headers);
   Http::FilterDataStatus onData(Buffer::Instance& data, bool end_stream,
                                 envoy_filter_on_data_f on_data);
-  const envoy_http_filter* platform_filter_;
+  const std::string filter_name_;
+  envoy_http_filter platform_filter_;
 };
 
 } // namespace PlatformBridge

diff --git a/mobile/library/objective-c/BUILD b/mobile/library/objective-c/BUILD
@@ -13,6 +13,7 @@ objc_library(
         "EnvoyEngineImpl.m",
         "EnvoyHTTPCallbacks.m",
         "EnvoyHTTPFilter.m",
+        "EnvoyHTTPFilterFactory.m",
         "EnvoyHTTPStreamImpl.m",
         "EnvoyNetworkMonitor.m",
     ],

diff --git a/mobile/library/objective-c/EnvoyConfiguration.m b/mobile/library/objective-c/EnvoyConfiguration.m
@@ -9,7 +9,7 @@ - (instancetype)initWithStatsDomain:(NSString *)statsDomain
                   dnsRefreshSeconds:(UInt32)dnsRefreshSeconds
        dnsFailureRefreshSecondsBase:(UInt32)dnsFailureRefreshSecondsBase
         dnsFailureRefreshSecondsMax:(UInt32)dnsFailureRefreshSecondsMax
-                        filterChain:(NSArray<EnvoyHTTPFilter *> *)httpFilters
+                        filterChain:(NSArray<EnvoyHTTPFilterFactory *> *)httpFilterFactories
                   statsFlushSeconds:(UInt32)statsFlushSeconds
                          appVersion:(NSString *)appVersion
                               appId:(NSString *)appId
@@ -24,7 +24,7 @@ - (instancetype)initWithStatsDomain:(NSString *)statsDomain
   self.dnsRefreshSeconds = dnsRefreshSeconds;
   self.dnsFailureRefreshSecondsBase = dnsFailureRefreshSecondsBase;
   self.dnsFailureRefreshSecondsMax = dnsFailureRefreshSecondsMax;
-  self.httpFilters = httpFilters;
+  self.httpFilterFactories = httpFilterFactories;
   self.statsFlushSeconds = statsFlushSeconds;
   self.appVersion = appVersion;
   self.appId = appId;

diff --git a/mobile/library/objective-c/EnvoyEngine.h b/mobile/library/objective-c/EnvoyEngine.h
@@ -68,14 +68,22 @@ extern const int kEnvoyFilterHeadersStatusStopAllIterationAndBuffer;
 
 @interface EnvoyHTTPFilter : NSObject
 
-@property (nonatomic, strong) NSString *name;
-
 @property (nonatomic, strong) NSArray * (^onRequestHeaders)(EnvoyHeaders *headers, BOOL endStream);
 
 @property (nonatomic, strong) NSArray * (^onResponseHeaders)(EnvoyHeaders *headers, BOOL endStream);
 
 @end
 
+#pragma mark - EnvoyHTTPFilterFactory
+
+@interface EnvoyHTTPFilterFactory : NSObject
+
+@property (nonatomic, strong) NSString *filterName;
+
+@property (nonatomic, strong) EnvoyHTTPFilter * (^create)();
+
+@end
+
 #pragma mark - EnvoyHTTPStream
 
 @protocol EnvoyHTTPStream
@@ -143,7 +151,7 @@ extern const int kEnvoyFilterHeadersStatusStopAllIterationAndBuffer;
 @property (nonatomic, assign) UInt32 dnsRefreshSeconds;
 @property (nonatomic, assign) UInt32 dnsFailureRefreshSecondsBase;
 @property (nonatomic, assign) UInt32 dnsFailureRefreshSecondsMax;
-@property (nonatomic, strong) NSArray<EnvoyHTTPFilter *> *httpFilters;
+@property (nonatomic, strong) NSArray<EnvoyHTTPFilterFactory *> *httpFilterFactories;
 @property (nonatomic, assign) UInt32 statsFlushSeconds;
 @property (nonatomic, strong) NSString *appVersion;
 @property (nonatomic, strong) NSString *appId;
@@ -157,7 +165,7 @@ extern const int kEnvoyFilterHeadersStatusStopAllIterationAndBuffer;
                   dnsRefreshSeconds:(UInt32)dnsRefreshSeconds
        dnsFailureRefreshSecondsBase:(UInt32)dnsFailureRefreshSecondsBase
         dnsFailureRefreshSecondsMax:(UInt32)dnsFailureRefreshSecondsMax
-                        filterChain:(NSArray<EnvoyHTTPFilter *> *)httpFilters
+                        filterChain:(NSArray<EnvoyHTTPFilterFactory *> *)httpFilterFactories
                   statsFlushSeconds:(UInt32)statsFlushSeconds
                          appVersion:(NSString *)appVersion
                               appId:(NSString *)appId