[#1132] Fixed/improved the code and added new unit tests

src/bin/dhcp4/dhcp4_messages.cc

@@ -1,4 +1,4 @@
-// File created from ../../../src/bin/dhcp4/dhcp4_messages.mes on Thu Apr 16 2020 10:23
+// File created from ../../../src/bin/dhcp4/dhcp4_messages.mes on Fri Apr 17 2020 22:42
 
 #include <cstddef>
 #include <log/message_types.h>
@@ -226,7 +226,7 @@ const char* values[] = {
     "DHCP4_INIT_REBOOT", "%1: client is in INIT-REBOOT state and requests address %2",
     "DHCP4_LEASE_ADVERT", "%1: lease %2 will be advertised",
     "DHCP4_LEASE_ALLOC", "%1: lease %2 has been allocated for %3 seconds",
-    "DHCP4_MULTI_THREADING_INFO", "enabled: %1, number of threads: %2, queue size: %3",
+    "DHCP4_MULTI_THREADING_INFO", "enabled: %1, number of threads: %2, queue size per thread: %3",
     "DHCP4_MULTI_THREADING_WARNING", "The multi-threading feature is experimental. Don't use in production environment.",
     "DHCP4_NCR_CREATE", "%1: DDNS updates enabled, therefore sending name change requests",
     "DHCP4_NCR_CREATION_FAILED", "%1: failed to generate name change requests for DNS: %2",

src/bin/dhcp4/dhcp4_messages.h

@@ -1,4 +1,4 @@
-// File created from ../../../src/bin/dhcp4/dhcp4_messages.mes on Thu Apr 16 2020 10:23
+// File created from ../../../src/bin/dhcp4/dhcp4_messages.mes on Fri Apr 17 2020 22:42
 
 #ifndef DHCP4_MESSAGES_H
 #define DHCP4_MESSAGES_H

src/bin/dhcp4/dhcp4_messages.mes

@@ -7,10 +7,10 @@
 $NAMESPACE isc::dhcp
 
 % DHCP4_TESTING_MODE_SEND_TO_SOURCE_ENABLED All packets will be send to source address of an incoming packet - use only for testing
-This message is printed then KEA_TEST_SEND_RESPONSES_TO_SOURCE environment
+This message is printed then KEA_TEST_SEND_RESPONSES_TO_SOURCE
-variable is set to ENABLED. It's causing Kea to send packets to source address
+environment variable is set. It's causing Kea to send packets to
-of incoming packet. Usable just in testing environment to simulate multiple
+source address of incoming packet. Usable just in testing environment
-subnet traffic from single source.
+to simulate multiple subnet traffic from single source.
 
 % DHCP4_ACTIVATE_INTERFACE activating interface %1
 This message is printed when DHCPv4 server enabled an interface to be used

src/bin/dhcp4/dhcp4_srv.cc

@@ -69,6 +69,7 @@
 
 #include <iomanip>
 #include <set>
+#include <cstdlib>
 
 using namespace isc;
 using namespace isc::asiolink;
@@ -588,15 +589,14 @@ void Dhcpv4Exchange::evaluateClasses(const Pkt4Ptr& pkt, bool depend_on_known) {
 
 const std::string Dhcpv4Srv::VENDOR_CLASS_PREFIX("VENDOR_CLASS_");
 
-bool Dhcpv4Srv::test_send_responses_to_source_(false);
-
 Dhcpv4Srv::Dhcpv4Srv(uint16_t server_port, uint16_t client_port,
                      const bool use_bcast, const bool direct_response_desired)
     : io_service_(new IOService()), server_port_(server_port),
       client_port_(client_port), shutdown_(true),
       alloc_engine_(), use_bcast_(use_bcast),
       network_state_(new NetworkState(NetworkState::DHCPv4)),
-      cb_control_(new CBControlDHCPv4()) {
+      cb_control_(new CBControlDHCPv4()),
+      test_send_responses_to_source_(false) {
 
     const char* env = std::getenv("KEA_TEST_SEND_RESPONSES_TO_SOURCE");
     if (env) {
@@ -2779,10 +2779,10 @@ Dhcpv4Srv::adjustRemoteAddr(Dhcpv4Exchange& ex) {
     // to the address we got the query from.
     } else {
         response->setRemoteAddr(query->getRemoteAddr());
-
     }
 
-    if (Dhcpv4Srv::test_send_responses_to_source_) {
+    // For testing *only*.
+    if (getSendResponsesToSource()) {
         response->setRemoteAddr(query->getRemoteAddr());
     }
 }

src/bin/dhcp4/dhcp4_srv.h

@@ -425,11 +425,11 @@ public:
     /// of all packets.  Called during reconfigure and shutdown.
     void discardPackets();
 
-    /// @brief returns value of test_send_responses_to_source_
+    /// @brief Returns value of the test_send_responses_to_source_ flag.
     ///
-    /// @return bool value of test_send_responses_to_source_
+    /// @return value of the test_send_responses_to_source_ flag.
-    static bool getSendResponsesToSource() {
+    bool getSendResponsesToSource() const {
-        return test_send_responses_to_source_;
+        return (test_send_responses_to_source_);
     }
 
 protected:
@@ -738,6 +738,13 @@ protected:
     /// constructor.
     void setPacketStatisticsDefaults();
 
+    /// @brief Sets value of the test_send_responses_to_source_ flag.
+    ///
+    /// @param value new value of the test_send_responses_to_source_ flag.
+    void setSendResponsesToSource(bool value) {
+        test_send_responses_to_source_ = value;
+    }
+
 public:
 
     /// @brief this is a prefix added to the content of vendor-class option
@@ -914,12 +921,9 @@ protected:
     /// are valid. Make sure that pointers are correct before calling this
     /// function.
     ///
-    /// @note This method is static because it is not dependent on the class
-    /// state.
-    ///
     /// @param ex The exchange holding both the client's message and the
     /// server's response.
-    static void adjustRemoteAddr(Dhcpv4Exchange& ex);
+    void adjustRemoteAddr(Dhcpv4Exchange& ex);
 
     /// @brief converts server-id to text
     /// Converts content of server-id option to a text representation, e.g.
@@ -1071,7 +1075,7 @@ private:
 
     /// @brief store value that defines if kea will send responses
     /// to a source address of incoming packet. Only for testing.
-    static bool test_send_responses_to_source_;
+    bool test_send_responses_to_source_;
 
 public:
 

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -38,6 +38,7 @@
 #include <boost/scoped_ptr.hpp>
 
 #include <iostream>
+#include <cstdlib>
 
 #include <arpa/inet.h>
 
@@ -369,6 +370,55 @@ TEST_F(Dhcpv4SrvTest, adjustIfaceDataUseRouting) {
     EXPECT_EQ(1, resp->getIndex());
 }
 
+// This test verifies that the destination address of the response
+// message is set to source address when the testing mode is enabled.
+// Relayed message: not testing mode was tested in adjustIfaceDataRelay.
+TEST_F(Dhcpv4SrvTest, adjustRemoteAddressRelaySendToSourceTestingModeEnabled) {
+    IfaceMgrTestConfig test_config(true);
+    IfaceMgr::instance().openSockets4();
+
+    // Create the instance of the incoming packet.
+    boost::shared_ptr<Pkt4> req(new Pkt4(DHCPDISCOVER, 1234));
+    // Set the giaddr to non-zero address and hops to non-zero value
+    // as if it was relayed.
+    req->setGiaddr(IOAddress("192.0.1.1"));
+    req->setHops(2);
+    // Set ciaddr to zero. This simulates the client which applies
+    // for the new lease.
+    req->setCiaddr(IOAddress("0.0.0.0"));
+    // Clear broadcast flag.
+    req->setFlags(0x0000);
+
+    // Set local address, port and interface.
+    req->setLocalAddr(IOAddress("192.0.2.5"));
+    req->setLocalPort(1001);
+    req->setIface("eth1");
+    req->setIndex(1);
+
+    // Set remote address and port.
+    req->setRemoteAddr(IOAddress("192.0.2.1"));
+    req->setRemotePort(1234);
+
+    // Create the exchange using the req.
+    Dhcpv4Exchange ex = createExchange(req);
+
+    Pkt4Ptr resp = ex.getResponse();
+    resp->setYiaddr(IOAddress("192.0.1.100"));
+    // Clear the remote address.
+    resp->setRemoteAddr(IOAddress("0.0.0.0"));
+    // Set hops value for the response.
+    resp->setHops(req->getHops());
+
+    // Set the testing mode.
+    srv_.setSendResponsesToSource(true);
+
+    // This function never throws.
+    ASSERT_NO_THROW(srv_.adjustIfaceData(ex));
+
+    // Now the destination address should be source address.
+    EXPECT_EQ("192.0.2.1", resp->getRemoteAddr().toText());
+}
+
 // This test verifies that the destination address of the response message
 // is set to ciaddr when giaddr is set to zero and the ciaddr is set to
 // non-zero address in the received message. This is the case when the
@@ -435,6 +485,64 @@ TEST_F(Dhcpv4SrvTest, adjustIfaceDataRenew) {
 
 }
 
+// This test verifies that the destination address of the response message
+// is set to source address when the testing mode is enabled.
+// Renew: not testing mode was tested in adjustIfaceDataRenew.
+TEST_F(Dhcpv4SrvTest, adjustRemoteAddressRenewSendToSourceTestingModeEnabled) {
+    IfaceMgrTestConfig test_config(true);
+    IfaceMgr::instance().openSockets4();
+
+    // Create instance of the incoming packet.
+    boost::shared_ptr<Pkt4> req(new Pkt4(DHCPDISCOVER, 1234));
+
+    // Clear giaddr to simulate direct packet.
+    req->setGiaddr(IOAddress("0.0.0.0"));
+    // Set ciaddr to non-zero address. The response should be sent to this
+    // address as the client is in renewing or rebinding state (it is fully
+    // configured).
+    req->setCiaddr(IOAddress("192.0.1.15"));
+    // Let's configure broadcast flag. It should be ignored because
+    // we are responding directly to the client having an address
+    // and trying to extend his lease. Broadcast flag is only used
+    // when new lease is acquired and server must make a decision
+    // whether to unicast the response to the acquired address or
+    // broadcast it.
+    req->setFlags(Pkt4::FLAG_BROADCAST_MASK);
+    // This is a direct message, so the hops should be cleared.
+    req->setHops(0);
+    // Set local unicast address as if we are renewing a lease.
+    req->setLocalAddr(IOAddress("192.0.2.1"));
+    // Request is received on the DHCPv4 server port.
+    req->setLocalPort(DHCP4_SERVER_PORT);
+    // Set the interface. The response should be sent over the same interface.
+    req->setIface("eth1");
+    req->setIndex(1);
+    // Set remote address.
+    req->setRemoteAddr(IOAddress("192.0.2.1"));
+
+    // Create the exchange using the req.
+    Dhcpv4Exchange ex = createExchange(req);
+    Pkt4Ptr resp = ex.getResponse();
+
+    // Let's extend the lease for the client in such a way that
+    // it will actually get different address. The response
+    // should not be sent to this address but rather to ciaddr
+    // as client still have ciaddr configured.
+    resp->setYiaddr(IOAddress("192.0.1.13"));
+    // Clear the remote address.
+    resp->setRemoteAddr(IOAddress("0.0.0.0"));
+    // Copy hops value from the query.
+    resp->setHops(req->getHops());
+
+    // Set the testing mode.
+    srv_.setSendResponsesToSource(true);
+
+    ASSERT_NO_THROW(srv_.adjustIfaceData(ex));
+
+    // Check that server responds to source address.
+    EXPECT_EQ("192.0.2.1", resp->getRemoteAddr().toText());
+}
+
 // This test verifies that the destination address of the response message
 // is set correctly when giaddr and ciaddr is zeroed in the received message
 // and the new lease is acquired. The lease address is carried in the
@@ -524,6 +632,69 @@ TEST_F(Dhcpv4SrvTest, adjustIfaceDataSelect) {
     EXPECT_EQ("192.0.1.13", resp->getRemoteAddr().toText());
 }
 
+// This test verifies that the destination address of the response message
+// is set to source address when the testing mode is enabled.
+// Select cases: not testing mode were tested in adjustIfaceDataSelect.
+TEST_F(Dhcpv4SrvTest, adjustRemoteAddressSelectSendToSourceTestingModeEnabled) {
+    IfaceMgrTestConfig test_config(true);
+    IfaceMgr::instance().openSockets4();
+
+    // Create instance of the incoming packet.
+    boost::shared_ptr<Pkt4> req(new Pkt4(DHCPDISCOVER, 1234));
+
+    // Clear giaddr to simulate direct packet.
+    req->setGiaddr(IOAddress("0.0.0.0"));
+    // Clear client address as it hasn't got any address configured yet.
+    req->setCiaddr(IOAddress("0.0.0.0"));
+
+    // Let's clear the broadcast flag.
+    req->setFlags(0);
+
+    // This is a non-relayed message, so let's clear hops count.
+    req->setHops(0);
+    // The query is sent to the broadcast address in the Select state.
+    req->setLocalAddr(IOAddress("255.255.255.255"));
+    // The query has been received on the DHCPv4 server port 67.
+    req->setLocalPort(DHCP4_SERVER_PORT);
+    // Set the interface. The response should be sent via the same interface.
+    req->setIface("eth1");
+    req->setIndex(1);
+    // Set remote address.
+    req->setRemoteAddr(IOAddress("192.0.2.1"));
+
+    // Create the exchange using the req.
+    Dhcpv4Exchange ex = createExchange(req);
+    Pkt4Ptr resp = ex.getResponse();
+    // Assign some new address for this client.
+    resp->setYiaddr(IOAddress("192.0.1.13"));
+    // Clear the remote address.
+    resp->setRemoteAddr(IOAddress("0.0.0.0"));
+    // Copy hops count.
+    resp->setHops(req->getHops());
+
+    // Disable direct responses.
+    test_config.setDirectResponse(false);
+
+    // Set the testing mode.
+    srv_.setSendResponsesToSource(true);
+
+    ASSERT_NO_THROW(srv_.adjustIfaceData(ex));
+
+    // Check that server responds to source address.
+    EXPECT_EQ("192.0.2.1", resp->getRemoteAddr().toText());
+
+    // Enable direct responses.
+    test_config.setDirectResponse(true);
+
+    // Clear the remote address.
+    resp->setRemoteAddr(IOAddress("0.0.0.0"));
+
+    ASSERT_NO_THROW(srv_.adjustIfaceData(ex));
+
+    // Check that server still responds to source address.
+    EXPECT_EQ("192.0.2.1", resp->getRemoteAddr().toText());
+}
+
 // This test verifies that the destination address of the response message
 // is set to broadcast address when client set broadcast flag in its
 // query. Client sets this flag to indicate that it can't receive direct
@@ -582,6 +753,52 @@ TEST_F(Dhcpv4SrvTest, adjustIfaceDataBroadcast) {
 
 }
 
+// This test verifies that the destination address of the response message
+// is set to source address when the testing mode is enabled.
+// Broadcast case: not testing mode was tested in adjustIfaceDataBroadcast.
+TEST_F(Dhcpv4SrvTest, adjustRemoteAddressBroadcastSendToSourceTestingModeEnabled) {
+    IfaceMgrTestConfig test_config(true);
+    IfaceMgr::instance().openSockets4();
+
+    // Create instance of the incoming packet.
+    boost::shared_ptr<Pkt4> req(new Pkt4(DHCPDISCOVER, 1234));
+
+    // Clear giaddr to simulate direct packet.
+    req->setGiaddr(IOAddress("0.0.0.0"));
+    // Clear client address as it hasn't got any address configured yet.
+    req->setCiaddr(IOAddress("0.0.0.0"));
+    // The query is sent to the broadcast address in the Select state.
+    req->setLocalAddr(IOAddress("255.255.255.255"));
+    // The query has been received on the DHCPv4 server port 67.
+    req->setLocalPort(DHCP4_SERVER_PORT);
+    // Set the interface. The response should be sent via the same interface.
+    req->setIface("eth1");
+    req->setIndex(1);
+    // Set remote address.
+    req->setRemoteAddr(IOAddress("192.0.2.1"));
+
+    // Let's set the broadcast flag.
+    req->setFlags(Pkt4::FLAG_BROADCAST_MASK);
+
+    // Create the exchange using the req.
+    Dhcpv4Exchange ex = createExchange(req);
+    Pkt4Ptr resp = ex.getResponse();
+
+    // Assign some new address for this client.
+    resp->setYiaddr(IOAddress("192.0.1.13"));
+
+    // Clear the remote address.
+    resp->setRemoteAddr(IOAddress("0.0.0.0"));
+
+    // Set the testing mode.
+    srv_.setSendResponsesToSource(true);
+
+    ASSERT_NO_THROW(srv_.adjustIfaceData(ex));
+
+    // Check that server responds to source address.
+    EXPECT_EQ("192.0.2.1", resp->getRemoteAddr().toText());
+}
+
 // This test verifies that the mandatory to copy fields and options
 // are really copied into the response.
 TEST_F(Dhcpv4SrvTest, initResponse) {
@@ -697,6 +914,23 @@ TEST_F(Dhcpv4SrvTest, basic) {
     ASSERT_NO_THROW(naked_srv.reset(new NakedDhcpv4Srv(0)));
 }
 
+// This test verifies the test_send_responses_to_source_ is false by default
+// and sets by the KEA_TEST_SEND_RESPONSES_TO_SOURCE environment variable.
+TEST_F(Dhcpv4SrvTest, testSendResponsesToSource) {
+
+    ASSERT_FALSE(std::getenv("KEA_TEST_SEND_RESPONSES_TO_SOURCE"));
+    boost::scoped_ptr<NakedDhcpv4Srv> naked_srv;
+    ASSERT_NO_THROW(
+        naked_srv.reset(new NakedDhcpv4Srv(DHCP4_SERVER_PORT + 10000)));
+    EXPECT_FALSE(naked_srv->getSendResponsesToSource());
+    ::setenv("KEA_TEST_SEND_RESPONSES_TO_SOURCE", "ENABLED", 1);
+    // Do not use ASSERT as we want unsetenv to be always called.
+    EXPECT_NO_THROW(
+        naked_srv.reset(new NakedDhcpv4Srv(DHCP4_SERVER_PORT + 10000)));
+    EXPECT_TRUE(naked_srv->getSendResponsesToSource());
+    ::unsetenv("KEA_TEST_SEND_RESPONSES_TO_SOURCE");
+}
+
 // Verifies that DISCOVER message can be processed correctly,
 // that the OFFER message generated in response is valid and
 // contains necessary options.
@@ -3913,55 +4147,4 @@ TEST_F(Dhcpv4SrvTest, userContext) {
 /// @todo: Implement proper tests for MySQL lease/host database,
 ///        see ticket #4214.
 
-
+} // end of anonymous namespace
-TEST_F(Dhcpv4SrvTest, SendToSourceMode) {
-    string config  = "{"
-    "    \"interfaces-config\": {"
-    "        \"interfaces\": [ \"*\" ]"
-    "    },"
-    "    \"valid-lifetime\": 600,"
-    "    \"shared-networks\": ["
-    "        {"
-    "            \"name\": \"frog\","
-    "            \"relay\": {"
-    "                \"ip-address\": \"192.3.5.6\""
-    "            },"
-    "            \"subnet4\": ["
-    "                {"
-    "                    \"subnet\": \"192.0.2.0/26\","
-    "                    \"id\": 10,"
-    "                    \"pools\": ["
-    "                        {"
-    "                            \"pool\": \"192.0.2.63 - 192.0.2.63\""
-    "                        }"
-    "                    ]"
-    "                }"
-    "            ]"
-    "        }"
-    "    ],"
-    "    \"subnet4\": ["
-    "        {"
-    "            \"subnet\": \"192.0.2.64/26\","
-    "            \"id\": 1000,"
-    "            \"relay\": {"
-    "                \"ip-address\": \"192.1.2.3\""
-    "            },"
-    "            \"pools\": ["
-    "                {"
-    "                    \"pool\": \"192.0.2.65 - 192.0.2.65\""
-    "                }"
-    "            ]"
-    "        }"
-    "    ]"
-    "}";
-
-    // Set env variable that put kea into testing mode
-    //setenv("KEA_TEST_SEND_RESPONSES_TO_SOURCE", "ENABLED", 1);
-    Dhcp4Client client1(Dhcp4Client::SELECTING);
-    configure(config, *client1.getServer());
-    // Check if send to source testing mode is enabled
-    EXPECT_TRUE(isc::dhcp::test::NakedDhcpv4Srv::getSendResponsesToSource());
-    unsetenv("KEA_TEST_SEND_RESPONSES_TO_SOURCE");
-}
-
-}  // namespace

src/bin/dhcp4/tests/dhcp4_test_utils.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2019 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2020 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
@@ -226,6 +226,7 @@ public:
     using Dhcpv4Srv::accept;
     using Dhcpv4Srv::acceptMessageType;
     using Dhcpv4Srv::selectSubnet;
+    using Dhcpv4Srv::setSendResponsesToSource;
     using Dhcpv4Srv::VENDOR_CLASS_PREFIX;
     using Dhcpv4Srv::shutdown_;
     using Dhcpv4Srv::alloc_engine_;

src/bin/dhcp4/tests/shared_network_unittest.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2017-2019 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2017-2020 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
@@ -22,7 +22,6 @@
 #include <boost/pointer_cast.hpp>
 #include <boost/shared_ptr.hpp>
 #include <functional>
-#include <stdlib.h>
 
 using namespace isc;
 using namespace isc::asiolink;
@@ -2206,14 +2205,13 @@ TEST_F(Dhcpv4SharedNetworkTest, sharedNetworkSendToSourceTestingModeEnabled) {
     // address matching configured shared network.
     // Source address is set to unrelated to configuration.
 
-    // Set env variable that put kea into testing mode
-    setenv("KEA_TEST_SEND_RESPONSES_TO_SOURCE", "ENABLED", 1);
     Dhcp4Client client1(Dhcp4Client::SELECTING);
+    // Put Kea into testing mode.
+    client1.getServer()->setSendResponsesToSource(true);
     client1.useRelay(true, IOAddress("192.3.5.6"), IOAddress("1.1.1.2"));
     // Configure the server with one shared network and one subnet outside of the
     // shared network.
     configure(NETWORKS_CONFIG[1], *client1.getServer());
-    EXPECT_TRUE(isc::dhcp::test::NakedDhcpv4Srv::getSendResponsesToSource());
     // Client #1 should be assigned an address from shared network.
     testAssigned([this, &client1] {
         doDORA(client1, "192.0.2.63", "192.0.2.63");
@@ -2235,8 +2233,8 @@ TEST_F(Dhcpv4SharedNetworkTest, sharedNetworkSendToSourceTestingModeEnabled) {
 
     Pkt4Ptr resp2 = client2.getContext().response_;
     EXPECT_EQ("2.2.2.3", resp2->getLocalAddr().toText());
-    // remove variable
+    // reset testing mode.
-    unsetenv("KEA_TEST_SEND_RESPONSES_TO_SOURCE");
+    client1.getServer()->setSendResponsesToSource(false);
 }
 
 // Verify option processing precedence