[3436] Added position info to top level parser creation in D2

Added element position argument to DCfgMgrBase::createConfigParser(),
so derivations have access to print position info unsupported top level
element errors.

Removed two log messages DCT_ORDER_ERROR and DCT_ORDER_NO_ELEMENT. These
conditions are well explained in exceptions thrown and these logs just
cluttered the log output.

Removed extra text from DCTL_CONFIG_LOAD_FAIL and DCTL_PARSER_FAIL log
messages. The log ID is self-explanatory and the underlying exceptions
provide ample explanation of the error.  Makes the log output much easier
to understand.

Revised items-not-in-parse-order detection in DCfgMgrBase::parseConfig().
Rather than complicated counting logic, objects are removed from the list
as they are parsed.  Any left over were not in the parsing-order.

Removed try-catch-throw from DCfgMgrBase::buildAndCommit. This method
already throws its own exception. Catching, logging, and re-throwing
exceptions from underneath it really just server to clutter the log.

src/bin/d2/d2_cfg_mgr.cc

@@ -234,7 +234,8 @@ D2CfgMgr::buildParams(isc::data::ConstElementPtr params_config) {
 }
 
 isc::dhcp::ParserPtr
-D2CfgMgr::createConfigParser(const std::string& config_id) {
+D2CfgMgr::createConfigParser(const std::string& config_id,
+                             const isc::data::Element::Position& pos) {
     // Get D2 specific context.
     D2CfgContextPtr context = getD2CfgContext();
 
@@ -263,7 +264,7 @@ D2CfgMgr::createConfigParser(const std::string& config_id) {
     } else {
         isc_throw(NotImplemented,
                   "parser error: D2CfgMgr parameter not supported: "
-                  << config_id);
+                  << config_id << pos);
     }
 
     return (parser);

src/bin/d2/d2_cfg_mgr.h

@@ -264,11 +264,15 @@ protected:
     ///
     /// @param element_id is the string name of the element as it will appear
     /// in the configuration set.
+    /// @param pos position within the configuration text (or file) of element
+    /// to be parsed.  This is passed for error messaging.
     ///
     /// @return returns a ParserPtr to the parser instance.
     /// @throw throws DCfgMgrBaseError if an error occurs.
     virtual isc::dhcp::ParserPtr
-    createConfigParser(const std::string& element_id);
+    createConfigParser(const std::string& element_id,
+                       const isc::data::Element::Position& pos =
+                       isc::data::Element::Position());
 
     /// @brief Creates an new, blank D2CfgContext context
     ///

src/bin/d2/d2_messages.mes

@@ -32,7 +32,7 @@ new configuration. It is output during server startup, and when an updated
 configuration is committed by the administrator.  Additional information
 may be provided.
 
-% DCTL_CONFIG_FILE_LOAD_FAIL %1 configuration could not be loaded from file: %2
+% DCTL_CONFIG_FILE_LOAD_FAIL %1 : %2
 This fatal error message indicates that the application attempted to load its
 initial configuration from file and has failed. The service will exit.
 
@@ -72,22 +72,11 @@ application and will exit.
 A warning message is issued when an attempt is made to shut down the
 application when it is not running.
 
-% DCTL_ORDER_ERROR configuration contains more elements than the parsing order
-An error message which indicates that configuration being parsed includes
-element ids not specified the configuration manager's parse order list. This
-is a programmatic error.
-
-% DCTL_ORDER_NO_ELEMENT element: %1 is in the parsing order but is missing from the configuration
-An error message output during a configuration update.  The program is
-expecting an item but has not found it in the new configuration.  This may
-mean that the BIND 10 configuration database is corrupt.
-
-% DCTL_PARSER_FAIL configuration parsing failed for configuration element: %1, reason: %2
-On receipt of message containing details to a change of its configuration,
-the server failed to create a parser to decode the contents of the named
-configuration element, or the creation succeeded but the parsing actions
-and committal of changes failed.  The reason for the failure is given in
-the message.
+% DCTL_PARSER_FAIL %1
+On receipt of a new configuration, the server failed to create a parser to
+decode the contents of the named configuration element, or the creation
+succeeded but the parsing actions and committal of changes failed.
+The reason for the failure is given in the message.
 
 % DCTL_PROCESS_FAILED %1 application execution failed: %2
 The controller has encountered a fatal error while running the

src/bin/d2/d_cfg_mgr.cc

@@ -189,17 +189,15 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) {
             // thrown.  Note, that elements tagged as "optional" from the user
             // perspective must still have default or empty entries in the
             // configuration set to be parsed.
-            int parsed_count = 0;
             std::map<std::string, ConstElementPtr>::const_iterator it;
             BOOST_FOREACH(element_id, parse_order_) {
                 it = objects_map.find(element_id);
                 if (it != objects_map.end()) {
-                    ++parsed_count;
                     buildAndCommit(element_id, it->second);
+                    // We parsed it, take it out of the list.
+                    objects_map.erase(it);
                 }
                 else {
-                    LOG_ERROR(dctl_logger, DCTL_ORDER_NO_ELEMENT)
-                              .arg(element_id);
                     isc_throw(DCfgMgrBaseError, "Element:" << element_id <<
                               " is listed in the parse order but is not "
                               " present in the configuration");
@@ -208,18 +206,25 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) {
 
             // NOTE: When using ordered parsing, the parse order list MUST
             // include every possible element id that the value_map may contain.
-            // Entries in the map that are not in the parse order, would not be
+            // Entries in the map that are not in the parse order, will not be
             // parsed. For now we will flag this as a programmatic error.  One
             // could attempt to adjust for this, by identifying such entries
             // and parsing them either first or last but which would be correct?
-            // Better to hold the engineer accountable.  So, if we parsed none
-            // or we parsed fewer than are in the map; then either the parse i
-            // order is incomplete OR the map has unsupported values.
-            if (!parsed_count ||
-                (parsed_count && ((parsed_count + 1) < objects_map.size()))) {
-                LOG_ERROR(dctl_logger, DCTL_ORDER_ERROR);
+            // Better to hold the engineer accountable.  So, if there are any
+            // left in the objects_map then they were not in the parse order.
+            if (objects_map.size() > 0) {
+                std::ostringstream stream;
+                bool add_comma = false;
+                ConfigPair config_pair;
+                BOOST_FOREACH(config_pair, objects_map) {
+                    stream << ( add_comma ? ", " : "") << config_pair.first
+                           << " at: " << config_pair.second->getPosition();
+                    add_comma = true;
+                }
+
                 isc_throw(DCfgMgrBaseError,
-                        "Configuration contains elements not in parse order");
+                        "Configuration contains elements not in parse order: "
+                        << stream.str());
             }
         } else {
             // Order doesn't matter so iterate over the value map directly.
@@ -235,10 +240,9 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) {
         LOG_INFO(dctl_logger, DCTL_CONFIG_COMPLETE).arg("");
         answer = isc::config::createAnswer(0, "Configuration committed.");
 
-    } catch (const isc::Exception& ex) {
-        LOG_ERROR(dctl_logger, DCTL_PARSER_FAIL).arg(element_id).arg(ex.what());
-        answer = isc::config::createAnswer(1,
-                     string("Configuration parsing failed: ") + ex.what());
+    } catch (const std::exception& ex) {
+        LOG_ERROR(dctl_logger, DCTL_PARSER_FAIL).arg(ex.what());
+        answer = isc::config::createAnswer(1, + ex.what());
 
         // An error occurred, so make sure that we restore original context.
         context_ = original_context;
@@ -253,7 +257,8 @@ DCfgMgrBase::buildParams(isc::data::ConstElementPtr params_config) {
     // Loop through scalars parsing them and committing them to storage.
     BOOST_FOREACH(dhcp::ConfigPair param, params_config->mapValue()) {
         // Call derivation's method to create the proper parser.
-        dhcp::ParserPtr parser(createConfigParser(param.first));
+        dhcp::ParserPtr parser(createConfigParser(param.first,
+                                                  param.second->getPosition()));
         parser->build(param.second);
         parser->commit();
     }
@@ -263,28 +268,21 @@ void DCfgMgrBase::buildAndCommit(std::string& element_id,
                                  isc::data::ConstElementPtr value) {
     // Call derivation's implementation to create the appropriate parser
     // based on the element id.
-    ParserPtr parser = createConfigParser(element_id);
+    ParserPtr parser = createConfigParser(element_id, value->getPosition());
     if (!parser) {
         isc_throw(DCfgMgrBaseError, "Could not create parser");
     }
 
-    try {
-        // Invoke the parser's build method passing in the value. This will
-        // "convert" the Element form of value into the actual data item(s)
-        // and store them in parser's local storage.
-        parser->build(value);
-
-        // Invoke the parser's commit method. This "writes" the the data
-        // item(s) stored locally by the parser into the context.  (Note that
-        // parsers are free to do more than update the context, but that is an
-        // nothing something we are concerned with here.)
-        parser->commit();
-    } catch (const isc::Exception& ex) {
-        isc_throw(DCfgMgrBaseError,
-                  "Could not build and commit: " << ex.what());
-    } catch (...) {
-        isc_throw(DCfgMgrBaseError, "Non-ISC exception occurred");
-    }
+    // Invoke the parser's build method passing in the value. This will
+    // "convert" the Element form of value into the actual data item(s)
+    // and store them in parser's local storage.
+    parser->build(value);
+
+    // Invoke the parser's commit method. This "writes" the the data
+    // item(s) stored locally by the parser into the context.  (Note that
+    // parsers are free to do more than update the context, but that is an
+    // nothing something we are concerned with here.)
+    parser->commit();
 }
 
 }; // end of isc::dhcp namespace

src/bin/d2/d_cfg_mgr.h

@@ -316,11 +316,15 @@ protected:
     ///
     /// @param element_id is the string name of the element as it will appear
     /// in the configuration set.
+    /// @param pos position within the configuration text (or file) of element
+    /// to be parsed.  This is passed for error messaging.
     ///
     /// @return returns a ParserPtr to the parser instance.
     /// @throw throws DCfgMgrBaseError if an error occurs.
     virtual isc::dhcp::ParserPtr
-    createConfigParser(const std::string& element_id) = 0;
+    createConfigParser(const std::string& element_id,
+                       const isc::data::Element::Position& pos
+                       = isc::data::Element::Position()) = 0;
 
     /// @brief Abstract factory which creates a context instance.
     ///

src/bin/d2/tests/d2_cfg_mgr_unittests.cc

@@ -83,6 +83,12 @@ public:
         return (config.str());
     }
 
+    /// @brief Enumeration to select between expected configuration outcomes
+    enum RunConfigMode {
+       SHOULD_PASS,
+       SHOULD_FAIL
+    };
+
     /// @brief Parses a configuration string and tests against a given outcome
     ///
     /// Convenience method which accepts JSON text and an expected pass or fail
@@ -93,15 +99,16 @@ public:
     /// the D2Params pointer with the newly parsed instance.
     ///
     /// @param config_str the JSON configuration text to parse
-    /// @param should_fail boolean indicator if the parsing should fail or not.
-    /// It defaults to false.
-    void runConfig(std::string config_str, bool should_fail=false) {
+    /// @param mode indicator if the parsing should fail or not.  It defaults
+    /// defaults to SHOULD_PASS.
+    ///
+    void runConfig(std::string config_str, RunConfigMode mode=SHOULD_PASS) {
         // We assume the config string is valid JSON.
         ASSERT_TRUE(fromJSON(config_str));
 
         // Parse the configuration and verify we got the expected outcome.
         answer_ = cfg_mgr_->parseConfig(config_set_);
-        ASSERT_TRUE(checkAnswer(should_fail));
+        ASSERT_TRUE(checkAnswer(mode == SHOULD_FAIL));
 
         // Verify that the D2 context can be retrieved and is not null.
         D2CfgContextPtr context;
@@ -397,6 +404,44 @@ TEST_F(D2CfgMgrTest, defaultValues) {
               d2_params_->getNcrFormat());
 }
 
+/// @brief Tests the unsupported scalar parameters and objects are detected.
+TEST_F(D2CfgMgrTest, unsupportedTopLevelItems) {
+    // Check that an unsupported top level parameter fails.
+    std::string config =
+            "{"
+            " \"ip_address\": \"127.0.0.1\", "
+            " \"port\": 777 , "
+            " \"dns_server_timeout\": 333 , "
+            " \"ncr_protocol\": \"UDP\" , "
+            " \"ncr_format\": \"JSON\", "
+            "\"tsig_keys\": [], "
+            "\"forward_ddns\" : {}, "
+            "\"reverse_ddns\" : {}, "
+            "\"bogus_param\" : true "
+            "}";
+
+    runConfig(config, SHOULD_FAIL);
+
+    // Check that unsupported top level objects fails.  For
+    // D2 these fail as they are not in the parse order.
+    config =
+            "{"
+            " \"ip_address\": \"127.0.0.1\", "
+            " \"port\": 777 , "
+            " \"dns_server_timeout\": 333 , "
+            " \"ncr_protocol\": \"UDP\" , "
+            " \"ncr_format\": \"JSON\", "
+            "\"tsig_keys\": [], "
+            "\"bogus_object_one\" : {}, "
+            "\"forward_ddns\" : {}, "
+            "\"reverse_ddns\" : {}, "
+            "\"bogus_object_two\" : {} "
+            "}";
+
+    runConfig(config, SHOULD_FAIL);
+}
+
+
 /// @brief Tests the enforcement of data validation when parsing D2Params.
 /// It verifies that:
 /// -# ip_address cannot be "0.0.0.0"
@@ -409,27 +454,27 @@ TEST_F(D2CfgMgrTest, invalidEntry) {
     // Cannot use IPv4 ANY address
     std::string config = makeParamsConfigString ("0.0.0.0", 777, 333,
                                            "UDP", "JSON");
-    runConfig(config, 1);
+    runConfig(config, SHOULD_FAIL);
 
     // Cannot use IPv6 ANY address
     config = makeParamsConfigString ("::", 777, 333, "UDP", "JSON");
-    runConfig(config, 1);
+    runConfig(config, SHOULD_FAIL);
 
     // Cannot use port  0
     config = makeParamsConfigString ("127.0.0.1", 0, 333, "UDP", "JSON");
-    runConfig(config, 1);
+    runConfig(config, SHOULD_FAIL);
 
     // Cannot use dns server timeout of 0
     config = makeParamsConfigString ("127.0.0.1", 777, 0, "UDP", "JSON");
-    runConfig(config, 1);
+    runConfig(config, SHOULD_FAIL);
 
     // Invalid protocol
     config = makeParamsConfigString ("127.0.0.1", 777, 333, "BOGUS", "JSON");
-    runConfig(config, 1);
+    runConfig(config, SHOULD_FAIL);
 
     // Invalid format
     config = makeParamsConfigString ("127.0.0.1", 777, 333, "UDP", "BOGUS");
-    runConfig(config, 1);
+    runConfig(config, SHOULD_FAIL);
 }
 
 /// @brief Tests the enforcement of data validation when parsing TSIGKeyInfos.

src/bin/d2/tests/d_cfg_mgr_unittests.cc

@@ -53,7 +53,8 @@ public:
 
     /// @brief Dummy implementation as this method is abstract.
     virtual isc::dhcp::ParserPtr
-    createConfigParser(const std::string& /* element_id */) {
+    createConfigParser(const std::string& /* element_id */,
+                       const isc::data::Element::Position& /* pos */) {
         return (isc::dhcp::ParserPtr());
     }
 };
@@ -125,18 +126,23 @@ TEST_F(DStubCfgMgrTest, basicParseTest) {
     answer_ = cfg_mgr_->parseConfig(config_set_);
     EXPECT_TRUE(checkAnswer(0));
 
+
+std::cout << __FILE__ << ":" <<  __LINE__ << std::endl;
     // Verify that an error building the element is caught and returns a
     // failed parse result.
     SimFailure::set(SimFailure::ftElementBuild);
     answer_ = cfg_mgr_->parseConfig(config_set_);
     EXPECT_TRUE(checkAnswer(1));
 
+std::cout << __FILE__ << ":" <<  __LINE__ << std::endl;
     // Verify that an error committing the element is caught and returns a
     // failed parse result.
     SimFailure::set(SimFailure::ftElementCommit);
     answer_ = cfg_mgr_->parseConfig(config_set_);
     EXPECT_TRUE(checkAnswer(1));
 
+std::cout << __FILE__ << ":" <<  __LINE__ << std::endl;
+
     // Verify that an unknown element error is caught and returns a failed
     // parse result.
     SimFailure::set(SimFailure::ftElementUnknown);

src/bin/d2/tests/d_test_stubs.cc

@@ -396,7 +396,8 @@ DStubCfgMgr::createNewContext() {
 }
 
 isc::dhcp::ParserPtr
-DStubCfgMgr::createConfigParser(const std::string& element_id) {
+DStubCfgMgr::createConfigParser(const std::string& element_id,
+                                const isc::data::Element::Position& pos) {
     isc::dhcp::ParserPtr parser;
     DStubContextPtr context
         = boost::dynamic_pointer_cast<DStubContext>(getContext());
@@ -416,7 +417,8 @@ DStubCfgMgr::createConfigParser(const std::string& element_id) {
         // to "succeed" without specifically supporting them.
         if (SimFailure::shouldFailOn(SimFailure::ftElementUnknown)) {
             isc_throw(DCfgMgrBaseError,
-                      "Configuration parameter not supported: " << element_id);
+                      "Configuration parameter not supported: " << element_id
+                      << pos);
         }
 
         // Going to assume anything else is an object element.

src/bin/d2/tests/d_test_stubs.h

@@ -691,11 +691,15 @@ public:
     ///
     /// @param element_id is the string name of the element as it will appear
     /// in the configuration set.
+    /// @param pos position within the configuration text (or file) of element
+    /// to be parsed.  This is passed for error messaging.
     ///
     /// @return returns a ParserPtr to the parser instance.
     /// @throw throws DCfgMgrBaseError if SimFailure is ftElementUnknown.
     virtual isc::dhcp::ParserPtr
-    createConfigParser(const std::string& element_id);
+    createConfigParser(const std::string& element_id,
+                       const isc::data::Element::Position& pos
+                       = isc::data::Element::Position());
 
     /// @brief A list for remembering the element ids in the order they were
     /// parsed.