[#4496] Added cleanup/decapsulation

diff --git a/changelog_unreleased/4496-dhcp6-and-ntp-option-56-only-one-ipv6-ntp-server-is-possible b/changelog_unreleased/4496-dhcp6-and-ntp-option-56-only-one-ipv6-ntp-server-is-possible
new file mode 100644 (file)
index 0000000..0301990
--- /dev/null
+++ b/changelog_unreleased/4496-dhcp6-and-ntp-option-56-only-one-ipv6-ntp-server-is-possible
@@ -0,0 +1,8 @@
+[func]         fdupont
+       Changed the way sub-options are encapsulated for
+       special option spaces to allow multiple suboptions
+       with the same code point. Currently only the
+       "v6-ntp-server-suboptions" space of the DHCPv6
+       "ntp-server" is special but if this feature is
+       useful for other spaces it can be made configurable.
+       (Gitlab #4496)

diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc
index f6f07c44cd63445fac53b04ad3bdee334619a7e6..bef6d9e539e65fa0e18453bd343fe9a393a1b561 100644 (file)
--- a/src/bin/dhcp6/tests/config_parser_unittest.cc
+++ b/src/bin/dhcp6/tests/config_parser_unittest.cc
@@ -3841,6 +3841,79 @@ TEST_F(Dhcp6ParserTest, optionDataEncapsulate) {
     EXPECT_EQ(111U, option_foo2->getType());
 }
 
+// The v6-ntp-server-suboptions space is special because it allows
+// multiple sub-options of the same type.
+TEST_F(Dhcp6ParserTest, optionDataNtpServer) {
+        string config = "{ " + genIfaceConfig() + ","
+        "\"preferred-lifetime\": 3000,"
+        "\"valid-lifetime\": 4000,"
+        "\"rebind-timer\": 2000,"
+        "\"renew-timer\": 1000,"
+        "\"option-data\": [ {"
+        "    \"name\": \"ntp-server\""
+        " },"
+        " {"
+        "    \"name\": \"ntp-server-address\","
+        "    \"space\": \"v6-ntp-server-suboptions\","
+        "    \"data\": \"2001:db8::77\""
+        " },"
+        " {"
+        "    \"name\": \"ntp-server-address\","
+        "    \"space\": \"v6-ntp-server-suboptions\","
+        "    \"data\": \"2001:db8::88\""
+        " } ]"
+        "}";
+
+    ConstElementPtr json;
+    ASSERT_NO_THROW(json = parseDHCP6(config));
+    extractConfig(config);
+
+    ConstElementPtr status;
+    EXPECT_NO_THROW(status = Dhcpv6SrvTest::configure(srv_, json));
+    ASSERT_TRUE(status);
+    checkResult(status, 0);
+
+        // We should have one option available.
+    OptionContainerPtr options =
+        CfgMgr::instance().getStagingCfg()->getCfgOption()->getAll(DHCP6_OPTION_SPACE);
+    ASSERT_TRUE(options);
+    ASSERT_EQ(1U, options->size());
+
+    // Get the option.
+    OptionDescriptor desc =
+        CfgMgr::instance().getStagingCfg()->getCfgOption()->get(DHCP6_OPTION_SPACE, D6O_NTP_SERVER);
+    EXPECT_TRUE(desc.option_);
+    EXPECT_EQ(D6O_NTP_SERVER, desc.option_->getType());
+
+    // This option should comprise two sub-options.
+    auto const& subs = desc.option_->getOptions();
+    ASSERT_EQ(2U, subs.size());
+
+    // Check sub-options.
+    bool got_first = false;
+    bool got_second = false;
+    IOAddress addr1("2001:db8::77");
+    IOAddress addr2("2001:db8::88");
+    auto const buf1 = addr1.toBytes();
+    auto const buf2 = addr2.toBytes();
+    for (auto const& sub : subs) {
+        ASSERT_EQ(NTP_SUBOPTION_SRV_ADDR, sub.first);
+        ASSERT_TRUE(sub.second);
+        ASSERT_EQ(NTP_SUBOPTION_SRV_ADDR, sub.second->getType());
+        if (sub.second->getData() == buf1) {
+            EXPECT_FALSE(got_first);
+            got_first = true;
+        } else if (sub.second->getData() == buf2) {
+            EXPECT_FALSE(got_second);
+            got_second = true;
+        } else {
+            FAIL() << "unexpected content of " << sub.second->toText();
+        }
+    }
+    EXPECT_TRUE(got_first);
+    EXPECT_TRUE(got_second);
+}
+
 // Goal of this test is to verify options configuration
 // for a single subnet. In particular this test checks
 // that local options configuration overrides global

diff --git a/src/lib/dhcpsrv/cfg_option.cc b/src/lib/dhcpsrv/cfg_option.cc
index 3a07578b1c7186113409be1ed6dcad7af0d9dedb..7df192bd115c3825fce950ac480da7ab62ce5433 100644 (file)
--- a/src/lib/dhcpsrv/cfg_option.cc
+++ b/src/lib/dhcpsrv/cfg_option.cc
@@ -335,6 +335,14 @@ CfgOption::encapsulateInternal(const OptionPtr& option) {
     if (encap_space == DHCP4_OPTION_SPACE || encap_space == DHCP6_OPTION_SPACE) {
         return;
     }
+    // Handle multiple encapsulating spaces.
+    bool multiple_encapsulating = false;
+    if (multiple_encapsulating_spaces_.count(encap_space) > 0) {
+        multiple_encapsulating = true;
+        // Cleanup the option so encapsulate() can be called again.
+        decapsulateInternal(option);
+    }
+
     // Retrieve all options from the encapsulated option space.
     OptionContainerPtr encap_options = getAll(encap_space);
     for (auto const& encap_opt : *encap_options) {
@@ -346,14 +354,23 @@ CfgOption::encapsulateInternal(const OptionPtr& option) {
         // Add sub-option if there isn't one added already, or
         // if encapsulating space is a multiple exception.
         OptionPtr existing = option->getOption(encap_opt.option_->getType());
-        if (!existing ||
-            (multiple_encapsulating_spaces_.count(encap_space) > 0)) {
+        if (multiple_encapsulating |
+            !option->getOption(encap_opt.option_->getType())) {
             option->addOption(encap_opt.option_);
         }
         encapsulateInternal(encap_opt.option_);
     }
 }
 
+void
+CfgOption::decapsulateInternal(const OptionPtr& option) {
+    // Dereference any existing sub options.
+    auto sub_options = option->getOptions();
+    for (auto const& sub : sub_options) {
+        option->delOption(sub.second->getType());
+    }
+}
+
 template <typename Selector>
 void
 CfgOption::mergeInternal(const OptionSpaceContainer<OptionContainer,
@@ -505,16 +522,8 @@ CfgOption::del(const uint64_t id) {
         // Get all options for the option space.
         auto const& options = getAll(space_name);
         for (auto const& option_it : *options) {
-            if (!option_it.option_) {
-                continue;
-            }
-
-            // For each option within the option space we need to dereference
-            // any existing sub options.
-            auto sub_options = option_it.option_->getOptions();
-            for (auto const& sub : sub_options) {
-                // Dereference sub option.
-                option_it.option_->delOption(sub.second->getType());
+            if (option_it.option_) {
+                decapsulateInternal(option_it.option_);
             }
         }
     }

diff --git a/src/lib/dhcpsrv/cfg_option.h b/src/lib/dhcpsrv/cfg_option.h
index 9727a70aeed744a0e23ec3c7877cfd255b4c1e2f..48779ce27bdcb85518649365e9cca5eaf1226351 100644 (file)
--- a/src/lib/dhcpsrv/cfg_option.h
+++ b/src/lib/dhcpsrv/cfg_option.h
@@ -936,6 +936,11 @@ private:
     /// @param option which encapsulated options.
     void encapsulateInternal(const OptionPtr& option);
 
+    /// @brief Undo encapsulation for an option.
+    ///
+    /// @param option which decapsulated options.
+    void decapsulateInternal(const OptionPtr& option);
+
     /// @brief Merges data from two option containers.
     ///
     /// This method merges options from one option container to another

diff --git a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc
index 6625174123692659a9c3b88b276b260634b85432..7dfbb84fc4e63309b36432be0d70823056772393 100644 (file)
--- a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc
+++ b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc
@@ -839,6 +839,15 @@ TEST_F(CfgOptionTest, multipleEncapsulatingSpaces) {
     for (auto const& sub : subs) {
         EXPECT_EQ(NTP_SUBOPTION_SRV_ADDR, sub.first);
     }
+
+    // Encapsulate again.
+    ASSERT_NO_THROW(cfg.encapsulate());
+
+    // Check we have ntp-server with still 2 (not 4) sub-options.
+    desc = cfg.get(DHCP6_OPTION_SPACE, D6O_NTP_SERVER);
+    opt = desc.option_;
+    ASSERT_TRUE(opt);
+    ASSERT_EQ(2U, opt->getOptions().size());
 }
 
 // This test verifies that an option can be deleted from the configuration.