Throw IllegalStateException when handshake fails due to version or cluster mismatch (#18676)

We do throw ConnectTransportException which is logged in trace level hiding a potentially
important information  when an old or wrong node wants to connect. We should throw ISE and
log as warn.

core/src/main/java/org/elasticsearch/transport/TransportService.java

@@ -316,8 +316,8 @@ public class TransportService extends AbstractLifecycleComponent<TransportServic
      * @param checkClusterName whether or not to ignore cluster name
      *                         mismatches
      * @return the connected node
-     * @throws ConnectTransportException if the connection or the
-     *                                   handshake failed
+     * @throws ConnectTransportException if the connection failed
+     * @throws IllegalStateException if the handshake failed
      */
     public DiscoveryNode connectToNodeLightAndHandshake(
             final DiscoveryNode node,
@@ -329,7 +329,7 @@ public class TransportService extends AbstractLifecycleComponent<TransportServic
         transport.connectToNodeLight(node);
         try {
             return handshake(node, handshakeTimeout, checkClusterName);
-        } catch (ConnectTransportException e) {
+        } catch (ConnectTransportException | IllegalStateException e) {
             transport.disconnectFromNode(node);
             throw e;
         }
@@ -353,13 +353,13 @@ public class TransportService extends AbstractLifecycleComponent<TransportServic
                     }
                 }).txGet();
         } catch (Exception e) {
-            throw new ConnectTransportException(node, "handshake failed", e);
+            throw new IllegalStateException("handshake failed with " + node, e);
         }
 
         if (checkClusterName && !Objects.equals(clusterName, response.clusterName)) {
-            throw new ConnectTransportException(node, "handshake failed, mismatched cluster name [" + response.clusterName + "]");
+            throw new IllegalStateException("handshake failed, mismatched cluster name [" + response.clusterName + "] - " + node);
         } else if (!isVersionCompatible(response.version)) {
-            throw new ConnectTransportException(node, "handshake failed, incompatible version [" + response.version + "]");
+            throw new IllegalStateException("handshake failed, incompatible version [" + response.version + "] - " + node);
         }
 
         return response.discoveryNode;

core/src/test/java/org/elasticsearch/transport/NettyTransportServiceHandshakeTests.java

@@ -109,21 +109,20 @@ public class NettyTransportServiceHandshakeTests extends ESTestCase {
                         settings,
                         VersionUtils.randomVersionBetween(random(), Version.CURRENT.minimumCompatibilityVersion(), Version.CURRENT),
                         test);
-
+        DiscoveryNode discoveryNode = new DiscoveryNode(
+            "",
+            handleB.discoveryNode.getAddress(),
+            emptyMap(),
+            emptySet(),
+            Version.CURRENT.minimumCompatibilityVersion());
         DiscoveryNode connectedNode =
-                handleA.transportService.connectToNodeLightAndHandshake(
-                        new DiscoveryNode(
-                                "",
-                                handleB.discoveryNode.getAddress(),
-                                emptyMap(),
-                                emptySet(),
-                                Version.CURRENT.minimumCompatibilityVersion()),
-                        timeout);
+                handleA.transportService.connectToNodeLightAndHandshake(discoveryNode, timeout);
         assertNotNull(connectedNode);
 
         // the name and version should be updated
         assertEquals(connectedNode.getName(), "TS_B");
         assertEquals(connectedNode.getVersion(), handleB.discoveryNode.getVersion());
+        assertTrue(handleA.transportService.nodeConnected(discoveryNode));
     }
 
     public void testMismatchedClusterName() {
@@ -131,21 +130,17 @@ public class NettyTransportServiceHandshakeTests extends ESTestCase {
 
         NetworkHandle handleA = startServices("TS_A", settings, Version.CURRENT, new ClusterName("a"));
         NetworkHandle handleB = startServices("TS_B", settings, Version.CURRENT, new ClusterName("b"));
-
-        try {
-            handleA.transportService.connectToNodeLightAndHandshake(
-                    new DiscoveryNode(
-                            "",
-                            handleB.discoveryNode.getAddress(),
-                            emptyMap(),
-                            emptySet(),
-                            Version.CURRENT.minimumCompatibilityVersion()),
-                    timeout);
-            fail("expected handshake to fail from mismatched cluster names");
-        } catch (ConnectTransportException e) {
-            assertThat(e.getMessage(), containsString("handshake failed, mismatched cluster name [Cluster [b]]"));
-        }
-    }
+        DiscoveryNode discoveryNode = new DiscoveryNode(
+            "",
+            handleB.discoveryNode.getAddress(),
+            emptyMap(),
+            emptySet(),
+            Version.CURRENT.minimumCompatibilityVersion());
+        IllegalStateException ex = expectThrows(IllegalStateException.class, () -> handleA.transportService.connectToNodeLightAndHandshake(
+                discoveryNode, timeout));
+        assertThat(ex.getMessage(), containsString("handshake failed, mismatched cluster name [Cluster [b]]"));
+        assertFalse(handleA.transportService.nodeConnected(discoveryNode));
+}
 
     public void testIncompatibleVersions() {
         Settings settings = Settings.EMPTY;
@@ -154,20 +149,16 @@ public class NettyTransportServiceHandshakeTests extends ESTestCase {
         NetworkHandle handleA = startServices("TS_A", settings, Version.CURRENT, test);
         NetworkHandle handleB =
                 startServices("TS_B", settings, VersionUtils.getPreviousVersion(Version.CURRENT.minimumCompatibilityVersion()), test);
-
-        try {
-            handleA.transportService.connectToNodeLightAndHandshake(
-                    new DiscoveryNode(
-                            "",
-                            handleB.discoveryNode.getAddress(),
-                            emptyMap(),
-                            emptySet(),
-                            Version.CURRENT.minimumCompatibilityVersion()),
-                    timeout);
-            fail("expected handshake to fail from incompatible versions");
-        } catch (ConnectTransportException e) {
-            assertThat(e.getMessage(), containsString("handshake failed, incompatible version"));
-        }
+        DiscoveryNode discoveryNode = new DiscoveryNode(
+            "",
+            handleB.discoveryNode.getAddress(),
+            emptyMap(),
+            emptySet(),
+            Version.CURRENT.minimumCompatibilityVersion());
+        IllegalStateException ex = expectThrows(IllegalStateException.class, () -> handleA.transportService.connectToNodeLightAndHandshake(
+            discoveryNode, timeout));
+        assertThat(ex.getMessage(), containsString("handshake failed, incompatible version"));
+        assertFalse(handleA.transportService.nodeConnected(discoveryNode));
     }
 
     public void testIgnoreMismatchedClusterName() {
@@ -181,19 +172,17 @@ public class NettyTransportServiceHandshakeTests extends ESTestCase {
                         VersionUtils.randomVersionBetween(random(), Version.CURRENT.minimumCompatibilityVersion(), Version.CURRENT),
                         new ClusterName("b")
                 );
-
-        DiscoveryNode connectedNode = handleA.transportService.connectToNodeLightAndHandshake(
-                new DiscoveryNode(
-                        "",
-                        handleB.discoveryNode.getAddress(),
-                        emptyMap(),
-                        emptySet(),
-                        Version.CURRENT.minimumCompatibilityVersion()),
-                timeout,
-                false);
+        DiscoveryNode discoveryNode = new DiscoveryNode(
+            "",
+            handleB.discoveryNode.getAddress(),
+            emptyMap(),
+            emptySet(),
+            Version.CURRENT.minimumCompatibilityVersion());
+        DiscoveryNode connectedNode = handleA.transportService.connectToNodeLightAndHandshake(discoveryNode, timeout, false);
         assertNotNull(connectedNode);
         assertEquals(connectedNode.getName(), "TS_B");
         assertEquals(connectedNode.getVersion(), handleB.discoveryNode.getVersion());
+        assertTrue(handleA.transportService.nodeConnected(discoveryNode));
     }
 
     private static class NetworkHandle {