diff --git a/ChangeLog b/ChangeLog index 79672b695584cc7eebc968621741ffdf46acfc56..d4f019391b06179bfa7853015ccbcdf554d1c2de 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,9 @@ +1943. [bug] marcin + Fixed a bug in fetching client classes from the Config Backend. + The bug resulted in failures during attempts to evaluate the + classes for a received packet. + (Gitlab #2077) + 1942. [func] fdupont Added basic statistics to the DHCP-DDNS server. (Gitlab #2040) diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc index cf29082b31b7a36944214b7e9d7bbfd9ff370a03..54b10b2f869913cfc1724a4cb9ef570e5d41e4c7 100644 --- a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc +++ b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc @@ -3047,7 +3047,7 @@ public: // KNOWN/UNKNOWN built-ins. The callback always returns true to avoid // reporting the parsing error. The dependency check is performed later // at the database level. - parser.parse(expression, Element::create(client_class->getTest()), AF_INET, + parser.parse(expression, Element::create(client_class->getTest()), AF_INET6, [&dependencies, &depend_on_known](const ClientClass& client_class) -> bool { if (isClientClassBuiltIn(client_class)) { if ((client_class == "KNOWN") || (client_class == "UNKNOWN")) { diff --git a/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc b/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc index 2e23ed8395267bd40464546cf3b8af70e2cb9dca..6685659be1eb554be757c7fb3f0778600d2e02f7 100644 --- a/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc +++ b/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc @@ -4169,6 +4169,7 @@ TEST_F(MySqlConfigBackendDHCPv4Test, setAndGetAllClientClasses4) { } // Create first class. auto class1 = test_client_classes_[0]; + class1->setTest("pkt4.msgtype == 1"); ASSERT_NO_THROW(cbptr_->createUpdateClientClass4(ServerSelector::ALL(), class1, "")); { SCOPED_TRACE("client class foo is created"); diff --git a/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc b/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc index 2fc311446a9f444480d24b3733323ccb9a71adb2..30e3824781ea129cd1a8ce788f080257cc2eb009 100644 --- a/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc +++ b/src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc @@ -4340,6 +4340,7 @@ TEST_F(MySqlConfigBackendDHCPv6Test, setAndGetAllClientClasses6) { } // Create first class. auto class1 = test_client_classes_[0]; + class1->setTest("pkt6.msgtype == 1"); ASSERT_NO_THROW_LOG(cbptr_->createUpdateClientClass6(ServerSelector::ALL(), class1, "")); { SCOPED_TRACE("client class foo is created"); diff --git a/src/lib/dhcpsrv/cb_ctl_dhcp4.cc b/src/lib/dhcpsrv/cb_ctl_dhcp4.cc index 6b3c36773e7aa1fc5779bb9dbc0e84ad0aa09c3c..3c53d0522331863061cb5a0b973dd061fcd6a333 100644 --- a/src/lib/dhcpsrv/cb_ctl_dhcp4.cc +++ b/src/lib/dhcpsrv/cb_ctl_dhcp4.cc @@ -203,6 +203,9 @@ CBControlDHCPv4::databaseConfigApply(const BackendSelector& backend_selector, if (audit_entries.empty() || !updated_entries.empty()) { ClientClassDictionary client_classes = getMgr().getPool()->getAllClientClasses4(backend_selector, server_selector); + // Match expressions are not initialized for classes returned from the config backend. + // We have to ensure to initialize them before they can be used by the server. + client_classes.initMatchExpr(AF_INET); external_cfg->setClientClassDictionary(boost::make_shared(client_classes)); } diff --git a/src/lib/dhcpsrv/cb_ctl_dhcp6.cc b/src/lib/dhcpsrv/cb_ctl_dhcp6.cc index d2096045f32824540fab41b33c421c7c0f0ca6df..2713db2201f3609d662138291c45fe6472188125 100644 --- a/src/lib/dhcpsrv/cb_ctl_dhcp6.cc +++ b/src/lib/dhcpsrv/cb_ctl_dhcp6.cc @@ -202,6 +202,9 @@ CBControlDHCPv6::databaseConfigApply(const db::BackendSelector& backend_selector if (audit_entries.empty() || !updated_entries.empty()) { ClientClassDictionary client_classes = getMgr().getPool()->getAllClientClasses6(backend_selector, server_selector); + // Match expressions are not initialized for classes returned from the config backend. + // We have to ensure to initialize them before they can be used by the server. + client_classes.initMatchExpr(AF_INET6); external_cfg->setClientClassDictionary(boost::make_shared(client_classes)); } diff --git a/src/lib/dhcpsrv/client_class_def.cc b/src/lib/dhcpsrv/client_class_def.cc index 6a3b3c10cd093ebc97435cdb929384d4acf1d3e8..cbc2fede573606adc3e770dfae6c22fb72a226dc 100644 --- a/src/lib/dhcpsrv/client_class_def.cc +++ b/src/lib/dhcpsrv/client_class_def.cc @@ -9,8 +9,11 @@ #include #include #include +#include #include +#include + using namespace isc::data; namespace isc { @@ -384,6 +387,25 @@ ClientClassDictionary::equals(const ClientClassDictionary& other) const { return (true); } +void +ClientClassDictionary::initMatchExpr(uint16_t family) { + std::queue expressions; + for (auto c : *list_) { + ExpressionPtr match_expr = boost::make_shared(); + if (!c->getTest().empty()) { + ExpressionParser parser; + parser.parse(match_expr, Element::create(c->getTest()), family); + } + expressions.push(match_expr); + } + // All expressions successfully initialized. Let's set them for the + // client classes in the dictionary. + for (auto c : *list_) { + c->setMatchExpr(expressions.front()); + expressions.pop(); + } +} + ElementPtr ClientClassDictionary::toElement() const { ElementPtr result = Element::createList(); diff --git a/src/lib/dhcpsrv/client_class_def.h b/src/lib/dhcpsrv/client_class_def.h index 89542b02947c2ea33371966860b03c1532fc0e0f..012f3e9c39c4f878bb1614ad6f70782193237d49 100644 --- a/src/lib/dhcpsrv/client_class_def.h +++ b/src/lib/dhcpsrv/client_class_def.h @@ -397,6 +397,12 @@ public: /// @return true if descriptors equal, false otherwise. bool equals(const ClientClassDictionary& other) const; + /// @brief Iterates over the classes in the dictionary and ensures that + /// that match expressions are initialized. + /// + /// @param family Class universe, e.g. AF_INET or AF_INET6. + void initMatchExpr(uint16_t family); + /// @brief Equality operator. /// /// @param other Other client class dictionary to compare to. diff --git a/src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc b/src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc index 8bd10b8082de8b9795663769c3419e93ed3205f7..e6318b1336adfb904a481ed87a829e5df365bbfd 100644 --- a/src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc +++ b/src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc @@ -382,6 +382,7 @@ public: // Insert client classes into the database. auto expression = boost::make_shared(); ClientClassDefPtr client_class = boost::make_shared("first-class", expression); + client_class->setTest("substring(option[1].hex, 0, 8) == 'my-value'"); client_class->setId(1); client_class->setModificationTime(getTimestamp("dhcp4_client_class")); mgr.getPool()->createUpdateClientClass4(BackendSelector::UNSPEC(), ServerSelector::ALL(), @@ -592,6 +593,8 @@ public: if (hasConfigElement("dhcp4_client_class") && (getTimestamp("dhcp4_client_class") > lb_modification_time)) { ASSERT_TRUE(found_class); + ASSERT_TRUE(found_class->getMatchExpr()); + EXPECT_GT(found_class->getMatchExpr()->size(), 0); EXPECT_EQ("first-class", found_class->getName()); } else { @@ -1175,6 +1178,7 @@ public: // Insert client classes into the database. auto expression = boost::make_shared(); ClientClassDefPtr client_class = boost::make_shared("first-class", expression); + client_class->setTest("substring(option[1].hex, 0, 8) == 'my-value'"); client_class->setId(1); client_class->setModificationTime(getTimestamp("dhcp6_client_class")); mgr.getPool()->createUpdateClientClass6(BackendSelector::UNSPEC(), ServerSelector::ALL(), @@ -1385,6 +1389,8 @@ public: if (hasConfigElement("dhcp6_client_class") && (getTimestamp("dhcp6_client_class") > lb_modification_time)) { ASSERT_TRUE(found_class); + ASSERT_TRUE(found_class->getMatchExpr()); + EXPECT_GT(found_class->getMatchExpr()->size(), 0); EXPECT_EQ("first-class", found_class->getName()); } else { diff --git a/src/lib/dhcpsrv/tests/client_class_def_unittest.cc b/src/lib/dhcpsrv/tests/client_class_def_unittest.cc index 5532dd9050335fd62c78666d2ca4d3c47c3f6a47..d6ac1d5aa1dba525b6f2cfc704a7a4864d4eb009 100644 --- a/src/lib/dhcpsrv/tests/client_class_def_unittest.cc +++ b/src/lib/dhcpsrv/tests/client_class_def_unittest.cc @@ -542,6 +542,62 @@ TEST(ClientClassDictionary, dependency) { EXPECT_EQ("cc4", depend); } +// Tests that match expressions are set for all client classes in the +// dictionary. +TEST(ClientClassDictionary, initMatchExpr) { + ClientClassDictionaryPtr dictionary(new ClientClassDictionary()); + ExpressionPtr expr; + CfgOptionPtr cfg_option; + + // Add several classes. + ASSERT_NO_THROW(dictionary->addClass("foo", expr, "", false, + false, cfg_option)); + ASSERT_NO_THROW(dictionary->addClass("bar", expr, "member('KNOWN') or member('foo')", false, + false, cfg_option)); + ASSERT_NO_THROW(dictionary->addClass("baz", expr, "substring(option[61].hex,0,3) == 'foo'", false, + false, cfg_option)); + + // Create match expressions for all of them. + ASSERT_NO_THROW(dictionary->initMatchExpr(AF_INET)); + + // Ensure that the expressions were created. + auto classes = *(dictionary->getClasses()); + EXPECT_TRUE(classes[0]->getMatchExpr()); + EXPECT_EQ(0, classes[0]->getMatchExpr()->size()); + + EXPECT_TRUE(classes[1]->getMatchExpr()); + EXPECT_EQ(3, classes[1]->getMatchExpr()->size()); + + EXPECT_TRUE(classes[2]->getMatchExpr()); + EXPECT_EQ(6, classes[2]->getMatchExpr()->size()); +} + +// Tests that an error is returned when any of the test expressions is +// invalid, and that no expressions are initialized if there is an error +// for a single expression. +TEST(ClientClassDictionary, initMatchExprError) { + ClientClassDictionaryPtr dictionary(new ClientClassDictionary()); + ExpressionPtr expr; + CfgOptionPtr cfg_option; + + // Add several classes. One of them has invalid test expression. + ASSERT_NO_THROW(dictionary->addClass("foo", expr, "member('KNOWN')", false, + false, cfg_option)); + ASSERT_NO_THROW(dictionary->addClass("bar", expr, "wrong expression", false, + false, cfg_option)); + ASSERT_NO_THROW(dictionary->addClass("baz", expr, "substring(option[61].hex,0,3) == 'foo'", false, + false, cfg_option)); + + // An attempt to initialize match expressions should fail because the + // test expression for the second class is invalid. + ASSERT_THROW(dictionary->initMatchExpr(AF_INET), std::exception); + + // Ensure that no classes have their match expressions modified. + for (auto c : (*dictionary->getClasses())) { + EXPECT_FALSE(c->getMatchExpr()); + } +} + // Tests the default constructor regarding fixed fields TEST(ClientClassDef, fixedFieldsDefaults) { boost::scoped_ptr cclass;