From: Thomas Markwalder Date: Mon, 12 May 2025 18:58:31 +0000 (-0400) Subject: [#3831] Addressed further comments X-Git-Tag: Kea-2.7.9~49 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=c745954d522da62202f0b7098ca3ccfda6c0e0b0;p=thirdparty%2Fkea.git [#3831] Addressed further comments /doc/examples/kea6/all-keys-netconf.json /doc/examples/kea6/all-keys.json removed data-directory /doc/sphinx/arm/dhcp6-srv.rst Updated ARM /src/bin/dhcp6/dhcp6_messages.* Changed DHCP6_DATA_DIRECTORY_DEPRECATED /src/bin/dhcp6/json_config_parser.cc Removed dirExists() function parsing logic errors on invalid data-directory /src/bin/dhcp6/tests/config_parser_unittest.cc Updated data-directory tests Other minor cleanups --- diff --git a/doc/examples/kea6/all-keys-netconf.json b/doc/examples/kea6/all-keys-netconf.json index 7a980f80eb..72099171fb 100644 --- a/doc/examples/kea6/all-keys-netconf.json +++ b/doc/examples/kea6/all-keys-netconf.json @@ -1172,9 +1172,6 @@ // "subnet6" levels. "reservations-out-of-pool": false, - // Data directory. - "data-directory": "/tmp", - // Global compute T1 and T2 timers. "calculate-tee-times": true, diff --git a/doc/examples/kea6/all-keys.json b/doc/examples/kea6/all-keys.json index 1b2dc6e11d..9f9d4a3b1a 100644 --- a/doc/examples/kea6/all-keys.json +++ b/doc/examples/kea6/all-keys.json @@ -1380,9 +1380,6 @@ // "subnet6" levels. "reservations-out-of-pool": false, - // Data directory. - "data-directory": "/tmp", - // Global compute T1 and T2 timers. "calculate-tee-times": true, diff --git a/doc/sphinx/arm/dhcp6-srv.rst b/doc/sphinx/arm/dhcp6-srv.rst index bfb98fd14b..717df3dca0 100644 --- a/doc/sphinx/arm/dhcp6-srv.rst +++ b/doc/sphinx/arm/dhcp6-srv.rst @@ -319,9 +319,20 @@ that can be used to configure the memfile backend. default value of the ``persist`` parameter is ``true``, which enables writing lease updates to the lease file. -- ``name``: specifies an absolute location of the lease file in which - new leases and lease updates are recorded. The default value for - this parameter is ``"[kea-install-dir]/var/lib/kea/kea-leases6.csv"``. +- ``name``: specifies the lease file in which new leases and lease updates + are recorded. The default value for this parameter is + ``"[kea-install-dir]/var/lib/kea/kea-leases4.csv"``. + +.. note:: + + As of Kea 2.7.9, lease files may only be loaded from the data directory + determined during compilation: ``"[kea-install-dir]/var/lib/kea"``. This + path may be overridden at startup by setting the environment variable + ``KEA_DHCP_DATA_DIRECTORY`` to the desired path. If a path other than + this value is used in ``name``, Kea will emit an error and refuse to start + or, if already running, log an unrecoverable error. For ease of use in + specifying a custom file name simply omit the path component from ``name``. + - ``lfc-interval``: specifies the interval, in seconds, at which the server will perform a lease file cleanup (LFC). This removes @@ -6440,6 +6451,16 @@ memory lease file into its data directory. By default this directory is ... } +.. note:: + + As of Kea 2.7.9, ``data-directory`` is deprecated. The duid and lease + files may only be loaded from the directory determined at + compilation: ``"[kea-install-dir]/var/lib/kea"``. This path may be + overridden at startup by setting the environment variable + ``KEA_DHCP_DATA_DIRECTORY`` to the desired path. If a path other than + this value is used in ``data-directory``, Kea will emit an error and + refuse to start or, if already running, log an unrecoverable error. + .. _stateless-dhcp6: Stateless DHCPv6 (INFORMATION-REQUEST Message) diff --git a/src/bin/dhcp6/dhcp6_messages.cc b/src/bin/dhcp6/dhcp6_messages.cc index 3c3967bb78..0f05788a69 100644 --- a/src/bin/dhcp6/dhcp6_messages.cc +++ b/src/bin/dhcp6/dhcp6_messages.cc @@ -210,7 +210,7 @@ const char* values[] = { "DHCP6_CONFIG_SYNTAX_WARNING", "configuration syntax warning: %1", "DHCP6_CONFIG_UNRECOVERABLE_ERROR", "DHCPv6 server new configuration failed with an error which cannot be recovered", "DHCP6_CONFIG_UNSUPPORTED_OBJECT", "DHCPv6 server configuration includes an unsupported object: %1", - "DHCP6_DATA_DIRECTORY_DEPRECATED", "'data-directory' has been deprecated, the specified path %1 will be ignored, using %2", + "DHCP6_DATA_DIRECTORY_DEPRECATED", "'data-directory' has been deprecated and should no longer be used.", "DHCP6_DB_RECONNECT_DISABLED", "database reconnect is disabled: retries left: %1, reconnect wait time: %2, manager ID: %3, timer: %4", "DHCP6_DB_RECONNECT_FAILED", "maximum number of database reconnect attempts: %1, has been exhausted without success, manager ID: %2, timer: %3", "DHCP6_DB_RECONNECT_LOST_CONNECTION", "database connection lost: manager ID: %1, timer: %2.", diff --git a/src/bin/dhcp6/dhcp6_messages.mes b/src/bin/dhcp6/dhcp6_messages.mes index 6cabd5fd84..732a752cba 100644 --- a/src/bin/dhcp6/dhcp6_messages.mes +++ b/src/bin/dhcp6/dhcp6_messages.mes @@ -1153,10 +1153,9 @@ use it to extend their leases. As a result, they will have to go through a rebinding phase to re-acquire their leases and associate them with a new server id. -% DHCP6_DATA_DIRECTORY_DEPRECATED 'data-directory' has been deprecated, the specified path %1 will be ignored, using %2 +% DHCP6_DATA_DIRECTORY_DEPRECATED 'data-directory' has been deprecated and should no longer be used. This warning message is emitted when the configuration parsing detects that the `data-directory` parameter has been specified. This parameter -has been is no longer supported. The first argument is the path specified -the second argument is the supported path that will be used. This path +is no longer supported. This supported path, determined at compile time, may be overridden at runtime by setting the environment variable -'KEA_DHCP_DATA_DIR' +'KEA_DHCP_DATA_DIR'. diff --git a/src/bin/dhcp6/json_config_parser.cc b/src/bin/dhcp6/json_config_parser.cc index b48b9b041a..e37a9f0536 100644 --- a/src/bin/dhcp6/json_config_parser.cc +++ b/src/bin/dhcp6/json_config_parser.cc @@ -82,21 +82,6 @@ using namespace std; // namespace { -/// @brief Checks if specified directory exists. -/// -/// @param dir_path Path to a directory. -/// @throw BadValue If the directory does not exist or is not a directory. -void dirExists(const string& dir_path) { - struct stat statbuf; - if (stat(dir_path.c_str(), &statbuf) < 0) { - isc_throw(BadValue, "Bad directory '" << dir_path - << "': " << strerror(errno)); - } - if ((statbuf.st_mode & S_IFMT) != S_IFDIR) { - isc_throw(BadValue, "'" << dir_path << "' is not a directory"); - } -} - /// @brief Parser for list of RSOO options /// /// This parser handles a Dhcp6/relay-supplied-options entry. It contains a @@ -191,9 +176,15 @@ public: // Set the data directory for server id file. if (global->contains("data-directory")) { - LOG_WARN(dhcp6_logger, DHCP6_DATA_DIRECTORY_DEPRECATED) - .arg(getString(global, "data-directory")) - .arg(CfgMgr::instance().getDataDir()); + auto dd = getString(global, "data-directory"); + if (dd != CfgMgr::instance().getDataDir()) { + isc_throw(DhcpConfigError, + "'data-directory' of '" << dd << "' is invalid," + << " supported path is '" + << CfgMgr::instance().getDataDir() << "'"); + } + + LOG_WARN(dhcp6_logger, DHCP6_DATA_DIRECTORY_DEPRECATED); } // Set the probation period for decline handling. @@ -495,13 +486,6 @@ processDhcp6Config(isc::data::ConstElementPtr config_set) { // Apply global options in the staging config, e.g. ip-reservations-unique global_parser.parseEarly(srv_config, mutable_cfg); - // Specific check for this global parameter. - ConstElementPtr data_directory = mutable_cfg->get("data-directory"); - if (data_directory) { - parameter_name = "data-directory"; - dirExists(data_directory->stringValue()); - } - // We need definitions first ConstElementPtr option_defs = mutable_cfg->get("option-def"); if (option_defs) { diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index 4cb46ddd4b..ab7e949dba 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -388,7 +388,7 @@ const char* PARSER_CONFIGS[] = { "}" }; -class Dhcp6ParserTest : public LogContentTest { +class Dhcp6ParserTest : public BaseServerTest { protected: // Check that no hooks libraries are loaded. This is a pre-condition for // a number of tests, so is checked in one place. As this uses an @@ -6442,31 +6442,9 @@ TEST_F(Dhcp6ParserTest, rsooBogusName) { EXPECT_TRUE(errorContainsPosition(status, "")); } -/// Check that not existent data directory returns an error. -TEST_F(Dhcp6ParserTest, notExistDataDir) { - - string config_txt = "{\n" - "\"data-directory\": \"/does-not-exist--\"\n" - "}"; - ConstElementPtr config; - ASSERT_NO_THROW(config = parseDHCP6(config_txt)); - - ConstElementPtr status; - EXPECT_NO_THROW(status = Dhcpv6SrvTest::configure(srv_, config)); - - // returned value should be 1 (error) - int rcode; - ConstElementPtr comment = parseAnswerText(rcode, status); - EXPECT_EQ(1, rcode); - string text; - ASSERT_NO_THROW(text = comment->stringValue()); - EXPECT_EQ("Bad directory '/does-not-exist--': No such file or directory", - text); -} - -/// Check that not a directory data directory returns an error. -TEST_F(Dhcp6ParserTest, notDirDataDir) { - +/// Checks that an invalid data-directory path returns an error. +TEST_F(Dhcp6ParserTest, invalidDataDir) { + CfgMgr::instance().getDataDir(true, TEST_DATA_BUILDDIR); string config_txt = "{\n" "\"data-directory\": \"/dev/null\"\n" "}"; @@ -6482,27 +6460,27 @@ TEST_F(Dhcp6ParserTest, notDirDataDir) { EXPECT_EQ(1, rcode); string text; ASSERT_NO_THROW(text = comment->stringValue()); - EXPECT_EQ("'/dev/null' is not a directory", text); + std::ostringstream os; + os << "'data-directory' of '/dev/null' is invalid, supported path is '" + << CfgMgr::instance().getDataDir() << "'"; + + EXPECT_EQ(os.str(), text); } /// Check that a valid data directory is accepted. -TEST_F(Dhcp6ParserTest, testDataDir) { - string original_datadir(CfgMgr::instance().getDataDir()); - string config_txt = "{\n" - "\"data-directory\": \"/tmp\"\n" - "}"; +TEST_F(Dhcp6ParserTest, validDataDir) { + std::ostringstream os; + os << "{\"data-directory\": \"" + << CfgMgr::instance().getDataDir() << "\"}"; ConstElementPtr config; - ASSERT_NO_THROW(config = parseDHCP6(config_txt)); + ASSERT_NO_THROW(config = parseDHCP6(os.str())); ConstElementPtr status; EXPECT_NO_THROW(status = Dhcpv6SrvTest::configure(srv_, config)); // returned value should be 0 (success); checkResult(status, 0); - - // The value of data-directory should not have been updated. - ASSERT_EQ(CfgMgr::instance().getDataDir(), std::string(DHCP_DATA_DIR)); } /// Check that the dhcp4o6-port default value has a default value if not diff --git a/src/lib/testutils/env_var_wrapper.h b/src/lib/testutils/env_var_wrapper.h index 80b19178f4..48f9bce608 100644 --- a/src/lib/testutils/env_var_wrapper.h +++ b/src/lib/testutils/env_var_wrapper.h @@ -12,7 +12,8 @@ namespace isc { namespace test { -/// @brief Wrapper for environment variable that restores +/// @brief Wrapper for environment variable that restores the +/// variable to its original. class EnvVarWrapper { public: /// @brief Constructor @@ -37,7 +38,7 @@ public: /// /// @param value new value of the variable. If empty the /// variable is unset. - void setValue(const std::string value = "" ); + void setValue(const std::string value = ""); private: /// @brief Name of the variable. diff --git a/src/lib/util/filesystem.cc b/src/lib/util/filesystem.cc index 897f8ea958..2333236a5d 100644 --- a/src/lib/util/filesystem.cc +++ b/src/lib/util/filesystem.cc @@ -240,9 +240,9 @@ PathChecker::getPath(bool reset /* = false */, path_ = default_path_; } - // Remove the trailing "/" if it present so comparison to + // Remove the trailing "/" if it is present so comparison to // other Path::parentPath() works. - if (path_.back() == '/') { + while (!path_.empty() && path_.back() == '/') { path_.pop_back(); } } diff --git a/src/lib/util/filesystem.h b/src/lib/util/filesystem.h index ee00b691eb..153c1502d7 100644 --- a/src/lib/util/filesystem.h +++ b/src/lib/util/filesystem.h @@ -174,10 +174,10 @@ public: /// 3. Use the value of default path. /// /// @param reset recalculate when true, defaults to false. - /// @param explicit_path set default hooks path to this value. This is + /// @param explicit_path set the default path to this value. This is /// for testing purposes only. /// - /// @return String containing the default hooks path. + /// @return String containing the default path. std::string getPath(bool reset = false, const std::string explicit_path = ""); /// @brief Validates a file path against a supported path.