IMPALA-5696: Enable cipher configuration when using TLS / Thrift

The 'cipher suite' is a description of the set of algorithms used by SSL
and TLS to execute key exchange, encryption, message authentication, and
random number generation functions. SSL implementations allow the cipher
suite to be configured so that ciphers may be removed from the whitelist
if they are shown to be weak.

* Add a flag --ssl_cipher_list which controls cipher selection for both
  thrift servers and clients. Default is blank, which means use all
  available cipher suites.
* Add ThriftServerBuilder to simplify construction of
  ThriftServers (whose constructors were otherwise getting very long).

Testing: new tests added to thrift-server-test. Test cases added follow:

* A client cannot connect to a server which does not have any ciphers in
  common with it.
* If ciphers are identical on clients and servers, that ssl connections
  can be made.
* Bad cipher strings lead to errors on both client and server.

Change-Id: I735ae36eebfdf7228f235686c9c69642c3c9d84f
Reviewed-on: http://gerrit.cloudera.org:8080/7524
Reviewed-by: Henry Robinson <he...@cloudera.com>
Tested-by: Impala Public 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/68df21b4
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/68df21b4
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/68df21b4

Branch: refs/heads/master
Commit: 68df21b426feca8e7a458152d8dca1b7e1335bcb
Parents: d61065d
Author: Henry Robinson <he...@cloudera.com>
Authored: Fri Jul 21 14:46:13 2017 -0700
Committer: Impala Public Jenkins <impala-public-jenk...@gerrit.cloudera.org>
Committed: Tue Aug 8 10:33:31 2017 +0000

----------------------------------------------------------------------
 be/src/benchmarks/network-perf-benchmark.cc |   6 +-
 be/src/catalog/catalogd-main.cc             |  12 +-
 be/src/rpc/thrift-client.cc                 |   2 +
 be/src/rpc/thrift-server-test.cc            | 217 ++++++++++++++++++-----
 be/src/rpc/thrift-server.cc                 |   5 +-
 be/src/rpc/thrift-server.h                  | 149 +++++++++++++---
 be/src/runtime/data-stream-test.cc          |   2 +-
 be/src/service/impala-server.cc             |  52 ++++--
 be/src/statestore/statestore-subscriber.cc  |  16 +-
 be/src/statestore/statestored-main.cc       |  11 +-
 be/src/testutil/in-process-servers.cc       |   9 +-
 be/src/testutil/scoped-flag-setter.h        |  52 ++++++
 be/src/util/webserver-test.cc               |  50 ++----
 13 files changed, 440 insertions(+), 143 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/68df21b4/be/src/benchmarks/network-perf-benchmark.cc
----------------------------------------------------------------------
diff --git a/be/src/benchmarks/network-perf-benchmark.cc 
b/be/src/benchmarks/network-perf-benchmark.cc
index 26b2aae..6cebaaf 100644
--- a/be/src/benchmarks/network-perf-benchmark.cc
+++ b/be/src/benchmarks/network-perf-benchmark.cc
@@ -221,8 +221,10 @@ int main(int argc, char** argv) {
   boost::shared_ptr<ThreadFactory> thread_factory(
       new ThriftThreadFactory("test", "test"));
   boost::shared_ptr<TProcessor> processor(new 
NetworkTestServiceProcessor(handler));
-  ThriftServer* server = new ThriftServer("Network Test Server", processor,
-      FLAGS_port, NULL, NULL, 100, ThriftServer::ThreadPool);
+  ThriftServer* server;
+  ABORT_IF_ERROR(ThriftServerBuilder("Network Test Server", processor, 
FLAGS_port)
+                     .thread_pool(100)
+                     .Build(&server));
   thread* server_thread = new thread(&TestServer::Server, handler.get(), 
server);
 
   string input;

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/68df21b4/be/src/catalog/catalogd-main.cc
----------------------------------------------------------------------
diff --git a/be/src/catalog/catalogd-main.cc b/be/src/catalog/catalogd-main.cc
index ae1cdf6..4847920 100644
--- a/be/src/catalog/catalogd-main.cc
+++ b/be/src/catalog/catalogd-main.cc
@@ -47,6 +47,7 @@ DECLARE_int32(state_store_subscriber_port);
 DECLARE_string(ssl_server_certificate);
 DECLARE_string(ssl_private_key);
 DECLARE_string(ssl_private_key_password_cmd);
+DECLARE_string(ssl_cipher_list);
 
 #include "common/names.h"
 
@@ -89,13 +90,16 @@ int CatalogdMain(int argc, char** argv) {
       new RpcEventHandler("catalog-server", metrics.get()));
   processor->setEventHandler(event_handler);
 
-  ThriftServer* server = new ThriftServer("CatalogService", processor,
-      FLAGS_catalog_service_port, NULL, metrics.get(), 5);
+  ThriftServer* server;
+  ThriftServerBuilder builder("CatalogService", processor, 
FLAGS_catalog_service_port);
+
   if (EnableInternalSslConnections()) {
     LOG(INFO) << "Enabling SSL for CatalogService";
-    ABORT_IF_ERROR(server->EnableSsl(FLAGS_ssl_server_certificate, 
FLAGS_ssl_private_key,
-        FLAGS_ssl_private_key_password_cmd));
+    builder.ssl(FLAGS_ssl_server_certificate, FLAGS_ssl_private_key)
+        .pem_password_cmd(FLAGS_ssl_private_key_password_cmd)
+        .cipher_list(FLAGS_ssl_cipher_list);
   }
+  ABORT_IF_ERROR(builder.metrics(metrics.get()).Build(&server));
   ABORT_IF_ERROR(server->Start());
   LOG(INFO) << "CatalogService started on port: " << 
FLAGS_catalog_service_port;
   server->Join();

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/68df21b4/be/src/rpc/thrift-client.cc
----------------------------------------------------------------------
diff --git a/be/src/rpc/thrift-client.cc b/be/src/rpc/thrift-client.cc
index e0b9a6f..6f01073 100644
--- a/be/src/rpc/thrift-client.cc
+++ b/be/src/rpc/thrift-client.cc
@@ -32,6 +32,7 @@ using namespace apache::thrift;
 using namespace strings;
 
 DECLARE_string(ssl_client_ca_certificate);
+DECLARE_string(ssl_cipher_list);
 
 namespace impala {
 
@@ -100,6 +101,7 @@ Status ThriftClientImpl::CreateSocket() {
     socket_.reset(new TSocket(address_.hostname, address_.port));
   } else {
     try {
+      if (!FLAGS_ssl_cipher_list.empty()) 
ssl_factory_->ciphers(FLAGS_ssl_cipher_list);
       
ssl_factory_->loadTrustedCertificates(FLAGS_ssl_client_ca_certificate.c_str());
       socket_ = ssl_factory_->createSocket(address_.hostname, address_.port);
     } catch (const TException& e) {

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/68df21b4/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 f6edfb2..f7a2916 100644
--- a/be/src/rpc/thrift-server-test.cc
+++ b/be/src/rpc/thrift-server-test.cc
@@ -17,12 +17,13 @@
 
 #include <string>
 
-#include "testutil/gtest-util.h"
+#include "gen-cpp/StatestoreService.h"
+#include "gutil/strings/substitute.h"
 #include "rpc/thrift-client.h"
 #include "service/fe-support.h"
 #include "service/impala-server.h"
-#include "gen-cpp/StatestoreService.h"
-#include "gutil/strings/substitute.h"
+#include "testutil/gtest-util.h"
+#include "testutil/scoped-flag-setter.h"
 
 #include "common/names.h"
 
@@ -31,6 +32,7 @@ using namespace strings;
 using namespace apache::thrift;
 
 DECLARE_string(ssl_client_ca_certificate);
+DECLARE_string(ssl_cipher_list);
 
 DECLARE_int32(state_store_port);
 
@@ -71,12 +73,12 @@ int GetServerPort() {
 
 TEST(ThriftServer, Connectivity) {
   int port = GetServerPort();
-  ThriftClient<StatestoreServiceClientWrapper> wrong_port_client("localhost",
-      port, "", NULL, false);
+  ThriftClient<StatestoreServiceClientWrapper> wrong_port_client(
+      "localhost", port, "", nullptr, false);
   ASSERT_FALSE(wrong_port_client.Open().ok());
 
-  ThriftServer* server =
-      new ThriftServer("DummyStatestore", MakeProcessor(), port, NULL, NULL, 
5);
+  ThriftServer* server;
+  EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), 
port).Build(&server));
   ASSERT_OK(server->Start());
 
   // Test that client recovers from failure to connect.
@@ -89,14 +91,15 @@ TEST(SslTest, Connectivity) {
   // client cannot.
   // Here and elsewhere - allocate ThriftServers on the heap to avoid race 
during
   // destruction. See IMPALA-2283.
-  ThriftServer* server = new ThriftServer("DummyStatestore", MakeProcessor(), 
port, NULL,
-      NULL, 5);
-  ASSERT_OK(server->EnableSsl(SERVER_CERT, PRIVATE_KEY, "echo password"));
+  ThriftServer* server;
+  EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
+                .ssl(SERVER_CERT, PRIVATE_KEY)
+                .Build(&server));
   ASSERT_OK(server->Start());
 
   FLAGS_ssl_client_ca_certificate = SERVER_CERT;
   ThriftClient<StatestoreServiceClientWrapper> ssl_client(
-      "localhost", port, "", NULL, true);
+      "localhost", port, "", nullptr, true);
   ASSERT_OK(ssl_client.Open());
   TRegisterSubscriberResponse resp;
   bool send_done = false;
@@ -106,7 +109,7 @@ TEST(SslTest, Connectivity) {
 
   // Disable SSL for this client.
   ThriftClient<StatestoreServiceClientWrapper> non_ssl_client(
-      "localhost", port, "", NULL, false);
+      "localhost", port, "", nullptr, false);
   ASSERT_OK(non_ssl_client.Open());
   send_done = false;
   EXPECT_THROW(non_ssl_client.iface()->RegisterSubscriber(
@@ -116,13 +119,14 @@ TEST(SslTest, Connectivity) {
 TEST(SslTest, BadCertificate) {
   FLAGS_ssl_client_ca_certificate = "unknown";
   int port = GetServerPort();
-  ThriftClient<StatestoreServiceClientWrapper>
-      ssl_client("localhost", port, "", NULL, true);
+  ThriftClient<StatestoreServiceClientWrapper> ssl_client(
+      "localhost", port, "", nullptr, true);
   ASSERT_FALSE(ssl_client.Open().ok());
 
-  ThriftServer* server =
-      new ThriftServer("DummyStatestore", MakeProcessor(), port, NULL, NULL, 
5);
-  ASSERT_OK(server->EnableSsl(SERVER_CERT, PRIVATE_KEY, "echo password"));
+  ThriftServer* server;
+  EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
+                .ssl(SERVER_CERT, PRIVATE_KEY)
+                .Build(&server));
   ASSERT_OK(server->Start());
 
   // Check that client does not recover from failure to create socket.
@@ -133,14 +137,16 @@ TEST(PasswordProtectedPemFile, CorrectOperation) {
   // Require the server to execute a shell command to read the password to the 
private key
   // file.
   int port = GetServerPort();
-  ThriftServer* server = new ThriftServer("DummyStatestore", MakeProcessor(), 
port, NULL,
-      NULL, 5);
-  ASSERT_OK(server->EnableSsl(
-      SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY, "echo password"));
+  ThriftServer* server;
+  EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
+                .ssl(SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY)
+                .pem_password_cmd("echo password")
+                .Build(&server));
   ASSERT_OK(server->Start());
-  FLAGS_ssl_client_ca_certificate = SERVER_CERT;
-  ThriftClient<StatestoreServiceClientWrapper>
-      ssl_client("localhost", port, "", NULL, true);
+
+  auto s = ScopedFlagSetter<string>::Make(&FLAGS_ssl_client_ca_certificate, 
SERVER_CERT);
+  ThriftClient<StatestoreServiceClientWrapper> ssl_client(
+      "localhost", port, "", nullptr, true);
   ASSERT_OK(ssl_client.Open());
   TRegisterSubscriberResponse resp;
   bool send_done = false;
@@ -150,28 +156,38 @@ TEST(PasswordProtectedPemFile, CorrectOperation) {
 
 TEST(PasswordProtectedPemFile, BadPassword) {
   // Test failure when password to private key is wrong.
-  ThriftServer server("DummyStatestore", MakeProcessor(), GetServerPort(), 
NULL, NULL, 5);
-  ASSERT_OK(server.EnableSsl(
-      SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY, "echo wrongpassword"));
-  EXPECT_FALSE(server.Start().ok());
+  ThriftServer* server;
+  EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), 
GetServerPort())
+                .ssl(SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY)
+                .pem_password_cmd("echo wrongpassword")
+                .Build(&server));
+  EXPECT_FALSE(server->Start().ok());
 }
 
 TEST(PasswordProtectedPemFile, BadCommand) {
   // Test failure when password command is badly formed.
-  ThriftServer server("DummyStatestore", MakeProcessor(), GetServerPort(), 
NULL, NULL, 5);
-  EXPECT_FALSE(server.EnableSsl(
-      SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY, "cmd-no-exist").ok());
+  ThriftServer* server;
+
+  // Keep clang-tdy happy - NOLINT (which here is due to deliberately leaked 
'server')
+  // does not get pushed into EXPECT_ERROR.
+  Status s = ThriftServerBuilder("DummyStatestore", MakeProcessor(), 
GetServerPort()) // NOLINT
+      .ssl(SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY)
+      .pem_password_cmd("cmd-no-exist")
+      .Build(&server);
+  EXPECT_ERROR(s, TErrorCode::SSL_PASSWORD_CMD_FAILED);
 }
 
 TEST(SslTest, ClientBeforeServer) {
   // Instantiate a thrift client before a thrift server and test if it works 
(IMPALA-2747)
-  FLAGS_ssl_client_ca_certificate = SERVER_CERT;
+  auto s = ScopedFlagSetter<string>::Make(&FLAGS_ssl_client_ca_certificate, 
SERVER_CERT);
   int port = GetServerPort();
-  ThriftClient<StatestoreServiceClientWrapper>
-      ssl_client("localhost", port, "", NULL, true);
-  ThriftServer* server =
-      new ThriftServer("DummyStatestore", MakeProcessor(), port, NULL, NULL, 
5);
-  ASSERT_OK(server->EnableSsl(SERVER_CERT, PRIVATE_KEY));
+  ThriftClient<StatestoreServiceClientWrapper> ssl_client(
+      "localhost", port, "", nullptr, true);
+
+  ThriftServer* server;
+  EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
+                .ssl(SERVER_CERT, PRIVATE_KEY)
+                .Build(&server));
   ASSERT_OK(server->Start());
 
   ASSERT_OK(ssl_client.Open());
@@ -180,6 +196,113 @@ TEST(SslTest, ClientBeforeServer) {
   ssl_client.iface()->RegisterSubscriber(resp, TRegisterSubscriberRequest(), 
&send_done);
 }
 
+TEST(SslTest, BadCiphers) {
+  int port = GetServerPort();
+  {
+    ThriftServer* server;
+    EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
+                  .ssl(SERVER_CERT, PRIVATE_KEY)
+                  .cipher_list("this_is_not_a_cipher")
+                  .Build(&server));
+    EXPECT_FALSE(server->Start().ok());
+  }
+
+  {
+    ThriftServer* server;
+    EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
+                  .ssl(SERVER_CERT, PRIVATE_KEY)
+                  .Build(&server));
+    EXPECT_OK(server->Start());
+
+    auto s1 =
+        ScopedFlagSetter<string>::Make(&FLAGS_ssl_cipher_list, 
"this_is_not_a_cipher");
+    auto s2 =
+        ScopedFlagSetter<string>::Make(&FLAGS_ssl_client_ca_certificate, 
SERVER_CERT);
+
+    ThriftClient<StatestoreServiceClientWrapper> ssl_client(
+        "localhost", port, "", nullptr, true);
+    EXPECT_FALSE(ssl_client.Open().ok());
+  }
+}
+
+TEST(SslTest, MismatchedCiphers) {
+  int port = GetServerPort();
+  FLAGS_ssl_client_ca_certificate = SERVER_CERT;
+
+  ThriftServer* server;
+  EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
+                .ssl(SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY)
+                .pem_password_cmd("echo password")
+                .cipher_list("AES256-SHA256")
+                .Build(&server));
+  EXPECT_OK(server->Start());
+
+  auto s = ScopedFlagSetter<string>::Make(&FLAGS_ssl_cipher_list, "RC4-SHA");
+  ThriftClient<StatestoreServiceClientWrapper> ssl_client(
+      "localhost", port, "", nullptr, true);
+
+  // Failure to negotiate a cipher will show up when data is sent, not when 
socket is
+  // opened.
+  EXPECT_OK(ssl_client.Open());
+
+  bool send_done = false;
+  TRegisterSubscriberResponse resp;
+  EXPECT_THROW(ssl_client.iface()->RegisterSubscriber(
+                   resp, TRegisterSubscriberRequest(), &send_done),
+      TTransportException);
+}
+
+TEST(SslTest, MatchedCiphers) {
+  int port = GetServerPort();
+  ThriftServer* server;
+  EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
+                .ssl(SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY)
+                .pem_password_cmd("echo password")
+                .cipher_list("AES256-SHA256")
+                .Build(&server));
+  EXPECT_OK(server->Start());
+
+  FLAGS_ssl_client_ca_certificate = SERVER_CERT;
+  auto s = ScopedFlagSetter<string>::Make(&FLAGS_ssl_cipher_list, 
"AES256-SHA256");
+  ThriftClient<StatestoreServiceClientWrapper> ssl_client(
+      "localhost", port, "", nullptr, true);
+
+  EXPECT_OK(ssl_client.Open());
+
+  bool send_done = false;
+  TRegisterSubscriberResponse resp;
+  EXPECT_NO_THROW({
+    ssl_client.iface()->RegisterSubscriber(
+        resp, TRegisterSubscriberRequest(), &send_done);
+  });
+}
+
+TEST(SslTest, OverlappingMatchedCiphers) {
+  int port = GetServerPort();
+  ThriftServer* server;
+  EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
+      .ssl(SERVER_CERT, PASSWORD_PROTECTED_PRIVATE_KEY)
+      .pem_password_cmd("echo password")
+      .cipher_list("RC4-SHA,AES256-SHA256")
+      .Build(&server));
+  EXPECT_OK(server->Start());
+
+  FLAGS_ssl_client_ca_certificate = SERVER_CERT;
+  auto s = ScopedFlagSetter<string>::Make(&FLAGS_ssl_cipher_list,
+      "AES256-SHA256,not-a-cipher");
+  ThriftClient<StatestoreServiceClientWrapper> ssl_client(
+      "localhost", port, "", nullptr, true);
+
+  EXPECT_OK(ssl_client.Open());
+
+  bool send_done = false;
+  TRegisterSubscriberResponse resp;
+  EXPECT_NO_THROW({
+        ssl_client.iface()->RegisterSubscriber(
+            resp, TRegisterSubscriberRequest(), &send_done);
+      });
+}
+
 /// Test disabled because requires a high ulimit -n on build machines. Since 
the test does
 /// not always fail, we don't lose much coverage by disabling it until we fix 
the build
 /// infra issue.
@@ -189,13 +312,14 @@ TEST(ConcurrencyTest, DISABLED_ManyConcurrentConnections) 
{
   // Note that without the fix for IMPALA-4135, this test won't always fail, 
depending on
   // the hardware that it is run on.
   int port = GetServerPort();
-  ThriftServer* server = new ThriftServer("DummyServer", MakeProcessor(), 
port);
+  ThriftServer* server;
+  EXPECT_OK(ThriftServerBuilder("DummyServer", MakeProcessor(), 
port).Build(&server));
   ASSERT_OK(server->Start());
 
   ThreadPool<int64_t> pool(
       "group", "test", 256, 10000, [port](int tid, const int64_t& item) {
         using Client = ThriftClient<ImpalaInternalServiceClient>;
-        Client* client = new Client("127.0.0.1", port, "", NULL, false);
+        Client* client = new Client("127.0.0.1", port, "", nullptr, false);
         Status status = client->Open();
         ASSERT_OK(status);
       });
@@ -204,13 +328,16 @@ TEST(ConcurrencyTest, DISABLED_ManyConcurrentConnections) 
{
 }
 
 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;
+  int port = GetServerPort();
+  ThriftServer* server;
+  EXPECT_OK(ThriftServerBuilder("DummyStatestore", MakeProcessor(), port)
+                .ssl(BAD_SERVER_CERT, BAD_PRIVATE_KEY)
+                .Build(&server));
+  ASSERT_OK(server->Start());
+
+  auto s = ScopedFlagSetter<string>::Make(&FLAGS_ssl_client_ca_certificate, 
SERVER_CERT);
   ThriftClient<StatestoreServiceClientWrapper> ssl_client(
-      "localhost", FLAGS_state_store_port + 5, "", NULL, true);
+      "localhost", port, "", nullptr, true);
   EXPECT_OK(ssl_client.Open());
   TRegisterSubscriberResponse resp;
   bool send_done = false;

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/68df21b4/be/src/rpc/thrift-server.cc
----------------------------------------------------------------------
diff --git a/be/src/rpc/thrift-server.cc b/be/src/rpc/thrift-server.cc
index a0a86a2..e1e9a7f 100644
--- a/be/src/rpc/thrift-server.cc
+++ b/be/src/rpc/thrift-server.cc
@@ -66,6 +66,7 @@ DECLARE_string(principal);
 DECLARE_string(keytab_file);
 DECLARE_string(ssl_client_ca_certificate);
 DECLARE_string(ssl_server_certificate);
+DECLARE_string(ssl_cipher_list);
 
 namespace impala {
 
@@ -352,6 +353,7 @@ Status 
ThriftServer::CreateSocket(boost::shared_ptr<TServerTransport>* socket) {
         new ImpalaSslSocketFactory(key_password_));
     socket_factory->overrideDefaultPasswordCallback();
     try {
+      if (!cipher_list_.empty()) socket_factory->ciphers(cipher_list_);
       socket_factory->loadCertificate(certificate_path_.c_str());
       socket_factory->loadPrivateKey(private_key_path_.c_str());
       socket->reset(new TSSLServerSocket(port_, socket_factory));
@@ -366,7 +368,7 @@ Status 
ThriftServer::CreateSocket(boost::shared_ptr<TServerTransport>* socket) {
 }
 
 Status ThriftServer::EnableSsl(const string& certificate, const string& 
private_key,
-    const string& pem_password_cmd) {
+    const string& pem_password_cmd, const std::string& ciphers) {
   DCHECK(!started_);
   if (certificate.empty()) return 
Status(TErrorCode::SSL_CERTIFICATE_PATH_BLANK);
   if (private_key.empty()) return 
Status(TErrorCode::SSL_PRIVATE_KEY_PATH_BLANK);
@@ -383,6 +385,7 @@ Status ThriftServer::EnableSsl(const string& certificate, 
const string& private_
   ssl_enabled_ = true;
   certificate_path_ = certificate;
   private_key_path_ = private_key;
+  cipher_list_ = ciphers;
 
   if (!pem_password_cmd.empty()) {
     if (!RunShellProcess(pem_password_cmd, &key_password_, true)) {

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/68df21b4/be/src/rpc/thrift-server.h
----------------------------------------------------------------------
diff --git a/be/src/rpc/thrift-server.h b/be/src/rpc/thrift-server.h
index 5b85134..e52edea 100644
--- a/be/src/rpc/thrift-server.h
+++ b/be/src/rpc/thrift-server.h
@@ -15,7 +15,6 @@
 // specific language governing permissions and limitations
 // under the License.
 
-
 #ifndef IMPALA_RPC_THRIFT_SERVER_H
 #define IMPALA_RPC_THRIFT_SERVER_H
 
@@ -37,8 +36,11 @@ namespace impala {
 /// Utility class for all Thrift servers. Runs a threaded server by default, 
or a
 /// TThreadPoolServer with, by default, 2 worker threads, that exposes the 
interface
 /// described by a user-supplied TProcessor object.
+///
+/// Use a ThriftServerBuilder to construct a ThriftServer. ThriftServer's 
c'tors are
+/// private.
+///
 /// If TThreadPoolServer is used, client must use TSocket as transport.
-/// TODO: Need a builder to help with the unwieldy constructor
 /// TODO: shutdown is buggy (which only harms tests)
 class ThriftServer {
  public:
@@ -95,30 +97,6 @@ class ThriftServer {
   /// Threaded    -- Allocates 1 thread per connection, as needed.
   enum ServerType { ThreadPool = 0, Threaded };
 
-  /// Creates, but does not start, a new server on the specified port
-  /// that exports the supplied interface.
-  ///  - name: human-readable name of this server. Should not contain spaces
-  ///  - processor: Thrift processor to handle RPCs
-  ///  - port: The port the server will listen for connections on
-  ///  - auth_provider: Authentication scheme to use. If NULL, use the global 
default
-  ///    demon<->demon provider.
-  ///  - metrics: if not NULL, the server will register metrics on this object
-  ///  - num_worker_threads: the number of worker threads to use in any thread 
pool
-  ///  - server_type: the type of IO strategy this server should employ
-  ThriftServer(const std::string& name,
-      const boost::shared_ptr<apache::thrift::TProcessor>& processor, int port,
-      AuthProvider* auth_provider = NULL, MetricGroup* metrics = NULL,
-      int num_worker_threads = DEFAULT_WORKER_THREADS, ServerType server_type 
= Threaded);
-
-  /// Enables secure access over SSL. Must be called before Start(). The first 
two
-  /// arguments are paths to certificate and private key files in .PEM format,
-  /// respectively. If either file does not exist, an error is returned. The 
final
-  /// optional argument provides the command to run if a password is required 
to decrypt
-  /// the private key. It is invoked once, and the resulting password is used 
only for
-  /// password-protected .PEM files.
-  Status EnableSsl(const std::string& certificate, const std::string& 
private_key,
-      const std::string& pem_password_cmd = "");
-
   int port() const { return port_; }
 
   bool ssl_enabled() const { return ssl_enabled_; }
@@ -161,6 +139,32 @@ class ThriftServer {
   static const ConnectionContext* GetThreadConnectionContext();
 
  private:
+  friend class ThriftServerBuilder;
+
+  /// Creates, but does not start, a new server on the specified port
+  /// that exports the supplied interface.
+  ///  - name: human-readable name of this server. Should not contain spaces
+  ///  - processor: Thrift processor to handle RPCs
+  ///  - port: The port the server will listen for connections on
+  ///  - auth_provider: Authentication scheme to use. If nullptr, use the 
global default
+  ///    demon<->demon provider.
+  ///  - metrics: if not nullptr, the server will register metrics on this 
object
+  ///  - num_worker_threads: the number of worker threads to use in any thread 
pool
+  ///  - server_type: the type of IO strategy this server should employ
+  ThriftServer(const std::string& name,
+      const boost::shared_ptr<apache::thrift::TProcessor>& processor, int port,
+      AuthProvider* auth_provider = nullptr, MetricGroup* metrics = nullptr,
+      int num_worker_threads = DEFAULT_WORKER_THREADS, ServerType server_type 
= Threaded);
+
+  /// Enables secure access over SSL. Must be called before Start(). The first 
two
+  /// arguments are paths to certificate and private key files in .PEM format,
+  /// respectively. If either file does not exist, an error is returned. The 
final
+  /// optional argument provides the command to run if a password is required 
to decrypt
+  /// the private key. It is invoked once, and the resulting password is used 
only for
+  /// password-protected .PEM files.
+  Status EnableSsl(const std::string& certificate, const std::string& 
private_key,
+      const std::string& pem_password_cmd = "", const std::string& ciphers = 
"");
+
   /// Creates the server socket on which this server listens. May be SSL 
enabled. Returns
   /// OK unless there was a Thrift error.
   Status CreateSocket(
@@ -184,6 +188,9 @@ class ThriftServer {
   /// Password string retrieved by running command in EnableSsl().
   std::string key_password_;
 
+  /// List of ciphers that are ok for clients to use when connecting.
+  std::string cipher_list_;
+
   /// How many worker threads to use to serve incoming requests
   /// (requests are queued if no thread is immediately available)
   int num_worker_threads_;
@@ -201,7 +208,7 @@ class ThriftServer {
   boost::scoped_ptr<apache::thrift::server::TServer> server_;
   boost::shared_ptr<apache::thrift::TProcessor> processor_;
 
-  /// If not NULL, called when connection events happen. Not owned by us.
+  /// If not nullptr, called when connection events happen. Not owned by us.
   ConnectionHandlerIf* connection_handler_;
 
   /// Protects connection_contexts_
@@ -237,6 +244,94 @@ class ThriftServer {
   friend class ThriftServerEventProcessor;
 };
 
+/// Helper class to build new ThriftServer instances.
+class ThriftServerBuilder {
+ public:
+  ThriftServerBuilder(const std::string& name,
+      const boost::shared_ptr<apache::thrift::TProcessor>& processor, int port)
+    : name_(name), processor_(processor), port_(port) {}
+
+  /// Sets the auth provider for this server. Default is the system global 
auth provider.
+  ThriftServerBuilder& auth_provider(AuthProvider* provider) {
+    auth_provider_ = provider;
+    return *this;
+  }
+
+  /// Sets the metrics instance that this server should register metrics with. 
Default is
+  /// nullptr.
+  ThriftServerBuilder& metrics(MetricGroup* metrics) {
+    metrics_ = metrics;
+    return *this;
+  }
+
+  /// Make this server a thread-pool server with 'num_worker_threads' threads.
+  ThriftServerBuilder& thread_pool(int num_worker_threads) {
+    server_type_ = ThriftServer::ServerType::ThreadPool;
+    num_worker_threads_ = num_worker_threads;
+    return *this;
+  }
+
+  /// Make this server a threaded server (i.e. one thread per connection).
+  ThriftServerBuilder& threaded() {
+    server_type_ = ThriftServer::ServerType::Threaded;
+    return *this;
+  }
+
+  /// Enables SSL for this server.
+  ThriftServerBuilder& ssl(
+      const std::string& certificate, const std::string& private_key) {
+    enable_ssl_ = true;
+    certificate_ = certificate;
+    private_key_ = private_key;
+    return *this;
+  }
+
+  /// Sets the command used to compute the password for the SSL private key. 
Default is
+  /// empty, i.e. no password needed.
+  ThriftServerBuilder& pem_password_cmd(const std::string& pem_password_cmd) {
+    pem_password_cmd_ = pem_password_cmd;
+    return *this;
+  }
+
+  /// Sets the list of acceptable cipher suites for this server. Default is to 
use all
+  /// available system cipher suites.
+  ThriftServerBuilder& cipher_list(const std::string& ciphers) {
+    ciphers_ = ciphers;
+    return *this;
+  }
+
+  /// Constructs a new ThriftServer and puts it in 'server', if construction 
was
+  /// successful, returns an error otherwise. In the error case, 'server' will 
not have
+  /// been set and will not need to be freed, otherwise the caller assumes 
ownership of
+  /// '*server'.
+  Status Build(ThriftServer** server) {
+    std::unique_ptr<ThriftServer> ptr(new ThriftServer(name_, processor_, 
port_,
+        auth_provider_, metrics_, num_worker_threads_, server_type_));
+    if (enable_ssl_) {
+      RETURN_IF_ERROR(
+          ptr->EnableSsl(certificate_, private_key_, pem_password_cmd_, 
ciphers_));
+    }
+    (*server) = ptr.release();
+    return Status::OK();
+  }
+
+ private:
+  ThriftServer::ServerType server_type_ = ThriftServer::ServerType::Threaded;
+  int num_worker_threads_ = ThriftServer::DEFAULT_WORKER_THREADS;
+  std::string name_;
+  boost::shared_ptr<apache::thrift::TProcessor> processor_;
+  int port_ = 0;
+
+  AuthProvider* auth_provider_ = nullptr;
+  MetricGroup* metrics_ = nullptr;
+
+  bool enable_ssl_ = false;
+  std::string certificate_;
+  std::string private_key_;
+  std::string pem_password_cmd_;
+  std::string ciphers_;
+};
+
 // Returns true if, per the process configuration flags, server<->server 
communications
 // should use SSL.
 bool EnableInternalSslConnections();

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/68df21b4/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 a358286..990d8bb 100644
--- a/be/src/runtime/data-stream-test.cc
+++ b/be/src/runtime/data-stream-test.cc
@@ -454,7 +454,7 @@ class DataStreamTest : public testing::Test {
   void StartBackend() {
     boost::shared_ptr<ImpalaTestBackend> handler(new 
ImpalaTestBackend(stream_mgr_));
     boost::shared_ptr<TProcessor> processor(new 
ImpalaInternalServiceProcessor(handler));
-    server_ = new ThriftServer("DataStreamTest backend", processor, 
FLAGS_port, nullptr);
+    ThriftServerBuilder("DataStreamTest backend", processor, 
FLAGS_port).Build(&server_);
     server_->Start();
   }
 

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/68df21b4/be/src/service/impala-server.cc
----------------------------------------------------------------------
diff --git a/be/src/service/impala-server.cc b/be/src/service/impala-server.cc
index 4870091..92549cf 100644
--- a/be/src/service/impala-server.cc
+++ b/be/src/service/impala-server.cc
@@ -170,6 +170,13 @@ DEFINE_string(ssl_private_key_password_cmd, "", "A Unix 
command whose output ret
     "then all trailing whitespace will be trimmed before it is used to decrypt 
the "
     "private key");
 
+// TODO: For 3.0 (compatibility-breaking release), set this to a whitelist of 
ciphers,
+// e.g.  https://wiki.mozilla.org/Security/Server_Side_TLS
+DEFINE_string(ssl_cipher_list, "",
+    "The cipher suite preferences to use for TLS-secured "
+    "Thrift RPC connections. Uses the OpenSSL cipher preference list format. 
See man (1) "
+    "ciphers for more information. If empty, the default cipher list for your 
platform "
+    "is used");
 
 DEFINE_int32(idle_session_timeout, 0, "The time, in seconds, that a session 
may be idle"
     " for before it is closed (and all running queries cancelled) by Impala. 
If 0, idle"
@@ -1931,19 +1938,19 @@ Status CreateImpalaServer(ExecEnv* exec_env, int 
beeswax_port, int hs2_port, int
         new RpcEventHandler("backend", exec_env->metrics()));
     be_processor->setEventHandler(event_handler);
 
-    *be_server = new ThriftServer("backend", be_processor, be_port, nullptr,
-        exec_env->metrics());
+    ThriftServerBuilder be_builder("backend", be_processor, be_port);
+
     if (EnableInternalSslConnections()) {
       LOG(INFO) << "Enabling SSL for backend";
-      RETURN_IF_ERROR((*be_server)->EnableSsl(FLAGS_ssl_server_certificate,
-          FLAGS_ssl_private_key, FLAGS_ssl_private_key_password_cmd));
+      be_builder.ssl(FLAGS_ssl_server_certificate, FLAGS_ssl_private_key)
+          .pem_password_cmd(FLAGS_ssl_private_key_password_cmd)
+          .cipher_list(FLAGS_ssl_cipher_list);
     }
-
+    RETURN_IF_ERROR(be_builder.metrics(exec_env->metrics()).Build(be_server));
     LOG(INFO) << "ImpalaInternalService listening on " << be_port;
   }
 
   if (!FLAGS_is_coordinator) {
-
     LOG(INFO) << "Started executor Impala server on "
               << ExecEnv::GetInstance()->backend_address();
     return Status::OK();
@@ -1958,16 +1965,20 @@ Status CreateImpalaServer(ExecEnv* exec_env, int 
beeswax_port, int hs2_port, int
     boost::shared_ptr<TProcessorEventHandler> event_handler(
         new RpcEventHandler("beeswax", exec_env->metrics()));
     beeswax_processor->setEventHandler(event_handler);
-    *beeswax_server = new ThriftServer(BEESWAX_SERVER_NAME, beeswax_processor,
-        beeswax_port, AuthManager::GetInstance()->GetExternalAuthProvider(),
-        exec_env->metrics(), FLAGS_fe_service_threads, 
ThriftServer::ThreadPool);
+    ThriftServerBuilder builder(BEESWAX_SERVER_NAME, beeswax_processor, 
beeswax_port);
 
-    (*beeswax_server)->SetConnectionHandler(impala_server->get());
     if (!FLAGS_ssl_server_certificate.empty()) {
       LOG(INFO) << "Enabling SSL for Beeswax";
-      
RETURN_IF_ERROR((*beeswax_server)->EnableSsl(FLAGS_ssl_server_certificate,
-          FLAGS_ssl_private_key, FLAGS_ssl_private_key_password_cmd));
+      builder.ssl(FLAGS_ssl_server_certificate, FLAGS_ssl_private_key)
+          .pem_password_cmd(FLAGS_ssl_private_key_password_cmd)
+          .cipher_list(FLAGS_ssl_cipher_list);
     }
+    RETURN_IF_ERROR(
+        
builder.auth_provider(AuthManager::GetInstance()->GetExternalAuthProvider())
+            .metrics(exec_env->metrics())
+            .thread_pool(FLAGS_fe_service_threads)
+            .Build(beeswax_server));
+    (*beeswax_server)->SetConnectionHandler(impala_server->get());
 
     LOG(INFO) << "Impala Beeswax Service listening on " << beeswax_port;
   }
@@ -1980,17 +1991,22 @@ Status CreateImpalaServer(ExecEnv* exec_env, int 
beeswax_port, int hs2_port, int
         new RpcEventHandler("hs2", exec_env->metrics()));
     hs2_fe_processor->setEventHandler(event_handler);
 
-    *hs2_server = new ThriftServer(HS2_SERVER_NAME, hs2_fe_processor, hs2_port,
-        AuthManager::GetInstance()->GetExternalAuthProvider(), 
exec_env->metrics(),
-        FLAGS_fe_service_threads, ThriftServer::ThreadPool);
+    ThriftServerBuilder builder(HS2_SERVER_NAME, hs2_fe_processor, hs2_port);
 
-    (*hs2_server)->SetConnectionHandler(impala_server->get());
     if (!FLAGS_ssl_server_certificate.empty()) {
       LOG(INFO) << "Enabling SSL for HiveServer2";
-      RETURN_IF_ERROR((*hs2_server)->EnableSsl(FLAGS_ssl_server_certificate,
-          FLAGS_ssl_private_key, FLAGS_ssl_private_key_password_cmd));
+      builder.ssl(FLAGS_ssl_server_certificate, FLAGS_ssl_private_key)
+          .pem_password_cmd(FLAGS_ssl_private_key_password_cmd)
+          .cipher_list(FLAGS_ssl_cipher_list);
     }
 
+    RETURN_IF_ERROR(
+        
builder.auth_provider(AuthManager::GetInstance()->GetExternalAuthProvider())
+            .metrics(exec_env->metrics())
+            .thread_pool(FLAGS_fe_service_threads)
+            .Build(hs2_server));
+    (*hs2_server)->SetConnectionHandler(impala_server->get());
+
     LOG(INFO) << "Impala HiveServer2 Service listening on " << hs2_port;
   }
 

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/68df21b4/be/src/statestore/statestore-subscriber.cc
----------------------------------------------------------------------
diff --git a/be/src/statestore/statestore-subscriber.cc 
b/be/src/statestore/statestore-subscriber.cc
index 12efdcd..69ddfdc 100644
--- a/be/src/statestore/statestore-subscriber.cc
+++ b/be/src/statestore/statestore-subscriber.cc
@@ -47,11 +47,12 @@ DEFINE_int32(statestore_subscriber_cnxn_attempts, 10, "The 
number of times to re
     "RPC connection to the statestore. A setting of 0 means retry 
indefinitely");
 DEFINE_int32(statestore_subscriber_cnxn_retry_interval_ms, 3000, "The 
interval, in ms, "
     "to wait between attempts to make an RPC connection to the statestore.");
-DECLARE_string(ssl_client_ca_certificate);
 
+DECLARE_string(ssl_client_ca_certificate);
 DECLARE_string(ssl_server_certificate);
 DECLARE_string(ssl_private_key);
 DECLARE_string(ssl_private_key_password_cmd);
+DECLARE_string(ssl_cipher_list);
 
 namespace impala {
 
@@ -192,13 +193,18 @@ Status StatestoreSubscriber::Start() {
         new RpcEventHandler("statestore-subscriber", metrics_));
     processor->setEventHandler(event_handler);
 
-    heartbeat_server_.reset(new ThriftServer("StatestoreSubscriber", processor,
-        heartbeat_address_.port, NULL, NULL, 5));
+    ThriftServerBuilder builder(
+        "StatestoreSubscriber", processor, heartbeat_address_.port);
     if (EnableInternalSslConnections()) {
       LOG(INFO) << "Enabling SSL for Statestore subscriber";
-      
RETURN_IF_ERROR(heartbeat_server_->EnableSsl(FLAGS_ssl_server_certificate,
-          FLAGS_ssl_private_key, FLAGS_ssl_private_key_password_cmd));
+      builder.ssl(FLAGS_ssl_server_certificate, FLAGS_ssl_private_key)
+          .pem_password_cmd(FLAGS_ssl_private_key_password_cmd)
+          .cipher_list(FLAGS_ssl_cipher_list);
     }
+
+    ThriftServer* server;
+    RETURN_IF_ERROR(builder.Build(&server));
+    heartbeat_server_.reset(server);
     RETURN_IF_ERROR(heartbeat_server_->Start());
 
     LOG(INFO) << "Registering with statestore";

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/68df21b4/be/src/statestore/statestored-main.cc
----------------------------------------------------------------------
diff --git a/be/src/statestore/statestored-main.cc 
b/be/src/statestore/statestored-main.cc
index 40e2a73..f4c4672 100644
--- a/be/src/statestore/statestored-main.cc
+++ b/be/src/statestore/statestored-main.cc
@@ -42,6 +42,7 @@ DECLARE_string(principal);
 DECLARE_string(ssl_server_certificate);
 DECLARE_string(ssl_private_key);
 DECLARE_string(ssl_private_key_password_cmd);
+DECLARE_string(ssl_cipher_list);
 
 #include "common/names.h"
 
@@ -83,13 +84,15 @@ int StatestoredMain(int argc, char** argv) {
       new RpcEventHandler("statestore", metrics.get()));
   processor->setEventHandler(event_handler);
 
-  ThriftServer* server = new ThriftServer("StatestoreService", processor,
-      FLAGS_state_store_port, NULL, metrics.get(), 5);
+  ThriftServer* server;
+  ThriftServerBuilder builder("StatestoreService", processor, 
FLAGS_state_store_port);
   if (EnableInternalSslConnections()) {
     LOG(INFO) << "Enabling SSL for Statestore";
-    ABORT_IF_ERROR(server->EnableSsl(FLAGS_ssl_server_certificate, 
FLAGS_ssl_private_key,
-        FLAGS_ssl_private_key_password_cmd));
+    builder.ssl(FLAGS_ssl_server_certificate, FLAGS_ssl_private_key)
+        .pem_password_cmd(FLAGS_ssl_private_key_password_cmd)
+        .cipher_list(FLAGS_ssl_cipher_list);
   }
+  ABORT_IF_ERROR(builder.metrics(metrics.get()).Build(&server));
   ABORT_IF_ERROR(server->Start());
 
   statestore.MainLoop();

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/68df21b4/be/src/testutil/in-process-servers.cc
----------------------------------------------------------------------
diff --git a/be/src/testutil/in-process-servers.cc 
b/be/src/testutil/in-process-servers.cc
index 015e7e2..7a81915 100644
--- a/be/src/testutil/in-process-servers.cc
+++ b/be/src/testutil/in-process-servers.cc
@@ -162,13 +162,14 @@ Status InProcessStatestore::Start() {
   boost::shared_ptr<TProcessor> processor(
       new StatestoreServiceProcessor(statestore_->thrift_iface()));
 
-  statestore_server_.reset(new ThriftServer("StatestoreService", processor,
-      statestore_port_, NULL, metrics_.get(), 5));
+  ThriftServerBuilder builder("StatestoreService", processor, 
statestore_port_);
   if (EnableInternalSslConnections()) {
     LOG(INFO) << "Enabling SSL for Statestore";
-    ABORT_IF_ERROR(statestore_server_->EnableSsl(
-        FLAGS_ssl_server_certificate, FLAGS_ssl_private_key));
+    builder.ssl(FLAGS_ssl_server_certificate, FLAGS_ssl_private_key);
   }
+  ThriftServer* server;
+  ABORT_IF_ERROR(builder.metrics(metrics_.get()).Build(&server));
+  statestore_server_.reset(server);
   statestore_main_loop_.reset(
       new Thread("statestore", "main-loop", &Statestore::MainLoop, 
statestore_.get()));
 

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/68df21b4/be/src/testutil/scoped-flag-setter.h
----------------------------------------------------------------------
diff --git a/be/src/testutil/scoped-flag-setter.h 
b/be/src/testutil/scoped-flag-setter.h
new file mode 100644
index 0000000..49fa449
--- /dev/null
+++ b/be/src/testutil/scoped-flag-setter.h
@@ -0,0 +1,52 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+#ifndef IMPALA_TESTUTIL_SCOPED_FLAG_SETTER_H
+#define IMPALA_TESTUTIL_SCOPED_FLAG_SETTER_H
+
+namespace impala {
+
+/// Temporarily sets a flag for the duration of its scope, then resets the 
flag to its
+/// original value upon destruction.
+//
+/// Example (pre-condition: FLAGS_my_string_flag == "world"):
+/// {
+///   auto s = ScopedFlagSetter<string>::Make(&FLAGS_my_string_flag, "hello");
+///   // ... FLAGS_my_string_flag == "hello" for entire scope
+/// }
+/// // After destruction of 's', FLAGS_my_string_flag == "world" again.
+template <typename T>
+struct ScopedFlagSetter {
+  static ScopedFlagSetter<T> Make(T* f, const T& new_val) {
+    return ScopedFlagSetter(f, new_val);
+  }
+
+  ~ScopedFlagSetter() { *flag = old_val; }
+
+ private:
+  ScopedFlagSetter(T* f, T new_val) {
+    flag = f;
+    old_val = *f;
+    *f = new_val;
+  }
+
+  T* flag;
+  T old_val;
+};
+}
+
+#endif

http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/68df21b4/be/src/util/webserver-test.cc
----------------------------------------------------------------------
diff --git a/be/src/util/webserver-test.cc b/be/src/util/webserver-test.cc
index 7298ee8..1abbe42 100644
--- a/be/src/util/webserver-test.cc
+++ b/be/src/util/webserver-test.cc
@@ -21,10 +21,11 @@
 #include <boost/lexical_cast.hpp>
 #include <gutil/strings/substitute.h>
 
+#include "common/init.h"
 #include "testutil/gtest-util.h"
+#include "testutil/scoped-flag-setter.h"
 #include "util/webserver.h"
 #include "util/default-path-handlers.h"
-#include "common/init.h"
 
 DECLARE_int32(webserver_port);
 DECLARE_string(webserver_password_file);
@@ -201,25 +202,10 @@ TEST(Webserver, EscapeErrorUriTest) {
       string::npos);
 }
 
-template<typename T>
-struct ScopedFlagSetter {
-  T* flag;
-  T old_val;
-  ScopedFlagSetter(T* f, T new_val) {
-    flag = f;
-    old_val = *f;
-    *f = new_val;
-  }
-
-  ~ScopedFlagSetter() {
-    *flag = old_val;
-  }
-};
-
 TEST(Webserver, SslTest) {
-  ScopedFlagSetter<string> certificate(&FLAGS_webserver_certificate_file,
+  auto cert = ScopedFlagSetter<string>::Make(&FLAGS_webserver_certificate_file,
       Substitute("$0/be/src/testutil/server-cert.pem", getenv("IMPALA_HOME")));
-  ScopedFlagSetter<string> private_key(&FLAGS_webserver_private_key_file,
+  auto key = ScopedFlagSetter<string>::Make(&FLAGS_webserver_private_key_file,
       Substitute("$0/be/src/testutil/server-key.pem", getenv("IMPALA_HOME")));
 
   Webserver webserver(FLAGS_webserver_port);
@@ -227,9 +213,9 @@ TEST(Webserver, SslTest) {
 }
 
 TEST(Webserver, SslBadCertTest) {
-  ScopedFlagSetter<string> certificate(&FLAGS_webserver_certificate_file,
+  auto cert = ScopedFlagSetter<string>::Make(&FLAGS_webserver_certificate_file,
       Substitute("$0/be/src/testutil/invalid-server-cert.pem", 
getenv("IMPALA_HOME")));
-  ScopedFlagSetter<string> private_key(&FLAGS_webserver_private_key_file,
+  auto key = ScopedFlagSetter<string>::Make(&FLAGS_webserver_private_key_file,
       Substitute("$0/be/src/testutil/server-key.pem", getenv("IMPALA_HOME")));
 
   Webserver webserver(FLAGS_webserver_port);
@@ -237,11 +223,11 @@ TEST(Webserver, SslBadCertTest) {
 }
 
 TEST(Webserver, SslWithPrivateKeyPasswordTest) {
-  ScopedFlagSetter<string> certificate(&FLAGS_webserver_certificate_file,
+  auto cert = ScopedFlagSetter<string>::Make(&FLAGS_webserver_certificate_file,
       Substitute("$0/be/src/testutil/server-cert.pem", getenv("IMPALA_HOME")));
-  ScopedFlagSetter<string> private_key(&FLAGS_webserver_private_key_file,
+  auto key = ScopedFlagSetter<string>::Make(&FLAGS_webserver_private_key_file,
       Substitute("$0/be/src/testutil/server-key-password.pem", 
getenv("IMPALA_HOME")));
-  ScopedFlagSetter<string> password_cmd(
+  auto cmd = ScopedFlagSetter<string>::Make(
       &FLAGS_webserver_private_key_password_cmd, "echo password");
 
   Webserver webserver(FLAGS_webserver_port);
@@ -249,11 +235,11 @@ TEST(Webserver, SslWithPrivateKeyPasswordTest) {
 }
 
 TEST(Webserver, SslBadPrivateKeyPasswordTest) {
-  ScopedFlagSetter<string> certificate(&FLAGS_webserver_certificate_file,
+  auto cert = ScopedFlagSetter<string>::Make(&FLAGS_webserver_certificate_file,
       Substitute("$0/be/src/testutil/server-cert.pem", getenv("IMPALA_HOME")));
-  ScopedFlagSetter<string> private_key(&FLAGS_webserver_private_key_file,
+  auto key = ScopedFlagSetter<string>::Make(&FLAGS_webserver_private_key_file,
       Substitute("$0/be/src/testutil/server-key-password.pem", 
getenv("IMPALA_HOME")));
-  ScopedFlagSetter<string> password_cmd(
+  auto cmd = ScopedFlagSetter<string>::Make(
       &FLAGS_webserver_private_key_password_cmd, "echo wrongpassword");
 
   Webserver webserver(FLAGS_webserver_port);
@@ -263,8 +249,8 @@ TEST(Webserver, SslBadPrivateKeyPasswordTest) {
 TEST(Webserver, StartWithPasswordFileTest) {
   stringstream password_file;
   password_file << getenv("IMPALA_HOME") << "/be/src/testutil/htpasswd";
-  ScopedFlagSetter<string> password_flag(&FLAGS_webserver_password_file,
-      password_file.str());
+  auto password =
+      ScopedFlagSetter<string>::Make(&FLAGS_webserver_password_file, 
password_file.str());
 
   Webserver webserver(FLAGS_webserver_port);
   ASSERT_OK(webserver.Start());
@@ -277,8 +263,8 @@ TEST(Webserver, StartWithPasswordFileTest) {
 TEST(Webserver, StartWithMissingPasswordFileTest) {
   stringstream password_file;
   password_file << getenv("IMPALA_HOME") << "/be/src/testutil/doesntexist";
-  ScopedFlagSetter<string> password_flag(&FLAGS_webserver_password_file,
-      password_file.str());
+  auto password =
+      ScopedFlagSetter<string>::Make(&FLAGS_webserver_password_file, 
password_file.str());
 
   Webserver webserver(FLAGS_webserver_port);
   ASSERT_FALSE(webserver.Start().ok());
@@ -314,8 +300,8 @@ TEST(Webserver, NoFrameEmbeddingTest) {
 }
 TEST(Webserver, FrameAllowEmbeddingTest) {
   const string FRAME_TEST_PATH = "/frames_test";
-  ScopedFlagSetter<string> 
webserver_x_frame_options(&FLAGS_webserver_x_frame_options,
-      "ALLOWALL");
+  auto x_frame_opt =
+      ScopedFlagSetter<string>::Make(&FLAGS_webserver_x_frame_options, 
"ALLOWALL");
   Webserver webserver(FLAGS_webserver_port);
   Webserver::UrlCallback callback = bind<void>(FrameCallback, _1, _2);
   webserver.RegisterUrlCallback(FRAME_TEST_PATH, "raw_text.tmpl", callback);

Reply via email to