Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

filters: support new platform ResumeIteration status #1100

Merged
merged 21 commits into from
Sep 30, 2020
Merged

filters: support new platform ResumeIteration status #1100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
6ff3659
filters: add support for resuming iteration with mutations to pending…
goaway Sep 22, 2020
5e6eef2
compiles and passes tests
goaway Sep 22, 2020
5c012fa
syntax and format fix
goaway Sep 22, 2020
3d4caa0
ios fixes
goaway Sep 23, 2020
6b6e1a2
format
goaway Sep 23, 2020
aaf2a88
swiftlint
goaway Sep 23, 2020
6afe9d6
swiftlint 2
goaway Sep 23, 2020
e0914ab
change some panics to release assertions
goaway Sep 29, 2020
36e55fd
use NULL
goaway Sep 29, 2020
09088e7
change 'extra' to 'pending' and add comments
goaway Sep 29, 2020
6ce7113
additional inline comments
goaway Sep 29, 2020
bf45332
add tests
goaway Sep 29, 2020
954ee81
add tests
goaway Sep 29, 2020
b6afb52
format
goaway Sep 29, 2020
570121d
update variable comment
goaway Sep 29, 2020
98d8cee
add tests
goaway Sep 29, 2020
1777827
add tests
goaway Sep 29, 2020
154df83
format
goaway Sep 29, 2020
ed82982
fix asan failures in tests
goaway Sep 30, 2020
606505c
fix tests for asan
goaway Sep 30, 2020
c9a8028
format
goaway Sep 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion library/common/extensions/filters/http/platform_bridge/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ api_proto_package()
envoy_cc_library(
name = "platform_bridge_filter_lib",
srcs = [
"c_types.cc",
"c_type_definitions.h",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change this file to another header?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"filter.cc",
],
hdrs = [
Expand Down
23 changes: 21 additions & 2 deletions ...s/filters/http/platform_bridge/c_types.cc → ...http/platform_bridge/c_type_definitions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// NOLINT(namespace-envoy)
#include "library/common/extensions/filters/http/platform_bridge/c_types.h"

#include "envoy/http/filter.h"

#include "library/common/extensions/filters/http/platform_bridge/c_types.h"

const envoy_filter_headers_status_t kEnvoyFilterHeadersStatusContinue =
static_cast<envoy_filter_headers_status_t>(Envoy::Http::FilterHeadersStatus::Continue);
const envoy_filter_headers_status_t kEnvoyFilterHeadersStatusStopIteration =
Expand All @@ -13,15 +13,34 @@ const envoy_filter_headers_status_t kEnvoyFilterHeadersStatusContinueAndEndStrea
const envoy_filter_headers_status_t kEnvoyFilterHeadersStatusStopAllIterationAndBuffer =
static_cast<envoy_filter_headers_status_t>(
Envoy::Http::FilterHeadersStatus::StopAllIterationAndBuffer);
// ResumeIteration is not a status supported by Envoy itself, and only has relevance in Envoy
// Mobile's implementation of platform filters.
//
// Regarding enum values, the C++11 standard (7.2/2) states:
// If the first enumerator has no initializer, the value of the corresponding constant is zero.
// An enumerator-definition without an initializer gives the enumerator the value obtained by
// increasing the value of the previous enumerator by one.
//
// 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 =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

kEnvoyFilterHeadersStatusContinue - 1;

const envoy_filter_data_status_t kEnvoyFilterDataStatusContinue =
static_cast<envoy_filter_data_status_t>(Envoy::Http::FilterDataStatus::Continue);
const envoy_filter_data_status_t kEnvoyFilterDataStatusStopIterationAndBuffer =
static_cast<envoy_filter_data_status_t>(Envoy::Http::FilterDataStatus::StopIterationAndBuffer);
const envoy_filter_data_status_t kEnvoyFilterDataStatusStopIterationNoBuffer =
static_cast<envoy_filter_data_status_t>(Envoy::Http::FilterDataStatus::StopIterationNoBuffer);
// See comment above.
const envoy_filter_data_status_t kEnvoyFilterDataStatusResumeIteration =
kEnvoyFilterDataStatusContinue - 1;

const envoy_filter_trailers_status_t kEnvoyFilterTrailersStatusContinue =
static_cast<envoy_filter_trailers_status_t>(Envoy::Http::FilterTrailersStatus::Continue);
const envoy_filter_trailers_status_t kEnvoyFilterTrailersStatusStopIteration =
static_cast<envoy_filter_trailers_status_t>(Envoy::Http::FilterTrailersStatus::StopIteration);
// See comment above.
const envoy_filter_trailers_status_t kEnvoyFilterTrailersStatusResumeIteration =
kEnvoyFilterTrailersStatusContinue - 1;
22 changes: 22 additions & 0 deletions library/common/extensions/filters/http/platform_bridge/c_types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,16 @@

// NOLINT(namespace-envoy)

/**
* Convenience constant indicating no changes to data.
*/
extern const envoy_data envoy_unaltered_data;

/**
* Convenience constant indicating no changes to headers.
*/
extern const envoy_headers envoy_unaltered_headers;

/**
* Return codes for on-headers filter invocations. @see envoy/http/filter.h
*/
Expand All @@ -12,6 +22,9 @@ extern const envoy_filter_headers_status_t kEnvoyFilterHeadersStatusContinue;
extern const envoy_filter_headers_status_t kEnvoyFilterHeadersStatusStopIteration;
extern const envoy_filter_headers_status_t kEnvoyFilterHeadersStatusContinueAndEndStream;
extern const envoy_filter_headers_status_t kEnvoyFilterHeadersStatusStopAllIterationAndBuffer;
// Note this return status is unique to platform filters and used only to resume iteration after
// it has been previously stopped.
extern const envoy_filter_headers_status_t kEnvoyFilterHeadersStatusResumeIteration;

/**
* Compound return type for on-headers filter invocations.
Expand All @@ -28,13 +41,17 @@ typedef int envoy_filter_data_status_t;
extern const envoy_filter_data_status_t kEnvoyFilterDataStatusContinue;
extern const envoy_filter_data_status_t kEnvoyFilterDataStatusStopIterationAndBuffer;
extern const envoy_filter_data_status_t kEnvoyFilterDataStatusStopIterationNoBuffer;
// Note this return status is unique to platform filters and used only to resume iteration after
// it has been previously stopped.
extern const envoy_filter_data_status_t kEnvoyFilterDataStatusResumeIteration;

/**
* Compound return type for on-data filter invocations.
*/
typedef struct {
envoy_filter_data_status_t status;
envoy_data data;
envoy_headers* pending_headers;
} envoy_filter_data_status;

/**
Expand All @@ -43,13 +60,18 @@ typedef struct {
typedef int envoy_filter_trailers_status_t;
extern const envoy_filter_trailers_status_t kEnvoyFilterTrailersStatusContinue;
extern const envoy_filter_trailers_status_t kEnvoyFilterTrailersStatusStopIteration;
// Note this return status is unique to platform filters and used only to resume iteration after
// it has been previously stopped.
extern const envoy_filter_trailers_status_t kEnvoyFilterTrailersStatusResumeIteration;

/**
* Compound return type for on-trailers filter invocations.
*/
typedef struct {
envoy_filter_trailers_status_t status;
envoy_headers trailers;
envoy_headers* pending_headers;
envoy_data* pending_data;
} envoy_filter_trailers_status;

#ifdef __cplusplus
Expand Down
Loading