[5351] Checkpoint: found a shared-network bug

doc/examples/kea4/advanced.json

@@ -120,9 +120,9 @@
             "subnet": "192.0.2.0/24",
             "user-context": {
                 "comment": "Our first subnet!"
-            }
+            },
             // Equivalent using smart parser
-            // "comment": "Our first subnet!"
+            "comment": "Our first subnet!"
         },
         {
             // This particular subnet has match-client-id value changed.

doc/examples/kea6/advanced.json

@@ -107,7 +107,7 @@
         // Here's the user-context for the whole subnet.
         "user-context": { "comment": "Floor one, west wing" },
         // Equivalent using smart parser
-        // "comment": "Floor one, west wing",
+        "comment": "Floor one, west wing",
 
         // This defines PD (prefix delegation) pools. In this case
         // we have only one pool. That consists of /64 prefixes

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -5607,4 +5607,74 @@ TEST_F(Dhcp4ParserTest, sharedNetworksDeriveClientClass) {
     EXPECT_TRUE(classes.empty());
 }
 
+// This test checks comments. Please keep it last.
+TEST_F(Dhcp4ParserTest, comments) {
+
+    string config = "{\n"
+        "\"shared-networks\": [ {\n"
+        "    \"name\": \"foo\"\n,"
+        "    \"comment\": \"A shared-network\"\n,"
+        "    \"subnet4\": [\n"
+        "    { \n"
+        "        \"subnet\": \"192.0.1.0/24\",\n"
+        "        \"comment\": \"A subnet\"\n,"
+        "        \"pools\": [\n"
+        "        {\n"
+        "             \"pool\": \"192.0.1.1-192.0.1.10\",\n"
+        "             \"comment\": \"A pool\"\n"
+        "        }\n"
+        "        ]\n"
+        "    }\n"
+        "    ]\n"
+        " } ]\n"
+        "} \n";
+
+    extractConfig(config);
+    configure(config, CONTROL_RESULT_SUCCESS, "");
+
+    // Now verify that the shared network was indeed configured.
+    CfgSharedNetworks4Ptr cfg_net = CfgMgr::instance().getStagingCfg()
+        ->getCfgSharedNetworks4();
+    ASSERT_TRUE(cfg_net);
+    const SharedNetwork4Collection* nets = cfg_net->getAll();
+    ASSERT_TRUE(nets);
+    ASSERT_EQ(1, nets->size());
+    SharedNetwork4Ptr net = nets->at(0);
+    ASSERT_TRUE(net);
+
+    // Check shared network user context
+    ConstElementPtr ctx_net = net->getContext();
+    ASSERT_TRUE(ctx_net);
+    ASSERT_EQ(1, ctx_net->size());
+    ASSERT_TRUE(ctx_net->get("comment"));
+    EXPECT_EQ("\"A shared-network\"", ctx_net->get("comment")->str());
+
+    // The shared network has a subnet.
+    const Subnet4Collection * subs = net->getAllSubnets();
+    ASSERT_TRUE(subs);
+    ASSERT_EQ(1, subs->size());
+    Subnet4Ptr sub = subs->at(0);
+    ASSERT_TRUE(sub);
+
+    // Check subnet user context
+    ConstElementPtr ctx_sub = sub->getContext();
+    ASSERT_TRUE(ctx_sub);
+    ASSERT_EQ(1, ctx_sub->size());
+    ASSERT_TRUE(ctx_sub->get("comment"));
+    EXPECT_EQ("\"A subnet\"", ctx_sub->get("comment")->str());
+
+    // The subnet has a pool
+    const PoolCollection& pools = sub->getPools(Lease::TYPE_V4);
+    ASSERT_EQ(1, pools.size());
+    PoolPtr pool = pools.at(0);
+    ASSERT_TRUE(pool);
+
+    // Check pool user context                                               
+    ConstElementPtr ctx_pool = pool->getContext();
+    ASSERT_TRUE(ctx_pool);
+    ASSERT_EQ(1, ctx_pool->size());
+    ASSERT_TRUE(ctx_pool->get("comment"));
+    EXPECT_EQ("\"A pool\"", ctx_pool->get("comment")->str());
+}
+
 }

src/bin/dhcp4/tests/parser_unittest.cc

@@ -8,6 +8,7 @@
 #include <cc/data.h>
 #include <dhcp4/parser_context.h>
 #include <testutils/io_utils.h>
+#include <testutils/user_context_utils.h>
 
 using namespace isc::data;
 using namespace isc::test;
@@ -209,6 +210,7 @@ TEST(ParserTest, multilineComments) {
 ///
 /// @param fname name of the file to be loaded
 void testFile(const std::string& fname) {
+    ElementPtr json;
     ElementPtr reference_json;
     ConstElementPtr test_json;
 
@@ -216,7 +218,8 @@ void testFile(const std::string& fname) {
 
     cout << "Parsing file " << fname << " (" << decommented << ")" << endl;
 
-    EXPECT_NO_THROW(reference_json = Element::fromJSONFile(decommented, true));
+    EXPECT_NO_THROW(json = Element::fromJSONFile(decommented, true));
+    reference_json = moveComments(json);
 
     // remove the temporary file
     EXPECT_NO_THROW(::remove(decommented.c_str()));

src/bin/dhcp6/tests/parser_unittest.cc

@@ -8,6 +8,7 @@
 #include <cc/data.h>
 #include <dhcp6/parser_context.h>
 #include <testutils/io_utils.h>
+#include <testutils/user_context_utils.h>
 
 using namespace isc::data;
 using namespace std;
@@ -212,6 +213,7 @@ TEST(ParserTest, multilineComments) {
 ///
 /// @param fname name of the file to be loaded
 void testFile(const std::string& fname) {
+    ElementPtr json;
     ElementPtr reference_json;
     ConstElementPtr test_json;
 
@@ -219,7 +221,8 @@ void testFile(const std::string& fname) {
 
     cout << "Parsing file " << fname << "(" << decommented << ")" << endl;
 
-    EXPECT_NO_THROW(reference_json = Element::fromJSONFile(decommented, true));
+    EXPECT_NO_THROW(json = Element::fromJSONFile(decommented, true));
+    reference_json = moveComments(json);
 
     // remove the temporary file
     EXPECT_NO_THROW(::remove(decommented.c_str()));

src/lib/dhcpsrv/pool.h

@@ -135,11 +135,6 @@ protected:
     /// @brief The last address in a pool
     isc::asiolink::IOAddress last_;
 
-    /// @brief Comments field
-    ///
-    /// @todo: This field is currently not used.
-    std::string comments_;
-
     /// @brief defines a lease type that will be served from this pool
     Lease::Type type_;
 

src/lib/testutils/Makefile.am

@@ -10,7 +10,8 @@ noinst_LTLIBRARIES = libkea-testutils.la
 libkea_testutils_la_SOURCES  = io_utils.cc io_utils.h
 libkea_testutils_la_SOURCES += log_utils.cc log_utils.h
 libkea_testutils_la_SOURCES += test_to_element.cc test_to_element.h
-libkea_testutils_la_SOURCES += unix_control_client.h unix_control_client.cc
+libkea_testutils_la_SOURCES += unix_control_client.cc unix_control_client.h
+libkea_testutils_la_SOURCES += user_context_utils.cc user_context_utils.h
 libkea_testutils_la_CPPFLAGS = $(AM_CPPFLAGS) $(GTEST_INCLUDES)
 libkea_testutils_la_LIBADD  = $(top_builddir)/src/lib/asiolink/libkea-asiolink.la
 libkea_testutils_la_LIBADD += $(top_builddir)/src/lib/dns/libkea-dns++.la

src/lib/testutils/user_context_utils.cc

@@ -0,0 +1,125 @@
+// Copyright (C) 2017 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/.
+
+#include <config.h>
+
+#include <testutils/user_context_utils.h>
+
+using namespace isc::data;
+
+namespace {
+
+/// @brief Exception to handle a unmodified value
+/// @tparam EP ElementPtr or ConstElementPtr (compiler can't infer which one)
+template<typename EP>
+class UnModified {
+public:
+    /// @brief Constructor
+    /// @param value the unmodified value
+    UnModified(EP value) : value_(value) { }
+
+    /// @brief Get the value
+    /// @return the value
+    EP get() const { return (value_); }
+
+protected:
+    /// @brief the unmodified value
+    EP value_;
+};
+
+/// @brief Recursive helper
+///
+/// Instead of returning a unmodified copy a @ref UnModified exception
+/// is raised with the unmodified value
+///
+/// @tparam EP ElementPtr or ConstElementPtr (compiler will infer which one)
+/// @param element the element to traverse
+/// @return a modified copy where comment entries were moved to user-context
+/// @throw UnModified with the unmodified value
+template<typename EP>
+EP moveComments1(EP element) {
+    bool modified = false;
+
+    // On lists recurse on items
+    if (element->getType() == Element::list) {
+        ElementPtr result = ElementPtr(new ListElement());
+        typedef std::vector<ElementPtr> ListType;
+        const ListType& list = element->listValue();
+        for (ListType::const_iterator it = list.cbegin();
+             it != list.cend(); ++it) {
+            try {
+                result->add(moveComments1(*it));
+                modified = true;
+            }
+            catch (const UnModified<ElementPtr>& ex) {
+                result->add(ex.get());
+            }
+        }
+        if (!modified) {
+            throw UnModified<EP>(element);
+        }
+        return (result);
+    } else if (element->getType() != Element::map) {
+        throw UnModified<EP>(element);
+    }
+
+    // Process maps: recurse on items
+    ElementPtr result = ElementPtr(new MapElement());
+    bool has_comment = false;
+    typedef std::map<std::string, ConstElementPtr> map_type;
+    const map_type& map = element->mapValue();
+    for (map_type::const_iterator it = map.cbegin(); it != map.cend(); ++it) {
+        if (it->first == "comment") {
+            // Note there is a comment entry to move
+            has_comment = true;
+        } else if (it->first == "user-context") {
+            // Do not traverse user-context entries
+            result->set("user-context", it->second);
+        } else {
+            // Not comment or user-context
+            try {
+                result->set(it->first, moveComments1(it->second));
+                modified = true;
+            }
+            catch (const UnModified<ConstElementPtr>& ex) {
+                result->set(it->first, ex.get());
+            }
+        }
+    }
+    // Check if the value should be not modified
+    if (!has_comment && !modified) {
+        throw UnModified<EP>(element);
+    }
+
+    if (has_comment) {
+        // Move the comment entry
+        ConstElementPtr comment = element->get("comment");
+        ElementPtr moved = Element::createMap();
+        moved->set("comment", comment);
+        result->combine_set("user-context", moved);
+    }
+
+    return (result);
+}
+
+} // anonymous namespace
+
+namespace isc {
+namespace test {
+
+ElementPtr moveComments(ElementPtr element) {
+    ElementPtr result;
+    try {
+        result = moveComments1(element);
+    }
+    catch (const UnModified<ElementPtr>& ex) {
+        result = ex.get();
+    }
+    return result;
+}
+
+}; // end of isc::test namespace
+}; // end of isc namespace

src/lib/testutils/user_context_utils.h

@@ -0,0 +1,24 @@
+// Copyright (C) 2017 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/.
+
+#ifndef USER_CONTEXT_UTILS_H
+#define USER_CONTEXT_UTILS_H
+
+#include <cc/data.h>
+
+namespace isc {
+namespace test {
+
+/// @brief Move comment entries to user-context
+///
+/// @param element
+/// @return a processed copy of element or unmodified element
+isc::data::ElementPtr moveComments(isc::data::ElementPtr element);
+
+}; // end of isc::test namespace
+}; // end of isc namespace
+
+#endif // USER_CONTEXT_UTILS_H