[#1449] Fixed v6 appending multiple option instances

diff --git a/ChangeLog b/ChangeLog
index 90fb429bf59ebb609434993712eca26afbf2d923..aa04767d0899543d39450e730136bc0263b0a913 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+1838.  [bug]           fdupont
+       The DHCPv6 sent multiple instances of an option or a
+       sub-option when it seems to be requested more than once
+       directly by the client or using the always-send flag.
+       (Gitlab #1449)
+
 1837.  [doc]           cstrotm
        Several Kea ARM corrections.
        (Gitlab #1514)

diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc
index ba9f90a3662a56ca9294f05bdeeb04629ed64bd8..82804556067465643457bf8092c8e3e5ea152ec0 100644 (file)
--- a/src/bin/dhcp4/dhcp4_srv.cc
+++ b/src/bin/dhcp4/dhcp4_srv.cc
@@ -1859,7 +1859,7 @@ Dhcpv4Srv::appendRequestedVendorOptions(Dhcpv4Exchange& ex) {
     bool added = false;
     for (std::vector<uint8_t>::const_iterator code = requested_opts.begin();
          code != requested_opts.end(); ++code) {
-        if  (!vendor_rsp->getOption(*code)) {
+        if (!vendor_rsp->getOption(*code)) {
             for (CfgOptionList::const_iterator copts = co_list.begin();
                  copts != co_list.end(); ++copts) {
                 OptionDescriptor desc = (*copts)->get(vendor_id, *code);

diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc
index 7544a52facaa7a346d7e9ecef7a1164f03a9fb98..b723bb9ab44ca1837c5f5f291e715c146d3f6acd 100644 (file)
--- a/src/bin/dhcp6/dhcp6_srv.cc
+++ b/src/bin/dhcp6/dhcp6_srv.cc
@@ -1384,15 +1384,18 @@ Dhcpv6Srv::appendRequestedOptions(const Pkt6Ptr& question, Pkt6Ptr& answer,
         }
     }
 
-    BOOST_FOREACH(uint16_t opt, requested_opts) {
-        // Iterate on the configured option list
-        for (CfgOptionList::const_iterator copts = co_list.begin();
-             copts != co_list.end(); ++copts) {
-            OptionDescriptor desc = (*copts)->get(DHCP6_OPTION_SPACE, opt);
-            // Got it: add it and jump to the outer loop
-            if (desc.option_) {
-                answer->addOption(desc.option_);
-                break;
+    for (uint16_t opt : requested_opts) {
+        // Add nothing when it is already there.
+        if (!answer->getOption(opt)) {
+            // Iterate on the configured option list
+            for (CfgOptionList::const_iterator copts = co_list.begin();
+                 copts != co_list.end(); ++copts) {
+                OptionDescriptor desc = (*copts)->get(DHCP6_OPTION_SPACE, opt);
+                // Got it: add it and jump to the outer loop
+                if (desc.option_) {
+                    answer->addOption(desc.option_);
+                    break;
+                }
             }
         }
     }
@@ -1488,14 +1491,16 @@ Dhcpv6Srv::appendRequestedVendorOptions(const Pkt6Ptr& question,
     // Get the list of options that client requested.
     bool added = false;
 
-    BOOST_FOREACH(uint16_t opt, requested_opts) {
+    for (uint16_t opt : requested_opts) {
         for (CfgOptionList::const_iterator copts = co_list.begin();
              copts != co_list.end(); ++copts) {
-            OptionDescriptor desc = (*copts)->get(vendor_id, opt);
-            if (desc.option_) {
-                vendor_rsp->addOption(desc.option_);
-                added = true;
-                break;
+            if (!vendor_rsp->getOption(opt)) {
+                OptionDescriptor desc = (*copts)->get(vendor_id, opt);
+                if (desc.option_) {
+                    vendor_rsp->addOption(desc.option_);
+                    added = true;
+                    break;
+                }
             }
         }
     }

diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
index 8108b8c225ceb086b6c07b6124fd4c1a06c39b56..192c7aed81f45728deaabf59ca3640092921d891 100644 (file)
--- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
+++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
@@ -2221,6 +2221,28 @@ TEST_F(Dhcpv6SrvTest, prlPersistency) {
     ASSERT_TRUE(response->getOption(D6O_NAME_SERVERS));
     // and still no sntp-servers
     ASSERT_FALSE(response->getOption(D6O_SNTP_SERVERS));
+
+    // Reset ORO adding subscriber-id
+    sol->delOption(D6O_ORO);
+    OptionUint16ArrayPtr oro2(new OptionUint16Array(Option::V6, D6O_ORO));
+    ASSERT_TRUE(oro2);
+    oro2->addValue(D6O_SUBSCRIBER_ID);
+    sol->addOption(oro2);
+
+    // Let the server process it again.
+    AllocEngine::ClientContext6 ctx3;
+    srv_.initContext(sol, ctx3, drop);
+    ASSERT_FALSE(drop);
+    response = srv_.processSolicit(ctx3);
+
+    // The subscriber-id option should be present but only once despite
+    // it is both requested and has always-send.
+    const OptionCollection& sifs = response->getOptions(D6O_SUBSCRIBER_ID);
+    ASSERT_EQ(1, sifs.size());
+    // But no dns-servers
+    ASSERT_FALSE(response->getOption(D6O_NAME_SERVERS));
+    // Nor a sntp-servers
+    ASSERT_FALSE(response->getOption(D6O_SNTP_SERVERS));
 }
 
 // Checks if server is able to handle a relayed traffic from DOCSIS3.0 modems

diff --git a/src/bin/dhcp6/tests/vendor_opts_unittest.cc b/src/bin/dhcp6/tests/vendor_opts_unittest.cc
index 0836d0101df8c4c1b5d073a03baabb83c56cd1a6..e869c69c24f00777d08407b1a7363aa14fb840ca 100644 (file)
--- a/src/bin/dhcp6/tests/vendor_opts_unittest.cc
+++ b/src/bin/dhcp6/tests/vendor_opts_unittest.cc
@@ -268,6 +268,36 @@ TEST_F(VendorOptsTest, vendorPersistentOptions) {
     OptionStringPtr config_file = boost::dynamic_pointer_cast<OptionString>(docsis33);
     ASSERT_TRUE(config_file);
     EXPECT_EQ("normal_erouter_v6.cm", config_file->getValue());
+
+    // Let's add a vendor-option (vendor-id=4491) with a single sub-option.
+    // That suboption has code 1 and is a docsis ORO option.
+    sol->delOption(D6O_VENDOR_OPTS);
+    boost::shared_ptr<OptionUint16Array> vendor_oro(new OptionUint16Array(Option::V6,
+                                                                          DOCSIS3_V6_ORO));
+    vendor_oro->addValue(DOCSIS3_V6_CONFIG_FILE); // Request option 33
+    OptionPtr vendor2(new OptionVendor(Option::V6, 4491));
+    vendor2->addOption(vendor_oro);
+    sol->addOption(vendor2);
+
+    // Need to process SOLICIT again after requesting new option.
+    AllocEngine::ClientContext6 ctx2;
+    srv_.initContext(sol, ctx2, drop);
+    ASSERT_FALSE(drop);
+    adv = srv_.processSolicit(ctx2);
+    ASSERT_TRUE(adv);
+
+    // Check if there is vendor option response
+    tmp = adv->getOption(D6O_VENDOR_OPTS);
+    ASSERT_TRUE(tmp);
+
+    // The response should be OptionVendor object
+    vendor_resp = boost::dynamic_pointer_cast<OptionVendor>(tmp);
+    ASSERT_TRUE(vendor_resp);
+
+    // There should be only one suboption despite config-file is both
+    // requested and has the always-send flag.
+    const OptionCollection& opts = vendor_resp->getOptions();
+    ASSERT_EQ(1, opts.size());
 }
 
 // Test checks whether it is possible to use option definitions defined in