filters: support internal buffering #1090

goaway · 2020-09-14T12:27:15Z

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

buildbreaker · 2020-09-16T19:55:58Z

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

+  switch (status) {
+  case Http::FilterDataStatus::Continue:
+    if (iteration_mode_ == IterationMode::Stopped) {
+      // Error: iteration must be Resumed first.


Are we planning to add an error here or will we silently fail?

Options are to fail the request or crash the app. Neither seems terribly ideal, but perhaps neither is silently failing. I hadn't made up my mind here yet.

Seems like the place for an assertion? i.e the contract is that if the FilterDataStatus is to continue it is a programming error to have the manager's iteration_mode as anything but ongoing?

Related. Can we write API docs for the programming contract that these filters have?

^ Yeah, after going through this section in it entirety I think we need some more comments about all the combinations and why we are treating them as we are.

Agreed about public documentation (there's an issue tracking it) - also this is currently captured in the planning doc I shared when we decided on this behavior. Whether and how it's a programming error is something I'm still ambivalent about. We could also generate an error response, too. Or do nothing here, and let Envoy do whatever it does when requests/responses try to forward invalid state. I'm going to make this a TODO I think for now.

buildbreaker · 2020-09-16T19:57:42Z

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

+    break;
+  case Http::FilterDataStatus::StopIterationAndBuffer:
+    if (iteration_mode_ == IterationMode::Stopped) {
+      // Data has already have been buffered.


Do we use the no buffer variant here because the data is being buffered in this filter?

Yes, that's correct.

junr03

Some comments!

library/common/buffer/utility.h

junr03 · 2020-09-17T21:40:00Z

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

@@ -29,6 +29,8 @@ class PlatformBridgeFilterConfig {

 typedef std::shared_ptr<PlatformBridgeFilterConfig> PlatformBridgeFilterConfigSharedPtr;

+enum class IterationMode { Ongoing, Stopped };


nit: The filter manager in envoy defines this as IterationState. Should we keep the same language here?

Similarly internally Ongoing/Continue?

Given that we now have a Resume status, I felt Ongoing was a bit clearer. Iteration will be ongoing or iteration will be stopped. I'm fine with IterationState though.

junr03 · 2020-09-17T21:43:26Z

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

+    internal_buffer->move(data);
+    in_data = Buffer::Utility::copyToBridgeData(*internal_buffer);
+  } else {
+    in_data = Buffer::Utility::copyToBridgeData(data);


maybe this will become clear later, but why is this a copy rather than a drain like before?

When the buffer is unmodified, we shouldn't be draining Envoy's buffer. Per our discussed API, we now explicitly disallow the buffer to be modified in any state other than Continue or Resume.

There are also other ways we could potentially handle this, but for now, this is expedient and has no real downside since transform was doing a copy anyways.

junr03 · 2020-09-17T21:46:00Z

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

+  switch (status) {
+  case Http::FilterDataStatus::Continue:
+    if (iteration_mode_ == IterationMode::Stopped) {
+      // Error: iteration must be Resumed first.


Seems like the place for an assertion? i.e the contract is that if the FilterDataStatus is to continue it is a programming error to have the manager's iteration_mode as anything but ongoing?

Related. Can we write API docs for the programming contract that these filters have?

junr03 · 2020-09-17T21:47:09Z

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

+    break;
+  case Http::FilterDataStatus::StopIterationAndBuffer:
+    if (iteration_mode_ == IterationMode::Stopped) {
+      // Data has already have been buffered.


Suggested change

// Data has already have been buffered.

// Data has already have been buffered by the PlatformBridgeFilter.

Just want something more explicit about whose responsibility it is to buffer

junr03 · 2020-09-17T21:48:55Z

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

+    iteration_mode_ = IterationMode::Stopped;
+    break;
+  default:
+    PANIC("unsupported status for platform filters");


convention is to use NOT_REACHED_GCOVR_EXCL_LINE. You can leave a comment above why the default should not be reached.

Actually, the default can be reached - we haven't enumerated all statuses Envoy defines, and a truly strange implementation of a filter could try to break things here. I think the assertion is maybe warranted?

@goaway, under the hood NOT_REACHED_GCOVR_EXCL_LINE panics. It doesn't mean that it cannot be reached, it means that if it reaches it is an error.

^ bump on this

junr03 · 2020-09-17T21:52:39Z

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

  // 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());


hmmm were we double draining before? Once in toBridge data and once here?

Yes, we were double draining before. Which was benign, but not ideal.

junr03 · 2020-09-17T21:53:22Z

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

+  switch (status) {
+  case Http::FilterDataStatus::Continue:
+    if (iteration_mode_ == IterationMode::Stopped) {
+      // Error: iteration must be Resumed first.


^ Yeah, after going through this section in it entirety I think we need some more comments about all the combinations and why we are treating them as we are.

goaway · 2020-09-18T19:00:02Z

@junr03 I tried to make comments a little bit more explicit. Let me know if you have other suggestions. In general, I agree on the need for holistic documentation of all this, but I do think the tests ultimately also make explicit the intended behavior.

junr03 · 2020-09-18T21:51:26Z

I do think the tests ultimately also make explicit the intended behavior.

Yeah that helps internally. I also worry about platform filter authors. But happy to see in a subsequent PR per your other comment

junr03 · 2020-09-18T21:55:48Z

Looks like ASAN is still failing on the latter two tests. Just need to release the received data in the callback I think

goaway · 2020-09-18T22:35:57Z

@junr03 I actually am releasing it. Not sure why it's still failing.

junr03

Thanks for the comments and the todo. Just one small bump on the panic, otherwise lgtm.

junr03 · 2020-09-21T19:46:16Z

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

+    iteration_mode_ = IterationMode::Stopped;
+    break;
+  default:
+    PANIC("unsupported status for platform filters");


^ bump on this

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

Base automatically changed from ms/pbf-unit to main September 14, 2020 18:49

goaway force-pushed the ms/filter-buffering branch 4 times, most recently from c78828f to c885cb7 Compare September 16, 2020 08:50

goaway marked this pull request as ready for review September 16, 2020 11:25

goaway requested a review from junr03 September 16, 2020 18:22

buildbreaker reviewed Sep 16, 2020

View reviewed changes

junr03 reviewed Sep 17, 2020

View reviewed changes

junr03 previously approved these changes Sep 21, 2020

View reviewed changes

goaway added 10 commits September 21, 2020 19:44

filters: pass aggregate buffered data to platform

b085bc0

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

fix test

7ad8910

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

add bidirectional support

b63df6d

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

add bidirectional coverage

8743a9c

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

format

9d144bf

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

fix asan

b6600c8

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

change mode to state

a73cda4

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

clarify some comments per feedback

975c3d8

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

fix typo

8c528dd

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

add missing release calls

3ffd978

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

goaway dismissed junr03’s stale review via 3ffd978 September 22, 2020 02:44

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

kick ci

d1d8f67

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

goaway merged commit 8cec75c into main Sep 22, 2020

goaway deleted the ms/filter-buffering branch September 22, 2020 04:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filters: support internal buffering #1090

filters: support internal buffering #1090

goaway commented Sep 14, 2020 •

edited

Loading

buildbreaker Sep 16, 2020

goaway Sep 17, 2020

junr03 Sep 17, 2020

junr03 Sep 17, 2020

goaway Sep 18, 2020

buildbreaker Sep 16, 2020

goaway Sep 17, 2020

junr03 left a comment

junr03 Sep 17, 2020

junr03 Sep 17, 2020

goaway Sep 18, 2020

junr03 Sep 17, 2020

goaway Sep 18, 2020

goaway Sep 18, 2020

junr03 Sep 17, 2020

junr03 Sep 17, 2020

junr03 Sep 17, 2020

goaway Sep 18, 2020

junr03 Sep 18, 2020

junr03 Sep 21, 2020

junr03 Sep 17, 2020

goaway Sep 18, 2020

junr03 Sep 17, 2020

goaway commented Sep 18, 2020

junr03 commented Sep 18, 2020

junr03 commented Sep 18, 2020

goaway commented Sep 18, 2020

junr03 left a comment

junr03 Sep 21, 2020

		@@ -29,6 +29,8 @@ class PlatformBridgeFilterConfig {

		typedef std::shared_ptr<PlatformBridgeFilterConfig> PlatformBridgeFilterConfigSharedPtr;

		enum class IterationMode { Ongoing, Stopped };

	// Data has already have been buffered.
	// Data has already have been buffered by the PlatformBridgeFilter.

filters: support internal buffering #1090

filters: support internal buffering #1090

Conversation

goaway commented Sep 14, 2020 • edited Loading

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 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

goaway commented Sep 18, 2020

junr03 commented Sep 18, 2020

junr03 commented Sep 18, 2020

goaway commented Sep 18, 2020

junr03 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goaway commented Sep 14, 2020 •

edited

Loading