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 <[email protected]> Tested-by: Internal Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/a65864a4 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/a65864a4 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/a65864a4 Branch: refs/heads/hadoop-next Commit: a65864a40ddc73aebeaf04abb5c5ca6fa70ba0ee Parents: 5349993 Author: Sailesh Mukil <[email protected]> Authored: Tue Dec 8 13:07:49 2015 -0800 Committer: Internal Jenkins <[email protected]> Committed: Wed Dec 7 04:25:21 2016 +0000 ---------------------------------------------------------------------- be/src/rpc/thrift-client.cc | 6 ++++++ be/src/rpc/thrift-client.h | 1 + be/src/rpc/thrift-server-test.cc | 25 +++++++++++++++++++++++++ be/src/runtime/client-cache.h | 2 +- be/src/runtime/data-stream-test.cc | 2 +- be/src/testutil/bad-cert.pem | 22 ++++++++++++++++++++++ be/src/testutil/bad-key.pem | 28 ++++++++++++++++++++++++++++ 7 files changed, 84 insertions(+), 2 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a65864a4/be/src/rpc/thrift-client.cc ---------------------------------------------------------------------- diff --git a/be/src/rpc/thrift-client.cc b/be/src/rpc/thrift-client.cc index bfe54fc..e0b9a6f 100644 --- a/be/src/rpc/thrift-client.cc +++ b/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())); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a65864a4/be/src/rpc/thrift-client.h ---------------------------------------------------------------------- diff --git a/be/src/rpc/thrift-client.h b/be/src/rpc/thrift-client.h index a194551..26982a1 100644 --- a/be/src/rpc/thrift-client.h +++ b/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. http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a65864a4/be/src/rpc/thrift-server-test.cc ---------------------------------------------------------------------- diff --git a/be/src/rpc/thrift-server-test.cc b/be/src/rpc/thrift-server-test.cc index 244097d..7be286e 100644 --- a/be/src/rpc/thrift-server-test.cc +++ b/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(); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a65864a4/be/src/runtime/client-cache.h ---------------------------------------------------------------------- diff --git a/be/src/runtime/client-cache.h b/be/src/runtime/client-cache.h index e57b766..8f97b67 100644 --- a/be/src/runtime/client-cache.h +++ b/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 http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a65864a4/be/src/runtime/data-stream-test.cc ---------------------------------------------------------------------- diff --git a/be/src/runtime/data-stream-test.cc b/be/src/runtime/data-stream-test.cc index f5eb783..d53a146 100644 --- a/be/src/runtime/data-stream-test.cc +++ b/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); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a65864a4/be/src/testutil/bad-cert.pem ---------------------------------------------------------------------- diff --git a/be/src/testutil/bad-cert.pem b/be/src/testutil/bad-cert.pem new file mode 100644 index 0000000..e656963 --- /dev/null +++ b/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----- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/a65864a4/be/src/testutil/bad-key.pem ---------------------------------------------------------------------- diff --git a/be/src/testutil/bad-key.pem b/be/src/testutil/bad-key.pem new file mode 100644 index 0000000..0db3da9 --- /dev/null +++ b/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-----
