Commit fcbe15be authored by Francis Dupont's avatar Francis Dupont
Browse files

[5549a] Rebased/Updated code - toto unit tests to port

parent a6c46447
......@@ -160,7 +160,15 @@ Dhcpv4Exchange::Dhcpv4Exchange(const AllocEnginePtr& alloc_engine,
// Check for static reservations.
alloc_engine->findReservation(*context_);
// Set known builtin class if something was found.
if (!context_->hosts_.empty()) {
query->addClass("KNOWN");
}
}
// Perform second pass of classification.
Dhcpv4Srv::evaluateClasses(query, true);
}
const ClientClasses& classes = query_->getClasses();
......@@ -3186,10 +3194,14 @@ void Dhcpv4Srv::classifyPacket(const Pkt4Ptr& pkt) {
// All packets belongs to ALL.
pkt->addClass("ALL");
// First phase: built-in vendor class processing
// First: built-in vendor class processing.
classifyByVendor(pkt);
// Run match expressions
// Run match expressions on classes not depending on KNOWN.
evaluateClasses(pkt, false);
}
void Dhcpv4Srv::evaluateClasses(const Pkt4Ptr& pkt, bool depend_on_known) {
// Note getClientClassDictionary() cannot be null
const ClientClassDictionaryPtr& dict =
CfgMgr::instance().getCurrentCfg()->getClientClassDictionary();
......@@ -3206,6 +3218,10 @@ void Dhcpv4Srv::classifyPacket(const Pkt4Ptr& pkt) {
if ((*it)->getRequired()) {
continue;
}
// Not the right pass.
if ((*it)->getDependOnKnown() != depend_on_known) {
continue;
}
// Evaluate the expression which can return false (no match),
// true (match) or raise an exception (error)
try {
......
......@@ -850,6 +850,19 @@ protected:
/// @param pkt packet to be classified
void classifyPacket(const Pkt4Ptr& pkt);
public:
/// @brief Evaluate classes.
///
/// @note Second part of the classification.
///
/// @param pkt packet to be classified.
/// @param depend_on_known if false classes depending on the KNOWN
/// class are skipped, if true only these classes are evaluated.
static void evaluateClasses(const Pkt4Ptr& pkt, bool depend_on_known);
protected:
/// @brief Assigns incoming packet to zero or more classes (required pass).
///
/// @note This required classification evaluates all classes which
......
......@@ -378,6 +378,14 @@ Dhcpv6Srv::initContext(const Pkt6Ptr& pkt,
// Find host reservations using specified identifiers.
alloc_engine_->findReservation(ctx);
// Set known builtin class if something was found.
if (!ctx.hosts_.empty()) {
pkt->addClass("KNOWN");
}
// Perform second pass of classification.
evaluateClasses(pkt, true);
}
}
......@@ -3261,10 +3269,14 @@ void Dhcpv6Srv::classifyPacket(const Pkt6Ptr& pkt) {
pkt->addClass("ALL");
string classes = "ALL ";
// First phase: built-in vendor class processing
// First: built-in vendor class processing
classifyByVendor(pkt, classes);
// Run match expressions
// Run match expressions on classes not depending on KNOWN.
evaluateClasses(pkt, false);
}
void Dhcpv6Srv::evaluateClasses(const Pkt6Ptr& pkt, bool depend_on_known) {
// Note getClientClassDictionary() cannot be null
const ClientClassDictionaryPtr& dict =
CfgMgr::instance().getCurrentCfg()->getClientClassDictionary();
......@@ -3281,6 +3293,10 @@ void Dhcpv6Srv::classifyPacket(const Pkt6Ptr& pkt) {
if ((*it)->getRequired()) {
continue;
}
// Not the right pass.
if ((*it)->getDependOnKnown() != depend_on_known) {
continue;
}
// Evaluate the expression which can return false (no match),
// true (match) or raise an exception (error)
try {
......@@ -3291,7 +3307,6 @@ void Dhcpv6Srv::classifyPacket(const Pkt6Ptr& pkt) {
.arg(status);
// Matching: add the class
pkt->addClass((*it)->getName());
classes += (*it)->getName() + " ";
} else {
LOG_DEBUG(dhcp6_logger, DBG_DHCP6_DETAIL, EVAL_RESULT)
.arg((*it)->getName())
......
......@@ -674,6 +674,15 @@ protected:
/// @param pkt packet to be classified
void classifyPacket(const Pkt6Ptr& pkt);
/// @brief Evaluate classes.
///
/// @note Second part of the classification.
///
/// @param pkt packet to be classified.
/// @param depend_on_known if false classes depending on the KNOWN
/// class are skipped, if true only these classes are evaluated.
void evaluateClasses(const Pkt6Ptr& pkt, bool depend_on_known);
/// @brief Assigns classes retrieved from host reservation database.
///
/// @param pkt Pointer to the packet to which classes will be assigned.
......
......@@ -21,7 +21,7 @@ ClientClassDef::ClientClassDef(const std::string& name,
const ExpressionPtr& match_expr,
const CfgOptionPtr& cfg_option)
: name_(name), match_expr_(match_expr), required_(false),
cfg_option_(cfg_option),
depend_on_known_(false), cfg_option_(cfg_option),
next_server_(asiolink::IOAddress::IPV4_ZERO_ADDRESS()) {
// Name can't be blank
......@@ -39,8 +39,8 @@ ClientClassDef::ClientClassDef(const std::string& name,
}
ClientClassDef::ClientClassDef(const ClientClassDef& rhs)
: name_(rhs.name_), match_expr_(ExpressionPtr()),
required_(false), cfg_option_(new CfgOption()),
: name_(rhs.name_), match_expr_(ExpressionPtr()), required_(false),
depend_on_known_(false), cfg_option_(new CfgOption()),
next_server_(asiolink::IOAddress::IPV4_ZERO_ADDRESS()) {
if (rhs.match_expr_) {
......@@ -57,6 +57,7 @@ ClientClassDef::ClientClassDef(const ClientClassDef& rhs)
}
required_ = rhs.required_;
depend_on_known_ = rhs.depend_on_known_;
next_server_ = rhs.next_server_;
sname_ = rhs.sname_;
filename_ = rhs.filename_;
......@@ -105,6 +106,16 @@ ClientClassDef::setRequired(bool required) {
required_ = required;
}
bool
ClientClassDef::getDependOnKnown() const {
return (depend_on_known_);
}
void
ClientClassDef::setDependOnKnown(bool depend_on_known) {
depend_on_known_ = depend_on_known;
}
const CfgOptionDefPtr&
ClientClassDef::getCfgOptionDef() const {
return (cfg_option_def_);
......@@ -138,6 +149,7 @@ ClientClassDef::equals(const ClientClassDef& other) const {
(cfg_option_def_ && other.cfg_option_def_ &&
(*cfg_option_def_ == *other.cfg_option_def_))) &&
(required_ == other.required_) &&
(depend_on_known_ == other.depend_on_known_) &&
(next_server_ == other.next_server_) &&
(sname_ == other.sname_) &&
(filename_ == other.filename_));
......@@ -205,6 +217,7 @@ ClientClassDictionary::addClass(const std::string& name,
const ExpressionPtr& match_expr,
const std::string& test,
bool required,
bool depend_on_known,
const CfgOptionPtr& cfg_option,
CfgOptionDefPtr cfg_option_def,
ConstElementPtr user_context,
......@@ -214,6 +227,7 @@ ClientClassDictionary::addClass(const std::string& name,
ClientClassDefPtr cclass(new ClientClassDef(name, match_expr, cfg_option));
cclass->setTest(test);
cclass->setRequired(required);
cclass->setDependOnKnown(depend_on_known);
cclass->setCfgOptionDef(cfg_option_def);
cclass->setContext(user_context),
cclass->setNextServer(next_server);
......@@ -333,15 +347,24 @@ isClientClassBuiltIn(const ClientClass& client_class) {
bool
isClientClassDefined(ClientClassDictionaryPtr& class_dictionary,
bool& depend_on_known,
const ClientClass& client_class) {
// First check built-in classes
if (isClientClassBuiltIn(client_class)) {
// Check direct dependency on KNOWN
if (client_class == "KNOWN") {
depend_on_known = true;
}
return (true);
}
// Second check already defined, i.e. in the dictionary
ClientClassDefPtr def = class_dictionary->findClass(client_class);
if (def) {
// Check indirect dependency on KNOWN
if (def->getDependOnKnown()) {
depend_on_known = true;
}
return (true);
}
......
......@@ -87,8 +87,18 @@ public:
bool getRequired() const;
/// @brief Sets the only if required flag
///
/// @param required the value of the only if required flag
void setRequired(bool required);
/// @brief Fetches the depend on known flag aka use host flag
bool getDependOnKnown() const;
/// @brief Sets the depend on known flag aka use host flag
///
/// @param depend_on_known the value of the depend on known flag
void setDependOnKnown(bool depend_on_known);
/// @brief Fetches the class's option definitions
const CfgOptionDefPtr& getCfgOptionDef() const;
......@@ -195,6 +205,16 @@ private:
/// only when required and is usable only for option configuration.
bool required_;
/// @brief The depend on known aka use host flag: when false (the default),
/// the required flag is false and the class has a match expression
/// the expression is evaluated in the first pass. When true and the
/// two other conditions stand the expression is evaluated later when
/// the host reservation membership was determined.
/// This flag is set to true during the match expression parsing if
/// direct or indirect dependency on the builtin KNOWN class is
/// detected.
bool depend_on_known_;
/// @brief The option definition configuration for this class
CfgOptionDefPtr cfg_option_def_;
......@@ -253,6 +273,7 @@ public:
/// @param match_expr Expression the class will use to determine membership
/// @param test Original version of match_expr
/// @param required Original value of the only if required flag
/// @param depend_on_known Using host so will be evaluated later
/// @param options Collection of options members should be given
/// @param defs Option definitions (optional)
/// @param user_context User context (optional)
......@@ -264,7 +285,7 @@ public:
/// dictionary. See @ref dhcp::ClientClassDef::ClientClassDef() for
/// others.
void addClass(const std::string& name, const ExpressionPtr& match_expr,
const std::string& test, bool required,
const std::string& test, bool required, bool depend_on_known,
const CfgOptionPtr& options,
CfgOptionDefPtr defs = CfgOptionDefPtr(),
isc::data::ConstElementPtr user_context = isc::data::ConstElementPtr(),
......@@ -361,10 +382,15 @@ bool isClientClassBuiltIn(const ClientClass& client_class);
/// @brief Check if a client class name is already defined,
/// i.e. is built-in or in the dictionary,
///
/// The reference to depend on known flag is set to true if the class
/// is KNOWN (direct dependency) or has this flag set (indirect dependency).
///
/// @param class_dictionary A class dictionary where to look for.
/// @param depend_on_known A reference to depend on known flag.
/// @param client_class A client class name to look for.
/// @return true if defined or built-in, false if not.
bool isClientClassDefined(ClientClassDictionaryPtr& class_dictionary,
bool& depend_on_known,
const ClientClass& client_class);
} // namespace isc::dhcp
......
......@@ -77,15 +77,27 @@ ClientClassDefParser::parse(ClientClassDictionaryPtr& class_dictionary,
<< getPosition("name", class_def_cfg) << ")");
}
// Let's try to parse the only-if-required flag
bool required = false;
if (class_def_cfg->contains("only-if-required")) {
required = getBoolean(class_def_cfg, "only-if-required");
}
// Parse matching expression
ExpressionPtr match_expr;
ConstElementPtr test_cfg = class_def_cfg->get("test");
std::string test;
bool depend_on_known = false;
if (test_cfg) {
ExpressionParser parser;
using std::placeholders::_1;
auto check_defined =
std::bind(isClientClassDefined, class_dictionary, _1);
[&class_dictionary, &depend_on_known]
(const ClientClass& cclass) {
return (isClientClassDefined(class_dictionary,
depend_on_known,
cclass));
};
parser.parse(match_expr, test_cfg, family, check_defined);
test = test_cfg->stringValue();
}
......@@ -136,12 +148,6 @@ ClientClassDefParser::parse(ClientClassDictionaryPtr& class_dictionary,
// Parse user context
ConstElementPtr user_context = class_def_cfg->get("user-context");
// Let's try to parse the only-if-required flag
bool required = false;
if (class_def_cfg->contains("only-if-required")) {
required = getBoolean(class_def_cfg, "only-if-required");
}
// Let's try to parse the next-server field
IOAddress next_server("0.0.0.0");
if (class_def_cfg->contains("next-server")) {
......@@ -197,9 +203,9 @@ ClientClassDefParser::parse(ClientClassDictionaryPtr& class_dictionary,
// Add the client class definition
try {
class_dictionary->addClass(name, match_expr, test, required, options,
defs, user_context, next_server, sname,
filename);
class_dictionary->addClass(name, match_expr, test, required,
depend_on_known, options, defs,
user_context, next_server, sname, filename);
} catch (const std::exception& ex) {
isc_throw(DhcpConfigError, "Can't add class: " << ex.what()
<< " (" << class_def_cfg->getPosition() << ")");
......
......@@ -970,4 +970,60 @@ TEST_F(ClientClassDefListParserTest, dependentBackward) {
EXPECT_NO_THROW(parseClientClassDefList(cfg_text, AF_INET6));
}
// Verifies that the depend on known flag is correctly handled.
TEST_F(ClientClassDefListParserTest, dependOnKnown) {
std::string cfg_text =
"[ \n"
" { \n"
" \"name\": \"alpha\", \n"
" \"test\": \"member('ALL')\" \n"
" }, \n"
" { \n"
" \"name\": \"beta\", \n"
" \"test\": \"member('alpha')\" \n"
" }, \n"
" { \n"
" \"name\": \"gamma\", \n"
" \"test\": \"member('KNOWN') and member('alpha')\" \n"
" }, \n"
" { \n"
" \"name\": \"delta\", \n"
" \"test\": \"member('beta') and member('gamma')\" \n"
" } \n"
"] \n";
// Parsing the list should succeed.
ClientClassDictionaryPtr dictionary;
EXPECT_NO_THROW(dictionary = parseClientClassDefList(cfg_text, AF_INET6));
ASSERT_TRUE(dictionary);
// We should have four classes in the dictionary.
EXPECT_EQ(4, dictionary->getClasses()->size());
// Check alpha.
ClientClassDefPtr cclass;
ASSERT_NO_THROW(cclass = dictionary->findClass("alpha"));
ASSERT_TRUE(cclass);
EXPECT_EQ("alpha", cclass->getName());
EXPECT_FALSE(cclass->getDependOnKnown());
// Check beta.
ASSERT_NO_THROW(cclass = dictionary->findClass("beta"));
ASSERT_TRUE(cclass);
EXPECT_EQ("beta", cclass->getName());
EXPECT_FALSE(cclass->getDependOnKnown());
// Check gamma which directly depends on KNOWN.
ASSERT_NO_THROW(cclass = dictionary->findClass("gamma"));
ASSERT_TRUE(cclass);
EXPECT_EQ("gamma", cclass->getName());
EXPECT_TRUE(cclass->getDependOnKnown());
// Check delta which indirectly depends on KNOWN.
ASSERT_NO_THROW(cclass = dictionary->findClass("delta"));
ASSERT_TRUE(cclass);
EXPECT_EQ("delta", cclass->getName());
EXPECT_TRUE(cclass->getDependOnKnown());
}
} // end of anonymous namespace
......@@ -148,6 +148,15 @@ TEST(ClientClassDef, copyAndEquality) {
EXPECT_TRUE(cclass2->getRequired());
EXPECT_FALSE(*cclass == *cclass2);
EXPECT_TRUE(*cclass != *cclass2);
cclass2->setRequired(false);
EXPECT_TRUE(*cclass == *cclass2);
// Verify the depend on known flag is enough to make classes not equal.
EXPECT_FALSE(cclass->getDependOnKnown());
cclass2->setDependOnKnown(true);
EXPECT_TRUE(cclass2->getDependOnKnown());
EXPECT_FALSE(*cclass == *cclass2);
EXPECT_TRUE(*cclass != *cclass2);
// Make a class that differs from the first class only by name and
// verify that the equality tools reflect that the classes are not equal.
......@@ -233,15 +242,19 @@ TEST(ClientClassDictionary, basics) {
// Verify that we can add classes with both addClass variants
// First addClass(name, expression, cfg_option)
ASSERT_NO_THROW(dictionary->addClass("cc1", expr, "", false, cfg_option));
ASSERT_NO_THROW(dictionary->addClass("cc2", expr, "", false, cfg_option));
ASSERT_NO_THROW(dictionary->addClass("cc1", expr, "", false,
false, cfg_option));
ASSERT_NO_THROW(dictionary->addClass("cc2", expr, "", false,
false, cfg_option));
// Verify duplicate add attempt throws
ASSERT_THROW(dictionary->addClass("cc2", expr, "", false, cfg_option),
ASSERT_THROW(dictionary->addClass("cc2", expr, "", false,
false, cfg_option),
DuplicateClientClassDef);
// Verify that you cannot add a class with no name.
ASSERT_THROW(dictionary->addClass("", expr, "", false, cfg_option),
ASSERT_THROW(dictionary->addClass("", expr, "", false,
false, cfg_option),
BadValue);
// Now with addClass(class pointer)
......@@ -298,9 +311,12 @@ TEST(ClientClassDictionary, copyAndEquality) {
CfgOptionPtr options;
dictionary.reset(new ClientClassDictionary());
ASSERT_NO_THROW(dictionary->addClass("one", expr, "", false, options));
ASSERT_NO_THROW(dictionary->addClass("two", expr, "", false, options));
ASSERT_NO_THROW(dictionary->addClass("three", expr, "", false, options));
ASSERT_NO_THROW(dictionary->addClass("one", expr, "", false,
false, options));
ASSERT_NO_THROW(dictionary->addClass("two", expr, "", false,
false, options));
ASSERT_NO_THROW(dictionary->addClass("three", expr, "", false,
false, options));
// Copy constructor should succeed.
ASSERT_NO_THROW(dictionary2.reset(new ClientClassDictionary(*dictionary)));
......@@ -347,6 +363,7 @@ TEST(ClientClassDef, fixedFieldsDefaults) {
// Let's checks that it doesn't return any nonsense
EXPECT_FALSE(cclass->getRequired());
EXPECT_FALSE(cclass->getDependOnKnown());
EXPECT_FALSE(cclass->getCfgOptionDef());
string empty;
ASSERT_EQ(IOAddress("0.0.0.0"), cclass->getNextServer());
......@@ -370,6 +387,7 @@ TEST(ClientClassDef, fixedFieldsBasics) {
ASSERT_NO_THROW(cclass.reset(new ClientClassDef(name, expr)));
cclass->setRequired(true);
cclass->setDependOnKnown(true);
string sname = "This is a very long string that can be a server name";
string filename = "this-is-a-slightly-longish-name-of-a-file.txt";
......@@ -380,6 +398,7 @@ TEST(ClientClassDef, fixedFieldsBasics) {
// Let's checks that it doesn't return any nonsense
EXPECT_TRUE(cclass->getRequired());
EXPECT_TRUE(cclass->getDependOnKnown());
EXPECT_EQ(IOAddress("1.2.3.4"), cclass->getNextServer());
EXPECT_EQ(sname, cclass->getSname());
EXPECT_EQ(filename, cclass->getFilename());
......@@ -402,6 +421,8 @@ TEST(ClientClassDef, unparseDef) {
user_context += "\"bar\": 1 }";
cclass->setContext(isc::data::Element::fromJSON(user_context));
cclass->setRequired(true);
// The depend on known flag in not visible
cclass->setDependOnKnown(true);
std::string next_server = "1.2.3.4";
cclass->setNextServer(IOAddress(next_server));
std::string sname = "my-server.example.com";
......@@ -432,9 +453,12 @@ TEST(ClientClassDictionary, unparseDict) {
// Get a client class dictionary and fill it
dictionary.reset(new ClientClassDictionary());
ASSERT_NO_THROW(dictionary->addClass("one", expr, "", false, options));
ASSERT_NO_THROW(dictionary->addClass("two", expr, "", false, options));
ASSERT_NO_THROW(dictionary->addClass("three", expr, "", false, options));
ASSERT_NO_THROW(dictionary->addClass("one", expr, "", false,
false, options));
ASSERT_NO_THROW(dictionary->addClass("two", expr, "", false,
false, options));
ASSERT_NO_THROW(dictionary->addClass("three", expr, "", false,
false, options));
// Unparse it
auto add_defaults =
......
// Copyright (C) 2014-2017 Internet Systems Consortium, Inc. ("ISC")
// Copyright (C) 2014-2018 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
......@@ -67,11 +67,11 @@ public:
// Build our reference dictionary of client classes
ref_dictionary_->addClass("cc1", ExpressionPtr(),
"", false, CfgOptionPtr());
"", false, false, CfgOptionPtr());
ref_dictionary_->addClass("cc2", ExpressionPtr(),
"", false, CfgOptionPtr());
"", false, false, CfgOptionPtr());
ref_dictionary_->addClass("cc3", ExpressionPtr(),
"", false, CfgOptionPtr());
"", false, false, CfgOptionPtr());
}
......
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