From: Francis Dupont Date: Sat, 25 Jun 2016 15:45:34 +0000 (+0200) Subject: [4109a] Addressed comments X-Git-Tag: trac4273_base~5^2~3 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=0b2ec98559bd6bda4c3174d72df46770f9bcb1c5;p=thirdparty%2Fkea.git [4109a] Addressed comments --- diff --git a/doc/guide/dhcp6-srv.xml b/doc/guide/dhcp6-srv.xml index 5eec3e79da..0c9766f656 100644 --- a/doc/guide/dhcp6-srv.xml +++ b/doc/guide/dhcp6-srv.xml @@ -3279,6 +3279,33 @@ should include options from the isc option space: + + pkt6-dhcpv4-query-received + integer + + Number of DHCPv4-QUERY packets received. This + statistic is expected to grow if there are devices + that are using DHCPv4-over-DHCPv6. DHCPv4-QUERY + messages are used by DHCPv4 clients on an IPv6 only + line so that encapsulate requests over DHCPv6. + + + + + pkt6-dhcpv4-response-received + integer + + Number of DHCPv4-RESPONSE packets received. This + statistic is expected to remain zero at all times, as + DHCPv4-RESPONSE packets are sent by the server and the + server is never expected to receive them. A non-zero + value indicates an error. One likely cause would be a + misbehaving relay agent that incorrectly forwards + DHCPv4-RESPONSE message towards the server, rather + back to the clients. + + + pkt6-unknown-received integer @@ -3319,6 +3346,16 @@ should include options from the isc option space: + + pkt6-dhcpv4-response-sent + integer + Number of DHCPv4-RESPONSE packets sent. This + statistic is expected to grow in most cases after a + DHCPv4-QUERY is processed. There are certain cases where + there is no response. + + + subnet[id].total-nas integer diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index aa65cd13bc..136b8432b3 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -165,8 +165,6 @@ createStatusCode(const Pkt6& pkt, const Option6IA& ia, const uint16_t status_cod namespace isc { namespace dhcp { -int Dhcpv6Srv::hook_index_buffer6_send = Hooks.hook_index_buffer6_send_; - const std::string Dhcpv6Srv::VENDOR_CLASS_PREFIX("VENDOR_CLASS_"); Dhcpv6Srv::Dhcpv6Srv(uint16_t port) @@ -3092,6 +3090,13 @@ void Dhcpv6Srv::processStatsReceived(const Pkt6Ptr& query) { case DHCPV6_INFORMATION_REQUEST: stat_name = "pkt6-infrequest-received"; break; + case DHCPV6_DHCPV4_QUERY: + stat_name = "pkt6-dhcpv4-query-received"; + break; + case DHCPV6_DHCPV4_RESPONSE: + // Should not happen, but let's keep a counter for it + stat_name = "pkt6-dhcpv4-response-received"; + break; default: ; // do nothing } @@ -3112,6 +3117,9 @@ void Dhcpv6Srv::processStatsSent(const Pkt6Ptr& response) { case DHCPV6_REPLY: stat_name = "pkt6-reply-sent"; break; + case DHCPV6_DHCPV4_RESPONSE: + stat_name = "pkt6-dhcpv4-response-sent"; + break; default: // That should never happen return; @@ -3120,5 +3128,9 @@ void Dhcpv6Srv::processStatsSent(const Pkt6Ptr& response) { StatsMgr::instance().addValue(stat_name, static_cast(1)); } +int Dhcpv6Srv::getHookIndexBuffer6Send() { + return (Hooks.hook_index_buffer6_send_); +} + }; }; diff --git a/src/bin/dhcp6/dhcp6_srv.h b/src/bin/dhcp6/dhcp6_srv.h index 63847e85b8..6521e91733 100644 --- a/src/bin/dhcp6/dhcp6_srv.h +++ b/src/bin/dhcp6/dhcp6_srv.h @@ -292,7 +292,9 @@ protected: /// @brief Processes incoming DHCPv4-query message. /// /// @param dhcp4_query message received from client - /// @return Reply (empty) message to (not) be sent to the client. + /// @return Reply message to be sent to the client (always NULL + /// as the response will be returned after by + /// @ref isc::dhcp::Dhcp6to4Ipc::handler()). Pkt6Ptr processDhcp4Query(const Pkt6Ptr& dhcp4_query); /// @brief Selects a subnet for a given client's packet. @@ -791,14 +793,15 @@ private: uint16_t port_; public: - /// @note used by DHCPv4-over-DHCPv6 so must be public + /// @note used by DHCPv4-over-DHCPv6 so must be public and static /// @brief Updates statistics for transmitted packets /// @param response packet transmitted static void processStatsSent(const Pkt6Ptr& response); - /// the index of the buffer6_send hook - static int hook_index_buffer6_send; + /// @brief Returns the index of the buffer6_send hook + /// @return the index of the buffer6_send hook + static int getHookIndexBuffer6Send(); protected: diff --git a/src/bin/dhcp6/dhcp6to4_ipc.cc b/src/bin/dhcp6/dhcp6to4_ipc.cc index 95516e965d..fbc691da8f 100644 --- a/src/bin/dhcp6/dhcp6to4_ipc.cc +++ b/src/bin/dhcp6/dhcp6to4_ipc.cc @@ -85,7 +85,8 @@ void Dhcp6to4Ipc::handler() { buf.clear(); pkt->pack(); - // Don't use getType(): get the message type from the buffer + // Don't use getType(): get the message type from the buffer as we + // want to know if it is a relayed message (vs. internal message type). uint8_t msg_type = buf[0]; if ((msg_type == DHCPV6_RELAY_FORW) || (msg_type == DHCPV6_RELAY_REPL)) { pkt->setRemotePort(DHCP6_SERVER_PORT); @@ -99,7 +100,7 @@ void Dhcp6to4Ipc::handler() { try { // Let's execute all callouts registered for buffer6_send - if (HooksManager::calloutsPresent(Dhcpv6Srv::hook_index_buffer6_send)) { + if (HooksManager::calloutsPresent(Dhcpv6Srv::getHookIndexBuffer6Send())) { CalloutHandlePtr callout_handle = getCalloutHandle(pkt); // Delete previously set arguments @@ -109,7 +110,7 @@ void Dhcp6to4Ipc::handler() { callout_handle->setArgument("response6", pkt); // Call callouts - HooksManager::callCallouts(Dhcpv6Srv::hook_index_buffer6_send, + HooksManager::callCallouts(Dhcpv6Srv::getHookIndexBuffer6Send(), *callout_handle); // Callouts decided to skip the next processing step. The next diff --git a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc index cf81f9569e..9d93428140 100644 --- a/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc +++ b/src/bin/dhcp6/tests/dhcp6_srv_unittest.cc @@ -2585,6 +2585,13 @@ TEST_F(Dhcpv6SrvTest, receiveReplyStat) { testReceiveStats(DHCPV6_REPLY, "pkt6-reply-received"); } +// Test checks if pkt6-dhcpv4-response-received is bumped up correctly. +// Note that in properly configured network the server never receives +// Dhcpv4-Response messages. +TEST_F(Dhcpv6SrvTest, receiveDhcpv4ResponseStat) { + testReceiveStats(DHCPV6_DHCPV4_RESPONSE, "pkt6-dhcpv4-response-received"); +} + // Test checks if pkt6-unknown-received is bumped up correctly. TEST_F(Dhcpv6SrvTest, receiveUnknownStat) { testReceiveStats(123, "pkt6-unknown-received"); @@ -2610,6 +2617,11 @@ TEST_F(Dhcpv6SrvTest, receiveDeclineStat) { testReceiveStats(DHCPV6_DECLINE, "pkt6-decline-received"); } +// Test checks if pkt6-dhcpv4-query-received is bumped up correctly. +TEST_F(Dhcpv6SrvTest, receiveDhcpv4QueryStat) { + testReceiveStats(DHCPV6_DHCPV4_QUERY, "pkt6-dhcpv4-query-received"); +} + // Test checks if reception of a malformed packet increases pkt-parse-failed // and pkt6-receive-drop TEST_F(Dhcpv6SrvTest, receiveParseFailedStat) { diff --git a/src/bin/dhcp6/tests/dhcp6to4_ipc_unittest.cc b/src/bin/dhcp6/tests/dhcp6to4_ipc_unittest.cc index 905a9d8714..8fada87cc5 100644 --- a/src/bin/dhcp6/tests/dhcp6to4_ipc_unittest.cc +++ b/src/bin/dhcp6/tests/dhcp6to4_ipc_unittest.cc @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -25,6 +26,7 @@ using namespace isc; using namespace isc::asiolink; using namespace isc::dhcp; using namespace isc::dhcp::test; +using namespace isc::stats; using namespace isc::hooks; using namespace isc::util; @@ -51,6 +53,8 @@ public: // Install buffer6_send_callout EXPECT_NO_THROW(HooksManager::preCalloutsLibraryHandle(). registerCallout("buffer6_send", buffer6_send_callout)); + // Let's wipe all existing statistics. + StatsMgr::instance().removeAll(); } /// @brief Configure DHCP4o6 port. @@ -128,8 +132,15 @@ TEST_F(Dhcp6to4IpcTest, receive) { ASSERT_NO_THROW(ipc.open()); ASSERT_NO_THROW(src_ipc.open()); + // Get statistics + StatsMgr& mgr = StatsMgr::instance(); + ObservationPtr pkt6_snd = mgr.getObservation("pkt6-sent"); + ObservationPtr d4_resp = mgr.getObservation("pkt6-dhcpv4-response-sent"); + EXPECT_FALSE(pkt6_snd); + EXPECT_FALSE(d4_resp); + // Create message to be sent over IPC. - Pkt6Ptr pkt(new Pkt6(DHCPV6_DHCPV4_QUERY, 1234)); + Pkt6Ptr pkt(new Pkt6(DHCPV6_DHCPV4_RESPONSE, 1234)); pkt->addOption(createDHCPv4MsgOption()); pkt->setIface("eth0"); pkt->setRemoteAddr(IOAddress("2001:db8:1::123")); @@ -152,13 +163,20 @@ TEST_F(Dhcp6to4IpcTest, receive) { EXPECT_TRUE(forwarded->getOption(D6O_DHCPV4_MSG)); EXPECT_EQ("eth0", forwarded->getIface()); EXPECT_EQ("2001:db8:1::123", forwarded->getRemoteAddr().toText()); + + // Verify statistics + pkt6_snd = mgr.getObservation("pkt6-sent"); + d4_resp = mgr.getObservation("pkt6-dhcpv4-response-sent"); + ASSERT_TRUE(pkt6_snd); + ASSERT_TRUE(d4_resp); + EXPECT_EQ(1, pkt6_snd->getInteger().first); + EXPECT_EQ(1, d4_resp->getInteger().first); } -// #4296 addresses this -#if 0 // This test verifies that the DHCPv4 endpoint of the DHCPv4o6 IPC can // receive relayed messages. -TEST_F(Dhcp6to4IpcTest, receiveRelayed) { +// This is currently not supported: it is a known defect addressed by #4296. +TEST_F(Dhcp6to4IpcTest, DISABLED_receiveRelayed) { // Create instance of the IPC endpoint under test. Dhcp6to4Ipc& ipc = Dhcp6to4Ipc::instance(); // Create instance of the IPC endpoint being used as a source of messages. @@ -168,8 +186,15 @@ TEST_F(Dhcp6to4IpcTest, receiveRelayed) { ASSERT_NO_THROW(ipc.open()); ASSERT_NO_THROW(src_ipc.open()); + // Get statistics + StatsMgr& mgr = StatsMgr::instance(); + ObservationPtr pkt6_snd = mgr.getObservation("pkt6-sent"); + ObservationPtr d4_resp = mgr.getObservation("pkt6-dhcpv4-response-sent"); + EXPECT_FALSE(pkt6_snd); + EXPECT_FALSE(d4_resp); + // Create relayed message to be sent over IPC. - Pkt6Ptr pkt(new Pkt6(DHCPV6_DHCPV4_QUERY, 1234)); + Pkt6Ptr pkt(new Pkt6(DHCPV6_DHCPV4_RESPONSE, 1234)); pkt->addOption(createDHCPv4MsgOption()); Pkt6::RelayInfo relay; relay.linkaddr_ = IOAddress("3000:1::1"); @@ -199,7 +224,14 @@ TEST_F(Dhcp6to4IpcTest, receiveRelayed) { EXPECT_EQ("eth0", forwarded->getIface()); EXPECT_EQ("2001:db8:1::123", forwarded->getRemoteAddr().toText()); EXPECT_EQ(DHCP6_CLIENT_PORT, forwarded->getRemotePort()); + + // Verify statistics + pkt6_snd = mgr.getObservation("pkt6-sent"); + d4_resp = mgr.getObservation("pkt6-dhcpv4-response-sent"); + ASSERT_TRUE(pkt6_snd); + ASSERT_TRUE(d4_resp); + EXPECT_EQ(1, pkt6_snd->getInteger().first); + EXPECT_EQ(1, d4_resp->getInteger().first); } -#endif } // end of anonymous namespace