wip: platform extension filter poc #883

goaway · 2020-06-05T20:49:56Z

Description: Introduces a mechanism to write platform-native filters that can be linked into Envoy's HTTP filter chain.

Signed-off-by: Mike Schore [email protected]

goaway · 2020-06-08T02:25:35Z

@junr03 format checks pass for me locally - any idea how I can debug what's going on?

rebello95 · 2020-06-08T14:20:28Z

Looks like buildifier failed based on the CI log. Do you have a diff when you do:

buildifier library/common/extensions/filter/BUILD

junr03 · 2020-06-08T15:33:35Z

It might be the case that the version of buildifier you have is different than that in the build container https://github.com/envoyproxy/envoy-build-tools/blob/master/build_container/build_container_common.sh#L31. I would double check that.

junr03

This is so cool! I think this will be pretty awesome. Some comments to get started.

junr03 · 2020-06-08T16:20:05Z

library/common/extensions/filter/BUILD

+
+envoy_package()
+
+envoy_cc_library(


eventually I believe we want this to be envoy_cc_extension.

junr03 · 2020-06-08T16:21:00Z

library/common/extensions/filter/c_types.h

+  ENVOY_FILTER_HEADERS_STATUS_STOP_ITERATION,
+  ENVOY_FILTER_HEADERS_STATUS_CONTINUE_AND_END_STREAM,
+  ENVOY_FILTER_HEADERS_STATUS_STOP_ALL_ITERATION_AND_BUFFER,
+} envoy_filter_headers_status_t;


we discussed having a subset of these enums. Do you want to do that here in the declarations of in the mapping functions?

I decided to expose them all at this layer because there's no real reason not to.

Even if we don't want to expose all of that functionality to platform filters?

I think it's fine to expose them here. To explain myself better, my impulse is to not make assumptions at this layer.

junr03 · 2020-06-08T16:23:11Z

library/common/extensions/filter/c_types.h

+  envoy_filter_on_headers_f on_response_headers;
+  envoy_filter_on_data_f on_response_data;
+  void* context;
+} envoy_http_filter;


Do we want to create a distinction between a request(decode) and response(encode) filter? And a sub-type that is able to do both? What would be a good pattern to do that in c?

I mean you could use structs that have identical layouts but reserve the space for the missing callbacks. Or you could just use this one struct and set callbacks you're not using to null. I think the latter simplifies things.

But then you would have to do null checks or assertions, vs. having the type dictate what functions you should implement.

My plan was to default all callbacks to null and handle that gracefully in the base (bridge) implementation. I think it's more defensive, given the type variation.

junr03 · 2020-06-08T16:24:16Z

library/common/extensions/filter/platform_filter.h

+/**
+ * Harness to bridge Envoy filter invocations up to the platform layer.
+ */
+class PlatformExtensionFilter final : public PassThroughFilter {


nit: above in c_types this was called PlatformHarnessFilter

Yeah, I renamed a few times and missed that one. Settled on PlatformExtensionFilter, as in "the filter that enables platform extensions".

junr03 · 2020-06-08T16:26:16Z

library/common/extensions/filter/platform_filter.h

+  PlatformExtensionFilter(envoy_http_filter platform_filter) : platform_filter_(platform_filter) {}
+
+  // StreamDecoderFilter
+  FilterHeadersStatus decodeHeaders(RequestHeaderMap& headers, bool end_stream) override;


For now we don't need any of the other overrides except this one. We want to attempt to harness a call to decode headers to modify the request header, right? The PassThroughFilter takes care of the rest.

We don't need them for a prototype, but I left them here for reference (and if we go this route, we'll definitely have them).

Yeah, but given that the pass through filter implements, I was wondering if we should delete to make this PR easier to read TIOLI

Either way.

junr03 · 2020-06-08T16:32:00Z

library/common/extensions/filter/platform_filter.cc

+
+FilterHeadersStatus PlatformExtensionFilter::decodeHeaders(RequestHeaderMap& headers,
+                                                           bool end_stream) {
+  return mapStatus(platform_filter_.on_request_headers(Utility::toBridgeHeaders(headers),


Utility::toBridgeHeaders copies the headers. If the platform modifies the values then it would not be reflected in the RequestHeaderMap& headers. We can solve this a few ways (as of course you know). I might bias toward on_request_headers returning the value? It would cause a second copy but allows us to keep the memory contract were the caller of the function owns memory cleanup, i.e the platform code would own cleaning the passed in value, and this code would on clean up of the return value after we update RequestHeaderMap& headers here.

wdyt?

discussed offline, will add more context to the PR/comments

junr03 · 2020-06-23T17:35:51Z

@goaway looks like there is currently an assert being triggered: [2020-06-18 06:32:57.068][81354][critical][assert] [library/common/api/external.cc:17] assert failure: api.

rebello95 · 2020-06-25T00:43:34Z

library/common/api/external.h

+
+/**
+ * Register an external runtime API for usage (most likely in extensions).
+ * NOTE: This is a proof of concept implementation and a HACK. Registration is NOT thread-safe.


Do we intend to fix this before merging? Seems like both of these should be thread-safe now that we've proven it works

rebello95 · 2020-06-25T00:45:00Z

library/common/config_template.cc

+          - name: envoy.filters.http.platform_extension
+            typed_config:
+              "@type": type.googleapis.com/envoymobile.extensions.filters.http.platform_extension.Bridging
+              name: PlatformExample


It'd be great to document how this works since it'll be removed before merging. Can we open an issue to document it?

rebello95 · 2020-06-25T00:46:44Z

library/common/extensions/filters/http/platform_extension/bridging_filter.cc

+  case ENVOY_FILTER_HEADERS_STATUS_STOP_ALL_ITERATION_AND_BUFFER:
+    return Http::FilterHeadersStatus::StopAllIterationAndBuffer;
+  default:
+    ASSERT(false, "unrecognized filter status from platform: {}");


Looks like this is missing the format argument?

rebello95 · 2020-06-25T00:47:26Z

library/common/extensions/filters/http/platform_extension/bridging_filter.cc

+
+Http::FilterHeadersStatus BridgingFilter::decodeHeaders(Http::RequestHeaderMap& headers,
+                                                        bool end_stream) {
+  // This is a hack that results in a full copy of the header map every time.


Nit: this isn't really so much of a hack as an area for optimization, right? Can we file an issue and label it as tech debt?

rebello95 · 2020-06-25T00:49:13Z

library/common/extensions/filters/http/platform_extension/bridging_filter.cc

+}
+
+Http::FilterHeadersStatus
+BridgingFilter::encode100ContinueHeaders(Http::ResponseHeaderMap& /*headers*/) {


When you implement the rest of these functions, I assume we won't expose this one or metadata

Probably not initially, though probably both ultimately.

rebello95 · 2020-06-25T00:54:04Z

library/swift/src/filters/RequestFilter.swift

  func onRequestHeaders(_ headers: RequestHeaders, endStream: Bool)
-    -> FilterHeaderStatus<RequestHeaders>
+    -> UInt32


Is this the intended end state for this interface? I assume not since it doesn't actually allow the filter to mutate the headers (the existing interface requires the filter to return a status containing headers, which also has the benefit of ensuring the filter cannot mutate the headers after the function returns)

Fixed in other PR.

rebello95 · 2020-06-25T00:55:18Z

library/objective-c/EnvoyEngine.h

@@ -122,6 +133,7 @@ typedef NSDictionary<NSString *, NSArray<NSString *> *> EnvoyHeaders;
 @property (nonatomic, assign) UInt32 dnsRefreshSeconds;
 @property (nonatomic, assign) UInt32 dnsFailureRefreshSecondsBase;
 @property (nonatomic, assign) UInt32 dnsFailureRefreshSecondsMax;
+@property (nonatomic, strong) NSArray *httpFilters;


Should these be NSArray<EnvoyHTTPFilter *> * types?

rebello95 · 2020-06-25T00:56:31Z

library/objective-c/EnvoyEngineImpl.m

+static EnvoyMutableHeaders *to_ios_mutable_headers(envoy_headers headers) {
+  NSMutableDictionary *headerDict = [NSMutableDictionary new];
+  for (envoy_header_size_t i = 0; i < headers.length; i++) {
+    envoy_header header = headers.headers[i];
+    NSString *headerKey = [[NSString alloc] initWithBytes:header.key.bytes
+                                                   length:header.key.length
+                                                 encoding:NSUTF8StringEncoding];
+    NSString *headerValue = [[NSString alloc] initWithBytes:header.value.bytes
+                                                     length:header.value.length
+                                                   encoding:NSUTF8StringEncoding];
+    NSMutableArray *headerValueList = headerDict[headerKey];
+    if (headerValueList == nil) {
+      headerValueList = [NSMutableArray new];
+      headerDict[headerKey] = headerValueList;
+    }
+    [headerValueList addObject:headerValue];
+  }
+  // The C envoy_headers struct can be released now because the headers have been copied.
+  release_envoy_headers(headers);
+  return headerDict;
+}


Would be great to add tests for these bridging functions before merging

We don't yet have a framework in place for testing at the bridge layer, though @junr03 was talking about exploring this.

rebello95 · 2020-06-25T00:57:12Z

library/objective-c/EnvoyEngineImpl.m

+  return ret;
+}
+
+static EnvoyMutableHeaders *to_ios_mutable_headers(envoy_headers headers) {


Out of curiosity, why does this function use snake case when the other helpers use camel case?

Our convention has been to use snake case for functions called from the c pathway; camel case for functions called from the Objective-C pathway.

Got it. Maybe we should document that in our contribution guide?

rebello95 · 2020-06-25T00:59:01Z

library/swift/src/StreamClientBuilder.swift

+  ///
+  /// - returns: This builder.
+  @discardableResult
+  public func addFilterChain(_ filterChain: [RequestFilter]) -> StreamClientBuilder {


This should probably take an individual filter, which gets appended to the underlying chain (rather than replacing)

Also what are your thoughts on how inter-op ordering will work here with regard to C++ filters?

We have a few options:

platform filters always come first on the request path / last on the response path - straightforward and what we'll do initially.

platform filters can be appended or prepended - also pretty straightforward.

represent c++ filters as part of a platform-exposed chain that can be interacted with for arbitrary ordering - more complicated, and something to consider later, if necessary. possible, though.

I'm up for doing #1 for MVP 👍

Signed-off-by: Mike Schore <[email protected]>

Signed-off-by: Jose Nino <[email protected]>

Signed-off-by: Mike Schore <[email protected]>

stale · 2020-07-07T03:02:02Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

goaway · 2020-07-07T03:14:40Z

Closing in favor of #940 and upcoming PRs.

junr03 reviewed Jun 8, 2020

View reviewed changes

goaway force-pushed the ms/platform-filter branch 3 times, most recently from d0d1416 to 3ed68fa Compare June 18, 2020 01:15

rebello95 reviewed Jun 25, 2020

View reviewed changes

goaway force-pushed the ms/platform-filter branch from 204eb8f to 0663784 Compare June 25, 2020 13:31

goaway and others added 17 commits June 30, 2020 09:24

wip: platform extension filter poc

b3abb85

Signed-off-by: Mike Schore <[email protected]>

format fixes

4adeb66

Signed-off-by: Mike Schore <[email protected]>

jose's commit

c923f86

Signed-off-by: Mike Schore <[email protected]>

move filter

25d2208

Signed-off-by: Mike Schore <[email protected]>

move everything

7b25978

Signed-off-by: Mike Schore <[email protected]>

checkpoint because

938ed9e

Signed-off-by: Mike Schore <[email protected]>

fixes

9530105

Signed-off-by: Mike Schore <[email protected]>

fixes

e2db6be

Signed-off-by: Mike Schore <[email protected]>

wire up platform

d2444e9

Signed-off-by: Mike Schore <[email protected]>

fix header inclusion

556a439

Signed-off-by: Mike Schore <[email protected]>

building up to specific errors in filter code

e6c6bfd

Signed-off-by: Jose Nino <[email protected]>

add extension

b316cf0

Signed-off-by: Mike Schore <[email protected]>

format and update

193c8ba

Signed-off-by: Mike Schore <[email protected]>

fix inclusion

8989d61

Signed-off-by: Mike Schore <[email protected]>

green

a907544

Signed-off-by: Mike Schore <[email protected]>

add hardcoded filter

b376e65

Signed-off-by: Mike Schore <[email protected]>

updates

2ca901a

Signed-off-by: Mike Schore <[email protected]>

goaway force-pushed the ms/platform-filter branch from 0663784 to 2ca901a Compare June 30, 2020 02:22

stale bot added the stale label Jul 7, 2020

goaway closed this Jul 7, 2020

rebello95 mentioned this pull request Jul 7, 2020

filters: add initial support for bridged filters #940

Merged


		envoy_package()

		envoy_cc_library(

wip: platform extension filter poc #883

wip: platform extension filter poc #883

Conversation

goaway commented Jun 5, 2020

goaway commented Jun 8, 2020

rebello95 commented Jun 8, 2020 • edited Loading

junr03 commented Jun 8, 2020

junr03 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

junr03 commented Jun 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jul 7, 2020

goaway commented Jul 7, 2020

rebello95 commented Jun 8, 2020 •

edited

Loading