From 64338c35f3a0e16c1cc696799e868309e57cd2f0 Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Sat, 19 Sep 2015 20:08:56 +0200 Subject: [PATCH 1/2] [4057] Implemented RAI Link Selection --- src/bin/dhcp4/dhcp4_srv.cc | 15 +++ src/bin/dhcp4/tests/dhcp4_srv_unittest.cc | 100 ++++++++++++++++++ src/lib/dhcpsrv/cfg_subnets4.cc | 6 ++ src/lib/dhcpsrv/cfg_subnets4.h | 3 + src/lib/dhcpsrv/subnet_selector.h | 5 +- src/lib/dhcpsrv/tests/cfg_option_unittest.cc | 4 +- .../dhcpsrv/tests/cfg_subnets4_unittest.cc | 45 +++++++- .../dhcpsrv/tests/cfg_subnets6_unittest.cc | 2 +- src/lib/dhcpsrv/tests/pool_unittest.cc | 2 +- 9 files changed, 173 insertions(+), 9 deletions(-) diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 7f554e3c1c..e286e49a86 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -292,6 +292,21 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& query) const { selector.client_classes_ = query->classes_; selector.iface_name_ = query->getIface(); + OptionPtr rai = query->getOption(DHO_DHCP_AGENT_OPTIONS); + if (rai) { + OptionCustomPtr oc = boost::dynamic_pointer_cast(rai); + if (oc) { + OptionPtr ols = oc->getOption(RAI_OPTION_LINK_SELECTION); + if (ols) { + OptionBuffer lsb = ols->getData(); + if (lsb.size() == sizeof(uint32_t)) { + selector.option_select_ = + IOAddress::fromBytes(AF_INET, &lsb[0]); + } + } + } + } + CfgMgr& cfgmgr = CfgMgr::instance(); subnet = cfgmgr.getCurrentCfg()->getCfgSubnets4()->selectSubnet(selector); diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index 2392634ba3..022440d72e 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc @@ -3236,6 +3236,106 @@ TEST_F(Dhcpv4SrvTest, relayOverrideAndClientClass) { EXPECT_TRUE(subnet1 == srv_.selectSubnet(dis)); } +// Checks if a RAI link selection sub-option works as expected +TEST_F(Dhcpv4SrvTest, relayLinkSelect) { + + // We have 3 subnets defined. + string config = "{ \"interfaces-config\": {" + " \"interfaces\": [ \"*\" ]" + "}," + "\"rebind-timer\": 2000, " + "\"renew-timer\": 1000, " + "\"subnet4\": [ " + "{ \"pools\": [ { \"pool\": \"192.0.2.2 - 192.0.2.100\" } ]," + " \"relay\": { " + " \"ip-address\": \"192.0.5.1\"" + " }," + " \"subnet\": \"192.0.2.0/24\" }, " + "{ \"pools\": [ { \"pool\": \"192.0.3.1 - 192.0.3.100\" } ]," + " \"subnet\": \"192.0.3.0/24\" }, " + "{ \"pools\": [ { \"pool\": \"192.0.4.1 - 192.0.4.100\" } ]," + " \"client-class\": \"foo\", " + " \"subnet\": \"192.0.4.0/24\" } " + "]," + "\"valid-lifetime\": 4000 }"; + + // Use this config to set up the server + ASSERT_NO_THROW(configure(config)); + + // Let's get the subnet configuration objects + const Subnet4Collection* subnets = + CfgMgr::instance().getCurrentCfg()->getCfgSubnets4()->getAll(); + ASSERT_EQ(3, subnets->size()); + + // Let's get them for easy reference + Subnet4Ptr subnet1 = (*subnets)[0]; + Subnet4Ptr subnet2 = (*subnets)[1]; + Subnet4Ptr subnet3 = (*subnets)[2]; + ASSERT_TRUE(subnet1); + ASSERT_TRUE(subnet2); + ASSERT_TRUE(subnet3); + + // Let's create a packet. + Pkt4Ptr dis = Pkt4Ptr(new Pkt4(DHCPDISCOVER, 1234)); + dis->setRemoteAddr(IOAddress("192.0.2.1")); + dis->setIface("eth0"); + dis->setHops(1); + OptionPtr clientid = generateClientId(); + dis->addOption(clientid); + + // Let's create a Relay Agent Information option + OptionDefinitionPtr rai_def = LibDHCP::getOptionDef(Option::V4, + DHO_DHCP_AGENT_OPTIONS); + ASSERT_TRUE(rai_def); + OptionCustomPtr rai(new OptionCustom(*rai_def, Option::V4)); + ASSERT_TRUE(rai); + IOAddress addr("192.0.3.2"); + OptionPtr ols(new Option(Option::V4, + RAI_OPTION_LINK_SELECTION, + addr.toBytes())); + ASSERT_TRUE(ols); + rai->addOption(ols); + + // This is just a sanity check, we're using regular method: ciaddr 192.0.3.1 + // belongs to the second subnet, so it is selected + dis->setGiaddr(IOAddress("192.0.3.1")); + EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis)); + + // Setup a relay override for the first subnet as it has a high precedence + dis->setGiaddr(IOAddress("192.0.5.1")); + EXPECT_TRUE(subnet1 == srv_.selectSubnet(dis)); + + // Put a RAI to select back the second subnet as it has + // the highest precedence + dis->addOption(rai); + EXPECT_TRUE(subnet2 == srv_.selectSubnet(dis)); + + // Check client-classification still applies + IOAddress addr_foo("192.0.4.2"); + ols.reset(new Option(Option::V4, RAI_OPTION_LINK_SELECTION, + addr_foo.toBytes())); + rai->delOption(RAI_OPTION_LINK_SELECTION); + dis->delOption(DHO_DHCP_AGENT_OPTIONS); + rai->addOption(ols); + dis->addOption(rai); + // Note it shall fail (vs. try the next criterion). + EXPECT_FALSE(srv_.selectSubnet(dis)); + // Add the packet to the class and check again: now it shall succeed + dis->addClass("foo"); + EXPECT_TRUE(subnet3 == srv_.selectSubnet(dis)); + + // Check it fails with a bad address in the sub-option + IOAddress addr_bad("10.0.0.1"); + ols.reset(new Option(Option::V4, RAI_OPTION_LINK_SELECTION, + addr_bad.toBytes())); + rai->delOption(RAI_OPTION_LINK_SELECTION); + dis->delOption(DHO_DHCP_AGENT_OPTIONS); + rai->addOption(ols); + dis->addOption(rai); + EXPECT_FALSE(srv_.selectSubnet(dis)); + +} + // This test verifies that the direct message is dropped when it has been // received by the server via an interface for which there is no subnet // configured. It also checks that the message is not dropped (is processed) diff --git a/src/lib/dhcpsrv/cfg_subnets4.cc b/src/lib/dhcpsrv/cfg_subnets4.cc index c5a52f19dd..f83746f8f3 100644 --- a/src/lib/dhcpsrv/cfg_subnets4.cc +++ b/src/lib/dhcpsrv/cfg_subnets4.cc @@ -39,6 +39,12 @@ CfgSubnets4::add(const Subnet4Ptr& subnet) { Subnet4Ptr CfgSubnets4::selectSubnet(const SubnetSelector& selector) const { + // First use RAI link select sub-option or subnet select option + if (!selector.option_select_.isV4Zero()) { + return (selectSubnet(selector.option_select_, + selector.client_classes_)); + } + // If relayed message has been received, try to match the giaddr with the // relay address specified for a subnet. It is also possible that the relay // address will not match with any of the relay addresses accross all diff --git a/src/lib/dhcpsrv/cfg_subnets4.h b/src/lib/dhcpsrv/cfg_subnets4.h index 1e1c287119..af7d725e97 100644 --- a/src/lib/dhcpsrv/cfg_subnets4.h +++ b/src/lib/dhcpsrv/cfg_subnets4.h @@ -61,6 +61,9 @@ public: /// parameters extracted from the client's message using the following /// logic. /// + /// First when link select suboption of relay agent information option + /// or subnet select option in this order exists the address is used + /// /// If the giaddr value is set in the selector it means that the client's /// message was relayed. The subnet configuration allows for setting the /// relay address for each subnet to indicate that the subnet must be diff --git a/src/lib/dhcpsrv/subnet_selector.h b/src/lib/dhcpsrv/subnet_selector.h index ef189dc407..cfcdc5fc36 100644 --- a/src/lib/dhcpsrv/subnet_selector.h +++ b/src/lib/dhcpsrv/subnet_selector.h @@ -1,4 +1,4 @@ -// Copyright (C) 2014 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2014-2015 Internet Systems Consortium, Inc. ("ISC") // // Permission to use, copy, modify, and/or distribute this software for any // purpose with or without fee is hereby granted, provided that the above @@ -35,6 +35,8 @@ struct SubnetSelector { asiolink::IOAddress ciaddr_; /// @brief giaddr from the client's message. asiolink::IOAddress giaddr_; + /// @brief RAI link select or subnet select option + asiolink::IOAddress option_select_; //@} /// @name DHCPv6 specific parameters. @@ -60,6 +62,7 @@ struct SubnetSelector { SubnetSelector() : ciaddr_(asiolink::IOAddress("0.0.0.0")), giaddr_(asiolink::IOAddress("0.0.0.0")), + option_select_(asiolink::IOAddress("0.0.0.0")), interface_id_(), first_relay_linkaddr_(asiolink::IOAddress("::")), local_address_(asiolink::IOAddress("0.0.0.0")), diff --git a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc index 495556ba32..3f941c6a25 100644 --- a/src/lib/dhcpsrv/tests/cfg_option_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_option_unittest.cc @@ -132,7 +132,7 @@ TEST(CfgOptionTest, add) { } // This test verifies that two option configurations can be merged. -TEST(CfgOption, merge) { +TEST(CfgOptionTest, merge) { CfgOption cfg_src; CfgOption cfg_dst; @@ -308,7 +308,7 @@ TEST(CfgOptionTest, encapsulate) { // This test verifies that single option can be retrieved from the configuration // using option code and option space. -TEST(CfgOption, get) { +TEST(CfgOptionTest, get) { CfgOption cfg; // Add 10 options to a "dhcp6" option space in the subnet. diff --git a/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc b/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc index 0febd2fa5e..956557463b 100644 --- a/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_subnets4_unittest.cc @@ -143,9 +143,46 @@ TEST(CfgSubnets4Test, selectSubnetByClasses) { EXPECT_FALSE(cfg.selectSubnet(selector)); } +// This test verifies the option selection can be used and is only +// used when present. +TEST(CfgSubnets4Test, selectSubnetByOptionSelect) { + CfgSubnets4 cfg; + + // Create 3 subnets. + Subnet4Ptr subnet1(new Subnet4(IOAddress("192.0.2.0"), 26, 1, 2, 3)); + Subnet4Ptr subnet2(new Subnet4(IOAddress("192.0.2.64"), 26, 1, 2, 3)); + Subnet4Ptr subnet3(new Subnet4(IOAddress("192.0.2.128"), 26, 1, 2, 3)); + + // Add them to the configuration. + cfg.add(subnet1); + cfg.add(subnet2); + cfg.add(subnet3); + + SubnetSelector selector; + + // Check that without option selection something else is used + selector.ciaddr_ = IOAddress("192.0.2.5"); + EXPECT_EQ(subnet1, cfg.selectSubnet(selector)); + + // The option selection has precedence + selector.option_select_ = IOAddress("192.0.2.130"); + EXPECT_EQ(subnet3, cfg.selectSubnet(selector)); + + // Over relay-info too + selector.giaddr_ = IOAddress("10.0.0.1"); + subnet2->setRelayInfo(IOAddress("10.0.0.1")); + EXPECT_EQ(subnet3, cfg.selectSubnet(selector)); + selector.option_select_ = IOAddress("0.0.0.0"); + EXPECT_EQ(subnet2, cfg.selectSubnet(selector)); + + // Check that a not matching option selection it shall fail + selector.option_select_ = IOAddress("10.0.0.1"); + EXPECT_FALSE(cfg.selectSubnet(selector)); +} + // This test verifies that the relay information can be used to retrieve the // subnet. -TEST(CfgSubnetsTest, selectSubnetByRelayAddress) { +TEST(CfgSubnets4Test, selectSubnetByRelayAddress) { CfgSubnets4 cfg; // Create 3 subnets. @@ -184,7 +221,7 @@ TEST(CfgSubnetsTest, selectSubnetByRelayAddress) { // This test verifies that the subnet can be selected for the client // using a source address if the client hasn't set the ciaddr. -TEST(CfgSubnetsTest, selectSubnetNoCiaddr) { +TEST(CfgSubnets4Test, selectSubnetNoCiaddr) { CfgSubnets4 cfg; // Create 3 subnets. @@ -224,7 +261,7 @@ TEST(CfgSubnetsTest, selectSubnetNoCiaddr) { // This test verifies that the subnet can be selected using an address // set on the local interface. -TEST(CfgSubnetsTest, selectSubnetInterface) { +TEST(CfgSubnets4Test, selectSubnetInterface) { // The IfaceMgrTestConfig object initializes fake interfaces: // eth0, eth1 and lo on the configuration manager. The CfgSubnets4 // object uses addresses assigned to these fake interfaces to @@ -276,7 +313,7 @@ TEST(CfgSubnetsTest, selectSubnetInterface) { // Checks that detection of duplicated subnet IDs works as expected. It should // not be possible to add two IPv4 subnets holding the same ID. -TEST(CfgSubnets4, duplication) { +TEST(CfgSubnets4Test, duplication) { CfgSubnets4 cfg; Subnet4Ptr subnet1(new Subnet4(IOAddress("192.0.2.0"), 26, 1, 2, 3, 123)); diff --git a/src/lib/dhcpsrv/tests/cfg_subnets6_unittest.cc b/src/lib/dhcpsrv/tests/cfg_subnets6_unittest.cc index 983f8238e8..f9a5e2a6bc 100644 --- a/src/lib/dhcpsrv/tests/cfg_subnets6_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_subnets6_unittest.cc @@ -337,7 +337,7 @@ TEST(CfgSubnets6Test, selectSubnetByInterfaceIdAndClassify) { // Checks that detection of duplicated subnet IDs works as expected. It should // not be possible to add two IPv6 subnets holding the same ID. -TEST(CfgSubnets6, duplication) { +TEST(CfgSubnets6Test, duplication) { CfgSubnets6 cfg; Subnet6Ptr subnet1(new Subnet6(IOAddress("2000::"), 48, 1, 2, 3, 4, 123)); diff --git a/src/lib/dhcpsrv/tests/pool_unittest.cc b/src/lib/dhcpsrv/tests/pool_unittest.cc index 342bd7304a..6d4ad68f36 100644 --- a/src/lib/dhcpsrv/tests/pool_unittest.cc +++ b/src/lib/dhcpsrv/tests/pool_unittest.cc @@ -117,7 +117,7 @@ TEST(Pool4Test, unique_id) { } // Simple check if toText returns reasonable values -TEST(Poo4Test,toText) { +TEST(Pool4Test,toText) { Pool4 pool1(IOAddress("192.0.2.7"), IOAddress("192.0.2.17")); EXPECT_EQ("type=V4, 192.0.2.7-192.0.2.17", pool1.toText()); From 260842f54e44f0ef23fe52c0f20826b00f816edc Mon Sep 17 00:00:00 2001 From: Francis Dupont Date: Fri, 9 Oct 2015 20:34:53 +0200 Subject: [PATCH 2/2] [4057] Addressed review comments --- src/bin/dhcp4/dhcp4_srv.cc | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index e286e49a86..59bfd9d1f0 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -292,16 +292,24 @@ Dhcpv4Srv::selectSubnet(const Pkt4Ptr& query) const { selector.client_classes_ = query->classes_; selector.iface_name_ = query->getIface(); + // If the link-selection sub-option is present, extract its value. + // "The link-selection sub-option is used by any DHCP relay agent + // that desires to specify a subnet/link for a DHCP client request + // that it is relaying but needs the subnet/link specification to + // be different from the IP address the DHCP server should use + // when communicating with the relay agent." (RFC 3257) OptionPtr rai = query->getOption(DHO_DHCP_AGENT_OPTIONS); if (rai) { - OptionCustomPtr oc = boost::dynamic_pointer_cast(rai); - if (oc) { - OptionPtr ols = oc->getOption(RAI_OPTION_LINK_SELECTION); - if (ols) { - OptionBuffer lsb = ols->getData(); - if (lsb.size() == sizeof(uint32_t)) { + OptionCustomPtr rai_custom = + boost::dynamic_pointer_cast(rai); + if (rai_custom) { + OptionPtr link_select = + rai_custom->getOption(RAI_OPTION_LINK_SELECTION); + if (link_select) { + OptionBuffer link_select_buf = link_select->getData(); + if (link_select_buf.size() == sizeof(uint32_t)) { selector.option_select_ = - IOAddress::fromBytes(AF_INET, &lsb[0]); + IOAddress::fromBytes(AF_INET, &link_select_buf[0]); } } }