From fd124e8de667039c0f48def84b797783a5590f3d Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Thu, 12 Jan 2023 17:34:58 +0100 Subject: [PATCH] [#2688] Added tcp_user_timeout parameter --- src/lib/database/dbaccess_parser.cc | 14 ++ .../tests/dbaccess_parser_unittest.cc | 52 +++++++ src/lib/database/testutils/schema.cc | 5 +- src/lib/database/testutils/schema.h | 3 + src/lib/mysql/mysql_connection.cc | 10 +- src/lib/pgsql/pgsql_connection.cc | 131 +++++++++--------- src/lib/pgsql/pgsql_connection.h | 31 +++++ .../pgsql/tests/pgsql_connection_unittest.cc | 76 +++++++++- 8 files changed, 248 insertions(+), 74 deletions(-) diff --git a/src/lib/database/dbaccess_parser.cc b/src/lib/database/dbaccess_parser.cc index e3d14b485f..3f54098b46 100644 --- a/src/lib/database/dbaccess_parser.cc +++ b/src/lib/database/dbaccess_parser.cc @@ -51,6 +51,7 @@ DbAccessParser::parse(std::string& access_string, int64_t connect_timeout = 0; int64_t read_timeout = 0; int64_t write_timeout = 0; + int64_t tcp_user_timeout = 0; int64_t port = 0; int64_t max_reconnect_tries = 0; int64_t reconnect_wait_time = 0; @@ -84,6 +85,11 @@ DbAccessParser::parse(std::string& access_string, values_copy[param.first] = boost::lexical_cast(write_timeout); + } else if (param.first == "tcp-user-timeout") { + tcp_user_timeout = param.second->intValue(); + values_copy[param.first] = + boost::lexical_cast(tcp_user_timeout); + } else if (param.first == "max-reconnect-tries") { max_reconnect_tries = param.second->intValue(); values_copy[param.first] = @@ -185,6 +191,14 @@ DbAccessParser::parse(std::string& access_string, << std::numeric_limits::max() << " (" << value->getPosition() << ")"); } + if ((tcp_user_timeout < 0) || + (tcp_user_timeout > std::numeric_limits::max())) { + ConstElementPtr value = database_config->get("tcp-user-timeout"); + isc_throw(DbConfigError, "tcp-user-timeout value: " << tcp_user_timeout + << " is out of range, expected value: 0.." + << std::numeric_limits::max() + << " (" << value->getPosition() << ")"); + } // e. Check that the port is within a reasonable range. if ((port < 0) || diff --git a/src/lib/database/tests/dbaccess_parser_unittest.cc b/src/lib/database/tests/dbaccess_parser_unittest.cc index b51b6f4b80..ec6f1a9a6d 100644 --- a/src/lib/database/tests/dbaccess_parser_unittest.cc +++ b/src/lib/database/tests/dbaccess_parser_unittest.cc @@ -169,6 +169,7 @@ private: (parameter != "connect-timeout") && (parameter != "read-timeout") && (parameter != "write-timeout") && + (parameter != "tcp-user-timeout") && (parameter != "port") && (parameter != "max-row-errors") && (parameter != "readonly")); @@ -505,6 +506,57 @@ TEST_F(DbAccessParserTest, largeWriteTimeout) { EXPECT_THROW(parser.parse(json_elements), DbConfigError); } +// This test checks that the parser accepts the valid value of the +// tcp-user-timeout parameter. +TEST_F(DbAccessParserTest, validTcpUserTimeout) { + const char* config[] = {"type", "memfile", + "name", "/opt/var/lib/kea/kea-leases6.csv", + "tcp-user-timeout", "3600", + NULL}; + + string json_config = toJson(config); + ConstElementPtr json_elements = Element::fromJSON(json_config); + EXPECT_TRUE(json_elements); + + TestDbAccessParser parser; + EXPECT_NO_THROW(parser.parse(json_elements)); + checkAccessString("Valid write timeout", parser.getDbAccessParameters(), + config); +} + +// This test checks that the parser rejects the negative value of the +// tcp-user-timeout parameter. +TEST_F(DbAccessParserTest, negativeTcpUserTimeout) { + const char* config[] = {"type", "memfile", + "name", "/opt/var/lib/kea/kea-leases6.csv", + "tcp-user-timeout", "-1", + NULL}; + + string json_config = toJson(config); + ConstElementPtr json_elements = Element::fromJSON(json_config); + EXPECT_TRUE(json_elements); + + TestDbAccessParser parser; + EXPECT_THROW(parser.parse(json_elements), DbConfigError); +} + +// This test checks that the parser rejects a too large (greater than +// the max uint32_t) value of the tcp-user-timeout parameter. +TEST_F(DbAccessParserTest, largeTcpUserTimeout) { + const char* config[] = {"type", "memfile", + "name", "/opt/var/lib/kea/kea-leases6.csv", + "tcp-user-timeout", "4294967296", + NULL}; + + string json_config = toJson(config); + ConstElementPtr json_elements = Element::fromJSON(json_config); + EXPECT_TRUE(json_elements); + + TestDbAccessParser parser; + EXPECT_THROW(parser.parse(json_elements), DbConfigError); +} + + // This test checks that the parser accepts the valid value of the // port parameter. TEST_F(DbAccessParserTest, validPort) { diff --git a/src/lib/database/testutils/schema.cc b/src/lib/database/testutils/schema.cc index a794de6e6f..80371e5abc 100644 --- a/src/lib/database/testutils/schema.cc +++ b/src/lib/database/testutils/schema.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2016-2022 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2016-2023 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -41,6 +41,9 @@ const char* INVALID_READ_TIMEOUT_1 = "read-timeout=bar"; const char* VALID_WRITE_TIMEOUT = "write-timeout=12"; const char* VALID_WRITE_TIMEOUT_ZERO = "write-timeout=0"; const char* INVALID_WRITE_TIMEOUT_1 = "write-timeout=baz"; +const char* VALID_TCP_USER_TIMEOUT = "tcp-user-timeout=8"; +const char* VALID_TCP_USER_TIMEOUT_ZERO = "tcp-user-timeout=0"; +const char* INVALID_TCP_USER_TIMEOUT_1 = "-7"; const char* VALID_READONLY_DB = "readonly=true"; const char* INVALID_READONLY_DB = "readonly=5"; const char* VALID_CERT = "cert-file=" TEST_CA_DIR "/kea-client.crt"; diff --git a/src/lib/database/testutils/schema.h b/src/lib/database/testutils/schema.h index f278baa521..55c7b77864 100644 --- a/src/lib/database/testutils/schema.h +++ b/src/lib/database/testutils/schema.h @@ -37,6 +37,9 @@ extern const char* INVALID_READ_TIMEOUT_1; extern const char* VALID_WRITE_TIMEOUT; extern const char* VALID_WRITE_TIMEOUT_ZERO; extern const char* INVALID_WRITE_TIMEOUT_1; +extern const char* VALID_TCP_USER_TIMEOUT; +extern const char* VALID_TCP_USER_TIMEOUT_ZERO; +extern const char* INVALID_TCP_USER_TIMEOUT_1; extern const char* VALID_READONLY_DB; extern const char* INVALID_READONLY_DB; extern const char* VALID_CERT; diff --git a/src/lib/mysql/mysql_connection.cc b/src/lib/mysql/mysql_connection.cc index aadb89bab7..1b1e4fc7da 100644 --- a/src/lib/mysql/mysql_connection.cc +++ b/src/lib/mysql/mysql_connection.cc @@ -65,12 +65,7 @@ MySqlConnection::openDatabase() { } unsigned int port = 0; - setIntParameterValue("port", 0, numeric_limits::max(), port); - // The port is only valid when it is in the 0..65535 range. - // Again fall back to the default when the given value is invalid. - if (port > numeric_limits::max()) { - port = 0; - } + setIntParameterValue("port", 0, numeric_limits::max(), port); const char* user = NULL; string suser; @@ -108,6 +103,9 @@ MySqlConnection::openDatabase() { // database, a zero timeout might signify something like "wait // indefinitely". setIntParameterValue("connect-timeout", 1, numeric_limits::max(), connect_timeout); + // Other timeouts can be 0, meaning that the database client will follow a default + // behavior. Earlier MySQL versions didn't have these parameters, so we allow 0 + // to skip setting them. setIntParameterValue("read-timeout", 0, numeric_limits::max(), read_timeout); setIntParameterValue("write-timeout", 0, numeric_limits::max(), write_timeout); diff --git a/src/lib/pgsql/pgsql_connection.cc b/src/lib/pgsql/pgsql_connection.cc index ae314e45aa..d7a08686c0 100644 --- a/src/lib/pgsql/pgsql_connection.cc +++ b/src/lib/pgsql/pgsql_connection.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2016-2022 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2016-2023 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -180,8 +180,8 @@ PgSqlConnection::prepareStatements(const PgSqlTaggedStatement* start_statement, } } -void -PgSqlConnection::openDatabase() { +std::string +PgSqlConnection::getConnParameters() { string dbconnparameters; string shost = "localhost"; try { @@ -192,38 +192,14 @@ PgSqlConnection::openDatabase() { dbconnparameters += "host = '" + shost + "'" ; - string sport; - try { - sport = getParameter("port"); - } catch (...) { - // No port parameter, we are going to use the default port. - sport = ""; - } + unsigned int port = 0; + setIntParameterValue("port", 0, numeric_limits::max(), port); - if (sport.size() > 0) { - unsigned int port = 0; - - // Port was given, so try to convert it to an integer. - try { - port = boost::lexical_cast(sport); - } catch (...) { - // Port given but could not be converted to an unsigned int. - // Just fall back to the default value. - port = 0; - } - - // The port is only valid when it is in the 0..65535 range. - // Again fall back to the default when the given value is invalid. - if (port > numeric_limits::max()) { - port = 0; - } - - // Add it to connection parameters when not default. - if (port > 0) { - std::ostringstream oss; - oss << port; - dbconnparameters += " port = " + oss.str(); - } + // Add port to connection parameters when not default. + if (port > 0) { + std::ostringstream oss; + oss << port; + dbconnparameters += " port = " + oss.str(); } string suser; @@ -252,46 +228,36 @@ PgSqlConnection::openDatabase() { } unsigned int connect_timeout = PGSQL_DEFAULT_CONNECTION_TIMEOUT; - string stimeout; + unsigned int tcp_user_timeout = 0; 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 could not be converted to an unsigned int. Set - // the connection timeout to an invalid value to trigger throwing - // of an exception. - connect_timeout = 0; - } - // The timeout is only valid if greater than zero, as depending on the // database, a zero timeout might signify something like "wait // indefinitely". - // - // The check below also rejects a value greater than the maximum - // integer value. The lexical_cast operation used to obtain a numeric - // value from a string can get confused if trying to convert a negative - // integer to an unsigned int: instead of throwing an exception, it may - // produce a large positive value. - if ((connect_timeout == 0) || - (connect_timeout > numeric_limits::max())) { - isc_throw(DbInvalidTimeout, "database connection timeout (" << - stimeout << ") must be an integer greater than 0"); - } + setIntParameterValue("connect-timeout", 1, numeric_limits::max(), connect_timeout); + // This timeout value can be 0, meaning that the database client will + // follow a default behavior. Earlier Postgres versions didn't have + // this parameter, so we allow 0 to skip setting them for these + // earlier versions. + setIntParameterValue("tcp-user-timeout", 0, numeric_limits::max(), tcp_user_timeout); + + } catch (const std::exception& ex) { + isc_throw(DbInvalidTimeout, ex.what()); } + // Append timeouts. std::ostringstream oss; - oss << connect_timeout; - dbconnparameters += " connect_timeout = " + oss.str(); + oss << " connect_timeout = " << connect_timeout; + if (tcp_user_timeout > 0) { + oss << " tcp_user_timeout = " << tcp_user_timeout * 1000; + } + dbconnparameters += oss.str(); + return (dbconnparameters); +} + +void +PgSqlConnection::openDatabase() { + std::string dbconnparameters = getConnParameters(); // Connect to Postgres, saving the low level connection pointer // in the holder object PGconn* new_conn = PQconnectdb(dbconnparameters.c_str()); @@ -533,5 +499,38 @@ PgSqlConnection::updateDeleteQuery(PgSqlTaggedStatement& statement, return (boost::lexical_cast(PQcmdTuples(*result_set))); } +template +void +PgSqlConnection::setIntParameterValue(const std::string& name, int64_t min, int64_t max, T& value) { + string svalue; + try { + svalue = getParameter(name); + } catch (...) { + // Do nothing if the parameter is not present. + } + if (svalue.empty()) { + return; + } + try { + // Try to convert the value. + auto parsed_value = boost::lexical_cast(svalue); + // Check if the value is within the specified range. + if ((parsed_value < min) || (parsed_value > max)) { + isc_throw(BadValue, "bad " << svalue << " value"); + } + // Everything is fine. Return the parsed value. + value = parsed_value; + + } catch (...) { + // We may end up here when lexical_cast fails or when the + // parsed value is not within the desired range. In both + // cases let's throw the same general error. + isc_throw(BadValue, name << " parameter (" << + svalue << ") must be an integer between " + << min << " and " << max); + } +} + + } // end of isc::db namespace } // end of isc namespace diff --git a/src/lib/pgsql/pgsql_connection.h b/src/lib/pgsql/pgsql_connection.h index 65e8e42ada..eede3db055 100644 --- a/src/lib/pgsql/pgsql_connection.h +++ b/src/lib/pgsql/pgsql_connection.h @@ -265,6 +265,16 @@ public: void prepareStatements(const PgSqlTaggedStatement* start_statement, const PgSqlTaggedStatement* end_statement); + /// @brief Creates connection string from specified parameters. + /// + /// This function is called frin the @c openDatabase and from the unit + /// tests. + /// + /// @return connection string for @c openDatabase. + /// @throw NoDatabaseName Mandatory database name not given + /// @throw DbInvalidTimeout when the database timeout is wrong. + std::string getConnParameters(); + /// @brief Open Database /// /// Opens the database using the information supplied in the parameters @@ -500,6 +510,27 @@ public: return (conn_); } +private: + + /// @brief Convenience function parsing and setting an integer parameter, + /// if it exists. + /// + /// If the parameter is not present, this function doesn't change the @c value. + /// Otherwise, it tries to convert the parameter to the type @c T. Finally, + /// it checks if the converted number is within the specified range. + /// + /// @param name Parameter name. + /// @param min Expected minimal value. + /// @param max Expected maximal value. + /// @param [out] value Reference to a value returning the parsed parameter. + /// @tparam T Parameter type. + /// @throw BadValue if the parameter is not a valid number or if it is out + /// of range. + template + void setIntParameterValue(const std::string& name, int64_t min, int64_t max, T& value); + +public: + /// @brief Accessor function which returns the IOService that can be used to /// recover the connection. /// diff --git a/src/lib/pgsql/tests/pgsql_connection_unittest.cc b/src/lib/pgsql/tests/pgsql_connection_unittest.cc index b3b23e2fc0..897904a7ac 100644 --- a/src/lib/pgsql/tests/pgsql_connection_unittest.cc +++ b/src/lib/pgsql/tests/pgsql_connection_unittest.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2021-2022 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2021-2023 Internet Systems Consortium, Inc. ("ISC") // // This Source Code Form is subject to the terms of the Mozilla Public // License, v. 2.0. If a copy of the MPL was not distributed with this @@ -562,4 +562,78 @@ TEST_F(PgSqlConnectionTest, savepoints) { TestRowSet three_rows{{1, "one"}, {2, "two"}, {3, "three"}}; ASSERT_NO_THROW_LOG(testSelect(three_rows, 0, 10)); } + +// Tests that valid connection timeout is accepted. +TEST_F(PgSqlConnectionTest, connectionTimeout) { + std::string conn_str = connectionString(PGSQL_VALID_TYPE, VALID_NAME, + VALID_USER, VALID_PASSWORD, + VALID_TIMEOUT); + PgSqlConnection conn(DatabaseConnection::parse(conn_str)); + std::string parameters; + ASSERT_NO_THROW_LOG(parameters = conn.getConnParameters()); + EXPECT_TRUE(parameters.find("connect_timeout = 10") != std::string::npos) + << "parameter not found in " << parameters; +} + +// Tests that invalid timeout value type causes an error. +TEST_F(PgSqlConnectionTest, connectionTimeoutInvalid) { + std::string conn_str = connectionString(PGSQL_VALID_TYPE, VALID_NAME, + VALID_USER, VALID_PASSWORD, + INVALID_TIMEOUT_1); + PgSqlConnection conn(DatabaseConnection::parse(conn_str)); + EXPECT_THROW(conn.getConnParameters(), DbInvalidTimeout); +} + +// Tests that a negative connection timeout value causes an error. +TEST_F(PgSqlConnectionTest, connectionTimeoutInvalid2) { + std::string conn_str = connectionString(PGSQL_VALID_TYPE, VALID_NAME, + VALID_USER, VALID_PASSWORD, + INVALID_TIMEOUT_2); + PgSqlConnection conn(DatabaseConnection::parse(conn_str)); + EXPECT_THROW(conn.getConnParameters(), DbInvalidTimeout); +} + +// Tests that a zero connection timeout value causes an error. +TEST_F(PgSqlConnectionTest, connectionTimeoutInvalid3) { + std::string conn_str = connectionString(PGSQL_VALID_TYPE, VALID_NAME, + VALID_USER, VALID_PASSWORD, + INVALID_TIMEOUT_3); + PgSqlConnection conn(DatabaseConnection::parse(conn_str)); + EXPECT_THROW(conn.getConnParameters(), DbInvalidTimeout); +} + +// Tests that valid tcp user timeout is accepted. +TEST_F(PgSqlConnectionTest, tcpUserTimeout) { + std::string conn_str = connectionString(PGSQL_VALID_TYPE, VALID_NAME, + VALID_USER, VALID_PASSWORD, + VALID_TCP_USER_TIMEOUT); + PgSqlConnection conn(DatabaseConnection::parse(conn_str)); + std::string parameters; + ASSERT_NO_THROW_LOG(parameters = conn.getConnParameters()); + EXPECT_TRUE(parameters.find("tcp_user_timeout = 8000") != std::string::npos) + << "parameter not found in " << parameters; +} + +// Tests that a zero tcp user timeout is accepted. +TEST_F(PgSqlConnectionTest, tcpUserTimeoutZero) { + std::string conn_str = connectionString(PGSQL_VALID_TYPE, VALID_NAME, + VALID_USER, VALID_PASSWORD, + VALID_TCP_USER_TIMEOUT_ZERO); + PgSqlConnection conn(DatabaseConnection::parse(conn_str)); + std::string parameters; + ASSERT_NO_THROW_LOG(parameters = conn.getConnParameters()); + EXPECT_FALSE(parameters.find("tcp_user_timeout") != std::string::npos) + << "parameter found in " << parameters << " but expected to be gone"; +} + +// Tests that an invalid tcp user timeout causes an error. +TEST_F(PgSqlConnectionTest, tcpUserTimeoutInvalid) { + std::string conn_str = connectionString(PGSQL_VALID_TYPE, VALID_NAME, + VALID_USER, VALID_PASSWORD, + INVALID_TIMEOUT_1); + PgSqlConnection conn(DatabaseConnection::parse(conn_str)); + EXPECT_THROW(conn.getConnParameters(), DbInvalidTimeout); +} + + }; // namespace -- 2.47.2