Why do we need this new extension point in StringMatcher? Why not use the new-style unified matcher API instead, since that is already fully pluggable?

https://github.com/cncf/xds/blob/main/xds/type/matcher/v3/matcher.proto

Because StringMatcher is used in many places where the new matchers are not supported, and adding this here gets support in all those places.

I'm not very familiar with the new matcher API, but I looked at it briefly and it seems to always have actions associated with a match; I'm not exactly sure how to apply that in some cases where StringMatcher is used where it just needs a boolean response.

One specific case I want to use this extension matcher is in SAN validation in the tls transport socket.

Yeah, I think I'm fine conceptually here with this being an extension type for the reasons Greg gives.

Why was this needed?

This is needed to make the multi-envoy integration test pass. Without this, the 2nd server ASSERTs because it's trying to set a non-null singleton.

I think this means that extensions that need this singleton will not work in certain envoy-mobile setups where there are multiple engines running.

As best I can tell, this is the same situation we're in for a regex engine, which is also managed by a singleton (for identical reasons: it's REALLY ugly to pass all these bits in to every place where a StringMatcherImpl is constructed; I tried, and it touched too much code).

OK, maybe leave a comment on this? It's unclear to me which things are chosen for initialization here and why other than trial-and-error.

Agreed; adding a comment

Thanks, I can see the issue from this. I think it might be OK to selectively allow this violation of singleton safety, but I'd like to just quickly check with @mattklein123 (who did a lot of the singleton work) and @yanavlasov (who has coverage on how we create Google internal environments for Envoy binaries) to verify. Good to merge if we can agree on this.

Yep, still waiting on input/signoff.

Friendly ping @mattklein123 @yanavlasov.

It would be hard to tell what impact it will have internally. I think the code is contained enough that I'm not too concerned about patching it internally.

My main concern here is that a bad precedent with regex engine is getting reinforced here. If a factory does not provide the context that an extension needs, people will continue plumbing missing objects through a singleton.

I think it may be ok to accept this as a temporary solution. However I think the matcher factory needs to be fixed to have at least the base (or server) context passed to it. I suspect it is a major pain.

This should also be a requirement that all new factories have context available.

I agree with @yanavlasov. This is OK for now but it's not great and we should try to track cleaning it up as tech debt. In a perfect world it would be nice for use of this to fail in the multi-envoy case vs. just not work silently but I'm not sure if that is easily possible.

I agree it's not ideal. Before posting the PR, I started trying to pass the context through everywhere and the change was HUGE and I hadn't yet solved the most awkward case, which is that the TLS cert validator gets constructed from an SDS secret-update callback, and we'd need the context there.

Filed #32792 to track.