[3121] Moved validation logic to build method for TSIGKeyInfo parsing

Modified TSIGKeyInfoParser, TSIGKeyInfoListParser, and unit tests

src/bin/d2/d2_config.cc

@@ -201,30 +201,7 @@ TSIGKeyInfoParser::build(isc::data::ConstElementPtr key_config) {
         parser->build(config_pair.second);
         parser->commit();
     }
-}
-
-isc::dhcp::ParserPtr
-TSIGKeyInfoParser::createConfigParser(const std::string& config_id) {
-    DhcpConfigParser* parser = NULL;
-    // Based on the configuration id of the element, create the appropriate
-    // parser. Scalars are set to use the parser's local scalar storage.
-    if ((config_id == "name")  ||
-        (config_id == "algorithm") ||
-        (config_id == "secret")) {
-        parser = new isc::dhcp::StringParser(config_id,
-                                             local_scalars_.getStringStorage());
-    } else {
-        isc_throw(NotImplemented,
-                  "parser error: TSIGKeyInfo parameter not supported: "
-                  << config_id);
-    }
-
-    // Return the new parser instance.
-    return (isc::dhcp::ParserPtr(parser));
-}
 
-void
-TSIGKeyInfoParser::commit() {
     std::string name;
     std::string algorithm;
     std::string secret;
@@ -266,6 +243,32 @@ TSIGKeyInfoParser::commit() {
     (*keys_)[name]=key_info;
 }
 
+isc::dhcp::ParserPtr
+TSIGKeyInfoParser::createConfigParser(const std::string& config_id) {
+    DhcpConfigParser* parser = NULL;
+    // Based on the configuration id of the element, create the appropriate
+    // parser. Scalars are set to use the parser's local scalar storage.
+    if ((config_id == "name")  ||
+        (config_id == "algorithm") ||
+        (config_id == "secret")) {
+        parser = new isc::dhcp::StringParser(config_id,
+                                             local_scalars_.getStringStorage());
+    } else {
+        isc_throw(NotImplemented,
+                  "parser error: TSIGKeyInfo parameter not supported: "
+                  << config_id);
+    }
+
+    // Return the new parser instance.
+    return (isc::dhcp::ParserPtr(parser));
+}
+
+void
+TSIGKeyInfoParser::commit() {
+    /// @todo if at some point  TSIG keys need some form of resource
+    /// initialization do that here
+}
+
 // *********************** TSIGKeyInfoListParser  *************************
 
 TSIGKeyInfoListParser::TSIGKeyInfoListParser(const std::string& list_name,

src/bin/d2/d2_config.h

@@ -540,8 +540,9 @@ public:
 
     /// @brief Performs the actual parsing of the given  "tsig_key" element.
     ///
-    /// The results of the parsing are retained internally for use during
-    /// commit.
+    /// Parses a configuration for the elements needed to instantiate a
+    /// TSIGKeyInfo, validates those entries, creates a TSIGKeyInfo instance
+    /// then attempts to add to a list of keys
     ///
     /// @param key_config is the "tsig_key" configuration to parse
     virtual void build(isc::data::ConstElementPtr key_config);
@@ -559,9 +560,8 @@ public:
     /// @return returns a pointer to newly created parser.
     virtual isc::dhcp::ParserPtr createConfigParser(const std::string&
                                                     config_id);
-
-    /// @brief Instantiates a DnsServerInfo from internal data values
-    /// saves it to the storage area pointed to by servers_.
+    /// @brief commits the TSIGKeyInfo configuration
+    /// At the moment this method is a NOP.
     virtual void commit();
 
 private:
@@ -629,7 +629,7 @@ private:
     /// the list of newly created TSIGKeyInfo instances. This is given to us
     /// as a constructor argument by an upper level.
     TSIGKeyInfoMapPtr keys_;
-    
+
     /// @brief Local storage area to which individual key parsers commit.
     TSIGKeyInfoMapPtr local_keys_;
 

src/bin/d2/tests/d2_cfg_mgr_unittests.cc

@@ -261,9 +261,8 @@ TEST_F(TSIGKeyInfoTest, invalidEntry) {
                          "}";
     ASSERT_TRUE(fromJSON(config));
 
-    // Verify that build succeeds but commit fails on blank name.
-    EXPECT_NO_THROW(parser_->build(config_set_));
-    EXPECT_THROW(parser_->commit(), D2CfgError);
+    // Verify that build fails on blank name.
+    EXPECT_THROW(parser_->build(config_set_), D2CfgError);
 
     // Config with a blank algorithm entry.
     config = "{"
@@ -274,9 +273,8 @@ TEST_F(TSIGKeyInfoTest, invalidEntry) {
 
     ASSERT_TRUE(fromJSON(config));
 
-    // Verify that build succeeds but commit fails on blank algorithm.
-    EXPECT_NO_THROW(parser_->build(config_set_));
-    EXPECT_THROW(parser_->commit(), D2CfgError);
+    // Verify that build fails on blank algorithm.
+    EXPECT_THROW(parser_->build(config_set_), D2CfgError);
 
     // Config with a blank secret entry.
     config = "{"
@@ -287,9 +285,8 @@ TEST_F(TSIGKeyInfoTest, invalidEntry) {
 
     ASSERT_TRUE(fromJSON(config));
 
-    // Verify that build succeeds but commit fails on blank secret.
-    EXPECT_NO_THROW(parser_->build(config_set_));
-    EXPECT_THROW(parser_->commit(), D2CfgError);
+    // Verify that build fails blank secret
+    EXPECT_THROW(parser_->build(config_set_), D2CfgError);
 }
 
 /// @brief Verifies that TSIGKeyInfo parsing creates a proper TSIGKeyInfo
@@ -347,10 +344,7 @@ TEST_F(TSIGKeyInfoTest, invalidTSIGKeyList) {
     ASSERT_NO_THROW(parser.reset(new TSIGKeyInfoListParser("test", keys_)));
 
     // Verify that the list builds without errors.
-    ASSERT_NO_THROW(parser->build(config_set_));
-
-    // Verify that the list commit fails.
-    EXPECT_THROW(parser->commit(), D2CfgError);
+    EXPECT_THROW(parser->build(config_set_), D2CfgError);
 }
 
 /// @brief Verifies that attempting to parse an invalid list of TSIGKeyInfo
@@ -380,10 +374,7 @@ TEST_F(TSIGKeyInfoTest, duplicateTSIGKey) {
     ASSERT_NO_THROW(parser.reset(new TSIGKeyInfoListParser("test", keys_)));
 
     // Verify that the list builds without errors.
-    ASSERT_NO_THROW(parser->build(config_set_));
-
-    // Verify that the list commit fails.
-    EXPECT_THROW(parser->commit(), D2CfgError);
+    EXPECT_THROW(parser->build(config_set_), D2CfgError);
 }
 
 /// @brief Verifies a valid list of TSIG Keys parses correctly.
@@ -1013,7 +1004,7 @@ TEST_F(D2CfgMgrTest, fullConfig) {
     EXPECT_TRUE(cfg_mgr_->reverseUpdatesEnabled());
 
     // Verify that parsing the exact same configuration a second time
-    // does not cause a duplicate value errors. 
+    // does not cause a duplicate value errors.
     answer_ = cfg_mgr_->parseConfig(config_set_);
     ASSERT_TRUE(checkAnswer(0));
 }