From 934c9895e59a1992a208e4cc935b067027d51f84 Mon Sep 17 00:00:00 2001 From: Sergii Tkachenko Date: Mon, 10 Feb 2025 11:31:54 -0800 Subject: [PATCH] Address the feedback: variable names; generate fc name from idx --- .../java/io/grpc/xds/XdsListenerResource.java | 57 ++++++++++--------- .../grpc/xds/GrpcXdsClientImplDataTest.java | 36 +++++++----- 2 files changed, 53 insertions(+), 40 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/XdsListenerResource.java b/xds/src/main/java/io/grpc/xds/XdsListenerResource.java index b9633d09189..a18b093e38f 100644 --- a/xds/src/main/java/io/grpc/xds/XdsListenerResource.java +++ b/xds/src/main/java/io/grpc/xds/XdsListenerResource.java @@ -146,21 +146,19 @@ static EnvoyServerProtoData.Listener parseServerSideListener( Listener proto, TlsContextManager tlsContextManager, FilterRegistry filterRegistry, Set certProviderInstances, XdsResourceType.Args args) throws ResourceInvalidException { - String listenerName = proto.getName(); - TrafficDirection trafficDirection = proto.getTrafficDirection(); if (!trafficDirection.equals(TrafficDirection.INBOUND) && !trafficDirection.equals(TrafficDirection.UNSPECIFIED)) { throw new ResourceInvalidException( - "Listener " + listenerName + " with invalid traffic direction: " + trafficDirection); + "Listener " + proto.getName() + " with invalid traffic direction: " + trafficDirection); } if (!proto.getListenerFiltersList().isEmpty()) { throw new ResourceInvalidException( - "Listener " + listenerName + " cannot have listener_filters"); + "Listener " + proto.getName() + " cannot have listener_filters"); } if (proto.hasUseOriginalDst()) { throw new ResourceInvalidException( - "Listener " + listenerName + " cannot have use_original_dst set to true"); + "Listener " + proto.getName() + " cannot have use_original_dst set to true"); } String address = null; @@ -180,54 +178,62 @@ static EnvoyServerProtoData.Listener parseServerSideListener( } ImmutableList.Builder filterChains = ImmutableList.builder(); - Set foundFilterChainMatches = new HashSet<>(); + Set 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, foundFilterChainMatches, - certProviderInstances, args)); + parseFilterChain(fc, filterChainName, tlsContextManager, filterRegistry, + filterChainMatchSet, certProviderInstances, args)); + i++; } - // Default filter chain. 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); } return EnvoyServerProtoData.Listener.create( - listenerName, address, filterChains.build(), defaultFilterChain); + proto.getName(), address, filterChains.build(), defaultFilterChain); } @VisibleForTesting static FilterChain parseFilterChain( io.envoyproxy.envoy.config.listener.v3.FilterChain proto, + String filterChainName, TlsContextManager tlsContextManager, FilterRegistry filterRegistry, // null disables FilterChainMatch uniqueness check, used for defaultFilterChain - @Nullable Set foundFilterChainMatches, + @Nullable Set filterChainMatchSet, Set certProviderInstances, XdsResourceType.Args args) throws ResourceInvalidException { - // May be empty. Not required to be unique. - String filterChainName = proto.getName(); - // FilterChain contains L4 filters, so we ensure it contains only HCM. if (proto.getFiltersCount() != 1) { throw new ResourceInvalidException("FilterChain " + filterChainName + " should contain exact one HttpConnectionManager filter"); } io.envoyproxy.envoy.config.listener.v3.Filter l4Filter = proto.getFiltersList().get(0); - String hcmName = l4Filter.getName(); if (!l4Filter.hasTypedConfig()) { throw new ResourceInvalidException( - "FilterChain " + filterChainName + " contains filter " + hcmName + "FilterChain " + filterChainName + " contains filter " + l4Filter.getName() + " without typed_config"); } Any any = l4Filter.getTypedConfig(); if (!any.getTypeUrl().equals(TYPE_URL_HTTP_CONNECTION_MANAGER)) { throw new ResourceInvalidException( - "FilterChain " + filterChainName + " contains filter " + hcmName + "FilterChain " + filterChainName + " contains filter " + l4Filter.getName() + " with unsupported typed_config type " + any.getTypeUrl()); } @@ -237,7 +243,7 @@ static FilterChain parseFilterChain( hcmProto = any.unpack(HttpConnectionManager.class); } catch (InvalidProtocolBufferException e) { throw new ResourceInvalidException("FilterChain " + filterChainName + " with filter " - + hcmName + " failed to unpack message", e); + + l4Filter.getName() + " failed to unpack message", e); } io.grpc.xds.HttpConnectionManager httpConnectionManager = parseHttpConnectionManager( hcmProto, filterRegistry, false /* isForClient */, args); @@ -265,8 +271,8 @@ static FilterChain parseFilterChain( // Parse FilterChainMatch. FilterChainMatch filterChainMatch = parseFilterChainMatch(proto.getFilterChainMatch()); // null used to skip this check for defaultFilterChain. - if (foundFilterChainMatches != null) { - checkForUniqueness(foundFilterChainMatches, filterChainMatch); + if (filterChainMatchSet != null) { + validateFilterChainMatchForUniqueness(filterChainMatchSet, filterChainMatch); } return FilterChain.create( @@ -304,14 +310,13 @@ static DownstreamTlsContext validateDownstreamTlsContext( return downstreamTlsContext; } - private static void checkForUniqueness( - Set foundFilterChainMatches, - FilterChainMatch filterChainMatch) - throws ResourceInvalidException { + private static void validateFilterChainMatchForUniqueness( + Set filterChainMatchSet, + FilterChainMatch filterChainMatch) throws ResourceInvalidException { // Flattens complex FilterChainMatch into a list of simple FilterChainMatch'es. List crossProduct = getCrossProduct(filterChainMatch); for (FilterChainMatch cur : crossProduct) { - if (!foundFilterChainMatches.add(cur)) { + if (!filterChainMatchSet.add(cur)) { throw new ResourceInvalidException("FilterChainMatch must be unique. " + "Found duplicate: " + cur); } diff --git a/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java b/xds/src/test/java/io/grpc/xds/GrpcXdsClientImplDataTest.java index 5b9fdda1127..314b2094480 100644 --- 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