From c87fc052244a25e639746a099eb8c162af1bce75 Mon Sep 17 00:00:00 2001 From: yifeizhuang Date: Fri, 2 Dec 2022 10:35:26 -0800 Subject: [PATCH] xds: remove retained resources logics for RDS and EDS resources (#9724) We use state-of-the-world approach. For LDS/CDS, the control plane must return all resources that the client has subscribed to in each request. If some LDS/CDS resources are gone in a new update, their corresponding RDS/EDS resources names will be onAbsent(), unless there is cached data that is in use by other subscribers in other components. The motivations to remove this "retained resource" logic between resource types are: 1. Already handled by the subscribers, e.g. a CDS state would shut down its childLBs on new updates. XdsResolver for LdsUpdate would cancel all existing RDS subscriptions. Therefore the onAbsent() notification is effectively no-op. 2. Complexity. --- .../main/java/io/grpc/xds/XdsClientImpl.java | 57 +------------- .../java/io/grpc/xds/XdsClusterResource.java | 20 ++--- .../java/io/grpc/xds/XdsEndpointResource.java | 8 +- .../java/io/grpc/xds/XdsListenerResource.java | 39 ++++------ .../java/io/grpc/xds/XdsResourceType.java | 22 +++--- .../grpc/xds/XdsRouteConfigureResource.java | 8 +- .../io/grpc/xds/XdsClientImplDataTest.java | 74 +++++++++---------- .../io/grpc/xds/XdsClientImplTestBase.java | 43 +++++------ 8 files changed, 94 insertions(+), 177 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/XdsClientImpl.java b/xds/src/main/java/io/grpc/xds/XdsClientImpl.java index a30b4755a1..f05210b51c 100644 --- a/xds/src/main/java/io/grpc/xds/XdsClientImpl.java +++ b/xds/src/main/java/io/grpc/xds/XdsClientImpl.java @@ -48,8 +48,6 @@ import io.grpc.xds.LoadStatsManager2.ClusterDropStats; import io.grpc.xds.LoadStatsManager2.ClusterLocalityStats; import io.grpc.xds.XdsClient.ResourceStore; import io.grpc.xds.XdsClient.XdsResponseHandler; -import io.grpc.xds.XdsClusterResource.CdsUpdate; -import io.grpc.xds.XdsListenerResource.LdsUpdate; import io.grpc.xds.XdsLogger.XdsLogLevel; import java.net.URI; import java.util.Collection; @@ -107,7 +105,6 @@ final class XdsClientImpl extends XdsClient implements XdsResponseHandler, Resou private final XdsLogger logger; private volatile boolean isShutdown; - // TODO(zdapeng): rename to XdsClientImpl XdsClientImpl( XdsChannelFactory xdsChannelFactory, Bootstrapper.BootstrapInfo bootstrapInfo, @@ -398,7 +395,6 @@ final class XdsClientImpl extends XdsClient implements XdsResponseHandler, Resou xdsResourceType.typeName(), args.versionInfo, args.nonce, result.unpackedResources); Map> parsedResources = result.parsedResources; Set invalidResources = result.invalidResources; - Set retainedResources = result.retainedResources; List errors = result.errors; String errorDetail = null; if (errors.isEmpty()) { @@ -432,16 +428,14 @@ final class XdsClientImpl extends XdsClient implements XdsResponseHandler, Resou } // Nothing else to do for incremental ADS resources. - if (xdsResourceType.dependentResource() == null) { + if (!xdsResourceType.isFullStateOfTheWorld()) { continue; } // Handle State of the World ADS: invalid resources. if (invalidResources.contains(resourceName)) { // The resource is missing. Reuse the cached resource if possible. - if (subscriber.data != null) { - retainDependentResource(subscriber, retainedResources); - } else { + if (subscriber.data == null) { // No cached data. Notify the watchers of an invalid update. subscriber.onError(Status.UNAVAILABLE.withDescription(errorDetail)); } @@ -451,49 +445,6 @@ final class XdsClientImpl extends XdsClient implements XdsResponseHandler, Resou // For State of the World services, notify watchers when their watched resource is missing // from the ADS update. subscriber.onAbsent(); - // Retain any dependent resources if the resource deletion is ignored - // per bootstrap ignore_resource_deletion server feature. - if (!subscriber.absent) { - retainDependentResource(subscriber, retainedResources); - } - } - - // LDS/CDS responses represents the state of the world, RDS/EDS resources not referenced in - // LDS/CDS resources should be deleted. - if (xdsResourceType.dependentResource() != null) { - XdsResourceType dependency = xdsResourceType.dependentResource(); - Map> dependentSubscribers = - resourceSubscribers.get(dependency); - if (dependentSubscribers == null) { - return; - } - for (String resource : dependentSubscribers.keySet()) { - if (!retainedResources.contains(resource)) { - dependentSubscribers.get(resource).onAbsent(); - } - } - } - } - - private void retainDependentResource( - ResourceSubscriber subscriber, Set retainedResources) { - if (subscriber.data == null) { - return; - } - String resourceName = null; - if (subscriber.type == XdsListenerResource.getInstance()) { - LdsUpdate ldsUpdate = (LdsUpdate) subscriber.data; - io.grpc.xds.HttpConnectionManager hcm = ldsUpdate.httpConnectionManager(); - if (hcm != null) { - resourceName = hcm.rdsName(); - } - } else if (subscriber.type == XdsClusterResource.getInstance()) { - CdsUpdate cdsUpdate = (CdsUpdate) subscriber.data; - resourceName = cdsUpdate.edsServiceName(); - } - - if (resourceName != null) { - retainedResources.add(resourceName); } } @@ -657,9 +608,7 @@ final class XdsClientImpl extends XdsClient implements XdsResponseHandler, Resou // and the resource is reusable. boolean ignoreResourceDeletionEnabled = serverInfo != null && serverInfo.ignoreResourceDeletion(); - boolean isStateOfTheWorld = (type == XdsListenerResource.getInstance() - || type == XdsClusterResource.getInstance()); - if (ignoreResourceDeletionEnabled && isStateOfTheWorld && data != null) { + if (ignoreResourceDeletionEnabled && type.isFullStateOfTheWorld() && data != null) { if (!resourceDeletionIgnored) { logger.log(XdsLogLevel.FORCE_WARNING, "xds server {0}: ignoring deletion for resource type {1} name {2}}", diff --git a/xds/src/main/java/io/grpc/xds/XdsClusterResource.java b/xds/src/main/java/io/grpc/xds/XdsClusterResource.java index 82a977f7df..c0d6ebeefd 100644 --- a/xds/src/main/java/io/grpc/xds/XdsClusterResource.java +++ b/xds/src/main/java/io/grpc/xds/XdsClusterResource.java @@ -88,10 +88,9 @@ class XdsClusterResource extends XdsResourceType { return ADS_TYPE_URL_CDS_V2; } - @Nullable @Override - XdsResourceType dependentResource() { - return XdsEndpointResource.getInstance(); + boolean isFullStateOfTheWorld() { + return true; } @Override @@ -101,8 +100,7 @@ class XdsClusterResource extends XdsResourceType { } @Override - CdsUpdate doParse(Args args, Message unpackedMessage, - Set retainedResources, boolean isResourceV3) + CdsUpdate doParse(Args args, Message unpackedMessage, boolean isResourceV3) throws ResourceInvalidException { if (!(unpackedMessage instanceof Cluster)) { throw new ResourceInvalidException("Invalid message type: " + unpackedMessage.getClass()); @@ -111,12 +109,12 @@ class XdsClusterResource extends XdsResourceType { if (args.bootstrapInfo != null && args.bootstrapInfo.certProviders() != null) { certProviderInstances = args.bootstrapInfo.certProviders().keySet(); } - return processCluster((Cluster) unpackedMessage, retainedResources, certProviderInstances, + return processCluster((Cluster) unpackedMessage, certProviderInstances, args.serverInfo, args.loadBalancerRegistry); } @VisibleForTesting - static CdsUpdate processCluster(Cluster cluster, Set retainedEdsResources, + static CdsUpdate processCluster(Cluster cluster, Set certProviderInstances, Bootstrapper.ServerInfo serverInfo, LoadBalancerRegistry loadBalancerRegistry) @@ -124,7 +122,7 @@ class XdsClusterResource extends XdsResourceType { StructOrError structOrError; switch (cluster.getClusterDiscoveryTypeCase()) { case TYPE: - structOrError = parseNonAggregateCluster(cluster, retainedEdsResources, + structOrError = parseNonAggregateCluster(cluster, certProviderInstances, serverInfo); break; case CLUSTER_TYPE: @@ -178,8 +176,7 @@ class XdsClusterResource extends XdsResourceType { } private static StructOrError parseNonAggregateCluster( - Cluster cluster, Set edsResources, Set certProviderInstances, - Bootstrapper.ServerInfo serverInfo) { + Cluster cluster, Set certProviderInstances, Bootstrapper.ServerInfo serverInfo) { String clusterName = cluster.getName(); Bootstrapper.ServerInfo lrsServerInfo = null; Long maxConcurrentRequests = null; @@ -249,9 +246,6 @@ class XdsClusterResource extends XdsResourceType { // If the service_name field is set, that value will be used for the EDS request. if (!edsClusterConfig.getServiceName().isEmpty()) { edsServiceName = edsClusterConfig.getServiceName(); - edsResources.add(edsServiceName); - } else { - edsResources.add(clusterName); } return StructOrError.fromStruct(CdsUpdate.forEds( clusterName, edsServiceName, lrsServerInfo, maxConcurrentRequests, upstreamTlsContext, diff --git a/xds/src/main/java/io/grpc/xds/XdsEndpointResource.java b/xds/src/main/java/io/grpc/xds/XdsEndpointResource.java index c126d64331..a44fea0c57 100644 --- a/xds/src/main/java/io/grpc/xds/XdsEndpointResource.java +++ b/xds/src/main/java/io/grpc/xds/XdsEndpointResource.java @@ -78,10 +78,9 @@ class XdsEndpointResource extends XdsResourceType { return ADS_TYPE_URL_EDS_V2; } - @Nullable @Override - XdsResourceType dependentResource() { - return null; + boolean isFullStateOfTheWorld() { + return false; } @Override @@ -90,8 +89,7 @@ class XdsEndpointResource extends XdsResourceType { } @Override - EdsUpdate doParse(Args args, Message unpackedMessage, - Set retainedResources, boolean isResourceV3) + EdsUpdate doParse(Args args, Message unpackedMessage, boolean isResourceV3) throws ResourceInvalidException { if (!(unpackedMessage instanceof ClusterLoadAssignment)) { throw new ResourceInvalidException("Invalid message type: " + unpackedMessage.getClass()); diff --git a/xds/src/main/java/io/grpc/xds/XdsListenerResource.java b/xds/src/main/java/io/grpc/xds/XdsListenerResource.java index 5f7d6a27aa..f367bd9690 100644 --- a/xds/src/main/java/io/grpc/xds/XdsListenerResource.java +++ b/xds/src/main/java/io/grpc/xds/XdsListenerResource.java @@ -97,15 +97,13 @@ class XdsListenerResource extends XdsResourceType { return ADS_TYPE_URL_LDS_V2; } - @Nullable @Override - XdsResourceType dependentResource() { - return XdsRouteConfigureResource.getInstance(); + boolean isFullStateOfTheWorld() { + return true; } @Override - LdsUpdate doParse(Args args, Message unpackedMessage, Set retainedResources, - boolean isResourceV3) + LdsUpdate doParse(Args args, Message unpackedMessage, boolean isResourceV3) throws ResourceInvalidException { if (!(unpackedMessage instanceof Listener)) { throw new ResourceInvalidException("Invalid message type: " + unpackedMessage.getClass()); @@ -114,15 +112,14 @@ class XdsListenerResource extends XdsResourceType { if (listener.hasApiListener()) { return processClientSideListener( - listener, retainedResources, args, enableFaultInjection && isResourceV3); + listener, args, enableFaultInjection && isResourceV3); } else { return processServerSideListener( - listener, retainedResources, args, enableRbac && isResourceV3); + listener, args, enableRbac && isResourceV3); } } - private LdsUpdate processClientSideListener( - Listener listener, Set rdsResources, Args args, boolean parseHttpFilter) + private LdsUpdate processClientSideListener(Listener listener, Args args, boolean parseHttpFilter) throws ResourceInvalidException { // Unpack HttpConnectionManager from the Listener. HttpConnectionManager hcm; @@ -135,24 +132,22 @@ class XdsListenerResource extends XdsResourceType { "Could not parse HttpConnectionManager config from ApiListener", e); } return LdsUpdate.forApiListener(parseHttpConnectionManager( - hcm, rdsResources, args.filterRegistry, parseHttpFilter, true /* isForClient */)); + hcm, args.filterRegistry, parseHttpFilter, true /* isForClient */)); } - private LdsUpdate processServerSideListener( - Listener proto, Set rdsResources, Args args, boolean parseHttpFilter) + private LdsUpdate processServerSideListener(Listener proto, Args args, boolean parseHttpFilter) throws ResourceInvalidException { Set certProviderInstances = null; if (args.bootstrapInfo != null && args.bootstrapInfo.certProviders() != null) { certProviderInstances = args.bootstrapInfo.certProviders().keySet(); } - return LdsUpdate.forTcpListener(parseServerSideListener( - proto, rdsResources, args.tlsContextManager, args.filterRegistry, certProviderInstances, - parseHttpFilter)); + return LdsUpdate.forTcpListener(parseServerSideListener(proto, args.tlsContextManager, + args.filterRegistry, certProviderInstances, parseHttpFilter)); } @VisibleForTesting static EnvoyServerProtoData.Listener parseServerSideListener( - Listener proto, Set rdsResources, TlsContextManager tlsContextManager, + Listener proto, TlsContextManager tlsContextManager, FilterRegistry filterRegistry, Set certProviderInstances, boolean parseHttpFilter) throws ResourceInvalidException { if (!proto.getTrafficDirection().equals(TrafficDirection.INBOUND) @@ -190,13 +185,13 @@ class XdsListenerResource extends XdsResourceType { Set uniqueSet = new HashSet<>(); for (io.envoyproxy.envoy.config.listener.v3.FilterChain fc : proto.getFilterChainsList()) { filterChains.add( - parseFilterChain(fc, rdsResources, tlsContextManager, filterRegistry, uniqueSet, + parseFilterChain(fc, tlsContextManager, filterRegistry, uniqueSet, certProviderInstances, parseHttpFilter)); } FilterChain defaultFilterChain = null; if (proto.hasDefaultFilterChain()) { defaultFilterChain = parseFilterChain( - proto.getDefaultFilterChain(), rdsResources, tlsContextManager, filterRegistry, + proto.getDefaultFilterChain(), tlsContextManager, filterRegistry, null, certProviderInstances, parseHttpFilter); } @@ -206,7 +201,7 @@ class XdsListenerResource extends XdsResourceType { @VisibleForTesting static FilterChain parseFilterChain( - io.envoyproxy.envoy.config.listener.v3.FilterChain proto, Set rdsResources, + io.envoyproxy.envoy.config.listener.v3.FilterChain proto, TlsContextManager tlsContextManager, FilterRegistry filterRegistry, Set uniqueSet, Set certProviderInstances, boolean parseHttpFilters) throws ResourceInvalidException { @@ -235,7 +230,7 @@ class XdsListenerResource extends XdsResourceType { + filter.getName() + " failed to unpack message", e); } io.grpc.xds.HttpConnectionManager httpConnectionManager = parseHttpConnectionManager( - hcmProto, rdsResources, filterRegistry, parseHttpFilters, false /* isForClient */); + hcmProto, filterRegistry, parseHttpFilters, false /* isForClient */); EnvoyServerProtoData.DownstreamTlsContext downstreamTlsContext = null; if (proto.hasTransportSocket()) { @@ -466,7 +461,7 @@ class XdsListenerResource extends XdsResourceType { @VisibleForTesting static io.grpc.xds.HttpConnectionManager parseHttpConnectionManager( - HttpConnectionManager proto, Set rdsResources, FilterRegistry filterRegistry, + HttpConnectionManager proto, FilterRegistry filterRegistry, boolean parseHttpFilter, boolean isForClient) throws ResourceInvalidException { if (enableRbac && proto.getXffNumTrustedHops() != 0) { throw new ResourceInvalidException( @@ -541,8 +536,6 @@ class XdsListenerResource extends XdsResourceType { throw new ResourceInvalidException( "HttpConnectionManager contains invalid RDS: must specify ADS or self ConfigSource"); } - // Collect the RDS resource referenced by this HttpConnectionManager. - rdsResources.add(rds.getRouteConfigName()); return io.grpc.xds.HttpConnectionManager.forRdsName( maxStreamDuration, rds.getRouteConfigName(), filterConfigs); } diff --git a/xds/src/main/java/io/grpc/xds/XdsResourceType.java b/xds/src/main/java/io/grpc/xds/XdsResourceType.java index a377ee35d7..987151d899 100644 --- a/xds/src/main/java/io/grpc/xds/XdsResourceType.java +++ b/xds/src/main/java/io/grpc/xds/XdsResourceType.java @@ -85,9 +85,12 @@ abstract class XdsResourceType { abstract String typeUrlV2(); - // Non-null for State of the World resources. - @Nullable - abstract XdsResourceType dependentResource(); + // Do not confuse with the SotW approach: it is the mechanism in which the client must specify all + // resource names it is interested in with each request. Different resource types may behave + // differently in this approach. For LDS and CDS resources, the server must return all resources + // that the client has subscribed to in each request. For RDS and EDS, the server may only return + // the resources that need an update. + abstract boolean isFullStateOfTheWorld(); static class Args { final ServerInfo serverInfo; @@ -125,7 +128,6 @@ abstract class XdsResourceType { Set unpackedResources = new HashSet<>(resources.size()); Set invalidResources = new HashSet<>(); List errors = new ArrayList<>(); - Set retainedResources = new HashSet<>(); for (int i = 0; i < resources.size(); i++) { Any resource = resources.get(i); @@ -156,7 +158,7 @@ abstract class XdsResourceType { T resourceUpdate; try { - resourceUpdate = doParse(args, unpackedMessage, retainedResources, isResourceV3); + resourceUpdate = doParse(args, unpackedMessage, isResourceV3); } catch (XdsClientImpl.ResourceInvalidException e) { errors.add(String.format("%s response %s '%s' validation error: %s", typeName(), unpackedClassName().getSimpleName(), cname, e.getMessage())); @@ -168,12 +170,11 @@ abstract class XdsResourceType { parsedResources.put(cname, new ParsedResource(resourceUpdate, resource)); } return new ValidatedResourceUpdate(parsedResources, unpackedResources, invalidResources, - errors, retainedResources); + errors); } - abstract T doParse(Args args, Message unpackedMessage, Set retainedResources, - boolean isResourceV3) + abstract T doParse(Args args, Message unpackedMessage, boolean isResourceV3) throws ResourceInvalidException; /** @@ -231,19 +232,16 @@ abstract class XdsResourceType { Set unpackedResources; Set invalidResources; List errors; - Set retainedResources; // validated resource update public ValidatedResourceUpdate(Map> parsedResources, Set unpackedResources, Set invalidResources, - List errors, - Set retainedResources) { + List errors) { this.parsedResources = parsedResources; this.unpackedResources = unpackedResources; this.invalidResources = invalidResources; this.errors = errors; - this.retainedResources = retainedResources; } } diff --git a/xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java b/xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java index 166809c87e..d5a0eecc0e 100644 --- a/xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java +++ b/xds/src/main/java/io/grpc/xds/XdsRouteConfigureResource.java @@ -107,10 +107,9 @@ class XdsRouteConfigureResource extends XdsResourceType { return ADS_TYPE_URL_RDS_V2; } - @Nullable @Override - XdsResourceType dependentResource() { - return null; + boolean isFullStateOfTheWorld() { + return false; } @Override @@ -119,8 +118,7 @@ class XdsRouteConfigureResource extends XdsResourceType { } @Override - RdsUpdate doParse(XdsResourceType.Args args, Message unpackedMessage, - Set retainedResources, boolean isResourceV3) + RdsUpdate doParse(XdsResourceType.Args args, Message unpackedMessage, boolean isResourceV3) throws ResourceInvalidException { if (!(unpackedMessage instanceof RouteConfiguration)) { throw new ResourceInvalidException("Invalid message type: " + unpackedMessage.getClass()); diff --git a/xds/src/test/java/io/grpc/xds/XdsClientImplDataTest.java b/xds/src/test/java/io/grpc/xds/XdsClientImplDataTest.java index 993fb910f3..8c6ccc2780 100644 --- a/xds/src/test/java/io/grpc/xds/XdsClientImplDataTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsClientImplDataTest.java @@ -135,10 +135,8 @@ import io.grpc.xds.internal.Matchers.FractionMatcher; import io.grpc.xds.internal.Matchers.HeaderMatcher; import java.util.Arrays; import java.util.Collections; -import java.util.HashSet; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; @@ -1301,7 +1299,7 @@ public class XdsClientImplDataTest { thrown.expect(ResourceInvalidException.class); thrown.expectMessage("HttpConnectionManager with xff_num_trusted_hops unsupported"); XdsListenerResource.parseHttpConnectionManager( - hcm, new HashSet(), filterRegistry, false /* does not matter */, + hcm, filterRegistry, false /* does not matter */, true /* does not matter */); } @@ -1315,7 +1313,7 @@ public class XdsClientImplDataTest { thrown.expect(ResourceInvalidException.class); thrown.expectMessage("HttpConnectionManager with original_ip_detection_extensions unsupported"); XdsListenerResource.parseHttpConnectionManager( - hcm, new HashSet(), filterRegistry, false /* does not matter */, false); + hcm, filterRegistry, false /* does not matter */, false); } @Test @@ -1330,7 +1328,7 @@ public class XdsClientImplDataTest { thrown.expect(ResourceInvalidException.class); thrown.expectMessage("HttpConnectionManager neither has inlined route_config nor RDS"); XdsListenerResource.parseHttpConnectionManager( - hcm, new HashSet(), filterRegistry, false /* does not matter */, + hcm, filterRegistry, false /* does not matter */, true /* does not matter */); } @@ -1349,7 +1347,7 @@ public class XdsClientImplDataTest { thrown.expect(ResourceInvalidException.class); thrown.expectMessage("HttpConnectionManager contains duplicate HttpFilter: envoy.filter.foo"); XdsListenerResource.parseHttpConnectionManager( - hcm, new HashSet(), filterRegistry, true /* parseHttpFilter */, + hcm, filterRegistry, true /* parseHttpFilter */, true /* does not matter */); } @@ -1367,7 +1365,7 @@ public class XdsClientImplDataTest { thrown.expect(ResourceInvalidException.class); thrown.expectMessage("The last HttpFilter must be a terminal filter: envoy.filter.bar"); XdsListenerResource.parseHttpConnectionManager( - hcm, new HashSet(), filterRegistry, true /* parseHttpFilter */, + hcm, filterRegistry, true /* parseHttpFilter */, true /* does not matter */); } @@ -1385,7 +1383,7 @@ public class XdsClientImplDataTest { thrown.expect(ResourceInvalidException.class); thrown.expectMessage("A terminal HttpFilter must be the last filter: terminal"); XdsListenerResource.parseHttpConnectionManager( - hcm, new HashSet(), filterRegistry, true /* parseHttpFilter */, + hcm, filterRegistry, true /* parseHttpFilter */, true); } @@ -1401,7 +1399,7 @@ public class XdsClientImplDataTest { thrown.expect(ResourceInvalidException.class); thrown.expectMessage("The last HttpFilter must be a terminal filter: envoy.filter.bar"); XdsListenerResource.parseHttpConnectionManager( - hcm, new HashSet(), filterRegistry, true /* parseHttpFilter */, + hcm, filterRegistry, true /* parseHttpFilter */, true /* does not matter */); } @@ -1413,7 +1411,7 @@ public class XdsClientImplDataTest { thrown.expect(ResourceInvalidException.class); thrown.expectMessage("Missing HttpFilter in HttpConnectionManager."); XdsListenerResource.parseHttpConnectionManager( - hcm, new HashSet(), filterRegistry, true /* parseHttpFilter */, + hcm, filterRegistry, true /* parseHttpFilter */, true /* does not matter */); } @@ -1459,7 +1457,7 @@ public class XdsClientImplDataTest { .build(); io.grpc.xds.HttpConnectionManager parsedHcm = XdsListenerResource.parseHttpConnectionManager( - hcm, new HashSet(), filterRegistry, false /* parseHttpFilter */, + hcm, filterRegistry, false /* parseHttpFilter */, true /* does not matter */); VirtualHost virtualHost = Iterables.getOnlyElement(parsedHcm.virtualHosts()); @@ -1536,7 +1534,7 @@ public class XdsClientImplDataTest { thrown.expectMessage("Multiple ClusterSpecifierPlugins with the same name: rls-plugin-1"); XdsListenerResource.parseHttpConnectionManager( - hcm, new HashSet(), filterRegistry, false /* parseHttpFilter */, + hcm, filterRegistry, false /* parseHttpFilter */, true /* does not matter */); } @@ -1585,7 +1583,7 @@ public class XdsClientImplDataTest { thrown.expectMessage("ClusterSpecifierPlugin for [invalid-plugin-name] not found"); XdsListenerResource.parseHttpConnectionManager( - hcm, new HashSet(), filterRegistry, false /* parseHttpFilter */, + hcm, filterRegistry, false /* parseHttpFilter */, true /* does not matter */); } @@ -1655,8 +1653,8 @@ public class XdsClientImplDataTest { .addRoutes(optionalRoute)) .build(); io.grpc.xds.HttpConnectionManager parsedHcm = XdsListenerResource.parseHttpConnectionManager( - HttpConnectionManager.newBuilder().setRouteConfig(routeConfig).build(), - new HashSet<>(), filterRegistry, false /* parseHttpFilter */, true /* does not matter */); + HttpConnectionManager.newBuilder().setRouteConfig(routeConfig).build(), filterRegistry, + false /* parseHttpFilter */, true /* does not matter */); // Verify that the only route left is the one with the registered RLS plugin `rls-plugin-1`, // while the route with unregistered optional `optional-plugin-`1 has been skipped. @@ -1671,7 +1669,6 @@ public class XdsClientImplDataTest { @Test public void parseHttpConnectionManager_validateRdsConfigSource() throws Exception { XdsResourceType.enableRouteLookup = true; - Set rdsResources = new HashSet<>(); HttpConnectionManager hcm1 = HttpConnectionManager.newBuilder() @@ -1681,7 +1678,7 @@ public class XdsClientImplDataTest { ConfigSource.newBuilder().setAds(AggregatedConfigSource.getDefaultInstance()))) .build(); XdsListenerResource.parseHttpConnectionManager( - hcm1, rdsResources, filterRegistry, false /* parseHttpFilter */, + hcm1, filterRegistry, false /* parseHttpFilter */, true /* does not matter */); HttpConnectionManager hcm2 = @@ -1692,7 +1689,7 @@ public class XdsClientImplDataTest { ConfigSource.newBuilder().setSelf(SelfConfigSource.getDefaultInstance()))) .build(); XdsListenerResource.parseHttpConnectionManager( - hcm2, rdsResources, filterRegistry, false /* parseHttpFilter */, + hcm2, filterRegistry, false /* parseHttpFilter */, true /* does not matter */); HttpConnectionManager hcm3 = @@ -1707,7 +1704,7 @@ public class XdsClientImplDataTest { thrown.expectMessage( "HttpConnectionManager contains invalid RDS: must specify ADS or self ConfigSource"); XdsListenerResource.parseHttpConnectionManager( - hcm3, rdsResources, filterRegistry, false /* parseHttpFilter */, + hcm3, filterRegistry, false /* parseHttpFilter */, true /* does not matter */); } @@ -1892,7 +1889,7 @@ public class XdsClientImplDataTest { .build(); CdsUpdate update = XdsClusterResource.processCluster( - cluster, new HashSet(), null, LRS_SERVER_INFO, + cluster, null, LRS_SERVER_INFO, LoadBalancerRegistry.getDefaultRegistry()); LbConfig lbConfig = ServiceConfigUtil.unwrapLoadBalancingConfig(update.lbPolicyConfig()); assertThat(lbConfig.getPolicyName()).isEqualTo("ring_hash_experimental"); @@ -1914,7 +1911,7 @@ public class XdsClientImplDataTest { .build(); CdsUpdate update = XdsClusterResource.processCluster( - cluster, new HashSet(), null, LRS_SERVER_INFO, + cluster, null, LRS_SERVER_INFO, LoadBalancerRegistry.getDefaultRegistry()); LbConfig lbConfig = ServiceConfigUtil.unwrapLoadBalancingConfig(update.lbPolicyConfig()); assertThat(lbConfig.getPolicyName()).isEqualTo("wrr_locality_experimental"); @@ -1942,13 +1939,12 @@ public class XdsClientImplDataTest { thrown.expect(ResourceInvalidException.class); thrown.expectMessage( "Cluster cluster-foo.googleapis.com: transport-socket-matches not supported."); - XdsClusterResource.processCluster(cluster, new HashSet(), null, LRS_SERVER_INFO, + XdsClusterResource.processCluster(cluster, null, LRS_SERVER_INFO, LoadBalancerRegistry.getDefaultRegistry()); } @Test public void parseCluster_validateEdsSourceConfig() throws ResourceInvalidException { - Set retainedEdsResources = new HashSet<>(); Cluster cluster1 = Cluster.newBuilder() .setName("cluster-foo.googleapis.com") .setType(DiscoveryType.EDS) @@ -1960,7 +1956,7 @@ public class XdsClientImplDataTest { .setServiceName("service-foo.googleapis.com")) .setLbPolicy(LbPolicy.ROUND_ROBIN) .build(); - XdsClusterResource.processCluster(cluster1, retainedEdsResources, null, LRS_SERVER_INFO, + XdsClusterResource.processCluster(cluster1, null, LRS_SERVER_INFO, LoadBalancerRegistry.getDefaultRegistry()); Cluster cluster2 = Cluster.newBuilder() @@ -1974,7 +1970,7 @@ public class XdsClientImplDataTest { .setServiceName("service-foo.googleapis.com")) .setLbPolicy(LbPolicy.ROUND_ROBIN) .build(); - XdsClusterResource.processCluster(cluster2, retainedEdsResources, null, LRS_SERVER_INFO, + XdsClusterResource.processCluster(cluster2, null, LRS_SERVER_INFO, LoadBalancerRegistry.getDefaultRegistry()); Cluster cluster3 = Cluster.newBuilder() @@ -1993,7 +1989,7 @@ public class XdsClientImplDataTest { thrown.expectMessage( "Cluster cluster-foo.googleapis.com: field eds_cluster_config must be set to indicate to" + " use EDS over ADS or self ConfigSource"); - XdsClusterResource.processCluster(cluster3, retainedEdsResources, null, LRS_SERVER_INFO, + XdsClusterResource.processCluster(cluster3, null, LRS_SERVER_INFO, LoadBalancerRegistry.getDefaultRegistry()); } @@ -2007,7 +2003,7 @@ public class XdsClientImplDataTest { thrown.expect(ResourceInvalidException.class); thrown.expectMessage("Listener listener1 with invalid traffic direction: OUTBOUND"); XdsListenerResource.parseServerSideListener( - listener, new HashSet(), null, filterRegistry, null, true /* does not matter */); + listener, null, filterRegistry, null, true /* does not matter */); } @Test @@ -2017,7 +2013,7 @@ public class XdsClientImplDataTest { .setName("listener1") .build(); XdsListenerResource.parseServerSideListener( - listener, new HashSet(), null, filterRegistry, null, true /* does not matter */); + listener, null, filterRegistry, null, true /* does not matter */); } @Test @@ -2031,7 +2027,7 @@ public class XdsClientImplDataTest { thrown.expect(ResourceInvalidException.class); thrown.expectMessage("Listener listener1 cannot have listener_filters"); XdsListenerResource.parseServerSideListener( - listener, new HashSet(), null, filterRegistry, null, true /* does not matter */); + listener, null, filterRegistry, null, true /* does not matter */); } @Test @@ -2045,7 +2041,7 @@ public class XdsClientImplDataTest { thrown.expect(ResourceInvalidException.class); thrown.expectMessage("Listener listener1 cannot have use_original_dst set to true"); XdsListenerResource.parseServerSideListener( - listener, new HashSet(), null, filterRegistry, null, true /* does not matter */); + listener,null, filterRegistry, null, true /* does not matter */); } @Test @@ -2094,7 +2090,7 @@ public class XdsClientImplDataTest { thrown.expect(ResourceInvalidException.class); thrown.expectMessage("FilterChainMatch must be unique. Found duplicate:"); XdsListenerResource.parseServerSideListener( - listener, new HashSet(), null, filterRegistry, null, true /* does not matter */); + listener, null, filterRegistry, null, true /* does not matter */); } @Test @@ -2143,7 +2139,7 @@ public class XdsClientImplDataTest { thrown.expect(ResourceInvalidException.class); thrown.expectMessage("FilterChainMatch must be unique. Found duplicate:"); XdsListenerResource.parseServerSideListener( - listener, new HashSet(), null, filterRegistry, null, true /* does not matter */); + listener,null, filterRegistry, null, true /* does not matter */); } @Test @@ -2192,7 +2188,7 @@ public class XdsClientImplDataTest { .addAllFilterChains(Arrays.asList(filterChain1, filterChain2)) .build(); XdsListenerResource.parseServerSideListener( - listener, new HashSet(), null, filterRegistry, null, true /* does not matter */); + listener, null, filterRegistry, null, true /* does not matter */); } @Test @@ -2207,7 +2203,7 @@ public class XdsClientImplDataTest { thrown.expectMessage( "FilterChain filter-chain-foo should contain exact one HttpConnectionManager filter"); XdsListenerResource.parseFilterChain( - filterChain, new HashSet(), null, filterRegistry, null, null, + filterChain, null, filterRegistry, null, null, true /* does not matter */); } @@ -2226,7 +2222,7 @@ public class XdsClientImplDataTest { thrown.expectMessage( "FilterChain filter-chain-foo should contain exact one HttpConnectionManager filter"); XdsListenerResource.parseFilterChain( - filterChain, new HashSet(), null, filterRegistry, null, null, + filterChain, null, filterRegistry, null, null, true /* does not matter */); } @@ -2245,7 +2241,7 @@ public class XdsClientImplDataTest { "FilterChain filter-chain-foo contains filter envoy.http_connection_manager " + "without typed_config"); XdsListenerResource.parseFilterChain( - filterChain, new HashSet(), null, filterRegistry, null, null, + filterChain, null, filterRegistry, null, null, true /* does not matter */); } @@ -2268,7 +2264,7 @@ public class XdsClientImplDataTest { "FilterChain filter-chain-foo contains filter unsupported with unsupported " + "typed_config type unsupported-type-url"); XdsListenerResource.parseFilterChain( - filterChain, new HashSet(), null, filterRegistry, null, null, + filterChain, null, filterRegistry, null, null, true /* does not matter */); } @@ -2296,10 +2292,10 @@ public class XdsClientImplDataTest { .build(); EnvoyServerProtoData.FilterChain parsedFilterChain1 = XdsListenerResource.parseFilterChain( - filterChain1, new HashSet(), null, filterRegistry, null, + filterChain1, null, filterRegistry, null, null, true /* does not matter */); EnvoyServerProtoData.FilterChain parsedFilterChain2 = XdsListenerResource.parseFilterChain( - filterChain2, new HashSet(), null, filterRegistry, null, + filterChain2, null, filterRegistry, null, null, true /* does not matter */); assertThat(parsedFilterChain1.name()).isEqualTo(parsedFilterChain2.name()); } diff --git a/xds/src/test/java/io/grpc/xds/XdsClientImplTestBase.java b/xds/src/test/java/io/grpc/xds/XdsClientImplTestBase.java index 31d10abd84..ca996af074 100644 --- a/xds/src/test/java/io/grpc/xds/XdsClientImplTestBase.java +++ b/xds/src/test/java/io/grpc/xds/XdsClientImplTestBase.java @@ -806,19 +806,14 @@ public abstract class XdsClientImplTestBase { verifyResourceMetadataAcked(LDS, "C", resourcesV1.get("C"), VERSION_1, TIME_INCREMENT); } call.verifyRequestNack(LDS, subscribedResourceNames, VERSION_1, "0001", NODE, errorsV2); - // {A.1} -> does not exist, missing from {A} + // {A.1} -> version 1 // {B.1} -> version 1 // {C.1} -> does not exist because {C} does not exist - verifyResourceMetadataDoesNotExist(RDS, "A.1"); + verifyResourceMetadataAcked(RDS, "A.1", resourcesV11.get("A.1"), VERSION_1, TIME_INCREMENT * 2); verifyResourceMetadataAcked(RDS, "B.1", resourcesV11.get("B.1"), VERSION_1, TIME_INCREMENT * 2); - if (!ignoreResourceDeletion()) { - verifyResourceMetadataDoesNotExist(RDS, "C.1"); - } else { - // When resource deletion is disabled, {C.1} is not deleted when {C} is deleted. - // Verify {C.1} stays in the previous version VERSION_1. - verifyResourceMetadataAcked(RDS, "C.1", resourcesV11.get("C.1"), VERSION_1, - TIME_INCREMENT * 2); - } + // Verify {C.1} stays in the previous version VERSION_1, no matter {C} is deleted or not. + verifyResourceMetadataAcked(RDS, "C.1", resourcesV11.get("C.1"), VERSION_1, + TIME_INCREMENT * 2); } @Test @@ -1556,8 +1551,8 @@ public abstract class XdsClientImplTestBase { call.sendResponse(LDS, testListenerVhosts, VERSION_2, "0001"); verify(ldsResourceWatcher, times(2)).onChanged(ldsUpdateCaptor.capture()); verifyGoldenListenerVhosts(ldsUpdateCaptor.getValue()); - verify(rdsResourceWatcher).onResourceDoesNotExist(RDS_RESOURCE); - verifyResourceMetadataDoesNotExist(RDS, RDS_RESOURCE); + verifyNoMoreInteractions(rdsResourceWatcher); + verifyResourceMetadataAcked(RDS, RDS_RESOURCE, testRouteConfig, VERSION_1, TIME_INCREMENT * 2); verifyResourceMetadataAcked( LDS, LDS_RESOURCE, testListenerVhosts, VERSION_2, TIME_INCREMENT * 3); verifySubscribedResourcesMetadataSizes(1, 0, 1, 0); @@ -1624,8 +1619,8 @@ public abstract class XdsClientImplTestBase { parsedFilterChain = Iterables.getOnlyElement( ldsUpdateCaptor.getValue().listener().filterChains()); assertThat(parsedFilterChain.httpConnectionManager().virtualHosts()).hasSize(VHOST_SIZE); - verify(rdsResourceWatcher).onResourceDoesNotExist(RDS_RESOURCE); - verifyResourceMetadataDoesNotExist(RDS, RDS_RESOURCE); + verify(rdsResourceWatcher, never()).onResourceDoesNotExist(RDS_RESOURCE); + verifyResourceMetadataAcked(RDS, RDS_RESOURCE, testRouteConfig, VERSION_1, TIME_INCREMENT * 2); verifyResourceMetadataAcked( LDS, LISTENER_RESOURCE, packedListener, VERSION_2, TIME_INCREMENT * 3); verifySubscribedResourcesMetadataSizes(1, 0, 1, 0); @@ -1896,19 +1891,14 @@ public abstract class XdsClientImplTestBase { verifyResourceMetadataAcked(CDS, "C", resourcesV1.get("C"), VERSION_1, TIME_INCREMENT); } call.verifyRequestNack(CDS, subscribedResourceNames, VERSION_1, "0001", NODE, errorsV2); - // {A.1} -> does not exist, missing from {A} + // {A.1} -> version 1 // {B.1} -> version 1 // {C.1} -> does not exist because {C} does not exist - verifyResourceMetadataDoesNotExist(EDS, "A.1"); + verifyResourceMetadataAcked(EDS, "A.1", resourcesV11.get("A.1"), VERSION_1, TIME_INCREMENT * 2); verifyResourceMetadataAcked(EDS, "B.1", resourcesV11.get("B.1"), VERSION_1, TIME_INCREMENT * 2); - if (!ignoreResourceDeletion()) { - verifyResourceMetadataDoesNotExist(EDS, "C.1"); - } else { - // When resource deletion is disabled, {C.1} is not deleted when {C} is deleted. - // Verify {C.1} stays in the previous version VERSION_1. - verifyResourceMetadataAcked(EDS, "C.1", resourcesV11.get("C.1"), VERSION_1, - TIME_INCREMENT * 2); - } + // Verify {C.1} stays in the previous version VERSION_1. {C1} deleted or not does not matter. + verifyResourceMetadataAcked(EDS, "C.1", resourcesV11.get("C.1"), VERSION_1, + TIME_INCREMENT * 2); } @Test @@ -3026,10 +3016,11 @@ public abstract class XdsClientImplTestBase { assertThat(cdsUpdateCaptor.getValue().edsServiceName()).isNull(); // Note that the endpoint must be deleted even if the ignore_resource_deletion feature. // This happens because the cluster CDS_RESOURCE is getting replaced, and not deleted. - verify(edsResourceWatcher).onResourceDoesNotExist(EDS_RESOURCE); + verify(edsResourceWatcher, never()).onResourceDoesNotExist(EDS_RESOURCE); verify(edsResourceWatcher, never()).onResourceDoesNotExist(resource); verifyNoMoreInteractions(cdsWatcher, edsWatcher); - verifyResourceMetadataDoesNotExist(EDS, EDS_RESOURCE); + verifyResourceMetadataAcked( + EDS, EDS_RESOURCE, clusterLoadAssignments.get(0), VERSION_1, TIME_INCREMENT * 2); verifyResourceMetadataAcked( EDS, resource, clusterLoadAssignments.get(1), VERSION_1, TIME_INCREMENT * 2); // no change verifyResourceMetadataAcked(CDS, resource, clusters.get(0), VERSION_2, TIME_INCREMENT * 3);