IMPALA-2864: Ensure that client connections are closed after a failed…

… Open()

When a client tries to Open() a socket and fails, we
previously assumed that the socket was never opened and therefore did
not close it. However, if Kerberos is enabled, the
ThriftClientImpl::Open() calls TSaslTransport::Open(), which not only
opens the socket but also does the initial handshake. If there was an
error during the handshake, we just returned with an error without
closing the socket, causing the server side to wait on the same
connection.

This patch closes the client side socket, thereby terminating the
connection to the server in the above scenario, so that the server
side doesn't need to hold on to a connection until a timeout
terminates the connection.

A thrift-server-test is added to test the RPC failure path.

Change-Id: Ia7e883d8224304ad13a051f595d0e8abf4f9671e
Reviewed-on: http://gerrit.cloudera.org:8080/5385
Reviewed-by: Sailesh Mukil <sailesh@cloudera.com>
Tested-by: Internal Jenkins

be/src/rpc/thrift-client.cc

@@ -42,6 +42,12 @@ Status ThriftClientImpl::Open() {
       transport_->open();
     }
   } catch (const TException& e) {
+    try {
+      transport_->close();
+    } catch (const TException& e) {
+      VLOG(1) << "Error closing socket to: " << address_ << ", ignoring (" << e.what()
+                << ")";
+    }
     return Status(Substitute("Couldn't open transport for $0 ($1)",
         lexical_cast<string>(address_), e.what()));
   }

be/src/rpc/thrift-client.h

@@ -52,6 +52,7 @@ class ThriftClientImpl {
 
   /// Open the connection to the remote server. May be called repeatedly, is idempotent
   /// unless there is a failure to connect.
+  /// If Open() fails, the connection remains closed.
   Status Open();
 
   /// Retry the Open num_retries time waiting wait_ms milliseconds between retries.

be/src/rpc/thrift-server-test.cc

@@ -42,6 +42,10 @@ const string& SERVER_CERT =
     Substitute("$0/be/src/testutil/server-cert.pem", IMPALA_HOME);
 const string& PRIVATE_KEY =
     Substitute("$0/be/src/testutil/server-key.pem", IMPALA_HOME);
+const string& BAD_SERVER_CERT =
+    Substitute("$0/be/src/testutil/bad-cert.pem", IMPALA_HOME);
+const string& BAD_PRIVATE_KEY =
+    Substitute("$0/be/src/testutil/bad-key.pem", IMPALA_HOME);
 const string& PASSWORD_PROTECTED_PRIVATE_KEY =
     Substitute("$0/be/src/testutil/server-key-password.pem", IMPALA_HOME);
 
@@ -193,4 +197,25 @@ TEST(ConcurrencyTest, DISABLED_ManyConcurrentConnections) {
   pool.DrainAndShutdown();
 }
 
+TEST(NoPasswordPemFile, BadServerCertificate) {
+  ThriftServer* server = new ThriftServer("DummyStatestore", MakeProcessor(),
+      FLAGS_state_store_port + 5, NULL, NULL, 5);
+  EXPECT_OK(server->EnableSsl(BAD_SERVER_CERT, BAD_PRIVATE_KEY));
+  EXPECT_OK(server->Start());
+  FLAGS_ssl_client_ca_certificate = SERVER_CERT;
+  ThriftClient<StatestoreServiceClient> ssl_client(
+      "localhost", FLAGS_state_store_port + 5, "", NULL, true);
+  EXPECT_OK(ssl_client.Open());
+  TRegisterSubscriberResponse resp;
+  EXPECT_THROW({
+    ssl_client.iface()->RegisterSubscriber(resp, TRegisterSubscriberRequest());
+  }, TSSLException);
+  // Close and reopen the socket
+  ssl_client.Close();
+  EXPECT_OK(ssl_client.Open());
+  EXPECT_THROW({
+    ssl_client.iface()->RegisterSubscriber(resp, TRegisterSubscriberRequest());
+  }, TSSLException);
+}
+
 IMPALA_TEST_MAIN();

be/src/runtime/client-cache.h

@@ -89,7 +89,7 @@ class ClientCacheHelper {
   Status ReopenClient(ClientFactory factory_method, ClientKey* client_key);
 
   /// Returns a client to the cache. Upon return, *client_key will be NULL, and the
-  /// associated client will be available in the per-host cache..
+  /// associated client will be available in the per-host cache.
   void ReleaseClient(ClientKey* client_key);
 
   /// Close all connections to a host (e.g., in case of failure) so that on their

be/src/runtime/data-stream-test.cc

@@ -618,7 +618,7 @@ TEST_F(DataStreamTest, CloseRecvrWhileReferencesRemain) {
   Status rpc_status;
   ImpalaBackendConnection client(exec_env_.impalad_client_cache(),
       MakeNetworkAddress("localhost", FLAGS_port), &rpc_status);
-  EXPECT_TRUE(rpc_status.ok());
+  EXPECT_OK(rpc_status);
   TTransmitDataParams params;
   params.protocol_version = ImpalaInternalServiceVersion::V1;
   params.__set_eos(true);

be/src/testutil/bad-cert.pem

@@ -0,0 +1,22 @@
+-----BEGIN CERTIFICATE-----
+MIIDrTCCApWgAwIBAgIJAMcMGMuKLPyIMA0GCSqGSIb3DQEBCwUAMG0xCzAJBgNV
+BAYTAlVTMRMwEQYDVQQIDApTb21lLVN0YXRlMREwDwYDVQQKDAhDbG91ZGVyYTER
+MA8GA1UEAwwIYmFkLWhvc3QxIzAhBgkqhkiG9w0BCQEWFHNhaWxlc2hAY2xvdWRl
+cmEuY29tMB4XDTE1MTIwNzIzMDI0MVoXDTE2MDEwNjIzMDI0MVowbTELMAkGA1UE
+BhMCVVMxEzARBgNVBAgMClNvbWUtU3RhdGUxETAPBgNVBAoMCENsb3VkZXJhMREw
+DwYDVQQDDAhiYWQtaG9zdDEjMCEGCSqGSIb3DQEJARYUc2FpbGVzaEBjbG91ZGVy
+YS5jb20wggEiMA0GCSqGSIb3DQEBAQUAA4IBDwAwggEKAoIBAQC7+wbcxCdWsINS
+BeCMaI2Bv5Z7poPMCSDdC/dvv3LRNF40w86qEZPs7o6Dw7JEUy40eDdDWcfZU4bT
+B24ukgtdjBvXE4JlgZMOojUX1s/qgtMPvi20qo9bOYT+jI20/wAtaIiKo05f+gm8
+kFWYbqCUOYMKwkMlhhvOjsiZDSRDcep27zturbbF1rXtZWL4HNnpaDNvRBJEg+Y+
+m67uUNFGt8wcP+Ytku2vqNxvzdYTVccxIzfNYQEt49pQ6RJgE+cFePKYWuz7IzJk
+YlZt/WjIMyzR7WRkhlSAc1llQXJwGFKRIkRj3R6M+fdiYR9zWJUKgv1176Wb6+lw
+EJaw1f6FAgMBAAGjUDBOMB0GA1UdDgQWBBQsKWaEDS8lAHCgjMI6a/xfUswb0DAf
+BgNVHSMEGDAWgBQsKWaEDS8lAHCgjMI6a/xfUswb0DAMBgNVHRMEBTADAQH/MA0G
+CSqGSIb3DQEBCwUAA4IBAQBzWjquoS7Q1raZPFuYDLmlXa3CxUjqggfk40Ovja0r
+ZedwScgWd8/NVfXDDWPTJLlT+wEIRrbFkQw65dVNLA4hSwLGVSmG10JgxP+uhv8O
+kzGMCmVEhJDkpp0sdYdz3bGzxZX74BXe8pOjhHbv//Kv94k3Tu9LdKMi7V4Kqlct
+3Xpjjks2kKG1KMYy8aBmWlDw3RmI0hL79bdGG53oIeunEA7chPOwz+nAN6fgFYCg
+swDe17iFRhw9yw2d7yRWfq3dph6ao/Z65t3IHsMTWL+P/pAjRvAgj+RR/LS6BHL3
+TAmsDl93SMh/51kh0Zq035iv7+2YgS/NdRG9d0kdcrZX
+-----END CERTIFICATE-----

be/src/testutil/bad-key.pem

@@ -0,0 +1,28 @@
+-----BEGIN PRIVATE KEY-----
+MIIEvwIBADANBgkqhkiG9w0BAQEFAASCBKkwggSlAgEAAoIBAQC7+wbcxCdWsINS
+BeCMaI2Bv5Z7poPMCSDdC/dvv3LRNF40w86qEZPs7o6Dw7JEUy40eDdDWcfZU4bT
+B24ukgtdjBvXE4JlgZMOojUX1s/qgtMPvi20qo9bOYT+jI20/wAtaIiKo05f+gm8
+kFWYbqCUOYMKwkMlhhvOjsiZDSRDcep27zturbbF1rXtZWL4HNnpaDNvRBJEg+Y+
+m67uUNFGt8wcP+Ytku2vqNxvzdYTVccxIzfNYQEt49pQ6RJgE+cFePKYWuz7IzJk
+YlZt/WjIMyzR7WRkhlSAc1llQXJwGFKRIkRj3R6M+fdiYR9zWJUKgv1176Wb6+lw
+EJaw1f6FAgMBAAECggEAc0znyqWuE2g1RCxCrRy8Hydqn/FkydOXir36SVq+jD94
+wRiRPJOHjj5Mv9lbELmMj7Zk/zSkdlLbUbkvBfWibwCvWt6mjqhJkSJBOpwR75/K
+4c8ercAoKiY/wvpnOOtoKnIBvjeorQnqyvQk7Fh+uiwEiqbZFL0LdUjzFZ2P7qVz
+nRvMsyXEbQ/ycA6Ji34viNgOFNjWwemDtCutHZdUPYsD/JfuGg6ivGQRqyEFlY9g
+Tw/l9idCx7JTAMzH8hMzJNbLEWE0Sa1TxPFXGj+WYjeXFuAZ42D6jZsGNU675Wa+
+SFTxXvkXk8sXybnYY67vd6hNUUiCzMq09AJpYZ5qSQKBgQD4ewuaBRisFkIImWO9
+vYBcpcma3MDyF7FHpyApIEzQYUQbKR+91yr5r51Q1z9UwwX3stA58hPUkXNu3iBA
+PQqO8aPmTGo9yPRpqPzzt4Gh4Pzzmcik81zF2BIkCLvzrHZrB8chM+Az3JKOxI9T
+Lb2PNKKY81Rq9UIkPkJsFAXL/wKBgQDBq0sh2IfjHp6AHKh9QpyfdM/u3ALkf2hL
+OvpcnjyLPVDvYEGgN8GafbcpoN6XLep2oz9Od+7GVwqLjRMspyZM1Js8UFhtz8eN
+/MsQfFW9ivIMCdMMU1ozOmlcfLoq4/etVaYfLQRtlMKvL8zRKneoXjBZV68V4kZ2
+PxGMfu8FewKBgQCj2A7IWn/wSST1op9AJ8qSTMdpFBMuDy1Yf/0W4TOFW/2aoz1I
+4q51wbTL74LVE1vF/uSKsPMegWJKQrGlahqiMvfODakoYG+5lDJnSiNyaHai8k55
+ZfdQha9Aj3nPrXLQFGrbm+dEizcgaL/RKyIJYb2teRW7CUm5uEv4FCPWZQKBgQCJ
+YtVyliOXt5Hi+fGAom9vIrObE5ItvEAlFhqi91GlyQKQPW1wlf0Odl4n9snQ3y6z
+qIzxQl0tcHO3mYVfqNefqzbQa4K/q6U5kXoQINPGGTop1hJUbRDQxIAXrxd187Aw
+01B8TzgT8HLHShZ2zzSBSQftaSl4UcOAgK8XRriS3wKBgQDav8s2vEzedic608My
+sORIRNvNdtOTl3bDpKt2ho9yR3no3zGwGga45O2uD+MIo8VbddXhrOtB5VATF0Ar
+YowZmXULv2nqZFV5vr5btXD1NF31YMnqXHCRqyxUvldIGzGeCaDW1SVZp8VoDyTz
+jvKjQamR7uMxgVFxa3O2Si1fCQ==
+-----END PRIVATE KEY-----