From: Thomas Markwalder Date: Tue, 25 Jan 2022 11:31:37 +0000 (-0500) Subject: [#2246] Additional review comments X-Git-Tag: Kea-2.1.2~14 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=92e167c312f1ff875d1634cee9def059d6889e4d;p=thirdparty%2Fkea.git [#2246] Additional review comments src/lib/dhcpsrv/client_class_def.cc ClientClassDictionary::createOptions() - modified to merge the client and external option definitions together prior to creating class options. src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc Tests for a custom client option defined and specified in a class. --- diff --git a/src/lib/dhcpsrv/client_class_def.cc b/src/lib/dhcpsrv/client_class_def.cc index 0d090f7d24..49ff2d2da3 100644 --- a/src/lib/dhcpsrv/client_class_def.cc +++ b/src/lib/dhcpsrv/client_class_def.cc @@ -407,9 +407,42 @@ ClientClassDictionary::initMatchExpr(uint16_t family) { } void -ClientClassDictionary::createOptions(const CfgOptionDefPtr& cfg_option_def) { +ClientClassDictionary::createOptions(const CfgOptionDefPtr& external_defs) { for (auto c : *list_) { - c->getCfgOption()->createOptions(cfg_option_def); + // If the class has no options, skip it. + CfgOptionPtr class_options = c->getCfgOption(); + if (!class_options || class_options->empty()) { + continue; + } + + // If the class has no option definitions, use the set + // of definitions we were given as is to create its + // options. + if (!c->getCfgOptionDef()) { + class_options->createOptions(external_defs); + } else { + // Class has its own option definitions, we need a + // composite set of definitions to recreate its options. + // We make copies of both sets of definitions, then merge + // the external defs copy into the class defs copy. + // We do this because merging actually effects both sets + // of definitions and we cannot alter either set. + // Seed the composite set with the class's definitions. + CfgOptionDefPtr composite_defs(new CfgOptionDef()); + c->getCfgOptionDef()->copyTo(*composite_defs); + + // Make a copy of the external definitions and + // merge those into the composite set. This should give + // us a set of options with class definitions taking + // precedence. + CfgOptionDefPtr external_defs_copy(new CfgOptionDef()); + external_defs->copyTo(*external_defs_copy); + composite_defs->merge(*external_defs_copy); + + // Now create the class options using the composite + // set of definitions. + class_options->createOptions(composite_defs); + } } } diff --git a/src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc b/src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc index 531c9ae845..43a2f79c62 100644 --- a/src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc +++ b/src/lib/dhcpsrv/tests/cb_ctl_dhcp_unittest.cc @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -386,13 +387,28 @@ public: client_class->setId(1); client_class->setModificationTime(getTimestamp("dhcp4_client_class")); - // Add an option to the class. + // Add a standard option to the class. OptionPtr option = Option::create(Option::V4, DHO_BOOT_FILE_NAME); OptionDescriptorPtr desc = OptionDescriptor::create(option, true, "bogus-file.txt"); desc->space_name_ = DHCP4_OPTION_SPACE; desc->setModificationTime(getTimestamp("dhcp4_client_class")); client_class->getCfgOption()->add(*desc, desc->space_name_); + // Add a custom option definition to the class. + CfgOptionDefPtr cc_cfg_option_def(new CfgOptionDef()); + def.reset(new OptionDefinition("v4str", 201, "isc", "string")); + def->setId(201); + def->setModificationTime(getTimestamp("dhcp4_client_class")); + cc_cfg_option_def->add(def); + client_class->setCfgOptionDef(cc_cfg_option_def); + + // Add a custom option to the class. + option = Option::create(Option::V4, 201); + desc = OptionDescriptor::create(option, true, "custom-stuff"); + desc->space_name_ = "isc"; + desc->setModificationTime(getTimestamp("dhcp4_client_class")); + client_class->getCfgOption()->add(*desc, desc->space_name_); + mgr.getPool()->createUpdateClientClass4(BackendSelector::UNSPEC(), ServerSelector::ALL(), client_class, ""); @@ -523,8 +539,8 @@ public: ASSERT_FALSE(audit_entries_.empty()) << "Require at least one audit entry. The test is broken!"; - ctl_.databaseConfigApply(BackendSelector::UNSPEC(), ServerSelector::ALL(), - lb_modification_time, audit_entries_); + ASSERT_NO_THROW_LOG(ctl_.databaseConfigApply(BackendSelector::UNSPEC(), ServerSelector::ALL(), + lb_modification_time, audit_entries_)); // The updates should have been merged into current configuration. auto srv_cfg = CfgMgr::instance().getCurrentCfg(); @@ -605,7 +621,7 @@ public: EXPECT_GT(found_class->getMatchExpr()->size(), 0); EXPECT_EQ("first-class", found_class->getName()); - // Check for class option, make sure it has been "created". + // Check for the standard class option, make sure it has been "created". auto cfg_option_desc = found_class->getCfgOption(); ASSERT_TRUE(cfg_option_desc); auto option_desc = cfg_option_desc->get(DHCP4_OPTION_SPACE, DHO_BOOT_FILE_NAME); @@ -614,6 +630,14 @@ public: EXPECT_EQ(OptionBuffer(option_desc.formatted_value_.begin(), option_desc.formatted_value_.end()), option->getData()); + + // Check for the custom class option, make sure it has been "created". + option_desc = cfg_option_desc->get("isc", 201); + option = option_desc.option_; + ASSERT_TRUE(option); + EXPECT_EQ(OptionBuffer(option_desc.formatted_value_.begin(), + option_desc.formatted_value_.end()), + option->getData()); } else { EXPECT_FALSE(found_class); }