From: Francis Dupont Date: Fri, 18 May 2018 21:59:32 +0000 (+0200) Subject: [5549a] Rebased/Updated code - toto unit tests to port X-Git-Tag: 136-add-global-host-reservation-examples_base~4^2~2^2~5 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=fcbe15be4780f0a12972e4ed505ecb4ff2f65dfa;p=thirdparty%2Fkea.git [5549a] Rebased/Updated code - toto unit tests to port --- diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 6b8f365e97..149265a550 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -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 { diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index fd8c671f6c..03d7b79056 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/src/bin/dhcp4/dhcp4_srv.h @@ -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 diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 30066b1b0e..f193007985 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -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()) diff --git a/src/bin/dhcp6/dhcp6_srv.h b/src/bin/dhcp6/dhcp6_srv.h index 6debce96ac..bb7cfac474 100644 --- a/src/bin/dhcp6/dhcp6_srv.h +++ b/src/bin/dhcp6/dhcp6_srv.h @@ -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. diff --git a/src/lib/dhcpsrv/client_class_def.cc b/src/lib/dhcpsrv/client_class_def.cc index 91158d9205..95acbbadac 100644 --- a/src/lib/dhcpsrv/client_class_def.cc +++ b/src/lib/dhcpsrv/client_class_def.cc @@ -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); } diff --git a/src/lib/dhcpsrv/client_class_def.h b/src/lib/dhcpsrv/client_class_def.h index 623710eb83..9a1cbd0ab8 100644 --- a/src/lib/dhcpsrv/client_class_def.h +++ b/src/lib/dhcpsrv/client_class_def.h @@ -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 diff --git a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc index 0394f26e43..5a78efce9f 100644 --- a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc +++ b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc @@ -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() << ")"); diff --git a/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc b/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc index d3af6cf043..a1b1989aac 100644 --- a/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc +++ b/src/lib/dhcpsrv/tests/client_class_def_parser_unittest.cc @@ -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 diff --git a/src/lib/dhcpsrv/tests/client_class_def_unittest.cc b/src/lib/dhcpsrv/tests/client_class_def_unittest.cc index c29fc45d30..2fe8267705 100644 --- a/src/lib/dhcpsrv/tests/client_class_def_unittest.cc +++ b/src/lib/dhcpsrv/tests/client_class_def_unittest.cc @@ -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 = diff --git a/src/lib/dhcpsrv/tests/srv_config_unittest.cc b/src/lib/dhcpsrv/tests/srv_config_unittest.cc index 7f5a176fc5..cc24dadf25 100644 --- a/src/lib/dhcpsrv/tests/srv_config_unittest.cc +++ b/src/lib/dhcpsrv/tests/srv_config_unittest.cc @@ -1,4 +1,4 @@ -// 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()); }