From: Marcin Siodelski Date: Thu, 6 Feb 2014 17:10:07 +0000 (+0100) Subject: [3242] Addressed review comments. X-Git-Tag: bind10-1.2.0beta1-release~63^2~4 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=f22c490bd4a194ad693e29a8e9f65a0480405e5c;p=thirdparty%2Fkea.git [3242] Addressed review comments. --- diff --git a/src/bin/dhcp4/dhcp4_messages.mes b/src/bin/dhcp4/dhcp4_messages.mes index b2a8adb3e2..b9185d8f09 100644 --- a/src/bin/dhcp4/dhcp4_messages.mes +++ b/src/bin/dhcp4/dhcp4_messages.mes @@ -135,6 +135,11 @@ A "libreload" command was issued to reload the hooks libraries but for some reason the reload failed. Other error messages issued from the hooks framework will indicate the nature of the problem. +% DHCP4_UNRECOGNIZED_RCVD_PACKET_TYPE received message (transaction id %1) has unregonized type %2 +This debug message indicates that the message type carried in DHCPv4 option +53 is unrecognized by the server. The valid message types are available +on the IANA website. The message will not be processed by the server. + % DHCP4_LEASE_ADVERT lease %1 advertised (client client-id %2, hwaddr %3) This debug message indicates that the server successfully advertised a lease. It is up to the client to choose one server out of othe advertised diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index abd9405e2f..e6cb2522ac 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -266,7 +266,7 @@ Dhcpv4Srv::run() { continue; } - // We have sanity checked (in accept()) that the Message Type option + // We have sanity checked (in accept() that the Message Type option // exists, so we can safely get it here. int type = query->getType(); LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PACKET_RECEIVED) @@ -1573,15 +1573,12 @@ Dhcpv4Srv::accept(const Pkt4Ptr& query) const { return (false); } - // When receiving a packet without message type option, getType() will - // throw. - try { - query->getType(); - } catch (...) { - LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PACKET_DROP_NO_TYPE) - .arg(query->getIface()); + // Check that the message type is accepted by the server. We rely on the + // function called to log a message if needed. + if (!acceptMessageType(query)) { return (false); } + return (true); } @@ -1595,11 +1592,51 @@ Dhcpv4Srv::acceptDirectRequest(const Pkt4Ptr& pkt) const { return (false); } static const IOAddress bcast("255.255.255.255"); - return ((pkt->getLocalAddr() == bcast && !selectSubnet(pkt)) ? false : true); + return ((pkt->getLocalAddr() != bcast || selectSubnet(pkt))); +} + +bool +Dhcpv4Srv::acceptMessageType(const Pkt4Ptr& query) const { + // When receiving a packet without message type option, getType() will + // throw. + int type; + try { + type = query->getType(); + + } catch (...) { + LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, DHCP4_PACKET_DROP_NO_TYPE) + .arg(query->getIface()); + return (false); + } + + // If we receive a message with a non-existing type, we are logging it. + if (type > DHCPLEASEQUERYDONE) { + LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL, + DHCP4_UNRECOGNIZED_RCVD_PACKET_TYPE) + .arg(type) + .arg(query->getTransid()); + return (false); + } + + // Once we know that the message type is within a range of defined DHCPv4 + // messages, we do a detailed check to make sure that the received message + // is targeted at server. Note that we could have received some Offer + // message broadcasted by the other server to a relay. Even though, the + // server would rather unicast its response to a relay, let's be on the + // safe side. Also, we want to drop other messages which we don't support. + // All these valid messages that we are not going to process are dropped + // silently. + if ((type != DHCPDISCOVER) && (type != DHCPREQUEST) && + (type != DHCPRELEASE) && (type != DHCPDECLINE) && + (type != DHCPINFORM)) { + return (false); + } + + return (true); } bool -Dhcpv4Srv::acceptServerId(const Pkt4Ptr& pkt) const { +Dhcpv4Srv::acceptServerId(const Pkt4Ptr& query) const { // This function is meant to be called internally by the server class, so // we rely on the caller to sanity check the pointer and we don't check // it here. @@ -1609,7 +1646,7 @@ Dhcpv4Srv::acceptServerId(const Pkt4Ptr& pkt) const { // Note that we don't check cases that server identifier is mandatory // but not present. This is meant to be sanity checked in other // functions. - OptionPtr option = pkt->getOption(DHO_DHCP_SERVER_IDENTIFIER); + OptionPtr option = query->getOption(DHO_DHCP_SERVER_IDENTIFIER); if (!option) { return (true); } diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index 3649ce1eed..087120cb96 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/src/bin/dhcp4/dhcp4_srv.h @@ -167,6 +167,13 @@ public: protected: + /// @name Functions filtering and sanity-checking received messages. + /// + /// @todo These functions are supposed to be moved to a new class which + /// will manage different rules for accepting and rejecting messages. + /// Perhaps ticket #3116 is a good opportunity to do it. + /// + //@{ /// @brief Checks whether received message should be processed or discarded. /// /// This function checks whether received message should be processed or @@ -211,11 +218,31 @@ protected: /// - all broadcast messages received on the interface for which the /// suitable subnet exists (is configured). /// - /// @param pkt Message sent by a client. + /// @param query Message sent by a client. /// /// @return true if message is accepted for further processing, false /// otherwise. - bool acceptDirectRequest(const Pkt4Ptr& pkt) const; + bool acceptDirectRequest(const Pkt4Ptr& query) const; + + /// @brief Check if received message type is valid for the server to + /// process. + /// + /// This function checks that the received message type belongs to the range + /// of types regonized by the server and that the message of this type + /// should be processed by the server. + /// + /// The messages types accepted for processing are: + /// - Discover + /// - Request + /// - Release + /// - Decline + /// - Inform + /// + /// @param query Message sent by a client. + /// + /// @return true if message is accepted for further processing, false + /// otherwise. + bool acceptMessageType(const Pkt4Ptr& query) const; /// @brief Verifies if the server id belongs to our server. /// @@ -231,6 +258,7 @@ protected: /// @return true, if the server identifier is absent or matches one of the /// server identifiers that the server is using; false otherwise. bool acceptServerId(const Pkt4Ptr& pkt) const; + //@} /// @brief verifies if specified packet meets RFC requirements /// diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index 2b00c24820..b8f6a24c6b 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc @@ -3361,4 +3361,46 @@ TEST_F(Dhcpv4SrvTest, acceptDirectRequest) { } +// This test checks that the server rejects a message with invalid type. +TEST_F(Dhcpv4SrvTest, acceptMessageType) { + IfaceMgrTestConfig test_config(true); + IfaceMgr::instance().openSockets4(); + + NakedDhcpv4Srv srv(0); + + // Specify messages to be accepted by the server. + int allowed[] = { + DHCPDISCOVER, + DHCPREQUEST, + DHCPRELEASE, + DHCPDECLINE, + DHCPINFORM + }; + size_t allowed_size = sizeof(allowed) / sizeof(allowed[0]); + // Check that the server actually accepts these message types. + for (int i = 0; i < allowed_size; ++i) { + EXPECT_TRUE(srv.acceptMessageType(Pkt4Ptr(new Pkt4(allowed[i], 1234)))) + << "Test failed for message type " << i; + } + // Specify messages which server is supposed to drop. + int not_allowed[] = { + DHCPOFFER, + DHCPACK, + DHCPNAK, + DHCPLEASEQUERY, + DHCPLEASEUNASSIGNED, + DHCPLEASEUNKNOWN, + DHCPLEASEACTIVE, + DHCPBULKLEASEQUERY, + DHCPLEASEQUERYDONE + }; + size_t not_allowed_size = sizeof(not_allowed) / sizeof(not_allowed[0]); + // Actually check that the server will drop these messages. + for (int i = 0; i < not_allowed_size; ++i) { + EXPECT_FALSE(srv.acceptMessageType(Pkt4Ptr(new Pkt4(not_allowed[i], + 1234)))) + << "Test failed for message type " << i; + } +} + }; // end of anonymous namespace diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.cc b/src/bin/dhcp4/tests/dhcp4_test_utils.cc index 9d0f344392..ab1afc642f 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.cc +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.cc @@ -1,4 +1,4 @@ -// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2014 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 @@ -15,7 +15,9 @@ #include #include +#include #include +#include #include #include #include @@ -29,14 +31,14 @@ using namespace std; using namespace isc::asiolink; - +using namespace isc::data; namespace isc { namespace dhcp { namespace test { Dhcpv4SrvTest::Dhcpv4SrvTest() -:rcode_(-1) { +:rcode_(-1), srv_(0) { subnet_ = Subnet4Ptr(new Subnet4(IOAddress("192.0.2.0"), 24, 1000, 2000, 3000)); pool_ = Pool4Ptr(new Pool4(IOAddress("192.0.2.100"), IOAddress("192.0.2.110"))); @@ -553,6 +555,20 @@ Dhcpv4SrvTest::testDiscoverRequest(const uint8_t msg_type) { EXPECT_TRUE(noBasicOptions(rsp)); } +void +Dhcpv4SrvTest::configure(const std::string& config) { + ElementPtr json = Element::fromJSON(config); + ConstElementPtr status; + + // Configure the server and make sure the config is accepted + EXPECT_NO_THROW(status = configureDhcp4Server(srv_, json)); + ASSERT_TRUE(status); + int rcode; + ConstElementPtr comment = config::parseAnswer(rcode, status); + ASSERT_EQ(0, rcode); +} + + }; // end of isc::dhcp::test namespace }; // end of isc::dhcp namespace }; // end of isc namespace diff --git a/src/bin/dhcp4/tests/dhcp4_test_utils.h b/src/bin/dhcp4/tests/dhcp4_test_utils.h index 69e13f0c27..3f7c326a75 100644 --- a/src/bin/dhcp4/tests/dhcp4_test_utils.h +++ b/src/bin/dhcp4/tests/dhcp4_test_utils.h @@ -87,6 +87,120 @@ public: typedef boost::shared_ptr PktFilterTestPtr; +/// @brief "Naked" DHCPv4 server, exposes internal fields +class NakedDhcpv4Srv: public Dhcpv4Srv { +public: + + /// @brief Constructor. + /// + /// This constructor disables default modes of operation used by the + /// Dhcpv4Srv class: + /// - Send/receive broadcast messages through sockets on interfaces + /// which support broadcast traffic. + /// - Direct DHCPv4 traffic - communication with clients which do not + /// have IP address assigned yet. + /// + /// Enabling these modes requires root privilges so they must be + /// disabled for unit testing. + /// + /// Note, that disabling broadcast options on sockets does not impact + /// the operation of these tests because they use local loopback + /// interface which doesn't have broadcast capability anyway. It rather + /// prevents setting broadcast options on other (broadcast capable) + /// sockets which are opened on other interfaces in Dhcpv4Srv constructor. + /// + /// The Direct DHCPv4 Traffic capability can be disabled here because + /// it is tested with PktFilterLPFTest unittest. The tests which belong + /// to PktFilterLPFTest can be enabled on demand when root privileges can + /// be guaranteed. + /// + /// @param port port number to listen on; the default value 0 indicates + /// that sockets should not be opened. + NakedDhcpv4Srv(uint16_t port = 0) + : Dhcpv4Srv(port, "type=memfile", false, false) { + // Create fixed server id. + server_id_.reset(new Option4AddrLst(DHO_DHCP_SERVER_IDENTIFIER, + asiolink::IOAddress("192.0.3.1"))); + } + + /// @brief Returns fixed server identifier assigned to the naked server + /// instance. + OptionPtr getServerID() const { + return (server_id_); + } + + /// @brief fakes packet reception + /// @param timeout ignored + /// + /// The method receives all packets queued in receive queue, one after + /// another. Once the queue is empty, it initiates the shutdown procedure. + /// + /// See fake_received_ field for description + virtual Pkt4Ptr receivePacket(int /*timeout*/) { + + // If there is anything prepared as fake incoming traffic, use it + if (!fake_received_.empty()) { + Pkt4Ptr pkt = fake_received_.front(); + fake_received_.pop_front(); + return (pkt); + } + + // If not, just trigger shutdown and return immediately + shutdown(); + return (Pkt4Ptr()); + } + + /// @brief fake packet sending + /// + /// Pretend to send a packet, but instead just store it in fake_send_ list + /// where test can later inspect server's response. + virtual void sendPacket(const Pkt4Ptr& pkt) { + fake_sent_.push_back(pkt); + } + + /// @brief adds a packet to fake receive queue + /// + /// See fake_received_ field for description + void fakeReceive(const Pkt4Ptr& pkt) { + fake_received_.push_back(pkt); + } + + virtual ~NakedDhcpv4Srv() { + } + + /// @brief Dummy server identifier option used by various tests. + OptionPtr server_id_; + + /// @brief packets we pretend to receive + /// + /// Instead of setting up sockets on interfaces that change between OSes, it + /// is much easier to fake packet reception. This is a list of packets that + /// we pretend to have received. You can schedule new packets to be received + /// using fakeReceive() and NakedDhcpv4Srv::receivePacket() methods. + std::list fake_received_; + + std::list fake_sent_; + + using Dhcpv4Srv::adjustIfaceData; + using Dhcpv4Srv::appendServerID; + using Dhcpv4Srv::processDiscover; + using Dhcpv4Srv::processRequest; + using Dhcpv4Srv::processRelease; + using Dhcpv4Srv::processDecline; + using Dhcpv4Srv::processInform; + using Dhcpv4Srv::processClientName; + using Dhcpv4Srv::computeDhcid; + using Dhcpv4Srv::createNameChangeRequests; + using Dhcpv4Srv::acceptServerId; + using Dhcpv4Srv::sanityCheck; + using Dhcpv4Srv::srvidToString; + using Dhcpv4Srv::unpackOptions; + using Dhcpv4Srv::name_change_reqs_; + using Dhcpv4Srv::classifyPacket; + using Dhcpv4Srv::accept; + using Dhcpv4Srv::acceptMessageType; +}; + class Dhcpv4SrvTest : public ::testing::Test { public: @@ -280,6 +394,11 @@ public: /// @param msg_type DHCPDISCOVER or DHCPREQUEST void testDiscoverRequest(const uint8_t msg_type); + /// @brief Runs DHCPv4 configuration from the JSON string. + /// + /// @param config String holding server configuration in JSON format. + void configure(const std::string& config); + /// @brief This function cleans up after the test. virtual void TearDown(); @@ -296,119 +415,9 @@ public: isc::data::ConstElementPtr comment_; -}; - -/// @brief "Naked" DHCPv4 server, exposes internal fields -class NakedDhcpv4Srv: public Dhcpv4Srv { -public: + /// @brief Server object under test. + NakedDhcpv4Srv srv_; - /// @brief Constructor. - /// - /// This constructor disables default modes of operation used by the - /// Dhcpv4Srv class: - /// - Send/receive broadcast messages through sockets on interfaces - /// which support broadcast traffic. - /// - Direct DHCPv4 traffic - communication with clients which do not - /// have IP address assigned yet. - /// - /// Enabling these modes requires root privilges so they must be - /// disabled for unit testing. - /// - /// Note, that disabling broadcast options on sockets does not impact - /// the operation of these tests because they use local loopback - /// interface which doesn't have broadcast capability anyway. It rather - /// prevents setting broadcast options on other (broadcast capable) - /// sockets which are opened on other interfaces in Dhcpv4Srv constructor. - /// - /// The Direct DHCPv4 Traffic capability can be disabled here because - /// it is tested with PktFilterLPFTest unittest. The tests which belong - /// to PktFilterLPFTest can be enabled on demand when root privileges can - /// be guaranteed. - /// - /// @param port port number to listen on; the default value 0 indicates - /// that sockets should not be opened. - NakedDhcpv4Srv(uint16_t port = 0) - : Dhcpv4Srv(port, "type=memfile", false, false) { - // Create fixed server id. - server_id_.reset(new Option4AddrLst(DHO_DHCP_SERVER_IDENTIFIER, - asiolink::IOAddress("192.0.3.1"))); - } - - /// @brief Returns fixed server identifier assigned to the naked server - /// instance. - OptionPtr getServerID() const { - return (server_id_); - } - - /// @brief fakes packet reception - /// @param timeout ignored - /// - /// The method receives all packets queued in receive queue, one after - /// another. Once the queue is empty, it initiates the shutdown procedure. - /// - /// See fake_received_ field for description - virtual Pkt4Ptr receivePacket(int /*timeout*/) { - - // If there is anything prepared as fake incoming traffic, use it - if (!fake_received_.empty()) { - Pkt4Ptr pkt = fake_received_.front(); - fake_received_.pop_front(); - return (pkt); - } - - // If not, just trigger shutdown and return immediately - shutdown(); - return (Pkt4Ptr()); - } - - /// @brief fake packet sending - /// - /// Pretend to send a packet, but instead just store it in fake_send_ list - /// where test can later inspect server's response. - virtual void sendPacket(const Pkt4Ptr& pkt) { - fake_sent_.push_back(pkt); - } - - /// @brief adds a packet to fake receive queue - /// - /// See fake_received_ field for description - void fakeReceive(const Pkt4Ptr& pkt) { - fake_received_.push_back(pkt); - } - - virtual ~NakedDhcpv4Srv() { - } - - /// @brief Dummy server identifier option used by various tests. - OptionPtr server_id_; - - /// @brief packets we pretend to receive - /// - /// Instead of setting up sockets on interfaces that change between OSes, it - /// is much easier to fake packet reception. This is a list of packets that - /// we pretend to have received. You can schedule new packets to be received - /// using fakeReceive() and NakedDhcpv4Srv::receivePacket() methods. - std::list fake_received_; - - std::list fake_sent_; - - using Dhcpv4Srv::adjustIfaceData; - using Dhcpv4Srv::appendServerID; - using Dhcpv4Srv::processDiscover; - using Dhcpv4Srv::processRequest; - using Dhcpv4Srv::processRelease; - using Dhcpv4Srv::processDecline; - using Dhcpv4Srv::processInform; - using Dhcpv4Srv::processClientName; - using Dhcpv4Srv::computeDhcid; - using Dhcpv4Srv::createNameChangeRequests; - using Dhcpv4Srv::acceptServerId; - using Dhcpv4Srv::sanityCheck; - using Dhcpv4Srv::srvidToString; - using Dhcpv4Srv::unpackOptions; - using Dhcpv4Srv::name_change_reqs_; - using Dhcpv4Srv::classifyPacket; - using Dhcpv4Srv::accept; }; }; // end of isc::dhcp::test namespace diff --git a/src/bin/dhcp4/tests/direct_client_unittest.cc b/src/bin/dhcp4/tests/direct_client_unittest.cc index 694c848fb4..9a9dbaee17 100644 --- a/src/bin/dhcp4/tests/direct_client_unittest.cc +++ b/src/bin/dhcp4/tests/direct_client_unittest.cc @@ -12,7 +12,6 @@ // OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR // PERFORMANCE OF THIS SOFTWARE. -#include #include #include #include @@ -50,7 +49,9 @@ public: /// a specified prefix. /// /// The subnet parameters (such as options, timers etc.) are aribitrarily - /// selected. The subnet and pool mask is always /24. + /// selected. The subnet and pool mask is always /24. The real configuration + /// would exclude .0 (network address) and .255 (broadcast address), but we + /// ignore that fact for the sake of test simplicity. /// /// @param prefix Prefix for a subnet. void configureSubnet(const std::string& prefix); @@ -59,7 +60,9 @@ public: /// /// This function configures DHCPv4 server with two different subnets. /// The subnet parameters (such as options, timers etc.) are aribitrarily - /// selected. The subnet and pool mask is /24. + /// selected. The subnet and pool mask is /24. The real configuration + /// would exclude .0 (network address) and .255 (broadcast address), but we + /// ignore that fact for the sake of test simplicity. /// /// @param prefix1 Prefix of the first subnet to be configured. /// @param prefix2 Prefix of the second subnet to be configured. @@ -98,18 +101,9 @@ public: /// @return Configured and parsed message. Pkt4Ptr createClientMessage(const Pkt4Ptr &msg, const std::string& iface); - /// @brief Runs DHCPv4 configuration from the JSON string. - /// - /// @param config String holding server configuration in JSON format. - void configure(const std::string& config); - - /// @brief Server object to be unit tested. - NakedDhcpv4Srv srv_; - }; -DirectClientTest::DirectClientTest() - : srv_(0) { +DirectClientTest::DirectClientTest() : Dhcpv4SrvTest() { } void @@ -187,19 +181,6 @@ DirectClientTest::createClientMessage(const Pkt4Ptr& msg, return (received); } -void -DirectClientTest::configure(const std::string& config) { - ElementPtr json = Element::fromJSON(config); - ConstElementPtr status; - - // Configure the server and make sure the config is accepted - EXPECT_NO_THROW(status = configureDhcp4Server(srv_, json)); - ASSERT_TRUE(status); - int rcode; - ConstElementPtr comment = config::parseAnswer(rcode, status); - ASSERT_EQ(0, rcode); -} - // This test checks that the message from directly connected client // is processed and that client is offered IPv4 address from the subnet which // is suitable for the local interface on which the client's message is diff --git a/src/lib/dhcp/dhcp4.h b/src/lib/dhcp/dhcp4.h index 392478ae80..2b2440512c 100644 --- a/src/lib/dhcp/dhcp4.h +++ b/src/lib/dhcp/dhcp4.h @@ -1,5 +1,5 @@ /* - * Copyright (c) 2004-2011 by Internet Systems Consortium, Inc. ("ISC") + * Copyright (c) 2004-2011, 2014 by Internet Systems Consortium, Inc. ("ISC") * Copyright (c) 1995-2003 by Internet Software Consortium * * Permission to use, copy, modify, and distribute this software for any @@ -154,7 +154,9 @@ enum DHCPMessageType { DHCPLEASEQUERY = 10, DHCPLEASEUNASSIGNED = 11, DHCPLEASEUNKNOWN = 12, - DHCPLEASEACTIVE = 13 + DHCPLEASEACTIVE = 13, + DHCPBULKLEASEQUERY = 14, + DHCPLEASEQUERYDONE = 15 }; static const uint16_t DHCP4_CLIENT_PORT = 68; diff --git a/src/lib/dhcp/pkt4.cc b/src/lib/dhcp/pkt4.cc index b641a03a40..fa0e0f9935 100644 --- a/src/lib/dhcp/pkt4.cc +++ b/src/lib/dhcp/pkt4.cc @@ -396,6 +396,7 @@ Pkt4::DHCPTypeToBootpType(uint8_t dhcpType) { case DHCPRELEASE: case DHCPINFORM: case DHCPLEASEQUERY: + case DHCPBULKLEASEQUERY: return (BOOTREQUEST); case DHCPACK: @@ -404,6 +405,7 @@ Pkt4::DHCPTypeToBootpType(uint8_t dhcpType) { case DHCPLEASEUNASSIGNED: case DHCPLEASEUNKNOWN: case DHCPLEASEACTIVE: + case DHCPLEASEQUERYDONE: return (BOOTREPLY); default: diff --git a/src/lib/dhcp/tests/Makefile.am b/src/lib/dhcp/tests/Makefile.am index 0f2ea1bcbb..d41246a51f 100644 --- a/src/lib/dhcp/tests/Makefile.am +++ b/src/lib/dhcp/tests/Makefile.am @@ -25,6 +25,13 @@ TESTS_ENVIRONMENT = \ TESTS = if HAVE_GTEST +# Creates a library which is shared by various unit tests which require +# configuration of fake interfaces. +# The libdhcp++ does not link with this library because this would cause +# build failures being a result of concurrency between build of this +# library and the unit tests when make -j option was used, as they +# are built out of the same makefile. Instead, the libdhcp++ tests link to +# files belonging to this library, directly. noinst_LTLIBRARIES = libdhcptest.la libdhcptest_la_SOURCES = iface_mgr_test_config.cc iface_mgr_test_config.h @@ -39,6 +46,7 @@ TESTS += libdhcp++_unittests libdhcp___unittests_SOURCES = run_unittests.cc libdhcp___unittests_SOURCES += hwaddr_unittest.cc libdhcp___unittests_SOURCES += iface_mgr_unittest.cc +libdhcp___unittests_SOURCES += iface_mgr_test_config.cc iface_mgr_test_config.h libdhcp___unittests_SOURCES += libdhcp++_unittest.cc libdhcp___unittests_SOURCES += option4_addrlst_unittest.cc libdhcp___unittests_SOURCES += option4_client_fqdn_unittest.cc @@ -61,6 +69,7 @@ libdhcp___unittests_SOURCES += pkt6_unittest.cc libdhcp___unittests_SOURCES += pkt_filter_unittest.cc libdhcp___unittests_SOURCES += pkt_filter_inet_unittest.cc libdhcp___unittests_SOURCES += pkt_filter_inet6_unittest.cc +libdhcp___unittests_SOURCES += pkt_filter_test_stub.cc pkt_filter_test_stub.h libdhcp___unittests_SOURCES += pkt_filter_test_utils.h pkt_filter_test_utils.cc libdhcp___unittests_SOURCES += pkt_filter6_test_utils.h pkt_filter6_test_utils.cc @@ -84,7 +93,6 @@ libdhcp___unittests_CXXFLAGS += -Wno-unused-variable -Wno-unused-parameter endif libdhcp___unittests_LDADD = $(top_builddir)/src/lib/dhcp/libb10-dhcp++.la -libdhcp___unittests_LDADD += $(top_builddir)/src/lib/dhcp/tests/libdhcptest.la libdhcp___unittests_LDADD += $(top_builddir)/src/lib/log/libb10-log.la libdhcp___unittests_LDADD += $(top_builddir)/src/lib/util/libb10-util.la libdhcp___unittests_LDADD += $(top_builddir)/src/lib/asiolink/libb10-asiolink.la diff --git a/src/lib/dhcp/tests/iface_mgr_test_config.cc b/src/lib/dhcp/tests/iface_mgr_test_config.cc index ca219ed74d..8da7e6ed6a 100644 --- a/src/lib/dhcp/tests/iface_mgr_test_config.cc +++ b/src/lib/dhcp/tests/iface_mgr_test_config.cc @@ -98,7 +98,7 @@ IfaceMgrTestConfig::createIfaces() { // eth1 addIface("eth1", 2); addAddress("eth1", IOAddress("192.0.2.3")); - addAddress("eth1", IOAddress("fe80::3a60:77ff:fed5:cdef")); + addAddress("eth1", IOAddress("fe80::3a60:77ff:fed5:abcd")); } diff --git a/src/lib/dhcp/tests/iface_mgr_test_config.h b/src/lib/dhcp/tests/iface_mgr_test_config.h index bbf70461f8..a2ceba525c 100644 --- a/src/lib/dhcp/tests/iface_mgr_test_config.h +++ b/src/lib/dhcp/tests/iface_mgr_test_config.h @@ -26,6 +26,35 @@ namespace test { /// /// @name Set of structures describing interface flags. /// +/// These flags encapsulate the boolean type to pass the flags values +/// to @c IfaceMgrTestConfig methods. If the values passed to these methods +/// were not encapsulated by the types defined here, the API would become +/// prone to errors like swapping parameters being passed to specific functions. +/// For example, in the call to @c IfaceMgrTestConfig::setIfaceFlags: +/// @code +/// IfaceMgrTestConfig test_config(true); +/// test_config.setIfaceFlags("eth1", false, false, true, false, false); +/// @endcode +/// +/// it is quite likely that the developer by mistake swaps the values and +/// assigns them to wrong flags. When the flags are encapsulated with dedicated +/// structs, the compiler will return an error if values are swapped. For +/// example: +/// @code +/// IfaceMgrTestConfig test_config(true); +/// test_config.setIfaceFlags("eth1", FlagLoopback(false), FlagUp(false), +/// FlagRunning(true), FlagInactive4(false), +/// FlagInactive6(false)); +/// @endcode +/// will succeed, but the following code will result in the compilation error +/// and thus protect a developer from making an error: +/// @code +/// IfaceMgrTestConfig test_config(true); +/// test_config.setIfaceFlags("eth1", FlagLoopback(false), +/// FlagRunning(true), FlagUp(false), +/// FlagInactive4(false), FlagInactive6(false)); +/// @endcode +/// //@{ /// @brief Structure describing the loopback interface flag. struct FlagLoopback { @@ -84,6 +113,25 @@ struct FlagInactive6 { /// /// This class provides a set of convenience functions that should be called /// by unit tests to configure the @c IfaceMgr with fake interfaces. +/// +/// The class allows the caller to create custom fake interfaces (with custom +/// IPv4 and IPv6 addresses, flags etc.), but it also provides a default +/// test configuration for interfaces as follows: +/// - lo +/// - 127.0.0.1 +/// - ::1 +/// - eth0 +/// - 10.0.0.1 +/// - fe80::3a60:77ff:fed5:cdef +/// - 2001:db8:1::1 +/// - eth1 +/// - 192.0.2.3 +/// - fe80::3a60:77ff:fed5:abcd +/// +/// For all interfaces the following flags are set: +/// - multicast +/// - up +/// - running class IfaceMgrTestConfig : public boost::noncopyable { public: @@ -106,7 +154,7 @@ public: /// /// @param iface_name Name of the interface on which new address should /// be configured. - /// @param IPv4 or IPv6 address to be configured on the interface. + /// @param address IPv4 or IPv6 address to be configured on the interface. void addAddress(const std::string& iface_name, const asiolink::IOAddress& address); @@ -165,6 +213,7 @@ public: /// /// This function configures interface with new values for flags. /// + /// @param name Interface name. /// @param loopback Specifies if interface is a loopback interface. /// @param up Specifies if the interface is up. /// @param running Specifies if the interface is running. diff --git a/src/lib/dhcp/tests/pkt_filter_test_stub.h b/src/lib/dhcp/tests/pkt_filter_test_stub.h index db42b07f3a..f8c6130f11 100644 --- a/src/lib/dhcp/tests/pkt_filter_test_stub.h +++ b/src/lib/dhcp/tests/pkt_filter_test_stub.h @@ -65,7 +65,9 @@ public: /// fallback socket descriptor is set to a negative value. virtual SocketInfo openSocket(const Iface& iface, const isc::asiolink::IOAddress& addr, - const uint16_t port, const bool, const bool); + const uint16_t port, + const bool receive_bcast, + const bool send_bcast); /// @brief Simulate reception of the DHCPv4 message. /// diff --git a/src/lib/dhcp/tests/pkt_filter_test_utils.h b/src/lib/dhcp/tests/pkt_filter_test_utils.h index b2320cfc4a..2ce9dd5cca 100644 --- a/src/lib/dhcp/tests/pkt_filter_test_utils.h +++ b/src/lib/dhcp/tests/pkt_filter_test_utils.h @@ -1,4 +1,4 @@ -// Copyright (C) 2013 Internet Systems Consortium, Inc. ("ISC") +// Copyright (C) 2013-2014 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 @@ -129,7 +129,9 @@ public: /// fallback socket descriptor is set to a negative value. virtual SocketInfo openSocket(const Iface& iface, const isc::asiolink::IOAddress& addr, - const uint16_t port, const bool, const bool); + const uint16_t port, + const bool receive_bcast, + const bool send_bcast); /// @brief Simulate reception of the DHCPv4 message. /// diff --git a/src/lib/dhcpsrv/cfgmgr.cc b/src/lib/dhcpsrv/cfgmgr.cc index b6959db37f..d8466582e9 100644 --- a/src/lib/dhcpsrv/cfgmgr.cc +++ b/src/lib/dhcpsrv/cfgmgr.cc @@ -236,7 +236,7 @@ CfgMgr::getSubnet4(const std::string& iface_name) const { " doesn't exist and therefore it is impossible" " to find a suitable subnet for its IPv4 address"); } - IOAddress addr("::1"); + IOAddress addr("0.0.0.0"); // If IPv4 address assigned to the interface exists, find a suitable // subnet for it, else return NULL pointer to indicate that no subnet // could be found. diff --git a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc index a6223f767a..59ef85b8ec 100644 --- a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc @@ -718,8 +718,8 @@ TEST_F(CfgMgrTest, getSubnet4ForInterface) { // Initially, there are no subnets configured, so none of the IPv4 // addresses assigned to eth0 and eth1 can match with any subnet. - ASSERT_FALSE(CfgMgr::instance().getSubnet4("eth0")); - ASSERT_FALSE(CfgMgr::instance().getSubnet4("eth1")); + EXPECT_FALSE(CfgMgr::instance().getSubnet4("eth0")); + EXPECT_FALSE(CfgMgr::instance().getSubnet4("eth1")); // Configure first subnet which address on eth0 corresponds to. Subnet4Ptr subnet1(new Subnet4(IOAddress("10.0.0.1"), 24, 1, 2, 3));