From d9c77f8ad48f1add8b5ef16145998ca9b2375fd8 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Wed, 13 Jun 2018 14:56:39 +0200 Subject: [PATCH] [5549a] Addressed comments --- doc/examples/kea4/advanced.json | 27 -------- doc/examples/kea4/classify2.json | 28 ++++++++- doc/examples/kea6/advanced.json | 27 -------- doc/examples/kea6/classify2.json | 26 ++++++++ doc/guide/dhcp4-srv.xml | 2 +- doc/guide/dhcp6-srv.xml | 2 +- src/bin/dhcp4/dhcp4_srv.cc | 6 ++ src/bin/dhcp4/dhcp4_srv.h | 3 + src/bin/dhcp4/tests/dhcp4_srv_unittest.cc | 62 ++++++++++++++++++- src/bin/dhcp6/dhcp6_srv.cc | 6 ++ src/bin/dhcp6/dhcp6_srv.h | 3 + src/bin/dhcp6/tests/classify_unittests.cc | 4 +- .../parsers/client_class_def_parser.cc | 12 ++-- 13 files changed, 142 insertions(+), 66 deletions(-) diff --git a/doc/examples/kea4/advanced.json b/doc/examples/kea4/advanced.json index 6065a68d3e..608a154d2f 100644 --- a/doc/examples/kea4/advanced.json +++ b/doc/examples/kea4/advanced.json @@ -142,33 +142,6 @@ "relay": { "ip-address": "192.168.1.1" } - }, - { - // This subnet is divided in two pools for unknown and - // known (i.e. which have a reservation) clients. - // The built-in KNOWN and UNKNOWN classes are set or not - // at host reservation lookup (KNOWN if this returns something, - // UNKNOWN if this finds nothing) and client classes depending - // on it are evaluated. - // This happens after subnet selection and before address - // allocation from pools. - "pools": [ - { - "pool": "192.0.8.100 - 192.0.8.200", - "client-class": "UNKNOWN" - }, - { - "pool": "192.0.9.100 - 192.0.9.200", - "client-class": "KNOWN" - } - ], - "subnet": "192.0.8.0/23", - "reservations": [ - { "hw-address": "00:00:00:11:22:33", "hostname": "h1" }, - { "hw-address": "00:00:00:44:55:66", "hostname": "h4" }, - { "hw-address": "00:00:00:77:88:99", "hostname": "h7" }, - { "hw-address": "00:00:00:aa:bb:cc", "hostname": "ha" } - ] } ] }, diff --git a/doc/examples/kea4/classify2.json b/doc/examples/kea4/classify2.json index fca82e2ab8..f7cba5cc09 100644 --- a/doc/examples/kea4/classify2.json +++ b/doc/examples/kea4/classify2.json @@ -126,7 +126,33 @@ } ], "subnet": "192.0.4.0/23", "interface": "ethY" - } + }, +// This subnet is divided in two pools for unknown and known +// (i.e. which have a reservation) clients. The built-in KNOWN and +// UNKNOWN classes are set or not at host reservation lookup (KNOWN if +// this returns something, UNKNOWN if this finds nothing) and client +//classes depending on it are evaluated. +// This happens after subnet selection and before address allocation +//from pools. + { + "pools": [ + { + "pool": "192.0.8.100 - 192.0.8.200", + "client-class": "UNKNOWN" + }, + { + "pool": "192.0.9.100 - 192.0.9.200", + "client-class": "KNOWN" + } + ], + "subnet": "192.0.8.0/23", + "reservations": [ + { "hw-address": "00:00:00:11:22:33", "hostname": "h1" }, + { "hw-address": "00:00:00:44:55:66", "hostname": "h4" }, + { "hw-address": "00:00:00:77:88:99", "hostname": "h7" }, + { "hw-address": "00:00:00:aa:bb:cc", "hostname": "ha" } + ] + } ] }, diff --git a/doc/examples/kea6/advanced.json b/doc/examples/kea6/advanced.json index 9705c6e1a7..2d324b1a9d 100644 --- a/doc/examples/kea6/advanced.json +++ b/doc/examples/kea6/advanced.json @@ -139,33 +139,6 @@ "relay": { "ip-address": "3000::1" } - }, - { - // This subnet is divided in two pools for unknown and - // known (i.e. which have a reservation) clients. - // The built-in KNOWN and UNKNOWN classes are set or not - // at host reservation lookup (KNOWN if this returns something, - // UNKNOWN if this finds nothing) and client classes depending - // on it are evaluated. - // This happens after subnet selection and before address/prefix - // allocation from [pd]pools. - "pools": [ - { - "pool": "2001:db8:8::/64", - "client-class": "UNKNOWN" - }, - { - "pool": "2001:db8:9::/64", - "client-class": "KNOWN" - } - ], - "subnet": "2001:db8:8::/46", - "reservations": [ - { "hw-address": "00:00:00:11:22:33", "hostname": "h1" }, - { "hw-address": "00:00:00:44:55:66", "hostname": "h4" }, - { "hw-address": "00:00:00:77:88:99", "hostname": "h7" }, - { "hw-address": "00:00:00:aa:bb:cc", "hostname": "ha" } - ] } ] }, diff --git a/doc/examples/kea6/classify2.json b/doc/examples/kea6/classify2.json index f931ecbfd1..997dad3ad2 100644 --- a/doc/examples/kea6/classify2.json +++ b/doc/examples/kea6/classify2.json @@ -96,6 +96,32 @@ } ], "subnet": "2001:db8:4::/64", "interface": "ethY" + }, +// This subnet is divided in two pools for unknown and known +// (i.e. which have a reservation) clients. The built-in KNOWN and +// UNKNOWN classes are set or not at host reservation lookup (KNOWN if +// this returns something, UNKNOWN if this finds nothing) and client +//classes depending on it are evaluated. +// This happens after subnet selection and before address allocation +//from pools. + { + "pools": [ + { + "pool": "2001:db8:8::/64", + "client-class": "UNKNOWN" + }, + { + "pool": "2001:db8:9::/64", + "client-class": "KNOWN" + } + ], + "subnet": "2001:db8:8::/46", + "reservations": [ + { "hw-address": "00:00:00:11:22:33", "hostname": "h1" }, + { "hw-address": "00:00:00:44:55:66", "hostname": "h4" }, + { "hw-address": "00:00:00:77:88:99", "hostname": "h7" }, + { "hw-address": "00:00:00:aa:bb:cc", "hostname": "ha" } + ] } ] diff --git a/doc/guide/dhcp4-srv.xml b/doc/guide/dhcp4-srv.xml index 5b48a92a20..441c3d2278 100644 --- a/doc/guide/dhcp4-srv.xml +++ b/doc/guide/dhcp4-srv.xml @@ -2241,7 +2241,7 @@ It is merely echoed by the server In a similar way a pool can be constrained to serve only known clients, i.e. clients which have a reservation, using the - build-n "KNOWN" or "UNKNOWN" classes. One can assign addresses + built-in "KNOWN" or "UNKNOWN" classes. One can assign addresses to registered clients without giving a different address per reservations, for instance when there is not enough available addresses. The determination whether there is a reservation diff --git a/doc/guide/dhcp6-srv.xml b/doc/guide/dhcp6-srv.xml index 9279e76e33..b192d08775 100644 --- a/doc/guide/dhcp6-srv.xml +++ b/doc/guide/dhcp6-srv.xml @@ -2239,7 +2239,7 @@ should include options from the isc option space: In a similar way a pool can be constrained to serve only known clients, i.e. clients which have a reservation, using the - build-n "KNOWN" or "UNKNOWN" classes. One can assign addresses + built-in "KNOWN" or "UNKNOWN" classes. One can assign addresses to registered clients without giving a different address per reservations, for instance when there is not enough available addresses. The determination whether there is a reservation diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 0c67f0d603..6eb3458b15 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -166,8 +166,14 @@ Dhcpv4Exchange::Dhcpv4Exchange(const AllocEnginePtr& alloc_engine, // Set KNOWN builtin class if something was found, UNKNOWN if not. if (!context_->hosts_.empty()) { query->addClass("KNOWN"); + LOG_DEBUG(dhcp4_logger, DBG_DHCP4_BASIC, DHCP4_CLASS_ASSIGNED) + .arg(query->getLabel()) + .arg("KNOWN"); } else { query->addClass("UNKNOWN"); + LOG_DEBUG(dhcp4_logger, DBG_DHCP4_BASIC, DHCP4_CLASS_ASSIGNED) + .arg(query->getLabel()) + .arg("UNKNOWN"); } // Perform second pass of classification. diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index 1ea0e28b09..c095cdfef0 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/src/bin/dhcp4/dhcp4_srv.h @@ -856,6 +856,9 @@ public: /// /// @note Second part of the classification. /// + /// Evaluate expressions of client classes: if it returns true the class + /// is added to the incoming packet. + /// /// @param pkt packet to be classified. /// @param depend_on_known if false classes depending on the KNOWN or /// UNKNOWN classes are skipped, if true only these classes are evaluated. diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index b41561471c..03c12a32cf 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc @@ -2466,7 +2466,7 @@ TEST_F(Dhcpv4SrvTest, clientPoolClassify) { EXPECT_FALSE(offer->getYiaddr().isV4Zero()); } -// Checks if the [UN]KNOWN built-in classes is indeed used for pool selection. +// Checks if the KNOWN built-in classes is indeed used for pool selection. TEST_F(Dhcpv4SrvTest, clientPoolClassifyKnown) { IfaceMgrTestConfig test_config(true); IfaceMgr::instance().openSockets4(); @@ -2518,6 +2518,66 @@ TEST_F(Dhcpv4SrvTest, clientPoolClassifyKnown) { EXPECT_EQ("192.0.3.1", offer->getYiaddr().toText()); } +// Checks if the UNKNOWN built-in classes is indeed used for pool selection. +TEST_F(Dhcpv4SrvTest, clientPoolClassifyUnknown) { + IfaceMgrTestConfig test_config(true); + IfaceMgr::instance().openSockets4(); + + NakedDhcpv4Srv srv(0); + + // This test configures 2 pools. + // The first one requires no reservation, the second does the opposite. + string config = "{ \"interfaces-config\": {" + " \"interfaces\": [ \"*\" ]" + "}," + "\"rebind-timer\": 2000, " + "\"renew-timer\": 1000, " + "\"subnet4\": [ " + "{ \"pools\": [ { " + " \"pool\": \"192.0.2.1 - 192.0.2.100\", " + " \"client-class\": \"UNKNOWN\" }, " + " { \"pool\": \"192.0.3.1 - 192.0.3.100\", " + " \"client-class\": \"KNOWN\" } ], " + " \"subnet\": \"192.0.0.0/16\", " + " \"reservations\": [ { " + " \"hw-address\": \"00:00:00:11:22:33\", " + " \"hostname\": \"foo.bar\" } ] } " + "]," + "\"valid-lifetime\": 4000 }"; + + ConstElementPtr json; + ASSERT_NO_THROW(json = parseDHCP4(config, true)); + + ConstElementPtr status; + EXPECT_NO_THROW(status = configureDhcp4Server(srv, json)); + + CfgMgr::instance().commit(); + + // check if returned status is OK + ASSERT_TRUE(status); + comment_ = config::parseAnswer(rcode_, status); + ASSERT_EQ(0, rcode_); + + // Create a simple packet that we'll use for classification + Pkt4Ptr dis = Pkt4Ptr(new Pkt4(DHCPDISCOVER, 1234)); + dis->setRemoteAddr(IOAddress("192.0.2.1")); + dis->setCiaddr(IOAddress("192.0.2.1")); + dis->setIface("eth0"); + OptionPtr clientid = generateClientId(); + dis->addOption(clientid); + + // Set harware address / identifier + const HWAddr& hw = HWAddr::fromText("00:00:00:11:22:33"); + HWAddrPtr hw_addr(new HWAddr(hw)); + dis->setHWAddr(hw_addr); + + // First pool requires no reservation so the second will be used + Pkt4Ptr offer = srv.processDiscover(dis); + ASSERT_TRUE(offer); + EXPECT_EQ(DHCPOFFER, offer->getType()); + EXPECT_EQ("192.0.3.1", offer->getYiaddr().toText()); +} + // Verifies last resort option 43 is backward compatible TEST_F(Dhcpv4SrvTest, option43LastResort) { IfaceMgrTestConfig test_config(true); diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index 79f6f836e6..30ad693fe7 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -383,8 +383,14 @@ Dhcpv6Srv::initContext(const Pkt6Ptr& pkt, // Set KNOWN builtin class if something was found, UNKNOWN if not. if (!ctx.hosts_.empty()) { pkt->addClass("KNOWN"); + LOG_DEBUG(dhcp6_logger, DBG_DHCP6_BASIC, DHCP6_CLASS_ASSIGNED) + .arg(pkt->getLabel()) + .arg("KNOWN"); } else { pkt->addClass("UNKNOWN"); + LOG_DEBUG(dhcp6_logger, DBG_DHCP6_BASIC, DHCP6_CLASS_ASSIGNED) + .arg(pkt->getLabel()) + .arg("UNKNOWN"); } // Perform second pass of classification. diff --git a/src/bin/dhcp6/dhcp6_srv.h b/src/bin/dhcp6/dhcp6_srv.h index 9bd50cfbf0..7de5ab20af 100644 --- a/src/bin/dhcp6/dhcp6_srv.h +++ b/src/bin/dhcp6/dhcp6_srv.h @@ -678,6 +678,9 @@ protected: /// /// @note Second part of the classification. /// + /// Evaluate expressions of client classes: if it returns true the class + /// is added to the incoming packet. + /// /// @param pkt packet to be classified. /// @param depend_on_known if false classes depending on the KNOWN or /// UNKNOWN classes are skipped, if true only these classes are evaluated. diff --git a/src/bin/dhcp6/tests/classify_unittests.cc b/src/bin/dhcp6/tests/classify_unittests.cc index b8b2254d97..9620874853 100644 --- a/src/bin/dhcp6/tests/classify_unittests.cc +++ b/src/bin/dhcp6/tests/classify_unittests.cc @@ -1031,7 +1031,7 @@ TEST_F(ClassifyTest, clientClassifyPool) { " \"client-class\": \"xyzzy\" " " } " " ], " - " \"subnet\": \"2001:db8:2::/40\" " + " \"subnet\": \"2001:db8::/40\" " " } " "], " "\"valid-lifetime\": 4000 }"; @@ -1110,7 +1110,7 @@ TEST_F(ClassifyTest, clientClassifyPoolKnown) { " \"client-class\": \"UNKNOWN\" " " } " " ], " - " \"subnet\": \"2001:db8:2::/40\", " + " \"subnet\": \"2001:db8::/40\", " " \"reservations\": [ " " { \"duid\": \"01:02:03:04\", \"hostname\": \"foo\" } ] " " } " diff --git a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc index 5a78efce9f..3ac010b3de 100644 --- a/src/lib/dhcpsrv/parsers/client_class_def_parser.cc +++ b/src/lib/dhcpsrv/parsers/client_class_def_parser.cc @@ -77,12 +77,6 @@ ClientClassDefParser::parse(ClientClassDictionaryPtr& class_dictionary, << getPosition("name", class_def_cfg) << ")"); } - // Let's try to parse the only-if-required flag - bool required = false; - if (class_def_cfg->contains("only-if-required")) { - required = getBoolean(class_def_cfg, "only-if-required"); - } - // Parse matching expression ExpressionPtr match_expr; ConstElementPtr test_cfg = class_def_cfg->get("test"); @@ -148,6 +142,12 @@ ClientClassDefParser::parse(ClientClassDictionaryPtr& class_dictionary, // Parse user context ConstElementPtr user_context = class_def_cfg->get("user-context"); + // Let's try to parse the only-if-required flag + bool required = false; + if (class_def_cfg->contains("only-if-required")) { + required = getBoolean(class_def_cfg, "only-if-required"); + } + // Let's try to parse the next-server field IOAddress next_server("0.0.0.0"); if (class_def_cfg->contains("next-server")) { -- 2.47.3