Commit 0b070124 authored by Stephen Morris's avatar Stephen Morris
Browse files

[3164] The database connection timeout is now a configurable parameter

In addition, the default has been changed to five seconds.
parent d67528d0
......@@ -457,6 +457,12 @@ be followed by a comma and another object definition.</para>
<screen>
"Dhcp4": { "lease-database": { <userinput>"host" : ""</userinput>, ... }, ... }
</screen>
Should the database be located on a different system, you may need to specify a longer interval
for the connection timeout:
<screen>
"Dhcp4": { "lease-database": { <userinput>"connect-timeout" : <replaceable>tomeout-in-seconds</replaceable></userinput>, ... }, ... }
</screen>
The default value of five seconds should be more than adequate for local connections.
</para>
<para>Finally, the credentials of the account under which the server will
access the database should be set:
......
......@@ -456,6 +456,12 @@ be followed by a comma and another object definition.</para>
<screen>
"Dhcp6": { "lease-database": { <userinput>"host" : ""</userinput>, ... }, ... }
</screen>
Should the database be located on a different system, you may need to specify a longer interval
for the connection timeout:
<screen>
"Dhcp6": { "lease-database": { <userinput>"connect-timeout" : <replaceable>tomeout-in-seconds</replaceable></userinput>, ... }, ... }
</screen>
The default value of five seconds should be more than adequate for local connections.
</para>
<para>Finally, the credentials of the account under which the server will
access the database should be set:
......
......@@ -45,6 +45,16 @@ public:
isc::Exception(file, line, what) {}
};
/// @brief Invalid Timeout
///
/// Thrown when the timeout specified for the database connection is invalid.
class DbInvalidTimeout : public Exception {
public:
DbInvalidTimeout(const char* file, size_t line, const char* what) :
isc::Exception(file, line, what) {}
};
/// @brief Common database connection class.
///
/// This class provides functions that are common for establishing
......
......@@ -9,8 +9,9 @@
#include <dhcpsrv/mysql_connection.h>
#include <exceptions/exceptions.h>
#include <boost/lexical_cast.hpp>
#include <algorithm>
#include <iostream>
#include <iterator>
#include <stdint.h>
#include <string>
......@@ -27,6 +28,8 @@ const my_bool MLM_TRUE = 1;
const int MLM_MYSQL_FETCH_SUCCESS = 0;
const int MLM_MYSQL_FETCH_FAILURE = 1;
const int MYSQL_DEFAULT_CONNECTION_TIMEOUT = 5; // seconds
// Open the database using the parameters passed to the constructor.
void
......@@ -67,7 +70,33 @@ MySqlConnection::openDatabase() {
name = sname.c_str();
} catch (...) {
// No database name. Throw a "NoName" exception
isc_throw(NoDatabaseName, "must specified a name for the database");
isc_throw(NoDatabaseName, "must specify a name for the database");
}
int connect_timeout = MYSQL_DEFAULT_CONNECTION_TIMEOUT;
string stimeout;
try {
stimeout = getParameter("connect-timeout");
} catch (...) {
// No timeout parameter, we are going to use the default timeout.
stimeout = "";
}
if (stimeout.size() > 0) {
// Timeout was given, so try to convert it to an integer.
try {
connect_timeout = boost::lexical_cast<int>(stimeout);
} catch (...) {
// Timeout given but invalid.
isc_throw(DbInvalidTimeout, "database connection timeout (" << stimeout <<
") must be numeric");
}
// ... and check the range.
if (connect_timeout <= 0) {
isc_throw(DbInvalidTimeout, "database connection timeout (" << connect_timeout <<
") must be an integer greater than 0");
}
}
// Set options for the connection:
......@@ -93,6 +122,14 @@ MySqlConnection::openDatabase() {
mysql_error(mysql_));
}
// Connection timeout, the amount of time taken for the client to drop
// the connection if the server is not responding.
result = mysql_options(mysql_, MYSQL_OPT_CONNECT_TIMEOUT, &connect_timeout);
if (result != 0) {
isc_throw(DbOpenError, "unable to set database connection timeout: " <<
mysql_error(mysql_));
}
// Open the database.
//
// The option CLIENT_FOUND_ROWS is specified so that in an UPDATE,
......
......@@ -49,6 +49,7 @@ DbAccessParser::build(isc::data::ConstElementPtr config_value) {
std::map<string, string> values_copy = values_;
int64_t lfc_interval = 0;
int64_t timeout = 0;
// 2. Update the copy with the passed keywords.
BOOST_FOREACH(ConfigPair param, config_value->mapValue()) {
try {
......@@ -61,6 +62,11 @@ DbAccessParser::build(isc::data::ConstElementPtr config_value) {
values_copy[param.first] =
boost::lexical_cast<std::string>(lfc_interval);
} else if (param.first == "connect-timeout") {
timeout = param.second->intValue();
values_copy[param.first] =
boost::lexical_cast<std::string>(timeout);
} else {
values_copy[param.first] = param.second->stringValue();
}
......@@ -96,6 +102,14 @@ DbAccessParser::build(isc::data::ConstElementPtr config_value) {
<< std::numeric_limits<uint32_t>::max());
}
// d. Check that the timeout is a number and it is within a resonable
// range.
if ((timeout < 0) || (timeout > std::numeric_limits<uint32_t>::max())) {
isc_throw(BadValue, "timeout value: " << timeout
<< " is out of range, expected value: 0.."
<< std::numeric_limits<uint32_t>::max());
}
// 4. If all is OK, update the stored keyword/value pairs. We do this by
// swapping contents - values_copy is destroyed immediately after the
// operation (when the method exits), so we are not interested in its new
......
......@@ -68,6 +68,7 @@ public:
///
/// - "type" is "memfile", "mysql" or "postgresql"
/// - "lfc-interval" is a number from the range of 0 to 4294967295.
/// - "timeout" is a number from the range of 0 to 4294967295.
///
/// Once all has been validated, constructs the database access string
/// expected by the lease manager.
......
......@@ -14,7 +14,6 @@
#include <boost/static_assert.hpp>
#include <iostream>
#include <iomanip>
#include <limits>
#include <sstream>
......@@ -41,6 +40,9 @@ using namespace std;
namespace {
// Default connection timeout
const int PGSQL_DEFAULT_CONNECTION_TIMEOUT = 5; // seconds
// Maximum number of parameters used in any single query
const size_t MAX_PARAMETERS_IN_QUERY = 14;
......@@ -1099,7 +1101,6 @@ PgSqlLeaseMgr::openDatabase() {
} catch(...) {
// No host. Fine, we'll use "localhost"
}
dbconnparameters += "host = '" + shost + "'" ;
string suser;
......@@ -1127,6 +1128,36 @@ PgSqlLeaseMgr::openDatabase() {
isc_throw(NoDatabaseName, "must specify a name for the database");
}
int connect_timeout = PGSQL_DEFAULT_CONNECTION_TIMEOUT;
string stimeout;
try {
stimeout = dbconn_.getParameter("connect-timeout");
} catch (...) {
// No timeout parameter, we are going to use the default timeout.
stimeout = "";
}
if (stimeout.size() > 0) {
// Timeout was given, so try to convert it to an integer.
try {
connect_timeout = boost::lexical_cast<int>(stimeout);
} catch (...) {
// Timeout given but invalid.
isc_throw(DbInvalidTimeout, "database connection timeout (" << stimeout <<
") must be numeric");
}
// ... and check the range.
if (connect_timeout <= 0) {
isc_throw(DbInvalidTimeout, "database connection timeout (" << connect_timeout <<
") must be an integer greater than 0");
}
}
std::ostringstream oss;
oss << connect_timeout;
dbconnparameters += " connect_timeout = " + oss.str();
conn_ = PQconnectdb(dbconnparameters.c_str());
if (conn_ == NULL) {
isc_throw(DbOpenError, "could not allocate connection object");
......
......@@ -175,7 +175,8 @@ private:
///
/// @return true if the value of the parameter should be quoted.
bool quoteValue(const std::string& parameter) const {
return ((parameter != "persist") && (parameter != "lfc-interval"));
return ((parameter != "persist") && (parameter != "lfc-interval") &&
(parameter != "connect-timeout"));
}
};
......@@ -343,6 +344,56 @@ TEST_F(DbAccessParserTest, largeLFCInterval) {
EXPECT_THROW(parser.build(json_elements), BadValue);
}
// This test checks that the parser accepts the valid value of the
// timeout parameter.
TEST_F(DbAccessParserTest, validTimeout) {
const char* config[] = {"type", "memfile",
"name", "/opt/kea/var/kea-leases6.csv",
"connect-timeout", "3600",
NULL};
string json_config = toJson(config);
ConstElementPtr json_elements = Element::fromJSON(json_config);
EXPECT_TRUE(json_elements);
TestDbAccessParser parser("lease-database", DbAccessParser::LEASE_DB);
EXPECT_NO_THROW(parser.build(json_elements));
checkAccessString("Valid timeout", parser.getDbAccessParameters(),
config);
}
// This test checks that the parser rejects the negative value of the
// timeout parameter.
TEST_F(DbAccessParserTest, negativeTimeout) {
const char* config[] = {"type", "memfile",
"name", "/opt/kea/var/kea-leases6.csv",
"connect-timeout", "-1",
NULL};
string json_config = toJson(config);
ConstElementPtr json_elements = Element::fromJSON(json_config);
EXPECT_TRUE(json_elements);
TestDbAccessParser parser("lease-database", DbAccessParser::LEASE_DB);
EXPECT_THROW(parser.build(json_elements), BadValue);
}
// This test checks that the parser rejects a too large (greater than
// the max uint32_t) value of the timeout parameter.
TEST_F(DbAccessParserTest, largeTimeout) {
const char* config[] = {"type", "memfile",
"name", "/opt/kea/var/kea-leases6.csv",
"connect-timeout", "4294967296",
NULL};
string json_config = toJson(config);
ConstElementPtr json_elements = Element::fromJSON(json_config);
EXPECT_TRUE(json_elements);
TestDbAccessParser parser("lease-database", DbAccessParser::LEASE_DB);
EXPECT_THROW(parser.build(json_elements), BadValue);
}
// Check that the parser works with a valid MySQL configuration
TEST_F(DbAccessParserTest, validTypeMysql) {
const char* config[] = {"type", "mysql",
......
......@@ -109,6 +109,21 @@ TEST(MySqlHostDataSource, OpenDatabase) {
<< "*** before the MySQL tests will run correctly.\n";
}
// Check that lease manager open the database opens correctly with a longer
// timeout. If it fails, print the error message.
try {
string connection_string = validMySQLConnectionString() + string(" ") +
string(VALID_TIMEOUT);
HostDataSourceFactory::create(connection_string);
EXPECT_NO_THROW((void) HostDataSourceFactory::getHostDataSourcePtr());
HostDataSourceFactory::destroy();
} catch (const isc::Exception& ex) {
FAIL() << "*** ERROR: unable to open database, reason:\n"
<< " " << ex.what() << "\n"
<< "*** The test environment is broken and must be fixed\n"
<< "*** before the MySQL tests will run correctly.\n";
}
// Check that attempting to get an instance of the lease manager when
// none is set throws an exception.
EXPECT_FALSE(HostDataSourceFactory::getHostDataSourcePtr());
......@@ -136,6 +151,12 @@ TEST(MySqlHostDataSource, OpenDatabase) {
EXPECT_THROW(HostDataSourceFactory::create(connectionString(
MYSQL_VALID_TYPE, VALID_NAME, VALID_HOST, VALID_USER, INVALID_PASSWORD)),
DbOpenError);
EXPECT_THROW(HostDataSourceFactory::create(connectionString(
MYSQL_VALID_TYPE, VALID_NAME, VALID_HOST, VALID_USER, VALID_PASSWORD, INVALID_TIMEOUT_1)),
DbInvalidTimeout);
EXPECT_THROW(HostDataSourceFactory::create(connectionString(
MYSQL_VALID_TYPE, VALID_NAME, VALID_HOST, VALID_USER, VALID_PASSWORD, INVALID_TIMEOUT_2)),
DbInvalidTimeout);
// Check for missing parameters
EXPECT_THROW(HostDataSourceFactory::create(connectionString(
......
......@@ -101,7 +101,7 @@ TEST(MySqlOpenTest, OpenDatabase) {
createMySQLSchema(true);
// Check that lease manager open the database opens correctly and tidy up.
// If it fails, print the error message.
// If it fails, print the error message.
try {
LeaseMgrFactory::create(validMySQLConnectionString());
EXPECT_NO_THROW((void) LeaseMgrFactory::instance());
......@@ -113,6 +113,21 @@ TEST(MySqlOpenTest, OpenDatabase) {
<< "*** before the MySQL tests will run correctly.\n";
}
// Check that lease manager open the database opens correctly with a longer
// timeout. If it fails, print the error message.
try {
string connection_string = validMySQLConnectionString() + string(" ") +
string(VALID_TIMEOUT);
LeaseMgrFactory::create(connection_string);
EXPECT_NO_THROW((void) LeaseMgrFactory::instance());
LeaseMgrFactory::destroy();
} catch (const isc::Exception& ex) {
FAIL() << "*** ERROR: unable to open database, reason:\n"
<< " " << ex.what() << "\n"
<< "*** The test environment is broken and must be fixed\n"
<< "*** before the MySQL tests will run correctly.\n";
}
// Check that attempting to get an instance of the lease manager when
// none is set throws an exception.
EXPECT_THROW(LeaseMgrFactory::instance(), NoLeaseManager);
......@@ -140,6 +155,12 @@ TEST(MySqlOpenTest, OpenDatabase) {
EXPECT_THROW(LeaseMgrFactory::create(connectionString(
MYSQL_VALID_TYPE, VALID_NAME, VALID_HOST, VALID_USER, INVALID_PASSWORD)),
DbOpenError);
EXPECT_THROW(LeaseMgrFactory::create(connectionString(
MYSQL_VALID_TYPE, VALID_NAME, VALID_HOST, VALID_USER, VALID_PASSWORD, INVALID_TIMEOUT_1)),
DbInvalidTimeout);
EXPECT_THROW(LeaseMgrFactory::create(connectionString(
MYSQL_VALID_TYPE, VALID_NAME, VALID_HOST, VALID_USER, VALID_PASSWORD, INVALID_TIMEOUT_2)),
DbInvalidTimeout);
// Check for missing parameters
EXPECT_THROW(LeaseMgrFactory::create(connectionString(
......
......@@ -112,6 +112,22 @@ TEST(PgSqlOpenTest, OpenDatabase) {
<< "*** The test environment is broken and must be fixed\n"
<< "*** before the PostgreSQL tests will run correctly.\n";
}
// Check that lease manager open the database opens correctly with a longer
// timeout. If it fails, print the error message.
try {
string connection_string = validPgSQLConnectionString() + string(" ") +
string(VALID_TIMEOUT);
LeaseMgrFactory::create(connection_string);
EXPECT_NO_THROW((void) LeaseMgrFactory::instance());
LeaseMgrFactory::destroy();
} catch (const isc::Exception& ex) {
FAIL() << "*** ERROR: unable to open database, reason:\n"
<< " " << ex.what() << "\n"
<< "*** The test environment is broken and must be fixed\n"
<< "*** before the MySQL tests will run correctly.\n";
}
// Check that attempting to get an instance of the lease manager when
// none is set throws an exception.
EXPECT_THROW(LeaseMgrFactory::instance(), NoLeaseManager);
......@@ -147,6 +163,14 @@ TEST(PgSqlOpenTest, OpenDatabase) {
PGSQL_VALID_TYPE, VALID_NAME, VALID_HOST, VALID_USER, INVALID_PASSWORD)),
DbOpenError);
// Check for invalid timeouts
EXPECT_THROW(LeaseMgrFactory::create(connectionString(
PGSQL_VALID_TYPE, VALID_NAME, VALID_HOST, VALID_USER, VALID_PASSWORD, INVALID_TIMEOUT_1)),
DbInvalidTimeout);
EXPECT_THROW(LeaseMgrFactory::create(connectionString(
PGSQL_VALID_TYPE, VALID_NAME, VALID_HOST, VALID_USER, VALID_PASSWORD, INVALID_TIMEOUT_2)),
DbInvalidTimeout);
// Check for missing parameters
EXPECT_THROW(LeaseMgrFactory::create(connectionString(
PGSQL_VALID_TYPE, NULL, VALID_HOST, INVALID_USER, VALID_PASSWORD)),
......
......@@ -28,9 +28,12 @@ const char* VALID_USER = "user=keatest";
const char* INVALID_USER = "user=invaliduser";
const char* VALID_PASSWORD = "password=keatest";
const char* INVALID_PASSWORD = "password=invalid";
const char* VALID_TIMEOUT = "connect-timeout=10";
const char* INVALID_TIMEOUT_1 = "connect-timeout=foo";
const char* INVALID_TIMEOUT_2 = "connect-timeout=-17";
string connectionString(const char* type, const char* name, const char* host,
const char* user, const char* password) {
const char* user, const char* password, const char* timeout) {
const string space = " ";
string result = "";
......@@ -65,6 +68,13 @@ string connectionString(const char* type, const char* name, const char* host,
result += string(password);
}
if (timeout != NULL) {
if (! result.empty()) {
result += space;
}
result += string(timeout);
}
return (result);
}
......
......@@ -8,6 +8,7 @@
#define SCHEMA_H
#include <config.h>
#include <cstdlib>
#include <string>
namespace isc {
......@@ -23,17 +24,21 @@ extern const char* VALID_USER;
extern const char* INVALID_USER;
extern const char* VALID_PASSWORD;
extern const char* INVALID_PASSWORD;
extern const char* VALID_TIMEOUT;
extern const char* INVALID_TIMEOUT_1;
extern const char* INVALID_TIMEOUT_2;
/// @brief Given a combination of strings above, produce a connection string.
///
/// @param type type of the database
/// @param name name of the database to connect to
/// @param host hostname
/// @param user username used to authendicate during connection attempt
/// @param password password used to authendicate during connection attempt
/// @param user username used to authenticate during connection attempt
/// @param password password used to authenticate during connection attempt
/// @param timeout timeout used during connection attempt
/// @return string containing all specified parameters
std::string connectionString(const char* type, const char* name,
const char* host, const char* user,
const char* password);
std::string connectionString(const char* type, const char* name = NULL,
const char* host = NULL, const char* user = NULL,
const char* password = NULL, const char* timeout = NULL);
};
};
};
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment