Changes discussed.

Author: kannanjgithub

netty/src/main/java/io/grpc/netty/InternalProtocolNegotiators.java

@@ -47,11 +47,9 @@ public static InternalProtocolNegotiator.ProtocolNegotiator tls(SslContext sslCo
           ObjectPool<? extends Executor> executorPool,
           Optional<Runnable> handshakeCompleteRunnable,
           TrustManager extendedX509TrustManager,
-          String sni,
-          boolean isXdsTarget) {
+          String sni) {
     final io.grpc.netty.ProtocolNegotiator negotiator = ProtocolNegotiators.tls(sslContext,
-        executorPool, handshakeCompleteRunnable, (X509TrustManager) extendedX509TrustManager, sni,
-        isXdsTarget);
+        executorPool, handshakeCompleteRunnable, (X509TrustManager) extendedX509TrustManager, sni);
     final class TlsNegotiator implements InternalProtocolNegotiator.ProtocolNegotiator {
 
       @Override
@@ -79,9 +77,9 @@ public void close() {
    * may happen immediately, even before the TLS Handshake is complete.
    */
   public static InternalProtocolNegotiator.ProtocolNegotiator tls(
-      SslContext sslContext, String sni, boolean isXdsTarget,
+      SslContext sslContext, String sni,
       TrustManager extendedX509TrustManager) {
-    return tls(sslContext, null, Optional.absent(), extendedX509TrustManager, sni, isXdsTarget);
+    return tls(sslContext, null, Optional.absent(), extendedX509TrustManager, sni);
   }
 
   /**

netty/src/main/java/io/grpc/netty/NettyChannelBuilder.java

@@ -653,7 +653,7 @@ static ProtocolNegotiator createProtocolNegotiatorByType(
         return ProtocolNegotiators.plaintextUpgrade();
       case TLS:
         return ProtocolNegotiators.tls(
-            sslContext, executorPool, Optional.absent(), null, null, false);
+            sslContext, executorPool, Optional.absent(), null, null);
       default:
         throw new IllegalArgumentException("Unsupported negotiationType: " + negotiationType);
     }

netty/src/main/java/io/grpc/netty/ProtocolNegotiators.java

@@ -564,7 +564,7 @@ static final class ClientTlsProtocolNegotiator implements ProtocolNegotiator {
 
     public ClientTlsProtocolNegotiator(SslContext sslContext,
         ObjectPool<? extends Executor> executorPool, Optional<Runnable> handshakeCompleteRunnable,
-        X509TrustManager x509ExtendedTrustManager, String sni, boolean isXdsTarget) {
+        X509TrustManager x509ExtendedTrustManager, String sni) {
       this.sslContext = Preconditions.checkNotNull(sslContext, "sslContext");
       this.executorPool = executorPool;
       if (this.executorPool != null) {
@@ -573,18 +573,13 @@ public ClientTlsProtocolNegotiator(SslContext sslContext,
       this.handshakeCompleteRunnable = handshakeCompleteRunnable;
       this.x509ExtendedTrustManager = x509ExtendedTrustManager;
       this.sni = sni;
-      this.isXdsTarget = isXdsTarget;
     }
 
     private final SslContext sslContext;
     private final ObjectPool<? extends Executor> executorPool;
     private final Optional<Runnable> handshakeCompleteRunnable;
     private final X509TrustManager x509ExtendedTrustManager;
     private final String sni;
-    // For xds targets there may be no SNI determined, and no SNI may be sent in that case.
-    // Non xds-targets will always use channel authority for SNI. This field is used to handle
-    // the two cases differently.
-    private final boolean isXdsTarget;
     private Executor executor;
 
     @Override
@@ -597,7 +592,7 @@ public ChannelHandler newHandler(GrpcHttp2ConnectionHandler grpcHandler) {
       ChannelHandler gnh = new GrpcNegotiationHandler(grpcHandler);
       ChannelLogger negotiationLogger = grpcHandler.getNegotiationLogger();
       ChannelHandler cth = new ClientTlsHandler(gnh, sslContext,
-          isXdsTarget ? sni : grpcHandler.getAuthority(),
+          sni != null? sni : grpcHandler.getAuthority(),
           this.executor, negotiationLogger, handshakeCompleteRunnable, null,
           x509ExtendedTrustManager);
       return new WaitUntilActiveHandler(cth, negotiationLogger);
@@ -753,9 +748,9 @@ static HostPort parseAuthority(String authority) {
    */
   public static ProtocolNegotiator tls(SslContext sslContext,
       ObjectPool<? extends Executor> executorPool, Optional<Runnable> handshakeCompleteRunnable,
-      X509TrustManager x509ExtendedTrustManager, String sni, boolean isXdsTarget) {
+      X509TrustManager x509ExtendedTrustManager, String sni) {
     return new ClientTlsProtocolNegotiator(sslContext, executorPool, handshakeCompleteRunnable,
-        x509ExtendedTrustManager, sni, isXdsTarget);
+        x509ExtendedTrustManager, sni);
   }
 
   /**

netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java

@@ -877,7 +877,7 @@ public void tlsNegotiationServerExecutorShouldSucceed() throws Exception {
         .keyManager(clientCert, clientKey)
         .build();
     ProtocolNegotiator negotiator = ProtocolNegotiators.tls(clientContext, clientExecutorPool,
-        Optional.absent(), null, null, false);
+        Optional.absent(), null, null);
     // after starting the client, the Executor in the client pool should be used
     assertEquals(true, clientExecutorPool.isInUse());
     final NettyClientTransport transport = newTransport(negotiator);

netty/src/test/java/io/grpc/netty/ProtocolNegotiatorsTest.java

@@ -1271,7 +1271,7 @@ public void clientTlsHandler_firesNegotiation() throws Exception {
     }
     FakeGrpcHttp2ConnectionHandler gh = FakeGrpcHttp2ConnectionHandler.newHandler();
     ClientTlsProtocolNegotiator pn = new ClientTlsProtocolNegotiator(clientSslContext,
-        null, Optional.absent(), null, null, false);
+        null, Optional.absent(), null, null);
     WriteBufferingAndExceptionHandler clientWbaeh =
         new WriteBufferingAndExceptionHandler(pn.newHandler(gh));
 

xds/src/main/java/io/grpc/xds/TlsContextManager.java

@@ -30,7 +30,7 @@ SslContextProvider findOrCreateServerSslContextProvider(
 
   /** Creates a SslContextProvider. Used for retrieving a client-side SslContext. */
   SslContextProvider findOrCreateClientSslContextProvider(
-      UpstreamTlsContext upstreamTlsContext, String sni);
+      UpstreamTlsContext upstreamTlsContext);
 
   /**
    * Releases an instance of the given client-side {@link SslContextProvider}.
@@ -41,8 +41,7 @@ SslContextProvider findOrCreateClientSslContextProvider(
    * <p>Caller must not release a reference more than once. It's advised that you clear the
    * reference to the instance with the null returned by this method.
    */
-  SslContextProvider releaseClientSslContextProvider(SslContextProvider sslContextProvider,
-      String sni);
+  SslContextProvider releaseClientSslContextProvider(SslContextProvider sslContextProvider);
 
   /**
    * Releases an instance of the given server-side {@link SslContextProvider}.

xds/src/main/java/io/grpc/xds/internal/security/SecurityProtocolNegotiators.java

@@ -247,7 +247,7 @@ public void updateSslContextAndExtendedX509TrustManager(
                   new Object[]{grpcHandler.getAuthority(), ctx.name()});
               ChannelHandler handler =
                   InternalProtocolNegotiators.tls(
-                      sslContextAndTm.getKey(), sni, true, sslContextAndTm.getValue())
+                      sslContextAndTm.getKey(), sni, sslContextAndTm.getValue())
                       .newHandler(grpcHandler);
 
               // Delegate rest of handshake to TLS handler
@@ -260,8 +260,7 @@ public void updateSslContextAndExtendedX509TrustManager(
             public void onException(Throwable throwable) {
               ctx.fireExceptionCaught(throwable);
             }
-          },
-          sni);
+          });
     }
 
     @Override

xds/src/main/java/io/grpc/xds/internal/security/SslContextProviderSupplier.java

@@ -42,7 +42,6 @@ public final class SslContextProviderSupplier implements Closeable {
 
   private final BaseTlsContext tlsContext;
   private final TlsContextManager tlsContextManager;
-  private final Set<String> snisSentByClients = new HashSet<>();
   private SslContextProvider sslContextProvider;
   private boolean shutdown;
 
@@ -58,30 +57,30 @@ public BaseTlsContext getTlsContext() {
 
   /** Updates SslContext via the passed callback. */
   public synchronized void updateSslContext(
-      final SslContextProvider.Callback callback, String sni) {
+      final SslContextProvider.Callback callback) {
     checkNotNull(callback, "callback");
     try {
       if (!shutdown) {
         if (sslContextProvider == null) {
-          sslContextProvider = getSslContextProvider(sni);
+          sslContextProvider = getSslContextProvider();
         }
       }
       // we want to increment the ref-count so call findOrCreate again...
-      final SslContextProvider toRelease = getSslContextProvider(sni);
+      final SslContextProvider toRelease = getSslContextProvider();
       toRelease.addCallback(
           new SslContextProvider.Callback(callback.getExecutor()) {
 
           @Override
           public void updateSslContextAndExtendedX509TrustManager(
               AbstractMap.SimpleImmutableEntry<SslContext, TrustManager> sslContextAndTm) {
             callback.updateSslContextAndExtendedX509TrustManager(sslContextAndTm);
-            releaseSslContextProvider(toRelease, sni);
+            releaseSslContextProvider(toRelease);
           }
 
           @Override
           public void onException(Throwable throwable) {
             callback.onException(throwable);
-            releaseSslContextProvider(toRelease, sni);
+            releaseSslContextProvider(toRelease);
           }
         });
     } catch (final Throwable throwable) {
@@ -94,20 +93,18 @@ public void run() {
     }
   }
 
-  private void releaseSslContextProvider(SslContextProvider toRelease, String sni) {
+  private void releaseSslContextProvider(SslContextProvider toRelease) {
     if (tlsContext instanceof UpstreamTlsContext) {
-      tlsContextManager.releaseClientSslContextProvider(toRelease, sni);
-      snisSentByClients.remove(sni);
+      tlsContextManager.releaseClientSslContextProvider(toRelease);
     } else {
       tlsContextManager.releaseServerSslContextProvider(toRelease);
     }
   }
 
-  private SslContextProvider getSslContextProvider(String sni) {
+  private SslContextProvider getSslContextProvider() {
     if (tlsContext instanceof UpstreamTlsContext) {
-      snisSentByClients.add(sni);
       return tlsContextManager.findOrCreateClientSslContextProvider(
-          (UpstreamTlsContext) tlsContext, sni);
+          (UpstreamTlsContext) tlsContext);
     }
     return tlsContextManager.findOrCreateServerSslContextProvider(
         (DownstreamTlsContext) tlsContext);
@@ -122,9 +119,7 @@ private SslContextProvider getSslContextProvider(String sni) {
   public synchronized void close() {
     if (sslContextProvider != null) {
       if (tlsContext instanceof UpstreamTlsContext) {
-        for (String sni: snisSentByClients) {
-          tlsContextManager.releaseClientSslContextProvider(sslContextProvider, sni);
-        }
+        tlsContextManager.releaseClientSslContextProvider(sslContextProvider);
       } else {
         tlsContextManager.releaseServerSslContextProvider(sslContextProvider);
       }

xds/src/main/java/io/grpc/xds/internal/security/TlsContextManagerImpl.java

@@ -72,7 +72,7 @@ public SslContextProvider findOrCreateServerSslContextProvider(
 
   @Override
   public SslContextProvider findOrCreateClientSslContextProvider(
-      UpstreamTlsContext upstreamTlsContext, String sni) {
+      UpstreamTlsContext upstreamTlsContext) {
     checkNotNull(upstreamTlsContext, "upstreamTlsContext");
     return mapForClients.get(new AbstractMap.SimpleImmutableEntry<>(upstreamTlsContext, sni));
   }

xds/src/test/java/io/grpc/xds/ClusterImplLoadBalancerTest.java

@@ -1259,7 +1259,7 @@ public Map<ServerInfo, LoadReportClient> getServerLrsClientMap() {
   private static final class FakeTlsContextManager implements TlsContextManager {
     @Override
     public SslContextProvider findOrCreateClientSslContextProvider(
-        UpstreamTlsContext upstreamTlsContext, String sni) {
+        UpstreamTlsContext upstreamTlsContext) {
       SslContextProvider sslContextProvider = mock(SslContextProvider.class);
       when(sslContextProvider.getUpstreamTlsContext()).thenReturn(upstreamTlsContext);
       return sslContextProvider;