From: Tomek Mrugalski Date: Fri, 19 Jun 2015 13:32:32 +0000 (+0200) Subject: [3795] Changes after review: X-Git-Tag: trac3910_base~2^2~2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=ad36b40cea28c098aa1debcff61fb64ccec2e802;p=thirdparty%2Fkea.git [3795] Changes after review: - Added several more unit-tests for pkt6-receive-drop (not all cases are really testable) - User's Guide cleanup - using namespace isc::stats added - Dhcp6Client::useServerId() implemented --- diff --git a/doc/guide/dhcp4-srv.xml b/doc/guide/dhcp4-srv.xml index ead6e505b6..39322903fa 100644 --- a/doc/guide/dhcp4-srv.xml +++ b/doc/guide/dhcp4-srv.xml @@ -2834,7 +2834,7 @@ temporarily override a list of interface names and listen on all interfaces. reset during reconfiguration event. - subnet[id].assigned-addresses + subnet[id].assigned-addresses integer This statistic shows the number of assigned addresses in a given subnet. This statistic increases every time a new lease is diff --git a/doc/guide/dhcp6-srv.xml b/doc/guide/dhcp6-srv.xml index a190b986c1..60a59cc22b 100644 --- a/doc/guide/dhcp6-srv.xml +++ b/doc/guide/dhcp6-srv.xml @@ -2612,6 +2612,26 @@ should include options from the isc option space: to grow rapidly. + + pkt6-receive-drop + integer + Number of incoming packets that were dropped. Exact reason + for dropping packets is logged, but the most common reasons may + be: an unacceptable or not supported packet type, direct responses + are forbidden, the server-id sent by the client does not match the + server's server-id or the packet is malformed. + + + + pkt6-parse-failed + integer + Number of incoming packets that could not be parsed. + A non-zero value of this statistic indicates that the server + received a malformed or truncated packet. This may indicate problems + in your network, faulty clients, faulty relay agents or server + code bug. + + pkt6-solicit-received integer @@ -2682,10 +2702,10 @@ should include options from the isc option space: pkt6-release-received integer Number of RELEASE packets received. This statistic is expected - to grow every time a device is being shut down in the network. It + to grow when a device is being shut down in the network. It indicates that the address or prefix assigned is reported as no longer - needed. Note that in wireless networks, the number of RELEASE messages - is significantly lower than the number of REQUEST messages. + needed. Note that many devices, especially wireless, do not send RELEASE, + because of design choice or due to moving out of range. @@ -2701,7 +2721,7 @@ should include options from the isc option space: assigned conflicting addresses. - + pkt6-infrequest-received integer @@ -2754,26 +2774,6 @@ should include options from the isc option space: - - pkt6-parse-failed - integer - Number of incoming packets that could not be parsed. - A non-zero value of this statistic indicates that the server - received a malformed or truncated packet. This may indicate problems - in your network, faulty clients, faulty relay agents or server - code bug. - - - - pkt6-receive-drop - integer - Number of incoming packets that were dropped. Exact reason - for dropping packets is logged, but the most common reasons may - be: an unacceptable or not supported packet type, direct responses - are forbidden, the server-id sent by the client does not match the - server's server-id or the packet is malformed. - - diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index ffccd12e4a..2b66ee4899 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -74,6 +74,7 @@ using namespace isc::dhcp; using namespace isc::dhcp_ddns; using namespace isc::hooks; using namespace isc::log; +using namespace isc::stats; using namespace isc::util; using namespace std; @@ -335,8 +336,7 @@ bool Dhcpv6Srv::run() { // any failures in unpacking will cause the packet to be dropped. // we will increase type specific packets further down the road. // See processStatsReceived(). - isc::stats::StatsMgr::instance().addValue("pkt6-received", - static_cast(1)); + StatsMgr::instance().addValue("pkt6-received", static_cast(1)); } else { LOG_DEBUG(packet_logger, DBG_DHCP6_DETAIL, DHCP6_BUFFER_WAIT_INTERRUPTED) @@ -440,11 +440,11 @@ bool Dhcpv6Srv::run() { .arg(query->getIface()) .arg(e.what()); - // Increase the statistics of parse failures and dropped packets. - isc::stats::StatsMgr::instance().addValue("pkt6-parse-failed", - static_cast(1)); - isc::stats::StatsMgr::instance().addValue("pkt6-receive-drop", - static_cast(1)); + // Increase the statistics of parse failures and dropped packets. + StatsMgr::instance().addValue("pkt6-parse-failed", + static_cast(1)); + StatsMgr::instance().addValue("pkt6-receive-drop", + static_cast(1)); continue; } } @@ -457,8 +457,7 @@ bool Dhcpv6Srv::run() { if (!testServerID(query)) { // Increase the statistic of dropped packets. - isc::stats::StatsMgr::instance().addValue("pkt6-receive-drop", - static_cast(1)); + StatsMgr::instance().addValue("pkt6-receive-drop", static_cast(1)); continue; } @@ -468,8 +467,7 @@ bool Dhcpv6Srv::run() { if (!testUnicast(query)) { // Increase the statistic of dropped packets. - isc::stats::StatsMgr::instance().addValue("pkt6-receive-drop", - static_cast(1)); + StatsMgr::instance().addValue("pkt6-receive-drop", static_cast(1)); continue; } @@ -568,8 +566,7 @@ bool Dhcpv6Srv::run() { .arg(e.what()); // Increase the statistic of dropped packets. - isc::stats::StatsMgr::instance().addValue("pkt6-receive-drop", - static_cast(1)); + StatsMgr::instance().addValue("pkt6-receive-drop", static_cast(1)); } catch (const isc::Exception& e) { @@ -585,8 +582,7 @@ bool Dhcpv6Srv::run() { .arg(e.what()); // Increase the statistic of dropped packets. - isc::stats::StatsMgr::instance().addValue("pkt6-receive-drop", - static_cast(1)); + StatsMgr::instance().addValue("pkt6-receive-drop", static_cast(1)); } if (rsp) { @@ -2981,14 +2977,12 @@ void Dhcpv6Srv::processStatsReceived(const Pkt6Ptr& query) { ; // do nothing } - isc::stats::StatsMgr::instance().addValue(stat_name, - static_cast(1)); + StatsMgr::instance().addValue(stat_name, static_cast(1)); } void Dhcpv6Srv::processStatsSent(const Pkt6Ptr& response) { // Increase generic counter for sent packets. - isc::stats::StatsMgr::instance().addValue("pkt6-sent", - static_cast(1)); + StatsMgr::instance().addValue("pkt6-sent", static_cast(1)); // Increase packet type specific counter for packets sent. string stat_name; @@ -3004,8 +2998,7 @@ void Dhcpv6Srv::processStatsSent(const Pkt6Ptr& response) { return; } - isc::stats::StatsMgr::instance().addValue(stat_name, - static_cast(1)); + StatsMgr::instance().addValue(stat_name, static_cast(1)); } }; diff --git a/src/bin/dhcp6/tests/dhcp6_client.cc b/src/bin/dhcp6/tests/dhcp6_client.cc index 1fc120a9be..d09ad7eb4c 100644 --- a/src/bin/dhcp6/tests/dhcp6_client.cc +++ b/src/bin/dhcp6/tests/dhcp6_client.cc @@ -327,7 +327,11 @@ Dhcp6Client::doSolicit() { void Dhcp6Client::doRequest() { Pkt6Ptr query = createMsg(DHCPV6_REQUEST); - query->addOption(context_.response_->getOption(D6O_SERVERID)); + if (!forced_server_id_) { + query->addOption(context_.response_->getOption(D6O_SERVERID)); + } else { + query->addOption(forced_server_id_); + } copyIAs(context_.response_, query); // Add Client FQDN if configured. diff --git a/src/bin/dhcp6/tests/dhcp6_client.h b/src/bin/dhcp6/tests/dhcp6_client.h index 98091db61e..35462321ce 100644 --- a/src/bin/dhcp6/tests/dhcp6_client.h +++ b/src/bin/dhcp6/tests/dhcp6_client.h @@ -448,6 +448,17 @@ public: use_rapid_commit_ = rapid_commit; } + /// @brief Specifies server-id to be used in send messages + /// + /// Overrides the server-id to be sent when server-id is expected to be + /// sent. May be NULL, which means use proper server-id sent in Advertise + /// (which is a normal client behavior). + /// + /// @param server_id server-id to be sent + void useServerId(const OptionPtr& server_id) { + forced_server_id_ = server_id; + } + /// @brief Creates an instance of the Client FQDN option to be included /// in the client's message. /// @@ -614,6 +625,9 @@ private: /// to true. See @ref sendORO for details. std::vector oro_; + /// @brief forced (Overridden) value of the server-id option (may be NULL) + OptionPtr forced_server_id_; + /// @brief FQDN requested by the client. Option6ClientFqdnPtr fqdn_; }; diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc index e97a6dd3f7..b587becb4f 100644 --- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc @@ -2336,7 +2336,7 @@ TEST_F(Dhcpv6SrvTest, receiveAdvertiseStat) { // Note that in properly configured network the server never receives Reply // messages. TEST_F(Dhcpv6SrvTest, receiveReplyStat) { - testReceiveStats(DHCPV6_ADVERTISE, "pkt6-advertise-received"); + testReceiveStats(DHCPV6_REPLY, "pkt6-reply-received"); } // Test checks if pkt6-unknown-received is bumped up correctly. @@ -2407,6 +2407,7 @@ TEST_F(Dhcpv6SrvTest, receiveParseFailedStat) { EXPECT_EQ(1, recv_drop->getInteger().first); } + /// @todo: Add more negative tests for processX(), e.g. extend sanityCheck() test /// to call processX() methods. diff --git a/src/bin/dhcp6/tests/sarr_unittest.cc b/src/bin/dhcp6/tests/sarr_unittest.cc index 40e4e61c1f..f7a75b0218 100644 --- a/src/bin/dhcp6/tests/sarr_unittest.cc +++ b/src/bin/dhcp6/tests/sarr_unittest.cc @@ -19,6 +19,7 @@ #include #include #include +#include #include using namespace isc; @@ -342,4 +343,58 @@ TEST_F(SARRTest, sarrStats) { EXPECT_EQ(2, pkt6_sent->getInteger().first); } +// This test verifies that pkt6-receive-drop is increased properly when the +// client's packet is rejected due to mismatched server-id value. +TEST_F(SARRTest, pkt6ReceiveDropStat1) { + + // Dummy server-id (0xff repeated 10 times) + std::vector data(10, 0xff); + OptionPtr bogus_srv_id(new Option(Option::V6, D6O_SERVERID, data)); + + // Let's use one of the existing configurations and tell the client to + // ask for an address. + Dhcp6Client client; + configure(CONFIGS[1], *client.getServer()); + client.setInterface("eth1"); + client.useNA(); + + client.doSolicit(); + client.useServerId(bogus_srv_id); + client.doRequest(); + + // Ok, let's check the statistics. None should be present. + using namespace isc::stats; + StatsMgr& mgr = StatsMgr::instance(); + + ObservationPtr pkt6_recv_drop = mgr.getObservation("pkt6-receive-drop"); + ASSERT_TRUE(pkt6_recv_drop); + + EXPECT_EQ(1, pkt6_recv_drop->getInteger().first); +} + +// This test verifies that pkt6-receive-drop is increased properly when the +// client's packet is rejected due to sending to unicast. +TEST_F(SARRTest, pkt6ReceiveDropStat2) { + + // Let's use one of the existing configurations and tell the client to + // ask for an address. + Dhcp6Client client; + configure(CONFIGS[1], *client.getServer()); + client.setInterface("eth1"); + client.useNA(); + + client.setDestAddress(asiolink::IOAddress("2001:db8::1")); // Pretend it's unicast + client.doSolicit(); + + // Ok, let's check the statistics. None should be present. + using namespace isc::stats; + StatsMgr& mgr = StatsMgr::instance(); + + ObservationPtr pkt6_recv_drop = mgr.getObservation("pkt6-receive-drop"); + ASSERT_TRUE(pkt6_recv_drop); + + EXPECT_EQ(1, pkt6_recv_drop->getInteger().first); +} + + } // end of anonymous namespace