I think this field should be in RouteConfiguration, not in Route. That way, if there are many routes that use the same cluster_specifier_plugin, we don't need to specify this list of additional clusters multiple times.

Ah, right, that makes more sense. Done.

Might be a good idea to explicitly state that this is intended for clients (like gRPC) that fetch the specific set of cluster resources referred to in the RouteConfiguration rather than making a wildcard CDS subscription.

Sounds good; done.

Nit: add a RST link to cluster_specifier, *RouteAction* etc.

One thought: WDYT about colocating this with cluster_specifier_plugin perhaps in a wrapper message? When the client loads the route it could just as easily keep a set of additional_clusters that it comes across? Another option would be to elide this config entirely and just have the additional clusters concept be part of the plugin API that is read during load, though I understand that is probably not desirable given the desire to separate code from config.

I'm definitely open to adding a wrapper message, and optionally including this field in it. (Incidentally, I like the idea of a wrapper message anyway for a possible extension point, but I'll defer to whatever the convention is for Envoy.) However, the reason this is in RouteConfiguration is to avoid the situation where many different routes all reference the same plugin, which is capable of returning a large set of clusters (I've found it's ~1.5kb in practice for one data point). This could lead to config bloat, so we decided to move it here to be able to deduplicate.

This could lead to config bloat, so we decided to move it here to be able to deduplicate.

Fair enough, but then do you need the cluster_specifier_plugin on each route? Could it all just live at the route level and we can add vhost/route overrides later if needed?

I don't think that putting the actual plugin config at the top level would work, because you might need different plugins for different routes.

If the plugin config itself gets too large and we don't want to repeat it in multiple routes, we can move the bulk of the config to an ECDS resource, so that the only thing we need to include in each route is a ConfigSource pointing to the ECDS resource.

I think my point is that saying that one part of this might get too big for the route while the other one might not sounds strange to me. Why not just have a wrapper message and put it at the top level, and then also allow overriding it at the vhost or route level? This is how we do most other things.

I think my point is that saying that one part of this might get too big for the route while the other one might not sounds strange to me.

One key difference is that the config of the plugin is opaque to xDS clients, but the additional clusters needs to be understood.

Another option would be:

• In RouteConfiguration, a map from string (specifier plugin instance name) to ClusterSpecifierPluginConfig which includes a field for additional_clusters.
• Make cluster_specifier_plugin a string that references that map.

Yeah I think ^ would work. I would definitely do the wrapper message. What I'm saying though is to do the following:

1. ClusterSpecifierPluginConfig at route config level.
2. ClusterSpecifierPluginConfig at individual route level (and maybe vhost level)
3. Most specific ClusterSpecifierPluginConfig wins. This is probably generally wire efficient enough for almost all uses cases without the indirection.

Matt, I think your point about not treating these two pieces of data separately is well taken. As it happens, we just realized that we don't actually need the list of clusters anyway, so we're now back to just having the plugin config. However, I agree that it would be nice to have a way to avoid duplicating the plugin config in each route without having to use ECDS. So I think you're right that we should find a way to move that to the top level of the RouteConfiguration.

That having been said, I am thinking that we should design this in a way that makes it efficient to have multiple plugins (we don't need this now but might in the future), each of which is used in multiple routes. For example, consider a case where we have two plugins defined, plugin1 and plugin2, and we have the following list of routes:

1. cluster=A
2. cluster_specifier_plugin=plugin1
3. cluster_specifier_plugin=plugin2
4. cluster_specifier_plugin=plugin1
5. cluster_specifier_plugin=plugin2
6. cluster_specifier_plugin=plugin1
7. cluster_specifier_plugin=plugin2

If we do this as a single ClusterSpecifierPluginConfig per RouteConfiguration that can be overridden in individual routes, then we would need to set the top-level ClusterSpecifierPluginConfig to refer to plugin1, and then we'd need to duplicate the ClusterSpecifierPluginConfig for plugin2 in the three routes that need to use plugin2 instead of plugin1. Also, even for the routes that do use plugin1, we'd still need a new field in the cluster_specifier oneof in the individual routes to indicate that it should use the top-level ClusterPerSpecifierPluginConfig instead of specifying a particular cluster. So we'd wind up adding two new fields in each route, like so:

oneof cluster_specifier {
  // Existing fields.
  string cluster = N;
  string cluster_header = N;
  WeightedClusters weighted_clusters = N;

  // New field to indicate using the top-level ClusterSpecifierPluginConfig.
  bool use_plugin = N;
  // New field to allow overriding the ClusterSpecifierPluginConfig.
  ClusterSpecifierPluginConfig plugin_config = N;
}

And the resulting RouteConfiguration would look like this, with the plugin2 config duplicated three times (textproto format):

// Top-level field in RouteConfig
cluster_specifier_plugin: { name: "plugin1" typed_config: { [envoy.extensions.cluster_specifier.plugin1.Plugin1] { ... } } }

// ...

// Route 1
cluster: "A"
// Route 2
use_plugin: true
// Route 3
plugin_config: { name: "plugin2" typed_config: { [envoy.extensions.cluster_specifier.plugin2.Plugin2] { ... } } }
// Route 4
use_plugin: true
// Route 5
plugin_config: { name: "plugin2" typed_config: { [envoy.extensions.cluster_specifier.plugin2.Plugin2] { ... } } }
// Route 6
use_plugin: true
// Route 7
plugin_config: { name: "plugin2" typed_config: { [envoy.extensions.cluster_specifier.plugin2.Plugin2] { ... } } }

This duplicates the plugin2 config 3 times, which is both inefficient and somewhat counter-intuitive, especially since there will generally be only one actual instance of plugin2.

I think we can avoid a lot of that complexity by specifying the list of plugins at the top-level, keyed by instance name, and then referring to those plugins by name in the individual routes. The idea would be to have something like this in the top-level of the RouteConfiguration:

// A list of cluster_specifier plugin instances.  All names in the list must be unique.
// Note: Ideally, this would be a map, but the map key would just duplicate the name field in TypedExtensionConfig.
repeated core.v3.TypedExtensionConfig cluster_specifier_plugin_instances = N;

And then all we need in the cluster_specifier oneof is a single new field:

// Refers to the plugins defined in the RouteConfiguration top-level cluster_specifier_plugin_instances field.
string cluster_specifier_plugin_name = N;

That way, we can configure multiple plugins like this (textproto format):

cluster_specifier_plugin_instances: [
  { name: "plugin1", typed_config: { [envoy.extensions.cluster_specifier.plugin1.Plugin1] { ... } } },
  { name: "plugin2", typed_config: { [envoy.extensions.cluster_specifier.plugin2.Plugin2] { ... } } }
]

And then individual routes could specify either "plugin1" or "plugin2", and there could be multiple routes referring to each of the two plugins.

WDYT?

Yup ^ sounds great. Let's do it!

Done with minor style changes, and included a wrapper message for future proofing - PTAL.

nit: pedantically, can you mention that this string refers to the name inside the TypedExtensionConfig? It's not completely clear to the reader what this string refers to.

FWIW I'm not 100% sure about the refs -- it looks like envoy_v3_api_msg_config is used to link to a message and envoy_v3_api_field_config is used to link to a field within a message. Is that right?

Also I'm having some trouble running fix_format, so this should not be merged until I've worked that out. (I assume the presubmits will fail anyway.)