xds: Add composite filter by AgraVator · Pull Request #12646 · grpc/grpc-java

AgraVator · 2026-02-05T05:07:01Z

Implementation of A103

AgraVator · 2026-02-07T08:44:02Z

    if (proto.getHttpFiltersList().isEmpty()) {
      throw new ResourceInvalidException("Missing HttpFilter in HttpConnectionManager.");
    }


Keeping this the same, assuming we will still be getting Filters here and not all through ExtensionWithMatcherPerRoute

sauravzg · 2026-02-10T09:58:40Z

@AgraVator Can you split this into two PRs ? 1 for importing protos and another for business logic changes for composite filters? go/small-cls

sauravzg · 2026-02-10T14:55:03Z

    abstract ImmutableMap<String, FilterConfig> filterConfigOverrides();

+    @Nullable
+    abstract ImmutableMap<String, com.google.protobuf.Struct> filterMetadata();


go/java-practices/null#collection

Is there a semantic difference between a null filterMetadata and empty filterMetadata ?

In the current implementation, a null filterMetadata represents the absence of metadata (as per the proto not having it set), while an empty map would represent present but empty metadata. I followed the existing pattern in the codebase for optional maps. However, I agree that using non-null empty collections is generally preferred in Java to avoid null checks. We can consider a refactor to use empty maps consistently in a separate cleanup PR to keep this PR focused on the core feature ?

sauravzg · 2026-02-10T14:55:23Z

+    public String[] typeUrls() {
+      return new String[] {
+          TYPE_URL_EXTENSION_WITH_MATCHER,
+          TYPE_URL_COMPOSITE,


Why do we need this ?
I'd assume that TypeUrls represent the entry points which in case should be ...matcher or ..matcherperroute . Then we construct the composite filter as needed from the config proto.
Are there cases where we might need this at the top level?

The gRFC A103 specifies that the Composite Filter can be configured as an HTTP filter directly. In Envoy, it can appear at the top level in the http_filters list of the HttpConnectionManager. While it is more commonly used inside a matcher to combine filters, we need to support it at the top level to be compliant with the spec and support cases where a user wants to apply a composite filter to all routes by default.

sauravzg · 2026-02-10T14:56:41Z

+      }
+
+      if (matcherProto == null) {
+        return ConfigOrError.fromConfig(new CompositeFilterConfig(null));


What does this case cover? any is an arbitrary proto, so we return a default configerror? What does a CompositeFilterConfig with null semantically mean?

A CompositeFilterConfig with a null matcher represents a configuration where the message is just Composite without any specific matcher configured. Semantically, it means a default or passthrough configuration when no matching logic is specified. If an arbitrary proto is received that we cannot unpack or handle, we return a config error to fail fast, as per standard xDS parsing rules.

sauravzg · 2026-02-10T14:57:12Z

+                        throw new IllegalArgumentException("Failed to unpack TypedStruct", e);
+                      }
+
+                      Filter.Provider provider = FilterRegistry.getDefaultRegistry().get(typeUrl);


Usually, depending on a global registry may make testing difficult and brittle, which is somewhat evident by the need to add a visiblefortesting deletion method needed to be added in the FilterRegistry.

My recommendation here would be to follow "program to interface instead of implementation" , by creating an interface and inject it as a dependency into the CompositeFilter .
Then production can use an implementation which can adapt the DefaultFilterRegistry, while the unit test can use a mock.

Added a package-private static registryLookup function in CompositeFilter.Provider that defaults to using FilterRegistry.getDefaultRegistry(). This allows injecting a custom lookup function in CompositeFilterTest to return the mock provider without having to modify the global registry state during tests.

sauravzg · 2026-02-10T14:57:15Z

+
+    @Override
+    public String typeUrl() {
+      return TYPE_URL_COMPOSITE;


Is this the reason for having three entries in the filter provider ? So, that we can correlate the config type to the provider?

Yes, returning these three type URLs in typeUrls() allows the FilterRegistry to route all three configuration types to this CompositeFilter.Provider. This is necessary because CompositeFilter handles all three of these related proto types (ExtensionWithMatcher, Composite, and ExtensionWithMatcherPerRoute) to implement the feature, and returning them in typeUrls() ensures that the xDS config parser dispatches them to this provider.

sauravzg · 2026-02-10T14:57:26Z

+
+    return new ServerInterceptor() {
+      @Override
+      public <ReqT, RespT> io.grpc.ServerCall.Listener<ReqT> interceptCall(


nit: Any particular reason for the assymetry? ClientInterceptor is abstracted into a helper class while ServerInterceptor is inline?

The asymmetry is due to the different timing of when headers are available. On the client side, headers are only available when ClientCall.start() is called, so we need a helper class (CompositeClientCall) to delay the matching and interceptor creation until then. On the server side, headers are available immediately in ServerInterceptor.interceptCall(), so we can perform matching and create interceptors inline without needing a separate helper class.

sauravzg · 2026-02-10T14:57:32Z

+        UnifiedMatcher.MatchingData data = new MatchingDataImpl(headers, null,
+            call.getAttributes());
+
+        List<FilterDelegate> delegates = matcher.match(data);


Can we use non null empty containers only in the matchers API contract?

Will be replaced by the work being done for A106

sauravzg · 2026-02-10T15:00:32Z

+            }
+          } catch (Throwable t) {
+            for (Filter f : filters) {
+              f.close();


I am not sure of the solution here due to my unfamiliarity with the language , but we have repeated usages for "close all filters on error" . Is there an alternate design pattern that can do some form of RAII like try-with-resources (or something that allows autocolose on error /scope exit) ?

ejona86 · 2026-02-10T17:33:59Z

Can we use a typical title that includes a verb and specify the module? Like "xds: Add composite filter".

Definitely split out the protos upgrade. If you just added protos in this PR it wouldn't be as hard to look at and as likely to conflict with other changes. But given we're implementing multiple parts of this simultaneously, just adding protos probably should be separate to avoid drift while reviewing (you add a proto here, but someone else does an upgrade, but wouldn't upgrade your new proto).

Right now I'm left wondering why you needed to do the upgrade: what field/change are you pulling in? That's the sort of thing useful to describe in the PR/commit description, because there's no way I'll be able to find the specific piece you're wanting.

AgraVator · 2026-02-11T09:47:01Z

f you just added protos in this PR i

xds: add protos for composite filter

shivaspeaks · 2026-02-25T04:23:10Z

All of UnifiedMatcher should be gone with #12640 as per A106.

shivaspeaks · 2026-02-25T04:23:17Z

  }
+
+  /** Translate StringMatcher xDS proto to internal StringMatcher. */
+  public static Matchers.StringMatcher parseStringMatcher(


The stringMatcher parsing logic should not be in this PR. It will be covered by A106. You then need to just modify add the CUSTOM case.

ejona86

I just glanced around a bit.

ejona86 · 2026-05-08T17:43:55Z

@AgraVator, did you want to "Re-request review" from anyone?

ejona86 · 2026-05-08T18:03:03Z

+  }
+
+  static final class Provider implements Filter.Provider {
+    private static final ThreadLocal<Integer> recursionDepth = ThreadLocal.withInitial(() -> 0);


A ThreadLocal would be quite unfortunate. I strongly doubt that is necessary. The gRFC says:

Note that in order to avoid potential stack overflows, we will impose a maximum recursion depth of 8 when parsing HTTP filter configs.

That doesn't say a maximum recursion depth for composite filter configs, it says all HTTP filter configs. That would mean it'd be a change to parseFilterConfig()/parseFilterConfigOverride()

revert unintended changes

603d4fe

AgraVator force-pushed the xds-composite-filter branch from 6bf8474 to 603d4fe Compare February 5, 2026 16:44

AgraVator commented Feb 7, 2026

View reviewed changes

AgraVator added 2 commits February 7, 2026 20:05

add support for keep_matching

cd6c611

fix warnings

64e7c1c

AgraVator changed the title ~~composite filter init~~ composite filter Feb 9, 2026

AgraVator marked this pull request as ready for review February 9, 2026 18:07