From 2de01669f2af6b761f77eebfcba7d3af743a6980 Mon Sep 17 00:00:00 2001 From: yifeizhuang Date: Thu, 4 Nov 2021 14:10:03 -0700 Subject: [PATCH 1/2] xds: remove filter chain uuid name generator (#8663) Generating a uuid in filterChain breaks the de-duplication detection which causes XdsServer to cycle connections, so removing it. An empty name is now allowed. The name is currently only used for debug purpose. --- xds/src/main/java/io/grpc/xds/ClientXdsClient.java | 7 +------ xds/src/main/java/io/grpc/xds/EnvoyServerProtoData.java | 2 +- xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java | 4 ++-- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/ClientXdsClient.java b/xds/src/main/java/io/grpc/xds/ClientXdsClient.java index 8a43717f97e..216fcfc1703 100644 --- a/xds/src/main/java/io/grpc/xds/ClientXdsClient.java +++ b/xds/src/main/java/io/grpc/xds/ClientXdsClient.java @@ -104,7 +104,6 @@ import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.UUID; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import javax.annotation.Nullable; @@ -376,14 +375,10 @@ static FilterChain parseFilterChain( validateDownstreamTlsContext(downstreamTlsContextProto, certProviderInstances)); } - String name = proto.getName(); - if (name.isEmpty()) { - name = UUID.randomUUID().toString(); - } FilterChainMatch filterChainMatch = parseFilterChainMatch(proto.getFilterChainMatch()); checkForUniqueness(uniqueSet, filterChainMatch); return new FilterChain( - name, + proto.getName(), filterChainMatch, httpConnectionManager, downstreamTlsContext, diff --git a/xds/src/main/java/io/grpc/xds/EnvoyServerProtoData.java b/xds/src/main/java/io/grpc/xds/EnvoyServerProtoData.java index aa53d834d3b..09318a8c150 100644 --- a/xds/src/main/java/io/grpc/xds/EnvoyServerProtoData.java +++ b/xds/src/main/java/io/grpc/xds/EnvoyServerProtoData.java @@ -314,7 +314,7 @@ public String toString() { * Corresponds to Envoy proto message {@link io.envoyproxy.envoy.api.v2.listener.FilterChain}. */ static final class FilterChain { - // Unique name for the FilterChain. + // possibly empty private final String name; // TODO(sanjaypujare): flatten structure by moving FilterChainMatch class members here. private final FilterChainMatch filterChainMatch; diff --git a/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java b/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java index 0207c2b94e5..2dbab1bcb9d 100644 --- a/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java +++ b/xds/src/test/java/io/grpc/xds/ClientXdsClientDataTest.java @@ -1587,7 +1587,7 @@ public void parseFilterChain_unsupportedFilter() throws ResourceInvalidException } @Test - public void parseFilterChain_noName_generatedUuid() throws ResourceInvalidException { + public void parseFilterChain_noName() throws ResourceInvalidException { FilterChain filterChain1 = FilterChain.newBuilder() .setFilterChainMatch(FilterChainMatch.getDefaultInstance()) @@ -1615,7 +1615,7 @@ public void parseFilterChain_noName_generatedUuid() throws ResourceInvalidExcept EnvoyServerProtoData.FilterChain parsedFilterChain2 = ClientXdsClient.parseFilterChain( filterChain2, new HashSet(), null, filterRegistry, null, null, true /* does not matter */); - assertThat(parsedFilterChain1.getName()).isNotEqualTo(parsedFilterChain2.getName()); + assertThat(parsedFilterChain1.getName()).isEqualTo(parsedFilterChain2.getName()); } @Test From a98e0db93d2a0805b412b62fe9a665b0b951964c Mon Sep 17 00:00:00 2001 From: yifeizhuang Date: Mon, 8 Nov 2021 15:21:59 -0800 Subject: [PATCH 2/2] xds: fix xdsClient resource not exist for invalid resource, fix xdsServerWrapper start on resource not exist (#8660) Fix bugs: 1. Invalid resource at xdsClient, the watcher should have been delivered an error instead of resource not found. 2. If the resource is properly determined to not exist, it shouldn't cause start() to fail. From A36 xDS for Servers: "XdsServer's start must not fail due to transient xDS issues, like missing xDS configuration from the xDS server." --- .../java/io/grpc/xds/ClientXdsClient.java | 10 ++-- .../java/io/grpc/xds/XdsServerWrapper.java | 4 -- .../io/grpc/xds/ClientXdsClientTestBase.java | 50 +++++++++++++------ .../XdsClientWrapperForServerSdsTestMisc.java | 16 +++--- .../io/grpc/xds/XdsServerWrapperTest.java | 22 +++++--- 5 files changed, 64 insertions(+), 38 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/ClientXdsClient.java b/xds/src/main/java/io/grpc/xds/ClientXdsClient.java index 216fcfc1703..3d46d2b56a9 100644 --- a/xds/src/main/java/io/grpc/xds/ClientXdsClient.java +++ b/xds/src/main/java/io/grpc/xds/ClientXdsClient.java @@ -2077,11 +2077,13 @@ private void handleResourceUpdate( } retainedResources.add(edsName); } - continue; + } else if (invalidResources.contains(resourceName)) { + subscriber.onError(Status.UNAVAILABLE.withDescription(errorDetail)); + } else { + // For State of the World services, notify watchers when their watched resource is missing + // from the ADS update. + subscriber.onAbsent(); } - // For State of the World services, notify watchers when their watched resource is missing - // from the ADS update. - subscriber.onAbsent(); } } // LDS/CDS responses represents the state of the world, RDS/EDS resources not referenced in diff --git a/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java b/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java index ed51ccc9edf..dab3cd798f7 100644 --- a/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java +++ b/xds/src/main/java/io/grpc/xds/XdsServerWrapper.java @@ -571,10 +571,6 @@ private void handleConfigNotFound(StatusException exception) { for (SslContextProviderSupplier s: toRelease) { s.close(); } - if (!initialStarted) { - initialStarted = true; - initialStartFuture.set(exception); - } if (restartTimer != null) { restartTimer.cancel(); } diff --git a/xds/src/test/java/io/grpc/xds/ClientXdsClientTestBase.java b/xds/src/test/java/io/grpc/xds/ClientXdsClientTestBase.java index 5fa9e3da734..d3ea5d53f2f 100644 --- a/xds/src/test/java/io/grpc/xds/ClientXdsClientTestBase.java +++ b/xds/src/test/java/io/grpc/xds/ClientXdsClientTestBase.java @@ -1555,12 +1555,16 @@ public void cdsResponseErrorHandling_badUpstreamTlsContext() { call.sendResponse(CDS, clusters, VERSION_1, "0000"); // The response NACKed with errors indicating indices of the failed resources. - call.verifyRequestNack(CDS, CDS_RESOURCE, "", "0000", NODE, ImmutableList.of( - "CDS response Cluster 'cluster.googleapis.com' validation error: " + String errorMsg = "CDS response Cluster 'cluster.googleapis.com' validation error: " + "Cluster cluster.googleapis.com: malformed UpstreamTlsContext: " + "io.grpc.xds.ClientXdsClient$ResourceInvalidException: " - + "ca_certificate_provider_instance is required in upstream-tls-context")); - verifyNoInteractions(cdsResourceWatcher); + + "ca_certificate_provider_instance is required in upstream-tls-context"; + call.verifyRequestNack(CDS, CDS_RESOURCE, "", "0000", NODE, ImmutableList.of(errorMsg)); + ArgumentCaptor captor = ArgumentCaptor.forClass(Status.class); + verify(cdsResourceWatcher).onError(captor.capture()); + Status errorStatus = captor.getValue(); + assertThat(errorStatus.getCode()).isEqualTo(Status.UNAVAILABLE.getCode()); + assertThat(errorStatus.getDescription()).isEqualTo(errorMsg); } /** @@ -1579,10 +1583,14 @@ public void cdsResponseErrorHandling_badTransportSocketName() { call.sendResponse(CDS, clusters, VERSION_1, "0000"); // The response NACKed with errors indicating indices of the failed resources. - call.verifyRequestNack(CDS, CDS_RESOURCE, "", "0000", NODE, ImmutableList.of( - "CDS response Cluster 'cluster.googleapis.com' validation error: " - + "transport-socket with name envoy.transport_sockets.bad not supported.")); - verifyNoInteractions(cdsResourceWatcher); + String errorMsg = "CDS response Cluster 'cluster.googleapis.com' validation error: " + + "transport-socket with name envoy.transport_sockets.bad not supported."; + call.verifyRequestNack(CDS, CDS_RESOURCE, "", "0000", NODE, ImmutableList.of(errorMsg)); + ArgumentCaptor captor = ArgumentCaptor.forClass(Status.class); + verify(cdsResourceWatcher).onError(captor.capture()); + Status errorStatus = captor.getValue(); + assertThat(errorStatus.getCode()).isEqualTo(Status.UNAVAILABLE.getCode()); + assertThat(errorStatus.getDescription()).isEqualTo(errorMsg); } @Test @@ -2429,10 +2437,15 @@ public void serverSideListenerResponseErrorHandling_badDownstreamTlsContext() { List listeners = ImmutableList.of(Any.pack(listener)); call.sendResponse(ResourceType.LDS, listeners, "0", "0000"); // The response NACKed with errors indicating indices of the failed resources. - call.verifyRequestNack(LDS, LISTENER_RESOURCE, "", "0000", NODE, ImmutableList.of( - "LDS response Listener \'grpc/server?xds.resource.listening_address=0.0.0.0:7000\' " - + "validation error: common-tls-context is required in downstream-tls-context")); - verifyNoInteractions(ldsResourceWatcher); + String errorMsg = "LDS response Listener \'grpc/server?xds.resource.listening_address=" + + "0.0.0.0:7000\' validation error: " + + "common-tls-context is required in downstream-tls-context"; + call.verifyRequestNack(LDS, LISTENER_RESOURCE, "", "0000", NODE, ImmutableList.of(errorMsg)); + ArgumentCaptor captor = ArgumentCaptor.forClass(Status.class); + verify(ldsResourceWatcher).onError(captor.capture()); + Status errorStatus = captor.getValue(); + assertThat(errorStatus.getCode()).isEqualTo(Status.UNAVAILABLE.getCode()); + assertThat(errorStatus.getDescription()).isEqualTo(errorMsg); } @Test @@ -2453,11 +2466,16 @@ public void serverSideListenerResponseErrorHandling_badTransportSocketName() { List listeners = ImmutableList.of(Any.pack(listener)); call.sendResponse(ResourceType.LDS, listeners, "0", "0000"); // The response NACKed with errors indicating indices of the failed resources. + String errorMsg = "LDS response Listener \'grpc/server?xds.resource.listening_address=" + + "0.0.0.0:7000\' validation error: " + + "transport-socket with name envoy.transport_sockets.bad1 not supported."; call.verifyRequestNack(LDS, LISTENER_RESOURCE, "", "0000", NODE, ImmutableList.of( - "LDS response Listener \'grpc/server?xds.resource.listening_address=0.0.0.0:7000\' " - + "validation error: " - + "transport-socket with name envoy.transport_sockets.bad1 not supported.")); - verifyNoInteractions(ldsResourceWatcher); + errorMsg)); + ArgumentCaptor captor = ArgumentCaptor.forClass(Status.class); + verify(ldsResourceWatcher).onError(captor.capture()); + Status errorStatus = captor.getValue(); + assertThat(errorStatus.getCode()).isEqualTo(Status.UNAVAILABLE.getCode()); + assertThat(errorStatus.getDescription()).isEqualTo(errorMsg); } private DiscoveryRpcCall startResourceWatcher( diff --git a/xds/src/test/java/io/grpc/xds/XdsClientWrapperForServerSdsTestMisc.java b/xds/src/test/java/io/grpc/xds/XdsClientWrapperForServerSdsTestMisc.java index 1871cb79770..a39a5495c09 100644 --- a/xds/src/test/java/io/grpc/xds/XdsClientWrapperForServerSdsTestMisc.java +++ b/xds/src/test/java/io/grpc/xds/XdsClientWrapperForServerSdsTestMisc.java @@ -63,15 +63,15 @@ import io.netty.handler.codec.http2.Http2ConnectionDecoder; import io.netty.handler.codec.http2.Http2ConnectionEncoder; import io.netty.handler.codec.http2.Http2Settings; -import java.io.IOException; import java.net.InetAddress; import java.net.InetSocketAddress; import java.net.SocketAddress; import java.util.Arrays; import java.util.Collections; -import java.util.concurrent.ExecutionException; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; + import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -190,8 +190,8 @@ public void run() { try { start.get(5, TimeUnit.SECONDS); fail("Start should throw exception"); - } catch (ExecutionException ex) { - assertThat(ex.getCause()).isInstanceOf(IOException.class); + } catch (TimeoutException ex) { + assertThat(start.isDone()).isFalse(); } assertThat(selectorManager.getSelectorToUpdateSelector()).isSameInstanceAs(NO_FILTER_CHAIN); } @@ -214,8 +214,8 @@ public void run() { try { start.get(5, TimeUnit.SECONDS); fail("Start should throw exception"); - } catch (ExecutionException ex) { - assertThat(ex.getCause()).isInstanceOf(IOException.class); + } catch (TimeoutException ex) { + assertThat(start.isDone()).isFalse(); } assertThat(selectorManager.getSelectorToUpdateSelector()).isSameInstanceAs(NO_FILTER_CHAIN); } @@ -238,8 +238,8 @@ public void run() { try { start.get(5, TimeUnit.SECONDS); fail("Start should throw exception"); - } catch (ExecutionException ex) { - assertThat(ex.getCause()).isInstanceOf(IOException.class); + } catch (TimeoutException ex) { + assertThat(start.isDone()).isFalse(); } assertThat(selectorManager.getSelectorToUpdateSelector()).isSameInstanceAs(NO_FILTER_CHAIN); } diff --git a/xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java b/xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java index e68d0f5175c..1bd102db42d 100644 --- a/xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsServerWrapperTest.java @@ -74,6 +74,7 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicReference; import org.junit.Before; import org.junit.Rule; @@ -261,9 +262,10 @@ public void run() { xdsClient.ldsWatcher.onResourceDoesNotExist(ldsResource); try { start.get(5000, TimeUnit.MILLISECONDS); - fail("Start should throw exception"); - } catch (ExecutionException ex) { - assertThat(ex.getCause()).isInstanceOf(IOException.class); + fail("server should not start() successfully."); + } catch (TimeoutException ex) { + // expect to block here. + assertThat(start.isDone()).isFalse(); } verify(mockBuilder, times(1)).build(); verify(mockServer, never()).start(); @@ -602,9 +604,10 @@ public void run() { xdsClient.ldsWatcher.onResourceDoesNotExist(ldsResource); try { start.get(5000, TimeUnit.MILLISECONDS); - fail("Start should throw exception"); - } catch (ExecutionException ex) { - assertThat(ex.getCause()).isInstanceOf(IOException.class); + fail("server should not start()"); + } catch (TimeoutException ex) { + // expect to block here. + assertThat(start.isDone()).isFalse(); } verify(listener, times(1)).onNotServing(any(StatusException.class)); verify(mockBuilder, times(1)).build(); @@ -627,6 +630,13 @@ public void run() { assertThat(sslSupplier0.isShutdown()).isTrue(); xdsClient.deliverRdsUpdate("rds", Collections.singletonList(createVirtualHost("virtual-host-1"))); + try { + start.get(5000, TimeUnit.MILLISECONDS); + fail("Start should throw exception"); + } catch (ExecutionException ex) { + assertThat(ex.getCause()).isInstanceOf(IOException.class); + assertThat(ex.getCause().getMessage()).isEqualTo("error!"); + } RdsResourceWatcher saveRdsWatcher = xdsClient.rdsWatchers.get("rds"); assertThat(executor.forwardNanos(RETRY_DELAY_NANOS)).isEqualTo(1); verify(mockBuilder, times(1)).build();