xds: HttpFilter support #7932

dapengzhang0 · 2021-03-02T18:43:27Z

See for

Design doc:
https://github.com/grpc/proposal/blob/master/A39-xds-http-filters.md

The java approach is similar to Go because they both use FilterConfigs to generate interceptor rather than method config in ConfigSelector.

The new API io.xds.Filter is equivalent to httpfilter.go.

voidzcy

To be continued...

xds/src/main/java/io/grpc/xds/FaultConfig.java

xds/src/main/java/io/grpc/xds/Filter.java

xds/src/main/java/io/grpc/xds/XdsNameResolver.java

xds/src/main/java/io/grpc/xds/FaultFilter.java

xds/src/main/java/io/grpc/xds/Filter.java

xds/src/main/java/io/grpc/xds/FaultFilter.java

…ilter

voidzcy · 2021-03-04T20:04:21Z

xds/src/main/java/io/grpc/xds/XdsClient.java

-    @Nullable // Can be null even if hasFaultInjection is true.
-    final HttpFault httpFault;
+    // Filter instance names. Null if HttpFilter support is not enabled.
+    @Nullable final List<String> filterChain;


Why not have Filter contains FilterConfig? Then here you would only need a single list of Filters. And in the resolver, you do not need to look up the global registry again.

Did you mean have FilterConfig contain Filter?

See my comment for Filter below. I mean something like

interface Filter { // Returns the canonical name of the filter. String getName(); FilterConfig getConfig(); interface Parser { // Returns the list of typeUrls for filter configs that are acceptable by this parser. String[] getTypeUrls(); FilterConfig parseConfig(Any config); } ... }

Generics can probably make this better.

No, the filter config override is finalized in XdsNameResolver's selectConfig(). Way too late to instantiate such an object.

At the minimum, I think you could make the FilterConfig have a property for the name of filter it is associated with. Then you can eliminate this List + Map representation.

As a ref, C-core's representation looks simpler to manage: https://github.com/grpc/grpc/blob/0937cb1249129a84159c639c59f49a451157edf7/src/core/ext/xds/xds_api.h#L231-L242

Note, don't be confused by C-core's FilterChain, that's something used for server side. It's irrelevant.

These two approaches are just preferences. It's not that one of them is significantly simpler than other. For the FilterConfig with name approach, it's inconvenient to query the name and override config.

voidzcy · 2021-03-04T20:23:36Z

xds/src/main/java/io/grpc/xds/Filter.java

+import javax.annotation.Nullable;
+
+/**
+ * Defines the parsing functionality of an HTTP filter. A Filter may optionally implement either


Such an interface looks weird. It seems to be really just a Parser, as it is documented as "Defines the parsing functionality of an HTTP filter...". Then you probably want some interface to represent Filter objects (generics may help).

This is aligned with C-core and Go APIs. I don't want to deviate too much from APIs of other languages. The javadoc is copied from go.

The API spec in the design doc

Each filter implementation will provide the following: Method(s) for validating the config protos when parsing an xDS response. An indication of whether it is supported on the gRPC client and/or gRPC server. Methods to generate and configure the appropriate filters or interceptors in gRPC to perform the necessary functionality on the data plane.

…ilter

xds/src/main/java/io/grpc/xds/Filter.java

xds/src/main/java/io/grpc/xds/ClientXdsClient.java

voidzcy · 2021-03-08T21:01:29Z

xds/src/main/java/io/grpc/xds/Filter.java

+  /** Uses the FilterConfigs produced above to produce an HTTP filter interceptor for clients. */
+  interface ClientInterceptorBuilder {
+    @Nullable
+    ClientInterceptor buildClientInterceptor(


This interface is redundant. It looks you don't need to passing both the top-level filter config and the override config(s). It should be as simple as buildClientInterceptor(FilterConfig config, ...). The caller should just need to pass in the single filter config: the most fine-grained one (WeightedCluster > Route > VirtualHost > HttpConnectionManager). The existing interface is half-baked: you let the implementation decide whether to use config or overrideConfig, but let the caller decide which filter config to be passed in as the overrideConfig.

Given that, it can be further changed to have this method on FilterConfig interface:

interfacce FilterConfig { String typeUrl(); ClientInterceptor getClientInterceptor(PickSubchannelArgs args, ScheduledExecutorService scheduler); }

as the same type of FilterConfig always produces the same kind of ClientInterceptor.

The spec says:
"Note that, for a given request, the filter's configuration will come from the top-level filter list in the HttpConnectionManager config and the most specific override. "

Both c-core and go APIs are passing both the top-level and override configs.

Fault filter uses just override config if it is provided, and that's just a special case, and for general HttpFilters that's not the case.

I am commenting from interface/class design's perspective. I don't think direct copying method signatures from other languages is appropriate, and IMO such an interface looks very awkward.

If you want a general purpose interface for merging the top-level config and its overrides (could be up to 3 levels of overrides), the method should take in a list of overriding configs, not just the most specific one. If it is well-defined that merging configs is just letting the most specific one take over, the method only needs to take in a single config argument where as the caller always passes in the most specific config. And the method of returning a ClientInterceptor can just be associated with the FilterConfig instance, as the implementation of converting a certain type of FilterConfig to a ClientInterceptor is specific to each type of config itself.

Anyway, I can just let you pass if you just want to do exactly the same thing as C-core and Go.

The interface is for passing a top-level config and a merged override config (the 3 level of overrides merged as the finalized override), and in general, the top-level config and the merged override config are of different types, and both of them will be needed to generate ClientInterceptor. The interface is for a general HttpFilter. For the Fault Filter, the top-level and override configs are of the same type, and if override config is provided, the top-level will not be used, but that's a special implementation.

voidzcy · 2021-03-09T01:32:43Z

xds/src/main/java/io/grpc/xds/ClientXdsClient.java

+    Any anyConfig = httpFilter.getTypedConfig();
+    Message rawConfig = anyConfig;
+    String typeUrl = anyConfig.getTypeUrl();
+    if (anyConfig.getTypeUrl().equals(TYPE_URL_TYPED_STRUCT)) {


Can this also be "type.googleapis.com/envoy.config.route.v3.FilterConfig"?

No. The top-level config should not be envoy.config.route.v3.FilterConfig, which is a wrapper of config plus a is_optional flag that the top-level already has.

Alright. My main point was to put the logic of flattening the config message in a helper method and reuse the code. Even if the top-level config will not be envoy.config.route.v3.FilterConfig, your helper method can still be reused no matter it is top-level or override, right?

xds/src/main/java/io/grpc/xds/FaultFilter.java

voidzcy · 2021-03-09T10:52:38Z

xds/src/main/java/io/grpc/xds/LameFilter.java

+
+  @Nullable
+  @Override
+  public ClientInterceptor buildClientInterceptor(


I am very confused by how this method would be used. The LameFilter does not implement config parsing. That means you do not get a FilterConfig. In the xDS message parsing for top-level filters, if the config message doesn't exist, we just skip that filter entirely. That means, to use the LameFilter, we must have a config. Parsing the LameFilter's config throws UnsupportedOperationException. I am deadlocked...

LameFilter is not parsed from xDS message. It is added by grpc to the end of filter chain if RouterFilter is absent as in the spec :

if the router filter is not present, then gRPC will insert a special filter at the end of the filter chain that fails all RPCs.

parseFilterConfig() will never be called.

There are many ways to do it equivalently. I chose a way that is closest to c-core.

voidzcy · 2021-03-09T10:53:25Z

xds/src/main/java/io/grpc/xds/RouterFilter.java

+
+  @Override
+  public StructOrError<? extends FilterConfig> parseFilterConfig(Message rawProtoMessage) {
+    return StructOrError.fromStruct(ROUTER_CONFIG);


Why is this useful?

We don't parse any specific field in the raw config, so we just return a dummy "parsed" config.

voidzcy · 2021-03-09T21:21:40Z

xds/src/main/java/io/grpc/xds/ClientXdsClient.java

+          filterConfig =
+              anyConfig.unpack(io.envoyproxy.envoy.config.route.v3.FilterConfig.class);
+        } catch (InvalidProtocolBufferException e) {
+          return StructOrError.fromError("Invalid proto: " + e);


Include the filter name in the error message now that you are completely relying on this method to give you an error message.

Or you could let the caller decorate the error message with filter name (then you do not need to pass in the filter name).

Included the filter name in the error message now

xds/src/main/java/io/grpc/xds/FaultFilter.java

voidzcy · 2021-03-10T01:56:11Z

xds/src/main/java/io/grpc/xds/XdsNameResolver.java

+    private Result lameResult(RoutingConfig savedRoutingConfig, PickSubchannelArgs args) {
+      Map<String, ?> rawServiceConfig = Collections.emptyMap();
+      if (enableTimeout) {
+        Long timeoutNano = routingConfig.fallbackTimeoutNano;


Again, are we able to reorganize the code so that we do not need to repeat this logic (as well as the block for handling interceptor list)?

voidzcy · 2021-03-11T00:29:16Z

xds/src/main/java/io/grpc/xds/Filter.java

+   * by any of the {@link Filter#typeUrls(), type URLs}.
+   */
+  final class Registry {
+    static Registry GLOBAL_REGISTRY =


nit: GLOBAL_REGISTRY. I am not a fan of such a name, it looks strange to me... To me, FilterRegistry.getDefaultRegistry() looks much more comfortable than Filter.Registry.GLOBAL_REGISTRY.

voidzcy · 2021-03-11T00:55:23Z

xds/src/main/java/io/grpc/xds/XdsNameResolver.java

              .build();
    }

+    private ConfigOrError generateServiceConfig(


This method is confusing in terms of its signature and its name. You only need the fallbackTimeoutNano from the RoutingConfig. This is like I want a banana, but you give me a monkey holding a banana with the entire forest behind...

Also, why do you need a timeout method config for lame?

This method is confusing in terms of its signature and its name. You only need the fallbackTimeoutNano from the RoutingConfig. This is like I want a banana, but you give me a monkey holding a banana with the entire forest behind...

Well, the method name generateServiceConfigWithMethodTimeoutConfig() is already taken. The two methods can be merged, but generateServiceConfigWithMethodTimeoutConfig() is used for testing. That's why I was originally keeping a few lines of duplicate code.

I can change the RoutingConfig arg to fallbackTimeoutNano in signature.

Also, why do you need a timeout method config for lame?

If there is a delay injection filter ahead of lame, the fallback timeout can still be needed.

voidzcy · 2021-03-11T01:09:36Z

xds/src/main/java/io/grpc/xds/XdsNameResolver.java

-    private final boolean applyFaultInjection;
-    @Nullable
-    private final HttpFault faultConfig;
+    // Null if HttpFilter support is enabled but RouterFilter is absent in the filter chain.


I think this implementation is unnecessarily complicated with many small logic branchings. A very straightforward approach is just having a list of FilterConfig (not null, but can be empty). At the beginning processing the chain, you just peek the last entry, if it's not a RouterFilter, append a LameFilter. That's it. Then you have simple and invariant representations for all cases.

A very straightforward approach is just having a list of FilterConfig (not null, but can be empty).

Nullness of the list is needed for whether HttpFilter support enabled or not. That is simple and straightforward. Other tricks can be very misleading. Once HttpFilter support is no longer experimental, the list will be non-null.

I mean your code logic contains too many case-specific handlings that can be generalized. That's why you end up having so many ifs (and Null if ...). That introduces many edge cases and is more likely to have bugs, either hidden somewhere already or a small patch could introduce one. And it also makes testing hard/complicated (you would have more code branches need to be covered).

That's why I was saying you could just simply append a lame filter if the last filter is not a router filter, no matter there is one in the middle of the chain or not. And you just have a general code block for processing filters, rather than getting into different codepaths based on if router filter exists or not. The general idea is have a clear/strong specification for each method (AKA do one thing well) with weak precondition/call assumption.

The different codepath is to skip the routing logic if there is no rout filter. This branching is unavoidable even appending a lame filter ahead of time. I do agree that appending a lame filter ahead of time can handle all kinds of filters uniformly and reduce some duplicate code. However, to handle the lame filter in the same way as other filters, it need a synthetic filter name and type_url, which I don't like.

Changed as suggested. PTAL again.

xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java

voidzcy

Mostly that's the review. That's all my comments. If I cannot convince you, I will just accept it.

Btw, don't you think the tests are weak/test too litttle?

voidzcy · 2021-03-12T19:07:29Z

xds/src/main/java/io/grpc/xds/XdsNameResolver.java

-    private final boolean applyFaultInjection;
-    @Nullable
-    private final HttpFault faultConfig;
+    // Null if HttpFilter support is enabled but RouterFilter is absent in the filter chain.


I mean your code logic contains too many case-specific handlings that can be generalized. That's why you end up having so many ifs (and Null if ...). That introduces many edge cases and is more likely to have bugs, either hidden somewhere already or a small patch could introduce one. And it also makes testing hard/complicated (you would have more code branches need to be covered).

That's why I was saying you could just simply append a lame filter if the last filter is not a router filter, no matter there is one in the middle of the chain or not. And you just have a general code block for processing filters, rather than getting into different codepaths based on if router filter exists or not. The general idea is have a clear/strong specification for each method (AKA do one thing well) with weak precondition/call assumption.

…ilter

dapengzhang0 · 2021-03-12T19:28:29Z

Btw, don't you think the tests are weak/test too litttle?

What (strong) tests do you think should be added?

voidzcy · 2021-03-15T08:31:44Z

Don't forget to make types of supported HttpFilter messages printable.

voidzcy · 2021-03-15T18:02:20Z

xds/src/main/java/io/grpc/xds/XdsClient.java

      }
      if (listener != null) {
        toStringHelper.add("listener", listener);
      }
      return toStringHelper.toString();
    }
+
+    static final class NamedFilter {


Why this is defined here? Instead of in Filter? Also, this makes the definitions of Filter and FilterConfig very confusing. Why not just add the name property to FilterConfig?

Why this is defined here? Instead of in Filter?

Because it's only used for constructing or accessing data of LdsUpdate.

Why not just add the name property to FilterConfig?

The parsed config returned by the API StructOrError<FilterConfig> parseFilterConfig(Message rawProtoMessage) is not tied to a filter name. Actually it's possible that two filter instances with different names can have the same FilterConfig. It's just the pure config data parsed from envoy proto.

Actually it's possible that two filter instances with different names can have the same FilterConfig.

That's just two separate FilterConfig instances, I don't see the difference between putting the name property on FilterConfig vs wrapping with another layer. Anyway, I accept this approach, but NamedFilter shouldn't be defined here.

Because it's only used for constructing or accessing data of LdsUpdate.

That's not a valid reason, NamedFilter is useless without the feature of HttpFilter, it's HttpFilter-specific. Putting it here is anti-modularity (Filter and other Filter-related classes are defined elsewhere while this one is defined on XdsClient surface). I don't think this is just a style issue...

xds/src/main/java/io/grpc/xds/XdsClient.java

xds/src/main/java/io/grpc/xds/XdsNameResolver.java

voidzcy · 2021-03-15T18:59:01Z

xds/src/main/java/io/grpc/xds/XdsNameResolver.java

+      this.virtualHostOverrideConfig = Collections.unmodifiableMap(virtualHostOverrideConfig);
+    }
+
+    private boolean hasLameFilter() {


I don't see any usefulness of this method, especially compared with what you previously had (aka, a local variable when populating the filter chain). What I meant was, you do not need to populate the filter chain to see if a router filter exists (aka, code in updateRoutes()), just peek the last one. If it's not a router filter, insert a LAME.

Peeking the last one is not sufficient as the router may be present but is not the last one:
(https://github.com/grpc/proposal/blob/master/A39-xds-http-filters.md#the-router-filter)

In addition, if the router filter is present, any filters in the list after the router filter will not actually be used, although their configurations will still be validated when the config is received from the xDS server.

any filters in the list after the router filter will not actually be used

If it's a router filter presents in the middle, anything after it doesn't matter.

Not sure I understood what you mean correctly, removed the method.

…ilter

voidzcy

I do not agree with just creating another StructOrError (Filter.StructOrError), just because the existing one is not accessible outside ClientXdsClient. Before creating this PR, you should have solved that problem already. If not, that should be a blocker for making this change.

I do not agree with the development process that first check in code with poor integrity and then fix it later (This is like "currently I do not depend on gRPC so let's implement my own gRPC, then next PR will delete my own implementation and pull in a gRPC dependency"). That's how things become messy. Anyway, you need to coordinate with Sergii for resolving this.

/cc @sergiitk

xds: HttpFilter support

0e292de

dapengzhang0 force-pushed the httpfilter branch from e525ea8 to 0e292de Compare March 2, 2021 20:30

add tests

50a8bdf

dapengzhang0 requested a review from voidzcy March 2, 2021 21:07

voidzcy reviewed Mar 4, 2021

View reviewed changes

dapengzhang0 added 2 commits March 4, 2021 10:05

move constructor after fields declaration

c93e4a2

add javadoc and rename field name in Registry

c2678cb

voidzcy reviewed Mar 4, 2021

View reviewed changes

xds/src/main/java/io/grpc/xds/Filter.java Outdated Show resolved Hide resolved

xds/src/main/java/io/grpc/xds/FaultFilter.java Outdated Show resolved Hide resolved

Merge branch 'master' of https://github.com/grpc/grpc-java into httpf…

2d78841

…ilter

voidzcy reviewed Mar 4, 2021

View reviewed changes

minor cleanup

23781b9

voidzcy reviewed Mar 4, 2021

View reviewed changes

dapengzhang0 added 3 commits March 4, 2021 16:53

change parseFilterConfig argument type

c9fda44

Merge branch 'master' of https://github.com/grpc/grpc-java into httpf…

d6857a3

…ilter

remove empty line

d2ba7df

voidzcy self-requested a review March 8, 2021 19:16

voidzcy reviewed Mar 8, 2021

View reviewed changes

fix typo

884cc4c

voidzcy reviewed Mar 9, 2021

View reviewed changes

use util method to avoid duplicate code

dc46c71

voidzcy reviewed Mar 9, 2021

View reviewed changes

voidzcy reviewed Mar 10, 2021

View reviewed changes

dapengzhang0 added 3 commits March 10, 2021 10:57

futhure reduce duplicate code

6e8f269

enhance error message

5027c65

remove unused fields in FaultConfig data type

193677d

voidzcy reviewed Mar 11, 2021

View reviewed changes

voidzcy mentioned this pull request Mar 11, 2021

xds: add server side Listener processing to ClientXdsClient #7955

Merged

change generateServiceConfig() signature

700305d

voidzcy reviewed Mar 12, 2021

View reviewed changes

xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java Outdated Show resolved Hide resolved

voidzcy reviewed Mar 12, 2021

View reviewed changes

dapengzhang0 added 2 commits March 12, 2021 11:11

Merge branch 'master' of https://github.com/grpc/grpc-java into httpf…

db898a5

…ilter

move test

ab14e89

append LameFilter

69356f1

dapengzhang0 force-pushed the httpfilter branch from 1c2e369 to 69356f1 Compare March 12, 2021 23:47

override toString() for filter config

4805d43

voidzcy reviewed Mar 15, 2021

View reviewed changes

xds/src/main/java/io/grpc/xds/XdsClient.java Show resolved Hide resolved

voidzcy reviewed Mar 15, 2021

View reviewed changes

xds/src/main/java/io/grpc/xds/XdsNameResolver.java Outdated Show resolved Hide resolved

voidzcy reviewed Mar 15, 2021

View reviewed changes

dapengzhang0 added 7 commits March 16, 2021 22:29

Merge branch 'master' of https://github.com/grpc/grpc-java into httpf…

2552468

…ilter

simplify field assignment

1a9141b

remove hasLameFilter()

641dfe3

equals() and hashCode()

c8298cc

add HttpFilter to printables

934815a

refactor Filter.Registry

9485e53

refactor NamedFilter

10c2646

voidzcy approved these changes Mar 17, 2021

View reviewed changes

rename Filter.StructOrError to Filter.ConfigOrError

937f2b3

dapengzhang0 merged commit 3ebb3e1 into grpc:master Mar 17, 2021

dapengzhang0 deleted the httpfilter branch March 17, 2021 23:37

github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2021

xds: HttpFilter support #7932

xds: HttpFilter support #7932

Conversation

dapengzhang0 commented Mar 2, 2021

voidzcy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voidzcy Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dapengzhang0 Mar 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voidzcy Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voidzcy Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voidzcy Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voidzcy Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dapengzhang0 Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voidzcy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dapengzhang0 commented Mar 12, 2021

voidzcy commented Mar 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

voidzcy left a comment • edited Loading

Choose a reason for hiding this comment

voidzcy Mar 11, 2021 •

edited

Loading

dapengzhang0 Mar 4, 2021 •

edited

Loading

voidzcy Mar 10, 2021 •

edited

Loading

voidzcy Mar 11, 2021 •

edited

Loading

voidzcy Mar 11, 2021 •

edited

Loading

voidzcy Mar 11, 2021 •

edited

Loading

dapengzhang0 Mar 12, 2021 •

edited

Loading

voidzcy left a comment •

edited

Loading