[#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

Author: tmarkwalder

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

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
    <https://tools.ietf.org/html/rfc3594>`__: 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

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``

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",

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;

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.
+

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,

src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc

@@ -16,6 +16,7 @@
 #include <asiolink/io_address.h>
 #include <eval/evaluate.h>
 #include <testutils/gtest_utils.h>
+#include <testutils/log_utils.h>
 #include <gtest/gtest.h>
 #include <sstream>
 #include <stdint.h>
@@ -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 @@ class ExpressionParserTest : public ::testing::Test {
 };
 
 /// @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<ClientClassDef> cclass;
+
+    struct Scenario {
+        size_t line_no_;
+        std::string cfg_;
+        bool should_log_;
+    };
+
+    std::list<Scenario> 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<ClientClassDef> cclass;
+
+    struct Scenario {
+        size_t line_no_;
+        std::string cfg_;
+        bool should_log_;
+    };
+
+    std::list<Scenario> 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