[#2595] Finished

diff --git a/ChangeLog b/ChangeLog
index 640c498febff784eb4ad519e8ab4bb67645c782c..83ffa488af5bbd89a21248c0bd749cca3055074c 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2069.  [func]          fdupont
+       Added a new sanity checker named "extended-info-checks"
+       which checks and eventually upgrades lease extended
+       info which store into lease user context in DHCPv4
+       the dhcp-agent-options content and in DHCPv6 the
+       relay-msg fields and options.
+       (Gitlab #2595)
+
 2068.  [func]          djt
        Kea's official APK, Deb, and RPM packages have been restructured
        and made to follow a consistent packaging standard. Some of the

diff --git a/doc/sphinx/arm/dhcp4-srv.rst b/doc/sphinx/arm/dhcp4-srv.rst
index 6b3a76db7d3b3778ce7b63247bf4b0f295c2ec97..3e0a4e3739c6750e99b3177cab33c276b2ce8af6 100644 (file)
--- a/doc/sphinx/arm/dhcp4-srv.rst
+++ b/doc/sphinx/arm/dhcp4-srv.rst
@@ -3938,8 +3938,8 @@ would not want all the leases associated with it to disappear from the
 lease database. Kea has a mechanism to implement sanity checks for situations
 like this.
 
-Kea supports a configuration scope called ``sanity-checks``. It
-currently allows only a single parameter, called ``lease-checks``, which
+Kea supports a configuration scope called ``sanity-checks``.
+A parameter, called ``lease-checks``,
 governs the verification carried out when a new lease is loaded from a
 lease file. This mechanism permits Kea to attempt to correct inconsistent data.
 
@@ -4046,6 +4046,32 @@ Or with remote and relay suboptions:
     purposes. As long as no other purpose also writes an "ISC" element to
     ``user-context`` there should not be a conflict.
 
+Extended lease information is also subject to configurable sanity checking.
+The parameter in the ``sanity-checks`` scope is named ``extended-info-checks``
+and supports these levels:
+
+-  ``none`` - do no check nor upgrade. This level should be used on when
+   extended info is not used at all or when no badly formatted extended
+   info, including using the old format, is expected.
+
+-  ``fix`` - fix some common inconsistencies and upgrade extended info
+   using the old format to the new one. It is the default level and is
+   convenient when Lease Query hook library is not loaded.
+
+-  ``strict`` - fix all inconsistencies which have an impact on the (Bulk)
+   Lease Query hook library.
+
+-  ``pedantic`` - enforce full conformance to the format produced by the
+   Kea code, for instance no extra entries are allowed at the exception
+   of ``comment``.
+
+.. note::
+
+   Currently this feature is currently implemented for the memfile
+   backend. The sanity check applies to the lease database in memory,
+   not to the lease file, i.e. inconsistent leases will stay in the lease
+   file.
+
 .. _dhcp4-multi-threading-settings:
 
 Multi-Threading Settings

diff --git a/doc/sphinx/arm/dhcp6-srv.rst b/doc/sphinx/arm/dhcp6-srv.rst
index 504d9880619a88772345cc2072fd994f62ae818e..f95ab76cb1ee6658ea0609c6afc2f309f156354d 100644 (file)
--- a/doc/sphinx/arm/dhcp6-srv.rst
+++ b/doc/sphinx/arm/dhcp6-srv.rst
@@ -3386,8 +3386,8 @@ would not want all the leases associated with it to disappear from the
 lease database. Kea has a mechanism to implement sanity checks for situations
 like this.
 
-Kea supports a configuration scope called ``sanity-checks``. It
-currently allows only a single parameter, called ``lease-checks``, which
+Kea supports a configuration scope called ``sanity-checks``.
+A parameter, called ``lease-checks``,
 governs the verification carried out when a new lease is loaded from a
 lease file. This mechanism permits Kea to attempt to correct inconsistent data.
 
@@ -3513,6 +3513,32 @@ pretty-printed for clarity):
     container serving multiple purposes. As long as no other purpose also
     writes an "ISC" element to ``user-context`` there should not be a conflict.
 
+Extended lease information is also subject to configurable sanity checking.
+The parameter in the ``sanity-checks`` scope is named ``extended-info-checks``
+and supports these levels:
+
+-  ``none`` - do no check nor upgrade. This level should be used on when
+   extended info is not used at all or when no badly formatted extended
+   info, including using the old format, is expected.
+
+-  ``fix`` - fix some common inconsistencies and upgrade extended info
+   using the old format to the new one. It is the default level and is
+   convenient when Lease Query hook library is not loaded.
+
+-  ``strict`` - fix all inconsistencies which have an impact on the (Bulk)
+   Lease Query hook library.
+
+-  ``pedantic`` - enforce full conformance to the format produced by the
+   Kea code, for instance no extra entries are allowed at the exception
+   of ``comment``.
+
+.. note::
+
+   Currently this feature is currently implemented for the memfile
+   backend. The sanity check applies to the lease database in memory,
+   not to the lease file, i.e. inconsistent leases will stay in the lease
+   file.
+
 .. _dhcp6-multi-threading-settings:
 
 Multi-Threading Settings

diff --git a/src/lib/dhcpsrv/lease_mgr.cc b/src/lib/dhcpsrv/lease_mgr.cc
index c852f34e6ddcde6a2256789b353adb71b9604d9b..4be23770938a3d634abebbfd426dab1d55aa7d5c 100644 (file)
--- a/src/lib/dhcpsrv/lease_mgr.cc
+++ b/src/lib/dhcpsrv/lease_mgr.cc
@@ -903,7 +903,7 @@ LeaseMgr::upgradeLease6ExtendedInfo(const Lease6Ptr& lease,
             }
 
             verifying = (upgraded ? "relays" : "relay-info");
-            for (auto elem : relay_info->mapValue()) {
+            for (auto elem : relay->mapValue()) {
                 if ((elem.first != "hop") &&
                     (elem.first != "link") &&
                     (elem.first != "peer") &&

diff --git a/src/lib/dhcpsrv/tests/sanity_checks_unittest.cc b/src/lib/dhcpsrv/tests/sanity_checks_unittest.cc
index b926bfee593b346af1204e67dcebe1de43d8f22b..7185b931ae3337017c7beb1cdd7fa393276d24ca 100644 (file)
--- a/src/lib/dhcpsrv/tests/sanity_checks_unittest.cc
+++ b/src/lib/dhcpsrv/tests/sanity_checks_unittest.cc
@@ -1496,3 +1496,119 @@ TEST_F(ExtendedInfoChecksTest, validRelayId6strict) {
            " \"link\": \"2001::2\" } ] } }",
            CfgConsistency::EXTENDED_INFO_CHECK_STRICT);
 }
+
+// Pedantic requires a peer entry.
+TEST_F(ExtendedInfoChecksTest, noPeerpedantic) {
+    string description = "no peer, pedantic";
+    check6(description,
+           "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\" } ] } }",
+           "", CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC,
+           { "DHCPSRV_LEASE6_EXTENDED_INFO_SANITY_FAIL"
+             " extended info for lease 2001::1 failed checks"
+             " (in peer [relay#0] a problem was found:"
+             " no peer)" });
+}
+
+// peer entry with bad type is dropped by pedantic sanity check level.
+TEST_F(ExtendedInfoChecksTest, badTypePeer) {
+    string description = "peer is not a string, pedantic";
+    check6(description,
+           "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\","
+           " \"peer\": 1 } ] } }", "",
+           CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC,
+           { "DHCPSRV_LEASE6_EXTENDED_INFO_SANITY_FAIL"
+             " extended info for lease 2001::1 failed checks"
+             " (in peer [relay#0] a problem was found:"
+             " peer is not a string)" });
+}
+
+// peer entry which is not an address is dropped by pedantic sanity check level.
+TEST_F(ExtendedInfoChecksTest, notAddressPeer) {
+    string description = "peer is not an address, pedantic";
+    check6(description,
+           "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\","
+           " \"peer\": \"foo\" } ] } }", "",
+           CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC,
+           { "DHCPSRV_LEASE6_EXTENDED_INFO_SANITY_FAIL"
+             " extended info for lease 2001::1 failed checks"
+             " (in peer [relay#0] a problem was found:"
+             " Failed to convert string to address 'foo':"
+             " Invalid argument)" });
+}
+
+// peer entry which is an IPv4 (vs IPv6) address is dropped by pedantic sanity
+// check level.
+TEST_F(ExtendedInfoChecksTest, notV6Peer) {
+    string description = "peer is v4, pedantic";
+    check6(description,
+           "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\","
+           " \"peer\": \"192.128.1.1\" } ] } }", "",
+           CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC,
+           { "DHCPSRV_LEASE6_EXTENDED_INFO_SANITY_FAIL"
+             " extended info for lease 2001::1 failed checks"
+             " (in peer [relay#0] a problem was found:"
+             " peer is not an IPv6 address)" });
+}
+
+// Pedantic requires a hop entry.
+TEST_F(ExtendedInfoChecksTest, noHop) {
+    string description = "no hop, pedantic";
+    check6(description,
+           "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\","
+           " \"peer\": \"2001::3\" } ] } }", "",
+           CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC,
+           { "DHCPSRV_LEASE6_EXTENDED_INFO_SANITY_FAIL"
+             " extended info for lease 2001::1 failed checks"
+             " (in hop [relay#0] a problem was found:"
+             " no hop)" });
+}
+
+// hop entry with bad type is dropped by pedantic sanity check level.
+TEST_F(ExtendedInfoChecksTest, badTypeHop) {
+    string description = "hop is not an integer pedantic";
+    check6(description,
+           "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\","
+           " \"peer\": \"2001::3\", \"hop\": false } ] } }", "",
+           CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC,
+           { "DHCPSRV_LEASE6_EXTENDED_INFO_SANITY_FAIL"
+             " extended info for lease 2001::1 failed checks"
+             " (in hop [relay#0] a problem was found:"
+             " hop is not an integer)" });
+}
+
+// Valid relay.
+TEST_F(ExtendedInfoChecksTest, valid6Pedantic) {
+    string description = "valid, pedantic";
+    check6(description,
+           "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\","
+           " \"peer\": \"2001::3\", \"hop\": 10 } ] } }",
+           "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\","
+           " \"peer\": \"2001::3\", \"hop\": 10 } ] } }",
+           CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC);
+}
+
+// Junk entries are dropped at the pedantic level.
+TEST_F(ExtendedInfoChecksTest, junk6pedantic) {
+    string description = "junk entry, pedantic";
+    check6(description,
+           "{ \"ISC\": { \"relay-info\": [ { \"link\": \"2001::2\","
+           " \"peer\": \"2001::3\", \"hop\": 10, \"foo\": 1 } ] } }", "",
+           CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC,
+           { "DHCPSRV_LEASE6_EXTENDED_INFO_SANITY_FAIL"
+             " extended info for lease 2001::1 failed checks"
+             " (in relay-info [relay#0] a problem was found:"
+             " spurious 'foo' entry)" });
+}
+
+// Same with relays post upgrade checks.
+TEST_F(ExtendedInfoChecksTest, junkRelayspedantic) {
+    string description = "junk entry, pedantic";
+    check6(description,
+           "{ \"ISC\": { \"relays\": [ { \"link\": \"2001::2\","
+           " \"peer\": \"2001::3\", \"hop\": 10, \"foo\": 1 } ] } }", "",
+           CfgConsistency::EXTENDED_INFO_CHECK_PEDANTIC,
+           { "DHCPSRV_LEASE6_EXTENDED_INFO_SANITY_FAIL"
+             " extended info for lease 2001::1 failed checks"
+             " (in relays [relay#0] a problem was found:"
+             " spurious 'foo' entry)" });
+}