From: Francis Dupont Date: Wed, 22 Jul 2026 18:08:35 +0000 (+0200) Subject: [#4143] Checkpoint: more UTs and doc todo X-Git-Tag: Kea-3.3.0~17 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=025390ce8571a098470d440ca1fc5361daa3bd85;p=thirdparty%2Fkea.git [#4143] Checkpoint: more UTs and doc todo --- diff --git a/src/hooks/dhcp/flex_option/flex_option.cc b/src/hooks/dhcp/flex_option/flex_option.cc index e1a1647583..b7cb445b28 100644 --- a/src/hooks/dhcp/flex_option/flex_option.cc +++ b/src/hooks/dhcp/flex_option/flex_option.cc @@ -320,12 +320,16 @@ FlexOptionImpl::parseOptionConfig(ConstElementPtr option) { // Not working as expected: the destination is the query and classes // are used. if (!opt_cfg->getDestination() && !opt_cfg->getClass().empty()) { - // LOG + LOG_WARN(flex_option_logger, FLEX_OPTION_CONFIG_USELESS_CLASS) + .arg(code) + .arg(opt_cfg->getClass()); } if (!opt_cfg->getDestination() && opt_cfg->getExpr()) { for (auto const& tok : *opt_cfg->getExpr()) { if (boost::dynamic_pointer_cast(tok)) { - // LOG + LOG_WARN(flex_option_logger, FLEX_OPTION_CONFIG_USELESS_MEMBER) + .arg(code) + .arg(opt_cfg->getText()); break; } } diff --git a/src/hooks/dhcp/flex_option/flex_option.h b/src/hooks/dhcp/flex_option/flex_option.h index f8d52ed4a6..5a95dce2d4 100644 --- a/src/hooks/dhcp/flex_option/flex_option.h +++ b/src/hooks/dhcp/flex_option/flex_option.h @@ -325,9 +325,7 @@ public: // Handle the case where the source is the response and // there is a TokenMember in an expression. if (query && response && need_copy_classes_to_response_) { - for (auto const& cclass : query->getClasses()) { - response->addClass(cclass); - } + response->copyClasses(*query); } for (auto const& pair : getOptionConfigMap()) { for (const OptionConfigPtr& opt_cfg : pair.second) { diff --git a/src/hooks/dhcp/flex_option/flex_option_messages.cc b/src/hooks/dhcp/flex_option/flex_option_messages.cc index e1bd676cec..0f5cc6a69b 100644 --- a/src/hooks/dhcp/flex_option/flex_option_messages.cc +++ b/src/hooks/dhcp/flex_option/flex_option_messages.cc @@ -4,6 +4,8 @@ #include #include +extern const isc::log::MessageID FLEX_OPTION_CONFIG_USELESS_CLASS = "FLEX_OPTION_CONFIG_USELESS_CLASS"; +extern const isc::log::MessageID FLEX_OPTION_CONFIG_USELESS_MEMBER = "FLEX_OPTION_CONFIG_USELESS_MEMBER"; extern const isc::log::MessageID FLEX_OPTION_LOAD_ERROR = "FLEX_OPTION_LOAD_ERROR"; extern const isc::log::MessageID FLEX_OPTION_PROCESS_ADD = "FLEX_OPTION_PROCESS_ADD"; extern const isc::log::MessageID FLEX_OPTION_PROCESS_CLIENT_CLASS = "FLEX_OPTION_PROCESS_CLIENT_CLASS"; @@ -20,6 +22,8 @@ extern const isc::log::MessageID FLEX_OPTION_UNLOAD = "FLEX_OPTION_UNLOAD"; namespace { const char* values[] = { + "FLEX_OPTION_CONFIG_USELESS_CLASS", "For the option '%1' the client class '%2' is required before classification for a 'query' destination", + "FLEX_OPTION_CONFIG_USELESS_MEMBER", "For the option '%1' the member expression '%2' is evaluated before classification for a 'query' destination", "FLEX_OPTION_LOAD_ERROR", "loading Flex Option hooks library failed: %1", "FLEX_OPTION_PROCESS_ADD", "Added the option code %1 with value %2", "FLEX_OPTION_PROCESS_CLIENT_CLASS", "Skip processing of the option code %1 for class '%2'", diff --git a/src/hooks/dhcp/flex_option/flex_option_messages.h b/src/hooks/dhcp/flex_option/flex_option_messages.h index 7ffe7d81be..67950a2cb3 100644 --- a/src/hooks/dhcp/flex_option/flex_option_messages.h +++ b/src/hooks/dhcp/flex_option/flex_option_messages.h @@ -5,6 +5,8 @@ #include +extern const isc::log::MessageID FLEX_OPTION_CONFIG_USELESS_CLASS; +extern const isc::log::MessageID FLEX_OPTION_CONFIG_USELESS_MEMBER; extern const isc::log::MessageID FLEX_OPTION_LOAD_ERROR; extern const isc::log::MessageID FLEX_OPTION_PROCESS_ADD; extern const isc::log::MessageID FLEX_OPTION_PROCESS_CLIENT_CLASS; diff --git a/src/hooks/dhcp/flex_option/flex_option_messages.mes b/src/hooks/dhcp/flex_option/flex_option_messages.mes index 0c83e1cbaa..18da2940d4 100644 --- a/src/hooks/dhcp/flex_option/flex_option_messages.mes +++ b/src/hooks/dhcp/flex_option/flex_option_messages.mes @@ -9,6 +9,19 @@ This error message indicates an error during loading the Flex Option hooks library. The details of the error are provided as argument of the log message. +% FLEX_OPTION_CONFIG_USELESS_CLASS For the option '%1' the client class '%2' is required before classification for a 'query' destination +This warning message indicates the config specifies a class requirement +when the destination is the query but the callout point for patching +queries is before the classification so it will very likely not work +as expected. The code of the option and the client class are displayed. + +% FLEX_OPTION_CONFIG_USELESS_MEMBER For the option '%1' the member expression '%2' is evaluated before classification for a 'query' destination +This warning message indicates the config specifies an expression +with a member clause when the destination is the query but the callout point +for patching queries is before the classification so it will very likely +not work as expected. The code of the option and the expression are +displayed. + % FLEX_OPTION_PROCESS_ADD Added the option code %1 with value %2 Logged at debug log level 40. This debug message is printed when an option was added into the response diff --git a/src/hooks/dhcp/flex_option/tests/flex_option_unittests.cc b/src/hooks/dhcp/flex_option/tests/flex_option_unittests.cc index 20a991c4f9..92d56f0f2e 100644 --- a/src/hooks/dhcp/flex_option/tests/flex_option_unittests.cc +++ b/src/hooks/dhcp/flex_option/tests/flex_option_unittests.cc @@ -889,6 +889,32 @@ TEST_F(FlexOptionTest, processEmpty) { EXPECT_EQ(response_txt, response->toText()); } +// Verify that response processing does nothing with no response. +TEST_F(FlexOptionTest, processNoResponse) { + ElementPtr options = Element::createList(); + ElementPtr option = Element::createMap(); + options->add(option); + ElementPtr code = Element::create(DHO_HOST_NAME); + option->set("code", code); + ElementPtr add = Element::create(string("'abc'")); + option->set("add", add); + + option = Element::createMap(); + options->add(option); + code = Element::create(DHO_DOMAIN_SEARCH); + option->set("code", code); + add = Element::create(string("'example.com'")); + option->set("add", add); + // fqdn option data is parsed using option definition in csv format. + option->set("csv-format", Element::create(true)); + + EXPECT_NO_THROW(impl_->testConfigure(options)); + EXPECT_TRUE(impl_->getErrMsg().empty()) << impl_->getErrMsg(); + + Pkt4Ptr query(new Pkt4(DHCPDISCOVER, 12345)); + EXPECT_NO_THROW(impl_->process(Option::V4, query, Pkt4Ptr())); +} + // Verify that NONE action really does nothing. TEST_F(FlexOptionTest, processNone) { CfgMgr::instance().setFamily(AF_INET6); @@ -1583,4 +1609,276 @@ TEST_F(FlexOptionTest, optionConfigGuardMatch) { EXPECT_FALSE(response->getOption(D6O_BOOTFILE_URL)); } +// Verify that an unknown source keyword is rejected. +TEST_F(FlexOptionTest, unknownSource) { + ElementPtr options = Element::createList(); + ElementPtr option = Element::createMap(); + options->add(option); + ElementPtr add = Element::create(string("'ab'")); + option->set("add", add); + ElementPtr code = Element::create(123); + option->set("code", code); + ElementPtr source = Element::create(string("foo")); + option->set("source", source); + EXPECT_THROW(impl_->testConfigure(options), BadValue); + string expected = "unknown source 'foo', "; + expected += "valid values are 'query' and 'response'"; + EXPECT_EQ(expected, impl_->getErrMsg()); +} + +// Verify that the default source is 'query'. +TEST_F(FlexOptionTest, defaultSource) { + ElementPtr options = Element::createList(); + ElementPtr option = Element::createMap(); + options->add(option); + ElementPtr add = Element::create(string("'abc'")); + option->set("add", add); + ElementPtr code = Element::create(DHO_HOST_NAME); + option->set("code", code); + EXPECT_NO_THROW(impl_->testConfigure(options)); + EXPECT_TRUE(impl_->getErrMsg().empty()) << impl_->getErrMsg(); + + auto map = impl_->getOptionConfigMap(); + FlexOptionImpl::OptionConfigList opt_lst; + ASSERT_NO_THROW(opt_lst = map.at(DHO_HOST_NAME)); + ASSERT_FALSE(opt_lst.empty()); + EXPECT_EQ(1U, opt_lst.size()); + FlexOptionImpl::OptionConfigPtr opt_cfg; + ASSERT_NO_THROW(opt_cfg = opt_lst.front()); + + ASSERT_TRUE(opt_cfg); + EXPECT_EQ(DHO_HOST_NAME, opt_cfg->getCode()); + EXPECT_EQ(FlexOptionImpl::ADD, opt_cfg->getAction()); + EXPECT_EQ("'abc'", opt_cfg->getText()); + EXPECT_TRUE(opt_cfg->getSource()); +} + +// Verify that the source can be set to 'query'. +TEST_F(FlexOptionTest, querySource) { + ElementPtr options = Element::createList(); + ElementPtr option = Element::createMap(); + options->add(option); + ElementPtr add = Element::create(string("'abc'")); + option->set("add", add); + ElementPtr code = Element::create(DHO_HOST_NAME); + option->set("code", code); + ElementPtr source = Element::create(string("query")); + option->set("source", source); + EXPECT_NO_THROW(impl_->testConfigure(options)); + EXPECT_TRUE(impl_->getErrMsg().empty()) << impl_->getErrMsg(); + + auto map = impl_->getOptionConfigMap(); + FlexOptionImpl::OptionConfigList opt_lst; + ASSERT_NO_THROW(opt_lst = map.at(DHO_HOST_NAME)); + ASSERT_FALSE(opt_lst.empty()); + EXPECT_EQ(1U, opt_lst.size()); + FlexOptionImpl::OptionConfigPtr opt_cfg; + ASSERT_NO_THROW(opt_cfg = opt_lst.front()); + + ASSERT_TRUE(opt_cfg); + EXPECT_EQ(DHO_HOST_NAME, opt_cfg->getCode()); + EXPECT_EQ(FlexOptionImpl::ADD, opt_cfg->getAction()); + EXPECT_EQ("'abc'", opt_cfg->getText()); + EXPECT_TRUE(opt_cfg->getSource()); +} + +// Verify that the source can be set to 'response'. +TEST_F(FlexOptionTest, responseSource) { + ElementPtr options = Element::createList(); + ElementPtr option = Element::createMap(); + options->add(option); + ElementPtr add = Element::create(string("'abc'")); + option->set("add", add); + ElementPtr code = Element::create(DHO_HOST_NAME); + option->set("code", code); + ElementPtr source = Element::create(string("response")); + option->set("source", source); + EXPECT_NO_THROW(impl_->testConfigure(options)); + EXPECT_TRUE(impl_->getErrMsg().empty()) << impl_->getErrMsg(); + + auto map = impl_->getOptionConfigMap(); + FlexOptionImpl::OptionConfigList opt_lst; + ASSERT_NO_THROW(opt_lst = map.at(DHO_HOST_NAME)); + ASSERT_FALSE(opt_lst.empty()); + EXPECT_EQ(1U, opt_lst.size()); + FlexOptionImpl::OptionConfigPtr opt_cfg; + ASSERT_NO_THROW(opt_cfg = opt_lst.front()); + + ASSERT_TRUE(opt_cfg); + EXPECT_EQ(DHO_HOST_NAME, opt_cfg->getCode()); + EXPECT_EQ(FlexOptionImpl::ADD, opt_cfg->getAction()); + EXPECT_EQ("'abc'", opt_cfg->getText()); + EXPECT_FALSE(opt_cfg->getSource()); +} + +// Verify that an unknown destination keyword is rejected. +TEST_F(FlexOptionTest, unknownDestination) { + ElementPtr options = Element::createList(); + ElementPtr option = Element::createMap(); + options->add(option); + ElementPtr add = Element::create(string("'ab'")); + option->set("add", add); + ElementPtr code = Element::create(123); + option->set("code", code); + ElementPtr dest = Element::create(string("foo")); + option->set("destination", dest); + EXPECT_THROW(impl_->testConfigure(options), BadValue); + string expected = "unknown destination 'foo', "; + expected += "valid values are 'response' and 'query'"; + EXPECT_EQ(expected, impl_->getErrMsg()); +} + +// Verify that the default destination is 'response'. +TEST_F(FlexOptionTest, defaultDestination) { + ElementPtr options = Element::createList(); + ElementPtr option = Element::createMap(); + options->add(option); + ElementPtr add = Element::create(string("'abc'")); + option->set("add", add); + ElementPtr code = Element::create(DHO_HOST_NAME); + option->set("code", code); + EXPECT_NO_THROW(impl_->testConfigure(options)); + EXPECT_TRUE(impl_->getErrMsg().empty()) << impl_->getErrMsg(); + + auto map = impl_->getOptionConfigMap(); + FlexOptionImpl::OptionConfigList opt_lst; + ASSERT_NO_THROW(opt_lst = map.at(DHO_HOST_NAME)); + ASSERT_FALSE(opt_lst.empty()); + EXPECT_EQ(1U, opt_lst.size()); + FlexOptionImpl::OptionConfigPtr opt_cfg; + ASSERT_NO_THROW(opt_cfg = opt_lst.front()); + + ASSERT_TRUE(opt_cfg); + EXPECT_EQ(DHO_HOST_NAME, opt_cfg->getCode()); + EXPECT_EQ(FlexOptionImpl::ADD, opt_cfg->getAction()); + EXPECT_EQ("'abc'", opt_cfg->getText()); + EXPECT_TRUE(opt_cfg->getDestination()); +} + +// Verify that the destination can be set to 'response'. +TEST_F(FlexOptionTest, responseDestination) { + ElementPtr options = Element::createList(); + ElementPtr option = Element::createMap(); + options->add(option); + ElementPtr add = Element::create(string("'abc'")); + option->set("add", add); + ElementPtr code = Element::create(DHO_HOST_NAME); + option->set("code", code); + ElementPtr dest = Element::create(string("response")); + option->set("destination", dest); + EXPECT_NO_THROW(impl_->testConfigure(options)); + EXPECT_TRUE(impl_->getErrMsg().empty()) << impl_->getErrMsg(); + + auto map = impl_->getOptionConfigMap(); + FlexOptionImpl::OptionConfigList opt_lst; + ASSERT_NO_THROW(opt_lst = map.at(DHO_HOST_NAME)); + ASSERT_FALSE(opt_lst.empty()); + EXPECT_EQ(1U, opt_lst.size()); + FlexOptionImpl::OptionConfigPtr opt_cfg; + ASSERT_NO_THROW(opt_cfg = opt_lst.front()); + + ASSERT_TRUE(opt_cfg); + EXPECT_EQ(DHO_HOST_NAME, opt_cfg->getCode()); + EXPECT_EQ(FlexOptionImpl::ADD, opt_cfg->getAction()); + EXPECT_EQ("'abc'", opt_cfg->getText()); + EXPECT_TRUE(opt_cfg->getDestination()); +} + +// Verify that the destination can be set to 'query'. +TEST_F(FlexOptionTest, queryDestination) { + ElementPtr options = Element::createList(); + ElementPtr option = Element::createMap(); + options->add(option); + ElementPtr add = Element::create(string("'abc'")); + option->set("add", add); + ElementPtr code = Element::create(DHO_HOST_NAME); + option->set("code", code); + ElementPtr dest = Element::create(string("query")); + option->set("destination", dest); + EXPECT_NO_THROW(impl_->testConfigure(options)); + EXPECT_TRUE(impl_->getErrMsg().empty()) << impl_->getErrMsg(); + + auto map = impl_->getOptionConfigMap(); + FlexOptionImpl::OptionConfigList opt_lst; + ASSERT_NO_THROW(opt_lst = map.at(DHO_HOST_NAME)); + ASSERT_FALSE(opt_lst.empty()); + EXPECT_EQ(1U, opt_lst.size()); + FlexOptionImpl::OptionConfigPtr opt_cfg; + ASSERT_NO_THROW(opt_cfg = opt_lst.front()); + + ASSERT_TRUE(opt_cfg); + EXPECT_EQ(DHO_HOST_NAME, opt_cfg->getCode()); + EXPECT_EQ(FlexOptionImpl::ADD, opt_cfg->getAction()); + EXPECT_EQ("'abc'", opt_cfg->getText()); + EXPECT_FALSE(opt_cfg->getDestination()); +} + +// Verify that destination query and source response combo is rejected. +TEST_F(FlexOptionTest, badSourceDestination) { + ElementPtr options = Element::createList(); + ElementPtr option = Element::createMap(); + options->add(option); + ElementPtr add = Element::create(string("'ab'")); + option->set("add", add); + ElementPtr code = Element::create(123); + option->set("code", code); + ElementPtr source = Element::create(string("response")); + option->set("source", source); + ElementPtr dest = Element::create(string("query")); + option->set("destination", dest); + EXPECT_THROW(impl_->testConfigure(options), BadValue); + string expected = "destination 'query' requires source 'query'"; + EXPECT_EQ(expected, impl_->getErrMsg()); +} + +// Verify that the query can be processed. +TEST_F(FlexOptionTest, processQuery) { + ElementPtr options = Element::createList(); + ElementPtr option = Element::createMap(); + options->add(option); + ElementPtr code = Element::create(DHO_HOST_NAME); + option->set("code", code); + ElementPtr add = Element::create(string("'abc'")); + option->set("add", add); + ElementPtr dest = Element::create(string("query")); + option->set("destination", dest); + + option = Element::createMap(); + options->add(option); + code = Element::create(DHO_DOMAIN_SEARCH); + option->set("code", code); + add = Element::create(string("'example.com'")); + option->set("add", add); + // fqdn option data is parsed using option definition in csv format. + option->set("csv-format", Element::create(true)); + option->set("destination", dest); + + EXPECT_NO_THROW(impl_->testConfigure(options)); + EXPECT_TRUE(impl_->getErrMsg().empty()) << impl_->getErrMsg(); + + Pkt4Ptr query(new Pkt4(DHCPDISCOVER, 12345)); + EXPECT_FALSE(query->getOption(DHO_HOST_NAME)); + EXPECT_FALSE(query->getOption(DHO_DOMAIN_SEARCH)); + + EXPECT_NO_THROW(impl_->process(Option::V4, query, Pkt4Ptr())); + + OptionPtr opt = query->getOption(DHO_HOST_NAME); + ASSERT_TRUE(opt); + EXPECT_EQ(DHO_HOST_NAME, opt->getType()); + const OptionBuffer& buffer = opt->getData(); + ASSERT_EQ(3U, buffer.size()); + EXPECT_EQ(0, memcmp(&buffer[0], "abc", 3)); + + opt = query->getOption(DHO_DOMAIN_SEARCH); + ASSERT_TRUE(opt); + EXPECT_EQ(DHO_DOMAIN_SEARCH, opt->getType()); + const OptionBuffer& buffer_fqdn = opt->getData(); + ASSERT_EQ(13U, buffer_fqdn.size()); + EXPECT_EQ(7U, buffer_fqdn[0]); + EXPECT_EQ(0, memcmp(&buffer_fqdn[1], "example", 7)); + EXPECT_EQ(3U, buffer_fqdn[8]); + EXPECT_EQ(0, memcmp(&buffer_fqdn[9], "com", 3)); + EXPECT_EQ(0U, buffer_fqdn[12]); +} + } // end of anonymous namespace diff --git a/src/lib/dhcp/pkt.cc b/src/lib/dhcp/pkt.cc index ec22ea5894..87d1d06deb 100644 --- a/src/lib/dhcp/pkt.cc +++ b/src/lib/dhcp/pkt.cc @@ -150,6 +150,15 @@ Pkt::addSubClass(const ClientClass& class_def, const ClientClass& subclass) { } } +void +Pkt::copyClasses(const Pkt& other) { + for (auto const& cclass : other.classes_) { + if (!classes_.contains(cclass)) { + classes_.insert(cclass); + } + } +} + void Pkt::updateTimestamp() { timestamp_ = boost::posix_time::microsec_clock::universal_time(); diff --git a/src/lib/dhcp/pkt.h b/src/lib/dhcp/pkt.h index ad690d6461..5182591bda 100644 --- a/src/lib/dhcp/pkt.h +++ b/src/lib/dhcp/pkt.h @@ -404,6 +404,12 @@ public: return (subclasses_); } + /// @brief Simple copy of classes from another packet. + /// + /// @note To be used by the flex option hook. + /// @param other the other packet. + void copyClasses(const Pkt& other); + /// @brief Unparsed data (in received packets). /// /// @warning This public member is accessed by derived