Add perFilerConfigObject to store pre-processed per-route-config by qiwzhang · Pull Request #3128 · envoyproxy/envoy

qiwzhang · 2018-04-18T18:22:53Z

Description:

Add a new function in filter_factory to allow filter to generate a pre-processed object for per-filter-config.
In VHost, Route, and RouteEntry, add a new function perFilterConfigObject to retrieve this pre-processed object.

Risk Level: Low

qiwzhang · 2018-04-18T18:33:50Z

@mattklein123 @rodaine, could you help to review this? Thanks

ggreenway · 2018-04-18T18:38:31Z

include/envoy/router/router.h

I think this can be named "PerRouteConfig", or maybe "RouteSpecificConfig". Or "RouteSpecificFilterConfig".

I would vote for "RouteSpecificFilterConfig" but I don't feel that strongly about it.

Now, this is going to be painful isn't it? Every filter will now have two config objects in some sense. a preprocessed per-route thing and a filter level config object. I guess thats okay, given that the perf benefit

Every filter is basically doing this today, and it's entirely optional, so it seems OK to me?

fair enough.

ggreenway · 2018-04-18T18:39:32Z

include/envoy/server/filter_config.h

Style: delete this line, change previous line to createPerRouteConfigObject(const Protobuf::Message&) {

ggreenway · 2018-04-18T18:43:44Z

source/common/router/config_impl.cc

style: methods get camelCase, not snake_case

ggreenway · 2018-04-18T18:44:21Z

source/common/router/config_impl.cc

nit: I'd name this "it" or "iterator"

ggreenway · 2018-04-18T18:46:56Z

test/common/router/config_impl_test.cc

std::make_shared();

ggreenway · 2018-04-18T18:58:14Z

include/envoy/router/router.h

I think we should add an additional template method:

template <typename Derived> const Derived* perFilterConfigObjectTyped(const std::string& name) const { const PerRouteConfigObject* obj = perFilterConfigObject(name); if (obj == nullptr) { return obj; } ASSERT(dynamic_cast<const Derived*>(obj) != nullptr); return static_cast<const Derived*>(obj); }

Or similar.

Yeah we have basically this for TLS. +1

mattklein123

In general love this approach. Thanks @qiwzhang. cc @rodaine @rshriram this will allow you to trivially improve the perf of your filters? Should we wait until this lands?

I actually wonder if we should just kill the other functions and only have this type of config. If the filter just wants the proto they can just store it in a wrapper? WDYT?

mattklein123 · 2018-04-18T20:23:30Z

include/envoy/router/router.h

I would vote for "RouteSpecificFilterConfig" but I don't feel that strongly about it.

mattklein123 · 2018-04-18T20:26:52Z

include/envoy/router/router.h

Yeah we have basically this for TLS. +1

mattklein123 · 2018-04-18T20:28:27Z

source/common/router/config_impl.cc

nit: Use ternary operator here. Can you fix other cases of this? I think @rodaine might have another one that I didn't comment on.

mattklein123 · 2018-04-18T20:34:58Z

As an aside, I don't think this solution will work for Lua which is more complicated. This is because the Lua code must be JITd per-thread and can't be constant global like this data. I will think about about the Lua case but I think we can probably proceed with this as it's blocking a few things.

ggreenway · 2018-04-18T20:38:31Z

@mattklein123 for Lua, could the config object own a TLS slot, and store the Lua state in that? Might have to make this non-const, but that wouldn't substantially change the approach.

rshriram · 2018-04-18T20:50:42Z

I actually wonder if we should just kill the other functions and only have this type of config. If the filter just wants the proto they can just store it in a wrapper? WDYT?

+1 to that.

rshriram · 2018-04-18T20:52:04Z

include/envoy/router/router.h

IF we are going down the path of merging with perFilterConfig, I would kill this function and change signature of the other..

rshriram

This is nice.. as noted earlier, just change the signature of the existing function (perFilterConfig) to return the wrapper object you have instead of proto message.

mattklein123 · 2018-04-18T20:58:02Z

yeah +1 to @rshriram suggestion. We can hold on the fault/buffer PRs until we finish this.

for Lua, could the config object own a TLS slot, and store the Lua state in that? Might have to make this non-const, but that wouldn't substantially change the approach.

Yes, that should work. The only downside to that approach is we would need a TLS slot per route which might be inefficient in the case where we have lots of routes, but, I think that's probably an edge case. IMO I would just leave the signature as const for now since the wrapper really does have to be const. If the implementation does non-const/mutable things under the hood in special cases that's probably OK...

rodaine

Awesome! Agreed with @rshriram and @mattklein123, let's just replace the existing perFilterConfig methods.

rodaine · 2018-04-18T23:50:40Z

include/envoy/router/router.h

qiwzhang · 2018-04-19T00:55:53Z

Thanks for the review. Here is my plan:

not to add another perFilterConfigObject(), change exising perFilterConfig() to return RouteSpecificFilterConfig
In filter_factory, replace createEmptyRouteConfigProto() with

Router::RouteSpecificFilterConfigConstSharedPtr
createRouteSpecificFilterConfig(const ProtobufWkt::Struct&)

will add

templated function as
const Derived* perFilterConfigTyped()

Thanks

qiwzhang · 2018-04-19T02:11:50Z

PTAL

mattklein123 · 2018-04-19T02:37:06Z

I will review tomorrow, but @ggreenway @rshriram I don't think this by itself will work for Lua. The reason being that in order to use TLS and more complicated things, the factory will need access to the FactoryContext. @ggreenway this bring us back full circle and I think we will need to provide the full FactoryContext all the way to the route config, or think of some other cleaner callback way of getting it in.

I would still suggest we just get this in as is and we can figure out the Lua (and maybe WebSocket case after).

mattklein123

LGTM, just some small nits. I think we can sort out the Lua case later.

mattklein123 · 2018-04-19T16:18:13Z

include/envoy/router/router.h

This entire function can be simplified to:

return dynamic_cast<const Derived*>(perFilterConfig(name));

We need to do the real dynamic_cast for correctness, and it's fine to dynamic_cast nullptr. Same for the other spots.

mattklein123 · 2018-04-19T16:19:03Z

include/envoy/router/router.h

"If there is no per-filter config..." (same below)

mattklein123 · 2018-04-19T16:19:23Z

include/envoy/router/router.h

"or the filter factory returns nullptr, ..." (same below)

mattklein123 · 2018-04-19T16:20:14Z

include/envoy/server/filter_config.h

"will be stored in the loaded route configuration."

mattklein123 · 2018-04-19T16:20:50Z

include/envoy/server/filter_config.h

nit: return {} or return nullptr should work

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

mattklein123

LGTM, thanks. @ggreenway if this works for you can you approve/merge?

rshriram · 2018-04-19T19:09:10Z

Question: can we get similar functionality for tcp ?

rshriram · 2018-04-19T19:09:47Z

I don’t know what gets carried through from SNI filter chain match.

ggreenway · 2018-04-19T19:28:37Z

@rshriram for tcp, routes will be defined by the FilterChainMatch, so each route will have a completely independent filter-chain (with config) anyways, so no additional work will be needed.

lizan · 2018-04-19T23:42:44Z

include/envoy/server/filter_config.h

-  virtual ProtobufTypes::MessagePtr createEmptyRouteConfigProto() {
-    return createEmptyConfigProto();
+  virtual Router::RouteSpecificFilterConfigConstSharedPtr
+  createRouteSpecificFilterConfig(const ProtobufWkt::Struct&) {


I would strongly -1 to exposing Struct to filter interface and in favor of wrapping them in an interface like #3109 did. Hiding Struct from filter interface is important when we see scaling issues with the config and want to migrate to Any (via oneof) in future, as mentioned in https://github.com/envoyproxy/data-plane-api/issues/540#issuecomment-373250850 . cc @htuch.

ggreenway requested changes Apr 18, 2018

View reviewed changes

mattklein123 reviewed Apr 18, 2018

View reviewed changes

mattklein123 mentioned this pull request Apr 18, 2018

Route-local fault filter config #3084

Merged

rshriram reviewed Apr 18, 2018

View reviewed changes

rshriram mentioned this pull request Apr 18, 2018

Per-port Destination rules and fault injection istio/istio#5055

Merged

rodaine reviewed Apr 18, 2018

View reviewed changes

include/envoy/router/router.h Outdated

Copy link
Copy Markdown

Member

rodaine Apr 18, 2018

Choose a reason for hiding this comment

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

+1

rodaine mentioned this pull request Apr 18, 2018

buffer filter: route-local overrides and disabling #3120

Merged

qiwzhang force-pushed the per_route_obj branch from f6d4196 to 4c4cbcf Compare April 19, 2018 02:09

mattklein123 reviewed Apr 19, 2018

View reviewed changes

danielhochman assigned ggreenway and mattklein123 Apr 19, 2018

qiwzhang added 3 commits April 19, 2018 17:51

Add perFilerConfigObject to store pre-processed per-route-config

46a5d5e

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

Merge two perFilterConfig functions

fffa517

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

Update per review

af5003a

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

qiwzhang force-pushed the per_route_obj branch from 4c4cbcf to af5003a Compare April 19, 2018 17:52

mattklein123 approved these changes Apr 19, 2018

View reviewed changes

rshriram approved these changes Apr 19, 2018

View reviewed changes

ggreenway approved these changes Apr 19, 2018

View reviewed changes

ggreenway merged commit 47c63e5 into envoyproxy:master Apr 19, 2018

lizan reviewed Apr 19, 2018

View reviewed changes

This was referenced Apr 20, 2018

filter: Hide Struct from filter interface #3155

Closed

config: hide Struct from filter interface #3158

Merged

Conversation

qiwzhang commented Apr 18, 2018

Uh oh!

qiwzhang commented Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Apr 18, 2018

Uh oh!

ggreenway commented Apr 18, 2018

Uh oh!

rshriram commented Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rshriram left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Apr 18, 2018

Uh oh!

rodaine left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiwzhang commented Apr 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qiwzhang commented Apr 19, 2018

Uh oh!

mattklein123 commented Apr 19, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiwzhang commented Apr 19, 2018 •

edited

Loading

lizan Apr 19, 2018 •

edited

Loading