Skip to content

Commit a6df9de

Browse files
YifeiZhuangYifeiZhuang
authored
xds: add terminal http filter verification, remove lame route filter, add hcm as terminal network filter verification (grpc#8342)
* xds: add terminal filter verification, remove lame route filter * move last filter check inline * add server validate terminal filter
1 parent 67d5f1b commit a6df9de

8 files changed

Lines changed: 174 additions & 239 deletions

File tree

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

Lines changed: 44 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -334,41 +334,32 @@ static FilterChain parseFilterChain(
334334
TlsContextManager tlsContextManager, FilterRegistry filterRegistry,
335335
Set<FilterChainMatch> uniqueSet, Set<String> certProviderInstances, boolean parseHttpFilters)
336336
throws ResourceInvalidException {
337-
io.grpc.xds.HttpConnectionManager httpConnectionManager = null;
338-
HashSet<String> uniqueNames = new HashSet<>();
339-
for (io.envoyproxy.envoy.config.listener.v3.Filter filter : proto.getFiltersList()) {
340-
if (!uniqueNames.add(filter.getName())) {
341-
throw new ResourceInvalidException(
342-
"FilterChain " + proto.getName() + " with duplicated filter: " + filter.getName());
343-
}
344-
if (!filter.hasTypedConfig()) {
345-
throw new ResourceInvalidException(
346-
"FilterChain " + proto.getName() + " contains filter " + filter.getName()
347-
+ " without typed_config");
348-
}
349-
Any any = filter.getTypedConfig();
350-
// HttpConnectionManager is the only supported network filter at the moment.
351-
if (!any.getTypeUrl().equals(TYPE_URL_HTTP_CONNECTION_MANAGER)) {
352-
throw new ResourceInvalidException(
353-
"FilterChain " + proto.getName() + " contains filter " + filter.getName()
354-
+ " with unsupported typed_config type " + any.getTypeUrl());
355-
}
356-
if (httpConnectionManager == null) {
357-
HttpConnectionManager hcmProto;
358-
try {
359-
hcmProto = any.unpack(HttpConnectionManager.class);
360-
} catch (InvalidProtocolBufferException e) {
361-
throw new ResourceInvalidException("FilterChain " + proto.getName() + " with filter "
362-
+ filter.getName() + " failed to unpack message", e);
363-
}
364-
httpConnectionManager = parseHttpConnectionManager(
365-
hcmProto, rdsResources, filterRegistry, parseHttpFilters, false /* isForClient */);
366-
}
367-
}
368-
if (httpConnectionManager == null) {
337+
if (proto.getFiltersCount() != 1) {
369338
throw new ResourceInvalidException("FilterChain " + proto.getName()
370-
+ " missing required HttpConnectionManager filter");
339+
+ " should contain exact one HttpConnectionManager filter");
371340
}
341+
io.envoyproxy.envoy.config.listener.v3.Filter filter = proto.getFiltersList().get(0);
342+
if (!filter.hasTypedConfig()) {
343+
throw new ResourceInvalidException(
344+
"FilterChain " + proto.getName() + " contains filter " + filter.getName()
345+
+ " without typed_config");
346+
}
347+
Any any = filter.getTypedConfig();
348+
// HttpConnectionManager is the only supported network filter at the moment.
349+
if (!any.getTypeUrl().equals(TYPE_URL_HTTP_CONNECTION_MANAGER)) {
350+
throw new ResourceInvalidException(
351+
"FilterChain " + proto.getName() + " contains filter " + filter.getName()
352+
+ " with unsupported typed_config type " + any.getTypeUrl());
353+
}
354+
HttpConnectionManager hcmProto;
355+
try {
356+
hcmProto = any.unpack(HttpConnectionManager.class);
357+
} catch (InvalidProtocolBufferException e) {
358+
throw new ResourceInvalidException("FilterChain " + proto.getName() + " with filter "
359+
+ filter.getName() + " failed to unpack message", e);
360+
}
361+
io.grpc.xds.HttpConnectionManager httpConnectionManager = parseHttpConnectionManager(
362+
hcmProto, rdsResources, filterRegistry, parseHttpFilters, false /* isForClient */);
372363

373364
EnvoyServerProtoData.DownstreamTlsContext downstreamTlsContext = null;
374365
if (proto.hasTransportSocket()) {
@@ -762,17 +753,26 @@ static io.grpc.xds.HttpConnectionManager parseHttpConnectionManager(
762753
// Parse http filters.
763754
List<NamedFilterConfig> filterConfigs = null;
764755
if (parseHttpFilter) {
756+
if (proto.getHttpFiltersList().isEmpty()) {
757+
throw new ResourceInvalidException("Missing HttpFilter in HttpConnectionManager.");
758+
}
765759
filterConfigs = new ArrayList<>();
766760
Set<String> names = new HashSet<>();
767-
for (io.envoyproxy.envoy.extensions.filters.network.http_connection_manager.v3.HttpFilter
768-
httpFilter : proto.getHttpFiltersList()) {
761+
for (int i = 0; i < proto.getHttpFiltersCount(); i++) {
762+
io.envoyproxy.envoy.extensions.filters.network.http_connection_manager.v3.HttpFilter
763+
httpFilter = proto.getHttpFiltersList().get(i);
769764
String filterName = httpFilter.getName();
770765
if (!names.add(filterName)) {
771766
throw new ResourceInvalidException(
772767
"HttpConnectionManager contains duplicate HttpFilter: " + filterName);
773768
}
774769
StructOrError<FilterConfig> filterConfig =
775770
parseHttpFilter(httpFilter, filterRegistry, isForClient);
771+
if ((i == proto.getHttpFiltersCount() - 1)
772+
&& (filterConfig == null || !isTerminalFilter(filterConfig.struct))) {
773+
throw new ResourceInvalidException("The last HttpFilter must be a terminal filter: "
774+
+ filterName);
775+
}
776776
if (filterConfig == null) {
777777
continue;
778778
}
@@ -781,6 +781,10 @@ static io.grpc.xds.HttpConnectionManager parseHttpConnectionManager(
781781
"HttpConnectionManager contains invalid HttpFilter: "
782782
+ filterConfig.getErrorDetail());
783783
}
784+
if ((i < proto.getHttpFiltersCount() - 1) && isTerminalFilter(filterConfig.getStruct())) {
785+
throw new ResourceInvalidException("A terminal HttpFilter must be the last filter: "
786+
+ filterName);
787+
}
784788
filterConfigs.add(new NamedFilterConfig(filterName, filterConfig.struct));
785789
}
786790
}
@@ -821,6 +825,11 @@ static io.grpc.xds.HttpConnectionManager parseHttpConnectionManager(
821825
"HttpConnectionManager neither has inlined route_config nor RDS");
822826
}
823827

828+
// hard-coded: currently router config is the only terminal filter.
829+
private static boolean isTerminalFilter(FilterConfig filterConfig) {
830+
return RouterFilter.ROUTER_CONFIG.equals(filterConfig);
831+
}
832+
824833
@VisibleForTesting
825834
@Nullable // Returns null if the filter is optional but not supported.
826835
static StructOrError<FilterConfig> parseHttpFilter(

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

Lines changed: 0 additions & 121 deletions
This file was deleted.

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

Lines changed: 2 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@
2323
import com.google.common.base.Joiner;
2424
import com.google.common.base.Strings;
2525
import com.google.common.collect.ImmutableMap;
26-
import com.google.common.collect.Iterables;
2726
import com.google.common.collect.Sets;
2827
import com.google.gson.Gson;
2928
import com.google.protobuf.util.Durations;
@@ -95,8 +94,6 @@ final class XdsNameResolver extends NameResolver {
9594
CallOptions.Key.create("io.grpc.xds.CLUSTER_SELECTION_KEY");
9695
static final CallOptions.Key<Long> RPC_HASH_KEY =
9796
CallOptions.Key.create("io.grpc.xds.RPC_HASH_KEY");
98-
private static final NamedFilterConfig LAME_FILTER =
99-
new NamedFilterConfig(null, LameFilter.LAME_CONFIG);
10097
@VisibleForTesting
10198
static boolean enableTimeout =
10299
Strings.isNullOrEmpty(System.getenv("GRPC_XDS_EXPERIMENTAL_ENABLE_TIMEOUT"))
@@ -374,10 +371,6 @@ public Result selectConfig(PickSubchannelArgs args) {
374371
do {
375372
routingCfg = routingConfig;
376373
selectedOverrideConfigs = new HashMap<>(routingCfg.virtualHostOverrideConfig);
377-
if (routingCfg.filterChain != null
378-
&& Iterables.getLast(routingCfg.filterChain).equals(LAME_FILTER)) {
379-
break;
380-
}
381374
for (Route route : routingCfg.routes) {
382375
if (matchRoute(route.routeMatch(), "/" + args.getMethodDescriptor().getFullMethodName(),
383376
headers, random)) {
@@ -442,12 +435,7 @@ public Result selectConfig(PickSubchannelArgs args) {
442435
if (routingCfg.filterChain != null) {
443436
for (NamedFilterConfig namedFilter : routingCfg.filterChain) {
444437
FilterConfig filterConfig = namedFilter.filterConfig;
445-
Filter filter;
446-
if (namedFilter.equals(LAME_FILTER)) {
447-
filter = LameFilter.INSTANCE;
448-
} else {
449-
filter = filterRegistry.get(filterConfig.typeUrl());
450-
}
438+
Filter filter = filterRegistry.get(filterConfig.typeUrl());
451439
if (filter instanceof ClientInterceptorBuilder) {
452440
ClientInterceptor interceptor = ((ClientInterceptorBuilder) filter)
453441
.buildClientInterceptor(
@@ -458,12 +446,6 @@ public Result selectConfig(PickSubchannelArgs args) {
458446
}
459447
}
460448
}
461-
if (Iterables.getLast(routingCfg.filterChain).equals(LAME_FILTER)) {
462-
return Result.newBuilder()
463-
.setConfig(config)
464-
.setInterceptor(combineInterceptors(filterInterceptors))
465-
.build();
466-
}
467449
}
468450
final String finalCluster = cluster;
469451
final long hash = generateHash(selectedRoute.routeAction().hashPolicies(), headers);
@@ -754,27 +736,7 @@ private void updateRoutes(List<VirtualHost> virtualHosts, long httpMaxStreamDura
754736
return;
755737
}
756738

757-
// A router filter is required for request routing. For backward compatibility, routing
758-
// is always enabled for gRPC clients without HttpFilter support.
759739
List<Route> routes = virtualHost.routes();
760-
List<NamedFilterConfig> filterChain = null;
761-
if (filterConfigs != null) {
762-
boolean hasRouter = false;
763-
filterChain = new ArrayList<>(filterConfigs.size());
764-
for (NamedFilterConfig namedFilter : filterConfigs) {
765-
filterChain.add(namedFilter);
766-
if (namedFilter.filterConfig.equals(RouterFilter.ROUTER_CONFIG)) {
767-
hasRouter = true;
768-
break;
769-
}
770-
}
771-
if (!hasRouter) {
772-
// Fail all RPCs if a router filter is not present. Reference counts for all currently
773-
// selectable clusters should be reclaimed.
774-
filterChain.add(LAME_FILTER);
775-
routes = Collections.emptyList();
776-
}
777-
}
778740

779741
// Populate all clusters to which requests can be routed to through the virtual host.
780742
Set<String> clusters = new HashSet<>();
@@ -815,7 +777,7 @@ private void updateRoutes(List<VirtualHost> virtualHosts, long httpMaxStreamDura
815777
// selectable.
816778
routingConfig =
817779
new RoutingConfig(
818-
httpMaxStreamDurationNano, routes, filterChain,
780+
httpMaxStreamDurationNano, routes, filterConfigs,
819781
virtualHost.filterConfigOverrides());
820782
shouldUpdateResult = false;
821783
for (String cluster : deletedClusters) {

0 commit comments

Comments
 (0)