VHost/Route-local filter configurations #605

rodaine · 2018-04-04T21:13:46Z

This patch begins addressing #540, adding per_filter_config fields to both VirtualHost and Route configs. A following PR will, as an initial PoC/example, implement vhost/route specific configs for the HTTP buffer filter.

Signed-off-by: Chris Roche <croche@lyft.com>

mattklein123

LGTM. @htuch @lizan @rshriram @kyessenov for further review.

kyessenov

LGTM

htuch · 2018-04-04T22:12:30Z

envoy/api/v2/route/route.proto

+  // The per_filter_config field can be used to provide virtual host-specific
+  // configurations for filters. The key should match the filter name, such as
+  // *envoy.buffer* for the HTTP buffer filter. Use of this field is filter
+  // specific; see the HTTP filter documentation for if and how it is utilized.


Need a link here to the docs.

htuch · 2018-04-04T22:12:38Z

envoy/api/v2/route/route.proto

+  // The per_filter_config field can be used to provide route-specific
+  // configurations for filters. The key should match the filter name, such as
+  // *envoy.buffer* for the HTTP buffer filter. Use of this field is filter
+  // specific; see the HTTP filter documentation for if and how it is utilized.


Signed-off-by: Chris Roche <croche@lyft.com>

rodaine · 2018-04-04T23:48:37Z

@htuch ref links added. LMK if there's anything else.

qiwzhang · 2018-04-05T00:48:14Z

envoy/api/v2/route/route.proto

+  // *envoy.buffer* for the HTTP buffer filter. Use of this field is filter
+  // specific; see the :ref:`HTTP filter documentation <config_http_filters>`
+  // for if and how it is utilized.
+  map<string, google.protobuf.Struct> per_filter_config = 12;


Suggest to add another map:

// map of filter name to filter config sha of SHA(seriazlized_per_filter_config).
map<string, string> per_filter_config_sha = 13;

Some filters need to pre-process the filter config. This sha can be used to skip the pre-process step if fitler_config is not changed.

if (sha != old_saved_sha) {
obj = pre_process_config(config_data);
}

The question is do we want to do this for all filters universally? I don't have strong opinion on this. The filter can do this kind of SHA cache inside the Struct.

If so, define another

message CachableFilterConfig { string sha = 1; Struct config = 2; }

And then use the message as map value.

The benefit of using a separate map is:
only the filters wanted to have this optimization need to fill in the sha map.

Even if we wanted to add the SHA map (and I'm not sure I agree it's worth the complexity), I'm not sure how it would be used unless we make the Envoy side feature a lot more complicated. Basically, since this information is delivered over RDS, it's going to be in the route table, and orthogonal from the filter. The filter will just be able to look up the proto and dynamic_cast it from the route. So I don't think any kind of SHA will really help here? What did you have in mind?

I think this doesn't belong in the initial version. We can always add it later. Also, hashing isn't that expensive. The filter can just compute the xxHash or whatevs.

Agreed. @qiwzhang can you open a new issue on the SHA stuff? Again I don't really understand how it would work at the route level in any case.

I like to clarify how per_route_config_sha can be used.

A filter needs to preprocess the per_route_config.

obj = pre_process(pre_route_config);

Such pre-processing can be slow, it should NOT be performed for each request. One way to optimize it is to save the preprocessed "obj" in some global config. When a request comes, the filter just needs to detect if per_route_config has been changed. Without "sha" map, the filter needs to serialize the proto and sha it to detect the config change. If per-route config is big, these two steps can be slow too.

Another option is to add a new function in FilterFactory

void setRouteConfig(string unique_route_id, Struct config) PURE;

This function will be called when RDS receives new route tables, This will give filter chance to preprocess per_route config and save the pre-processed obj into its global map (not pre-request).

as global_map[ route_id ] = obj;

During request, obj can be retrieved as

obj = global_map[ filter_callback->route()->unique_route_id() ];

This is better than first approach, the pre-process is done in the config thread, not at worker thread.

Agreed that, such optimization can be done in the later phase.

Yeah if we do this we need a new filter factory function. Can we please treat this as a different feature request? I would open an issue on this. We don't need this at Lyft and I would like to move this along for the basis use case.

Also, per @htuch in general you can just hash the proto which is what we do in a lot of other places.

Created envoyproxy/envoy#3008 to track this

rshriram · 2018-04-05T14:48:43Z

@rodaine I suggest adding this to weighted clusters as well, so that we can merge @yuval-k 's usecase as well and we can merge the two implementations (envoyproxy/envoy#3003)

mattklein123 · 2018-04-05T16:43:32Z

@rshriram agreed, if we already need this per the other PR, let's just add it to weighted clusters now.

yuval-k · 2018-04-05T17:44:56Z

If we add per_filter_config to route action and weighted clusters, how will we access it from the filter code? will it be exposed on decoder_callbacks_.route()->perFilterConfig() ?

I know this is an implementation detail and the reason I'm bringing this up is because I think it presents a trade-off:

if we expose it in route()->perFilterConfig I think it means (and please correct me if I'm wrong here) that we will have to allocate a new route object and do protobuf merging on the request path (as now the route has unique properties per request).
if on the other hand we expose it in route()->routeEntry()->perFilterConfig(), it will not be available on routes without RouteAction.

If we want it available on both, we might want to consider different names to avoid confusion.

Signed-off-by: Chris Roche <croche@lyft.com>

rodaine · 2018-04-05T18:53:06Z

@rshriram added to ClusterWeight, PTAL

@yuval-k I haven't dug in deep enough to the implementation yet to answer your question specifically, but the field is added to the Route not RouteAction. Do we need to expose it on the Action instead? I'm guessing no since it's one-to-one, and doing it on the Route captures the other action oneof options.

yuval-k · 2018-04-05T18:54:43Z

That's true for RouteAction;
The problem is mostly with ClusterWeight you want to filter to see the config for the 'chosen' cluster only.

rodaine · 2018-04-05T19:48:38Z

Talked offline with @yuval-k to clarify the implementation plan. TL;DR: the per_filter_config values will be resolved once on boot/RDS payload into proto configs defined by the target filter's factory (essentially a new RDS-specific createEmptyConfigProto method). The config messages will be available from within the filter by querying the route/vhost/w.cluster for the config by filter name.

htuch · 2018-04-05T19:54:35Z

@rodaine @yuval-k have you considered merging these so we have just a single config to worry about? I think it would be best to avoid having lots of cases in filters which look like "First check if the config is in the vhost, then in the route, then in the cluster", where you mostly just want a unified effective config based on the selected vhost/route/cluster.

yuval-k · 2018-04-05T19:59:14Z

@htuch the problem I see with merging is that w.cluster (and hence its config) are available from route()->routeEntry(); while config from the route and vhost should probably live someplace else (maybe route()?) to support routes that don't have a routeEntry

mattklein123 · 2018-04-05T20:06:01Z

IMO it should be up to the filter to decide what they do w/ the vhost vs. route data. I suppose we could have a different accessor on the route which also returns already merged protos? But that is more of an Envoy side detail.

rodaine · 2018-04-05T20:33:53Z

They don't need to be mutually exclusive. We can provide the methods for folks to access the route/vhost/w.cluster configs individually at first and also offer a merged config that rolls them all up.

mattklein123 · 2018-04-05T20:41:31Z

@rodaine sorry that's exactly what I meant.

htuch · 2018-04-05T21:05:34Z

OK, that makes sense, let's go with this proposal then.

htuch · 2018-04-05T21:06:03Z

envoy/api/v2/route/route.proto

@@ -197,6 +212,13 @@ message WeightedCluster {
    // :ref:`cluster_metadata <envoy_api_field_route.RouteAction.cluster_metadata>` from the
    // enclosing :ref:`envoy_api_msg_route.RouteAction`.
    core.Metadata cluster_metadata = 7;
+


Are we going to rollback cluster_metadata?

Yes I think @yuval-k is going to do in a follow up. Right @rodaine ?

yes, That's correct; once this is merged.

htuch · 2018-04-05T21:40:36Z

@rshriram can I get an approval from your end and then we can merge?

rshriram · 2018-04-06T16:39:12Z

late ACK. Thanks for resolving this @rodaine / @yuval-k

VHost/Route-local filter configurations

c6f0904

Signed-off-by: Chris Roche <croche@lyft.com>

mattklein123 previously approved these changes Apr 4, 2018

View reviewed changes

kyessenov reviewed Apr 4, 2018

View reviewed changes

htuch suggested changes Apr 4, 2018

View reviewed changes

rodaine dismissed mattklein123’s stale review via 288b51d April 4, 2018 23:43

Add ref to HTTP filter docs

ca434c9

Signed-off-by: Chris Roche <croche@lyft.com>

lizan mentioned this pull request Apr 5, 2018

How to pass per-route service config to mixer client filter in Envoy v2 rDS istio/istio#4743

Closed

lizan approved these changes Apr 5, 2018

View reviewed changes

qiwzhang reviewed Apr 5, 2018

View reviewed changes

htuch mentioned this pull request Apr 5, 2018

router: Add clusterMetadata RouteEntry interface and implementation. envoyproxy/envoy#3003

Closed

qiwzhang mentioned this pull request Apr 5, 2018

Optimize per_route config pre-processing for filters. envoyproxy/envoy#3008

Closed

per_filter_config on weighted cluster (ClusterWeight, specifically)

133454c

Signed-off-by: Chris Roche <croche@lyft.com>

htuch closed this Apr 5, 2018

htuch reopened this Apr 5, 2018

mattklein123 approved these changes Apr 5, 2018

View reviewed changes

mattklein123 assigned mattklein123 and htuch Apr 5, 2018

htuch reviewed Apr 5, 2018

View reviewed changes

htuch approved these changes Apr 5, 2018

View reviewed changes

htuch merged commit d7769ae into envoyproxy:master Apr 6, 2018

yuval-k mentioned this pull request Apr 6, 2018

Remove cluster_metadata #611

Merged

rodaine deleted the route-local-config branch April 6, 2018 17:55

rodaine mentioned this pull request Apr 10, 2018

router: vhost/route/w.cluster local filter configuration envoyproxy/envoy#3045

Merged

VHost/Route-local filter configurations #605

VHost/Route-local filter configurations #605

Uh oh!

Conversation

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

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

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

rodaine commented Apr 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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 Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

rshriram commented Apr 5, 2018

Uh oh!

mattklein123 commented Apr 5, 2018

Uh oh!

yuval-k commented Apr 5, 2018

Uh oh!

rodaine commented Apr 5, 2018

Uh oh!

yuval-k commented Apr 5, 2018

Uh oh!

rodaine commented Apr 5, 2018

Uh oh!

htuch commented Apr 5, 2018

Uh oh!

yuval-k commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Apr 5, 2018

Uh oh!

rodaine commented Apr 5, 2018

Uh oh!

mattklein123 commented Apr 5, 2018

Uh oh!

htuch commented Apr 5, 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!

htuch commented Apr 5, 2018

Uh oh!

rshriram commented Apr 6, 2018

Uh oh!

Uh oh!

rodaine commented Apr 4, 2018 •

edited

Loading

lizan Apr 5, 2018 •

edited

Loading

qiwzhang Apr 5, 2018 •

edited

Loading

yuval-k commented Apr 5, 2018 •

edited

Loading