Hi @paul-r-gall I noticed this PR is in Draft. Would you like me to assign a maintainer to start the code review process, or wait until you mark ready for review?

/wait-any

@junr03 Yes, that would be great to get a first pass from someone who can LGTM the approach, at which point I can finish up the rest of the testing.

Thanks for the feedback! I'm new to Envoy, so really appreciate the comments.

The advantage of factory-style lookup is that it allows end users to customize the matching however they like. For example, using Listener again, if there's some_specific_parameter buried deep inside the externally populated metadata field that the user wants to filter by, there's no way for a standard isMatch method to know about where this parameter lives, or how to match against it.

With regards to not separating the Factory and the Matcher, I agree, probably unnecessary, and a consequence of me not yet fully getting Envoy style yet.

In terms of templating, I would be fine with that approach rather then registering all separate matchers. Just to make sure that I understand, integrations would make template specializations for whatever types needed custom matching (Listener, RouteConfiguration, etc).

The advantage of factory-style lookup is that it allows end users to customize the matching however they like. For example, using Listener again, if there's some_specific_parameter buried deep inside the externally populated metadata field that the user wants to filter by, there's no way for a standard isMatch method to know about where this parameter lives, or how to match against it.

I don't propose a single isMatch that would work for all the object types, I'm really just questioning whether a factory lookup is necessary vs. just writing an isMatch method for each type that can be dumped.

For the metadata case, since the matching is always controlled by the DumpMatcher object registered for the protobuf type (e.g. Listener or Cluster), that DumpMatcher implementation would need to extract the Metadata object from Listener (or Cluster) and then compare that with the match params. You could implement that by making another call to look up a DumpMatcher for Metadata, but you could also just write a utility method that matched Metadata and call it directly.

I took the DumpMatcher implementation in the listener test as an example of what the real implementation would look like, but maybe that's too trivial an example?

So, the scenario that I'm thinking of is closer to the following more complicated metadata case:

An Envoy integration has an external proto some.external.Proto, that is one of the fields inside of the metadata field of Cluster. So a cluster proto will look like:

name: cluster_name
// non-metadata values
metadata {
  filter_metadata {
    key: "some.external.Proto"
    value {
      fields {
        key: "field_of_interest"
        value { string_value: "important_value" }
      }
      // other fields may be here
    }
    // other filter_metadata may be here
  }
}

Ideally, the external user would be able to send something like /config_dump?match_some_external_proto_field_of_interest=important_value which only returns clusters which have metadata that looks like the above proto. So they'd write their Cluster specialization of match as follows:

bool match(const envoy::config::cluster::v3::Cluster& cluster,
           const MatchingParameters& params) override {
  bool is_match = true;
  if (params.contains("some_external_proto_field_of_interest")) {
    // parse cluster.metadata["some.external.Proto"] into an instance of some.external.Proto
    is_match &= parsed_proto.field_of_interest() == params["some_external_proto_field_of_interest"]
  }
  // other matching logic w/ other params
  return is_match;
}

Whereas, if all this were to be done on the Envoy side, there'd need to be an Envoy function like

bool isMatch(const envoy::config::core::v3::Metadata& metadata,
             const MatchingParameters& params) {
  // somehow parse/match params and metadata in such a way that's flexible enough to cover every
  // scenario that users can come up with.
}

Admittedly, this is an extreme case of complexity, but I think I would prefer to provide the end user with as much matching flexibility as possible.

As a non metadata example, similar flexibility would be needed to filter fragment information from a ScopedRouteConfiguration.

Another thought about why I used name-based factory lookup instead of templates -- the name-based factory lookup allows tests to inject resource-specific behavior, whereas with a template approach all matching behavior for a resource type is fully decided at compile time.

So they'd write their Cluster specialization of match as follows

It seems like you want to be able to extend Envoy with custom implementations that might override built-in matchers. In that case, if the registered name is just the type name (e.g. "envoy.config.cluster.v3.Cluster") there's an initialization order problem. Envoy's registry system makes use of static initializers. This works because we expect each extension to register a unique name (which is [enforced by the code[(https://github.com/envoyproxy/envoy/blob/main/envoy/registry/registry.h#L216)). We could relax that check, but C++ offers no guarantees about whether the default implementation for Cluster would be registered before or after a custom one. The ordering could easily change from build-to-build, depending on the compiler. You could have custom filtering extensions and request one using a URL parameter, (e.g. config_dump?filter_ext=envoy.admin_filters.my_custom_filter&filter=... but that doesn't feel like a very natural extension point for Envoy.

Honestly, I think you could make use of descriptor and message to implement simple filtering in a generic way. I agree that coming up with a way to encode arbitrarily complex queries would be difficult, but I'm also not convinced that should be the goal. I expect most use cases for this are "name=x" or "metadata.filter_metadata.com.namespace.custom.key=value" and combinations via logical and. I think that's that's mostly just a matter of deciding the right way to delimit and escape proto field names, map keys, and array indexes and writing the filtering code. All of which could live in source/server/admin. Since the output of config dump is JSON-encoded protobuf objects, I don't think it's too much to expect people to provide their own scripting for complex cases.

I was not actually planning on creating any default matchers, so there wouldn't be a name conflict.

CPU profiling indicates that about 35% of the CPU time in handlerConfigDump is spent in API_RECOVER_ORIGINAL and Any::PackFrom, so moving filtering to source/server/admin won't fix that issue (as all protos will still have API_RECOVER_ORIGINAL and Any::PackFrom called on them). Additionally, another ~40% of time is spent in redact, and an additional 10% is spent in getJsonStringFromMessage, so expecting integrations to do their own filtering for complex cases after the JSON is returned also doesn't solve the CPU time issue (which, along with memory usage, is the main reason for this PR).

Based on the complexity of the code in trimResourceMessage to apply a FieldMask to config dump, and the various TODOs in there, I'm skeptical that it'd be easy to do simple filtering in a generic way.

All that said, if you think it would be better to just do really simple filtering/matching by name (maybe w/ a regex or something), and leave the more complicated material for a future PR to allow more discussion and consideration, I would be fine with that (and it would satisfy the current Google-internal requirements).

Thanks for taking the time to provide those measurements. I can see why you want to push the filtering down as far as possible.

Historically, we've not provided extension points that aren't used at all from within Envoy. That said, I think we could have a relatively simple default implementation (maybe something that only filters on name and simple metadata) and a way to configure Admin to use a different implementation, provided as an extension. I think this provides similar performance to what's in the PR now: the filtering occurs as each object is retrieved and therefore avoids the expensive operations for objects that are filtered out.

The differences are:

1. Instead of looking the extension up by the type of object being dumped, we'd add a config_dump_filter field to Admin. (Feel free to pick a better name.) The type would be a message with the usual name/typed_config fields that we use for specifying extensions. If unset, the default would be used, and your filter can be provided as an extension.
2. Rather than passing MatchParameters into the callback, you'd lookup the extension factory, construct a filter implementation from the factory by passing the query param(s), and then pass the filter into the callback.
3. It's probably easiest if the filter implementations are expected to handle all the various object types (and ignore unknown types), rather than having a separate filter implementation for each type. This leaves open the possibility of someone writing a completely generic filter, but doesn't require it. You can just check the message type, cast and examine fields directly.

I think that addresses my concerns and leaves you free to implement private filtering code that does what you need without worrying about other operators might want.

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

Added new commit implementing @zuercher's suggestions. @htuch please let me know what you think.

Thanks! Yes, that looks like the correct approach to me. In order to make sure there's consensus, I would like still like to have another maintainer chime in and then I'll do a closer read. Perhaps @htuch or @mattklein123?

@paul-r-gall just to make sure we're doing due diligence here. In the near future, API_RECOVER_ORIGINAL will go away and I think we can filter prior to redaction. If we do this, what is the overhead of using a field mask matcher?

I.e. what is the cost of doing field mask match at the exact same points at which the ConfigDumpFilter match is applied?

@htuch I still definitely think that filtering needs to happen within the callback. Even if API_RECOVER_ORIGINAL goes away, there's still the overhead of Any::PackFrom, which accounts for about a third of the CPU time during the callback.

By field mask match, I'm assuming you mean something like path.to.first.field=regex1, path.to.second.field=regex2, right? The only issue I have there is that FieldMasks don't by default work with Any, and so to do metadata matching, we'd have to do something like the trimming fxn from config_dump_handler.cc.

As I mentioned above, at the moment simple name/metadata regexp matching is sufficient for Google's current (edit: but perhaps not future) usecase, so I'll defer to the maintainers as to whether having this capability be extendable is worth the extra review, effort, and maintenance.

My personal vote is to not add an extension point right now. I would just build the matching functionality in, and if we need to extend it later I think we can add an extension point and convert the existing default implementation into a default extension.

Apart from that, I wonder if there is a way we can avoid every config dump handler being modified to do this matching which seems fragile. That seems really unfortunate. Is the issue that at the admin layer we have an Any and at that point we lose the ability to do reflection and field matching (or we would have to unpack from the Any again and waste a lot of time)? If that is the case I wonder if there is some way we could refactor the config dump handlers so that before packing the Any the admin code gets a callback to do filtering? This is not fully formed but something like: Admin passes to the handler some type of maybePackAny lambda, and the config dump handler somehow uses this to do the packing (and also the filtering). I haven't thought about it a ton but it seems like something like this would be better than missing filtering in new config dump handlers.

@paul-r-gall maybe just do simple name matching for now then. I think anything that needs to reach inside metadata is going to end up involving some amount of Any interaction which is not going to be super fast. We could later add specific metadata matchers as well, which probably solves most use cases (between metadata matching and top-level field matching).

Coverage run indicates that source/server coverage is lower than needed; 94.3 vs 94.4, but source/server indicates that coverage is 94.4, and all the files I changed have the correct coverage. @mattklein123 what action should I take here besides /retest ?

Coverage run indicates that source/server coverage is lower than needed; 94.3 vs 94.4, but source/server indicates that coverage is 94.4, and all the files I changed have the correct coverage. @mattklein123 what action should I take here besides /retest ?

cc @alyssawilk. Coverage can be flaky so you could try a retest? I'm not sure.

/retest

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

/retest due to timeout

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

Sorry can you merge main which should fix coverage?

/wait

Sorry one more merge should fix this.

/wait

/retest

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

/retest

macos presubmit continues to time out

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit