Commit 1517f32c authored by Tomek Mrugalski's avatar Tomek Mrugalski 🛰
Browse files

[2269] Changes in Dhcp6 config parser after review.

parent d5a5e815
......@@ -2911,7 +2911,7 @@ Dhcp6/subnet6 [] list (default)</screen>
<screen>
&gt; <userinput>config add Dhcp6/subnet6</userinput>
&gt; <userinput>config set Dhcp6/subnet6[0]/subnet "2001:db8:1::/64"</userinput>
&gt; <userinput>config set Dhcp6/subnet6[0]/pool [ "2001:db8:1::1 - 2001:db8:1::ffff" ]</userinput>
&gt; <userinput>config set Dhcp6/subnet6[0]/pool [ "2001:db8:1::0 - 2001:db8:1::ffff" ]</userinput>
&gt; <userinput>config commit</userinput></screen>
Note that subnet is defined as a simple string, but the pool parameter
is actually a list of pools: for this reason, the pool definition is
......@@ -2946,11 +2946,19 @@ Dhcp6/subnet6 [] list (default)</screen>
very wasteful, it is certainly a valid configuration to dedicate the
whole /48 subnet for that purpose.
</para>
<para>
When configuring a DHCPv6 server using prefix/length notation, please pay
attention to the boundary values. When specifying that the server should use
a given pool, it will be able to allocate also first (typically network
address) address from that pool. For example for pool 2001:db8::/64 the
2001:db8:: address may be assigned as well. If you want to avoid this,
please use min-max notation.
</para>
<para>
Note: Although configuration is now accepted, it is not internally used
by they server yet. At this stage of development, the only way to alter
server configuration is to tweak its source code. To do so, please edit
server configuration is to modify its source code. To do so, please edit
src/bin/dhcp6/dhcp6_srv.cc file, modify the following parameters and
recompile:
<screen>
......
......@@ -93,7 +93,7 @@ public:
/// that corresponds to this derived class and prepares a new value to
/// apply to the server.
/// In the above example, the derived class for the identifier "param1"
/// would be passed an data \c Element storing an integer whose value
/// would be passed a data \c Element storing an integer whose value
/// is 10, and would record that value internally;
/// the derived class for the identifier "param2" would be passed a
/// map element and (after parsing) convert it into some internal
......
......@@ -43,7 +43,7 @@ namespace dhcp {
typedef pair<string, ConstElementPtr> ConfigPair;
/// @brief a factory method that will create a parser for a given element name
typedef Dhcp6ConfigParser* ParserFactory(const std::string& config_id);
typedef DhcpConfigParser* ParserFactory(const std::string& config_id);
/// @brief a collection of factories that creates parsers for specified element names
typedef std::map<std::string, ParserFactory*> FactoryMap;
......@@ -72,21 +72,21 @@ StringStorage string_defaults;
/// will accept any configuration and will just print it out
/// on commit. Useful for debugging existing configurations and
/// adding new ones.
class DummyParser : public Dhcp6ConfigParser {
class DebugParser : public DhcpConfigParser {
public:
/// @brief Constructor
///
/// See \ref Dhcp6ConfigParser class for details.
/// See \ref DhcpConfigParser class for details.
///
/// @param param_name name of the parsed parameter
DummyParser(const std::string& param_name)
DebugParser(const std::string& param_name)
:param_name_(param_name) {
}
/// @brief builds parameter value
///
/// See \ref Dhcp6ConfigParser class for details.
/// See \ref DhcpConfigParser class for details.
///
/// @param new_config pointer to the new configuration
virtual void build(ConstElementPtr new_config) {
......@@ -100,20 +100,20 @@ public:
/// This is a method required by base class. It pretends to apply the
/// configuration, but in fact it only prints the parameter out.
///
/// See \ref Dhcp6ConfigParser class for details.
/// See \ref DhcpConfigParser class for details.
virtual void commit() {
// Debug message. The whole DummyParser class is used only for parser
// Debug message. The whole DebugParser class is used only for parser
// debugging, and is not used in production code. It is very convenient
// to keep it around. Please do not turn this cout into logger calls.
std::cout << "Commit for token: [" << param_name_ << "] = ["
<< value_->str() << "]" << std::endl;
}
/// @brief factory that constructs DummyParser objects
/// @brief factory that constructs DebugParser objects
///
/// @param param_name name of the parameter to be parsed
static Dhcp6ConfigParser* Factory(const std::string& param_name) {
return (new DummyParser(param_name));
static DhcpConfigParser* Factory(const std::string& param_name) {
return (new DebugParser(param_name));
}
protected:
......@@ -131,11 +131,11 @@ protected:
/// (uint32_defaults). If used in smaller scopes (e.g. to parse parameters
/// in subnet config), it can be pointed to a different storage, using
/// setStorage() method. This class follows the parser interface, laid out
/// in its base class, \ref Dhcp6ConfigParser.
/// in its base class, \ref DhcpConfigParser.
///
/// For overview of usability of this generic purpose parser, see
/// \ref dhcpv6-config-inherit page.
class Uint32Parser : public Dhcp6ConfigParser {
class Uint32Parser : public DhcpConfigParser {
public:
/// @brief constructor for Uint32Parser
......@@ -174,7 +174,7 @@ public:
/// @brief factory that constructs Uint32Parser objects
///
/// @param param_name name of the parameter to be parsed
static Dhcp6ConfigParser* Factory(const std::string& param_name) {
static DhcpConfigParser* Factory(const std::string& param_name) {
return (new Uint32Parser(param_name));
}
......@@ -189,7 +189,7 @@ public:
protected:
/// pointer to the storage, where parsed value will be stored
Uint32Storage * storage_;
Uint32Storage* storage_;
/// name of the parameter to be parsed
std::string param_name_;
......@@ -205,11 +205,11 @@ protected:
/// (string_defaults). If used in smaller scopes (e.g. to parse parameters
/// in subnet config), it can be pointed to a different storage, using
/// setStorage() method. This class follows the parser interface, laid out
/// in its base class, \ref Dhcp6ConfigParser.
/// in its base class, \ref DhcpConfigParser.
///
/// For overview of usability of this generic purpose parser, see
/// \ref dhcpv6-config-inherit page.
class StringParser : public Dhcp6ConfigParser {
class StringParser : public DhcpConfigParser {
public:
/// @brief constructor for StringParser
......@@ -244,7 +244,7 @@ public:
/// @brief factory that constructs StringParser objects
///
/// @param param_name name of the parameter to be parsed
static Dhcp6ConfigParser* Factory(const std::string& param_name) {
static DhcpConfigParser* Factory(const std::string& param_name) {
return (new StringParser(param_name));
}
......@@ -253,13 +253,13 @@ public:
/// See \ref dhcpv6-config-inherit for details.
///
/// @param storage pointer to the storage container
void setStorage(StringStorage * storage) {
void setStorage(StringStorage* storage) {
storage_ = storage;
}
protected:
/// pointer to the storage, where parsed value will be stored
StringStorage * storage_;
StringStorage* storage_;
/// name of the parameter to be parsed
std::string param_name_;
......@@ -277,7 +277,7 @@ protected:
/// designates all interfaces.
///
/// It is useful for parsing Dhcp6/interface parameter.
class InterfaceListConfigParser : public Dhcp6ConfigParser {
class InterfaceListConfigParser : public DhcpConfigParser {
public:
/// @brief constructor
......@@ -314,7 +314,7 @@ public:
/// @brief factory that constructs InterfaceListConfigParser objects
///
/// @param param_name name of the parameter to be parsed
static Dhcp6ConfigParser* Factory(const std::string& param_name) {
static DhcpConfigParser* Factory(const std::string& param_name) {
return (new InterfaceListConfigParser(param_name));
}
......@@ -333,7 +333,7 @@ protected:
/// before build(). Otherwise exception will be thrown.
///
/// It is useful for parsing Dhcp6/subnet6[X]/pool parameters.
class PoolParser : public Dhcp6ConfigParser {
class PoolParser : public DhcpConfigParser {
public:
/// @brief constructor.
......@@ -350,8 +350,8 @@ public:
void build(ConstElementPtr pools_list) {
// setStorage() should have been called before build
if (!pools_) {
isc_throw(NotImplemented, "Parser logic error. No pool storage set,"
" but pool parser asked to parse pools");
isc_throw(NotImplemented, "Parser logic error. No pool storage set,"
" but pool parser asked to parse pools");
}
BOOST_FOREACH(ConstElementPtr text_pool, pools_list->listValue()) {
......@@ -372,14 +372,19 @@ public:
uint8_t len = 0;
try {
addr = IOAddress(txt.substr(0, pos));
string num = txt.substr(pos+1);
// it is lexical cast to int and then downcast to uint8_t
// direct cast to uint8_t (which is really an unsigned char)
// start with the first charater after /
string prefix_len = txt.substr(pos + 1);
// It is lexical cast to int and then downcast to uint8_t.
// Direct cast to uint8_t (which is really an unsigned char)
// will result in interpreting the first digit as output
// value and throwing exception if length written on two
// digits (because there are extra characters left over)
len = boost::lexical_cast<int>(num);
// value and throwing exception if length is written on two
// digits (because there are extra characters left over).
// No checks for values over 128. Range correctness will
// be checked in Pool6 constructor.
len = boost::lexical_cast<int>(prefix_len);
} catch (...) {
isc_throw(Dhcp6ConfigError, "Failed to parse pool "
"definition: " << text_pool->stringValue());
......@@ -394,8 +399,8 @@ public:
pos = txt.find("-");
if (pos != string::npos) {
// using min-max notation
IOAddress min(txt.substr(0,pos-1));
IOAddress max(txt.substr(pos+1));
IOAddress min(txt.substr(0,pos - 1));
IOAddress max(txt.substr(pos + 1));
Pool6Ptr pool(new Pool6(Pool6::TYPE_IA, min, max));
......@@ -422,13 +427,13 @@ public:
///
/// This method is required for all parser. The value itself
/// is not commited anywhere. Higher level parsers (for subnet) are expected
/// to use values stored in the storage,
/// to use values stored in the storage.
virtual void commit() {}
/// @brief factory that constructs PoolParser objects
///
/// @param param_name name of the parameter to be parsed
static Dhcp6ConfigParser* Factory(const std::string& param_name) {
static DhcpConfigParser* Factory(const std::string& param_name) {
return (new PoolParser(param_name));
}
......@@ -437,14 +442,14 @@ protected:
///
/// That is typically a storage somewhere in Subnet parser
/// (an upper level parser).
PoolStorage * pools_;
PoolStorage* pools_;
};
/// @brief this class parses a single subnet
///
/// This class parses the whole subnet definition. It creates parsers
/// for received configuration parameters as needed.
class Subnet6ConfigParser : public Dhcp6ConfigParser {
class Subnet6ConfigParser : public DhcpConfigParser {
public:
/// @brief constructor
......@@ -468,18 +473,20 @@ public:
boost::dynamic_pointer_cast<Uint32Parser>(parser);
if (uintParser) {
uintParser->setStorage(&uint32_values_);
}
boost::shared_ptr<StringParser> stringParser =
boost::dynamic_pointer_cast<StringParser>(parser);
if (stringParser) {
stringParser->setStorage(&string_values_);
}
boost::shared_ptr<PoolParser> poolParser =
boost::dynamic_pointer_cast<PoolParser>(parser);
if (poolParser) {
poolParser->setStorage(&pools_);
} else {
boost::shared_ptr<StringParser> stringParser =
boost::dynamic_pointer_cast<StringParser>(parser);
if (stringParser) {
stringParser->setStorage(&string_values_);
} else {
boost::shared_ptr<PoolParser> poolParser =
boost::dynamic_pointer_cast<PoolParser>(parser);
if (poolParser) {
poolParser->setStorage(&pools_);
}
}
}
parser->build(param.second);
......@@ -544,7 +551,7 @@ protected:
///
/// @param config_id name od the entry
/// @return parser object for specified entry name
Dhcp6ConfigParser* createSubnet6ConfigParser(const std::string& config_id) {
DhcpConfigParser* createSubnet6ConfigParser(const std::string& config_id) {
FactoryMap factories;
factories.insert(pair<string, ParserFactory*>(
......@@ -565,7 +572,7 @@ protected:
FactoryMap::iterator f = factories.find(config_id);
if (f == factories.end()) {
// Used for debugging only.
// return new DummyParser(config_id);
// return new DebugParser(config_id);
isc_throw(NotImplemented,
"Parser error: Subnet6 parameter not supported: "
......@@ -624,7 +631,7 @@ protected:
/// This is a wrapper parser that handles the whole list of Subnet6
/// definitions. It iterates over all entries and creates Subnet6ConfigParser
/// for each entry.
class Subnets6ListConfigParser : public Dhcp6ConfigParser {
class Subnets6ListConfigParser : public DhcpConfigParser {
public:
/// @brief constructor
......@@ -673,7 +680,7 @@ public:
/// @brief Returns Subnet6ListConfigParser object
/// @param param_name name of the parameter
/// @return Subnets6ListConfigParser object
static Dhcp6ConfigParser* Factory(const std::string& param_name) {
static DhcpConfigParser* Factory(const std::string& param_name) {
return (new Subnets6ListConfigParser(param_name));
}
......@@ -688,7 +695,7 @@ public:
///
/// @param config_id pointer to received global configuration entry
/// @return parser for specified global DHCPv6 parameter
Dhcp6ConfigParser* createGlobalDhcp6ConfigParser(const std::string& config_id) {
DhcpConfigParser* createGlobalDhcpConfigParser(const std::string& config_id) {
FactoryMap factories;
//
......@@ -712,7 +719,7 @@ Dhcp6ConfigParser* createGlobalDhcp6ConfigParser(const std::string& config_id) {
FactoryMap::iterator f = factories.find(config_id);
if (f == factories.end()) {
// Used for debugging only.
// return new DummyParser(config_id);
// return new DebugParser(config_id);
isc_throw(NotImplemented,
"Parser error: Global configuration parameter not supported: "
......@@ -751,7 +758,7 @@ configureDhcp6Server(Dhcpv6Srv& , ConstElementPtr config_set) {
try {
BOOST_FOREACH(ConfigPair config_pair, config_set->mapValue()) {
ParserPtr parser(createGlobalDhcp6ConfigParser(config_pair.first));
ParserPtr parser(createGlobalDhcpConfigParser(config_pair.first));
parser->build(config_pair.second);
parsers.push_back(parser);
}
......
......@@ -38,7 +38,7 @@ Dhcp6ConfigError(const char* file, size_t line, const char* what) :
isc::Exception(file, line, what) {}
};
class Dhcp6ConfigParser {
class DhcpConfigParser {
///
/// \name Constructors and Destructor
///
......@@ -47,30 +47,24 @@ class Dhcp6ConfigParser {
/// pure base class.
//@{
private:
Dhcp6ConfigParser(const Dhcp6ConfigParser& source);
Dhcp6ConfigParser& operator=(const Dhcp6ConfigParser& source);
DhcpConfigParser(const DhcpConfigParser& source);
DhcpConfigParser& operator=(const DhcpConfigParser& source);
protected:
/// \brief The default constructor.
///
/// This is intentionally defined as \c protected as this base class should
/// never be instantiated (except as part of a derived class).
Dhcp6ConfigParser() {}
DhcpConfigParser() {}
public:
/// The destructor.
virtual ~Dhcp6ConfigParser() {}
virtual ~DhcpConfigParser() {}
//@}
/// Prepare configuration value.
/// \brief Prepare configuration value.
///
/// This method parses the "value part" of the configuration identifier
/// that corresponds to this derived class and prepares a new value to
/// apply to the server.
/// In the above example, the derived class for the identifier "param1"
/// would be passed an data \c Element storing an integer whose value
/// is 10, and would record that value internally;
/// the derived class for the identifier "param2" would be passed a
/// map element and (after parsing) convert it into some internal
/// data structure.
///
/// This method must validate the given value both in terms of syntax
/// and semantics of the configuration, so that the server will be
......@@ -84,15 +78,15 @@ public:
/// allocation. If it fails, it may throw a corresponding standard
/// exception.
///
/// This method is not expected to be called more than once. Although
/// multiple calls are not prohibited by the interface, the behavior
/// is undefined.
/// This method is not expected to be called more than once in the
/// life of the object. Although multiple calls are not prohibited
/// by the interface, the behavior is undefined.
///
/// \param config_value The configuration value for the identifier
/// corresponding to the derived class.
virtual void build(isc::data::ConstElementPtr config_value) = 0;
/// Apply the prepared configuration value to the server.
/// \brief Apply the prepared configuration value to the server.
///
/// This method is expected to be exception free, and, as a consequence,
/// it should normally not involve resource allocation.
......@@ -110,7 +104,7 @@ public:
};
/// @brief a pointer to configuration parser
typedef boost::shared_ptr<Dhcp6ConfigParser> ParserPtr;
typedef boost::shared_ptr<DhcpConfigParser> ParserPtr;
/// @brief a collection of parsers
///
......@@ -118,12 +112,12 @@ typedef boost::shared_ptr<Dhcp6ConfigParser> ParserPtr;
typedef std::vector<ParserPtr> ParserCollection;
/// Configure an \c Dhcpv6Srv object with a set of configuration values.
/// \brief Configure an \c Dhcpv6Srv object with a set of configuration values.
///
/// This function parses configuration information stored in \c config_set
/// and configures the \c server by applying the configuration to it.
/// It provides the strong exception guarantee as long as the underlying
/// derived class implementations of \c Dhcp6ConfigParser meet the assumption,
/// derived class implementations of \c DhcpConfigParser meet the assumption,
/// that is, it ensures that either configuration is fully applied or the
/// state of the server is intact.
///
......@@ -147,38 +141,6 @@ isc::data::ConstElementPtr
configureDhcp6Server(Dhcpv6Srv& server,
isc::data::ConstElementPtr config_set);
/// Create a new \c Dhcp6ConfigParser object for a given configuration
/// identifier.
///
/// It internally identifies an appropriate derived class for the given
/// identifier and creates a new instance of that class. The caller can
/// then configure the \c server regarding the identifier by calling
/// the \c build() and \c commit() methods of the returned object.
///
/// In practice, this function is only expected to be used as a backend of
/// \c configureDhcp6Server() and is not supposed to be called directly
/// by applications. It is publicly available mainly for testing purposes.
/// When called directly, the created object must be deleted by the caller.
/// Note: this means if this module and the caller use incompatible sets of
/// new/delete, it may cause unexpected strange failure. We could avoid that
/// by providing a separate deallocation function or by using a smart pointer,
/// but since the expected usage of this function is very limited (i.e. for
/// our own testing purposes) it would be an overkilling. We therefore prefer
/// simplicity and keeping the interface intuitive.
///
/// If the resource allocation for the new object fails, a corresponding
/// standard exception will be thrown. Otherwise this function is not
/// expected to throw an exception, unless the constructor of the underlying
/// derived class implementation (unexpectedly) throws.
///
/// \param server The \c Dhcpv6Srv object to be configured.
/// \param config_id The configuration identifier for which a parser object
/// is to be created.
/// \return A pointer to an \c Dhcp6ConfigParser object.
Dhcp6ConfigParser* createDhcp6ConfigParser(Dhcpv6Srv& server,
const std::string& config_id);
}; // end of isc::dhcp namespace
}; // end of isc namespace
......
......@@ -53,7 +53,8 @@ ControlledDhcpv6Srv::dhcp6ConfigHandler(ConstElementPtr new_config) {
return (configureDhcp6Server(*server_, new_config));
}
// that should never happen as we install config_handler after we
// That should never happen as we install config_handler after we instantiate
// the server.
ConstElementPtr answer = isc::config::createAnswer(1,
"Configuration rejected, server is during startup/shutdown phase.");
return (answer);
......
......@@ -39,7 +39,9 @@ class Dhcp6ParserTest : public ::testing::Test {
public:
Dhcp6ParserTest()
:rcode_(-1) {
// open port 0 means to not do anything at all
// Open port 0 means to not do anything at all. We don't want to
// deal with sockets here, just check if configuration handling
// is sane.
srv_ = new Dhcpv6Srv(0);
}
......@@ -47,13 +49,15 @@ public:
delete srv_;
};
Dhcpv6Srv * srv_;
Dhcpv6Srv* srv_;
int rcode_;
ConstElementPtr comment_;
};
// Goal of this test is a verification if a very simple config update
// with just a bumped version number. That's the simplest possible
// config update.
TEST_F(Dhcp6ParserTest, version) {
ConstElementPtr x;
......@@ -67,6 +71,8 @@ TEST_F(Dhcp6ParserTest, version) {
EXPECT_EQ(0, rcode_);
}
/// The goal of this test is to verify that the code accepts only
/// valid commands and malformed or unsupported parameters are rejected.
TEST_F(Dhcp6ParserTest, bogus_command) {
ConstElementPtr x;
......@@ -80,11 +86,14 @@ TEST_F(Dhcp6ParserTest, bogus_command) {
EXPECT_EQ(1, rcode_);
}
/// The goal of this test is to verify if wrongly defined subnet will
/// be rejected. Properly defined subnet must include at least one
/// pool definition.
TEST_F(Dhcp6ParserTest, empty_subnet) {
ConstElementPtr x;
ConstElementPtr status;
EXPECT_NO_THROW(x = configureDhcp6Server(*srv_,
EXPECT_NO_THROW(status = configureDhcp6Server(*srv_,
Element::fromJSON("{ \"interface\": [ \"all\" ],"
"\"preferred-lifetime\": 3000,"
"\"rebind-timer\": 2000, "
......@@ -92,15 +101,17 @@ TEST_F(Dhcp6ParserTest, empty_subnet) {
"\"subnet6\": [ ], "
"\"valid-lifetime\": 4000 }")));
// returned value must be 1 (configuration parse error)
ASSERT_TRUE(x);
comment_ = parseAnswer(rcode_, x);
// returned value should be 0 (success)
ASSERT_TRUE(status);
comment_ = parseAnswer(rcode_, status);
EXPECT_EQ(0, rcode_);
}
/// The goal of this test is to verify if defined subnet uses global
/// parameter timer definitions.
TEST_F(Dhcp6ParserTest, subnet_global_defaults) {
ConstElementPtr x;
ConstElementPtr status;
string config = "{ \"interface\": [ \"all\" ],"
"\"preferred-lifetime\": 3000,"
......@@ -114,13 +125,15 @@ TEST_F(Dhcp6ParserTest, subnet_global_defaults) {
ElementPtr json = Element::fromJSON(config);
EXPECT_NO_THROW(x = configureDhcp6Server(*srv_, json));
EXPECT_NO_THROW(status = configureDhcp6Server(*srv_, json));
// returned value must be 1 (configuration parse error)
ASSERT_TRUE(x);
comment_ = parseAnswer(rcode_, x);
// check if returned status is OK
ASSERT_TRUE(status);
comment_ = parseAnswer(rcode_, status);
EXPECT_EQ(0, rcode_);
// Now check if the configuration was indeed handled and we have
// expected pool configured.
Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(IOAddress("2001:db8:1::5"));
ASSERT_TRUE(subnet);
EXPECT_EQ(1000, subnet->getT1());
......@@ -129,10 +142,11 @@ TEST_F(Dhcp6ParserTest, subnet_global_defaults) {
EXPECT_EQ(4000, subnet->getValid());
}
//
// This test checks if it is possible to override global values
// on a per subnet basis.
TEST_F(Dhcp6ParserTest, subnet_local) {
ConstElementPtr x;
ConstElementPtr status;
string config = "{ \"interface\": [ \"all\" ],"
"\"preferred-lifetime\": 3000,"
......@@ -150,11 +164,11 @@ TEST_F(Dhcp6ParserTest, subnet_local) {
ElementPtr json = Element::fromJSON(config);
EXPECT_NO_THROW(x = configureDhcp6Server(*srv_, json));
EXPECT_NO_THROW(status = configureDhcp6Server(*srv_, json));
// returned value must be 1 (configuration parse error)
ASSERT_TRUE(x);
comment_ = parseAnswer(rcode_, x);
// returned value should be 0 (configuration success)
ASSERT_TRUE(status);
comment_ = parseAnswer(rcode_, status);
EXPECT_EQ(0, rcode_);
Subnet6Ptr subnet = CfgMgr::instance().getSubnet6(IOAddress("2001:db8:1::5"));
......@@ -165,9 +179,11 @@ TEST_F(Dhcp6ParserTest, subnet_local) {
EXPECT_EQ(4, subnet->getValid());
}
// Test verifies that a subnet with pool values that do not belong to that
// pool are rejected.
TEST_F(Dhcp6ParserTest, pool_out_of_subnet) {