filters: add initial support for bridged filters #940

goaway · 2020-07-07T03:07:15Z

Description: Adds support for platform-bridged filter implementations at the common and bridge layers of the library. Includes a stub implementation hardcoded into configuration. Dynamic configuration and data support, with platform-specific presentations, to come.
Risk Level: Moderate
Testing: Local end-to-end

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

goaway · 2020-07-07T03:08:26Z

~~Note: this requires upstream Envoy update, and will need to be rebased when that merges. See: #923~~

goaway · 2020-07-07T03:10:15Z

~~Note: pending completed stub implementation.~~

rebello95

LGTM overall, some comments (and it'd be great if we could add tests)

library/common/api/external.h

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

rebello95 · 2020-07-07T17:50: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: PlatformStub


Same comment from #883 (comment) applies

(Same as below)
This particular part won't ever really be publicly exposed/used. But I do intend to make a documentation PR that fully describes how to create and use platform extensions.

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

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

library/common/extensions/filters/http/platform_extension/config.h

rebello95 · 2020-07-07T20:22:33Z

library/common/extensions/filters/http/platform_extension/filter.proto

+message Bridging {
+  string name = 1 [(validate.rules).string.min_bytes = 1];
+}


Can we document how this works/is intended to be used?

This particular part won't ever really be publicly exposed/used. But I do intend to make a documentation PR that fully describes how to create and use platform extensions.

junr03

nice! Some comments

envoy_build_config/BUILD

envoy_build_config/extensions_build_config.bzl

junr03 · 2020-07-07T22:30:41Z

library/common/api/external.cc

+
+namespace Envoy {
+namespace Api {
+namespace External {


Suggested change

namespace External {

namespace PlatformExtension {

I think this meshes well with the rest of the PR, because it tells us that these function signatures allow us to register any platform extension api?

Sure - it may be obvious that I waffled a bit on naming and "external" was just a possibility I considered, too.

Now that I've updated everything else to standardize on PlatformBridge, I actually think this should remain External. Any API external to the library could be registered here, but everything labeled PlatformBridge relates specifically to bridging up to a platform runtime environment.

library/common/api/external.cc

library/common/api/external.h

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

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

goaway · 2020-07-08T09:05:08Z

This update adds an optimized path for no-op filter callbacks, as well as allowing calloc to trivially create a no-op filter - utilized here as a PlatformStub implementation.

It also factors out common callback logic on the request and response path for headers.

goaway · 2020-07-08T09:07:05Z

With this the PR should be functionally green, once upstream can be updated.

rebello95

Great work! 2 small call outs, but feel free to merge so we can iterate on main!

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

library/common/types/c_types.h

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

goaway · 2020-07-15T03:11:16Z

Finally rebased on upstream Envoy update.

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

goaway · 2020-07-15T04:34:59Z

CI is green; proceeding with merge (with previous approvals).

@junr03

Description: Missed this request from @junr03 on #940. Risk Level: Low Testing: Local and CI Signed-off-by: Mike Schore <[email protected]>

goaway mentioned this pull request Jul 7, 2020

wip: platform extension filter poc #883

Closed

rebello95 reviewed Jul 7, 2020

View reviewed changes

junr03 reviewed Jul 7, 2020

View reviewed changes

This was referenced Jul 8, 2020

envoy: update upstream for multiple fixes #923

Merged

filters: add support for platform filters bridged into Envoy's native filter chain #942

Closed

goaway changed the base branch from master to main July 8, 2020 21:52

rebello95 previously approved these changes Jul 13, 2020

View reviewed changes

library/common/extensions/filters/http/platform_bridge/c_types.h Outdated Show resolved Hide resolved

library/common/types/c_types.h Show resolved Hide resolved

goaway dismissed rebello95’s stale review via de6f080 July 13, 2020 19:40

rebello95 previously approved these changes Jul 13, 2020

View reviewed changes

goaway dismissed rebello95’s stale review via 6483bdd July 14, 2020 03:10

goaway force-pushed the ms/bridge-filter branch from de6f080 to 6483bdd Compare July 14, 2020 03:10

goaway added 10 commits July 15, 2020 11:08

filters: add initial support for bridged (header) filters

84b561a

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

refactor BridgeFilter and add PlatformStub implementation

edb56dc

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

updates for comments

827ccb8

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

update comment

9e0c3a8

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

standardize naming to PlatformBridgeFilter

7b9983c

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

updates for comments

c46d9b1

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

update api retrieval to occur during config loading

bf578af

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

formatting

0e5196d

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

add link to optimization issue

fa84b3c

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

minor update for comments

3af5dd4

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

goaway force-pushed the ms/bridge-filter branch from 6483bdd to 3af5dd4 Compare July 15, 2020 03:10

goaway added 2 commits July 15, 2020 11:17

fix envoy ref

f431456

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

fix format checker

9d31150

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

goaway merged commit f87359c into main Jul 15, 2020

goaway deleted the ms/bridge-filter branch July 15, 2020 04:41

goaway mentioned this pull request Jul 15, 2020

filters: rename config property #955

Merged

goaway added a commit that referenced this pull request Jul 17, 2020

filters: rename config property (#955)

0b7ed82

Description: Missed this request from @junr03 on #940. Risk Level: Low Testing: Local and CI Signed-off-by: Mike Schore <[email protected]>

This was referenced Aug 5, 2020

docs: generate documentation from protos #1001

Open

coverage: is not reporting failure #1009

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

filters: add initial support for bridged filters #940

filters: add initial support for bridged filters #940

goaway commented Jul 7, 2020

goaway commented Jul 7, 2020 •

edited

Loading

goaway commented Jul 7, 2020 •

edited

Loading

rebello95 left a comment

rebello95 Jul 7, 2020

goaway Jul 8, 2020

rebello95 Jul 7, 2020

goaway Jul 8, 2020

junr03 left a comment

junr03 Jul 7, 2020

goaway Jul 8, 2020

goaway Jul 10, 2020

goaway commented Jul 8, 2020

goaway commented Jul 8, 2020

rebello95 left a comment

goaway commented Jul 15, 2020

goaway commented Jul 15, 2020

filters: add initial support for bridged filters #940

filters: add initial support for bridged filters #940

Conversation

goaway commented Jul 7, 2020

goaway commented Jul 7, 2020 • edited Loading

goaway commented Jul 7, 2020 • edited Loading

rebello95 left a comment

Choose a reason for hiding this comment

rebello95 Jul 7, 2020

Choose a reason for hiding this comment

goaway Jul 8, 2020

Choose a reason for hiding this comment

rebello95 Jul 7, 2020

Choose a reason for hiding this comment

goaway Jul 8, 2020

Choose a reason for hiding this comment

junr03 left a comment

Choose a reason for hiding this comment

junr03 Jul 7, 2020

Choose a reason for hiding this comment

goaway Jul 8, 2020

Choose a reason for hiding this comment

goaway Jul 10, 2020

Choose a reason for hiding this comment

goaway commented Jul 8, 2020

goaway commented Jul 8, 2020

rebello95 left a comment

Choose a reason for hiding this comment

goaway commented Jul 15, 2020

goaway commented Jul 15, 2020

goaway commented Jul 7, 2020 •

edited

Loading

goaway commented Jul 7, 2020 •

edited

Loading