From: Thomas Markwalder <tmark@isc.org>
Date: Thu, 19 Jan 2017 18:57:49 +0000 (-0500)
Subject: [5110] Added alternate parsing hooks into DController and DCfgMgrBase
X-Git-Tag: trac1205_base^2~19
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4a5e73f7bc270568906d3f38d978fcffa96f1db3;p=thirdparty%2Fkea.git

[5110] Added alternate parsing hooks into DController and DCfgMgrBase

In order to accomodate bison parsing for JSON text and SimpleParser
based element parsing, virtual methods were added to allow derivations
to migrate.

src/lib/process/d_cfg_mgr.h
src/lib/process/d_cfg_mgr.cc
    DCfgMgrBase::parseElement() - new method to allow derivaitons
    to support alternate Element parsers on a element by element basis

    DCfgMgrBase::buildParams()
    DCfgMgrBase::buildAndCommit() - added call to parseElement()

src/lib/process/d_controller.h
src/lib/process/d_controller.cc
    DControllerBase::parseFile() - new method to allow derivations
    to use alternate JSON parsers
    DControllerBase::configFromFile() - added call to parseFile()
---

diff --git a/src/lib/process/d_cfg_mgr.cc b/src/lib/process/d_cfg_mgr.cc
index 21475912c1..babf27b369 100644
--- a/src/lib/process/d_cfg_mgr.cc
+++ b/src/lib/process/d_cfg_mgr.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-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
@@ -259,6 +259,12 @@ void
 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 element parser, if it handled the element
+        // go on to the next one, otherwise use the old methods
+        if (parseElement(param.first, param.second)) {
+            continue;
+        }
+
         // Call derivation's method to create the proper parser.
         dhcp::ParserPtr parser(createConfigParser(param.first,
                                                   param.second->getPosition()));
@@ -269,6 +275,12 @@ DCfgMgrBase::buildParams(isc::data::ConstElementPtr params_config) {
 
 void DCfgMgrBase::buildAndCommit(std::string& element_id,
                                  isc::data::ConstElementPtr value) {
+    // Call derivation's element parser, if it handled the element
+    // go on to the next one, otherwise use the old methods
+    if (parseElement(element_id, value)) {
+        return;
+    }
+
     // Call derivation's implementation to create the appropriate parser
     // based on the element id.
     ParserPtr parser = createConfigParser(element_id, value->getPosition());
diff --git a/src/lib/process/d_cfg_mgr.h b/src/lib/process/d_cfg_mgr.h
index 34453ccadf..47f56f7a50 100644
--- a/src/lib/process/d_cfg_mgr.h
+++ b/src/lib/process/d_cfg_mgr.h
@@ -320,13 +320,33 @@ public:
     virtual std::string getConfigSummary(const uint32_t selection) = 0;
 
 protected:
+
+    /// @brief Parses the an element using an alternate parsing mechanism
+    ///
+    /// Each element to be parsed is passed first into this method to allow
+    /// alternate parsing mechanisms to process specific elements.  The method
+    /// should return true if it has processed the element or false if the
+    /// element should be passed onto the original parsing mechanisms.
+    /// The default implementation simply returns false.
+    ///
+    /// @param element_id name of the element as it is expected in the cfg
+    /// @param element value of the element as ElementPtr
+    ///
+    /// @return true if the element was parsed, false otherwise
+    virtual bool parseElement(const std::string& element_id,
+                              isc::data::ConstElementPtr element) {
+        return (false);
+    };
+
     /// @brief Parses a set of scalar configuration elements into global
     /// parameters
     ///
     /// For each scalar element in the set:
-    ///  - create a parser for the element
-    ///  - invoke the parser's build method
-    ///  - invoke the parser's commit method
+    /// - Invoke parseElement
+    /// - If it returns true go to the next element othewise:
+    ///     - create a parser for the element
+    ///     - invoke the parser's build method
+    ///     - invoke the parser's commit method
     ///
     /// This will commit the values to context storage making them accessible
     /// during object parsing.
@@ -377,8 +397,10 @@ private:
 
     /// @brief Parse a configuration element.
     ///
-    /// Given an element_id and data value, instantiate the appropriate
-    /// parser,  parse the data value, and commit the results.
+    /// Given an element_id and data value, invoke parseElement. If
+    /// it returns true the return, otherwise created the appropriate
+    /// parser, parse the data value, and commit the results.
+    ///
     ///
     /// @param element_id is the string name of the element as it will appear
     /// in the configuration set.
diff --git a/src/lib/process/d_controller.cc b/src/lib/process/d_controller.cc
index 851b97fe15..3caa71d256 100644
--- a/src/lib/process/d_controller.cc
+++ b/src/lib/process/d_controller.cc
@@ -257,9 +257,13 @@ DControllerBase::configFromFile() {
                                 "use -c command line option.");
         }
 
-        // Read contents of the file and parse it as JSON
-        isc::data::ConstElementPtr whole_config =
-            isc::data::Element::fromJSONFile(config_file, true);
+        // If parseFile returns an empty pointer, then pass the file onto the
+        // original JSON parser.
+        isc::data::ConstElementPtr whole_config = parseFile(config_file);
+        if (!whole_config) {
+            // Read contents of the file and parse it as JSON
+            whole_config = isc::data::Element::fromJSONFile(config_file, true);
+        }
 
         // Let's configure logging before applying the configuration,
         // so we can log things during configuration process.
diff --git a/src/lib/process/d_controller.h b/src/lib/process/d_controller.h
index 0c90f46209..7bdc4c54a8 100644
--- a/src/lib/process/d_controller.h
+++ b/src/lib/process/d_controller.h
@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2015,2017 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-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
@@ -113,7 +113,7 @@ public:
     /// @brief returns Kea version on stdout and exit.
     /// redeclaration/redefinition. @ref isc::dhcp::Daemon::getVersion()
     static std::string getVersion(bool extended);
- 
+
     /// @brief Acts as the primary entry point into the controller execution
     /// and provides the outermost application control logic:
     ///
@@ -167,7 +167,11 @@ public:
     /// include at least:
     ///
     /// @code
-    ///  { "<module-name>": {<module-config>} }
+    ///  { "<module-name>": {<module-config>}
+    ///
+    ///   # Logging element is optional
+    ///   ,"Logging": {<logger connfig}
+    ///  }
     ///
     ///  where:
     ///     module-name : is a label which uniquely identifies the
@@ -177,9 +181,17 @@ public:
     ///                    the application's configuration values
     /// @endcode
     ///
-    /// The method extracts the set of configuration elements for the
-    /// module-name which matches the controller's app_name_ and passes that
-    /// set into @c updateConfig().
+    /// To translate the JSON content into Elements, @c parseFile() is called
+    /// first.  This virtual method provides derivations a means to parse the
+    /// file content using an alternate parser.  If it returns an empty pointer
+    /// than the JSON parsing providing by Element::fromJSONFile() is called.
+    ///
+    /// Once parsed, the method looks for the Element "Logging" and, if present
+    /// uses it to configure loging.
+    ///
+    /// It then extracts the set of configuration elements for the
+    /// module-name that matches the controller's app_name_ and passes that
+    /// set into @c udpateConfig().
     ///
     /// The file may contain an arbitrary number of other modules.
     ///
@@ -383,6 +395,47 @@ protected:
     /// @throw VersionMessage if the -v or -V arguments is given.
     void parseArgs(int argc, char* argv[]);
 
+
+    ///@brief Parse a given file into Elements
+    ///
+    /// This method provides a means for deriving classes to use alternate
+    /// parsing mechanisms to parse configuration files into the corresponding
+    /// isc::data::Elements. The elements produced must be equivalent to those
+    /// which would be produced by the original JSON parsing.  Implementations
+    /// should throw when encountering errors.
+    ///
+    /// The default implementation returns an empty pointer, signifying to
+    /// callers that they should submit the file to the original parser.
+    ///
+    /// @param file_name pathname of the file to parse
+    ///
+    /// @return pointer to the elements created
+    ///
+    virtual isc::data::ConstElementPtr parseFile(const std::string& file_name) {
+        isc::data::ConstElementPtr elements;
+        return (elements);
+    }
+
+    ///@brief Parse text into Elements
+    ///
+    /// This method provides a means for deriving classes to use alternate
+    /// parsing mechanisms to parse configuration text into the corresponding
+    /// isc::data::Elements. The elements produced must be equivalent to those
+    /// which would be produced by the original JSON parsing.  Implementations
+    /// should throw when encountering errors.
+    ///
+    /// The default implementation returns an empty pointer, signifying to
+    /// callers that they should submit the text to the original parser.
+    ///
+    /// @param input text to parse
+    ///
+    /// @return pointer to the elements created
+    ///
+    virtual isc::data::ConstElementPtr parseText(const std::string& input) {
+        isc::data::ConstElementPtr elements;
+        return (elements);
+    }
+
     /// @brief Instantiates the application process and then initializes it.
     /// This is the second step taken during launch, following successful
     /// command line parsing. It is used to invoke the derivation-specific
diff --git a/src/lib/process/tests/d_cfg_mgr_unittests.cc b/src/lib/process/tests/d_cfg_mgr_unittests.cc
index fe37fe5e6c..d2644e1180 100644
--- a/src/lib/process/tests/d_cfg_mgr_unittests.cc
+++ b/src/lib/process/tests/d_cfg_mgr_unittests.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-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
@@ -22,6 +22,7 @@ using namespace std;
 using namespace isc;
 using namespace isc::config;
 using namespace isc::process;
+using namespace isc::data;
 using namespace boost::posix_time;
 
 namespace {
@@ -86,6 +87,84 @@ public:
     DStubCfgMgrPtr cfg_mgr_;
 };
 
+/// @brief Cfg manager which implements the parseElement() method
+/// This allows testing managers which use the new and/or old parsing
+/// mechanisms to parse configurations.  Eventually the latter will
+/// likely be removed.
+class ParseElementMgr : public DStubCfgMgr {
+public:
+
+    /// @brief Constructor
+    ParseElementMgr(){
+    }
+
+    /// @brief Destructor
+    ~ParseElementMgr() {
+    }
+
+    /// @brief Parse the given element if appropriate
+    ///
+    /// Overrides the DCfgMgrBase implementation.
+    /// Looks for the given element by name in the list of "parsable"
+    /// elements.  If it is found, it is added to the map of "parsed"
+    /// elements.
+    ///
+    /// @param element_id name of the element as it is expected in the cfg
+    /// @param element value of the element as ElementPtr
+    /// @return true if the element was parsed, false otherwise
+    virtual bool parseElement(const std::string& element_id,
+                              isc::data::ConstElementPtr element) {
+        std::string id;
+        BOOST_FOREACH(id, parsable_elements_) {
+            if (element_id == id) {
+                parsed_elements_.set(element_id, element);
+                return (true);
+            }
+        }
+
+        return (false);
+    }
+
+
+    /// @brief List of element ids which should be parsed by parseElement
+    ElementIdList parsable_elements_;
+
+    /// @brief Map of elements parsed by parseElement
+    MapElement parsed_elements_;
+};
+
+typedef boost::shared_ptr<ParseElementMgr> ParseElementMgrPtr;
+
+/// @brief Test fixture for testing a ParseElementMgr
+class ParseElementMgrTest : public ConfigParseTest {
+public:
+
+    /// @brief Constructor
+    ParseElementMgrTest():cfg_mgr_(new ParseElementMgr()) {
+    }
+
+    /// @brief Destructor
+    ~ParseElementMgrTest() {
+    }
+
+    /// @brief Convenience method which returns a DStubContextPtr to the
+    /// configuration context.
+    ///
+    /// @return returns a DStubContextPtr.
+    DStubContextPtr getStubContext() {
+        return (boost::dynamic_pointer_cast<DStubContext>
+                (cfg_mgr_->getContext()));
+    }
+
+    /// @brief Adds an element id to the list of "parsable" elements
+    void addToParsableElements(const std::string& element_id) {
+        cfg_mgr_->parsable_elements_.push_back(element_id);
+    }
+
+    /// @brief Configuration manager instance.
+    ParseElementMgrPtr cfg_mgr_;
+};
+
 ///@brief Tests basic construction/destruction of configuration manager.
 /// Verifies that:
 /// 1. Proper construction succeeds.
@@ -477,5 +556,69 @@ TEST_F(DStubCfgMgrTest, paramPosition) {
     EXPECT_EQ(pos.file_, isc::data::Element::ZERO_POSITION().file_);
 }
 
+// Tests that elements not handled by the parseElement() method are
+// handled by the old parsing mechanisms
+TEST_F(ParseElementMgrTest, basic) {
+    // Create the test config
+    string config = "{ \"bool_test\": true , \n"
+                    "  \"uint32_test\": 77 , \n"
+                    "  \"parse_one\": 1, \n"
+                    "  \"parse_two\": 2, \n"
+                    "  \"parse_three\": \"3\", \n"
+                    "  \"string_test\": \"hmmm chewy\" }";
+    ASSERT_TRUE(fromJSON(config));
+
+    // Add two elements to the list of elements handled by parseElement
+    addToParsableElements("parse_one");
+    addToParsableElements("parse_three");
+
+    // Verify that the configuration parses without error.
+    answer_ = cfg_mgr_->parseConfig(config_set_);
+    ASSERT_TRUE(checkAnswer(0));
+    DStubContextPtr context = getStubContext();
+    ASSERT_TRUE(context);
+
+    // Verify that the list of parsed elements is as expected
+    // It should have two entries: "parse_one" and "parse_three"
+    ASSERT_EQ(cfg_mgr_->parsed_elements_.size(), 2);
+    EXPECT_TRUE(cfg_mgr_->parsed_elements_.contains("parse_one"));
+    ConstElementPtr element = cfg_mgr_->parsed_elements_.get("parse_one");
+    EXPECT_EQ(element->intValue(), 1);
+
+    // "parse_two" should not be in the parsed list
+    EXPECT_FALSE(cfg_mgr_->parsed_elements_.contains("parse_two"));
+
+    // "parse_three" should be there
+    EXPECT_TRUE(cfg_mgr_->parsed_elements_.contains("parse_three"));
+    element = cfg_mgr_->parsed_elements_.get("parse_three");
+    EXPECT_EQ(element->stringValue(), "3");
+
+    // Now verify the original mechanism elements were parsed correctly
+    // Verify that the boolean parameter was parsed correctly by retrieving
+    // its value from the context.
+    bool actual_bool = false;
+    isc::data::Element::Position pos;
+    EXPECT_NO_THROW(pos = context->getParam("bool_test", actual_bool));
+    EXPECT_EQ(true, actual_bool);
+    EXPECT_EQ(1, pos.line_);
+
+    // Verify that the uint32 parameter was parsed correctly by retrieving
+    // its value from the context.
+    uint32_t actual_uint32 = 0;
+    EXPECT_NO_THROW(pos = context->getParam("uint32_test", actual_uint32));
+    EXPECT_EQ(77, actual_uint32);
+    EXPECT_EQ(2, pos.line_);
+
+    // Verify that the string parameter was parsed correctly by retrieving
+    // its value from the context.
+    std::string actual_string = "";
+    EXPECT_NO_THROW(pos = context->getParam("string_test", actual_string));
+    EXPECT_EQ("hmmm chewy", actual_string);
+    EXPECT_EQ(6, pos.line_);
+
+    // Verify that "parse_two" wasn't parsed by old parsing either
+    EXPECT_THROW(context->getParam("parse_two", actual_string, false),
+                                   isc::dhcp::DhcpConfigError);
+}
 
 } // end of anonymous namespace
diff --git a/src/lib/process/tests/d_controller_unittests.cc b/src/lib/process/tests/d_controller_unittests.cc
index 1b4d03d1f4..92ffb0d478 100644
--- a/src/lib/process/tests/d_controller_unittests.cc
+++ b/src/lib/process/tests/d_controller_unittests.cc
@@ -396,6 +396,35 @@ TEST_F(DStubControllerTest, invalidConfigReload) {
     EXPECT_EQ(SIGHUP, signals[0]);
 }
 
+// Tests that the original configuration is retained after a SIGHUP triggered
+// reconfiguration fails due to invalid config content.
+TEST_F(DStubControllerTest, alternateParsing) {
+    controller_->useAlternateParser(true);
+
+    // Setup to raise SIGHUP in 200 ms.
+    TimedSignal sighup(*getIOService(), SIGHUP, 200);
+
+    // Write the config and then run launch() for 500 ms
+    // After startup, which will load the initial configuration this enters
+    // the process's runIO() loop. We will first rewrite the config file.
+    // Next we process the SIGHUP signal which should cause us to reconfigure.
+    time_duration elapsed_time;
+    runWithConfig("{ \"string_test\": \"first value\" }", 500, elapsed_time);
+
+    // Context is still available post launch. Check to see that our
+    // configuration value is still the original value.
+    std::string  actual_value = "";
+    ASSERT_NO_THROW(getContext()->getParam("string_test", actual_value));
+    EXPECT_EQ("alt value", actual_value);
+
+    // Verify that we saw the signal.
+    std::vector<int>& signals = controller_->getProcessedSignals();
+    ASSERT_EQ(1, signals.size());
+    EXPECT_EQ(SIGHUP, signals[0]);
+}
+
+
+
 // Tests that the original configuration is replaced after a SIGHUP triggered
 // reconfiguration succeeds.
 TEST_F(DStubControllerTest, validConfigReload) {
diff --git a/src/lib/process/testutils/d_test_stubs.cc b/src/lib/process/testutils/d_test_stubs.cc
index f643563e9b..0874540f93 100644
--- a/src/lib/process/testutils/d_test_stubs.cc
+++ b/src/lib/process/testutils/d_test_stubs.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-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
@@ -149,7 +149,7 @@ DStubController::instance() {
 
 DStubController::DStubController()
     : DControllerBase(stub_app_name_, stub_bin_name_),
-      processed_signals_(), record_signal_only_(false) {
+      processed_signals_(), record_signal_only_(false), use_alternate_parser_(false) {
 
     if (getenv("KEA_FROM_BUILD")) {
         setSpecFileName(std::string(getenv("KEA_FROM_BUILD")) +
@@ -218,6 +218,22 @@ DStubController::processSignal(int signum){
     DControllerBase::processSignal(signum);
 }
 
+isc::data::ConstElementPtr
+DStubController::parseFile(const std::string& file_name) {
+    isc::data::ConstElementPtr elements;
+    if (use_alternate_parser_) {
+        std::ostringstream os;
+
+        os << "{ \"" << getController()->getAppName()
+            << "\": " << std::endl;
+        os <<  "{ \"string_test\": \"alt value\" } ";
+        os << " } " << std::endl;
+        elements = isc::data::Element::fromJSON(os.str());
+    }
+
+    return (elements);
+}
+
 DStubController::~DStubController() {
 }
 
diff --git a/src/lib/process/testutils/d_test_stubs.h b/src/lib/process/testutils/d_test_stubs.h
index 83953d4103..b82169dd03 100644
--- a/src/lib/process/testutils/d_test_stubs.h
+++ b/src/lib/process/testutils/d_test_stubs.h
@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-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
@@ -235,6 +235,19 @@ public:
        record_signal_only_ = value;
     }
 
+    /// @brief Determines if parseFile() implementation is used
+    ///
+    /// If true, parseFile() will return a Map of elements with fixed content,
+    /// mimicking a controller which is using alternate JSON parsing.
+    /// If false, parseFile() will return an empty pointer mimicking a
+    /// controller which is using original JSON parsing supplied by the
+    /// Element class.
+    ///
+    /// @param value boolean which if true enables record-only behavior
+    void useAlternateParser(bool value) {
+       use_alternate_parser_ = value;
+    }
+
 protected:
     /// @brief Handles additional command line options that are supported
     /// by DStubController.  This implementation supports an option "-x".
@@ -291,6 +304,25 @@ protected:
     /// @param signum OS signal value received
     virtual void processSignal(int signum);
 
+    /// @brief Provides alternate parse file implementation
+    ///
+    /// Overrides the base class implementation to mimick controllers which
+    /// implement alternate file parsing.  If enabled via useAlternateParser()
+    /// the method will return a fixed map of elements reflecting the following
+    /// JSON:
+    ///
+    /// @code
+    ///     { "<name>getController()->getAppName()" :
+    ///          { "string_test": "alt value" };
+    ///     }
+    ///
+    /// @endcode
+    ///
+    ///  where <name> is getController()->getAppName()
+    ///
+    /// otherwise it return an empty pointer.
+    virtual isc::data::ConstElementPtr parseFile(const std::string&);
+
 private:
     /// @brief Constructor is private to protect singleton integrity.
     DStubController();
@@ -301,6 +333,9 @@ private:
     /// @brief Boolean for controlling if signals are merely recorded.
     bool record_signal_only_;
 
+    /// @brief Boolean for controlling if parseFile is "implemented"
+    bool use_alternate_parser_;
+
 public:
     virtual ~DStubController();
 };