[5207] Improved duplication checks when adding reservations

src/lib/dhcpsrv/cfg_hosts.cc

@@ -573,7 +573,9 @@ CfgHosts::add(const HostPtr& host) {
 
 void
 CfgHosts::add4(const HostPtr& host) {
-    /// @todo This may need further sanity checks.
+    /// @todo: Should we add this host at all if IPv4 subnet-id is 0?
+    /// Why, if it's IPv6-only host?
+
     HWAddrPtr hwaddr = host->getHWAddress();
     DuidPtr duid = host->getDuid();
 
@@ -633,7 +635,16 @@ CfgHosts::add4(const HostPtr& host) {
                   << ": There's already a reservation for this address");
     }
 
-    /// @todo This may need further sanity checks.
+    // Check if the (identifier type, identifier) tuple is already used.
+    const std::vector<uint8_t>& id = host->getIdentifier();
+    if ((host->getIPv4SubnetID() > 0) && !id.empty()) {
+        if (get4(host->getIPv4SubnetID(), host->getIdentifierType(), &id[0],
+                 id.size())) {
+            isc_throw(DuplicateHost, "failed to add duplicate host using identifier: "
+                      << Host::getIdentifierAsText(host->getIdentifierType(),
+                                                   &id[0], id.size()));
+        }
+    }
 
     // This is a new instance - add it.
     hosts_.insert(host);
@@ -641,13 +652,26 @@ CfgHosts::add4(const HostPtr& host) {
 
 void
 CfgHosts::add6(const HostPtr& host) {
-    /// @todo This may need further sanity checks.
+
+    /// @todo: Should we add this host at all if IPv6 subnet-id is 0?
+    /// Why, if it's IPv4-only host?
     HWAddrPtr hwaddr = host->getHWAddress();
     DuidPtr duid = host->getDuid();
 
     // Get all reservations for this host.
     IPv6ResrvRange reservations = host->getIPv6Reservations();
 
+    // Check if the (identifier type, identifier) tuple is already used.
+    const std::vector<uint8_t>& id = host->getIdentifier();
+    if ((host->getIPv6SubnetID() > 0) && !id.empty()) {
+        if (get6(host->getIPv4SubnetID(), host->getIdentifierType(), &id[0],
+                 id.size())) {
+            isc_throw(DuplicateHost, "failed to add duplicate host using identifier: "
+                      << Host::getIdentifierAsText(host->getIdentifierType(),
+                                                   &id[0], id.size()));
+        }
+    }
+
     // Check if there are any IPv6 reservations.
     if (std::distance(reservations.first, reservations.second) == 0) {
         // If there aren't, we don't need to add this to hosts6_, which is used

src/lib/dhcpsrv/tests/host_reservations_list_parser_unittest.cc

@@ -212,6 +212,7 @@ TEST_F(HostReservationsListParserTest, duplicatedIdentifierValue4) {
     identifiers.push_back("hw-address");
     identifiers.push_back("duid");
     identifiers.push_back("circuit-id");
+    identifiers.push_back("flex-id");
 
     for (unsigned int i = 0; i < identifiers.size(); ++i) {
         SCOPED_TRACE("Using identifier " + identifiers[i]);
@@ -236,11 +237,17 @@ TEST_F(HostReservationsListParserTest, duplicatedIdentifierValue4) {
         HostCollection hosts;
         HostReservationsListParser<HostReservationParser4> parser;
         EXPECT_THROW({
-            parser.parse(SubnetID(1), config_element, hosts);
-            for (auto h = hosts.begin(); h != hosts.end(); ++h) {
-                CfgMgr::instance().getStagingCfg()->getCfgHosts()->add(*h);
-            }
-        }, isc::Exception);
+                parser.parse(SubnetID(1), config_element, hosts);
+                for (auto h = hosts.begin(); h != hosts.end(); ++h) {
+                    CfgMgr::instance().getStagingCfg()->getCfgHosts()->add(*h);
+                }
+            }, DuplicateHost);
+        // The code threw exception, because the second insertion failed.
+        // Nevertheless, the first host insertion succeeded, so the next
+        // time we try to insert them, we will get ReservedAddress exception,
+        // rather than DuplicateHost. Therefore we need to remove the
+        // first host that's still there.
+        CfgMgr::instance().clear();
     }
 }
 
@@ -331,6 +338,7 @@ TEST_F(HostReservationsListParserTest, duplicatedIdentifierValue6) {
     std::vector<std::string> identifiers;
     identifiers.push_back("hw-address");
     identifiers.push_back("duid");
+    identifiers.push_back("flex-id");
 
     for (unsigned int i = 0; i < identifiers.size(); ++i) {
         SCOPED_TRACE("Using identifier " + identifiers[i]);
@@ -359,7 +367,7 @@ TEST_F(HostReservationsListParserTest, duplicatedIdentifierValue6) {
             for (auto h = hosts.begin(); h != hosts.end(); ++h) {
                 CfgMgr::instance().getStagingCfg()->getCfgHosts()->add(*h);
             }
-        }, isc::Exception);
+        }, DuplicateHost);
     }
 }