From f788eec9e05bc81a870cd4de14b24bd9e0724f9f Mon Sep 17 00:00:00 2001 From: sanjaypujare Date: Wed, 6 Jan 2021 16:44:34 -0800 Subject: [PATCH] xds: multiple changes needed for PSM security GA as discussed (#7777) * xds: multiple changes needed for GA: - check to allow XdsServerBuilder.build() only once - add transportBuilder() to XdsServerBuilder - remove "grpc/server" hardcoding - reorder the shutdown of delegate and xdsClient as per new design --- xds/src/main/java/io/grpc/xds/ServerXdsClient.java | 2 +- .../io/grpc/xds/XdsClientWrapperForServerSds.java | 8 +++++++- .../main/java/io/grpc/xds/XdsServerBuilder.java | 11 +++++++++++ .../grpc/xds/internal/sds/ServerWrapperForXds.java | 4 ++-- .../java/io/grpc/xds/XdsServerBuilderTest.java | 14 +++++++++++++- 5 files changed, 34 insertions(+), 5 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/ServerXdsClient.java b/xds/src/main/java/io/grpc/xds/ServerXdsClient.java index edb0275566..337fdb3838 100644 --- a/xds/src/main/java/io/grpc/xds/ServerXdsClient.java +++ b/xds/src/main/java/io/grpc/xds/ServerXdsClient.java @@ -76,7 +76,7 @@ final class ServerXdsClient extends AbstractXdsClient { super(channel, useProtocolV3, node, timeService, backoffPolicyProvider, stopwatchSupplier); this.useNewApiForListenerQuery = useProtocolV3 && useNewApiForListenerQuery; this.instanceIp = (instanceIp != null ? instanceIp : "0.0.0.0"); - this.grpcServerResourceId = grpcServerResourceId != null ? grpcServerResourceId : "grpc/server"; + this.grpcServerResourceId = grpcServerResourceId; } @Override diff --git a/xds/src/main/java/io/grpc/xds/XdsClientWrapperForServerSds.java b/xds/src/main/java/io/grpc/xds/XdsClientWrapperForServerSds.java index 454090289e..feadbd304a 100644 --- a/xds/src/main/java/io/grpc/xds/XdsClientWrapperForServerSds.java +++ b/xds/src/main/java/io/grpc/xds/XdsClientWrapperForServerSds.java @@ -114,6 +114,12 @@ public final class XdsClientWrapperForServerSds { .keepAliveTime(5, TimeUnit.MINUTES).build(); timeService = SharedResourceHolder.get(timeServiceResource); newServerApi = serverInfo.isUseProtocolV3() && experimentalNewServerApiEnvVar; + String grpcServerResourceId = bootstrapInfo.getGrpcServerResourceId(); + if (newServerApi && grpcServerResourceId == null) { + reportError( + Status.INVALID_ARGUMENT.withDescription("missing grpc_server_resource_name_id value")); + throw new IOException("missing grpc_server_resource_name_id value"); + } XdsClient xdsClientImpl = new ServerXdsClient( channel, @@ -124,7 +130,7 @@ public final class XdsClientWrapperForServerSds { GrpcUtil.STOPWATCH_SUPPLIER, experimentalNewServerApiEnvVar, "0.0.0.0", - bootstrapInfo.getGrpcServerResourceId()); + grpcServerResourceId); start(xdsClientImpl); } diff --git a/xds/src/main/java/io/grpc/xds/XdsServerBuilder.java b/xds/src/main/java/io/grpc/xds/XdsServerBuilder.java index a2911b26e9..a5e5640f62 100644 --- a/xds/src/main/java/io/grpc/xds/XdsServerBuilder.java +++ b/xds/src/main/java/io/grpc/xds/XdsServerBuilder.java @@ -16,8 +16,11 @@ package io.grpc.xds; +import static com.google.common.base.Preconditions.checkState; + import com.google.common.annotations.VisibleForTesting; import io.grpc.Attributes; +import io.grpc.ExperimentalApi; import io.grpc.ForwardingServerBuilder; import io.grpc.Internal; import io.grpc.Server; @@ -28,16 +31,19 @@ import io.grpc.netty.InternalNettyServerBuilder; import io.grpc.netty.NettyServerBuilder; import io.grpc.xds.internal.sds.SdsProtocolNegotiators; import io.grpc.xds.internal.sds.ServerWrapperForXds; +import java.util.concurrent.atomic.AtomicBoolean; /** * A version of {@link ServerBuilder} to create xDS managed servers that will use SDS to set up SSL * with peers. Note, this is not ready to use yet. */ +@ExperimentalApi("https://github.com/grpc/grpc-java/issues/7514") public final class XdsServerBuilder extends ForwardingServerBuilder { private final NettyServerBuilder delegate; private final int port; private ErrorNotifier errorNotifier; + private AtomicBoolean isServerBuilt = new AtomicBoolean(false); private XdsServerBuilder(NettyServerBuilder nettyDelegate, int port) { this.delegate = nettyDelegate; @@ -81,12 +87,17 @@ public final class XdsServerBuilder extends ForwardingServerBuilder transportBuilder() { + return delegate; + } + /** Watcher to receive error notifications from xDS control plane during {@code start()}. */ public interface ErrorNotifier { diff --git a/xds/src/main/java/io/grpc/xds/internal/sds/ServerWrapperForXds.java b/xds/src/main/java/io/grpc/xds/internal/sds/ServerWrapperForXds.java index bd92fc8fee..24f3e19dfa 100644 --- a/xds/src/main/java/io/grpc/xds/internal/sds/ServerWrapperForXds.java +++ b/xds/src/main/java/io/grpc/xds/internal/sds/ServerWrapperForXds.java @@ -113,15 +113,15 @@ public final class ServerWrapperForXds extends Server { @Override public Server shutdown() { - xdsClientWrapperForServerSds.shutdown(); delegate.shutdown(); + xdsClientWrapperForServerSds.shutdown(); return this; } @Override public Server shutdownNow() { - xdsClientWrapperForServerSds.shutdown(); delegate.shutdownNow(); + xdsClientWrapperForServerSds.shutdown(); return this; } diff --git a/xds/src/test/java/io/grpc/xds/XdsServerBuilderTest.java b/xds/src/test/java/io/grpc/xds/XdsServerBuilderTest.java index eca66dc383..5817347e94 100644 --- a/xds/src/test/java/io/grpc/xds/XdsServerBuilderTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsServerBuilderTest.java @@ -61,7 +61,7 @@ public class XdsServerBuilderTest { private int port; private XdsClientWrapperForServerSds xdsClientWrapperForServerSds; - private void buildServer( + private XdsServerBuilder buildServer( XdsServerBuilder.ErrorNotifier errorNotifier, boolean injectMockXdsClient) throws IOException { port = XdsServerTestHelper.findFreePort(); @@ -78,6 +78,7 @@ public class XdsServerBuilderTest { XdsServerTestHelper.startAndGetWatcher(xdsClientWrapperForServerSds, mockXdsClient, port); } xdsServer = cleanupRule.register(builder.buildServer(xdsClientWrapperForServerSds)); + return builder; } private void verifyServer( @@ -273,4 +274,15 @@ public class XdsServerBuilderTest { verifyShutdown(); } + @Test + public void xdsServer_2ndBuild_expectException() throws IOException { + XdsServerBuilder.ErrorNotifier mockErrorNotifier = mock(XdsServerBuilder.ErrorNotifier.class); + XdsServerBuilder builder = buildServer(mockErrorNotifier, true); + try { + builder.build(); + fail("exception expected"); + } catch (IllegalStateException expected) { + assertThat(expected).hasMessageThat().contains("Server already built!"); + } + } }