[#2795] addressed review comments

diff --git a/src/lib/dhcpsrv/mysql_host_data_source.cc b/src/lib/dhcpsrv/mysql_host_data_source.cc
index 95bcbef97f96cac940b4bb0f74dda6f0b6f2ae51..6995d068ed3c8db46f6dadaf6bdd4dabc17496e6 100644 (file)
--- a/src/lib/dhcpsrv/mysql_host_data_source.cc
+++ b/src/lib/dhcpsrv/mysql_host_data_source.cc
@@ -2510,7 +2510,9 @@ TaggedStatementArray tagged_statements = { {
      "ON h.host_id = o.host_id "
      "LEFT JOIN ipv6_reservations AS r "
      "ON h.host_id = r.host_id "
-     "WHERE r.address = ? "
+     "WHERE h.host_id IN "
+     "(SELECT host_id FROM ipv6_reservations "
+     "WHERE address = ?) "
      "ORDER BY h.host_id, o.option_id, r.reservation_id"},
 
     // Retrieves host information along with the DHCPv4 options associated with

diff --git a/src/lib/dhcpsrv/pgsql_host_data_source.cc b/src/lib/dhcpsrv/pgsql_host_data_source.cc
index 106b385259c00311aba65ba63205a90e813260dd..7b1b8f0791852a5d76760bcff4f090768b03b6e6 100644 (file)
--- a/src/lib/dhcpsrv/pgsql_host_data_source.cc
+++ b/src/lib/dhcpsrv/pgsql_host_data_source.cc
@@ -1838,7 +1838,9 @@ TaggedStatementArray tagged_statements = { {
      "FROM hosts AS h "
      "LEFT JOIN dhcp6_options AS o ON h.host_id = o.host_id "
      "LEFT JOIN ipv6_reservations AS r ON h.host_id = r.host_id "
-     "WHERE r.address = $1 "
+     "WHERE h.host_id IN "
+     "  (SELECT host_id FROM ipv6_reservations "
+     "   WHERE address = $1) "
      "ORDER BY h.host_id, o.option_id, r.reservation_id"
     },
 

diff --git a/src/lib/dhcpsrv/pgsql_host_data_source.h b/src/lib/dhcpsrv/pgsql_host_data_source.h
index 9496736c7549439164e60f6ef7954586b6506719..51b4856fdd8d464e015563985405e376618ae037 100644 (file)
--- a/src/lib/dhcpsrv/pgsql_host_data_source.h
+++ b/src/lib/dhcpsrv/pgsql_host_data_source.h
@@ -451,6 +451,27 @@ public:
     getAll6(const SubnetID& subnet_id,
             const asiolink::IOAddress& address) const;
 
+    /// @brief Returns all hosts having a reservation for a specified
+    /// address or delegated prefix (lease) in all subnets.
+    ///
+    /// In most cases it is desired that there is at most one reservation
+    /// for a given IPv6 lease within a subnet. In a default configuration,
+    /// the backend does not allow for inserting more than one host with
+    /// the same IPv6 address or prefix.
+    ///
+    /// If the backend is configured to allow multiple hosts with reservations
+    /// for the same IPv6 lease in the given subnet, this method can return
+    /// more than one host per subnet.
+    ///
+    /// The typical use case when a single IPv6 lease is reserved for multiple
+    /// hosts is when these hosts represent different interfaces of the same
+    /// machine and each interface comes with a different MAC address. In that
+    /// case, the same IPv6 lease is assigned regardless of which interface is
+    /// used by the DHCP client to communicate with the server.
+    ///
+    /// @param address reserved IPv6 address/prefix.
+    ///
+    /// @return Collection of const @c Host objects.
     virtual ConstHostCollection getAll6(const asiolink::IOAddress& address) const;
 
     /// @brief Implements @ref BaseHostDataSource::update() for PostgreSQL.

diff --git a/src/lib/dhcpsrv/testutils/generic_host_data_source_unittest.cc b/src/lib/dhcpsrv/testutils/generic_host_data_source_unittest.cc
index 032118f9ec78db83c484ef943f35185d1f754b02..80a83b73592e386bde52e6926abcf4d391347c76 100644 (file)
--- a/src/lib/dhcpsrv/testutils/generic_host_data_source_unittest.cc
+++ b/src/lib/dhcpsrv/testutils/generic_host_data_source_unittest.cc
@@ -3013,6 +3013,22 @@ HostMgrTest::addHost6(BaseHostDataSource& data_source,
     data_source.add(new_host);
 }
 
+void
+HostMgrTest::addHost6(BaseHostDataSource& data_source,
+                      const DuidPtr& duid,
+                      const SubnetID& subnet_id,
+                      const std::vector<IOAddress>& addresses,
+                      const uint8_t prefix_len) {
+    HostPtr new_host(new Host(duid->toText(), "duid", SubnetID(1),
+                              subnet_id, IOAddress::IPV4_ZERO_ADDRESS()));
+    for (const IOAddress& address : addresses) {
+        new_host->addReservation(IPv6Resrv(prefix_len == 128 ? IPv6Resrv::TYPE_NA :
+                                                               IPv6Resrv::TYPE_PD,
+                                           address, prefix_len));
+    }
+
+    data_source.add(new_host);
+}
 
 void
 HostMgrTest::testGetAll(BaseHostDataSource& data_source1,
@@ -4394,8 +4410,7 @@ HostMgrTest::testGetAll6BySubnetIP(BaseHostDataSource& data_source1,
 }
 
 void
-HostMgrTest::testGetAll6ByIP(BaseHostDataSource& data_source1,
-                             BaseHostDataSource& data_source2) {
+HostMgrTest::testGetAll6ByIP(BaseHostDataSource& data_source1, BaseHostDataSource& data_source2) {
     // Set the mode of operation with multiple reservations for the same
     // IP address.
     ASSERT_TRUE(HostMgr::instance().setIPReservationsUnique(false));
@@ -4403,34 +4418,54 @@ HostMgrTest::testGetAll6ByIP(BaseHostDataSource& data_source1,
 
     // Initially, no reservations should be present.
     ConstHostCollection hosts = HostMgr::instance().getAll6(SubnetID(1),
-                                                            IOAddress("2001:db8:1::5"));
+                                                            IOAddress("2001:db8:1::10"));
     ASSERT_TRUE(hosts.empty());
 
-    // Add two reservations for the same subnet.
-    addHost6(data_source1, duids_[0], SubnetID(1), IOAddress("2001:db8:1::5"));
-    addHost6(data_source2, duids_[1], SubnetID(1), IOAddress("2001:db8:1::5"));
+    // Prepare vectors of IPv6 address reservations for new hosts.
+    std::vector<IOAddress> addresses1;
+    std::vector<IOAddress> addresses2;
+    addresses1.push_back(IOAddress("2001:db8:1::5"));
+    addresses1.push_back(IOAddress("2001:db8:1::10"));
+    addresses2.push_back(IOAddress("2001:db8:1::6"));
+    addresses2.push_back(IOAddress("2001:db8:1::10"));
+
+    // Add two hosts for the same subnet with 2 IPv6 addresses reservations per host.
+    addHost6(data_source1, duids_[0], SubnetID(1), addresses1);
+    addHost6(data_source2, duids_[1], SubnetID(1), addresses2);
 
     CfgMgr::instance().commit();
 
-    // If there non-matching subnet is specified, nothing should be returned.
+    // If a non-matching subnet is specified, nothing should be returned.
     hosts = HostMgr::instance().getAll6(SubnetID(100), IOAddress("2001:db8:1::5"));
     ASSERT_TRUE(hosts.empty());
 
-    // For given IP there should be two reservations.
+    // For given IP there should be one reservation.
     hosts = HostMgr::instance().getAll6(IOAddress("2001:db8:1::5"));
+    ASSERT_EQ(1, hosts.size());
+
+    // For given IP there should be one reservation.
+    hosts = HostMgr::instance().getAll6(IOAddress("2001:db8:1::6"));
+    ASSERT_EQ(1, hosts.size());
+
+    // For given IP there should be two reservations.
+    hosts = HostMgr::instance().getAll6(IOAddress("2001:db8:1::10"));
     ASSERT_EQ(2, hosts.size());
 
     // Make sure that subnet is correct.
     EXPECT_EQ(1, hosts[0]->getIPv6SubnetID());
     EXPECT_EQ(1, hosts[1]->getIPv6SubnetID());
 
-    // Make sure that two hosts were returned with different identifiers
-    // but the same address.
+    // Make sure that all hosts were returned with different identifiers, and
+    // they have expected reservations.
     EXPECT_NE(hosts[0]->getIdentifierAsText(), hosts[1]->getIdentifierAsText());
-    EXPECT_TRUE(hosts[0]->hasReservation(
-        IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::5"))));
-    EXPECT_TRUE(hosts[1]->hasReservation(
-        IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::5"))));
+    EXPECT_TRUE(
+        hosts[0]->hasReservation(IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::10"))));
+    EXPECT_TRUE(
+        hosts[0]->hasReservation(IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::5"))));
+    EXPECT_TRUE(
+        hosts[1]->hasReservation(IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::10"))));
+    EXPECT_TRUE(
+        hosts[1]->hasReservation(IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::6"))));
 
     // Make sure that the operation target is supported.
     bool is_first_source_primary = isPrimaryDataSource(data_source1);
@@ -4438,31 +4473,42 @@ HostMgrTest::testGetAll6ByIP(BaseHostDataSource& data_source1,
     size_t hosts_in_primary_source = is_first_source_primary + is_second_source_primary;
 
     // Select hosts only from the primary source.
-    hosts = HostMgr::instance().getAll6(IOAddress("2001:db8:1::5"), HostMgrOperationTarget::PRIMARY_SOURCE);
+    hosts = HostMgr::instance().getAll6(IOAddress("2001:db8:1::10"),
+                                        HostMgrOperationTarget::PRIMARY_SOURCE);
     EXPECT_EQ(hosts_in_primary_source, hosts.size());
     if (is_first_source_primary) {
-        EXPECT_TRUE(hosts[0]->hasReservation(
-            IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::5"))));
+        EXPECT_TRUE(
+            hosts[0]->hasReservation(IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::10"))));
+        EXPECT_TRUE(
+            hosts[0]->hasReservation(IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::5"))));
     }
     if (is_second_source_primary) {
         EXPECT_TRUE(hosts[hosts_in_primary_source - 1]->hasReservation(
-            IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::5"))));
+            IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::10"))));
+        EXPECT_TRUE(hosts[hosts_in_primary_source - 1]->hasReservation(
+            IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::6"))));
     }
 
     // Select hosts only from the alternate sources.
-    hosts = HostMgr::instance().getAll6(IOAddress("2001:db8:1::5"), HostMgrOperationTarget::ALTERNATE_SOURCES);
+    hosts = HostMgr::instance().getAll6(IOAddress("2001:db8:1::10"),
+                                        HostMgrOperationTarget::ALTERNATE_SOURCES);
     EXPECT_EQ(2 - hosts_in_primary_source, hosts.size());
     if (!is_first_source_primary) {
-        EXPECT_TRUE(hosts[0]->hasReservation(
-            IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::5"))));
+        EXPECT_TRUE(
+            hosts[0]->hasReservation(IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::10"))));
+        EXPECT_TRUE(
+            hosts[0]->hasReservation(IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::5"))));
     }
     if (!is_second_source_primary) {
         EXPECT_TRUE(hosts[2 - hosts_in_primary_source - 1]->hasReservation(
-            IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::5"))));
+            IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::10"))));
+        EXPECT_TRUE(hosts[2 - hosts_in_primary_source - 1]->hasReservation(
+            IPv6Resrv(IPv6Resrv::TYPE_NA, IOAddress("2001:db8:1::6"))));
     }
 
     // Select hosts for an unspecified source.
-    hosts = HostMgr::instance().getAll4(IOAddress("2001:db8:1::5"), HostMgrOperationTarget::UNSPECIFIED_SOURCE);
+    hosts = HostMgr::instance().getAll4(IOAddress("2001:db8:1::10"),
+                                        HostMgrOperationTarget::UNSPECIFIED_SOURCE);
     EXPECT_EQ(0, hosts.size());
 }
 

diff --git a/src/lib/dhcpsrv/testutils/generic_host_data_source_unittest.h b/src/lib/dhcpsrv/testutils/generic_host_data_source_unittest.h
index 786b88e4a8283a09ec79390d3af15e30e4e86073..b804e29cdbf3f4d4e9822efd864a24a44f7f4c41 100644 (file)
--- a/src/lib/dhcpsrv/testutils/generic_host_data_source_unittest.h
+++ b/src/lib/dhcpsrv/testutils/generic_host_data_source_unittest.h
@@ -746,6 +746,23 @@ protected:
                   const isc::asiolink::IOAddress& address,
                   const uint8_t prefix_len = 128);
 
+    /// @brief Inserts IPv6 reservation into the host data source.
+    ///
+    /// @param data_source Reference to the data source to which the reservation
+    /// should be inserted.
+    /// @param duid Pointer to the DUID to be associated with the reservation.
+    /// @param subnet_id IPv6 subnet id.
+    /// @param addresses IPv6 addresses/prefixes to be reserved.
+    /// @param prefix_len Prefix length. The default value is 128 which
+    /// indicates that the reservation is for an IPv6 address rather than a
+    /// prefix. Notice that this is common for all addresses in given vector
+    /// of addresses.
+    void addHost6(BaseHostDataSource& data_source,
+                  const DuidPtr& duid,
+                  const SubnetID& subnet_id,
+                  const std::vector<isc::asiolink::IOAddress>& addresses,
+                  const uint8_t prefix_len = 128);
+
     /// @brief This test verifies that HostMgr returns all reservations for the
     /// specified HW address.
     ///

diff --git a/src/share/api/reservation-get-by-address.json b/src/share/api/reservation-get-by-address.json
index c9a40bbc8e1278774f81de9e69267c588802124d..5d440ce456bb603dea3579fab966e9af98c46a03 100644 (file)
--- a/src/share/api/reservation-get-by-address.json
+++ b/src/share/api/reservation-get-by-address.json
@@ -1,6 +1,6 @@
 {
     "access": "read",
-    "avail": "2.3.9",
+    "avail": "2.4.0",
     "brief": [
         "This command retrieves all host reservations for given ip-address and optionally a specified subnet."
     ],