filters: support new platform ResumeIteration status #1100

goaway · 2020-09-16T13:13:13Z

Description: Adds support for resuming iteration to platform filters via a new *ResumeIteration filter status. The API for platform filters requires all HTTP entities that should be forwarded to be attached to a result status of a filter invocation. When iteration is stopped, previous entities that were not forwarded may now be attached to the special ResumeIteration status, e.g., pending headers might be attached to ResumeIteration returned during an on-data invocation. See code documentation for additional detail. Full API documentation will be included in an upcoming PR.
Risk Level: Moderate
Testing: Unit tests

Signed-off-by: Mike Schore mike.schore@gmail.com

Description: Adds internal buffering support for platform filters. Filters may now utilize Envoy's internal buffering by returning the status StopIterationAndBuffer. The complete buffer will be passed on subsequent on*Data invocations until iteration is resumed. This change does not support modifying the buffer (support coming in #1100). Risk Level: Moderate Testing: Unit coverage, local and CI Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway · 2020-09-22T14:40:31Z

Will be adding additional coverage, but implementation is ready for review.

junr03

some initial questions about the core.

I actually would like @rebello95 and @buildbreaker to review this, as we are getting to more and more complicated implementations and I'd like all of us to understand.

junr03 · 2020-09-23T18:49:52Z

library/common/extensions/filters/http/platform_bridge/c_type_definitions.h

+// Creating a new return status like this is brittle, but at least somewhat more resilient to
+// a new status being added in Envoy, since it won't overlap as long as the new status is added
+// rather than prepended.
+const envoy_filter_headers_status_t kEnvoyFilterHeadersStatusResumeIteration =


Sorry, at first glance I am a little confused about this status vs continue, which in envoy has the semantics of continuing, but also resuming if previously stopped. Perhaps it will become clear as I read.

After looking at onData, I am still confused why we need two different statuses rather than driving difference via the presence of absence of headers to replace (or headers/data in onTrailers). I might just be being dense, but I wonder if we can eliminate the need of another status to support the use case?

If we do need the new status, can we write more documentation about it in the filter.cc file?

I think I see? In continue iteration the buffer copied is only the current frame, but in resume iteration it is the entire internal buffer?

@junr03 I think it will be helpful for you to reference the API doc we put together back when we discussed our implementation for advanced filters. It lays out differences between Envoy's native filters and the platform filters. Also the docstrings on the platform implementations of these APIs contain some detail as well, e.g.
https://github.com/lyft/envoy-mobile/blob/main/library/swift/src/filters/AsyncRequestFilter.swift
https://github.com/lyft/envoy-mobile/blob/main/library/swift/src/filters/FilterResumeStatus.swift

Sorry actually that's the other side of the coin - I mixed up the PRs because I'm working on the asynchronous side right now. For the synchronous side there's this (e.g.):
https://github.com/lyft/envoy-mobile/blob/main/library/swift/src/filters/FilterDataStatus.swift
Because iteration may have been stopped on headers, we need a mechanism for the platform filter to still inform Envoy of what modifications, if any, it wants to perform on headers when iteration is resumed. Same for headers and data, when resumption occurs on trailers.

Part of the reason to have two statuses is that it keeps things simple for the main/golden path. When all you're doing is continuing, you never need to worry about sending previous entities.

junr03 · 2020-09-23T18:51:41Z

library/common/extensions/filters/http/platform_bridge/BUILD

@@ -10,7 +10,7 @@ api_proto_package()
 envoy_cc_library(
    name = "platform_bridge_filter_lib",
    srcs = [
-        "c_types.cc",
+        "c_type_definitions.h",


Why change this file to another header?

It's not possible to switch on externed constants without compile-time visibility into their defined values. Making this a header allows me to import their actual definitions as part of the only file that needs them at compile time - the filter implementation.

junr03 · 2020-09-23T18:54:39Z

library/common/extensions/filters/http/platform_bridge/filter.cc

@@ -56,6 +57,16 @@ void PlatformBridgeFilter::onDestroy() {
  platform_filter_.instance_context = nullptr;
 }

+void PlatformBridgeFilter::replaceHeaders(Http::HeaderMap& headers, envoy_headers c_headers) {


I think we should put this in the header utility.

The standard request/response path produces new headers; this is the only place we modify them, and moreover, need to modify them in place. I guess I'm not opposed to it, but I don't see an obvious use for it elsewhere at the moment.

junr03 · 2020-09-23T18:58:12Z

library/common/extensions/filters/http/platform_bridge/filter.cc

-  return onData(data, end_stream, internal_buffer, platform_filter_.on_response_data);
+  auto status = onTrailers(trailers, internal_buffer, &pending_request_headers_,
+                           platform_filter_.on_request_trailers);
+  if (status == Http::FilterTrailersStatus::StopIteration) {


I am confused who would flush this, since a particular filter should not have any calls past trailers?

Filters can stop iteration on trailers and still asynchronously resume iteration later. Consider for instance a filter that buffers an entire request, then kicks off some async side request, then resumes iteration when that side request completes. (We actually internally have some examples of exactly this.)

junr03 · 2020-09-23T19:01:04Z

library/common/extensions/filters/http/platform_bridge/filter.cc

+    return Http::FilterHeadersStatus::StopIteration;
+
+  default:
+    PANIC("invalid filter state: unsupported status for platform filters");


Can we change this to NOT_REACHED_GCOVR_EXCL_LINE? We can add a new macro for NOT_REACHED that takes a string if we want to modify the message. Same with everywhere else.

Is that strictly correct though? I mean this could be reached - we receive the value from a public API.

junr03 · 2020-09-23T19:01:58Z

library/common/extensions/filters/http/platform_bridge/filter.cc

-  case Http::FilterDataStatus::Continue:
+
+  switch (result.status) {
+  case kEnvoyFilterDataStatusContinue:
    if (iteration_state_ == IterationState::Stopped) {


Instead of having the if and the panic we could just RELEASE_ASSERT(iteration_state_ != IterationState::Stopped, msg)

Yes, you're absolutely right.

junr03 · 2020-09-23T19:04:38Z

library/common/extensions/filters/http/platform_bridge/filter.cc

-    data.drain(data.length());
-    data.addBufferFragment(*Buffer::BridgeFragment::createBridgeFragment(result.data));
+  case kEnvoyFilterDataStatusResumeIteration:
+    if (iteration_state_ != IterationState::Stopped) {


same as above with the release assert

junr03 · 2020-09-23T19:16:19Z

library/common/extensions/filters/http/platform_bridge/filter.cc

+      PANIC("invalid filter state: ResumeIteration  may only be used when filter iteration is "
+            "stopped");
+    }
+    if (result.extra_headers) {


Why are extra headers only allowed here?

They're only allowed where pending headers might still exist. That means onData or onTrailers (or async onResume) when iteration has been previously stopped.

rebello95

Looks good to me after adding inline documentation about behavior (similar to what we have on the platform layer) and addressing Jose's other comments.

rebello95 · 2020-09-25T23:55:44Z

library/common/extensions/filters/http/platform_bridge/c_types.h

+  envoy_headers* extra_headers;
+  envoy_data* extra_data;


I think adding inline documentation as to how these are used (similarly to how the values are documented on the platform layer's interfaces) would be helpful for future reference when reading this code

Inline documentation coming.

library/common/extensions/filters/http/platform_bridge/filter.cc

rebello95 · 2020-09-26T00:00:41Z

library/common/extensions/filters/http/platform_bridge/filter.cc

+    internal_buffer->addBufferFragment(*Buffer::BridgeFragment::createBridgeFragment(result.data));
+    return Http::FilterDataStatus::Continue;
+
+  default:


I think it's worth having this be exhaustive so that if upstream Envoy adds a new iteration state Envoy Mobile won't compile

Unfortunately we won't be able to get these compile-time guarantees here regardless. C/C++ don't enforce this, and static analysis would miss it too, since we're iterating over a constant set only partially defined in terms of the internal enum (ResumeIteration doesn't map to anything internal).

library/common/extensions/filters/http/platform_bridge/filter.cc

rebello95 · 2020-09-26T00:03:58Z

library/common/extensions/filters/http/platform_bridge/filter.cc

-                                                              bool end_stream) {
-  // Delegate to shared implementation for request and response path.
-  return onHeaders(headers, end_stream, platform_filter_.on_response_headers);
+  return onData(data, end_stream, internal_buffer, &pending_response_headers_,


Is passing a null buffer here acceptable? (I assume yes)

Yes, but good eye for catching the possibility. Here, nullptr represents "none".

rebello95 · 2020-09-26T00:05:49Z

library/common/extensions/filters/http/platform_bridge/filter.h

+  Http::HeaderMap* pending_request_headers_{};
+  Http::HeaderMap* pending_response_headers_{};
+  Http::HeaderMap* pending_request_trailers_{};
+  Http::HeaderMap* pending_response_trailers_{};


I actually don't see that these references are being cleared - are we assuming they'll be automatically released when the filter is deallocated and that's sufficient?

These member pointers are all being initialized to nullptr. This syntax is idiomatic (though it's also newer and perhaps less obvious to an unfamiliar observer than = nullptr;). When they're set to something else in the implementation, they're being set to point to an object they don't manage - something else owns and will clean up that object. (This may be inferred by the type signatures that provide those objects.) The actual space the pointer itself occupies will be automatically reclaimed when the object is destructed.

(Don't worry - these details and their syntax are not all that intuitive.)

library/objective-c/EnvoyBridgeUtility.h

goaway · 2020-09-28T12:03:07Z

Thanks for the review @rebello95!

… HTTP entities Signed-off-by: Mike Schore <mike.schore@gmail.com>

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway force-pushed the ms/filters-resumption branch from 3691127 to e9f63f8 Compare September 18, 2020 13:55

goaway changed the base branch from main to ms/filter-buffering September 18, 2020 13:56

goaway force-pushed the ms/filters-resumption branch from e9f63f8 to 58e21e9 Compare September 18, 2020 13:57

goaway force-pushed the ms/filter-buffering branch from 5499521 to 3ffd978 Compare September 22, 2020 02:44

goaway mentioned this pull request Sep 22, 2020

filters: support internal buffering #1090

Merged

Base automatically changed from ms/filter-buffering to main September 22, 2020 04:52

goaway force-pushed the ms/filters-resumption branch from 58e21e9 to 6bf647a Compare September 22, 2020 05:06

goaway marked this pull request as ready for review September 22, 2020 14:38

goaway requested a review from junr03 September 22, 2020 14:40

goaway force-pushed the ms/filters-resumption branch from b1be28b to 6c9c47c Compare September 22, 2020 14:56

junr03 reviewed Sep 23, 2020

View reviewed changes

rebello95 previously approved these changes Sep 26, 2020

View reviewed changes

goaway added 10 commits September 29, 2020 08:41

filters: add support for resuming iteration with mutations to pending…

6ff3659

… HTTP entities Signed-off-by: Mike Schore <mike.schore@gmail.com>

compiles and passes tests

5e6eef2

Signed-off-by: Mike Schore <mike.schore@gmail.com>

syntax and format fix

5c012fa

Signed-off-by: Mike Schore <mike.schore@gmail.com>

ios fixes

3d4caa0

Signed-off-by: Mike Schore <mike.schore@gmail.com>

format

6b6e1a2

Signed-off-by: Mike Schore <mike.schore@gmail.com>

swiftlint

aaf2a88

Signed-off-by: Mike Schore <mike.schore@gmail.com>

swiftlint 2

6afe9d6

Signed-off-by: Mike Schore <mike.schore@gmail.com>

change some panics to release assertions

e0914ab

Signed-off-by: Mike Schore <mike.schore@gmail.com>

use NULL

36e55fd

Signed-off-by: Mike Schore <mike.schore@gmail.com>

change 'extra' to 'pending' and add comments

09088e7

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway dismissed rebello95’s stale review via 09088e7 September 29, 2020 01:22

goaway force-pushed the ms/filters-resumption branch from 88b0eaa to 09088e7 Compare September 29, 2020 01:22

goaway added 3 commits September 28, 2020 20:14

additional inline comments

6ce7113

Signed-off-by: Mike Schore <mike.schore@gmail.com>

add tests

bf45332

Signed-off-by: Mike Schore <mike.schore@gmail.com>

add tests

954ee81

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway added 6 commits September 29, 2020 21:07

format

b6afb52

Signed-off-by: Mike Schore <mike.schore@gmail.com>

update variable comment

570121d

Signed-off-by: Mike Schore <mike.schore@gmail.com>

add tests

98d8cee

Signed-off-by: Mike Schore <mike.schore@gmail.com>

add tests

1777827

Signed-off-by: Mike Schore <mike.schore@gmail.com>

format

154df83

Signed-off-by: Mike Schore <mike.schore@gmail.com>

fix asan failures in tests

ed82982

Signed-off-by: Mike Schore <mike.schore@gmail.com>

buildbreaker previously approved these changes Sep 30, 2020

View reviewed changes

fix tests for asan

606505c

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway dismissed buildbreaker’s stale review via 606505c September 30, 2020 11:39

format

c9a8028

Signed-off-by: Mike Schore <mike.schore@gmail.com>

goaway merged commit 1e6eeae into main Sep 30, 2020

goaway deleted the ms/filters-resumption branch September 30, 2020 13:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filters: support new platform ResumeIteration status #1100

filters: support new platform ResumeIteration status #1100

goaway commented Sep 16, 2020 •

edited

Loading

goaway commented Sep 22, 2020

junr03 left a comment

junr03 Sep 23, 2020

junr03 Sep 23, 2020

junr03 Sep 23, 2020

goaway Sep 23, 2020

goaway Sep 23, 2020

goaway Sep 23, 2020

junr03 Sep 23, 2020

goaway Sep 23, 2020

junr03 Sep 23, 2020

goaway Sep 23, 2020

junr03 Sep 23, 2020

goaway Sep 23, 2020

junr03 Sep 23, 2020

goaway Sep 23, 2020

junr03 Sep 23, 2020

goaway Sep 23, 2020

junr03 Sep 23, 2020

junr03 Sep 23, 2020

goaway Sep 23, 2020

rebello95 left a comment

rebello95 Sep 25, 2020

goaway Sep 28, 2020

rebello95 Sep 26, 2020

goaway Sep 30, 2020

rebello95 Sep 26, 2020

goaway Sep 28, 2020

rebello95 Sep 26, 2020

goaway Sep 28, 2020

goaway commented Sep 28, 2020

filters: support new platform ResumeIteration status #1100

filters: support new platform ResumeIteration status #1100

Conversation

goaway commented Sep 16, 2020 • edited Loading

goaway commented Sep 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebello95 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

goaway commented Sep 28, 2020

goaway commented Sep 16, 2020 •

edited

Loading