xds: google_default should use TLS if address contains no cluster name (#7818)

Fixes bug introduced by 4130c5a1b8. TLS should be selected for addresses without cluster name attributes, even if grpc-xds is in classpath.

alts/src/main/java/io/grpc/alts/internal/AltsProtocolNegotiator.java

@@ -247,8 +247,13 @@ public final class AltsProtocolNegotiator {
     public ChannelHandler newHandler(GrpcHttp2ConnectionHandler grpcHandler) {
       ChannelHandler gnh = InternalProtocolNegotiators.grpcNegotiationHandler(grpcHandler);
       ChannelHandler securityHandler;
-      boolean isXdsDirectPath = clusterNameAttrKey != null
-          && !"google_cfe".equals(grpcHandler.getEagAttributes().get(clusterNameAttrKey));
+      boolean isXdsDirectPath = false;
+      if (clusterNameAttrKey != null) {
+        String clusterName = grpcHandler.getEagAttributes().get(clusterNameAttrKey);
+        if (clusterName != null && !clusterName.equals("google_cfe")) {
+          isXdsDirectPath = true;
+        }
+      }
       if (grpcHandler.getEagAttributes().get(GrpclbConstants.ATTR_LB_ADDR_AUTHORITY) != null
           || grpcHandler.getEagAttributes().get(GrpclbConstants.ATTR_LB_PROVIDED_BACKEND) != null
           || isXdsDirectPath) {

alts/src/test/java/io/grpc/alts/internal/GoogleDefaultProtocolNegotiatorTest.java

@@ -36,25 +36,21 @@ import io.netty.channel.ChannelHandlerContext;
 import io.netty.channel.ChannelInboundHandlerAdapter;
 import io.netty.channel.embedded.EmbeddedChannel;
 import io.netty.handler.ssl.SslContext;
-import java.util.Arrays;
 import java.util.concurrent.atomic.AtomicReference;
+import javax.annotation.Nullable;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.experimental.runners.Enclosed;
 import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-import org.junit.runners.Parameterized.Parameters;
+import org.junit.runners.JUnit4;
 
-@RunWith(Parameterized.class)
+@RunWith(Enclosed.class)
 public final class GoogleDefaultProtocolNegotiatorTest {
-  @Parameterized.Parameter
-  public boolean withXds;
 
+  @RunWith(JUnit4.class)
+  public abstract static class HandlerSelectionTest {
     private ProtocolNegotiator googleProtocolNegotiator;
-
-  // Same as io.grpc.xds.InternalXdsAttributes.ATTR_CLUSTER_NAME
-  private final Attributes.Key<String> clusterNameAttrKey =
-      Attributes.Key.create("io.grpc.xds.InternalXdsAttributes.clusterName");
     private final ObjectPool<Channel> handshakerChannelPool = new ObjectPool<Channel>() {
 
       @Override
@@ -69,11 +65,6 @@ public final class GoogleDefaultProtocolNegotiatorTest {
       }
     };
 
-  @Parameters(name = "Run with xDS : {0}")
-  public static Iterable<Boolean> data() {
-    return Arrays.asList(true, false);
-  }
-
     @Before
     public void setUp() throws Exception {
       SslContext sslContext = GrpcSslContexts.forClient().build();
@@ -82,7 +73,7 @@ public final class GoogleDefaultProtocolNegotiatorTest {
           ImmutableList.<String>of(),
           handshakerChannelPool,
           sslContext,
-        withXds ? clusterNameAttrKey : null)
+          getClusterNameAttrKey())
           .newNegotiator();
     }
 
@@ -91,16 +82,22 @@ public final class GoogleDefaultProtocolNegotiatorTest {
       googleProtocolNegotiator.close();
     }
 
+    @Nullable
+    abstract Attributes.Key<String> getClusterNameAttrKey();
+
     @Test
-  public void altsHandler() {
-    Attributes eagAttributes;
-    if (withXds) {
-      eagAttributes =
-          Attributes.newBuilder().set(clusterNameAttrKey, "api.googleapis.com").build();
-    } else {
-      eagAttributes =
+    public void altsHandler_lbProvidedBackend() {
+      Attributes attrs =
           Attributes.newBuilder().set(GrpclbConstants.ATTR_LB_PROVIDED_BACKEND, true).build();
+      subtest_altsHandler(attrs);
     }
+
+    @Test
+    public void tlsHandler_emptyAttributes() {
+      subtest_tlsHandler(Attributes.EMPTY);
+    }
+
+    void subtest_altsHandler(Attributes eagAttributes) {
       GrpcHttp2ConnectionHandler mockHandler = mock(GrpcHttp2ConnectionHandler.class);
       when(mockHandler.getEagAttributes()).thenReturn(eagAttributes);
 
@@ -114,8 +111,8 @@ public final class GoogleDefaultProtocolNegotiatorTest {
       };
       ChannelHandler h = googleProtocolNegotiator.newHandler(mockHandler);
       EmbeddedChannel chan = new EmbeddedChannel(exceptionCaught);
-    // Add the negotiator handler last, but to the front.  Putting this in ctor above would make it
-    // throw early.
+      // Add the negotiator handler last, but to the front. Putting this in ctor above would make
+      // it throw early.
       chan.pipeline().addFirst(h);
       chan.pipeline().fireUserEventTriggered(InternalProtocolNegotiationEvent.getDefault());
 
@@ -124,14 +121,7 @@ public final class GoogleDefaultProtocolNegotiatorTest {
       assertThat(failure.get()).hasMessageThat().contains("TsiHandshakeHandler");
     }
 
-  @Test
-  public void tlsHandler() {
-    Attributes eagAttributes;
-    if (withXds) {
-      eagAttributes = Attributes.newBuilder().set(clusterNameAttrKey, "google_cfe").build();
-    } else {
-      eagAttributes = Attributes.EMPTY;
-    }
+    void subtest_tlsHandler(Attributes eagAttributes) {
       GrpcHttp2ConnectionHandler mockHandler = mock(GrpcHttp2ConnectionHandler.class);
       when(mockHandler.getEagAttributes()).thenReturn(eagAttributes);
       when(mockHandler.getAuthority()).thenReturn("authority");
@@ -143,3 +133,41 @@ public final class GoogleDefaultProtocolNegotiatorTest {
       assertThat(chan.pipeline().first().getClass().getSimpleName()).isEqualTo("SslHandler");
     }
   }
+
+  @RunWith(JUnit4.class)
+  public static class WithoutXdsInClasspath extends HandlerSelectionTest {
+
+    @Nullable
+    @Override
+    Attributes.Key<String> getClusterNameAttrKey() {
+      return null;
+    }
+  }
+
+  @RunWith(JUnit4.class)
+  public static class WithXdsInClasspath extends HandlerSelectionTest {
+    // Same as io.grpc.xds.InternalXdsAttributes.ATTR_CLUSTER_NAME
+    private static final Attributes.Key<String> XDS_CLUSTER_NAME_ATTR_KEY =
+        Attributes.Key.create("io.grpc.xds.InternalXdsAttributes.clusterName");
+
+    @Nullable
+    @Override
+    Attributes.Key<String> getClusterNameAttrKey() {
+      return XDS_CLUSTER_NAME_ATTR_KEY;
+    }
+
+    @Test
+    public void altsHandler_xdsCluster() {
+      Attributes attrs =
+          Attributes.newBuilder().set(XDS_CLUSTER_NAME_ATTR_KEY, "api.googleapis.com").build();
+      subtest_altsHandler(attrs);
+    }
+
+    @Test
+    public void tlsHandler_googleCfe() {
+      Attributes attrs =
+          Attributes.newBuilder().set(XDS_CLUSTER_NAME_ATTR_KEY, "google_cfe").build();
+      subtest_tlsHandler(attrs);
+    }
+  }
+}