From 6938762bba13d89ee2d02b59e41b2356cc2ec86c Mon Sep 17 00:00:00 2001 From: Keith Wansbrough Date: Tue, 10 Apr 2018 16:50:28 +0100 Subject: [PATCH 1/6] CPP-499 Add new API and tests. --- gtests/src/integration/objects/cluster.hpp | 13 ++++ .../tests/test_control_connection.cpp | 61 +++++++++++++++++++ include/cassandra.h | 33 ++++++++++ src/cluster.cpp | 12 ++++ src/config.hpp | 8 +++ 5 files changed, 127 insertions(+) diff --git a/gtests/src/integration/objects/cluster.hpp b/gtests/src/integration/objects/cluster.hpp index 029cd4c6a..3f77b275e 100644 --- a/gtests/src/integration/objects/cluster.hpp +++ b/gtests/src/integration/objects/cluster.hpp @@ -134,6 +134,19 @@ class Cluster : public Object { return *this; } + /** + * Assign the local address to bind; passing an empty string will clear + * the local address. + * + * @param name An IP address or hostname + * @return Cluster object + */ + Cluster& with_local_address(const std::string& name) { + EXPECT_EQ(CASS_OK, cass_cluster_set_local_address(get(), + name.c_str())); + return *this; + } + /** * Assign the number of connections made to each node/server for each * connections thread diff --git a/gtests/src/integration/tests/test_control_connection.cpp b/gtests/src/integration/tests/test_control_connection.cpp index 06b4587b0..897f39cdf 100644 --- a/gtests/src/integration/tests/test_control_connection.cpp +++ b/gtests/src/integration/tests/test_control_connection.cpp @@ -186,6 +186,67 @@ CASSANDRA_INTEGRATION_TEST_F(ControlConnectionTests, ConnectUsingInvalidPort) { } } +/** + * Perform session connection using invalid local IP address + * + * This test will attempt to perform a connection using an invalid local IP + * address and ensure the control connection is not established against a + * single node cluster. + * + * @test_category control_connection + * @since core:1.0.0 + * @expected_result Control connection will not be established + */ +CASSANDRA_INTEGRATION_TEST_F(ControlConnectionTests, + ConnectUsingInvalidLocalIpAddress) { + CHECK_FAILURE; + + // Attempt to connect to the server using an invalid local IP address + logger_.add_critera("Unable to establish a control connection to host " \ + "1.1.1.1 because of the following error: Connection " \ + "timeout"); + Cluster cluster = default_cluster().with_local_address("1.1.1.1"); + try { + cluster.connect(); + FAIL() << "Connection was established using invalid local IP address"; + } catch (Session::Exception& se) { + ASSERT_EQ(CASS_ERROR_LIB_NO_HOSTS_AVAILABLE, se.error_code()); + ASSERT_GE(logger_.count(), 1u); + } +} + +/** + * Perform session connection using valid local IP address but invalid + * remote address + * + * This test will attempt to perform a connection using a valid local IP + * address and invalid remote address and ensure the control connection is + * not established against a single node cluster. + * + * @test_category control_connection + * @since core:1.0.0 + * @expected_result Control connection will not be established + */ +CASSANDRA_INTEGRATION_TEST_F(ControlConnectionTests, + ConnectUsingValidLocalIpAddressButInvalidRemote) { + CHECK_FAILURE; + + // Attempt to connect to the server using an valid local IP address + // but invalid remote address + logger_.add_critera("Unable to establish a control connection to host " \ + "1.1.1.1 because of the following error: Connection " \ + "timeout"); + Cluster cluster = Cluster::build().with_contact_points("1.1.1.1") + .with_local_address("127.0.0.1"); + try { + cluster.connect(); + FAIL() << "Connection was established using invalid IP address"; + } catch (Session::Exception& se) { + ASSERT_EQ(CASS_ERROR_LIB_NO_HOSTS_AVAILABLE, se.error_code()); + ASSERT_GE(logger_.count(), 1u); + } +} + /** * Perform session connection while forcing a control connection reconnect * diff --git a/include/cassandra.h b/include/cassandra.h index 86e16b43d..3d5fbb0a7 100644 --- a/include/cassandra.h +++ b/include/cassandra.h @@ -946,6 +946,39 @@ CASS_EXPORT CassError cass_cluster_set_port(CassCluster* cluster, int port); +/** + * Same as cass_cluster_set_local_address(), but with lengths for string + * parameters. + * + * @public @memberof CassCluster + * + * @param[in] cluster + * @param[in] name + * @param[in] name_length + * @return same as cass_cluster_set_local_address() + * + * @see cass_cluster_set_local_address() + */ +CASS_EXPORT CassError +cass_cluster_set_local_address_n(CassCluster* cluster, + const char* name, + size_t name_length); + +/** + * Sets the local address to bind when connecting to the cluster, + * if desired. + * + * @public @memberof CassCluster + * + * @param[in] cluster + * @param[in] name IP address or hostname to bind, or empty string + * for no binding. + * @return CASS_OK if successful, otherwise an error occurred. + */ +CASS_EXPORT CassError +cass_cluster_set_local_address(CassCluster* cluster, + const char* name); + /** * Sets the SSL context and enables SSL. * diff --git a/src/cluster.cpp b/src/cluster.cpp index fe2b6a398..b8c636668 100644 --- a/src/cluster.cpp +++ b/src/cluster.cpp @@ -465,6 +465,18 @@ CassError cass_cluster_set_prepare_on_up_or_add_host(CassCluster* cluster, return CASS_OK; } +CassError cass_cluster_set_local_address(CassCluster* cluster, + const char* name) { + return cass_cluster_set_local_address_n(cluster, name, SAFE_STRLEN(name)); +} + +CassError cass_cluster_set_local_address_n(CassCluster* cluster, + const char* name, + size_t name_length) { + cluster->config().set_local_address(std::string(name, name_length)); + return CASS_OK; +} + CassError cass_cluster_set_no_compact(CassCluster* cluster, cass_bool_t enabled) { cluster->config().set_no_compact(enabled == cass_true); diff --git a/src/config.hpp b/src/config.hpp index 257c33a06..ac33fcfdb 100644 --- a/src/config.hpp +++ b/src/config.hpp @@ -81,6 +81,7 @@ class Config { , use_randomized_contact_points_(true) , prepare_on_all_hosts_(true) , prepare_on_up_or_add_host_(true) + , local_address_("") , no_compact_(false) { } Config new_instance() const { @@ -395,6 +396,12 @@ class Config { prepare_on_up_or_add_host_ = enabled; } + const std::string& local_address() const { return local_address_; } + + void set_local_address(const std::string& name) { + local_address_ = name; + } + bool no_compact() const { return no_compact_; } void set_no_compact(bool enabled) { @@ -448,6 +455,7 @@ class Config { bool use_randomized_contact_points_; bool prepare_on_all_hosts_; bool prepare_on_up_or_add_host_; + std::string local_address_; bool no_compact_; }; From 7fd6eb0f02758ce4b6ebdb176cad69b53a56f4a9 Mon Sep 17 00:00:00 2001 From: Keith Wansbrough Date: Wed, 11 Apr 2018 14:37:41 +0100 Subject: [PATCH 2/6] CPP-499 Improve tests. - Don't let it connect until we've set the cluster config! - Use better test names and fix up log messages. --- .../tests/test_control_connection.cpp | 53 +++++++++++++++---- 1 file changed, 42 insertions(+), 11 deletions(-) diff --git a/gtests/src/integration/tests/test_control_connection.cpp b/gtests/src/integration/tests/test_control_connection.cpp index 897f39cdf..353a592e3 100644 --- a/gtests/src/integration/tests/test_control_connection.cpp +++ b/gtests/src/integration/tests/test_control_connection.cpp @@ -22,6 +22,17 @@ * Control connection integration tests; single node cluster */ class ControlConnectionTests : public Integration { + +public: + + void SetUp() { + // Call the parent setup function (don't automatically start session, + // because we don't want any connections established until we have + // set up the cluster). + is_session_requested_ = false; + Integration::SetUp(); + } + protected: /** * Execute multiple requests and ensure the expected nodes are used during @@ -187,9 +198,30 @@ CASSANDRA_INTEGRATION_TEST_F(ControlConnectionTests, ConnectUsingInvalidPort) { } /** - * Perform session connection using invalid local IP address + * Perform session connection using unresolvable local IP address + * + * This test will attempt to perform a connection using an unresolvable local + * IP address and ensure the control connection is not established against a + * single node cluster. + * + * @test_category control_connection + * @since core:1.0.0 + * @expected_result Control connection will not be established + */ +CASSANDRA_INTEGRATION_TEST_F(ControlConnectionTests, + ConnectUsingUnresolvableLocalIpAddress) { + CHECK_FAILURE; + + // Attempt to connect to the server using an unresolvable local IP address + Cluster cluster = default_cluster(); + EXPECT_EQ(CASS_ERROR_LIB_HOST_RESOLUTION, + cass_cluster_set_local_address(cluster.get(), "unknown.invalid")); +} + +/** + * Perform session connection using unbindable local IP address * - * This test will attempt to perform a connection using an invalid local IP + * This test will attempt to perform a connection using an unbindable local IP * address and ensure the control connection is not established against a * single node cluster. * @@ -198,17 +230,15 @@ CASSANDRA_INTEGRATION_TEST_F(ControlConnectionTests, ConnectUsingInvalidPort) { * @expected_result Control connection will not be established */ CASSANDRA_INTEGRATION_TEST_F(ControlConnectionTests, - ConnectUsingInvalidLocalIpAddress) { + ConnectUsingUnbindableLocalIpAddress) { CHECK_FAILURE; - // Attempt to connect to the server using an invalid local IP address - logger_.add_critera("Unable to establish a control connection to host " \ - "1.1.1.1 because of the following error: Connection " \ - "timeout"); + // Attempt to connect to the server using an unbindable local IP address + logger_.add_critera("Unable to bind local address"); Cluster cluster = default_cluster().with_local_address("1.1.1.1"); try { cluster.connect(); - FAIL() << "Connection was established using invalid local IP address"; + FAIL() << "Connection was established using unbindable local IP address"; } catch (Session::Exception& se) { ASSERT_EQ(CASS_ERROR_LIB_NO_HOSTS_AVAILABLE, se.error_code()); ASSERT_GE(logger_.count(), 1u); @@ -232,10 +262,11 @@ CASSANDRA_INTEGRATION_TEST_F(ControlConnectionTests, CHECK_FAILURE; // Attempt to connect to the server using an valid local IP address - // but invalid remote address + // but invalid remote address. The specified remote is not routable + // from the specified local. logger_.add_critera("Unable to establish a control connection to host " \ - "1.1.1.1 because of the following error: Connection " \ - "timeout"); + "1.1.1.1 because of the following error: " \ + "Connect error 'operation not permitted'"); Cluster cluster = Cluster::build().with_contact_points("1.1.1.1") .with_local_address("127.0.0.1"); try { From 23a2f7d14951ab9553f132319662b1def05bf712 Mon Sep 17 00:00:00 2001 From: Keith Wansbrough Date: Wed, 11 Apr 2018 14:39:07 +0100 Subject: [PATCH 3/6] CPP-499 Implement local address binding. - Look up address at config time (but don't use resolver, because that might be slow). - Apply it at connection time. --- src/cluster.cpp | 8 +++++++- src/config.hpp | 10 +++++----- src/connection.cpp | 20 ++++++++++++++++---- 3 files changed, 28 insertions(+), 10 deletions(-) diff --git a/src/cluster.cpp b/src/cluster.cpp index b8c636668..00558080a 100644 --- a/src/cluster.cpp +++ b/src/cluster.cpp @@ -473,7 +473,13 @@ CassError cass_cluster_set_local_address(CassCluster* cluster, CassError cass_cluster_set_local_address_n(CassCluster* cluster, const char* name, size_t name_length) { - cluster->config().set_local_address(std::string(name, name_length)); + cass::Address address; + if (name_length == 0 || + cass::Address::from_string(std::string(name, name_length), 0, &address)) { + cluster->config().set_local_address(address); + } else { + return CASS_ERROR_LIB_HOST_RESOLUTION; + } return CASS_OK; } diff --git a/src/config.hpp b/src/config.hpp index ac33fcfdb..4f86c9612 100644 --- a/src/config.hpp +++ b/src/config.hpp @@ -81,7 +81,6 @@ class Config { , use_randomized_contact_points_(true) , prepare_on_all_hosts_(true) , prepare_on_up_or_add_host_(true) - , local_address_("") , no_compact_(false) { } Config new_instance() const { @@ -396,10 +395,11 @@ class Config { prepare_on_up_or_add_host_ = enabled; } - const std::string& local_address() const { return local_address_; } + const Address* local_address() const { + return local_address_.is_valid() ? &local_address_ : NULL; } - void set_local_address(const std::string& name) { - local_address_ = name; + void set_local_address(const Address& address) { + local_address_ = address; } bool no_compact() const { return no_compact_; } @@ -455,7 +455,7 @@ class Config { bool use_randomized_contact_points_; bool prepare_on_all_hosts_; bool prepare_on_up_or_add_host_; - std::string local_address_; + Address local_address_; bool no_compact_; }; diff --git a/src/connection.cpp b/src/connection.cpp index ec9f2d36b..ba1ecf159 100644 --- a/src/connection.cpp +++ b/src/connection.cpp @@ -232,6 +232,7 @@ Connection::Connection(uv_loop_t* loop, , heartbeat_outstanding_(false) { socket_.data = this; uv_tcp_init(loop_, &socket_); + bool ok = true; if (uv_tcp_nodelay(&socket_, config.tcp_nodelay_enable() ? 1 : 0) != 0) { @@ -244,9 +245,19 @@ Connection::Connection(uv_loop_t* loop, LOG_WARN("Unable to set tcp keepalive"); } - SslContext* ssl_context = config_.ssl_context(); - if (ssl_context != NULL) { - ssl_session_.reset(ssl_context->create_session(host)); + const Address* local_address = config_.local_address(); + if (local_address) { + if (uv_tcp_bind(&socket_, local_address->addr(), 0)) { + ok = false; + notify_error("Unable to bind local address"); + } + } + + if (ok) { + SslContext* ssl_context = config_.ssl_context(); + if (ssl_context != NULL) { + ssl_session_.reset(ssl_context->create_session(host)); + } } } @@ -378,7 +389,8 @@ void Connection::set_state(ConnectionState new_state) { switch (state_) { case CONNECTION_STATE_NEW: - assert(new_state == CONNECTION_STATE_CONNECTING && + assert((new_state == CONNECTION_STATE_CONNECTING || + new_state == CONNECTION_STATE_CLOSE_DEFUNCT) && "Invalid connection state after new"); state_ = new_state; break; From 62da406bd406208478e1981ba710bde955487a14 Mon Sep 17 00:00:00 2001 From: Keith Wansbrough Date: Wed, 11 Apr 2018 14:41:03 +0100 Subject: [PATCH 4/6] CPP-499 Documentation for cass_cluster_set_local_address. --- include/cassandra.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/cassandra.h b/include/cassandra.h index 3d5fbb0a7..d524f8090 100644 --- a/include/cassandra.h +++ b/include/cassandra.h @@ -971,8 +971,8 @@ cass_cluster_set_local_address_n(CassCluster* cluster, * @public @memberof CassCluster * * @param[in] cluster - * @param[in] name IP address or hostname to bind, or empty string - * for no binding. + * @param[in] name IP address to bind, or empty string for no binding. + * Only numeric addresses are supported; no resolution is done. * @return CASS_OK if successful, otherwise an error occurred. */ CASS_EXPORT CassError From 3a460f66cc86f59ef1c26576b08d27d1c35b030d Mon Sep 17 00:00:00 2001 From: Keith Wansbrough Date: Mon, 16 Apr 2018 12:18:23 +0100 Subject: [PATCH 5/6] CPP-499 Review markups: error reporting, NULL handling. --- gtests/src/integration/tests/test_control_connection.cpp | 2 +- src/cluster.cpp | 3 ++- src/connection.cpp | 5 +++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/gtests/src/integration/tests/test_control_connection.cpp b/gtests/src/integration/tests/test_control_connection.cpp index 353a592e3..747f4e7ac 100644 --- a/gtests/src/integration/tests/test_control_connection.cpp +++ b/gtests/src/integration/tests/test_control_connection.cpp @@ -234,7 +234,7 @@ CASSANDRA_INTEGRATION_TEST_F(ControlConnectionTests, CHECK_FAILURE; // Attempt to connect to the server using an unbindable local IP address - logger_.add_critera("Unable to bind local address"); + logger_.add_critera("Unable to bind local address: address not available"); Cluster cluster = default_cluster().with_local_address("1.1.1.1"); try { cluster.connect(); diff --git a/src/cluster.cpp b/src/cluster.cpp index 00558080a..87ea51fc1 100644 --- a/src/cluster.cpp +++ b/src/cluster.cpp @@ -473,8 +473,9 @@ CassError cass_cluster_set_local_address(CassCluster* cluster, CassError cass_cluster_set_local_address_n(CassCluster* cluster, const char* name, size_t name_length) { - cass::Address address; + cass::Address address; // default to AF_UNSPEC if (name_length == 0 || + name == NULL || cass::Address::from_string(std::string(name, name_length), 0, &address)) { cluster->config().set_local_address(address); } else { diff --git a/src/connection.cpp b/src/connection.cpp index ba1ecf159..1280fb81e 100644 --- a/src/connection.cpp +++ b/src/connection.cpp @@ -247,9 +247,10 @@ Connection::Connection(uv_loop_t* loop, const Address* local_address = config_.local_address(); if (local_address) { - if (uv_tcp_bind(&socket_, local_address->addr(), 0)) { + int rc = uv_tcp_bind(&socket_, local_address->addr(), 0); + if (rc) { ok = false; - notify_error("Unable to bind local address"); + notify_error("Unable to bind local address: " + std::string(UV_ERRSTR(rc, loop_))); } } From 256b0a2ff15c34b64478649fed7165a7a46d151f Mon Sep 17 00:00:00 2001 From: Keith Wansbrough Date: Fri, 20 Apr 2018 14:22:22 +0100 Subject: [PATCH 6/6] CPP-499: Further review markups. --- .../tests/test_control_connection.cpp | 1 + include/cassandra.h | 34 +++++++++---------- src/connection.cpp | 11 +++--- 3 files changed, 22 insertions(+), 24 deletions(-) diff --git a/gtests/src/integration/tests/test_control_connection.cpp b/gtests/src/integration/tests/test_control_connection.cpp index 747f4e7ac..3bf7445f3 100644 --- a/gtests/src/integration/tests/test_control_connection.cpp +++ b/gtests/src/integration/tests/test_control_connection.cpp @@ -587,6 +587,7 @@ CASSANDRA_INTEGRATION_TEST_F(ControlConnectionTests, CHECK_FAILURE; // Stop the cluster and attempt to perform a request + connect(); ccm_->stop_cluster(); Result result = session_.execute(SELECT_ALL_SYSTEM_LOCAL_CQL, CASS_CONSISTENCY_ONE, false, false); diff --git a/include/cassandra.h b/include/cassandra.h index d524f8090..8d6b66996 100644 --- a/include/cassandra.h +++ b/include/cassandra.h @@ -947,37 +947,37 @@ cass_cluster_set_port(CassCluster* cluster, int port); /** - * Same as cass_cluster_set_local_address(), but with lengths for string - * parameters. + * Sets the local address to bind when connecting to the cluster, + * if desired. * * @public @memberof CassCluster * * @param[in] cluster - * @param[in] name - * @param[in] name_length - * @return same as cass_cluster_set_local_address() - * - * @see cass_cluster_set_local_address() + * @param[in] name IP address to bind, or empty string for no binding. + * Only numeric addresses are supported; no resolution is done. + * @return CASS_OK if successful, otherwise an error occurred. */ CASS_EXPORT CassError -cass_cluster_set_local_address_n(CassCluster* cluster, - const char* name, - size_t name_length); +cass_cluster_set_local_address(CassCluster* cluster, + const char* name); /** - * Sets the local address to bind when connecting to the cluster, - * if desired. + * Same as cass_cluster_set_local_address(), but with lengths for string + * parameters. * * @public @memberof CassCluster * * @param[in] cluster - * @param[in] name IP address to bind, or empty string for no binding. - * Only numeric addresses are supported; no resolution is done. - * @return CASS_OK if successful, otherwise an error occurred. + * @param[in] name + * @param[in] name_length + * @return same as cass_cluster_set_local_address() + * + * @see cass_cluster_set_local_address() */ CASS_EXPORT CassError -cass_cluster_set_local_address(CassCluster* cluster, - const char* name); +cass_cluster_set_local_address_n(CassCluster* cluster, + const char* name, + size_t name_length); /** * Sets the SSL context and enables SSL. diff --git a/src/connection.cpp b/src/connection.cpp index 1280fb81e..ce8628cad 100644 --- a/src/connection.cpp +++ b/src/connection.cpp @@ -232,7 +232,6 @@ Connection::Connection(uv_loop_t* loop, , heartbeat_outstanding_(false) { socket_.data = this; uv_tcp_init(loop_, &socket_); - bool ok = true; if (uv_tcp_nodelay(&socket_, config.tcp_nodelay_enable() ? 1 : 0) != 0) { @@ -249,16 +248,14 @@ Connection::Connection(uv_loop_t* loop, if (local_address) { int rc = uv_tcp_bind(&socket_, local_address->addr(), 0); if (rc) { - ok = false; notify_error("Unable to bind local address: " + std::string(UV_ERRSTR(rc, loop_))); + return; } } - if (ok) { - SslContext* ssl_context = config_.ssl_context(); - if (ssl_context != NULL) { - ssl_session_.reset(ssl_context->create_session(host)); - } + SslContext* ssl_context = config_.ssl_context(); + if (ssl_context != NULL) { + ssl_session_.reset(ssl_context->create_session(host)); } }