xds: unify client and server handling HttpConnectionManager #8228

voidzcy · 2021-06-02T22:23:28Z

Enables parsing HttpConnectionManager filter for the server side TCP listener, with the same codepath for handling it on the client side. Major changes include:

Remodeled LdsUpdate with HttpConnectionManager. Now LdsUpdate is an oneof of HttpConnectionManager (for client side) or Listener (for server side). Each of Listener's FiliterChain contains an HttpConnectionManager (required).
Refactored code for validating and parsing the TCP Listener (for server side), put it into ClientXdsClient. The common part of validating/parsing HttpConnectionManager is reused/shared for client side.
Included the name of FilterChain in the parsed form. As specified by the API, each FilterChain has a unique name. If the name is not provided by the control plane, a UUID is used. FilterChain names can be used for bookkeeping a set of FilterChain easily (e.g., used as map key).
Added methods isSupportedOnClients() and isSupportedOnServers() to the Filter interface. Parsing the top-level HttpFilter requires knowing if the HttpFilter implementation is supported for the target usage (client-side or server-side). Note, parsing override HttpFilter configs does not need to know whether the config is used for an HttpFilter that is only supported for the client-side or server side.
Added a new kind of Route: Route with non-forwarding action. Updated the XdsNameResolver being able to handle Route with non-forwarding action: if such a Route is matched to an RPC, that RPC is failed. Note, it is possible that XdsNameResolver receives xDS updates with all Routes with non-forwarding action. That is, the service config will not reference any cluster. Such case can be handled by cluster_manager LB policy's LB config parser: the parser returns the error to Channel and the Channel will handle it as error service config.

…I listener and network filter in TCP listener's filter chain.

…nectionManager.

…ent.

…unified_lds_message_handler_for_client_and_server

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

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

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

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

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

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

…nt and server side. This adds the requirement for server side HttpConnectionManager to have either RDS or inlined RouteConfiguration.

…unified_lds_message_handler_for_client_and_server

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

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

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

…ering cluster_manager LB policy config parser is able to handle config with empty childPolicy.

voidzcy · 2021-06-15T17:47:15Z

Added support for Route with non-forwarding action and did more cleanup for tests. Also, the PR description is updated. PTAL.

voidzcy · 2021-06-15T22:24:52Z

Added a test case for covering parsing RBACPerRoute. CIs should pass after #8262 is merged.

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

…unified_lds_message_handler_for_client_and_server

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

sanjaypujare · 2021-06-17T04:49:46Z

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

+            "FilterChain " + proto.getName() + " contains filter " + filter.getName()
+                + " with unsupported typed_config type " + any.getTypeUrl());
+      }
+      // Any filters after the first HttpConnectionManager are ignored.


The code doesn't actually ignore filters after the first HCM. If you break the loop after line 335 it would have.

Why? there is a if (httpConnectionManager == null) statement guarding the code of parsing HCM. We still want to loop over the list of network filters, just in case there are filters with duplicated name or filters other than HCM.

... We still want to loop over the list of network filters, just in case there are filters with duplicated name or filters other than HCM.

That is not ignoring and the comment is wrong. But if the code matches the spec (from A36 gRFC) then it's a minor issue. The spec tells to validate all of them but not use any for processing. So the code seems to be right (but the comment doesn't match)

Well, the comment is really intend to mean we will take the first HttpConnectionManager. I will update or delete the comment if you think that is confusing.

Deleted the comment.

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

sanjaypujare · 2021-06-17T06:11:55Z

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

-    if (filter == null) {
+    Filter filter = filterRegistry.get(typeUrl);
+    if (filter == null || (!isForClient && filter.isSupportedOnClients())
+        || (isForClient && filter.isSupportedOnServers())) {


This expression should be:

if (filter == null || (isForClient && !filter.isSupportedOnClients()) || (!isForClient && !filter.isSupportedOnServers()))

Basically: if I am processing for-client but the filter-is-not-supported-on-clients OR I am processing for-server but the filter-is-not-supported-on-servers

The 2 are not equivalent because isSupportedOnClients() and isSupportedOnServers() are not opposite of each other.

Tests should also be added/modified to enforce the correct logic.

Ahhh, good catch. !isForClient == isForServer, this is awkward. I wish I could use an enum instead of a boolean. But thought that's an overkill for such a small thing. I will fix and add some test.

Fixed, and enabled Router filter for server side. It will be used for both client and server, and we can easily use it in the test to cover this logic. Also, I deleted the newly added interface methods isSupportedOnClients() and isSupportedOnServers(). Those are redundant, can just check with instance of ClientInterceptorBuilder and instance of ServerInterceptorBuilder. It's cumbersome to have two sources of truth.

Those are redundant, can just check with instance of ClientInterceptorBuilder and instance of ServerInterceptorBuilder. It's cumbersome to have two sources of truth.

Yes but isSupported... are more readable and I prefer those. These could have been made default methods in the interface with the correct Java version.

sanjaypujare · 2021-06-17T06:48:26Z

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

      }
+      Message rawConfig = anyConfig;
+      if (anyConfig.getTypeUrl().equals(TYPE_URL_TYPED_STRUCT)) {


anyConfig.getTypeUrl() should be same as typeUrl so no need to use the longer expression?

Sounds good. Updated.

sanjaypujare · 2021-06-17T07:04:18Z

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

      case FILTER_ACTION:
-        return StructOrError.fromError("Unsupported action type: filter_action");
+        return StructOrError.fromError(
+            "Route [" + proto.getName() + "] with unsupported action type: filter_action");


Why not simplify:

case REDIRECT: case DIRECT_RESPONSE: case FILTER_ACTION: return StructOrError.fromError( "Route [" + proto.getName() + "] with unsupported action type: " + proto.getActionCase());

…mentation is supported for client/server. Also implement the Router filter for server side's usage.

…entation can be used for client/server.

…ops.

sanjaypujare · 2021-06-18T04:30:18Z

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

      if (isOptional) {
        return null;
      } else {
        return StructOrError.fromError(
-            "HttpFilter [" + filterName + "] is not optional and has an unsupported config type: "
-                + typeUrl);
+            "HttpFilter [" + filterName + "](" + typeUrl + ") is required but unsupported for "


I noticed you don't have a test to verify this error generation. So the original bug on lines 523-524 went undetected.

Actually it is tested. See tests started with parseHttpFilter_:

The RouterFilter is supported for both client and server, so we use it to cover the branch isForClient && (filter instanceof ClientInterceptorBuilder) and !isForClient && (filter instanceof ServerInterceptorBuilder).

The FaultFilter is supported for client only, so we used it to cover the branch !isForClient && !(filter instanceof ServerInterceptorBuilder)

The RbacFilter is supported for server only, so we used it to cover the branch isForClient && !(filter instanceof ClientInterceptorBuilder)

The four branches covered all forClient + supported/unsupported and forServer + supported/unsupported cases.

Eliminated the boolean zen in line 523-524, filter == null || is not needed, its included in instanceof.

sanjaypujare · 2021-06-18T04:42:24Z

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

 import java.util.concurrent.ScheduledExecutorService;
 import javax.annotation.Nullable;

 /**
 * Router filter implementation. Currently this filter does not parse any field in the config.
 */
-enum RouterFilter implements Filter, ClientInterceptorBuilder {
+enum RouterFilter implements Filter, ClientInterceptorBuilder, ServerInterceptorBuilder {


Not from this PR but why use enum when clearly this type is not used to enumerate constants? enum works given how this is used but I wouldn't think of the RouterFilter as an enum.

In Java, enum types are considered to be a special type of class. When creating an enum class, the compiler will create instances (objects) of each enum constants. So enum can be an easy trick to create singletons.

sanjaypujare

LG. I have left a comment about an error case not being tested. It will be good to address that.

voidzcy added 8 commits June 2, 2021 15:12

Create HttpConnectionManager datatype to model HCM received as the AP…

18060b4

…I listener and network filter in TCP listener's filter chain.

Update LdsUpdate definition to use HttpConnection data model.

0883219

Update existing usages for LdsUpdate with data populated from HttpCon…

6753f15

…nectionManager.

Move logics for parsing and validating TCP listener into ClientXdsCli…

fbc3bd1

…ent.

Fix tests.

c825bd0

Merge branch 'master' of github.com:grpc/grpc-java into refactor/use_…

308da0b

…unified_lds_message_handler_for_client_and_server

Revert excessive changes.

4837170

Listener address port is optional for server side TCP listener.

57ec58d

voidzcy marked this pull request as ready for review June 3, 2021 21:20

voidzcy requested review from dapengzhang0 and sanjaypujare June 3, 2021 21:40

Minor polishment.

bb967b3

sanjaypujare reviewed Jun 7, 2021

View reviewed changes

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

sanjaypujare reviewed Jun 7, 2021

View reviewed changes

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

sanjaypujare reviewed Jun 7, 2021

View reviewed changes

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