From 733a34a843d449bc190ce4d4bef505a80afc76ca Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Sat, 4 Sep 2021 17:49:29 +0200 Subject: [PATCH 1/5] [#2077] Init match expressions Match expressions are now initialized for client classes fetched from the configuration backend. --- src/lib/dhcpsrv/cb_ctl_dhcp4.cc | 3 + src/lib/dhcpsrv/cb_ctl_dhcp6.cc | 3 + src/lib/dhcpsrv/client_class_def.cc | 22 ++++++++ src/lib/dhcpsrv/client_class_def.h | 6 ++ src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc | 6 ++ .../tests/client_class_def_unittest.cc | 56 +++++++++++++++++++ 6 files changed, 96 insertions(+) diff --git a/src/lib/dhcpsrv/cb_ctl_dhcp4.cc b/src/lib/dhcpsrv/cb_ctl_dhcp4.cc index 6b3c36773e..3c53d05223 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 d2096045f3..2713db2201 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 6a3b3c10cd..8666f9432a 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 initialied. 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 89542b0294..012f3e9c39 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 8bd10b8082..e6318b1336 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 5532dd9050..b93c7198ea 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 expresion. +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; -- GitLab From 595355e46c50eb809dced10b06e1d9cb27208fd2 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Sat, 4 Sep 2021 18:55:33 +0200 Subject: [PATCH 2/5] [#2077] Added ChangeLog for issue 2077 --- ChangeLog | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/ChangeLog b/ChangeLog index 79672b6955..d4f019391b 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) -- GitLab From 86c4b3218508eae1eb39fae9f89a4b957a906ecb Mon Sep 17 00:00:00 2001 From: Andrei Pavel Date: Thu, 16 Sep 2021 13:59:20 +0000 Subject: [PATCH 3/5] [#2077] Applied typo fix --- src/lib/dhcpsrv/client_class_def.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/dhcpsrv/client_class_def.cc b/src/lib/dhcpsrv/client_class_def.cc index 8666f9432a..cbc2fede57 100644 --- a/src/lib/dhcpsrv/client_class_def.cc +++ b/src/lib/dhcpsrv/client_class_def.cc @@ -398,7 +398,7 @@ ClientClassDictionary::initMatchExpr(uint16_t family) { } expressions.push(match_expr); } - // All expressions successfully initialied. Let's set them for the + // All expressions successfully initialized. Let's set them for the // client classes in the dictionary. for (auto c : *list_) { c->setMatchExpr(expressions.front()); -- GitLab From 8cf39b37a10093b221a98f04fcf6df3b411dcbda Mon Sep 17 00:00:00 2001 From: Andrei Pavel Date: Thu, 16 Sep 2021 14:00:03 +0000 Subject: [PATCH 4/5] [#2077] Applied typo fix --- src/lib/dhcpsrv/tests/client_class_def_unittest.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/dhcpsrv/tests/client_class_def_unittest.cc b/src/lib/dhcpsrv/tests/client_class_def_unittest.cc index b93c7198ea..d6ac1d5aa1 100644 --- a/src/lib/dhcpsrv/tests/client_class_def_unittest.cc +++ b/src/lib/dhcpsrv/tests/client_class_def_unittest.cc @@ -574,7 +574,7 @@ TEST(ClientClassDictionary, initMatchExpr) { // 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 expresion. +// for a single expression. TEST(ClientClassDictionary, initMatchExprError) { ClientClassDictionaryPtr dictionary(new ClientClassDictionary()); ExpressionPtr expr; -- GitLab From 5d04c7553d175e8e85745d11543c2edbe7fb4f16 Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Thu, 16 Sep 2021 17:41:02 +0200 Subject: [PATCH 5/5] [#2077] Corrected an issue in MySQL CB (v6) DHCPv6 client classes failed to evaluate during insertion to the database. --- src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc | 2 +- src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp4_unittest.cc | 1 + src/hooks/dhcp/mysql_cb/tests/mysql_cb_dhcp6_unittest.cc | 1 + 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc b/src/hooks/dhcp/mysql_cb/mysql_cb_dhcp6.cc index cf29082b31..54b10b2f86 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 2e23ed8395..6685659be1 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 2fc311446a..30e3824781 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"); -- GitLab