From: Piotrek Zadroga Date: Wed, 21 Jun 2023 22:28:38 +0000 (+0200) Subject: [#2795] addressed review comments X-Git-Tag: Kea-2.4.0~91 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=bc506ed6df6eef2ff9c10957901f8daf778d88ed;p=thirdparty%2Fkea.git [#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 95bcbef97f..6995d068ed 100644 --- 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 106b385259..7b1b8f0791 100644 --- 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 9496736c75..51b4856fdd 100644 --- 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 032118f9ec..80a83b7359 100644 --- 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& 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 addresses1; + std::vector 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 786b88e4a8..b804e29cdb 100644 --- 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& 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 c9a40bbc8e..5d440ce456 100644 --- 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." ],