From 0b0079c8a136a8fd011be5b74fef3bb906f4b4d5 Mon Sep 17 00:00:00 2001 From: yifeizhuang Date: Mon, 8 Nov 2021 15:21:59 -0800 Subject: [PATCH] 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 c4fedea4aa..005da16a01 100644 --- a/xds/src/main/java/io/grpc/xds/ClientXdsClient.java +++ b/xds/src/main/java/io/grpc/xds/ClientXdsClient.java @@ -2209,11 +2209,13 @@ final class ClientXdsClient extends XdsClient implements XdsResponseHandler, Res } 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 ed51ccc9ed..dab3cd798f 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 @@ final class XdsServerWrapper extends Server { 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 9809738c68..4a34538954 100644 --- a/xds/src/test/java/io/grpc/xds/ClientXdsClientTestBase.java +++ b/xds/src/test/java/io/grpc/xds/ClientXdsClientTestBase.java @@ -1563,12 +1563,16 @@ public abstract class ClientXdsClientTestBase { 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); } /** @@ -1587,10 +1591,14 @@ public abstract class ClientXdsClientTestBase { 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 @@ -2438,10 +2446,15 @@ public abstract class ClientXdsClientTestBase { 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 @@ -2462,11 +2475,16 @@ public abstract class ClientXdsClientTestBase { 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 1871cb7977..a39a5495c0 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.DefaultHttp2FrameWriter; 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 class XdsClientWrapperForServerSdsTestMisc { 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 class XdsClientWrapperForServerSdsTestMisc { 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 class XdsClientWrapperForServerSdsTestMisc { 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 e68d0f5175..1bd102db42 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.CountDownLatch; 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 class XdsServerWrapperTest { 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 class XdsServerWrapperTest { 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 class XdsServerWrapperTest { 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();