Related to the convo we had in meeting yesterday, and to discussions with @alyssawilk, is there any way to make this more readable? By potentially having YAML templates for listeners for most things and small helper functions that are mostly abstracted? I can look at a YAML config and immediately understand what is going on. This is pretty hard to look at...

I don't think this function is particularly readable but I think the use of it is, or can be made to be.

If we

addListener("listener1, "route1")
addRoute("route1" "cluster1" "/foo");
addCluster("cluster1", Something::Mumble::HTTP1);

Then create envoy, I think it's pretty clear we have 1 listener, which maps /foo to cluster1 which speaks HTTP
That's the sort of config I'd like to see in the long run.

Yes I agree, that's where we need to end up, with maybe the functions themselves being based on templates that are easy to understand (or at least with good comments).

I have made this nicer in the commit I'm working on, here's what it will look like:

  envoy::api::v2::Cluster buildCluster(const std::string& name) {
    auto cluster = TestUtility::parseYaml<envoy::api::v2::Cluster>(R"EOF(
      connect_timeout: 5s
      type: EDS
      eds_cluster_config: { eds_config: { ads: {} } }
      lb_policy: ROUND_ROBIN
      http2_protocol_options: {}
    )EOF");
    cluster.set_name(name);
    return cluster;
  }

Q: I assume this needs some type of error checking?

Added.

This feels a lot like LinkedObject, but it doesn't store unique_ptr internally. Would be nice to somehow reuse, but I probably not worth it.

Ack, I know, I as looking at LinkedObject but it just wasn't the right thing for this.

yeah, why not just store internally as map?

We expect that most watches that name resources (as opposed to open CDS/LDS watches, which just wildcard via an empty resource list) will have a single resource, but the current APIs internally allows for more. We need to assemble the subset of resources from those received and pass them to onConfigUpdate() once per watch. This means we need to do some gather operation. This could be either done by building buckets for each watch and populating them as we walk the watch list (as done today), or building buckets and populating them as we walk the resource list.

I'm not a fan of either of these. If we change the API to allow zero (wildcard) or one resource per watch, we can easily use a map. Given that's how it's actually used by the xDS implementations in Envoy today, I think I'm going to modify the ADS internal API to switch to this. Let me know if that sounds good.

+1 sounds good simpler is better

After playing around with this, we still need to do the map construction. This is because we need to deliver empty updates to watches (e.g. EDS) when a resource is not present, so we basically need to walk all the watches and have a O(1) map back to the delivered resource matching the watched name. So, I'll leave it as is.