From 34e88d731226d9f65a057cc6f1340e8daaffb4c5 Mon Sep 17 00:00:00 2001 From: Thomas Markwalder Date: Tue, 12 Nov 2024 12:06:56 -0500 Subject: [PATCH] [#2736] Warn on additional and lifetime params Updated the ARM: /doc/sphinx/arm/dhcp4-srv.rst /doc/sphinx/arm/dhcp6-srv.rst Added ChangeLog /src/lib/dhcpsrv/dhcpsrv_messages.* DHCPSRV_CLASS_WITH_ADDTIONAL_AND_LIFETIMES - new message /src/lib/dhcpsrv/parsers/client_class_def_parser.cc ClientClassDefParser::parse() - now emits WARN log /src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc TEST_F(ClientClassDefParserTest, addtionalWithLifetimes4) TEST_F(ClientClassDefParserTest, addtionalWithLifetimes6) - new tests --- ChangeLog | 8 + doc/sphinx/arm/dhcp4-srv.rst | 8 +- doc/sphinx/arm/dhcp6-srv.rst | 6 + src/lib/dhcpsrv/dhcpsrv_messages.cc | 2 + src/lib/dhcpsrv/dhcpsrv_messages.h | 1 + src/lib/dhcpsrv/dhcpsrv_messages.mes | 9 ++ .../parsers/client_class_def_parser.cc | 8 + .../tests/client_class_def_parser_unittest.cc | 153 +++++++++++++++++- 8 files changed, 193 insertions(+), 2 deletions(-) diff --git a/ChangeLog b/ChangeLog index c77fc9129c..29bad8f83d 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,11 @@ +2304. [func] tmark + Both kea-dhcp4 and kea-dhcp6 will now emit a warning + log message when classes are configured with both + ``only-in-additional-list`` true and parameter(s) + that normally impact lease lifetimes (e.g. 'valid- + lifetime', 'preferred-lifetime`). + (Gitlab #2736) + 2303. [bug] tmark Modified both kea-dhcp4 and kea-dhcp6 to avoid generating DDNS update requests when leases are diff --git a/doc/sphinx/arm/dhcp4-srv.rst b/doc/sphinx/arm/dhcp4-srv.rst index ddeddbf39a..bbb428e081 100644 --- a/doc/sphinx/arm/dhcp4-srv.rst +++ b/doc/sphinx/arm/dhcp4-srv.rst @@ -3483,6 +3483,12 @@ Since Kea version 2.7.4 additional classes configured without a test expression are unconditionally added, i.e. they are considered to always be evaluated to ``true``. +.. note:: + Because additional evaluation occurs after lease assignment, parameters + that would otherwise impact lease life times (e.g. ``valid-lifetime``, + ``offer-lifetime``) will have no effect when specified in a class that + also sets ``only-in-additional-list`` true. + .. note:: As of Kea version 2.7.4, ``only-if-required`` and ``require-client-classes`` @@ -8021,7 +8027,7 @@ The following standards are currently supported in Kea: Client Configuration (CCC) Option*, `RFC 3594 `__: The Security Ticket Control sub-option is supported. - + - *Key Distribution Center (KDC) Server Address Sub-option for the Dynamic Host Configuration Protocol (DHCP) CableLabs Client Configuration (CCC) Option*, `RFC 3634 diff --git a/doc/sphinx/arm/dhcp6-srv.rst b/doc/sphinx/arm/dhcp6-srv.rst index 6cae8527b2..4ea40f4321 100644 --- a/doc/sphinx/arm/dhcp6-srv.rst +++ b/doc/sphinx/arm/dhcp6-srv.rst @@ -3219,6 +3219,12 @@ Since Kea version 2.7.4 additional classes configured without a test expression are unconditionally added, i.e. they are considered to always be evaluated to ``true``. +.. note:: + Because additional evaluation occurs after lease assignment, parameters + that would otherwise impact lease life times (e.g. ``valid-lifetime``, + ``preferred-lifetime``) will have no effect when specified in a class that + also sets ``only-in-additional-list`` true. + .. note:: As of Kea version 2.7.4, ``only-if-required`` and ``require-client-classes`` diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.cc b/src/lib/dhcpsrv/dhcpsrv_messages.cc index b2f7b9088d..34e868c03b 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.cc +++ b/src/lib/dhcpsrv/dhcpsrv_messages.cc @@ -46,6 +46,7 @@ extern const isc::log::MessageID DHCPSRV_CFGMGR_UPDATE_SUBNET6 = "DHCPSRV_CFGMGR extern const isc::log::MessageID DHCPSRV_CFGMGR_USE_ADDRESS = "DHCPSRV_CFGMGR_USE_ADDRESS"; extern const isc::log::MessageID DHCPSRV_CFGMGR_USE_ALLOCATOR = "DHCPSRV_CFGMGR_USE_ALLOCATOR"; extern const isc::log::MessageID DHCPSRV_CFGMGR_USE_UNICAST = "DHCPSRV_CFGMGR_USE_UNICAST"; +extern const isc::log::MessageID DHCPSRV_CLASS_WITH_ADDTIONAL_AND_LIFETIMES = "DHCPSRV_CLASS_WITH_ADDTIONAL_AND_LIFETIMES"; extern const isc::log::MessageID DHCPSRV_CLOSE_DB = "DHCPSRV_CLOSE_DB"; extern const isc::log::MessageID DHCPSRV_DDNS_TTL_PERCENT_TOO_SMALL = "DHCPSRV_DDNS_TTL_PERCENT_TOO_SMALL"; extern const isc::log::MessageID DHCPSRV_DHCP4O6_RECEIVED_BAD_PACKET = "DHCPSRV_DHCP4O6_RECEIVED_BAD_PACKET"; @@ -215,6 +216,7 @@ const char* values[] = { "DHCPSRV_CFGMGR_USE_ADDRESS", "listening on address %1, on interface %2", "DHCPSRV_CFGMGR_USE_ALLOCATOR", "using the %1 allocator for %2 leases in subnet %3", "DHCPSRV_CFGMGR_USE_UNICAST", "listening on unicast address %1, on interface %2", + "DHCPSRV_CLASS_WITH_ADDTIONAL_AND_LIFETIMES", "class: %1 has 'only-in-additional-list' true while specifying one or more lease life time values. Life time values will be ignored.", "DHCPSRV_CLOSE_DB", "closing currently open %1 database", "DHCPSRV_DDNS_TTL_PERCENT_TOO_SMALL", "ddns-ttl-percent %1 of lease lifetime %2 is too small, ignoring it", "DHCPSRV_DHCP4O6_RECEIVED_BAD_PACKET", "received bad DHCPv4o6 packet: %1", diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.h b/src/lib/dhcpsrv/dhcpsrv_messages.h index 4c7bf154fa..a0ae287b01 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.h +++ b/src/lib/dhcpsrv/dhcpsrv_messages.h @@ -47,6 +47,7 @@ extern const isc::log::MessageID DHCPSRV_CFGMGR_UPDATE_SUBNET6; extern const isc::log::MessageID DHCPSRV_CFGMGR_USE_ADDRESS; extern const isc::log::MessageID DHCPSRV_CFGMGR_USE_ALLOCATOR; extern const isc::log::MessageID DHCPSRV_CFGMGR_USE_UNICAST; +extern const isc::log::MessageID DHCPSRV_CLASS_WITH_ADDTIONAL_AND_LIFETIMES; extern const isc::log::MessageID DHCPSRV_CLOSE_DB; extern const isc::log::MessageID DHCPSRV_DDNS_TTL_PERCENT_TOO_SMALL; extern const isc::log::MessageID DHCPSRV_DHCP4O6_RECEIVED_BAD_PACKET; diff --git a/src/lib/dhcpsrv/dhcpsrv_messages.mes b/src/lib/dhcpsrv/dhcpsrv_messages.mes index e07a05bf97..cdb3a42c63 100644 --- a/src/lib/dhcpsrv/dhcpsrv_messages.mes +++ b/src/lib/dhcpsrv/dhcpsrv_messages.mes @@ -957,3 +957,12 @@ included in the message. % DHCPSRV_UNKNOWN_DB unknown database type: %1 The database access string specified a database type (given in the message) that is unknown to the software. This is a configuration error. + +% DHCPSRV_CLASS_WITH_ADDTIONAL_AND_LIFETIMES class: %1 has 'only-in-additional-list' true while specifying one or more lease life time values. Life time values will be ignored. +This warning is emitted whenever a class is configured with +'only-in-addition-list' true as well as specifying one or +more lease life time parameters (e.g. 'valid-lifetime', +'preferred-lifetime', or 'offer-lifetime'). Additional list classes +are evaluated after lease assignment, thus parameters that would otherwise +impact lease life times will have no affect. + diff --git a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc index 07153b5343..3245786b6d 100644 --- a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc +++ b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc @@ -292,6 +292,14 @@ ClientClassDefParser::parse(ClientClassDictionaryPtr& class_dictionary, // depend_on_known is now allowed } + if (additional && + (!valid_lft.unspecified() || + !preferred_lft.unspecified() || + !offer_lft.unspecified())) { + LOG_WARN(dhcpsrv_logger, DHCPSRV_CLASS_WITH_ADDTIONAL_AND_LIFETIMES) + .arg(name); + } + // Add the client class definition try { class_dictionary->addClass(name, match_expr, test, additional, diff --git a/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc b/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc index 9e1fb124ba..90130b60ec 100644 --- a/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc +++ b/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc @@ -16,6 +16,7 @@ #include #include #include +#include #include #include #include @@ -28,6 +29,7 @@ using namespace isc::data; using namespace isc::dhcp; using namespace isc::asiolink; using namespace isc::util; +using namespace isc::dhcp::test; namespace { @@ -101,7 +103,8 @@ protected: }; /// @brief Test fixture class for @c ClientClassDefParser. -class ClientClassDefParserTest : public ::testing::Test { +//class ClientClassDefParserTest : public ::testing::Test { +class ClientClassDefParserTest : public LogContentTest { protected: /// @brief Convenience method for parsing a configuration @@ -2192,4 +2195,152 @@ TEST_F(ClientClassDefParserTest, deprecatedOnlyIfRequired) { "'only-in-additional-list'. Use only the latter."); } +// Verifies that the parser detects use of life time parameters +// in classes that also set `only-in-additinal-list' true. +TEST_F(ClientClassDefParserTest, addtionalWithLifetimes4) { + CfgMgr::instance().setFamily(AF_INET); + boost::scoped_ptr cclass; + + struct Scenario { + size_t line_no_; + std::string cfg_; + bool should_log_; + }; + + std::list scenarios = { + { + __LINE__, + R"({ + "name": "boo", + "only-in-additional-list": true + })", + },{ + __LINE__, + R"({ + "name": "boo", + "only-in-additional-list": true, + "valid-lifetime": 100 + })", + true + },{ + __LINE__, + R"({ + "name": "boo", + "only-in-additional-list": true, + "offer-lifetime": 100 + })", + true + },{ + __LINE__, + R"({ + "name": "boo", + "only-in-additional-list": false, + "valid-lifetime": 100 + })", + false + },{ + __LINE__, + R"({ + "name": "boo", + "only-in-additional-list": false, + "offer-lifetime": 100 + })", + false + } + }; + + size_t exp_log_count = 0; + for (auto& scenario : scenarios) { + std::stringstream oss; + oss << "scenario at: " << scenario.line_no_; + SCOPED_TRACE(oss.str()); + // Parse the class definition. + auto class_def = parseClientClassDef(scenario.cfg_, AF_INET); + ASSERT_TRUE(class_def); + + // If we expect the warning log to be emitted the occurrences + // in the log file should bump by 1. + if (scenario.should_log_) { + // Veriy we emitted another instance of the log message. + ++exp_log_count; + ASSERT_EQ(countFile("DHCPSRV_CLASS_WITH_ADDTIONAL_AND_LIFETIMES"), + exp_log_count); + } + } +} + +// Verifies that the parser detects use of life time parameters +// in classes that also set `only-in-additinal-list' true. +TEST_F(ClientClassDefParserTest, addtionalWithLifetimes6) { + CfgMgr::instance().setFamily(AF_INET6); + boost::scoped_ptr cclass; + + struct Scenario { + size_t line_no_; + std::string cfg_; + bool should_log_; + }; + + std::list scenarios = { + { + __LINE__, + R"({ + "name": "boo", + "only-in-additional-list": true + })", + },{ + __LINE__, + R"({ + "name": "boo", + "only-in-additional-list": true, + "valid-lifetime": 100 + })", + true + },{ + __LINE__, + R"({ + "name": "boo", + "only-in-additional-list": true, + "preferred-lifetime": 100 + })", + true + },{ + __LINE__, + R"({ + "name": "boo", + "only-in-additional-list": false, + "valid-lifetime": 100 + })", + false + },{ + __LINE__, + R"({ + "name": "boo", + "only-in-additional-list": false, + "preferred-lifetime": 100 + })", + false + } + }; + + size_t exp_log_count = 0; + for (auto& scenario : scenarios) { + std::stringstream oss; + oss << "scenario at: " << scenario.line_no_; + SCOPED_TRACE(oss.str()); + // Parse the class definition. + auto class_def = parseClientClassDef(scenario.cfg_, AF_INET6); + ASSERT_TRUE(class_def); + + // If we expect the warning log to be emitted the occurrences + // in the log file should bump by 1. + if (scenario.should_log_) { + // Veriy we emitted another instance of the log message. + ++exp_log_count; + ASSERT_EQ(countFile("DHCPSRV_CLASS_WITH_ADDTIONAL_AND_LIFETIMES"), + exp_log_count); + } + } +} + } // end of anonymous namespace