From: Thomas Markwalder Date: Sat, 10 Nov 2018 18:20:22 +0000 (-0500) Subject: [#260,!20] Packet queueing is now optional X-Git-Tag: 204-move-models-base~4^2~18 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=bbb822b115a39d7430eaafe22204d200eb809b38;p=thirdparty%2Fkea.git [#260,!20] Packet queueing is now optional src/bin/dhcp<4/6>/ctrl_dhcp<4/6>_srv.cc ControlledDhcpv<4/6>Srv::processConfig() - now calls IfaceMgr::configureDHCPPacketQueue src/bin/dhcp<4/6>/dhcp<4/6>_parser.yy dhpc-queue-control parsing updated to enforce enable-queue/queue-type rules src/bin/dhcp<4/6>/tests/config_parser_unittest.cc TEST_F(Dhcp<4/6>ParserTest, dhcpQueueControl) TEST_F(Dhcp<4/6>ParserTest, dhcpQueueControlInvalid) src/lib/dhcp/iface_mgr.* IfaceMgr - closeSockets() - now calls stopDHCPReceiver - openSockets<4/6>() - now calls startDHCPReceiver - receive<4/6>Indirect() - new function which monitors receiver thread watch sockets, reads DHCP packets from queue - receive<4/6>Direct() - new function which monitors and reads DHCP packets from interface sockets directly - receive<4/6>() - rewritten to call receive<4/6>Indirect if receiver thread is running, otherwise it calls receive<4/6>Direct - configureDHCPPacketQueue() - new function which either enables queuing by creating a new packet queue, or disables it by destroying the existing queue src/lib/dhcp/packet_queue_mgr.h PacketQueue::destroyPacketQueue() - new function src/lib/dhcp/packet_queue_mgr<4/6>.cc PacketQueueMgr<4/6>::PacketQueueMgr<4/6>() - no longer creates a default packet queue src/lib/dhcpsrv/cfg_iface.cc CfgIface::closeSockets() - removed call to stopDHCPReceiver CfgIface::openSockets() - removed call to startDHCPReceiver src/lib/dhcpsrv/parsers/dhcp_queue_control_parser.* DHCPQueueControlParser - removed unused family_ member - parse() - added support for enable-queue src/lib/dhcpsrv/tests/dhcp_queue_control_parser_unittest.cc - new file --- diff --git a/src/bin/dhcp4/ctrl_dhcp4_srv.cc b/src/bin/dhcp4/ctrl_dhcp4_srv.cc index 20b78029b7..b78154bc55 100644 --- a/src/bin/dhcp4/ctrl_dhcp4_srv.cc +++ b/src/bin/dhcp4/ctrl_dhcp4_srv.cc @@ -634,24 +634,15 @@ ControlledDhcpv4Srv::processConfig(isc::data::ConstElementPtr config) { return (isc::config::createAnswer(1, err.str())); } - // Configure packet queue + // Configure DHCP packet queueing try { data::ConstElementPtr qc; qc = CfgMgr::instance().getStagingCfg()->getDHCPQueueControl(); - if (!qc) { - // @todo For now we're manually constructing default queue config - // This probably needs to be built into the PQM? - data::ElementPtr default_qc = data::Element::createMap(); - default_qc->set("queue-type", data::Element::create("kea-ring4")); - default_qc->set("capacity", data::Element::create(static_cast(500))); - PacketQueueMgr4::instance().createPacketQueue(default_qc); - } else { - PacketQueueMgr4::instance().createPacketQueue(qc); + if (IfaceMgr::instance().configureDHCPPacketQueue(AF_INET, qc)) { + LOG_DEBUG(dhcp4_logger, DBG_DHCP4_BASIC, DHCP4_CONFIG_PACKET_QUEUE) + .arg(PacketQueueMgr4::instance().getPacketQueue()->getInfoStr()); } - LOG_DEBUG(dhcp4_logger, DBG_DHCP4_BASIC, DHCP4_CONFIG_PACKET_QUEUE) - .arg(PacketQueueMgr4::instance().getPacketQueue()->getInfoStr()); - } catch (const std::exception& ex) { err << "Error setting packet queue controls after server reconfiguration: " << ex.what(); diff --git a/src/bin/dhcp4/dhcp4_parser.yy b/src/bin/dhcp4/dhcp4_parser.yy index 0a03e7eba7..ffe7a2b1b5 100644 --- a/src/bin/dhcp4/dhcp4_parser.yy +++ b/src/bin/dhcp4/dhcp4_parser.yy @@ -1844,13 +1844,40 @@ dhcp_queue_control: DHCP_QUEUE_CONTROL { ElementPtr qc = $4; ctx.stack_.back()->set("dhcp-queue-control", qc); - if (!qc->contains("queue-type")) { + // Doing this manually, because dhcp-queue-control + // content is otherwise arbitrary + if (!qc->contains("enable-queue")) { std::stringstream msg; - msg << "'queue-type' is required: "; + msg << "'enable-queue' is required: "; msg << qc->getPosition().str() << ")"; error(@1, msg.str()); } + ConstElementPtr enable_queue = qc->get("enable-queue"); + if (enable_queue->getType() != Element::boolean) { + std::stringstream msg; + msg << "'enable-queue' must be boolean: "; + msg << qc->getPosition().str() << ")"; + error(@1, msg.str()); + } + + if (enable_queue->boolValue()) { + if (!qc->contains("queue-type")) { + std::stringstream msg; + msg << "'queue-type' is required, when 'enable-queue' is true: "; + msg << qc->getPosition().str() << ")"; + error(@1, msg.str()); + } + + ConstElementPtr queue_type = qc->get("queue-type"); + if (queue_type->getType() != Element::string) { + std::stringstream msg; + msg << "'queue-type' must be a string: "; + msg << qc->getPosition().str() << ")"; + error(@1, msg.str()); + } + } + ctx.leave(); }; diff --git a/src/bin/dhcp4/json_config_parser.cc b/src/bin/dhcp4/json_config_parser.cc index fcaaa75b00..c62678b5c4 100644 --- a/src/bin/dhcp4/json_config_parser.cc +++ b/src/bin/dhcp4/json_config_parser.cc @@ -391,7 +391,7 @@ configureDhcp4Server(Dhcpv4Srv& server, isc::data::ConstElementPtr config_set, } if (config_pair.first == "dhcp-queue-control") { - DHCPQueueControlParser parser(AF_INET); + DHCPQueueControlParser parser; srv_cfg->setDHCPQueueControl(parser.parse(config_pair.second)); continue; } diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index f3d5e02258..9188b5f8a3 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/src/bin/dhcp4/tests/config_parser_unittest.cc @@ -6488,20 +6488,33 @@ TEST_F(Dhcp4ParserTest, dhcpQueueControl) { "" }, { - "valid entry", + "queue disabled", "{ \n" - " \"queue-type\": \"some-type\", \n" - " \"capacity\": 75 \n" + " \"enable-queue\": false \n" + "} \n" + }, + { + "queue disabled, arbitrary content allowed", + "{ \n" + " \"enable-queue\": false, \n" + " \"foo\": \"bogus\", \n" + " \"random-int\" : 1234 \n" + "} \n" + }, + { + "queue enabled, with queue-type", + "{ \n" + " \"enable-queue\": true, \n" + " \"queue-type\": \"some-type\" \n" "} \n" }, { - "valid arbitrary content", + "queue enabled with queue-type and arbitrary content", "{ \n" - " \"queue-type\": \"some-type\", \n" - " \"capacity\": 90, \n" - " \"user-context\": { \"comment\": \"some text\" },\n" - " \"random-bool\" : false, \n" - " \"random-int\" : 1234 \n" + " \"enable-queue\": true, \n" + " \"queue-type\": \"some-type\", \n" + " \"foo\": \"bogus\", \n" + " \"random-int\" : 1234 \n" "} \n" } }; @@ -6511,9 +6524,7 @@ TEST_F(Dhcp4ParserTest, dhcpQueueControl) { control = CfgMgr::instance().getStagingCfg()->getDHCPQueueControl(); ASSERT_FALSE(control); - // Iterate over the incorrect scenarios and verify they - // fail as expected. Note, we use parseDHCP4() directly - // as all of the errors above are enforced by the grammar. + // Iterate over the valid scenarios and verify they succeed. data::ConstElementPtr exp_elems; for (auto scenario : scenarios) { SCOPED_TRACE(scenario.description_); @@ -6558,24 +6569,43 @@ TEST_F(Dhcp4ParserTest, dhcpQueueControlInvalid) { struct Scenario { std::string description_; std::string json_; + std::string exp_error_; }; std::vector scenarios = { { - "not a map", - "{ " + genIfaceConfig() + ", \n" + - " \"subnet4\": [ ], \n" - " \"dhcp-queue-control\": 75 \n" - "} \n" + "not a map", + "75 \n", + ":2.24-25: syntax error, unexpected integer, expecting {" + }, + { + "enable-queue missing", + "{ \n" + " \"enable-type\": \"some-type\" \n" + "} \n", + ":2.2-21: 'enable-queue' is required: :2:24)" }, { - "queue type missing", - "{ " + genIfaceConfig() + ", \n" + - " \"subnet4\": [ ], \n" - " \"dhcp-queue-control\": { \n" - " \"capacity\": 100 \n" - " } \n" - "} \n" + "enable-queue not boolean", + "{ \n" + " \"enable-queue\": \"always\" \n" + "} \n", + ":2.2-21: 'enable-queue' must be boolean: :2:24)" + }, + { + "queue enabled, type missing", + "{ \n" + " \"enable-queue\": true \n" + "} \n", + ":2.2-21: 'queue-type' is required, when 'enable-queue' is true: :2:24)" + }, + { + "queue enabled, type not a string", + "{ \n" + " \"enable-queue\": true, \n" + " \"queue-type\": 7777 \n" + "} \n", + ":2.2-21: 'queue-type' must be a string: :2:24)" } }; @@ -6585,10 +6615,23 @@ TEST_F(Dhcp4ParserTest, dhcpQueueControlInvalid) { for (auto scenario : scenarios) { SCOPED_TRACE(scenario.description_); { - EXPECT_THROW(parseDHCP4(scenario.json_), Dhcp4ParseError); + // Construct the config JSON + std::stringstream os; + os << "{ " + genIfaceConfig(); + os << ",\n \"dhcp-queue-control\": " << scenario.json_; + os << "} \n"; + + std::string error_msg = ""; + try { + ASSERT_TRUE(parseDHCP4(os.str(), false)) << "parser returned empty element"; + } catch(const std::exception& ex) { + error_msg = ex.what(); + } + + ASSERT_FALSE(error_msg.empty()) << "parseDHCP4 should have thrown"; + EXPECT_EQ(scenario.exp_error_, error_msg); } } } - } diff --git a/src/bin/dhcp6/ctrl_dhcp6_srv.cc b/src/bin/dhcp6/ctrl_dhcp6_srv.cc index a7241c1170..08e6e2f65d 100644 --- a/src/bin/dhcp6/ctrl_dhcp6_srv.cc +++ b/src/bin/dhcp6/ctrl_dhcp6_srv.cc @@ -653,27 +653,18 @@ ControlledDhcpv6Srv::processConfig(isc::data::ConstElementPtr config) { return (isc::config::createAnswer(1, err.str())); } - // Configure DHCP packet queue + // Configure DHCP packet queueing try { data::ConstElementPtr qc; qc = CfgMgr::instance().getStagingCfg()->getDHCPQueueControl(); - if (!qc) { - // @todo For now we're manually constructing default queue config - // This probably needs to be built into the PQM? - data::ElementPtr default_qc = data::Element::createMap(); - default_qc->set("queue-type", data::Element::create("kea-ring6")); - default_qc->set("capacity", data::Element::create(static_cast(500))); - PacketQueueMgr6::instance().createPacketQueue(default_qc); - } else { - PacketQueueMgr6::instance().createPacketQueue(qc); + if (IfaceMgr::instance().configureDHCPPacketQueue(AF_INET6, qc)) { + LOG_DEBUG(dhcp6_logger, DBG_DHCP6_BASIC, DHCP6_CONFIG_PACKET_QUEUE) + .arg(PacketQueueMgr6::instance().getPacketQueue()->getInfoStr()); } - LOG_DEBUG(dhcp6_logger, DBG_DHCP6_BASIC, DHCP6_CONFIG_PACKET_QUEUE) - .arg(PacketQueueMgr6::instance().getPacketQueue()->getInfoStr()); - } catch (const std::exception& ex) { std::ostringstream err; - err << "Error setting DHCP packet queue controls after server reconfiguration: " + err << "Error setting packet queue controls after server reconfiguration: " << ex.what(); return (isc::config::createAnswer(1, err.str())); } diff --git a/src/bin/dhcp6/dhcp6_parser.yy b/src/bin/dhcp6/dhcp6_parser.yy index be817255bf..232476e897 100644 --- a/src/bin/dhcp6/dhcp6_parser.yy +++ b/src/bin/dhcp6/dhcp6_parser.yy @@ -1933,13 +1933,41 @@ dhcp_queue_control: DHCP_QUEUE_CONTROL { ElementPtr qc = $4; ctx.stack_.back()->set("dhcp-queue-control", qc); - if (!qc->contains("queue-type")) { + // Doing this manually, because dhcp-queue-control + // content is otherwise arbitrary + if (!qc->contains("enable-queue")) { std::stringstream msg; - msg << "'queue-type' is required: "; + msg << "'enable-queue' is required: "; msg << qc->getPosition().str() << ")"; error(@1, msg.str()); } + ConstElementPtr enable_queue = qc->get("enable-queue"); + if (enable_queue->getType() != Element::boolean) { + std::stringstream msg; + msg << "'enable-queue' must be boolean: "; + msg << qc->getPosition().str() << ")"; + error(@1, msg.str()); + } + + if (enable_queue->boolValue()) { + if (!qc->contains("queue-type")) { + std::stringstream msg; + msg << "'queue-type' is required, when 'enable-queue' is true: "; + msg << qc->getPosition().str() << ")"; + error(@1, msg.str()); + } + + ConstElementPtr queue_type = qc->get("queue-type"); + if (queue_type->getType() != Element::string) { + std::stringstream msg; + msg << "'queue-type' must be a string: "; + msg << qc->getPosition().str() << ")"; + error(@1, msg.str()); + } + } + + ctx.leave(); }; diff --git a/src/bin/dhcp6/json_config_parser.cc b/src/bin/dhcp6/json_config_parser.cc index 555b3500c9..26d4831061 100644 --- a/src/bin/dhcp6/json_config_parser.cc +++ b/src/bin/dhcp6/json_config_parser.cc @@ -484,7 +484,7 @@ configureDhcp6Server(Dhcpv6Srv& server, isc::data::ConstElementPtr config_set, } if (config_pair.first == "dhcp-queue-control") { - DHCPQueueControlParser parser(AF_INET); + DHCPQueueControlParser parser; srv_config->setDHCPQueueControl(parser.parse(config_pair.second)); continue; } diff --git a/src/bin/dhcp6/tests/config_parser_unittest.cc b/src/bin/dhcp6/tests/config_parser_unittest.cc index 646f5c3975..12f6f2f240 100644 --- a/src/bin/dhcp6/tests/config_parser_unittest.cc +++ b/src/bin/dhcp6/tests/config_parser_unittest.cc @@ -6993,20 +6993,33 @@ TEST_F(Dhcp6ParserTest, dhcpQueueControl) { "" }, { - "valid entry", + "queue disabled", "{ \n" - " \"queue-type\": \"some-type\", \n" - " \"capacity\": 75 \n" + " \"enable-queue\": false \n" + "} \n" + }, + { + "queue disabled, arbitrary content allowed", + "{ \n" + " \"enable-queue\": false, \n" + " \"foo\": \"bogus\", \n" + " \"random-int\" : 1234 \n" "} \n" }, { - "valid arbitrary content", + "queue enabled, with queue-type", "{ \n" - " \"queue-type\": \"some-type\", \n" - " \"capacity\": 90, \n" - " \"user-context\": { \"comment\": \"some text\" },\n" - " \"random-bool\" : false, \n" - " \"random-int\" : 1236 \n" + " \"enable-queue\": true, \n" + " \"queue-type\": \"some-type\" \n" + "} \n" + }, + { + "queue enabled with queue-type and arbitrary content", + "{ \n" + " \"enable-queue\": true, \n" + " \"queue-type\": \"some-type\", \n" + " \"foo\": \"bogus\", \n" + " \"random-int\" : 1234 \n" "} \n" } }; @@ -7016,9 +7029,7 @@ TEST_F(Dhcp6ParserTest, dhcpQueueControl) { control = CfgMgr::instance().getStagingCfg()->getDHCPQueueControl(); ASSERT_FALSE(control); - // Iterate over the incorrect scenarios and verify they - // fail as expected. Note, we use parseDHCP6() directly - // as all of the errors above are enforced by the grammar. + // Iterate over the valid scenarios and verify they succeed. data::ConstElementPtr exp_elems; for (auto scenario : scenarios) { SCOPED_TRACE(scenario.description_); @@ -7063,24 +7074,43 @@ TEST_F(Dhcp6ParserTest, dhcpQueueControlInvalid) { struct Scenario { std::string description_; std::string json_; + std::string exp_error_; }; std::vector scenarios = { { - "not a map", - "{ " + genIfaceConfig() + ", \n" + - " \"subnet6\": [ ], \n" - " \"dhcp-queue-control\": 75 \n" - "} \n" + "not a map", + "75 \n", + ":2.24-25: syntax error, unexpected integer, expecting {" + }, + { + "enable-queue missing", + "{ \n" + " \"enable-type\": \"some-type\" \n" + "} \n", + ":2.2-21: 'enable-queue' is required: :2:24)" + }, + { + "enable-queue not boolean", + "{ \n" + " \"enable-queue\": \"always\" \n" + "} \n", + ":2.2-21: 'enable-queue' must be boolean: :2:24)" }, { - "queue type missing", - "{ " + genIfaceConfig() + ", \n" + - " \"subnet6\": [ ], \n" - " \"dhcp-queue-control\": { \n" - " \"capacity\": 100 \n" - " } \n" - "} \n" + "queue enabled, type missing", + "{ \n" + " \"enable-queue\": true \n" + "} \n", + ":2.2-21: 'queue-type' is required, when 'enable-queue' is true: :2:24)" + }, + { + "queue enabled, type not a string", + "{ \n" + " \"enable-queue\": true, \n" + " \"queue-type\": 7777 \n" + "} \n", + ":2.2-21: 'queue-type' must be a string: :2:24)" } }; @@ -7090,7 +7120,21 @@ TEST_F(Dhcp6ParserTest, dhcpQueueControlInvalid) { for (auto scenario : scenarios) { SCOPED_TRACE(scenario.description_); { - EXPECT_THROW(parseDHCP6(scenario.json_), Dhcp6ParseError); + // Construct the config JSON + std::stringstream os; + os << "{ " + genIfaceConfig(); + os << ",\n \"dhcp-queue-control\": " << scenario.json_; + os << "} \n"; + + std::string error_msg = ""; + try { + ASSERT_TRUE(parseDHCP6(os.str(), false)) << "parser returned empty element"; + } catch(const std::exception& ex) { + error_msg = ex.what(); + } + + ASSERT_FALSE(error_msg.empty()) << "parseDHCP6 should have thrown"; + EXPECT_EQ(scenario.exp_error_, error_msg); } } } diff --git a/src/lib/dhcp/iface_mgr.cc b/src/lib/dhcp/iface_mgr.cc index 8792b216e6..8c6ace93fc 100644 --- a/src/lib/dhcp/iface_mgr.cc +++ b/src/lib/dhcp/iface_mgr.cc @@ -93,6 +93,7 @@ Iface::closeSockets(const uint16_t family) { << " specified when requested to close all sockets" << " which belong to this family"); } + // Search for the socket of the specific type. SocketCollection::iterator sock = sockets_.begin(); while (sock != sockets_.end()) { @@ -282,6 +283,9 @@ Iface::countActive4() const { } void IfaceMgr::closeSockets() { + // Stops the receiver thread if there is one. + stopDHCPReceiver(); + BOOST_FOREACH(IfacePtr iface, ifaces_) { iface->closeSockets(); } @@ -293,11 +297,17 @@ void IfaceMgr::stopDHCPReceiver() { receiver_thread_->wait(); receiver_thread_.reset(); error_watch_.clearReady(); + } receiver_error_ = "no error"; - getPacketQueue4()->clear(); - getPacketQueue4()->clear(); + if (getPacketQueue4()) { + getPacketQueue4()->clear(); + } + + if (getPacketQueue6()) { + getPacketQueue6()->clear(); + } } IfaceMgr::~IfaceMgr() { @@ -584,6 +594,12 @@ IfaceMgr::openSockets4(const uint16_t port, const bool use_bcast, } } + + if (count > 0) { + // starts the receiver thread (if queueing is enabled); + startDHCPReceiver(AF_INET); + } + return (count > 0); } @@ -662,6 +678,11 @@ IfaceMgr::openSockets6(const uint16_t port, } } + + if (count > 0) { + // starts the receiver thread (if queueing is enabled); + startDHCPReceiver(AF_INET6); + } return (count > 0); } @@ -674,14 +695,14 @@ IfaceMgr::startDHCPReceiver(const uint16_t family) { switch (family) { case AF_INET: if(!getPacketQueue4()) { - isc_throw(Unexpected, "startDHCPRecever - no packet queue?"); + return; } receiver_thread_.reset(new Thread(boost::bind(&IfaceMgr::receiveDHCP4Packets, this))); break; case AF_INET6: if(!getPacketQueue6()) { - isc_throw(Unexpected, "startDHCPRecever - no packet queue?"); + return; } receiver_thread_.reset(new Thread(boost::bind(&IfaceMgr::receiveDHCP6Packets, this))); @@ -942,8 +963,15 @@ IfaceMgr::send(const Pkt4Ptr& pkt) { return (packet_filter_->send(*iface, getSocket(*pkt).sockfd_, pkt)); } - Pkt4Ptr IfaceMgr::receive4(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */) { + if (receiver_thread_) { + return (receive4Indirect(timeout_sec, timeout_usec)); + } + + return (receive4Direct(timeout_sec, timeout_usec)); +} + +Pkt4Ptr IfaceMgr::receive4Indirect(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */) { // Sanity check for microsecond timeout. if (timeout_usec >= 1000000) { isc_throw(BadValue, "fractional timeout must be shorter than" @@ -1051,7 +1079,233 @@ Pkt4Ptr IfaceMgr::receive4(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ return (pkt); } -Pkt6Ptr IfaceMgr::receive6(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ ) { +Pkt4Ptr IfaceMgr::receive4Direct(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */) { + // Sanity check for microsecond timeout. + if (timeout_usec >= 1000000) { + isc_throw(BadValue, "fractional timeout must be shorter than" + " one million microseconds"); + } + boost::scoped_ptr candidate; + IfacePtr iface; + fd_set sockets; + int maxfd = 0; + + FD_ZERO(&sockets); + + /// @todo: marginal performance optimization. We could create the set once + /// and then use its copy for select(). Please note that select() modifies + /// provided set to indicated which sockets have something to read. + BOOST_FOREACH(iface, ifaces_) { + BOOST_FOREACH(SocketInfo s, iface->getSockets()) { + + // Only deal with IPv4 addresses. + if (s.addr_.isV4()) { + + // Add this socket to listening set + FD_SET(s.sockfd_, &sockets); + if (maxfd < s.sockfd_) { + maxfd = s.sockfd_; + } + } + } + } + + // if there are any callbacks for external sockets registered... + if (!callbacks_.empty()) { + BOOST_FOREACH(SocketCallbackInfo s, callbacks_) { + FD_SET(s.socket_, &sockets); + if (maxfd < s.socket_) { + maxfd = s.socket_; + } + } + } + + struct timeval select_timeout; + select_timeout.tv_sec = timeout_sec; + select_timeout.tv_usec = timeout_usec; + + // zero out the errno to be safe + errno = 0; + + int result = select(maxfd + 1, &sockets, NULL, NULL, &select_timeout); + + if (result == 0) { + // nothing received and timeout has been reached + return (Pkt4Ptr()); // NULL + + } else if (result < 0) { + // In most cases we would like to know whether select() returned + // an error because of a signal being received or for some other + // reason. This is because DHCP servers use signals to trigger + // certain actions, like reconfiguration or graceful shutdown. + // By catching a dedicated exception the caller will know if the + // error returned by the function is due to the reception of the + // signal or for some other reason. + if (errno == EINTR) { + isc_throw(SignalInterruptOnSelect, strerror(errno)); + } else { + isc_throw(SocketReadError, strerror(errno)); + } + } + + // Let's find out which socket has the data + BOOST_FOREACH(SocketCallbackInfo s, callbacks_) { + if (!FD_ISSET(s.socket_, &sockets)) { + continue; + } + + // something received over external socket + + // Calling the external socket's callback provides its service + // layer access without integrating any specific features + // in IfaceMgr + if (s.callback_) { + s.callback_(); + } + + return (Pkt4Ptr()); + } + + // Let's find out which interface/socket has the data + BOOST_FOREACH(iface, ifaces_) { + BOOST_FOREACH(SocketInfo s, iface->getSockets()) { + if (FD_ISSET(s.sockfd_, &sockets)) { + candidate.reset(new SocketInfo(s)); + break; + } + } + if (candidate) { + break; + } + } + + if (!candidate) { + isc_throw(SocketReadError, "received data over unknown socket"); + } + + // Now we have a socket, let's get some data from it! + // Assuming that packet filter is not NULL, because its modifier checks it. + return (packet_filter_->receive(*iface, *candidate)); +} + +Pkt6Ptr IfaceMgr::receive6(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */) { + if (receiver_thread_) { + return (receive6Indirect(timeout_sec, timeout_usec)); + } + + return (receive6Direct(timeout_sec, timeout_usec)); +} + +Pkt6Ptr IfaceMgr::receive6Direct(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ ) { + // Sanity check for microsecond timeout. + if (timeout_usec >= 1000000) { + isc_throw(BadValue, "fractional timeout must be shorter than" + " one million microseconds"); + } + + boost::scoped_ptr candidate; + fd_set sockets; + int maxfd = 0; + + FD_ZERO(&sockets); + + /// @todo: marginal performance optimization. We could create the set once + /// and then use its copy for select(). Please note that select() modifies + /// provided set to indicated which sockets have something to read. + BOOST_FOREACH(IfacePtr iface, ifaces_) { + + BOOST_FOREACH(SocketInfo s, iface->getSockets()) { + // Only deal with IPv6 addresses. + if (s.addr_.isV6()) { + + // Add this socket to listening set + FD_SET(s.sockfd_, &sockets); + if (maxfd < s.sockfd_) { + maxfd = s.sockfd_; + } + } + } + } + + // if there are any callbacks for external sockets registered... + if (!callbacks_.empty()) { + BOOST_FOREACH(SocketCallbackInfo s, callbacks_) { + // Add it to the set as well + FD_SET(s.socket_, &sockets); + if (maxfd < s.socket_) { + maxfd = s.socket_; + } + } + } + + struct timeval select_timeout; + select_timeout.tv_sec = timeout_sec; + select_timeout.tv_usec = timeout_usec; + + // zero out the errno to be safe + errno = 0; + + int result = select(maxfd + 1, &sockets, NULL, NULL, &select_timeout); + + if (result == 0) { + // nothing received and timeout has been reached + return (Pkt6Ptr()); // NULL + + } else if (result < 0) { + // In most cases we would like to know whether select() returned + // an error because of a signal being received or for some other + // reason. This is because DHCP servers use signals to trigger + // certain actions, like reconfiguration or graceful shutdown. + // By catching a dedicated exception the caller will know if the + // error returned by the function is due to the reception of the + // signal or for some other reason. + if (errno == EINTR) { + isc_throw(SignalInterruptOnSelect, strerror(errno)); + } else { + isc_throw(SocketReadError, strerror(errno)); + } + } + + // Let's find out which socket has the data + BOOST_FOREACH(SocketCallbackInfo s, callbacks_) { + if (!FD_ISSET(s.socket_, &sockets)) { + continue; + } + + // something received over external socket + + // Calling the external socket's callback provides its service + // layer access without integrating any specific features + // in IfaceMgr + if (s.callback_) { + s.callback_(); + } + + return (Pkt6Ptr()); + } + + // Let's find out which interface/socket has the data + BOOST_FOREACH(IfacePtr iface, ifaces_) { + BOOST_FOREACH(SocketInfo s, iface->getSockets()) { + if (FD_ISSET(s.sockfd_, &sockets)) { + candidate.reset(new SocketInfo(s)); + break; + } + } + if (candidate) { + break; + } + } + + if (!candidate) { + isc_throw(SocketReadError, "received data over unknown socket"); + } + // Assuming that packet filter is not NULL, because its modifier checks it. + return (packet_filter6_->receive(*candidate)); +} + + +Pkt6Ptr IfaceMgr::receive6Indirect(uint32_t timeout_sec, uint32_t timeout_usec /* = 0 */ ) { // Sanity check for microsecond timeout. if (timeout_usec >= 1000000) { isc_throw(BadValue, "fractional timeout must be shorter than" @@ -1485,5 +1739,42 @@ IfaceMgr::getSocket(isc::dhcp::Pkt4 const& pkt) { return (*candidate); } +bool +IfaceMgr::configureDHCPPacketQueue(uint16_t family, data::ConstElementPtr queue_control) { + if (receiver_thread_) { + isc_throw(InvalidOperation, "Cannot reconfigure queueing" + " while receiver thread is running"); + } + + bool enable_queue = false; + if (queue_control) { + try { + enable_queue = data::SimpleParser::getBoolean(queue_control, "enable-queue"); + } catch (...) { + // @todo - for now swallow not found errors. + // if not present we assume default + } + } + + if (enable_queue) { + // Try to create the queue as configured. + if (family == AF_INET) { + PacketQueueMgr4::instance().createPacketQueue(queue_control); + } else { + PacketQueueMgr6::instance().createPacketQueue(queue_control); + } + } else { + // Destroy the current queue (if one), this inherently disables threading. + if (family == AF_INET) { + PacketQueueMgr4::instance().destroyPacketQueue(); + } else { + PacketQueueMgr6::instance().destroyPacketQueue(); + } + } + + return(enable_queue); +} + + } // end of namespace isc::dhcp } // end of namespace isc diff --git a/src/lib/dhcp/iface_mgr.h b/src/lib/dhcp/iface_mgr.h index 979e2667a7..37c4d1ffe4 100644 --- a/src/lib/dhcp/iface_mgr.h +++ b/src/lib/dhcp/iface_mgr.h @@ -699,6 +699,9 @@ public: /// @return Pkt6 object representing received packet (or NULL) Pkt6Ptr receive6(uint32_t timeout_sec, uint32_t timeout_usec = 0); + Pkt6Ptr receive6Direct(uint32_t timeout_sec, uint32_t timeout_usec = 0); + Pkt6Ptr receive6Indirect(uint32_t timeout_sec, uint32_t timeout_usec = 0); + /// @brief Tries to receive IPv4 packet over open IPv4 sockets. /// /// Attempts to receive a single DHCPv4 message over any of the open @@ -721,6 +724,9 @@ public: /// @return Pkt4 object representing received packet (or NULL) Pkt4Ptr receive4(uint32_t timeout_sec, uint32_t timeout_usec = 0); + Pkt4Ptr receive4Direct(uint32_t timeout_sec, uint32_t timeout_usec = 0); + Pkt4Ptr receive4Indirect(uint32_t timeout_sec, uint32_t timeout_usec = 0); + /// Opens UDP/IP socket and binds it to address, interface and port. /// /// Specific type of socket (UDP/IPv4 or UDP/IPv6) depends on passed addr @@ -808,6 +814,9 @@ public: /// but it is not running, it is down, or is a loopback interface when /// loopback is not allowed, an error is reported. /// + /// If sockets were successfully opened, it calls @ startDHCPReceiver to + /// start the receiver thread (if packet queueing is enabled). + /// /// On the systems with multiple interfaces, it is often desired that the /// failure to open a socket on a particular interface doesn't cause a /// fatal error and sockets should be opened on remaining interfaces. @@ -857,6 +866,9 @@ public: /// represented by a class derived from @c isc::dhcp::PktFilter abstract /// class. /// + /// If sockets were successfully opened, it calls @ startDHCPReceiver to + /// start the receiver thread (if packet queueing is enabled). + /// /// It is possible to specify whether sockets should be broadcast capable. /// In most of the cases, the sockets should support broadcast traffic, e.g. /// DHCPv4 server and relay need to listen to broadcast messages sent by @@ -916,6 +928,10 @@ public: IfaceMgrErrorMsgCallback error_handler = 0); /// @brief Closes all open sockets. + /// + /// It calls @c stopDHCPReceiver to stop the receiver thread and then + /// it closes all open interface sockets. + /// /// Is used in destructor, but also from Dhcpv4Srv and Dhcpv6Srv classes. void closeSockets(); @@ -1047,7 +1063,8 @@ public: /// @brief Starts DHCP packet receiver. /// /// Starts the DHCP packet receiver thread for the given. - /// protocol, AF_NET or AF_INET6 + /// protocol, AF_NET or AF_INET6, if the packet queue + /// exists, otherwise it simply returns. /// /// @param family indicates which receiver to start, /// (AF_INET or AF_INET6) @@ -1057,9 +1074,27 @@ public: /// @brief Stops the DHCP packet receiver. /// - /// Stops the receiver and deletes the dedicated thread. + /// If the thread exists, it is stopped, deleted, and + /// the packet queue is flushed. void stopDHCPReceiver(); + /// @brief Configures DHCP packet queue + /// + /// If the given configuration enables packet queueing, then the + /// appropriate queue is created. Otherwise, the existing queue is + /// destroyed. If the receiver thread is running when this function + /// is invoked, it will throw. + /// + /// @param family indicates which receiver to start, + /// (AF_INET or AF_INET6) + /// @parm queue_control configuration containing "dhcp-queue-control" + /// content + /// @return true if packet queueuing has been enabled, false otherwise + /// @throw InvalidOperation if the receiver thread is currently running. + bool configureDHCPPacketQueue(const uint16_t family, + data::ConstElementPtr queue_control); + + // don't use private, we need derived classes in tests protected: diff --git a/src/lib/dhcp/packet_queue_mgr.h b/src/lib/dhcp/packet_queue_mgr.h index e34bf261e5..5b1d4da779 100644 --- a/src/lib/dhcp/packet_queue_mgr.h +++ b/src/lib/dhcp/packet_queue_mgr.h @@ -158,6 +158,12 @@ public: return (packet_queue_); } + /// @brief Destroys the current packet queue. + /// Any queued packets will be discarded. + void destroyPacketQueue() { + packet_queue_.reset(); + } + protected: /// @brief A map holding registered backend factory functions. std::map factories_; diff --git a/src/lib/dhcp/packet_queue_mgr4.cc b/src/lib/dhcp/packet_queue_mgr4.cc index 079c067de1..82f7740fc8 100644 --- a/src/lib/dhcp/packet_queue_mgr4.cc +++ b/src/lib/dhcp/packet_queue_mgr4.cc @@ -30,11 +30,6 @@ PacketQueueMgr4::PacketQueueMgr4() { PacketQueue4Ptr queue(new PacketQueueRing4("kea-ring4", capacity)); return (queue); }); - - data::ElementPtr parameters = data::Element::createMap(); - parameters->set("queue-type", data::Element::create("kea-ring4")); - parameters->set("capacity", data::Element::create(static_cast(500))); - createPacketQueue(parameters); } boost::scoped_ptr& diff --git a/src/lib/dhcp/packet_queue_mgr4.h b/src/lib/dhcp/packet_queue_mgr4.h index dc1a26bb77..a770f51d0c 100644 --- a/src/lib/dhcp/packet_queue_mgr4.h +++ b/src/lib/dhcp/packet_queue_mgr4.h @@ -55,8 +55,7 @@ public: private: /// @brief Private constructor. /// - /// It registers a default factory for DHCPv4 queues and creates - /// an default DHCPv4 packet queue. + /// It registers a default factory for DHCPv4 queues. PacketQueueMgr4(); /// @brief Returns a pointer to the currently instance of the diff --git a/src/lib/dhcp/packet_queue_mgr6.cc b/src/lib/dhcp/packet_queue_mgr6.cc index b6c8ad9212..705daaa66c 100644 --- a/src/lib/dhcp/packet_queue_mgr6.cc +++ b/src/lib/dhcp/packet_queue_mgr6.cc @@ -30,11 +30,6 @@ PacketQueueMgr6::PacketQueueMgr6() { PacketQueue6Ptr queue(new PacketQueueRing6("kea-ring6", capacity)); return (queue); }); - - data::ElementPtr parameters = data::Element::createMap(); - parameters->set("queue-type", data::Element::create("kea-ring6")); - parameters->set("capacity", data::Element::create(static_cast(500))); - createPacketQueue(parameters); } boost::scoped_ptr& diff --git a/src/lib/dhcp/packet_queue_mgr6.h b/src/lib/dhcp/packet_queue_mgr6.h index fb9d4e5057..a4d89301ee 100644 --- a/src/lib/dhcp/packet_queue_mgr6.h +++ b/src/lib/dhcp/packet_queue_mgr6.h @@ -56,8 +56,7 @@ public: private: /// @brief Private constructor. /// - /// It registers a default factory for DHCPv6 queues and creates - /// an default DHCPv6 packet queue. + /// It registers a default factory for DHCPv6 queues. PacketQueueMgr6(); /// @brief Returns a pointer to the currently used instance of the diff --git a/src/lib/dhcp/tests/iface_mgr_unittest.cc b/src/lib/dhcp/tests/iface_mgr_unittest.cc index 7975efbc1b..fb4114f586 100644 --- a/src/lib/dhcp/tests/iface_mgr_unittest.cc +++ b/src/lib/dhcp/tests/iface_mgr_unittest.cc @@ -645,12 +645,14 @@ TEST_F(IfaceMgrTest, clearIfaces) { TEST_F(IfaceMgrTest, packetQueue4) { NakedIfaceMgr ifacemgr; - // Get the default queue. - PacketQueue4Ptr q4 = ifacemgr.getPacketQueue4(); - ASSERT_TRUE(q4); + // Should not have a queue at start up. + ASSERT_FALSE(ifacemgr.getPacketQueue4()); - // Verify that the queue is what we expect. - checkInfo(q4, "{ \"capacity\": 500, \"queue-type\": \"kea-ring4\", \"size\": 0 }"); + // Verify that we can create a queue with default factory. + data::ConstElementPtr config = makeQueueConfig("kea-ring4", 2000); + ASSERT_NO_THROW(PacketQueueMgr4::instance().createPacketQueue(config)); + checkInfo(ifacemgr.getPacketQueue4(), + "{ \"capacity\": 2000, \"queue-type\": \"kea-ring4\", \"size\": 0 }"); // Verify that fetching the queue via IfaceMgr and PacketQueueMgr // returns the same queue. @@ -661,18 +663,20 @@ TEST_F(IfaceMgrTest, packetQueue4) { TEST_F(IfaceMgrTest, packetQueue6) { NakedIfaceMgr ifacemgr; - // Get the default queue. - PacketQueue6Ptr q6 = ifacemgr.getPacketQueue6(); + // Should not have a queue at start up. + ASSERT_FALSE(ifacemgr.getPacketQueue6()); - // Verify that we have a default queue and its info is correct. - checkInfo(q6, "{ \"capacity\": 500, \"queue-type\": \"kea-ring6\", \"size\": 0 }"); + // Verify that we can create a queue with default factory. + data::ConstElementPtr config = makeQueueConfig("kea-ring6", 2000); + ASSERT_NO_THROW(PacketQueueMgr6::instance().createPacketQueue(config)); + checkInfo(ifacemgr.getPacketQueue6(), + "{ \"capacity\": 2000, \"queue-type\": \"kea-ring6\", \"size\": 0 }"); // Verify that fetching the queue via IfaceMgr and PacketQueueMgr // returns the same queue. ASSERT_EQ(ifacemgr.getPacketQueue6(), PacketQueueMgr6::instance().getPacketQueue()); } - TEST_F(IfaceMgrTest, receiveTimeout6) { using namespace boost::posix_time; std::cout << "Testing DHCPv6 packet reception timeouts." @@ -732,7 +736,7 @@ TEST_F(IfaceMgrTest, receiveTimeout6) { TEST_F(IfaceMgrTest, receiveTimeout4) { using namespace boost::posix_time; - std::cout << "Testing DHCPv6 packet reception timeouts." + std::cout << "Testing DHCPv4 packet reception timeouts." << " Test will block for a few seconds when waiting" << " for timeout to occur." << std::endl; @@ -745,6 +749,7 @@ TEST_F(IfaceMgrTest, receiveTimeout4) { ); // Socket is open if returned value is non-negative. ASSERT_GE(socket1, 0); + // Start receiver. ASSERT_NO_THROW(ifacemgr->startDHCPReceiver(AF_INET)); @@ -780,8 +785,8 @@ TEST_F(IfaceMgrTest, receiveTimeout4) { EXPECT_LE(duration.total_microseconds(), 700000); // Test with invalid fractional timeout values. - EXPECT_THROW(ifacemgr->receive6(0, 1000000), isc::BadValue); - EXPECT_THROW(ifacemgr->receive6(2, 1000005), isc::BadValue); + EXPECT_THROW(ifacemgr->receive4(0, 1000000), isc::BadValue); + EXPECT_THROW(ifacemgr->receive4(2, 1000005), isc::BadValue); // Stop receiver. EXPECT_NO_THROW(ifacemgr->stopDHCPReceiver()); @@ -1129,7 +1134,9 @@ TEST_F(IfaceMgrTest, sendReceive4) { EXPECT_GE(socket1, 0); +#if 0 ifacemgr->startDHCPReceiver(AF_INET); +#endif boost::shared_ptr sendPkt(new Pkt4(DHCPDISCOVER, 1234) ); @@ -1213,7 +1220,9 @@ TEST_F(IfaceMgrTest, sendReceive4) { EXPECT_THROW(ifacemgr->send(sendPkt), SocketWriteError); +#if 0 ifacemgr->stopDHCPReceiver(); +#endif } // Verifies that it is possible to set custom packet filter object diff --git a/src/lib/dhcp/tests/packet_queue_mgr4_unittest.cc b/src/lib/dhcp/tests/packet_queue_mgr4_unittest.cc index 14ea873b34..34f7ea4ad2 100644 --- a/src/lib/dhcp/tests/packet_queue_mgr4_unittest.cc +++ b/src/lib/dhcp/tests/packet_queue_mgr4_unittest.cc @@ -96,23 +96,20 @@ public: }; // Verifies that DHCPv4 PQM provides a default queue factory -// and packet queue. TEST_F(PacketQueueMgr4Test, defaultQueue) { + // Should not be a queue at start-up + ASSERT_FALSE(mgr().getPacketQueue()); - // Verify that we have a default queue and its info is correct. - checkMyInfo("{ \"capacity\": 500, \"queue-type\": \"kea-ring4\", \"size\": 0 }"); - + // Verify that we can create a queue with default factory. data::ConstElementPtr config = makeQueueConfig("kea-ring4", 2000); - - // Verify that we can replace the default queue with different capacity queue ASSERT_NO_THROW(mgr().createPacketQueue(config)); checkMyInfo("{ \"capacity\": 2000, \"queue-type\": \"kea-ring4\", \"size\": 0 }"); // We should be able to recreate the manager. ASSERT_NO_THROW(PacketQueueMgr4::create()); - // And be back to having the default queue. - checkMyInfo("{ \"capacity\": 500, \"queue-type\": \"kea-ring4\", \"size\": 0 }"); + // Should not be a queue. + ASSERT_FALSE(mgr().getPacketQueue()); } // Verifies that PQM registry and creation of custome queue implementations. @@ -125,10 +122,7 @@ TEST_F(PacketQueueMgr4Test, customQueueType) { // Register our adjustable-type factory ASSERT_TRUE(addCustomQueueType("custom-queue")); - // We still have our default queue. - checkMyInfo("{ \"capacity\": 500, \"queue-type\": \"kea-ring4\", \"size\": 0 }"); - - // Verify that we can replace the default queue with a "custom-queue" queue + // Verify that we can create a custom queue. ASSERT_NO_THROW(mgr().createPacketQueue(config)); checkMyInfo("{ \"capacity\": 2000, \"queue-type\": \"custom-queue\", \"size\": 0 }"); diff --git a/src/lib/dhcp/tests/packet_queue_mgr6_unittest.cc b/src/lib/dhcp/tests/packet_queue_mgr6_unittest.cc index 36ab4a44cd..8e9c746e87 100644 --- a/src/lib/dhcp/tests/packet_queue_mgr6_unittest.cc +++ b/src/lib/dhcp/tests/packet_queue_mgr6_unittest.cc @@ -84,23 +84,20 @@ public: }; // Verifies that DHCPv6 PQM provides a default queue factory -// and packet queue. TEST_F(PacketQueueMgr6Test, defaultQueue) { + // Should not be a queue at start-up + ASSERT_FALSE(mgr().getPacketQueue()); - // Verify that we have a default queue and its info is correct. - checkMyInfo("{ \"capacity\": 500, \"queue-type\": \"kea-ring6\", \"size\": 0 }"); - + // Verify that we can create a queue with default factory. data::ConstElementPtr config = makeQueueConfig("kea-ring6", 2000); - - // Verify that we can replace the default queue with different capacity queue ASSERT_NO_THROW(mgr().createPacketQueue(config)); checkMyInfo("{ \"capacity\": 2000, \"queue-type\": \"kea-ring6\", \"size\": 0 }"); // We should be able to recreate the manager. ASSERT_NO_THROW(PacketQueueMgr6::create()); - // And be back to having the default queue. - checkMyInfo("{ \"capacity\": 500, \"queue-type\": \"kea-ring6\", \"size\": 0 }"); + // Should not be a queue. + ASSERT_FALSE(mgr().getPacketQueue()); } // Verifies that PQM registry and creation of custome queue implementations. @@ -113,10 +110,7 @@ TEST_F(PacketQueueMgr6Test, customQueueType) { // Register our adjustable-type factory ASSERT_TRUE(addCustomQueueType("custom-queue")); - // We still have our default queue. - checkMyInfo("{ \"capacity\": 500, \"queue-type\": \"kea-ring6\", \"size\": 0 }"); - - // Verify that we can replace the default queue with a "custom-queue" queue + // Verify that we can create a custom queue. ASSERT_NO_THROW(mgr().createPacketQueue(config)); checkMyInfo("{ \"capacity\": 2000, \"queue-type\": \"custom-queue\", \"size\": 0 }"); diff --git a/src/lib/dhcpsrv/cfg_iface.cc b/src/lib/dhcpsrv/cfg_iface.cc index 31e4a48cc6..951d880ee6 100644 --- a/src/lib/dhcpsrv/cfg_iface.cc +++ b/src/lib/dhcpsrv/cfg_iface.cc @@ -28,7 +28,6 @@ CfgIface::CfgIface() void CfgIface::closeSockets() const { - IfaceMgr::instance().stopDHCPReceiver(); IfaceMgr::instance().closeSockets(); } @@ -174,11 +173,7 @@ CfgIface::openSockets(const uint16_t family, const uint16_t port, sopen = IfaceMgr::instance().openSockets6(port, error_callback); } - if (sopen) { - // @todo we may consider starting/stopping this when DHCP service is - // enable/disabled, rather then when we open sockets. - IfaceMgr::instance().startDHCPReceiver(family); - } else { + if (!sopen) { // If no socket were opened, log a warning because the server will // not respond to any queries. LOG_WARN(dhcpsrv_logger, DHCPSRV_NO_SOCKETS_OPEN); diff --git a/src/lib/dhcpsrv/parsers/dhcp_queue_control_parser.cc b/src/lib/dhcpsrv/parsers/dhcp_queue_control_parser.cc index b852683ddf..728c06338a 100644 --- a/src/lib/dhcpsrv/parsers/dhcp_queue_control_parser.cc +++ b/src/lib/dhcpsrv/parsers/dhcp_queue_control_parser.cc @@ -18,15 +18,6 @@ using namespace isc::data; namespace isc { namespace dhcp { -DHCPQueueControlParser::DHCPQueueControlParser(const uint16_t family) - : family_(family) { - // @todo Not sure we need family but just in case. - if (family_ != AF_INET && family_ != AF_INET6) { - isc_throw(BadValue, "DHCPQueueControlParser - invalid family: " - << family_ << ", must be AF_INET or AF_INET6"); - } -} - data::ElementPtr DHCPQueueControlParser::parse(const isc::data::ConstElementPtr& control_elem) { // All we really do here is verify that it is a map that @@ -36,12 +27,17 @@ DHCPQueueControlParser::parse(const isc::data::ConstElementPtr& control_elem) { isc_throw(DhcpConfigError, "dhcp-queue-control must be a map"); } - ConstElementPtr elem = control_elem->get("queue-type"); - if (!elem) { - isc_throw(DhcpConfigError, "queue-type is required"); - } else { - if (elem->getType() != Element::string) { - isc_throw(DhcpConfigError, "queue-type must be a string"); + // enable-queue is mandatory. + bool enable_queue = getBoolean(control_elem, "enable-queue"); + + if (enable_queue) { + ConstElementPtr elem = control_elem->get("queue-type"); + if (!elem) { + isc_throw(DhcpConfigError, "when queue is enabled, queue-type is required"); + } else { + if (elem->getType() != Element::string) { + isc_throw(DhcpConfigError, "queue-type must be a string"); + } } } diff --git a/src/lib/dhcpsrv/parsers/dhcp_queue_control_parser.h b/src/lib/dhcpsrv/parsers/dhcp_queue_control_parser.h index 5d1fbe8ca0..663874903f 100644 --- a/src/lib/dhcpsrv/parsers/dhcp_queue_control_parser.h +++ b/src/lib/dhcpsrv/parsers/dhcp_queue_control_parser.h @@ -18,9 +18,16 @@ namespace dhcp { /// /// This parser parses the "dhcp-queue-control" parameter which holds the /// the configurable parameters that tailor DHCP packet queue behavior. -/// Currently "dhcp-queue-control" is treated as a map of arbitrary values, -/// with only one required value, "queue-type". This was done to -/// provide latitude for differing queue implementations. +/// In order to provide wide latitude to packet queue implementators, +/// 'dhcp-queue-control' is mostly treated as a map of arbitrary values. +/// There is only mandatory value, 'enable-queue', which enables/disables +/// DHCP packet queueing. If this value is true, then the content must +/// also include a value for 'queue-type'. Beyond these values, the +/// map may contain any combination of valid JSON elements. +/// +/// Unlike most other parsers, this parser primarily serves to validate +/// the aforementioned rules, and rather than instantiate an object as +/// a result, it simply returns a copy original map of elements. /// /// This parser is used in both DHCPv4 and DHCPv6. Derived parsers /// are not needed. @@ -29,22 +36,19 @@ public: /// @brief Constructor /// - /// @param family AF_INET for DHCPv4 and AF_INET6 for DHCPv6. - explicit DHCPQueueControlParser(const uint16_t family); + DHCPQueueControlParser(){}; /// @brief Parses content of the "dhcp-queue-control". /// - /// @param values pointer to the content of parsed values + /// @param control_elem MapElement containing the queue control values to + /// parse /// - /// @return A pointer to a newly constructed DHCPQueueControl populated - /// with the parsed values + /// @return A copy of the of the input MapElement. /// /// @throw DhcpConfigError if any of the values are invalid. - data::ElementPtr parse(const isc::data::ConstElementPtr& values); + data::ElementPtr parse(const isc::data::ConstElementPtr& control_elem); private: - /// @brief AF_INET for DHCPv4 and AF_INET6 for DHCPv6. - int family_; }; } diff --git a/src/lib/dhcpsrv/tests/Makefile.am b/src/lib/dhcpsrv/tests/Makefile.am index 644a4716c1..ea30468e7a 100644 --- a/src/lib/dhcpsrv/tests/Makefile.am +++ b/src/lib/dhcpsrv/tests/Makefile.am @@ -85,6 +85,7 @@ libdhcpsrv_unittests_SOURCES += csv_lease_file6_unittest.cc libdhcpsrv_unittests_SOURCES += d2_client_unittest.cc libdhcpsrv_unittests_SOURCES += d2_udp_unittest.cc libdhcpsrv_unittests_SOURCES += database_connection_unittest.cc +libdhcpsrv_unittests_SOURCES += dhcp_queue_control_parser_unittest.cc libdhcpsrv_unittests_SOURCES += dhcp4o6_ipc_unittest.cc libdhcpsrv_unittests_SOURCES += duid_config_parser_unittest.cc libdhcpsrv_unittests_SOURCES += expiration_config_parser_unittest.cc diff --git a/src/lib/dhcpsrv/tests/dhcp_queue_control_parser_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_queue_control_parser_unittest.cc new file mode 100644 index 0000000000..2aae12b08d --- /dev/null +++ b/src/lib/dhcpsrv/tests/dhcp_queue_control_parser_unittest.cc @@ -0,0 +1,171 @@ +// Copyright (C) 2018 Internet Systems Consortium, Inc. ("ISC") +// +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +#include + +#include +#include +#include +#include +#include + +using namespace isc::data; +using namespace isc::dhcp; +using namespace isc::test; + +namespace { + +/// @brief Test fixture class for @c DHCPQueueControlParser +class DHCPQueueControlParserTest : public ::testing::Test { +protected: + + /// @brief Setup for each test. + /// + /// Clears the configuration in the @c CfgMgr. + virtual void SetUp(); + + /// @brief Cleans up after each test. + /// + /// Clears the configuration in the @c CfgMgr. + virtual void TearDown(); + +}; + +void +DHCPQueueControlParserTest::SetUp() { + CfgMgr::instance().clear(); +} + +void +DHCPQueueControlParserTest::TearDown() { + CfgMgr::instance().clear(); +} + +// Verifies that DHCPQueueControlParser handles +// expected valid dhcp-queue-control contet +TEST_F(DHCPQueueControlParserTest, validContent) { + struct Scenario { + std::string description_; + std::string json_; + }; + + std::vector scenarios = { + { + "queue disabled", + "{ \n" + " \"enable-queue\": false \n" + "} \n" + }, + { + "queue disabled, arbitrary content allowed", + "{ \n" + " \"enable-queue\": false, \n" + " \"foo\": \"bogus\", \n" + " \"random-int\" : 1234 \n" + "} \n" + }, + { + "queue enabled, with queue-type", + "{ \n" + " \"enable-queue\": true, \n" + " \"queue-type\": \"some-type\" \n" + "} \n" + }, + { + "queue enabled with queue-type and arbitrary content", + "{ \n" + " \"enable-queue\": true, \n" + " \"queue-type\": \"some-type\", \n" + " \"foo\": \"bogus\", \n" + " \"random-int\" : 1234 \n" + "} \n" + } + }; + + // Iterate over the valid scenarios and verify they succeed. + ConstElementPtr config_elems; + ConstElementPtr queue_control; + for (auto scenario : scenarios) { + SCOPED_TRACE(scenario.description_); + { + // Construct the config JSON + ASSERT_NO_THROW(config_elems = Element::fromJSON(scenario.json_)) + << "invalid JSON, test is broken"; + + // Parsing config into a queue control should succeed. + DHCPQueueControlParser parser; + try { + queue_control = parser.parse(config_elems); + } catch (const std::exception& ex) { + ADD_FAILURE() << "parser threw an exception: " << ex.what(); + } + + // Verify the resultant queue control. + ASSERT_TRUE(queue_control); + + // The parser should have created a duplicate of the + // configuration elements. + ASSERT_TRUE(queue_control.get() != config_elems.get()); + EXPECT_TRUE(queue_control->equals(*config_elems)); + } + } +} + +// Verifies that DHCPQueueControlParser correctly catches +// invalid dhcp-queue-control content +TEST_F(DHCPQueueControlParserTest, invalidContent) { + struct Scenario { + std::string description_; + std::string json_; + }; + + std::vector scenarios = { + { + "enable-queue missing", + "{ \n" + " \"enable-type\": \"some-type\" \n" + "} \n" + }, + { + "enable-queue not boolean", + "{ \n" + " \"enable-queue\": \"always\" \n" + "} \n" + }, + { + "queue enabled, type missing", + "{ \n" + " \"enable-queue\": true \n" + "} \n" + }, + { + "queue enabled, type not a string", + "{ \n" + " \"enable-queue\": true, \n" + " \"queue-type\": 7777 \n" + "} \n" + } + }; + + // Iterate over the valid scenarios and verify they succeed. + ConstElementPtr config_elems; + ConstElementPtr queue_control; + for (auto scenario : scenarios) { + SCOPED_TRACE(scenario.description_); + { + // Construct the config JSON + ASSERT_NO_THROW(config_elems = Element::fromJSON(scenario.json_)) + << "invalid JSON, test is broken"; + + // Parsing config into a queue control should succeed. + DHCPQueueControlParser parser; + EXPECT_THROW(parser.parse(config_elems), DhcpConfigError); + } + } +} + + +}; // anonymous namespace