[#1387] Addressed comments

Author: fxdupont

doc/sphinx/arm/dhcp6-srv.rst

@@ -4273,17 +4273,16 @@ another.
 
 .. note::
 
-   Since Kea 2.7.1, a reserved (so delegated) prefix can be associated
-   with a single prefix to exclude as for prefix delegation pools
-   :ref:`pd-exclude-option`. The host reservation syntax is extended
-   by a new entry ``excluded-prefixes`` which when is present must have
-   the same size as the ``prefixes`` entry: both contains strings
-   representing IPv6 prefixes (e.g. ``2001:db8::/48``). Each element of
-   the ``excluded-prefixes`` must be either the empty string or match
-   the prefix at the same position in the ``prefixes`` list, e.g.
-   ``2001:db8:0:1::/64`` matches / can be associated with ``2001:db8::/48``.
-   An empty ``excluded-prefixes`` list or a list with only empty strings
-   can be omitted (and will be omitted when produced by Kea).
+   Beginning with Kea 2.7.3, the host reservation syntax supports
+   a new entry, ``excluded-prefixes``. It can be used to specify
+   prefixes the client should exclude from delegated prefixes.
+   When present it must have the same number of elements as the
+   ``prefixes`` entry. Both entries contain strings representing IPv6
+   prefixes. Each element of the ``excluded-prefixes`` must be either
+   an empty string or match the prefix at the same position in
+   ``prefixes``. An empty ``excluded-prefixes`` list or a list with
+   only empty strings can be omitted. An example which excludes
+   ``2001:db8:0:1::/64`` from ``2001:db8::/48`` is shown below:
 
 ::
 
@@ -4303,10 +4302,11 @@ another.
 
 .. note::
 
-   Host reservations have precedence over prefix pools so when a reserved
-   prefix without an excluded prefix is assigned no pd-exclude option
-   is added to the prefix option even the prefix is in a configured
-   prefix pool with an excluded prefix (different from previous behavior).
+   Since host reservations have precedence over prefix pools, a reserved
+   prefix without an excluded prefix will not add a pd-exclude option
+   to the prefix option even if the delegated prefix is in a configured
+   prefix pool that does specify an excluded prefix (different from
+   previous behavior).
 
 .. _reservation6-conflict:
 

src/bin/dhcp6/dhcp6_srv.h

@@ -1064,6 +1064,7 @@ class Dhcpv6Srv : public process::Daemon {
     ///
     /// @param ctx client context (contains subnet and hosts).
     /// @param lease lease (contains address/prefix and prefix length).
+    /// @param the prefix exclude option or null.
     OptionPtr getPDExclude(const AllocEngine::ClientContext6& ctx,
                            const Lease6Ptr& lease);
 

src/bin/dhcp6/tests/sarr_unittest.cc

@@ -419,12 +419,16 @@ class SARRTest : public Dhcpv6SrvTest {
     /// @brief This test verifies that it is possible to specify an excluded
     /// prefix (RFC 6603) and send it back to the client requesting prefix
     /// delegation using a pool.
-    void directClientExcludedPrefixPool();
+    ///
+    /// @param request_pdx request pd exclude option.
+    void directClientExcludedPrefixPool(bool request_pdx);
 
     /// @brief This test verifies that it is possible to specify an excluded
     /// prefix (RFC 6603) and send it back to the client requesting prefix
     /// delegation using a reservation.
-    void directClientExcludedPrefixHost();
+    ///
+    /// @param request_pdx request pd exclude option.
+    void directClientExcludedPrefixHost(bool request_pdx);
 
     /// @brief Check that when the client includes the Rapid Commit option in
     /// its Solicit, the server responds with Reply and commits the lease.
@@ -682,11 +686,14 @@ TEST_F(SARRTest, optionsInheritanceMultiThreading) {
 }
 
 void
-SARRTest::directClientExcludedPrefixPool() {
+SARRTest::directClientExcludedPrefixPool(bool request_pdx) {
     Dhcp6Client client;
     // Configure client to request IA_PD.
     client.requestPrefix();
-    client.requestOption(D6O_PD_EXCLUDE);
+    // Request pd exclude option when wanted.
+    if (request_pdx) {
+        client.requestOption(D6O_PD_EXCLUDE);
+    }
     configure(CONFIGS[3], *client.getServer());
     // Make sure we ended-up having expected number of subnets configured.
     const Subnet6Collection* subnets = CfgMgr::instance().getCurrentCfg()->
@@ -713,6 +720,10 @@ SARRTest::directClientExcludedPrefixPool() {
     Option6IAPrefixPtr pd_option = boost::dynamic_pointer_cast<Option6IAPrefix>(option);
     ASSERT_TRUE(pd_option);
     option = pd_option->getOption(D6O_PD_EXCLUDE);
+    if (!request_pdx) {
+        EXPECT_FALSE(option);
+        return;
+    }
     ASSERT_TRUE(option);
     Option6PDExcludePtr pd_exclude = boost::dynamic_pointer_cast<Option6PDExclude>(option);
     ASSERT_TRUE(pd_exclude);
@@ -723,22 +734,35 @@ SARRTest::directClientExcludedPrefixPool() {
 
 TEST_F(SARRTest, directClientExcludedPrefixPool) {
     Dhcpv6SrvMTTestGuard guard(*this, false);
-    directClientExcludedPrefixPool();
+    directClientExcludedPrefixPool(true);
 }
 
 TEST_F(SARRTest, directClientExcludedPrefixPoolMultiThreading) {
     Dhcpv6SrvMTTestGuard guard(*this, true);
-    directClientExcludedPrefixPool();
+    directClientExcludedPrefixPool(true);
+}
+
+TEST_F(SARRTest, directClientExcludedPrefixPoolNoOro) {
+    Dhcpv6SrvMTTestGuard guard(*this, false);
+    directClientExcludedPrefixPool(false);
+}
+
+TEST_F(SARRTest, directClientExcludedPrefixPoolNoOroMultiThreading) {
+    Dhcpv6SrvMTTestGuard guard(*this, true);
+    directClientExcludedPrefixPool(false);
 }
 
 void
-SARRTest::directClientExcludedPrefixHost() {
+SARRTest::directClientExcludedPrefixHost(bool request_pdx) {
     Dhcp6Client client;
     // Set DUID matching the one used to create host reservations.
     client.setDUID("01:02:03:05");
     // Configure client to request IA_PD.
     client.requestPrefix();
-    client.requestOption(D6O_PD_EXCLUDE);
+    // Request pd exclude option when wanted.
+    if (request_pdx) {
+        client.requestOption(D6O_PD_EXCLUDE);
+    }
     configure(CONFIGS[8], *client.getServer());
     // Make sure we ended-up having expected number of subnets configured.
     const Subnet6Collection* subnets = CfgMgr::instance().getCurrentCfg()->
@@ -765,6 +789,10 @@ SARRTest::directClientExcludedPrefixHost() {
     Option6IAPrefixPtr pd_option = boost::dynamic_pointer_cast<Option6IAPrefix>(option);
     ASSERT_TRUE(pd_option);
     option = pd_option->getOption(D6O_PD_EXCLUDE);
+    if (!request_pdx) {
+        EXPECT_FALSE(option);
+        return;
+    }
     ASSERT_TRUE(option);
     Option6PDExcludePtr pd_exclude = boost::dynamic_pointer_cast<Option6PDExclude>(option);
     ASSERT_TRUE(pd_exclude);
@@ -775,12 +803,22 @@ SARRTest::directClientExcludedPrefixHost() {
 
 TEST_F(SARRTest, directClientExcludedPrefixHost) {
     Dhcpv6SrvMTTestGuard guard(*this, false);
-    directClientExcludedPrefixHost();
+    directClientExcludedPrefixHost(true);
 }
 
 TEST_F(SARRTest, directClientExcludedPrefixHostMultiThreading) {
     Dhcpv6SrvMTTestGuard guard(*this, true);
-    directClientExcludedPrefixHost();
+    directClientExcludedPrefixHost(true);
+}
+
+TEST_F(SARRTest, directClientExcludedPrefixHostNoOro) {
+    Dhcpv6SrvMTTestGuard guard(*this, false);
+    directClientExcludedPrefixHost(false);
+}
+
+TEST_F(SARRTest, directClientExcludedPrefixHostNoOroMultiThreading) {
+    Dhcpv6SrvMTTestGuard guard(*this, true);
+    directClientExcludedPrefixHost(false);
 }
 
 void

src/lib/dhcpsrv/parsers/host_reservation_parser.cc

@@ -381,6 +381,8 @@ HostReservationParser6::parseInternal(const SubnetID& subnet_id,
                     uint8_t excluded_prefix_len(0);
                     parsePrefix(exclude, excluded_prefix, excluded_prefix_len,
                                 "exclude prefix");
+                    // setPDExclude calls the Option6PDExclude constructor
+                    // which throws on invalid prefix.
                     res.setPDExclude(excluded_prefix, excluded_prefix_len);
                 }
                 host->addReservation(res);

src/lib/dhcpsrv/tests/host_reservation_parser_unittest.cc

@@ -968,6 +968,24 @@ TEST_F(HostReservationParserTest, dhcp6ExcludedTooLong) {
     testInvalidConfig<HostReservationParser6>(config);
 }
 
+// This test verifies that the configuration parser throws an exception
+// when the excluded prefix is invalid (prefixes do not match).
+TEST_F(HostReservationParserTest, dhcp6ExcludedPrefixNotMatch) {
+    std::string config = "{ \"duid\": \"01:02:03:04:05:06:07:08:09:0A\","
+        "\"prefixes\": [ \"2001:db8::/48\" ],"
+        "\"excluded-prefixes\": [ \"2001:db9:0:1::/64\" ] }";
+    testInvalidConfig<HostReservationParser6>(config);
+}
+
+// This test verifies that the configuration parser throws an exception
+// when the excluded prefix is invalid (bad length).
+TEST_F(HostReservationParserTest, dhcp6ExcludedPrefixBadLength) {
+    std::string config = "{ \"duid\": \"01:02:03:04:05:06:07:08:09:0A\","
+        "\"prefixes\": [ \"2001:db8::/48\" ],"
+        "\"excluded-prefixes\": [ \"2001:db8::/48\" ] }";
+    testInvalidConfig<HostReservationParser6>(config);
+}
+
 // This test verifies that the configuration parser throws an exception
 // when invalid prefix length type is specified.
 TEST_F(HostReservationParserTest, dhcp6InvalidPrefixLengthType) {

src/lib/dhcpsrv/testutils/generic_host_data_source_unittest.cc

@@ -1531,19 +1531,8 @@ GenericHostDataSourceTest::testPrefixExclude(std::string prefix,
     ASSERT_TRUE(from_hds);
 
     HostDataSourceUtils::compareHosts(host, from_hds);
-
-#if 0
-    // Verify the test is meaningful.
-    HostPtr host2 = HostDataSourceUtils::initializeHost6(prefix,
-                                                         "2001:db8:0:2::",
-                                                         Host::IDENT_DUID,
-                                                         false);
-    host2->setIPv4SubnetID(host->getIPv4SubnetID());
-    host2->setIPv6SubnetID(host->getIPv6SubnetID());
-    ASSERT_TRUE(host2);
-    HostDataSourceUtils::compareHosts(host, host2);
-#endif
 }
+
 void
 GenericHostDataSourceTest::testMultipleSubnets(int subnets,
                                                const Host::IdentifierType& id) {