[#2961] Checkpoint: updated code, tests and doc

fxdupont · fxdupont · commit 5b60e5fbba47 · 2024-05-13T18:23:59.000+02:00
diff --git a/doc/sphinx/arm/dhcp4-srv.rst b/doc/sphinx/arm/dhcp4-srv.rst
@@ -1207,26 +1207,8 @@ IPv4 Subnet Identifier
 
 The subnet identifier (subnet ID) is a unique number associated with a particular
 subnet. In principle, it is used to associate clients' leases with their
-respective subnets. The server configuration should contain unique and stable
-identifiers for all subnets. When a subnet identifier is not specified for a
-subnet, it is automatically assigned by the configuration mechanism. The identifiers
-are assigned starting at 1 and are monotonically increased for each subsequent
-subnet: 1, 2, 3, ....
-
-If there are multiple subnets configured with auto-generated identifiers
-and one of them is removed, the subnet identifiers may be renumbered.
-For example: if there are four subnets and the third is removed, the
-last subnet will be assigned the identifier that the third subnet had
-before removal. As a result, the leases stored in the lease database for
-subnet 3 are now associated with subnet 4, something that may have
-unexpected consequences. It is one of the reasons why auto-generated subnet
-identifiers are deprecated starting from Kea version 2.4.0.
-
-.. note::
-
-   The auto-generation of the subnet identifiers will be removed in a future
-   release. Starting from Kea 2.4.0, a subnet without an ``id`` entry
-   or with the zero value raises a warning at the configuration time.
+respective subnets. The server configuration must contain unique and stable
+identifiers for all subnets.
 
 .. note::
 
@@ -1247,10 +1229,6 @@ to a newly configured subnet:
        ]
    }
 
-This identifier will not change for this subnet unless the ``id``
-parameter is removed or set to 0. The value of 0 forces auto-generation
-of the subnet identifier.
-
 .. _ipv4-subnet-prefix:
 
 IPv4 Subnet Prefix
diff --git a/doc/sphinx/arm/dhcp6-srv.rst b/doc/sphinx/arm/dhcp6-srv.rst
@@ -988,26 +988,8 @@ IPv6 Subnet Identifier
 
 The subnet identifier (subnet ID) is a unique number associated with a particular
 subnet. In principle, it is used to associate clients' leases with their
-respective subnets. The server configuration should contain unique and stable
-identifiers for all subnets. When a subnet identifier is not specified for a
-subnet, it is automatically assigned by the configuration mechanism. The identifiers
-are assigned starting at 1 and are monotonically increased for each subsequent
-subnet: 1, 2, 3, ....
-
-If there are multiple subnets configured with auto-generated identifiers
-and one of them is removed, the subnet identifiers may be renumbered.
-For example: if there are four subnets and the third is removed, the
-last subnet will be assigned the identifier that the third subnet had
-before removal. As a result, the leases stored in the lease database for
-subnet 3 are now associated with subnet 4, something that may have
-unexpected consequences. It is one of the reasons why auto-generated subnet
-identifiers are deprecated starting from Kea version 2.4.0.
-
-.. note::
-
-   The auto-generation of the subnet identifiers will be removed in a future
-   release. Starting from Kea 2.4.0, a subnet without an ``id`` entry
-   or with the zero value raises a warning at the configuration time.
+respective subnets. The server configuration must contain unique and stable
+identifiers for all subnets.
 
 .. note::
 
@@ -1028,10 +1010,6 @@ to a newly configured subnet:
        ]
    }
 
-This identifier will not change for this subnet unless the ``id``
-parameter is removed or set to 0. The value of 0 forces auto-generation
-of the subnet identifier.
-
 .. _ipv6-subnet-prefix:
 
 IPv6 Subnet Prefix
diff --git a/src/bin/dhcp4/json_config_parser.cc b/src/bin/dhcp4/json_config_parser.cc
@@ -327,10 +327,6 @@ void configureCommandChannel() {
 /// @param config_set the configuration being processed
 isc::data::ConstElementPtr
 processDhcp4Config(isc::data::ConstElementPtr config_set) {
-    // Before starting any subnet operations, let's reset the subnet-id counter,
-    // so newly recreated configuration starts with first subnet-id equal 1.
-    Subnet::resetSubnetID();
-
     // Revert any runtime option definitions configured so far and not committed.
     LibDHCP::revertRuntimeOptionDefs();
     // Let's set empty container in case a user hasn't specified any configuration
diff --git a/src/bin/dhcp6/json_config_parser.cc b/src/bin/dhcp6/json_config_parser.cc
@@ -429,10 +429,6 @@ void configureCommandChannel() {
 /// @param config_set the configuration being processed
 isc::data::ConstElementPtr
 processDhcp6Config(isc::data::ConstElementPtr config_set) {
-    // Before starting any subnet operations, let's reset the subnet-id counter,
-    // so newly recreated configuration starts with first subnet-id equal 1.
-    Subnet::resetSubnetID();
-
     // Revert any runtime option definitions configured so far and not committed.
     LibDHCP::revertRuntimeOptionDefs();
     // Let's set empty container in case a user hasn't specified any configuration
diff --git a/src/lib/dhcpsrv/parsers/dhcp_parsers.cc b/src/lib/dhcpsrv/parsers/dhcp_parsers.cc
@@ -730,11 +730,9 @@ Subnet4ConfigParser::parse(ConstElementPtr subnet, bool encapsulate_options) {
 void
 Subnet4ConfigParser::initSubnet(data::ConstElementPtr params,
                                 asiolink::IOAddress addr, uint8_t len) {
-    // Subnet ID is optional. If it is not supplied the value of 0 is used,
-    // which means autogenerate. The value was inserted earlier by calling
-    // SimpleParser4::setAllDefaults.
+    // Subnet ID is required and must be in 1..SUBNET_ID_MAX.
     int64_t subnet_id_max = static_cast<int64_t>(SUBNET_ID_MAX);
-    SubnetID subnet_id = static_cast<SubnetID>(getInteger(params, "id", 0,
+    SubnetID subnet_id = static_cast<SubnetID>(getInteger(params, "id", 1,
                                                           subnet_id_max));
 
     auto subnet4 = Subnet4::create(addr, len, Triplet<uint32_t>(),
@@ -1259,11 +1257,9 @@ Subnet6ConfigParser::duplicateOptionWarning(uint32_t code,
 void
 Subnet6ConfigParser::initSubnet(data::ConstElementPtr params,
                                 asiolink::IOAddress addr, uint8_t len) {
-    // Subnet ID is optional. If it is not supplied the value of 0 is used,
-    // which means autogenerate. The value was inserted earlier by calling
-    // SimpleParser6::setAllDefaults.
+    // Subnet ID is required and must be in 1..SUBNET_ID_MAX.
     int64_t subnet_id_max = static_cast<int64_t>(SUBNET_ID_MAX);
-    SubnetID subnet_id = static_cast<SubnetID>(getInteger(params, "id", 0,
+    SubnetID subnet_id = static_cast<SubnetID>(getInteger(params, "id", 1,
                                                           subnet_id_max));
 
     // We want to log whether rapid-commit is enabled, so we get this
diff --git a/src/lib/dhcpsrv/parsers/simple_parser4.cc b/src/lib/dhcpsrv/parsers/simple_parser4.cc
@@ -270,7 +270,6 @@ const SimpleKeywords SimpleParser4::SUBNET4_PARAMETERS = {
 /// defined on global level. Currently there are two such parameters:
 /// interface and reservation-mode
 const SimpleDefaults SimpleParser4::SUBNET4_DEFAULTS = {
-    { "id",               Element::integer, "0" }, // 0 means autogenerate
     { "interface",        Element::string,  "" },
     { "client-class",     Element::string,  "" },
     { "4o6-interface",    Element::string,  "" },
@@ -285,7 +284,6 @@ const SimpleDefaults SimpleParser4::SUBNET4_DEFAULTS = {
 /// that can be derived from shared-network, but cannot from global scope.
 /// Those are: interface and reservation-mode.
 const SimpleDefaults SimpleParser4::SHARED_SUBNET4_DEFAULTS = {
-    { "id",               Element::integer, "0" }, // 0 means autogenerate
     { "4o6-interface",    Element::string,  "" },
     { "4o6-interface-id", Element::string,  "" },
     { "4o6-subnet",       Element::string,  "" },
diff --git a/src/lib/dhcpsrv/parsers/simple_parser6.cc b/src/lib/dhcpsrv/parsers/simple_parser6.cc
@@ -261,7 +261,6 @@ const SimpleKeywords SimpleParser6::SUBNET6_PARAMETERS = {
 /// where a parameter can be derived from shared-networks, but is not
 /// defined on global level.
 const SimpleDefaults SimpleParser6::SUBNET6_DEFAULTS = {
-    { "id",               Element::integer, "0" }, // 0 means autogenerate
     { "interface",        Element::string,  "" },
     { "client-class",     Element::string,  "" },
     { "rapid-commit",     Element::boolean, "false" }, // rapid-commit disabled by default
@@ -274,7 +273,6 @@ const SimpleDefaults SimpleParser6::SUBNET6_DEFAULTS = {
 /// This is mostly the same as @ref SUBNET6_DEFAULTS, except the parameters
 /// that can be derived from shared-network, but cannot from global scope.
 const SimpleDefaults SimpleParser6::SHARED_SUBNET6_DEFAULTS = {
-    { "id",               Element::integer, "0" } // 0 means autogenerate
 };
 
 /// @brief This table defines default values for each IPv6 shared network.
diff --git a/src/lib/dhcpsrv/subnet.cc b/src/lib/dhcpsrv/subnet.cc
@@ -65,21 +65,14 @@ comparePoolFirstAddress(const PoolPtr& pool1,
 namespace isc {
 namespace dhcp {
 
-// This is an initial value of subnet-id. See comments in subnet.h for details.
-SubnetID Subnet::static_id_ = 1;
-
 Subnet::Subnet(const isc::asiolink::IOAddress& prefix, uint8_t len,
                const SubnetID id)
-    : id_(id == 0 ? generateNextID() : id), prefix_(prefix),
-      prefix_len_(len),
-      shared_network_name_() {
-    if ((id == 0) && (id_ == 1)) {
-        // Emit a warning on the first auto-numbered subnet.
-        LOG_WARN(dhcpsrv_logger, DHCPSRV_CONFIGURED_SUBNET_WITHOUT_ID)
-            .arg(toText());
-    }
-    if ((prefix.isV6() && len > 128) ||
-        (prefix.isV4() && len > 32)) {
+    : id_(id), prefix_(prefix), prefix_len_(len), shared_network_name_() {
+    if ((id == SUBNET_ID_GLOBAL) || (id == SUBNET_ID_UNUSED)) {
+        isc_throw(BadValue,
+                  "Invalid id specified for subnet: " << id);
+    }
+    if ((prefix.isV6() && len > 128) || (prefix.isV4() && len > 32)) {
         isc_throw(BadValue,
                   "Invalid prefix length specified for subnet: " << len);
     }
diff --git a/src/lib/dhcpsrv/subnet.h b/src/lib/dhcpsrv/subnet.h
@@ -211,15 +211,6 @@ class Subnet : public virtual Network {
     /// @return textual representation
     virtual std::string toText() const;
 
-    /// @brief Resets subnet-id counter to its initial value (1).
-    ///
-    /// This should be called during reconfiguration, before any new
-    /// subnet objects are created. It will ensure that the subnet_id will
-    /// be consistent between reconfigures.
-    static void resetSubnetID() {
-        static_id_ = 1;
-    }
-
     /// @brief Retrieves pointer to a shared network associated with a subnet.
     ///
     /// By implementing it as a template function we overcome a need to
@@ -338,15 +329,9 @@ class Subnet : public virtual Network {
     /// By making the constructor protected, we make sure that no one will
     /// ever instantiate that class. Subnet4 and Subnet6 should be used instead.
     ///
-    /// This constructor assigns a new subnet-id (see @ref generateNextID).
-    /// This subnet-id has unique value that is strictly monotonously increasing
-    /// for each subnet, until it is explicitly reset back to 1 during
-    /// reconfiguration process.
-    ///
     /// @param prefix subnet prefix
     /// @param len prefix length for the subnet
-    /// @param id arbitrary subnet id, value of 0 triggers autogeneration
-    /// of subnet id
+    /// @param id arbitrary subnet id between 0 and 2^32-1 excluded.
     Subnet(const isc::asiolink::IOAddress& prefix, uint8_t len,
            const SubnetID id);
 
@@ -356,31 +341,6 @@ class Subnet : public virtual Network {
     /// derive from this class.
     virtual ~Subnet() { };
 
-    /// @brief keeps the subnet-id value.
-    ///
-    /// It is incremented every time a new Subnet object is created. It is reset
-    /// (@ref resetSubnetID) every time reconfiguration
-    /// occurs.
-    ///
-    /// Static value initialized in subnet.cc.
-    static SubnetID static_id_;
-
-    /// @brief returns the next unique Subnet-ID.
-    ///
-    /// This method generates and returns the next unique subnet-id.
-    /// It is a strictly monotonously increasing value (1,2,3,...) for
-    /// each new Subnet object created. It can be explicitly reset
-    /// back to 1 during reconfiguration (@ref resetSubnetID).
-    ///
-    /// @return the next unique Subnet-ID
-    static SubnetID generateNextID() {
-        if (static_id_ == SUBNET_ID_MAX) {
-            resetSubnetID();
-        }
-
-        return (static_id_++);
-    }
-
     /// @brief Checks if used pool type is valid.
     ///
     /// Allowed type for Subnet4 is Pool::TYPE_V4.
diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.cc b/src/lib/dhcpsrv/tests/alloc_engine_utils.cc
@@ -171,9 +171,6 @@ AllocEngine4Test::generateDeclinedLease(const std::string& addr,
 }
 
 AllocEngine6Test::AllocEngine6Test() {
-    // No longer used but this means too that tests relied far too much on it.
-    //Subnet::resetSubnetID();
-
     CfgMgr::instance().clear();
 
     // This lease mgr needs to exist to before configuration commits.
@@ -649,9 +646,6 @@ AllocEngine4Test::initSubnet(const asiolink::IOAddress& pool_start,
 }
 
 AllocEngine4Test::AllocEngine4Test() {
-    // No longer used but this means too that tests relied far too much on it.
-    //Subnet::resetSubnetID();
-
     CfgMgr::instance().clear();
 
     // This lease mgr needs to exist to before configuration commits.
diff --git a/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc b/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc
@@ -1298,6 +1298,78 @@ TEST(CfgSubnets4Test, hasSubnetWithServerId) {
     EXPECT_FALSE(cfg.hasSubnetWithServerId(IOAddress("2.3.4.5")));
 }
 
+// This test verifies the Subnet4 parser's validation logic for id parameter.
+TEST(CfgSubnets4Test, idValidation) {
+    {
+        // id 0.
+        SCOPED_TRACE("id 0");
+        std::string json =
+            "        {\n"
+            "            \"id\": 0,\n"
+            "            \"subnet\": \"10.1.2.0/24\"\n"
+            "        }\n";
+        data::ElementPtr elems;
+        ASSERT_NO_THROW(elems = data::Element::fromJSON(json))
+            << "invalid JSON:" << json  << "\n test is broken";
+        std::string expected = "subnet configuration failed: ";
+        expected += "The 'id' value (0) is not within ";
+        expected += "expected range: (1 - 4294967294)";
+        try {
+            // Attempt to parse the configuration.
+            Subnet4ConfigParser parser;
+            Subnet4Ptr subnet = parser.parse(elems);
+            ADD_FAILURE() << "expected exception";
+        } catch (const std::exception& ex) {
+            EXPECT_EQ(expected, ex.what());
+        }
+    }
+
+    {
+        // id 4294967295.
+        SCOPED_TRACE("id 4294967295");
+        std::string json =
+            "        {\n"
+            "            \"id\": 4294967295,\n"
+            "            \"subnet\": \"10.1.2.0/24\"\n"
+            "        }\n";
+        data::ElementPtr elems;
+        ASSERT_NO_THROW(elems = data::Element::fromJSON(json))
+            << "invalid JSON:" << json  << "\n test is broken";
+        std::string expected = "subnet configuration failed: ";
+        expected += "The 'id' value (4294967295) is not within ";
+        expected += "expected range: (1 - 4294967294)";
+        try {
+            // Attempt to parse the configuration.
+            Subnet4ConfigParser parser;
+            Subnet4Ptr subnet = parser.parse(elems);
+            ADD_FAILURE() << "expected exception";
+        } catch (const std::exception& ex) {
+            EXPECT_EQ(expected, ex.what());
+        }
+    }
+
+    {
+        // id 1 (valid).
+        SCOPED_TRACE("id 1");
+        std::string json =
+            "        {\n"
+            "            \"id\": 1,\n"
+            "            \"subnet\": \"10.1.2.0/24\"\n"
+            "        }\n";
+        data::ElementPtr elems;
+        ASSERT_NO_THROW(elems = data::Element::fromJSON(json))
+            << "invalid JSON:" << json  << "\n test is broken";
+        try {
+            // Attempt to parse the configuration.
+            Subnet4ConfigParser parser;
+            Subnet4Ptr subnet = parser.parse(elems);
+            EXPECT_TRUE(subnet);
+        } catch (const std::exception& ex) {
+            ADD_FAILURE() << "unexpected failure " << ex.what();
+        }
+    }
+}
+
 // This test verifies the Subnet4 parser's validation logic for
 // t1-percent and t2-percent parameters.
 TEST(CfgSubnets4Test, teeTimePercentValidation) {
diff --git a/src/lib/dhcpsrv/tests/cfg_subnets6_unittest.cc b/src/lib/dhcpsrv/tests/cfg_subnets6_unittest.cc
diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc
diff --git a/src/lib/dhcpsrv/tests/subnet_unittest.cc b/src/lib/dhcpsrv/tests/subnet_unittest.cc
