xds: improve code readability of server FilterChain parsing

sergiitk · web-flow · commit bd6af59221fa · 2025-02-10T17:14:07.000-08:00
- Improve code flow and variable names
- Reduce nesting
- Add comments between logical blocks
- Add comments explaining some xDS/gRPC nuances
diff --git a/xds/src/main/java/io/grpc/xds/XdsListenerResource.java b/xds/src/main/java/io/grpc/xds/XdsListenerResource.java
@@ -146,11 +146,11 @@ static EnvoyServerProtoData.Listener parseServerSideListener(
       Listener proto, TlsContextManager tlsContextManager,
       FilterRegistry filterRegistry, Set<String> certProviderInstances, XdsResourceType.Args args)
       throws ResourceInvalidException {
-    if (!proto.getTrafficDirection().equals(TrafficDirection.INBOUND)
-        && !proto.getTrafficDirection().equals(TrafficDirection.UNSPECIFIED)) {
+    TrafficDirection trafficDirection = proto.getTrafficDirection();
+    if (!trafficDirection.equals(TrafficDirection.INBOUND)
+        && !trafficDirection.equals(TrafficDirection.UNSPECIFIED)) {
       throw new ResourceInvalidException(
-          "Listener " + proto.getName() + " with invalid traffic direction: "
-              + proto.getTrafficDirection());
+          "Listener " + proto.getName() + " with invalid traffic direction: " + trafficDirection);
     }
     if (!proto.getListenerFiltersList().isEmpty()) {
       throw new ResourceInvalidException(
@@ -178,16 +178,29 @@ static EnvoyServerProtoData.Listener parseServerSideListener(
     }
 
     ImmutableList.Builder<FilterChain> filterChains = ImmutableList.builder();
-    Set<FilterChainMatch> uniqueSet = new HashSet<>();
+    Set<FilterChainMatch> filterChainMatchSet = new HashSet<>();
+    int i = 0;
     for (io.envoyproxy.envoy.config.listener.v3.FilterChain fc : proto.getFilterChainsList()) {
+      // May be empty. If it's not empty, required to be unique.
+      String filterChainName = fc.getName();
+      if (filterChainName.isEmpty()) {
+        // Generate a name, so we can identify it in the logs.
+        filterChainName = "chain_" + i;
+      }
       filterChains.add(
-          parseFilterChain(fc, tlsContextManager, filterRegistry, uniqueSet,
-              certProviderInstances, args));
+          parseFilterChain(fc, filterChainName, tlsContextManager, filterRegistry,
+              filterChainMatchSet, certProviderInstances, args));
+      i++;
     }
+
     FilterChain defaultFilterChain = null;
     if (proto.hasDefaultFilterChain()) {
+      String defaultFilterChainName = proto.getDefaultFilterChain().getName();
+      if (defaultFilterChainName.isEmpty()) {
+        defaultFilterChainName = "chain_default";
+      }
       defaultFilterChain = parseFilterChain(
-          proto.getDefaultFilterChain(), tlsContextManager, filterRegistry,
+          proto.getDefaultFilterChain(), defaultFilterChainName, tlsContextManager, filterRegistry,
           null, certProviderInstances, args);
     }
 
@@ -198,36 +211,44 @@ static EnvoyServerProtoData.Listener parseServerSideListener(
   @VisibleForTesting
   static FilterChain parseFilterChain(
       io.envoyproxy.envoy.config.listener.v3.FilterChain proto,
-      TlsContextManager tlsContextManager, FilterRegistry filterRegistry,
-      Set<FilterChainMatch> uniqueSet, Set<String> certProviderInstances, XdsResourceType.Args args)
+      String filterChainName,
+      TlsContextManager tlsContextManager,
+      FilterRegistry filterRegistry,
+      // null disables FilterChainMatch uniqueness check, used for defaultFilterChain
+      @Nullable Set<FilterChainMatch> filterChainMatchSet,
+      Set<String> certProviderInstances,
+      XdsResourceType.Args args)
       throws ResourceInvalidException {
+    // FilterChain contains L4 filters, so we ensure it contains only HCM.
     if (proto.getFiltersCount() != 1) {
-      throw new ResourceInvalidException("FilterChain " + proto.getName()
+      throw new ResourceInvalidException("FilterChain " + filterChainName
           + " should contain exact one HttpConnectionManager filter");
     }
-    io.envoyproxy.envoy.config.listener.v3.Filter filter = proto.getFiltersList().get(0);
-    if (!filter.hasTypedConfig()) {
+    io.envoyproxy.envoy.config.listener.v3.Filter l4Filter = proto.getFiltersList().get(0);
+    if (!l4Filter.hasTypedConfig()) {
       throw new ResourceInvalidException(
-          "FilterChain " + proto.getName() + " contains filter " + filter.getName()
+          "FilterChain " + filterChainName + " contains filter " + l4Filter.getName()
               + " without typed_config");
     }
-    Any any = filter.getTypedConfig();
-    // HttpConnectionManager is the only supported network filter at the moment.
+    Any any = l4Filter.getTypedConfig();
     if (!any.getTypeUrl().equals(TYPE_URL_HTTP_CONNECTION_MANAGER)) {
       throw new ResourceInvalidException(
-          "FilterChain " + proto.getName() + " contains filter " + filter.getName()
+          "FilterChain " + filterChainName + " contains filter " + l4Filter.getName()
               + " with unsupported typed_config type " + any.getTypeUrl());
     }
+
+    // Parse HCM.
     HttpConnectionManager hcmProto;
     try {
       hcmProto = any.unpack(HttpConnectionManager.class);
     } catch (InvalidProtocolBufferException e) {
-      throw new ResourceInvalidException("FilterChain " + proto.getName() + " with filter "
-          + filter.getName() + " failed to unpack message", e);
+      throw new ResourceInvalidException("FilterChain " + filterChainName + " with filter "
+          + l4Filter.getName() + " failed to unpack message", e);
     }
     io.grpc.xds.HttpConnectionManager httpConnectionManager = parseHttpConnectionManager(
         hcmProto, filterRegistry, false /* isForClient */, args);
 
+    // Parse Transport Socket.
     EnvoyServerProtoData.DownstreamTlsContext downstreamTlsContext = null;
     if (proto.hasTransportSocket()) {
       if (!TRANSPORT_SOCKET_NAME_TLS.equals(proto.getTransportSocket().getName())) {
@@ -239,18 +260,23 @@ static FilterChain parseFilterChain(
         downstreamTlsContextProto =
             proto.getTransportSocket().getTypedConfig().unpack(DownstreamTlsContext.class);
       } catch (InvalidProtocolBufferException e) {
-        throw new ResourceInvalidException("FilterChain " + proto.getName()
+        throw new ResourceInvalidException("FilterChain " + filterChainName
             + " failed to unpack message", e);
       }
       downstreamTlsContext =
           EnvoyServerProtoData.DownstreamTlsContext.fromEnvoyProtoDownstreamTlsContext(
               validateDownstreamTlsContext(downstreamTlsContextProto, certProviderInstances));
     }
 
+    // Parse FilterChainMatch.
     FilterChainMatch filterChainMatch = parseFilterChainMatch(proto.getFilterChainMatch());
-    checkForUniqueness(uniqueSet, filterChainMatch);
+    // null used to skip this check for defaultFilterChain.
+    if (filterChainMatchSet != null) {
+      validateFilterChainMatchForUniqueness(filterChainMatchSet, filterChainMatch);
+    }
+
     return FilterChain.create(
-        proto.getName(),
+        filterChainName,
         filterChainMatch,
         httpConnectionManager,
         downstreamTlsContext,
@@ -284,15 +310,15 @@ static DownstreamTlsContext validateDownstreamTlsContext(
     return downstreamTlsContext;
   }
 
-  private static void checkForUniqueness(Set<FilterChainMatch> uniqueSet,
+  private static void validateFilterChainMatchForUniqueness(
+      Set<FilterChainMatch> filterChainMatchSet,
       FilterChainMatch filterChainMatch) throws ResourceInvalidException {
-    if (uniqueSet != null) {
-      List<FilterChainMatch> crossProduct = getCrossProduct(filterChainMatch);
-      for (FilterChainMatch cur : crossProduct) {
-        if (!uniqueSet.add(cur)) {
-          throw new ResourceInvalidException("FilterChainMatch must be unique. "
-              + "Found duplicate: " + cur);
-        }
+    // Flattens complex FilterChainMatch into a list of simple FilterChainMatch'es.
+    List<FilterChainMatch> crossProduct = getCrossProduct(filterChainMatch);
+    for (FilterChainMatch cur : crossProduct) {
+      if (!filterChainMatchSet.add(cur)) {
+        throw new ResourceInvalidException("FilterChainMatch must be unique. "
+            + "Found duplicate: " + cur);
       }
     }
   }
diff --git a/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java b/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java
@@ -60,7 +60,6 @@
 import java.io.IOException;
 import java.net.SocketAddress;
 import java.util.ArrayList;
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -459,47 +458,69 @@ private void shutdown() {
     }
 
     private void updateSelector() {
-      Map<FilterChain, AtomicReference<ServerRoutingConfig>> filterChainRouting = new HashMap<>();
+      // This is regenerated in generateRoutingConfig() calls below.
       savedRdsRoutingConfigRef.clear();
+
+      // Prepare server routing config map.
+      ImmutableMap.Builder<FilterChain, AtomicReference<ServerRoutingConfig>> routingConfigs =
+          ImmutableMap.builder();
       for (FilterChain filterChain: filterChains) {
-        filterChainRouting.put(filterChain, generateRoutingConfig(filterChain));
+        routingConfigs.put(filterChain, generateRoutingConfig(filterChain));
       }
-      FilterChainSelector selector = new FilterChainSelector(
-          Collections.unmodifiableMap(filterChainRouting),
-          defaultFilterChain == null ? null : defaultFilterChain.sslContextProviderSupplier(),
-          defaultFilterChain == null ? new AtomicReference<ServerRoutingConfig>() :
-              generateRoutingConfig(defaultFilterChain));
-      List<SslContextProviderSupplier> toRelease = getSuppliersInUse();
+
+      // Prepare the new selector.
+      FilterChainSelector selector;
+      if (defaultFilterChain != null) {
+        selector = new FilterChainSelector(
+            routingConfigs.build(),
+            defaultFilterChain.sslContextProviderSupplier(),
+            generateRoutingConfig(defaultFilterChain));
+      } else {
+        selector = new FilterChainSelector(routingConfigs.build(), null, new AtomicReference<>());
+      }
+
+      // Prepare the list of current selector's resources to close later.
+      List<SslContextProviderSupplier> oldSslSuppliers = getSuppliersInUse();
+
+      // Swap the selectors, initiate a graceful shutdown of the old one.
       logger.log(Level.FINEST, "Updating selector {0}", selector);
       filterChainSelectorManager.updateSelector(selector);
-      for (SslContextProviderSupplier e: toRelease) {
-        e.close();
+
+      // Release old resources.
+      for (SslContextProviderSupplier supplier: oldSslSuppliers) {
+        supplier.close();
       }
+
+      // Now that we have valid Transport Socket config, we can start/restart listening on a port.
       startDelegateServer();
     }
 
     private AtomicReference<ServerRoutingConfig> generateRoutingConfig(FilterChain filterChain) {
       HttpConnectionManager hcm = filterChain.httpConnectionManager();
+      ImmutableMap<Route, ServerInterceptor> interceptors;
+
+      // Inlined routes.
       if (hcm.virtualHosts() != null) {
-        ImmutableMap<Route, ServerInterceptor> interceptors = generatePerRouteInterceptors(
-                hcm.httpFilterConfigs(), hcm.virtualHosts());
-        return new AtomicReference<>(ServerRoutingConfig.create(hcm.virtualHosts(),interceptors));
+        interceptors = generatePerRouteInterceptors(hcm.httpFilterConfigs(), hcm.virtualHosts());
+        return new AtomicReference<>(ServerRoutingConfig.create(hcm.virtualHosts(), interceptors));
+      }
+
+      // Routes from RDS.
+      RouteDiscoveryState rds = routeDiscoveryStates.get(hcm.rdsName());
+      checkNotNull(rds, "rds");
+
+      ServerRoutingConfig routingConfig;
+      ImmutableList<VirtualHost> savedVhosts = rds.savedVirtualHosts;
+      if (savedVhosts != null) {
+        interceptors = generatePerRouteInterceptors(hcm.httpFilterConfigs(), savedVhosts);
+        routingConfig = ServerRoutingConfig.create(savedVhosts, interceptors);
       } else {
-        RouteDiscoveryState rds = routeDiscoveryStates.get(hcm.rdsName());
-        checkNotNull(rds, "rds");
-        AtomicReference<ServerRoutingConfig> serverRoutingConfigRef = new AtomicReference<>();
-        if (rds.savedVirtualHosts != null) {
-          ImmutableMap<Route, ServerInterceptor> interceptors = generatePerRouteInterceptors(
-              hcm.httpFilterConfigs(), rds.savedVirtualHosts);
-          ServerRoutingConfig serverRoutingConfig =
-              ServerRoutingConfig.create(rds.savedVirtualHosts, interceptors);
-          serverRoutingConfigRef.set(serverRoutingConfig);
-        } else {
-          serverRoutingConfigRef.set(ServerRoutingConfig.FAILING_ROUTING_CONFIG);
-        }
-        savedRdsRoutingConfigRef.put(filterChain, serverRoutingConfigRef);
-        return serverRoutingConfigRef;
+        routingConfig = ServerRoutingConfig.FAILING_ROUTING_CONFIG;
       }
+
+      AtomicReference<ServerRoutingConfig> routingConfigRef = new AtomicReference<>(routingConfig);
+      savedRdsRoutingConfigRef.put(filterChain, routingConfigRef);
+      return routingConfigRef;
     }
 
     private ImmutableMap<Route, ServerInterceptor> generatePerRouteInterceptors(
diff --git a/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java b/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java
@@ -2719,7 +2719,7 @@ public void parseFilterChain_noHcm() throws ResourceInvalidException {
     thrown.expectMessage(
         "FilterChain filter-chain-foo should contain exact one HttpConnectionManager filter");
     XdsListenerResource.parseFilterChain(
-        filterChain, null, filterRegistry, null, null,
+        filterChain, "filter-chain-foo", null, filterRegistry, null, null,
         getXdsResourceTypeArgs(true));
   }
 
@@ -2738,7 +2738,7 @@ public void parseFilterChain_duplicateFilter() throws ResourceInvalidException {
     thrown.expectMessage(
         "FilterChain filter-chain-foo should contain exact one HttpConnectionManager filter");
     XdsListenerResource.parseFilterChain(
-        filterChain, null, filterRegistry, null, null,
+        filterChain, "filter-chain-foo", null, filterRegistry, null, null,
         getXdsResourceTypeArgs(true));
   }
 
@@ -2757,7 +2757,7 @@ public void parseFilterChain_filterMissingTypedConfig() throws ResourceInvalidEx
         "FilterChain filter-chain-foo contains filter envoy.http_connection_manager "
             + "without typed_config");
     XdsListenerResource.parseFilterChain(
-        filterChain, null, filterRegistry, null, null,
+        filterChain, "filter-chain-foo", null, filterRegistry, null, null,
         getXdsResourceTypeArgs(true));
   }
 
@@ -2780,13 +2780,13 @@ public void parseFilterChain_unsupportedFilter() throws ResourceInvalidException
         "FilterChain filter-chain-foo contains filter unsupported with unsupported "
             + "typed_config type unsupported-type-url");
     XdsListenerResource.parseFilterChain(
-        filterChain, null, filterRegistry, null, null,
+        filterChain, "filter-chain-foo", null, filterRegistry, null, null,
         getXdsResourceTypeArgs(true));
   }
 
   @Test
   public void parseFilterChain_noName() throws ResourceInvalidException {
-    FilterChain filterChain1 =
+    FilterChain filterChain0 =
         FilterChain.newBuilder()
             .setFilterChainMatch(FilterChainMatch.getDefaultInstance())
             .addFilters(buildHttpConnectionManagerFilter(
@@ -2796,9 +2796,11 @@ public void parseFilterChain_noName() throws ResourceInvalidException {
                     .setTypedConfig(Any.pack(Router.newBuilder().build()))
                     .build()))
             .build();
-    FilterChain filterChain2 =
+
+    FilterChain filterChain1 =
         FilterChain.newBuilder()
-            .setFilterChainMatch(FilterChainMatch.getDefaultInstance())
+            .setFilterChainMatch(
+                FilterChainMatch.newBuilder().addAllSourcePorts(Arrays.asList(443, 8080)))
             .addFilters(buildHttpConnectionManagerFilter(
                 HttpFilter.newBuilder()
                     .setName("http-filter-bar")
@@ -2807,13 +2809,19 @@ public void parseFilterChain_noName() throws ResourceInvalidException {
                     .build()))
             .build();
 
-    EnvoyServerProtoData.FilterChain parsedFilterChain1 = XdsListenerResource.parseFilterChain(
-        filterChain1, null, filterRegistry, null,
-        null, getXdsResourceTypeArgs(true));
-    EnvoyServerProtoData.FilterChain parsedFilterChain2 = XdsListenerResource.parseFilterChain(
-        filterChain2, null, filterRegistry, null,
-        null, getXdsResourceTypeArgs(true));
-    assertThat(parsedFilterChain1.name()).isEqualTo(parsedFilterChain2.name());
+    Listener listenerProto =
+        Listener.newBuilder()
+            .setName("listener1")
+            .setTrafficDirection(TrafficDirection.INBOUND)
+            .addAllFilterChains(Arrays.asList(filterChain0, filterChain1))
+            .setDefaultFilterChain(filterChain0)
+            .build();
+    EnvoyServerProtoData.Listener listener = XdsListenerResource.parseServerSideListener(
+        listenerProto, null, filterRegistry, null, getXdsResourceTypeArgs(true));
+
+    assertThat(listener.filterChains().get(0).name()).isEqualTo("chain_0");
+    assertThat(listener.filterChains().get(1).name()).isEqualTo("chain_1");
+    assertThat(listener.defaultFilterChain().name()).isEqualTo("chain_default");
   }
 
   @Test
