From: Razvan Becheriu Date: Fri, 25 Sep 2020 15:14:50 +0000 (+0300) Subject: [#1373] addressed review X-Git-Tag: Kea-1.9.0~13 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=646fceea05e994525b758efec613b7e84b6e19bb;p=thirdparty%2Fkea.git [#1373] addressed review --- diff --git a/src/hooks/dhcp/flex_option/flex_option.cc b/src/hooks/dhcp/flex_option/flex_option.cc index 83dcf34f56..0ba0d4be54 100644 --- a/src/hooks/dhcp/flex_option/flex_option.cc +++ b/src/hooks/dhcp/flex_option/flex_option.cc @@ -74,8 +74,9 @@ parseAction(ConstElementPtr option, namespace isc { namespace flex_option { -FlexOptionImpl::OptionConfig::OptionConfig(uint16_t code) - : code_(code), action_(NONE), csv_format_(false) { +FlexOptionImpl::OptionConfig::OptionConfig(uint16_t code, + OptionDefinitionPtr def) + : code_(code), def_(def), action_(NONE), csv_format_(false) { } FlexOptionImpl::OptionConfig::~OptionConfig() { @@ -116,10 +117,20 @@ FlexOptionImpl::parseOptionConfig(ConstElementPtr option) { ConstElementPtr code_elem = option->get("code"); ConstElementPtr name_elem = option->get("name"); ConstElementPtr csv_format_elem = option->get("csv-format"); + OptionDefinitionPtr def; if (!code_elem && !name_elem) { isc_throw(BadValue, "'code' or 'name' must be specified: " << option->str()); } + string space; + Option::Universe universe; + if (family == AF_INET) { + space = DHCP4_OPTION_SPACE; + universe = Option::V4; + } else { + space = DHCP6_OPTION_SPACE; + universe = Option::V6; + } uint16_t code; if (code_elem) { if (code_elem->getType() != Element::integer) { @@ -151,6 +162,17 @@ FlexOptionImpl::parseOptionConfig(ConstElementPtr option) { } } code = static_cast(value); + def = isc::dhcp::LibDHCP::getOptionDef(space, code); + if (!def) { + def = isc::dhcp::LibDHCP::getRuntimeOptionDef(space, code); + } + if (!def) { + def = isc::dhcp::LibDHCP::getLastResortOptionDef(space, code); + } + if (!def) { + isc_throw(BadValue, "no known option with code '" << code + << "' in '" << space << "' space"); + } } if (name_elem) { if (name_elem->getType() != Element::string) { @@ -161,13 +183,7 @@ FlexOptionImpl::parseOptionConfig(ConstElementPtr option) { if (name.empty()) { isc_throw(BadValue, "'name' must not be empty"); } - string space; - if (family == AF_INET) { - space = DHCP4_OPTION_SPACE; - } else { - space = DHCP6_OPTION_SPACE; - } - OptionDefinitionPtr def = LibDHCP::getOptionDef(space, name); + def = LibDHCP::getOptionDef(space, name); if (!def) { def = LibDHCP::getRuntimeOptionDef(space, name); } @@ -199,14 +215,7 @@ FlexOptionImpl::parseOptionConfig(ConstElementPtr option) { csv_format = csv_format_elem->boolValue(); } - Option::Universe universe; - if (family == AF_INET) { - universe = Option::V4; - } else { - universe = Option::V6; - } - - OptionConfigPtr opt_cfg(new OptionConfig(code)); + OptionConfigPtr opt_cfg(new OptionConfig(code, def)); if (csv_format_elem) { opt_cfg->setCSVFormat(csv_format); diff --git a/src/hooks/dhcp/flex_option/flex_option.dox b/src/hooks/dhcp/flex_option/flex_option.dox index 90c2db886b..249316f551 100644 --- a/src/hooks/dhcp/flex_option/flex_option.dox +++ b/src/hooks/dhcp/flex_option/flex_option.dox @@ -94,7 +94,9 @@ The sole parameter is a options list of options with: already exists in the response packet, the option is removed from the response. Only one action can be specified. - @b csv-format - Specifies the option format used for input. If not - specified, it will default to false (raw data). + specified, it will default to false (raw data). When enabled, the option + data will be parsed using csv format and packed according to the option + definition. Note for the rare options which can be empty this mechanism does not work. The proposed solution in this case is to use a client class to set the diff --git a/src/hooks/dhcp/flex_option/flex_option.h b/src/hooks/dhcp/flex_option/flex_option.h index dcfe6a36f6..fedfbbc1e8 100644 --- a/src/hooks/dhcp/flex_option/flex_option.h +++ b/src/hooks/dhcp/flex_option/flex_option.h @@ -55,7 +55,7 @@ public: /// @brief Constructor. /// /// @param code the option code. - OptionConfig(uint16_t code); + OptionConfig(uint16_t code, isc::dhcp::OptionDefinitionPtr def); /// @brief Destructor. virtual ~OptionConfig(); @@ -67,6 +67,13 @@ public: return (code_); } + /// @brief Return option definition. + /// + /// @return option definition. + isc::dhcp::OptionDefinitionPtr getOptionDef() const { + return (def_); + } + /// @brief Set action. /// /// @param action the action. @@ -127,6 +134,9 @@ public: /// @brief The code. uint16_t code_; + /// @brief The option definition. + isc::dhcp::OptionDefinitionPtr def_; + /// @brief The action. Action action_; @@ -174,15 +184,12 @@ public: template void process(isc::dhcp::Option::Universe universe, PktType query, PktType response) { - std::string space = (universe == isc::dhcp::Option::V4 ? - DHCP4_OPTION_SPACE : DHCP6_OPTION_SPACE); - for (auto pair : getOptionConfigMap()) { const OptionConfigPtr& opt_cfg = pair.second; std::string value; isc::dhcp::OptionBuffer buffer; isc::dhcp::OptionPtr opt = response->getOption(opt_cfg->getCode()); - isc::dhcp::OptionDefinitionPtr def; + isc::dhcp::OptionDefinitionPtr def = opt_cfg->getOptionDef(); switch (opt_cfg->getAction()) { case NONE: break; @@ -196,18 +203,8 @@ public: if (value.empty()) { break; } - + // Check for cvs format. if (opt_cfg->getCSVFormat()) { - def = isc::dhcp::LibDHCP::getOptionDef(space, opt_cfg->getCode()); - - if (!def) { - def = isc::dhcp::LibDHCP::getRuntimeOptionDef(space, opt_cfg->getCode()); - } - - if (!def) { - def = isc::dhcp::LibDHCP::getLastResortOptionDef(space, opt_cfg->getCode()); - } - if (!def) { buffer.assign(value.begin(), value.end()); opt.reset(new isc::dhcp::Option(universe, opt_cfg->getCode(), @@ -223,7 +220,6 @@ public: opt.reset(new isc::dhcp::Option(universe, opt_cfg->getCode(), buffer)); } - // Add the option. response->addOption(opt); logAction(ADD, opt_cfg->getCode(), value); @@ -239,18 +235,8 @@ public: response->delOption(opt_cfg->getCode()); opt = response->getOption(opt_cfg->getCode()); } - + // Check for cvs format. if (opt_cfg->getCSVFormat()) { - def = isc::dhcp::LibDHCP::getOptionDef(space, opt_cfg->getCode()); - - if (!def) { - def = isc::dhcp::LibDHCP::getRuntimeOptionDef(space, opt_cfg->getCode()); - } - - if (!def) { - def = isc::dhcp::LibDHCP::getLastResortOptionDef(space, opt_cfg->getCode()); - } - if (!def) { buffer.assign(value.begin(), value.end()); opt.reset(new isc::dhcp::Option(universe, opt_cfg->getCode(), @@ -266,7 +252,6 @@ public: opt.reset(new isc::dhcp::Option(universe, opt_cfg->getCode(), buffer)); } - // Add the option. response->addOption(opt); logAction(SUPERSEDE, opt_cfg->getCode(), value); 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 368682c8b2..715fe711da 100644 --- a/src/hooks/dhcp/flex_option/tests/flex_option_unittests.cc +++ b/src/hooks/dhcp/flex_option/tests/flex_option_unittests.cc @@ -170,7 +170,7 @@ TEST_F(FlexOptionTest, optionConfigBadCode4) { EXPECT_NO_THROW(impl_->testConfigure(options)); EXPECT_TRUE(impl_->getErrMsg().empty()); - code = Element::create(254); + code = Element::create(2); option->set("code", code); EXPECT_NO_THROW(impl_->testConfigure(options)); EXPECT_TRUE(impl_->getErrMsg().empty()); @@ -210,7 +210,7 @@ TEST_F(FlexOptionTest, optionConfigBadCode6) { EXPECT_NO_THROW(impl_->testConfigure(options)); EXPECT_TRUE(impl_->getErrMsg().empty()); - code = Element::create(65535); + code = Element::create(2); option->set("code", code); EXPECT_NO_THROW(impl_->testConfigure(options)); EXPECT_TRUE(impl_->getErrMsg().empty()); @@ -250,11 +250,24 @@ TEST_F(FlexOptionTest, optionConfigUnknownName) { ElementPtr add = Element::create(string("'ab'")); option->set("add", add); ElementPtr name = Element::create(string("foobar")); - option->set("name",name); + option->set("name", name); EXPECT_THROW(impl_->testConfigure(options), BadValue); EXPECT_EQ("no known 'foobar' option in 'dhcp4' space", impl_->getErrMsg()); } +// Verify that the code must be a known option. +TEST_F(FlexOptionTest, optionConfigUnknownCode) { + 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(109); + option->set("code", code); + EXPECT_THROW(impl_->testConfigure(options), BadValue); + EXPECT_EQ("no known option with code '109' in 'dhcp4' space", impl_->getErrMsg()); +} + // Verify that the name can be a standard option. TEST_F(FlexOptionTest, optionConfigStandardName) { ElementPtr options = Element::createList(); @@ -751,8 +764,9 @@ TEST_F(FlexOptionTest, processEmpty) { TEST_F(FlexOptionTest, processNone) { CfgMgr::instance().setFamily(AF_INET6); + OptionDefinitionPtr def = LibDHCP::getOptionDef(DHCP6_OPTION_SPACE, D6O_BOOTFILE_URL); FlexOptionImpl::OptionConfigPtr - opt_cfg(new FlexOptionImpl::OptionConfig(D6O_BOOTFILE_URL)); + opt_cfg(new FlexOptionImpl::OptionConfig(D6O_BOOTFILE_URL, def)); EXPECT_EQ(FlexOptionImpl::NONE, opt_cfg->getAction()); auto map = impl_->getMutableOptionConfigMap(); map[DHO_HOST_NAME] = opt_cfg; @@ -769,7 +783,6 @@ TEST_F(FlexOptionTest, processNone) { // Verify that ADD action adds the specified option in csv format. TEST_F(FlexOptionTest, processAddEnableCSVFormat) { ElementPtr options = Element::createList(); - ElementPtr csv_format = Element::create(true); ElementPtr option = Element::createMap(); options->add(option); ElementPtr code = Element::create(DHO_HOST_NAME); @@ -783,7 +796,8 @@ TEST_F(FlexOptionTest, processAddEnableCSVFormat) { option->set("code", code); add = Element::create(string("'example.com'")); option->set("add", add); - option->set("csv-format", csv_format); + // 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()); @@ -817,7 +831,6 @@ TEST_F(FlexOptionTest, processAddEnableCSVFormat) { // Verify that ADD action adds the specified option in raw format. TEST_F(FlexOptionTest, processAddDisableCSVFormat) { ElementPtr options = Element::createList(); - ElementPtr csv_format = Element::create(false); ElementPtr option = Element::createMap(); options->add(option); ElementPtr code = Element::create(DHO_HOST_NAME); @@ -831,7 +844,8 @@ TEST_F(FlexOptionTest, processAddDisableCSVFormat) { option->set("code", code); add = Element::create(string("0x076578616d706c6503636f6d00")); option->set("add", add); - option->set("csv-format", csv_format); + // fqdn option data is specified in raw format. + option->set("csv-format", Element::create(false)); EXPECT_NO_THROW(impl_->testConfigure(options)); EXPECT_TRUE(impl_->getErrMsg().empty()); @@ -915,7 +929,6 @@ TEST_F(FlexOptionTest, processAddEmpty) { // Verify that SUPERSEDE action supersedes the specified option in csv format. TEST_F(FlexOptionTest, processSupersedeEnableCSVFormat) { ElementPtr options = Element::createList(); - ElementPtr csv_format = Element::create(true); ElementPtr option = Element::createMap(); options->add(option); ElementPtr code = Element::create(DHO_HOST_NAME); @@ -929,7 +942,8 @@ TEST_F(FlexOptionTest, processSupersedeEnableCSVFormat) { option->set("code", code); supersede = Element::create(string("'example.com'")); option->set("supersede", supersede); - option->set("csv-format", csv_format); + // 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()); @@ -963,7 +977,6 @@ TEST_F(FlexOptionTest, processSupersedeEnableCSVFormat) { // Verify that SUPERSEDE action supersedes the specified option in raw format. TEST_F(FlexOptionTest, processSupersedeDisableCSVFormat) { ElementPtr options = Element::createList(); - ElementPtr csv_format = Element::create(false); ElementPtr option = Element::createMap(); options->add(option); ElementPtr code = Element::create(DHO_HOST_NAME); @@ -977,7 +990,8 @@ TEST_F(FlexOptionTest, processSupersedeDisableCSVFormat) { option->set("code", code); supersede = Element::create(string("0x076578616d706c6503636f6d00")); option->set("supersede", supersede); - option->set("csv-format", csv_format); + // fqdn option data is specified in raw format. + option->set("csv-format", Element::create(false)); EXPECT_NO_THROW(impl_->testConfigure(options)); EXPECT_TRUE(impl_->getErrMsg().empty()); @@ -1013,7 +1027,6 @@ TEST_F(FlexOptionTest, processSupersedeExisting) { CfgMgr::instance().setFamily(AF_INET6); ElementPtr options = Element::createList(); - ElementPtr csv_format = Element::create(true); ElementPtr option = Element::createMap(); options->add(option); ElementPtr code = Element::create(D6O_BOOTFILE_URL); @@ -1027,7 +1040,8 @@ TEST_F(FlexOptionTest, processSupersedeExisting) { option->set("code", code); supersede = Element::create(string("'example.com'")); option->set("supersede", supersede); - option->set("csv-format", csv_format); + // 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()); @@ -1205,7 +1219,6 @@ TEST_F(FlexOptionTest, processFullTest) { // Verify that complex strings with escaped characters are properly parsed on add. TEST_F(FlexOptionTest, processFullAddWithComplexString) { ElementPtr options = Element::createList(); - ElementPtr csv_format = Element::create(true); ElementPtr option = Element::createMap(); options->add(option); ElementPtr code = Element::create(D6O_NEW_POSIX_TIMEZONE); @@ -1213,7 +1226,8 @@ TEST_F(FlexOptionTest, processFullAddWithComplexString) { string expr = "ifelse(option[39].exists,'EST5EDT4\\,M3.2.0/02:00\\,M11.1.0/02:00','')"; ElementPtr add = Element::create(expr); option->set("add", add); - option->set("csv-format", csv_format); + // strings with escape characters are parsed in csv format. + option->set("csv-format", Element::create(true)); EXPECT_NO_THROW(impl_->testConfigure(options)); EXPECT_TRUE(impl_->getErrMsg().empty()); @@ -1238,7 +1252,6 @@ TEST_F(FlexOptionTest, processFullAddWithComplexString) { // Verify that complex strings with escaped characters are properly parsed on supersede. TEST_F(FlexOptionTest, processFullSupersedeWithComplexString) { ElementPtr options = Element::createList(); - ElementPtr csv_format = Element::create(true); ElementPtr option = Element::createMap(); options->add(option); ElementPtr code = Element::create(D6O_NEW_POSIX_TIMEZONE); @@ -1246,7 +1259,8 @@ TEST_F(FlexOptionTest, processFullSupersedeWithComplexString) { string expr = "ifelse(option[39].exists,'EST5EDT4\\,M3.2.0/02:00\\,M11.1.0/02:00','')"; ElementPtr supersede = Element::create(expr); option->set("supersede", supersede); - option->set("csv-format", csv_format); + // strings with escape characters are parsed in csv format. + option->set("csv-format", Element::create(true)); EXPECT_NO_THROW(impl_->testConfigure(options)); EXPECT_TRUE(impl_->getErrMsg().empty());