s/RDS/Route/ ?
The route config can come from static config as well.

+1

revert this file other than this line? those seems unrelated.

the vscode bazel plugin seems a bit overzealous in its linting...

I don't think you need the first template parameter, it is always Struct.

was being consistent with the other translate function but will update!

Need to perform validation when PerFilterConfigs are loaded too.

I don't think we can do this unfortunately. What type would we validate? (This is related to me asking very nicely for Validate() to be a method on the base proto message which is virtual and empty and can be overriden...

I meant Validate above, but on second thought, we need a method in Factory to pre-process the route-config at loading time, not in the data path. Currently in this PR there is no way to do pre-processing of the filter config in routes. (for configs in listeners, it is done in createFilterFactoryFromProto)

Pre-processing is being treated as an additional feature. That will need more thinking. We won't be doing that as part of this PR.

we'll likely need to delegate this to the factory(factory) where we already ask it for the pb message. will think about this some more!

super nit: "the nullptr" is not typically said. Just "nullptr." Same below.

thought it sounded funny. Thanks, the @mattklein123!

+1

I would just RELEASE_ASSERT() this like I recently did for the other instances of this.

I was being consistent with the translate method above. Shall I update both?

Yes please. I don't think it's really needed to throw an exception here.

In general we try to not have common depend on server, though this isn't really enforced explicitly. I would add a TODO here and in code to figure out a better way to break this cycle.

sg. i imagine this could be done with bazel visibility rules?

I need to think about it. Will look at it as part of repo reorg work.

I don't think we can do this unfortunately. What type would we validate? (This is related to me asking very nicely for Validate() to be a method on the base proto message which is virtual and empty and can be overriden...

s/ASSERT/EXPECT in all places for the most part.

This has been moved and will need a master merge.

Instead of using buffer filter, can you register a test filter for this test, so that you can correctly verify that you can override to be a different type for the filter data? If you need to you can have a proto just for testing.

is there an existing one I can use for this or should I create one? Should I create the test proto in envoy or data plane?

Just make one local to the test.

This suggestion is trying to address this issue: #3008
We can do it in a separate PR. But it is so simple, you can also put in in this PR too.

In these VHost/Route/Cluster, add another function

// If perFitlerConfig exists for this filter, get its config hash value to detect if config is updated.
// return 0 if no config for the filter.
size_t perFitlerConfigHash(const std::string& name) const PURE;

I don't quite understand how this will help you. In the filter are you going to check this on a per-route basis? What you really want is another callback which allows a filter to update global structures on a route update.

This is to help filters need to pre-process per_route_config. per_route_config can be dynamically changed anytime. But you don't want to pre-process the config for each request. This hash will help to detect config change, and skip pre_processing step:

This is how it can be used, For each request in decodeHeader()

auto hash = route()->perFilterConfigHash("my_filter_name");
if (hash == 0) // handle not per_route_config case

 obj =  global_obj_map.find(hash);
 if (obj == null) {
         obj = pre_process_config( route()->perFilterConfig("my_filter_name") );
         global_obj_map[hash] = obj;
 }

This will also achieve: different routes with same config can share the same pre_processed object.

This is not how I would recommend implementing this. (The above is not thread safe.). I have a pretty good idea of how to efficiently implement this and give you the functionality you need. Let's do this in a follow up.

In istio mixer-client filter, the "global_map" actually is per_thread_global_map. so this approach works for us.
the filter_factory callback approach is better, the global_map can be used by all worker threads. But it is not easy for Istio to use it since its "object_map" object is way deep inside a per_thread object.
My request is: can we support both: 1) calculate the hash of per_route_config at config update time and filter can access it. 2) implements factory callback to support a config change update.

create another map

config_hash_ to store hash of config

The hash can be calculated by
https://github.com/envoyproxy/envoy/blob/master/source/common/protobuf/utility.h#L127

Same comment as above. I don't think this is very helpful and I would rather sort out filter update callbacks as a separate feature?

I would use EXPECT_THROW_WITH_MESSAGE here for better checking. Same elsewhere if applicable.

nit: I think this can just be stack allocated with the test? Do you need a unique_ptr?

It's a bit self-referential, so I was having issues creating it in the constructor, but probably can do it in the setup. Let me give that a shot.

Ended up just splitting the filter into a nested class of the fixture

nit: This would be a better test if you had a full route config that got parsed that then resulted in the exception.

nit: I would just use the ternary operator here to collapse the rest of these lines into one.