[3826] addressed review comments

Razvan Becheriu · Razvan Becheriu · commit 66a216e62d77 · 2025-07-31T14:22:37.000Z
diff --git a/ChangeLog b/ChangeLog
@@ -1,3 +1,8 @@
+2382.	[build]		razvan
+	Implemented the 'lease6-get-by-hw-address' command used to query
+	IPv6 leases by HW Address.
+	(Gitlab #3826)
+
 Kea 3.1.0 (development) released on July 30, 2025
 
 2381.	[build]		razvan
diff --git a/src/bin/admin/tests/mysql_tests.sh.in b/src/bin/admin/tests/mysql_tests.sh.in
@@ -157,7 +157,7 @@ mysql_db_version_test() {
     run_command \
         "${kea_admin}" db-version mysql -u "${db_user}" -p "${db_password}" -n "${db_name}"
     version="${OUTPUT}"
-    assert_str_eq "31.0" "${version}" "Expected kea-admin to return %s, returned value was %s"
+    assert_str_eq "32.0" "${version}" "Expected kea-admin to return %s, returned value was %s"
 
     # Let's wipe the whole database
     mysql_wipe
@@ -1200,7 +1200,7 @@ mysql_upgrade_test() {
 
     # Verify that the upgraded schema reports the latest version.
     version=$("${kea_admin}" db-version mysql -u "${db_user}" -p "${db_password}" -n "${db_name}" -d "${db_scripts_dir}")
-    assert_str_eq "31.0" "${version}" "Expected kea-admin to return %s, returned value was %s"
+    assert_str_eq "32.0" "${version}" "Expected kea-admin to return %s, returned value was %s"
 
     # Let's check that the new tables are indeed there.
 
diff --git a/src/bin/admin/tests/pgsql_tests.sh.in b/src/bin/admin/tests/pgsql_tests.sh.in
@@ -158,7 +158,7 @@ pgsql_db_version_test() {
     run_command \
         "${kea_admin}" db-version pgsql -u "${db_user}" -p "${db_password}" -n "${db_name}"
     version="${OUTPUT}"
-    assert_str_eq "30.0" "${version}" "Expected kea-admin to return %s, returned value was %s"
+    assert_str_eq "31.0" "${version}" "Expected kea-admin to return %s, returned value was %s"
 
     # Let's wipe the whole database
     pgsql_wipe
@@ -1171,7 +1171,7 @@ pgsql_upgrade_test() {
 
     # Verify upgraded schema reports the latest version.
     version=$("${kea_admin}" db-version pgsql -u "${db_user}" -p "${db_password}" -n "${db_name}" -d "${db_scripts_dir}")
-    assert_str_eq "30.0" "${version}" 'Expected kea-admin to return %s, returned value was %s'
+    assert_str_eq "31.0" "${version}" 'Expected kea-admin to return %s, returned value was %s'
 
     # Check 1.0 to 2.0 upgrade
     pgsql_upgrade_1_0_to_2_0_test
diff --git a/src/hooks/dhcp/lease_cmds/lease_cmds.cc b/src/hooks/dhcp/lease_cmds/lease_cmds.cc
@@ -1597,6 +1597,10 @@ LeaseCmdsImpl::leaseGetByHwAddressHandler(CalloutHandle& handle) {
             isc_throw(BadValue, "'hw-address' parameter must be a string");
         }
 
+        if (!v4 && hw_address->stringValue().empty()) {
+            isc_throw(BadValue, "'hw-address' parameter must not be empty");
+        }
+
         HWAddr hwaddr = HWAddr::fromText(hw_address->stringValue());
 
         ElementPtr leases_json = Element::createList();
diff --git a/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds4_unittest.cc b/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds4_unittest.cc
@@ -1794,6 +1794,16 @@ void Lease4CmdsTest::testLease4GetByHwAddressFind0() {
         "}";
     string exp_rsp = "0 IPv4 lease(s) found.";
     testCommand(cmd, CONTROL_RESULT_EMPTY, exp_rsp);
+
+    // Empty HWAddr.
+    cmd =
+        "{\n"
+        "    \"command\": \"lease4-get-by-hw-address\",\n"
+        "    \"arguments\": {"
+        "        \"hw-address\": \"\"\n"
+        "    }\n"
+        "}";
+    testCommand(cmd, CONTROL_RESULT_EMPTY, exp_rsp);
 }
 
 void Lease4CmdsTest::testLease4GetByHwAddressFind2() {
diff --git a/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds6_unittest.cc b/src/hooks/dhcp/lease_cmds/libloadtests/lease_cmds6_unittest.cc
@@ -2014,6 +2014,17 @@ void Lease6CmdsTest::testLease6GetByHwAddressParams() {
     exp_rsp = "'hw-address' parameter must be a string";
     testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp);
 
+    // Empty HWAddr.
+    cmd =
+        "{\n"
+        "    \"command\": \"lease6-get-by-hw-address\",\n"
+        "    \"arguments\": {"
+        "        \"hw-address\": \"\"\n"
+        "    }\n"
+        "}";
+    exp_rsp = "'hw-address' parameter must not be empty";
+    testCommand(cmd, CONTROL_RESULT_ERROR, exp_rsp);
+
     // Simply bad value.
     cmd =
         "{\n"
@@ -2071,10 +2082,10 @@ void Lease6CmdsTest::testLease6GetByHwAddressFind2() {
     // Let's check if the response makes any sense.
     ConstElementPtr lease = leases->get(0);
     ASSERT_TRUE(lease);
-    checkLease6(lease, "2001:db8:1::1", 0, 66, "42:42:42:42:42:42:42:42", "08:08:08:08:08:08");
+    checkLease6(lease, "2001:db8:2::1", 0, 99, "42:42:42:42:42:42:42:42", "08:08:08:08:08:08");
     lease = leases->get(1);
     ASSERT_TRUE(lease);
-    checkLease6(lease, "2001:db8:2::1", 0, 99, "42:42:42:42:42:42:42:42", "08:08:08:08:08:08");
+    checkLease6(lease, "2001:db8:1::1", 0, 66, "42:42:42:42:42:42:42:42", "08:08:08:08:08:08");
 }
 
 void Lease6CmdsTest::testLease6GetByDuidParams() {
diff --git a/src/lib/dhcpsrv/memfile_lease_mgr.cc b/src/lib/dhcpsrv/memfile_lease_mgr.cc
@@ -1459,13 +1459,11 @@ Memfile_LeaseMgr::getLease6Internal(Lease::Type type,
 void
 Memfile_LeaseMgr::getLease6Internal(const HWAddr& hwaddr,
                                     Lease6Collection& collection) const {
-    // Using composite index by 'hw address' and 'subnet id'. It is
-    // ok to use it for searching by the 'hw address' only.
-    const Lease6StorageHWAddressSubnetIdIndex& idx =
-        storage6_.get<HWAddressSubnetIdIndexTag>();
-    std::pair<Lease6StorageHWAddressSubnetIdIndex::const_iterator,
-              Lease6StorageHWAddressSubnetIdIndex::const_iterator> l
-        = idx.equal_range(boost::make_tuple(hwaddr.hwaddr_));
+    const Lease6StorageHWAddressIndex& idx =
+        storage6_.get<HWAddressIndexTag>();
+    std::pair<Lease6StorageHWAddressIndex::const_iterator,
+              Lease6StorageHWAddressIndex::const_iterator> l
+        = idx.equal_range(hwaddr.hwaddr_);
 
     BOOST_FOREACH(auto const& lease, l) {
         collection.push_back(Lease6Ptr(new Lease6(*lease)));
diff --git a/src/lib/dhcpsrv/memfile_lease_storage.h b/src/lib/dhcpsrv/memfile_lease_storage.h
@@ -37,6 +37,9 @@ struct ExpirationIndexTag { };
 /// @brief Tag for indexes by HW address, subnet-id tuple.
 struct HWAddressSubnetIdIndexTag { };
 
+/// @brief Tag for indexes by HW address.
+struct HWAddressIndexTag { };
+
 /// @brief Tag for indexes by client-id, subnet-id tuple.
 struct ClientIdSubnetIdIndexTag { };
 
@@ -176,25 +179,15 @@ typedef boost::multi_index_container<
             >
         >,
 
-        // Specification of the eight index starts here.
-        boost::multi_index::ordered_non_unique<
-            boost::multi_index::tag<HWAddressSubnetIdIndexTag>,
-            // This is a composite index that combines two attributes of the
-            // Lease6 object: hardware address and subnet id.
-            boost::multi_index::composite_key<
-                Lease6,
-                // The hardware address is held in the hwaddr_ member of the
-                // Lease4 object, which is a HWAddr object. Boost does not
-                // provide a key extractor for getting a member of a member,
-                // so we need a simple method for that.
-                boost::multi_index::const_mem_fun<Lease, const std::vector<uint8_t>&,
-                                                  &Lease::getHWAddrVector>,
-                // The subnet id is held in the subnet_id_ member of Lease6
-                // class. Note that the subnet_id_ is defined in the base
-                // class (Lease) so we have to point to this class rather
-                // than derived class: Lease6.
-                boost::multi_index::member<Lease, SubnetID, &Lease::subnet_id_>
-            >
+        // Specification of the eighth index starts here.
+        boost::multi_index::hashed_non_unique<
+            boost::multi_index::tag<HWAddressIndexTag>,
+            // The hardware address is held in the hwaddr_ member of the
+            // Lease6 object, which is a HWAddr object. Boost does not
+            // provide a key extractor for getting a member of a member,
+            // so we need a simple method for that.
+            boost::multi_index::const_mem_fun<Lease, const std::vector<uint8_t>&,
+                                              &Lease::getHWAddrVector>
         >
     >
 > Lease6Storage; // Specify the type name of this container.
@@ -364,9 +357,9 @@ typedef Lease6Storage::index<DuidIaidTypeIndexTag>::type Lease6StorageDuidIaidTy
 /// @brief DHCPv6 lease storage index by expiration time.
 typedef Lease6Storage::index<ExpirationIndexTag>::type Lease6StorageExpirationIndex;
 
-/// @brief DHCPv6 lease storage index by HW address and subnet-id.
-typedef Lease6Storage::index<HWAddressSubnetIdIndexTag>::type
-Lease6StorageHWAddressSubnetIdIndex;
+/// @brief DHCPv6 lease storage index by HW address.
+typedef Lease6Storage::index<HWAddressIndexTag>::type
+Lease6StorageHWAddressIndex;
 
 /// @brief DHCPv6 lease storage index by subnet-id.
 typedef Lease6Storage::index<SubnetIdIndexTag>::type Lease6StorageSubnetIdIndex;
diff --git a/src/lib/mysql/mysql_constants.h b/src/lib/mysql/mysql_constants.h
@@ -52,7 +52,7 @@ const int MLM_MYSQL_FETCH_FAILURE = 0;
 
 /// @name Current database schema version values.
 //@{
-const uint32_t MYSQL_SCHEMA_VERSION_MAJOR = 31;
+const uint32_t MYSQL_SCHEMA_VERSION_MAJOR = 32;
 const uint32_t MYSQL_SCHEMA_VERSION_MINOR = 0;
 
 //@}
diff --git a/src/lib/pgsql/pgsql_connection.h b/src/lib/pgsql/pgsql_connection.h
@@ -18,7 +18,7 @@ namespace isc {
 namespace db {
 
 /// @brief Define the PostgreSQL backend version.
-const uint32_t PGSQL_SCHEMA_VERSION_MAJOR = 30;
+const uint32_t PGSQL_SCHEMA_VERSION_MAJOR = 31;
 const uint32_t PGSQL_SCHEMA_VERSION_MINOR = 0;
 
 // Maximum number of parameters that can be used a statement
diff --git a/src/share/database/scripts/mysql/dhcpdb_create.mysql b/src/share/database/scripts/mysql/dhcpdb_create.mysql
@@ -6451,6 +6451,17 @@ UPDATE schema_version
 
 -- This line concludes the schema upgrade to version 31.0.
 
+-- This line starts the schema upgrade to version 32.0.
+
+# Create index for searching leases by hwaddr.
+CREATE INDEX lease6_by_hwaddr ON lease6 (hwaddr);
+
+-- Update the schema version number.
+UPDATE schema_version
+    SET version = '32', minor = '0';
+
+-- This line concludes the schema upgrade to version 32.0.
+
 # Notes:
 #
 # Indexes
diff --git a/src/share/database/scripts/mysql/meson.build b/src/share/database/scripts/mysql/meson.build
@@ -72,6 +72,7 @@ upgrade_scripts = [
     'upgrade_028_to_029.sh',
     'upgrade_029_to_030.sh',
     'upgrade_030_to_031.sh',
+    'upgrade_031_to_032.sh',
 ]
 list = run_command(
     GRABBER,
diff --git a/src/share/database/scripts/mysql/upgrade_031_to_032.sh.in b/src/share/database/scripts/mysql/upgrade_031_to_032.sh.in
@@ -0,0 +1,68 @@
+#!/bin/sh
+
+# Copyright (C) 2025 Internet Systems Consortium, Inc. ("ISC") #
+#
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+# Exit with error if commands exit with non-zero and if undefined variables are
+# used.
+set -eu
+
+# shellcheck disable=SC2034
+# SC2034: ... appears unused. Verify use (or export if used externally).
+prefix="@prefix@"
+
+# Include utilities based on location of this script. Check for sources first,
+# so that the unexpected situations with weird paths fall on the default
+# case of installed.
+script_path=$(cd "$(dirname "${0}")" && pwd)
+if test "${script_path}" = "@abs_top_builddir@/src/share/database/scripts/mysql"; then
+    # shellcheck source=./src/bin/admin/admin-utils.sh.in
+    . "@abs_top_builddir@/src/bin/admin/admin-utils.sh"
+else
+    # shellcheck source=./src/bin/admin/admin-utils.sh.in
+    . "@datarootdir@/@PACKAGE_NAME@/scripts/admin-utils.sh"
+fi
+
+# Check only major version to allow for intermediary backported schema changes.
+version=$(mysql_version "${@}" | cut -d '.' -f 1)
+if test "${version}" != '31'; then
+    printf 'This script upgrades 31.* to 32.0. '
+    printf 'Reported version is %s. Skipping upgrade.\n' "${version}"
+    exit 0
+fi
+
+# Get the schema name from database argument. We need this to
+# query information_schema for the right database.
+for arg in "${@}"
+do
+    if ! printf '%s' "${arg}" | grep -Eq -- '^--'
+    then
+        schema="$arg"
+        break
+    fi
+done
+
+# Make sure we have the schema.
+if [ -z "$schema" ]
+then
+    printf "Could not find database schema name in cmd line args: %s\n" "${*}"
+    exit 255
+fi
+
+mysql "$@" <<EOF
+
+-- This line starts the schema upgrade to version 32.0.
+
+# Create index for searching leases by hwaddr.
+CREATE INDEX lease6_by_hwaddr ON lease6 (hwaddr);
+
+-- Update the schema version number.
+UPDATE schema_version
+    SET version = '32', minor = '0';
+
+-- This line concludes the schema upgrade to version 32.0.
+
+EOF
diff --git a/src/share/database/scripts/pgsql/dhcpdb_create.pgsql b/src/share/database/scripts/pgsql/dhcpdb_create.pgsql
@@ -6728,6 +6728,17 @@ UPDATE schema_version
 
 -- This line concludes the schema upgrade to version 30.0.
 
+-- This line starts the schema upgrade to version 31.0.
+
+-- Create index for searching leases by hwaddr.
+CREATE INDEX lease6_by_hwaddr ON lease6 (hwaddr);
+
+-- Update the schema version number.
+UPDATE schema_version
+    SET version = '31', minor = '0';
+
+-- This line concludes the schema upgrade to version 31.0.
+
 -- Commit the script transaction.
 COMMIT;
 
diff --git a/src/share/database/scripts/pgsql/meson.build b/src/share/database/scripts/pgsql/meson.build
@@ -66,6 +66,7 @@ upgrade_scripts = [
     'upgrade_027_to_028.sh',
     'upgrade_028_to_029.sh',
     'upgrade_029_to_030.sh',
+    'upgrade_030_to_031.sh',
 ]
 list = run_command(
     GRABBER,
diff --git a/src/share/database/scripts/pgsql/upgrade_030_to_031.sh.in b/src/share/database/scripts/pgsql/upgrade_030_to_031.sh.in
@@ -0,0 +1,54 @@
+#!/bin/sh
+
+# Copyright (C) 2024-2025 Internet Systems Consortium, Inc. ("ISC")
+#
+# This Source Code Form is subject to the terms of the Mozilla Public
+# License, v. 2.0. If a copy of the MPL was not distributed with this
+# file, You can obtain one at http://mozilla.org/MPL/2.0/.
+
+# Exit with error if commands exit with non-zero and if undefined variables are
+# used.
+set -eu
+
+# shellcheck disable=SC2034
+# SC2034: ... appears unused. Verify use (or export if used externally).
+prefix="@prefix@"
+
+# Include utilities based on location of this script. Check for sources first,
+# so that the unexpected situations with weird paths fall on the default
+# case of installed.
+script_path=$(cd "$(dirname "${0}")" && pwd)
+if test "${script_path}" = "@abs_top_builddir@/src/share/database/scripts/pgsql"; then
+    # shellcheck source=./src/bin/admin/admin-utils.sh.in
+    . "@abs_top_builddir@/src/bin/admin/admin-utils.sh"
+else
+    # shellcheck source=./src/bin/admin/admin-utils.sh.in
+    . "@datarootdir@/@PACKAGE_NAME@/scripts/admin-utils.sh"
+fi
+
+# Check only major version to allow for intermediary backported schema changes.
+version=$(pgsql_version "${@}" | cut -d '.' -f 1)
+if test "${version}" != '30'; then
+    printf 'This script upgrades 30.* to 31.0. '
+    printf 'Reported version is %s. Skipping upgrade.\n' "${version}"
+    exit 0
+fi
+
+psql "$@" >/dev/null <<EOF
+START TRANSACTION;
+
+-- This line starts the schema upgrade to version 31.0.
+
+-- Create index for searching leases by hwaddr.
+CREATE INDEX lease6_by_hwaddr ON lease6 (hwaddr);
+
+-- Update the schema version number.
+UPDATE schema_version
+    SET version = '31', minor = '0';
+
+-- This line concludes the schema upgrade to version 31.0.
+
+-- Commit the script transaction.
+COMMIT;
+
+EOF

Original file line number	Diff line number	Diff line change
`@@ -1597,6 +1597,10 @@ LeaseCmdsImpl::leaseGetByHwAddressHandler(CalloutHandle& handle) {`
`1597`	`1597`	`isc_throw(BadValue, "'hw-address' parameter must be a string");`
`1598`	`1598`	`}`
`1599`	`1599`
	`1600`	`+ if (!v4 && hw_address->stringValue().empty()) {`
	`1601`	`+ isc_throw(BadValue, "'hw-address' parameter must not be empty");`
	`1602`	`+ }`
	`1603`	`+`
`1600`	`1604`	`HWAddr hwaddr = HWAddr::fromText(hw_address->stringValue());`
`1601`	`1605`
`1602`	`1606`	`ElementPtr leases_json = Element::createList();`