Commit 95d65737 authored by Thomas Markwalder's avatar Thomas Markwalder
Browse files

[2957] Review comments. Added in the ability to configure TSIG Key

information.
parent ae6f706d
......@@ -30,7 +30,8 @@ namespace d2 {
D2CfgContext::D2CfgContext()
: forward_mgr_(new DdnsDomainListMgr("forward_mgr")),
reverse_mgr_(new DdnsDomainListMgr("reverse_mgr")) {
reverse_mgr_(new DdnsDomainListMgr("reverse_mgr")),
keys_(new TSIGKeyInfoMap()) {
}
D2CfgContext::D2CfgContext(const D2CfgContext& rhs) : DCfgContextBase(rhs) {
......@@ -43,6 +44,8 @@ D2CfgContext::D2CfgContext(const D2CfgContext& rhs) : DCfgContextBase(rhs) {
reverse_mgr_.reset(new DdnsDomainListMgr(rhs.reverse_mgr_->getName()));
reverse_mgr_->setDomains(rhs.reverse_mgr_->getDomains());
}
keys_ = rhs.keys_;
}
D2CfgContext::~D2CfgContext() {
......@@ -51,6 +54,14 @@ D2CfgContext::~D2CfgContext() {
// *********************** D2CfgMgr *************************
D2CfgMgr::D2CfgMgr() : DCfgMgrBase(DCfgContextBasePtr(new D2CfgContext())) {
// TSIG keys need to parse before the Domains, so we can catch Domains
// that specify undefined keys. Create the necessary parsing order now.
addToParseOrder("interface");
addToParseOrder("ip_address");
addToParseOrder("port");
addToParseOrder("tsig_keys");
addToParseOrder("forward_ddns");
addToParseOrder("reverse_ddns");
}
D2CfgMgr::~D2CfgMgr() {
......@@ -58,13 +69,13 @@ D2CfgMgr::~D2CfgMgr() {
bool
D2CfgMgr::matchForward(const std::string& fqdn, DdnsDomainPtr& domain) {
if (fqdn == "") {
if (fqdn.empty()) {
// This is a programmatic error and should not happen.
isc_throw (D2CfgError, "matchForward passed an empty fqdn");
isc_throw(D2CfgError, "matchForward passed an empty fqdn");
}
// Fetch the forward manager from the D2 context.
DdnsDomainListMgrPtr& mgr = getD2CfgContext()->getForwardMgr();
DdnsDomainListMgrPtr mgr = getD2CfgContext()->getForwardMgr();
// Call the manager's match method and return the result.
return (mgr->matchDomain(fqdn, domain));
......@@ -72,13 +83,13 @@ D2CfgMgr::matchForward(const std::string& fqdn, DdnsDomainPtr& domain) {
bool
D2CfgMgr::matchReverse(const std::string& fqdn, DdnsDomainPtr& domain) {
if (fqdn == "") {
if (fqdn.empty()) {
// This is a programmatic error and should not happen.
isc_throw (D2CfgError, "matchReverse passed a null or empty fqdn");
isc_throw(D2CfgError, "matchReverse passed a null or empty fqdn");
}
// Fetch the reverse manager from the D2 context.
DdnsDomainListMgrPtr& mgr = getD2CfgContext()->getReverseMgr();
DdnsDomainListMgrPtr mgr = getD2CfgContext()->getReverseMgr();
// Call the manager's match method and return the result.
return (mgr->matchDomain(fqdn, domain));
......@@ -99,10 +110,14 @@ D2CfgMgr::createConfigParser(const std::string& config_id) {
parser = new Uint32Parser(config_id, context->getUint32Storage());
} else if (config_id == "forward_ddns") {
parser = new DdnsDomainListMgrParser("forward_mgr",
context->getForwardMgr());
context->getForwardMgr(),
context->getKeys());
} else if (config_id == "reverse_ddns") {
parser = new DdnsDomainListMgrParser("reverse_mgr",
context->getReverseMgr());
context->getReverseMgr(),
context->getKeys());
} else if (config_id == "tsig_keys") {
parser = new TSIGKeyInfoListParser("tsig_key_list", context->getKeys());
} else {
isc_throw(NotImplemented,
"parser error: D2CfgMgr parameter not supported: "
......
......@@ -28,6 +28,7 @@ namespace isc {
namespace d2 {
/// @brief DHCP-DDNS Configuration Context
///
/// Implements the storage container for configuration context.
/// It provides a single enclosure for the storage of configuration parameters
/// and any other DHCP-DDNS specific information that needs to be accessible
......@@ -45,23 +46,30 @@ public:
///
/// @return returns a raw pointer to the new clone.
virtual D2CfgContext* clone() {
return (new D2CfgContext(*this));
return (new D2CfgContext(*this));
}
/// @brief Fetches the forward DNS domain list manager.
///
/// @return returns a pointer reference to the forward manager.
DdnsDomainListMgrPtr& getForwardMgr() {
/// @return returns a pointer to the forward manager.
DdnsDomainListMgrPtr getForwardMgr() {
return (forward_mgr_);
}
/// @brief Fetches the reverse DNS domain list manager.
///
/// @return returns a pointer reference to the reverse manager.
DdnsDomainListMgrPtr& getReverseMgr() {
/// @return returns a pointer to the reverse manager.
DdnsDomainListMgrPtr getReverseMgr() {
return (reverse_mgr_);
}
/// @brief Fetches the map of TSIG keys.
///
/// @return returns a pointer to the key map.
TSIGKeyInfoMapPtr getKeys() {
return (keys_);
}
protected:
/// @brief Copy constructor for use by derivations in clone().
D2CfgContext(const D2CfgContext& rhs);
......@@ -75,6 +83,9 @@ private:
/// @brief Reverse domain list manager.
DdnsDomainListMgrPtr reverse_mgr_;
/// @brief Storage for the map of TSIGKeyInfos
TSIGKeyInfoMapPtr keys_;
};
/// @brief Defines a pointer for DdnsDomain instances.
......
This diff is collapsed.
This diff is collapsed.
......@@ -68,10 +68,14 @@ 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
A debug message which indicates that configuration being parsed includes
element ids not specified the configuration manager's parse order list. This is
programming logic error.
% 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 indicating that the listed element is in specified in the
parsing order but is not in the configuration. The configuration is incorrect.
% DCTL_PARSER_FAIL configuration parsing failed for configuration element: %1
On receipt of message containing details to a change of its configuration,
......@@ -121,8 +125,8 @@ unrecoverable error from within the event loop.
% DHCP_DDNS_NO_MATCH No DNS servers match FQDN: %1
This is warning message issued when there are no domains in the configuration
which match the cited FQDN. The DNS Update request for the FQDN cannot be
processed.
which match the cited fully qualified domain name (FQDN). The DNS Update
request for the FQDN cannot be processed.
% DHCP_DDNS_PROCESS_INIT application init invoked
This is a debug message issued when the Dhcp-Ddns application enters
......
......@@ -40,8 +40,6 @@ namespace d2 {
// *********************** DCfgContextBase *************************
const bool DCfgContextBase::optional_ = true;
DCfgContextBase::DCfgContextBase():
boolean_values_(new BooleanStorage()),
uint32_values_(new Uint32Storage()),
......@@ -143,30 +141,42 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) {
// elements are parsed in the order the value_map presents them.
if (parse_order_.size() > 0) {
// 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
// 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 make hold the engineer accountable.
if (values_map.size() > parse_order_.size()) {
LOG_ERROR(dctl_logger, DCTL_ORDER_ERROR);
return (isc::config::createAnswer(1,
"Configuration contains elements not in parse order"));
}
// For each element_id in the parse order list, look for it in the
// value map. If the element exists in the map, pass it and it's
// associated data in for parsing. If there is no matching entry
// in the value map, then assume the element is optional and move
// on to next element_id.
// associated data in for parsing.
// If there is no matching entry in the value map an error is
// thrown. Optional elements may not be used with ordered parsing.
int parsed_count = 0;
std::map<std::string, ConstElementPtr>::const_iterator it;
BOOST_FOREACH(element_id, parse_order_) {
it = values_map.find(element_id);
if (it != values_map.end()) {
++parsed_count;
buildAndCommit(element_id, it->second);
}
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");
}
}
// 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
// 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) < values_map.size()))) {
LOG_ERROR(dctl_logger, DCTL_ORDER_ERROR);
isc_throw(DCfgMgrBaseError,
"Configuration contains elements not in parse order");
}
} else {
// Order doesn't matter so iterate over the value map directly.
......@@ -185,8 +195,7 @@ DCfgMgrBase::parseConfig(isc::data::ConstElementPtr config_set) {
} 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() +
" for element: " + element_id);
string("Configuration parsing failed: ") + ex.what());
// An error occurred, so make sure that we restore original context.
context_ = original_context;
......@@ -202,7 +211,7 @@ void DCfgMgrBase::buildAndCommit(std::string& element_id,
// based on the element id.
ParserPtr parser = createConfigParser(element_id);
if (!parser) {
isc_throw(DCfgMgrBaseError, std::string("Could not create parser"));
isc_throw(DCfgMgrBaseError, "Could not create parser");
}
try {
......@@ -217,8 +226,8 @@ void DCfgMgrBase::buildAndCommit(std::string& element_id,
// nothing something we are concerned with here.)
parser->commit();
} catch (const isc::Exception& ex) {
isc_throw(DCfgMgrBaseError, std::string("Could not build and commit")
+ ex.what());
isc_throw(DCfgMgrBaseError,
"Could not build and commit: " << ex.what());
} catch (...) {
isc_throw(DCfgMgrBaseError, "Non-ISC exception occurred");
}
......
......@@ -40,10 +40,9 @@ public:
/// Derivations simply add additional storage as needed. Note that this class
/// declares the pure virtual clone() method, its copy constructor is protected,
/// and its copy operator is inaccessible. Derivations must supply an
/// implementation of clone that calls the base class copy constructor as
/// well as performing whatever steps are necessary to copy its additional
/// storage. This allows the management class to perform context backup and
/// restoration without derivation specific knowledge using logic like
/// implementation of clone that calls the base class copy constructor.
/// This allows the management class to perform context backup and restoration
/// without derivation specific knowledge using logic like
/// the following:
///
/// // Make a backup copy
......@@ -55,7 +54,8 @@ public:
class DCfgContextBase {
public:
/// @brief Indicator that a configuration parameter is optional.
static const bool optional_;
static const bool OPTIONAL = true;
static const bool REQUIRED = false;
/// @brief Constructor
DCfgContextBase();
......@@ -129,13 +129,14 @@ public:
return (string_values_);
}
/// @brief Creates a clone of this context object. As mentioned in the
/// the class brief, derivation must supply an implementation that
/// initializes the base class storage as well as its own. Typically
/// the derivation's clone method would return the result of passing
/// "*this" into its own copy constructor:
/// @brief Creates a clone of this context object.
///
/// -----------------------------------------------------------------
/// As mentioned in the the class brief, derivation must supply an
/// implementation that initializes the base class storage as well as its
/// own. Typically the derivation's clone method would return the result
/// of passing "*this" into its own copy constructor:
///
/// @code
/// class DStubContext : public DCfgContextBase {
/// public:
/// :
......@@ -153,7 +154,7 @@ public:
/// // Here's the derivation's additional storage.
/// isc::dhcp::Uint32StoragePtr extra_values_;
/// :
/// -----------------------------------------------------------------
/// @endcode
///
/// @return returns a raw pointer to the new clone.
virtual DCfgContextBase* clone() = 0;
......@@ -189,17 +190,12 @@ typedef std::vector<std::string> ElementIdList;
/// configuration values, storing the parsed information in its converted form,
/// and retrieving the information on demand. It is intended to be the worker
/// class which is handed a set of configuration values to process by upper
/// application management layers. Typically this call chain would look like
/// this:
/// External configuration event:
/// --> Controller::configHandler(new configuration)
/// --> Controller.updateConfig(new configuration)
/// --> Controller.Process.configure(new configuration)
/// --> Process.CfgMgr.parseConfig(new configuration)
/// application management layers.
///
/// The class presents a public method for receiving new configurations,
/// parseConfig. This method coordinates the parsing effort as follows:
///
/// @code
/// make backup copy of configuration context
/// for each top level element in new configuration
/// get derivation-specific parser for element
......@@ -209,6 +205,7 @@ typedef std::vector<std::string> ElementIdList;
///
/// if an error occurred
/// restore configuration context from backup
/// @endcode
///
/// After making a backup of the current context, it iterates over the top-level
/// elements in the new configuration. The order in which the elements are
......@@ -219,16 +216,19 @@ typedef std::vector<std::string> ElementIdList;
///
/// This allows a derivation to specify the order in which its elements are
/// parsed if there are dependencies between elements.
///
/// To parse a given element, its id is passed into createConfigParser,
/// which returns an instance of the appropriate parser. This method is
/// abstract so the derivation's implementation determines the type of parser
/// created. This isolates the knowledge of specific element ids and which
/// application specific parsers to derivation.
///
/// Once the parser has been created, it is used to parse the data value
/// associated with the element id and update the context with the parsed
/// results.
/// In the event that an error occurs, parsing is halted and the configuration
/// context is restored from backup.
///
/// In the event that an error occurs, parsing is halted and the
/// configuration context is restored from backup.
class DCfgMgrBase {
public:
/// @brief Constructor
......@@ -254,12 +254,14 @@ public:
config_set);
/// @brief Adds a given element id to the end of the parse order list.
/// The order in which elements are retrieved from this is FIFO. Elements
/// should be added in the order in which they are to be parsed.
//
///
/// The order in which elements are retrieved from this is the order in
/// which they are added to the list. Derivations should use this method
/// to populate the parse order as part of their constructor.
///
/// @param element_id is the string name of the element as it will appear
/// in the configuration set.
void addToParseOrder(std::string& element_id){
void addToParseOrder(const std::string& element_id){
parse_order_.push_back(element_id);
}
......@@ -278,9 +280,11 @@ public:
}
protected:
/// @brief Given an element_id returns an instance of the appropriate
/// parser. This method is abstract, isolating any direct knowledge of
/// element_ids and parsers to within the application-specific derivation.
/// @brief Create a parser instance based on an element id.
///
/// Given an element_id returns an instance of the appropriate parser.
/// This method is abstract, isolating any direct knowledge of element_ids
/// and parsers to within the application-specific derivation.
///
/// @param element_id is the string name of the element as it will appear
/// in the configuration set.
......@@ -292,7 +296,9 @@ protected:
private:
/// @brief Given an element_id and data value, instantiate the appropriate
/// @brief Parse a configuration element.
///
/// Given an element_id and data value, instantiate 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
......@@ -304,9 +310,10 @@ private:
void buildAndCommit(std::string& element_id,
isc::data::ConstElementPtr value);
/// @brief An FIFO list of element ids, used to dictate the element
/// parsing order. If the list is empty, the natural order in the
/// configuration set it used.
/// @brief A list of element ids which specifies the element parsing order.
///
/// If the list is empty, the natural order in the configuration set
/// it used.
ElementIdList parse_order_;
/// @brief Pointer to the configuration context instance.
......
......@@ -24,9 +24,40 @@
"item_optional": true,
"item_default": 51771
},
{
"item_name": "foward_ddns",
"item_name": "tsig_keys",
"item_type": "list",
"item_optional": true,
"item_default": [],
"list_item_spec":
{
"item_name": "tsig_key",
"item_type": "map",
"item_optional": false,
"item_default": {"algorithm" : "hmac_md5"},
"map_item_spec": [
{
"item_name": "name",
"item_type": "string",
"item_optional": false,
"item_default": ""
},
{
"item_name": "algorithm",
"item_type": "string",
"item_optional": false,
"item_default": ""
},
{
"item_name": "secret",
"item_type": "string",
"item_optional": false,
"item_default": ""
}]
}
},
{
"item_name": "forward_ddns",
"item_type": "map",
"item_optional": false,
"item_default": {},
......@@ -171,3 +202,4 @@
]
}
}
This diff is collapsed.
......@@ -88,17 +88,17 @@ public:
/// 3. Destruction works properly.
/// 4. Construction with a null context is not allowed.
TEST(DCfgMgrBase, construction) {
DCfgMgrBase *cfg_mgr = NULL;
DCfgMgrBasePtr cfg_mgr;
// Verify that configuration manager constructions without error.
ASSERT_NO_THROW(cfg_mgr=new DStubCfgMgr());
ASSERT_NO_THROW(cfg_mgr.reset(new DStubCfgMgr()));
// Verify that the context can be retrieved and is not null.
DCfgContextBasePtr context = cfg_mgr->getContext();
EXPECT_TRUE(context);
// Verify that the manager can be destructed without error.
EXPECT_NO_THROW(delete cfg_mgr);
EXPECT_NO_THROW(cfg_mgr.reset());
// Verify that an attempt to construct a manger with a null context fails.
ASSERT_THROW(DCtorTestCfgMgr(), DCfgMgrBaseError);
......@@ -145,6 +145,7 @@ TEST_F(DStubCfgMgrTest, basicParseTest) {
/// 2. A parse order list with too few elements is detected.
/// 3. Ordered parsing parses the elements in the order specified by the
/// configuration manager's parse order list.
/// 4. A parse order list with too many elements is detected.
TEST_F(DStubCfgMgrTest, parseOrderTest) {
// Element ids used for test.
std::string charlie("charlie");
......@@ -213,6 +214,17 @@ TEST_F(DStubCfgMgrTest, parseOrderTest) {
// Verify that the parsed order is the order we configured.
EXPECT_TRUE(cfg_mgr_->getParseOrder() == cfg_mgr_->parsed_order_);
// Create a parse order list that has too many entries. Verify that
// when parsing the test config, it fails.
cfg_mgr_->addToParseOrder("delta");
// Verify the parse order list is the size we expect.
EXPECT_EQ(4, cfg_mgr_->getParseOrder().size());
// Verify the configuration fails.
answer_ = cfg_mgr_->parseConfig(config_set_);
EXPECT_TRUE(checkAnswer(1));
}
/// @brief Tests that element ids supported by the base class as well as those
......
......@@ -25,13 +25,18 @@ const char* valid_d2_config = "{ "
"\"interface\" : \"eth1\" , "
"\"ip_address\" : \"192.168.1.33\" , "
"\"port\" : 88 , "
"\"tsig_keys\": ["
"{ \"name\": \"d2_key.tmark.org\" , "
" \"algorithm\": \"md5\" ,"
" \"secret\": \"0123456989\" "
"} ],"
"\"forward_ddns\" : {"
"\"ddns_domains\": [ "
"{ \"name\": \"tmark.org\" , "
" \"key_name\": \"d2_key.tmark.org\" , "
" \"dns_servers\" : [ "
" { \"hostname\": \"one.tmark\" } "
" ] } ] }, "
"] } ] }, "
"\"reverse_ddns\" : {"
"\"ddns_domains\": [ "
"{ \"name\": \" 0.168.192.in.addr.arpa.\" , "
......@@ -296,7 +301,7 @@ DStubCfgMgr::createConfigParser(const std::string& element_id) {
// to test parse ordering, by permitting a wide range of element ids
// to "succeed" without specifically supporting them.
if (SimFailure::shouldFailOn(SimFailure::ftElementUnknown)) {
isc_throw(DCfgMgrBaseError, "Configuration parameter not supported"
isc_throw(DCfgMgrBaseError, "Configuration parameter not supported: "
<< element_id);
}
......
......@@ -628,10 +628,12 @@ public:
isc::data::ConstElementPtr comment;
comment = isc::config::parseAnswer(rcode, answer_);
// Handy for diagnostics
// if (rcode != 0) {
// std::cout << "checkAnswer rcode:" << rcode << " comment: "
// << *comment << std::endl;
//}
#if 1
if (rcode != 0) {
std::cout << "checkAnswer rcode:" << rcode << " comment: "
<< *comment << std::endl;
}
#endif
return (rcode == should_be);
}
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment