You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2016/12/15 23:00:31 UTC
[05/50] [abbrv] incubator-impala git commit: IMPALA-2864: Ensure that
client connections are closed after a failed Open()
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 <sa...@cloudera.com>
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 <sa...@cloudera.com>
Authored: Tue Dec 8 13:07:49 2015 -0800
Committer: Internal Jenkins <cl...@gerrit.cloudera.org>
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-----