From: Stephen Morris Date: Wed, 11 May 2016 19:40:25 +0000 (+0100) Subject: [3164] The database connection timeout is now a configurable parameter X-Git-Tag: trac4106_update_base~14^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0b070124ab3567079a18ee9bb383dba364bb8ab1;p=thirdparty%2Fkea.git [3164] The database connection timeout is now a configurable parameter In addition, the default has been changed to five seconds. --- diff --git a/doc/guide/dhcp4-srv.xml b/doc/guide/dhcp4-srv.xml index 32f64f4686..a4e5fdfcee 100644 --- a/doc/guide/dhcp4-srv.xml +++ b/doc/guide/dhcp4-srv.xml @@ -457,6 +457,12 @@ be followed by a comma and another object definition. "Dhcp4": { "lease-database": { "host" : "", ... }, ... } + Should the database be located on a different system, you may need to specify a longer interval + for the connection timeout: + +"Dhcp4": { "lease-database": { "connect-timeout" : tomeout-in-seconds, ... }, ... } + +The default value of five seconds should be more than adequate for local connections. Finally, the credentials of the account under which the server will access the database should be set: diff --git a/doc/guide/dhcp6-srv.xml b/doc/guide/dhcp6-srv.xml index fe6a873dd4..2b79707059 100644 --- a/doc/guide/dhcp6-srv.xml +++ b/doc/guide/dhcp6-srv.xml @@ -456,6 +456,12 @@ be followed by a comma and another object definition. "Dhcp6": { "lease-database": { "host" : "", ... }, ... } + Should the database be located on a different system, you may need to specify a longer interval + for the connection timeout: + +"Dhcp6": { "lease-database": { "connect-timeout" : tomeout-in-seconds, ... }, ... } + +The default value of five seconds should be more than adequate for local connections. Finally, the credentials of the account under which the server will access the database should be set: diff --git a/src/lib/dhcpsrv/database_connection.h b/src/lib/dhcpsrv/database_connection.h index 350a84dd03..2be98baf41 100755 --- a/src/lib/dhcpsrv/database_connection.h +++ b/src/lib/dhcpsrv/database_connection.h @@ -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 diff --git a/src/lib/dhcpsrv/mysql_connection.cc b/src/lib/dhcpsrv/mysql_connection.cc index 196ecce850..b7163e798f 100755 --- a/src/lib/dhcpsrv/mysql_connection.cc +++ b/src/lib/dhcpsrv/mysql_connection.cc @@ -9,8 +9,9 @@ #include #include +#include + #include -#include #include #include #include @@ -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(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, diff --git a/src/lib/dhcpsrv/parsers/dbaccess_parser.cc b/src/lib/dhcpsrv/parsers/dbaccess_parser.cc index b390275e37..8dc6b80cc3 100644 --- a/src/lib/dhcpsrv/parsers/dbaccess_parser.cc +++ b/src/lib/dhcpsrv/parsers/dbaccess_parser.cc @@ -49,6 +49,7 @@ DbAccessParser::build(isc::data::ConstElementPtr config_value) { std::map 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(lfc_interval); + } else if (param.first == "connect-timeout") { + timeout = param.second->intValue(); + values_copy[param.first] = + boost::lexical_cast(timeout); + } else { values_copy[param.first] = param.second->stringValue(); } @@ -96,6 +102,14 @@ DbAccessParser::build(isc::data::ConstElementPtr config_value) { << std::numeric_limits::max()); } + // d. Check that the timeout is a number and it is within a resonable + // range. + if ((timeout < 0) || (timeout > std::numeric_limits::max())) { + isc_throw(BadValue, "timeout value: " << timeout + << " is out of range, expected value: 0.." + << std::numeric_limits::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 diff --git a/src/lib/dhcpsrv/parsers/dbaccess_parser.h b/src/lib/dhcpsrv/parsers/dbaccess_parser.h index 7f5febd153..6dc7e8c905 100644 --- a/src/lib/dhcpsrv/parsers/dbaccess_parser.h +++ b/src/lib/dhcpsrv/parsers/dbaccess_parser.h @@ -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. diff --git a/src/lib/dhcpsrv/pgsql_lease_mgr.cc b/src/lib/dhcpsrv/pgsql_lease_mgr.cc index 40695e5446..40546916f9 100644 --- a/src/lib/dhcpsrv/pgsql_lease_mgr.cc +++ b/src/lib/dhcpsrv/pgsql_lease_mgr.cc @@ -14,7 +14,6 @@ #include -#include #include #include #include @@ -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(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"); diff --git a/src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc b/src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc index 972b6e5d53..846808da1f 100644 --- a/src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc +++ b/src/lib/dhcpsrv/tests/dbaccess_parser_unittest.cc @@ -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", diff --git a/src/lib/dhcpsrv/tests/mysql_host_data_source_unittest.cc b/src/lib/dhcpsrv/tests/mysql_host_data_source_unittest.cc index 7ec4e4e3b3..9e01fda320 100644 --- a/src/lib/dhcpsrv/tests/mysql_host_data_source_unittest.cc +++ b/src/lib/dhcpsrv/tests/mysql_host_data_source_unittest.cc @@ -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( diff --git a/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc index 20225398b9..7f70d24f83 100755 --- a/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/mysql_lease_mgr_unittest.cc @@ -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( diff --git a/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc b/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc index 5dc97e4fef..dddf322abf 100755 --- a/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/pgsql_lease_mgr_unittest.cc @@ -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)), diff --git a/src/lib/dhcpsrv/testutils/schema.cc b/src/lib/dhcpsrv/testutils/schema.cc index 9dc5bab5c4..cefcbdae5c 100644 --- a/src/lib/dhcpsrv/testutils/schema.cc +++ b/src/lib/dhcpsrv/testutils/schema.cc @@ -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); } diff --git a/src/lib/dhcpsrv/testutils/schema.h b/src/lib/dhcpsrv/testutils/schema.h index b6aa96410f..5a0471512f 100644 --- a/src/lib/dhcpsrv/testutils/schema.h +++ b/src/lib/dhcpsrv/testutils/schema.h @@ -8,6 +8,7 @@ #define SCHEMA_H #include +#include #include 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); }; }; };