Skip to content

Commit 64c15b1

Browse files
tmarkwaldertmarkwalder
committed
[#3583] Addressred review comments
modified: doc/sphinx/arm/classify.rst src/bin/dhcp4/dhcp4_srv.cc src/bin/dhcp4/tests/config_parser_unittest.cc src/bin/dhcp6/tests/config_parser_unittest.cc src/hooks/dhcp/mysql/mysql_cb_dhcp4.cc src/lib/dhcp/classify.cc src/lib/dhcp/classify.h src/lib/dhcp/tests/classify_unittest.cc src/lib/dhcpsrv/cfg_option.cc src/lib/dhcpsrv/cfg_option.h src/lib/dhcpsrv/parsers/option_data_parser.cc src/lib/dhcpsrv/parsers/simple_parser4.cc src/lib/dhcpsrv/parsers/simple_parser6.cc src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc src/lib/dhcpsrv/testutils/generic_backend_unittest.cc
1 parent c109b65 commit 64c15b1

15 files changed

+139
-28
lines changed

‎doc/sphinx/arm/classify.rst

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1246,6 +1246,12 @@ the same value for option "foo":
12461246
}]
12471247
}
12481248

1249+
The ``client-classes`` list is allowed in an option specification at
1250+
any scope. Option class-tagging is enforced at the time options are
1251+
being added to the response which occurs after lease assignment just
1252+
before the response is to be sent to the client.
1253+
1254+
12491255
When ``never-send`` for an option is true at any scope, all
12501256
``client-classes`` entries for that option are ignored. The
12511257
option will not included.

‎src/bin/dhcp4/dhcp4_srv.cc

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2342,6 +2342,7 @@ Dhcpv4Srv::appendRequestedVendorOptions(Dhcpv4Exchange& ex) {
23422342
}
23432343
}
23442344

2345+
const auto& cclasses = query->getClasses();
23452346
for (uint32_t vendor_id : vendor_ids) {
23462347

23472348
std::set<uint8_t> cancelled_opts;
@@ -2406,7 +2407,7 @@ Dhcpv4Srv::appendRequestedVendorOptions(Dhcpv4Exchange& ex) {
24062407
if (!vendor_rsp->getOption(opt)) {
24072408
for (auto const& copts : co_list) {
24082409
OptionDescriptor desc = copts->get(vendor_id, opt);
2409-
if (desc.option_ && desc.allowedForClientClasses(query->getClasses())) {
2410+
if (desc.option_ && desc.allowedForClientClasses(cclasses)) {
24102411
vendor_rsp->addOption(desc.option_);
24112412
added = true;
24122413
break;
@@ -2446,6 +2447,7 @@ Dhcpv4Srv::appendBasicOptions(Dhcpv4Exchange& ex) {
24462447
}
24472448

24482449
Pkt4Ptr resp = ex.getResponse();
2450+
const auto& cclasses = ex.getQuery()->getClasses();
24492451

24502452
// Try to find all 'required' options in the outgoing
24512453
// message. Those that are not present will be added.
@@ -2456,7 +2458,7 @@ Dhcpv4Srv::appendBasicOptions(Dhcpv4Exchange& ex) {
24562458
for (auto const& copts : co_list) {
24572459
OptionDescriptor desc = copts->get(DHCP4_OPTION_SPACE, required);
24582460
/// @todo TKM - not sure if otion class-tagging should be allowed here?
2459-
if (desc.option_ && desc.allowedForClientClasses(ex.getQuery()->getClasses())) {
2461+
if (desc.option_ && desc.allowedForClientClasses(cclasses)) {
24602462
resp->addOption(desc.option_);
24612463
break;
24622464
}
@@ -3635,7 +3637,7 @@ Dhcpv4Srv::setFixedFields(Dhcpv4Exchange& ex) {
36353637

36363638
// Step 2: Try to set the values based on classes.
36373639
// Any values defined in classes will override those from subnet level.
3638-
const ClientClasses classes = query->getClasses();
3640+
const ClientClasses& classes = query->getClasses();
36393641
if (!classes.empty()) {
36403642

36413643
// Let's get class definitions

‎src/bin/dhcp4/tests/config_parser_unittest.cc

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8013,4 +8013,38 @@ TEST_F(Dhcp4ParserTest, storeDdnsConflictResolutionMode) {
80138013
}
80148014
}
80158015

8016+
//This test verifies that duplicates in option-data.client-classes
8017+
// are ignored and do not affect class order.
8018+
TEST_F(Dhcp4ParserTest, optionClientClassesDuplicateCheck) {
8019+
std::string config = "{ " + genIfaceConfig() + ","
8020+
R"^(
8021+
"option-data": [{
8022+
"name": "domain-name",
8023+
"data": "example.com",
8024+
"client-classes": [ "foo", "bar", "foo", "bar" ]
8025+
}],
8026+
"rebind-timer": 2000,
8027+
"renew-timer": 1000,
8028+
"subnet4": [],
8029+
"valid-lifetime": 400
8030+
})^";
8031+
8032+
ConstElementPtr json;
8033+
ASSERT_NO_THROW(json = parseDHCP4(config));
8034+
extractConfig(config);
8035+
8036+
ConstElementPtr status;
8037+
ASSERT_NO_THROW(status = configureDhcp4Server(*srv_, json));
8038+
checkResult(status, 0);
8039+
8040+
CfgOptionPtr cfg = CfgMgr::instance().getStagingCfg()->getCfgOption();
8041+
const auto desc = cfg->get(DHCP4_OPTION_SPACE, DHO_DOMAIN_NAME);
8042+
ASSERT_TRUE(desc.option_);
8043+
ASSERT_EQ(desc.client_classes_.size(), 2);
8044+
auto cclasses = desc.client_classes_.begin();
8045+
EXPECT_EQ(*cclasses, "foo");
8046+
++cclasses;
8047+
EXPECT_EQ(*cclasses, "bar");
8048+
}
8049+
80168050
} // namespace

‎src/bin/dhcp6/tests/config_parser_unittest.cc

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8987,4 +8987,38 @@ TEST_F(Dhcp6ParserTest, storeDdnsConflictResolutionMode) {
89878987
}
89888988
}
89898989

8990+
// This test verifies that duplicates in option-data.client-classes
8991+
// are ignored and do not affect class order.
8992+
TEST_F(Dhcp6ParserTest, optionClientClassesDuplicateCheck) {
8993+
std::string config = "{ " + genIfaceConfig() + ","
8994+
R"^(
8995+
"option-data": [{
8996+
"name": "domain-search",
8997+
"data": "example.com",
8998+
"client-classes": [ "foo", "bar", "foo", "bar" ]
8999+
}],
9000+
"rebind-timer": 2000,
9001+
"renew-timer": 1000,
9002+
"subnet6": [],
9003+
"valid-lifetime": 400
9004+
})^";
9005+
9006+
ConstElementPtr json;
9007+
ASSERT_NO_THROW(json = parseDHCP6(config));
9008+
extractConfig(config);
9009+
9010+
ConstElementPtr status;
9011+
ASSERT_NO_THROW(status = configureDhcp6Server(srv_, json));
9012+
checkResult(status, 0);
9013+
9014+
CfgOptionPtr cfg = CfgMgr::instance().getStagingCfg()->getCfgOption();
9015+
const auto desc = cfg->get(DHCP6_OPTION_SPACE, D6O_DOMAIN_SEARCH);
9016+
ASSERT_TRUE(desc.option_);
9017+
ASSERT_EQ(desc.client_classes_.size(), 2);
9018+
auto cclasses = desc.client_classes_.begin();
9019+
EXPECT_EQ(*cclasses, "foo");
9020+
++cclasses;
9021+
EXPECT_EQ(*cclasses, "bar");
9022+
}
9023+
89909024
} // namespace

‎src/hooks/dhcp/mysql/mysql_cb_dhcp4.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1870,7 +1870,7 @@ class MySqlConfigBackendDHCPv4Impl : public MySqlConfigBackendImpl {
18701870
cc_binding,
18711871
MySqlBinding::createString(tag),
18721872
MySqlBinding::createInteger<uint8_t>(option->option_->getType()),
1873-
MySqlBinding::condCreateString(option->space_name_),
1873+
MySqlBinding::condCreateString(option->space_name_)
18741874
};
18751875

18761876
MySqlTransaction transaction(conn_);
@@ -2074,7 +2074,7 @@ class MySqlConfigBackendDHCPv4Impl : public MySqlConfigBackendImpl {
20742074
cc_binding,
20752075
MySqlBinding::createString(shared_network_name),
20762076
MySqlBinding::createInteger<uint8_t>(option->option_->getType()),
2077-
MySqlBinding::condCreateString(option->space_name_),
2077+
MySqlBinding::condCreateString(option->space_name_)
20782078
};
20792079

20802080
boost::scoped_ptr<MySqlTransaction> transaction;

‎src/lib/dhcp/classify.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ ClientClasses::toElement() const {
8282
}
8383

8484
void
85-
ClientClasses::fromElement(isc::data::ElementPtr cc_list) {
85+
ClientClasses::fromElement(isc::data::ConstElementPtr cc_list) {
8686
if (cc_list) {
8787
clear();
8888
if (cc_list->getType() != Element::list) {
@@ -95,7 +95,7 @@ ClientClasses::fromElement(isc::data::ElementPtr cc_list) {
9595
isc_throw(BadValue, "elements of list must be valid strings");
9696
}
9797

98-
insert(cclass->stringValue());
98+
static_cast<void>(insert(cclass->stringValue()));
9999
}
100100
}
101101
}

‎src/lib/dhcp/classify.h

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -124,18 +124,18 @@ namespace dhcp {
124124

125125
/// @brief Copy constructor.
126126
///
127-
/// @param ClientClasses object to be copied.
127+
/// @param other ClientClasses object to be copied.
128128
ClientClasses(const ClientClasses& other);
129129

130-
/// @brief Assigns the contents of on container to another
130+
/// @brief Assigns the contents of on container to another.
131131
ClientClasses& operator=(const ClientClasses& other);
132132

133133
/// @brief Compares two ClientClasses container for equality
134134
///
135135
/// @return True if the two containers are equal, false otherwise.
136136
bool equals(const ClientClasses& other) const;
137137

138-
/// @brief Compares two ClientClasses container for equality
138+
/// @brief Compares two ClientClasses containers for equality.
139139
///
140140
/// @return True if the two containers are equal, false otherwise.
141141
bool operator==(const ClientClasses& other) const {
@@ -227,9 +227,12 @@ namespace dhcp {
227227

228228
/// @brief Sets contents from a ListElement
229229
///
230+
/// @param list JSON list of classes from which to populate
231+
/// the container.
232+
///
230233
/// @throw BadValue if the element is not a list or contents
231234
/// are invalid
232-
void fromElement(isc::data::ElementPtr list);
235+
void fromElement(isc::data::ConstElementPtr list);
233236

234237
private:
235238
/// @brief container part

‎src/lib/dhcp/tests/classify_unittest.cc

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -253,4 +253,16 @@ TEST(ClassifyTest, ClientClassesFromElement) {
253253
cclasses_element = Element::createList();
254254
ASSERT_NO_THROW(classes.fromElement(cclasses_element));
255255
EXPECT_TRUE(classes.empty());
256+
257+
cclasses_element->add(Element::create("foo"));
258+
cclasses_element->add(Element::create("bar"));
259+
cclasses_element->add(Element::create("foo"));
260+
cclasses_element->add(Element::create("bar"));
261+
262+
ASSERT_NO_THROW(classes.fromElement(cclasses_element));
263+
ASSERT_EQ(classes.size(), 2);
264+
auto cclass = classes.begin();
265+
EXPECT_EQ(*cclass, "foo");
266+
++cclass;
267+
EXPECT_EQ(*cclass, "bar");
256268
}

‎src/lib/dhcpsrv/cfg_option.cc

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -64,17 +64,25 @@ OptionDescriptor::allowedForClientClasses(const ClientClasses& cclasses) const {
6464
if (client_classes_.empty()) {
6565
return (true);
6666
}
67-
68-
for (const auto& cclass : client_classes_) {
69-
if (cclasses.contains(cclass)) {
70-
return (true);
67+
68+
if (cclasses.size() > client_classes_.size()) {
69+
for (const auto& cclass : client_classes_) {
70+
if (cclasses.contains(cclass)) {
71+
return (true);
72+
}
73+
}
74+
}
75+
else {
76+
for (const auto& cclass : cclasses) {
77+
if (client_classes_.contains(cclass)) {
78+
return (true);
79+
}
7180
}
7281
}
7382

7483
return (false);
7584
}
7685

77-
7886
CfgOption::CfgOption()
7987
: encapsulated_(false) {
8088
}
@@ -108,7 +116,7 @@ CfgOption::add(const OptionDescriptor& desc, const std::string& option_space) {
108116
if (!desc.option_) {
109117
isc_throw(isc::BadValue, "option being configured must not be NULL");
110118

111-
} else if (!OptionSpace::validateName(option_space)) {
119+
} else if (!OptionSpace::validateName(option_space)) {
112120
isc_throw(isc::BadValue, "invalid option space name: '"
113121
<< option_space << "'");
114122
}

‎src/lib/dhcpsrv/cfg_option.h

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -817,8 +817,6 @@ class CfgOption : public isc::data::CfgToElement {
817817
VendorOptionSpaceCollection vendor_options_;
818818
};
819819

820-
class CfgOption; // forward declaration
821-
822820
/// @name Pointers to the @c CfgOption objects.
823821
//@{
824822

‎src/lib/dhcpsrv/parsers/option_data_parser.cc

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -441,11 +441,15 @@ OptionDataParser::createOption(ConstElementPtr option_data) {
441441
desc.setContext(user_context);
442442
}
443443

444+
#if 1
445+
desc.client_classes_.fromElement(client_classes);
446+
#else
444447
if (client_classes) {
445448
for (auto const& class_element : client_classes->listValue()) {
446449
desc.addClientClass(class_element->stringValue());
447450
}
448451
}
452+
#endif
449453

450454
// All went good, so we can set the option space name.
451455
return (make_pair(desc, space_param));

‎src/lib/dhcpsrv/parsers/simple_parser4.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -190,8 +190,8 @@ const SimpleKeywords SimpleParser4::OPTION4_PARAMETERS = {
190190
{ "never-send", Element::boolean },
191191
{ "user-context", Element::map },
192192
{ "comment", Element::string },
193-
{ "metadata", Element::map },
194-
{ "client-classes", Element::list }
193+
{ "client-classes", Element::list },
194+
{ "metadata", Element::map }
195195
};
196196

197197
/// @brief This table defines default values for options in DHCPv4.

‎src/lib/dhcpsrv/parsers/simple_parser6.cc

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -184,8 +184,8 @@ const SimpleKeywords SimpleParser6::OPTION6_PARAMETERS = {
184184
{ "never-send", Element::boolean },
185185
{ "user-context", Element::map },
186186
{ "comment", Element::string },
187-
{ "metadata", Element::map },
188-
{ "client-classes", Element::list }
187+
{ "client-classes", Element::list },
188+
{ "metadata", Element::map }
189189
};
190190

191191
/// @brief This table defines default values for options in DHCPv6.

‎src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -462,11 +462,12 @@ class ParseConfigTest : public ::testing::Test {
462462
return (option_ptr);
463463
}
464464

465-
/// @brief Find the OptionDescriptor for a given space and code within the parser
466-
/// context.
465+
/// @brief Find the OptionDescriptor for a given space and code within
466+
/// the parser context.
467+
///
467468
/// @param space is the space name of the desired option.
468469
/// @param code is the numeric "type" of the desired option.
469-
/// @return an OptionDecriptorPtr to the descriptor found or an empty ptr
470+
/// @return an OptionDecriptorPtr to the descriptor found or an empty ptr.
470471
OptionDescriptorPtr getOptionDescriptor(std::string space, uint32_t code) {
471472
OptionDescriptorPtr od_ptr;
472473
const auto &cfg_options = CfgMgr::instance().getStagingCfg()->getCfgOption();
@@ -1974,7 +1975,7 @@ TEST_F(ParseConfigTest, optionDataClientClassesEmpty4) {
19741975
ASSERT_TRUE(od);
19751976
EXPECT_TRUE(od->client_classes_.empty());
19761977

1977-
// We skip unparse test because client-classes is only emitited if not empty.
1978+
// We skip unparse test because client-classes is only emitted if not empty.
19781979
}
19791980

19801981
/// @brief Check parsing of a v6 option with a client-class list.
@@ -2053,7 +2054,7 @@ TEST_F(ParseConfigTest, optionDataClientClassesEmpty6) {
20532054
ASSERT_TRUE(od);
20542055
EXPECT_TRUE(od->client_classes_.empty());
20552056

2056-
// We skip unparse test because client-classes is only emitited if not empty.
2057+
// We skip unparse test because client-classes is only emitted if not empty.
20572058
}
20582059

20592060
// hooks-libraries element that does not contain anything.

‎src/lib/dhcpsrv/testutils/generic_backend_unittest.cc

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,15 @@ GenericBackendTest::testOptionsEquivalent(const OptionDescriptor& ref_option,
109109
EXPECT_EQ(ref_option.persistent_, tested_option.persistent_);
110110
EXPECT_EQ(ref_option.cancelled_, tested_option.cancelled_);
111111
EXPECT_EQ(ref_option.space_name_, tested_option.space_name_);
112+
EXPECT_EQ(ref_option.client_classes_, tested_option.client_classes_);
113+
auto ref_ctx = ref_option.getContext();
114+
auto test_ctx = tested_option.getContext();
115+
if (ref_ctx) {
116+
ASSERT_TRUE(test_ctx);
117+
EXPECT_EQ(*test_ctx, *ref_ctx);
118+
} else {
119+
EXPECT_FALSE(test_ctx);
120+
}
112121
}
113122

114123
void

0 commit comments

Comments
 (0)