From b428583eed0b72dba853b093f77145d4de061ef5 Mon Sep 17 00:00:00 2001 From: Wlodek Wencel Date: Fri, 17 Apr 2020 16:17:04 +0200 Subject: [PATCH] [#1132] addressed review comments --- src/bin/dhcp4/dhcp4_srv.cc | 15 ++--- src/bin/dhcp4/dhcp4_srv.h | 29 +++------- src/bin/dhcp4/tests/dhcp4_srv_unittest.cc | 51 +++++++++++++++++ src/bin/dhcp4/tests/inform_unittest.cc | 56 ------------------- .../dhcp4/tests/shared_network_unittest.cc | 12 ---- 5 files changed, 64 insertions(+), 99 deletions(-) diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 1a4f68a5bb..852432e986 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -588,7 +588,7 @@ void Dhcpv4Exchange::evaluateClasses(const Pkt4Ptr& pkt, bool depend_on_known) { const std::string Dhcpv4Srv::VENDOR_CLASS_PREFIX("VENDOR_CLASS_"); -bool Dhcpv4Srv::Dhcpv4Srv::test_send_responses_to_source_(false); +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) @@ -600,10 +600,8 @@ Dhcpv4Srv::Dhcpv4Srv(uint16_t server_port, uint16_t client_port, const char* env = std::getenv("KEA_TEST_SEND_RESPONSES_TO_SOURCE"); if (env) { - if (strncmp(env, "ENABLED", 7) == 0) { - LOG_WARN(dhcp4_logger, DHCP4_TESTING_MODE_SEND_TO_SOURCE_ENABLED); - setSendResponsesToSource(true); - } + LOG_WARN(dhcp4_logger, DHCP4_TESTING_MODE_SEND_TO_SOURCE_ENABLED); + test_send_responses_to_source_ = true; } LOG_DEBUG(dhcp4_logger, DBG_DHCP4_START, DHCP4_OPEN_SOCKET) @@ -2726,11 +2724,6 @@ Dhcpv4Srv::adjustRemoteAddr(Dhcpv4Exchange& ex) { } else { response->setRemoteAddr(query->getRemoteAddr()); } - - if (getSendResponsesToSource()) { - response->setRemoteAddr(query->getRemoteAddr()); - } - // Remote address is now set so return. return; } @@ -2789,7 +2782,7 @@ Dhcpv4Srv::adjustRemoteAddr(Dhcpv4Exchange& ex) { } - if (getSendResponsesToSource()) { + if (Dhcpv4Srv::test_send_responses_to_source_) { response->setRemoteAddr(query->getRemoteAddr()); } } diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index 107d6b4906..f6c9353e85 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/src/bin/dhcp4/dhcp4_srv.h @@ -425,6 +425,13 @@ public: /// of all packets. Called during reconfigure and shutdown. void discardPackets(); + /// @brief returns value of test_send_responses_to_source_ + /// + /// @return bool value of test_send_responses_to_source_ + static bool getSendResponsesToSource() { + return test_send_responses_to_source_; + } + protected: /// @name Functions filtering and sanity-checking received messages. @@ -741,24 +748,6 @@ public: /// will cause the packet to be assigned to class VENDOR_CLASS_FOO. static const std::string VENDOR_CLASS_PREFIX; - /// @brief This function set test_send_responses_to_source_ value - /// - /// If environment variable KEA_TEST_SEND_RESPONSES_TO_SOURCE will be - /// set to ENABLED this function will set value true to - /// test_send_responses_to_source_. - /// - /// @param bool value of test_send_responses_to_source_ - void setSendResponsesToSource(const bool value) { - test_send_responses_to_source_ = value; - } - - /// @brief returns value of test_send_responses_to_source_ - /// - /// @return bool value of test_send_responses_to_source_ - static bool getSendResponsesToSource() { - return test_send_responses_to_source_; - } - private: /// @brief Process Client FQDN %Option sent by a client. /// @@ -1080,8 +1069,8 @@ protected: private: - /// @brief value that define is send response to source mode is enabled - /// holds ture if it is. + /// @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_; public: diff --git a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc index ac9273082b..06239f36e1 100644 --- a/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc +++ b/src/bin/dhcp4/tests/dhcp4_srv_unittest.cc @@ -3913,4 +3913,55 @@ TEST_F(Dhcpv4SrvTest, userContext) { /// @todo: Implement proper tests for MySQL lease/host database, /// see ticket #4214. + +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 diff --git a/src/bin/dhcp4/tests/inform_unittest.cc b/src/bin/dhcp4/tests/inform_unittest.cc index 591d419246..1be526b4c3 100644 --- a/src/bin/dhcp4/tests/inform_unittest.cc +++ b/src/bin/dhcp4/tests/inform_unittest.cc @@ -485,60 +485,4 @@ TEST_F(InformTest, statisticsInform) { EXPECT_EQ(5, pkt4_sent->getInteger().first); } -// This test checks that the server receiving DHCPINFORM via relay, should -// unicasts the DHCPACK to the client (ciaddr) but sending to source address -// because of testing mode enabled with unaltred content. -TEST_F(InformTest, relayedClientSendToSourceTestingMode) { - setenv("KEA_TEST_SEND_RESPONSES_TO_SOURCE", "ENABLED", 1); - Dhcp4Client client; - // Configure DHCP server. - configure(INFORM_CONFIGS[1], *client.getServer()); - // Message is relayed. - EXPECT_TRUE(isc::dhcp::test::NakedDhcpv4Srv::getSendResponsesToSource()); - client.useRelay(); - // Request some configuration when DHCPINFORM is sent. - client.requestOptions(DHO_LOG_SERVERS, DHO_COOKIE_SERVERS); - // Preconfigure the client with the IP address. - client.createLease(IOAddress("192.0.2.56"), 600); - // Send DHCPINFORM message to the server. - ASSERT_NO_THROW(client.doInform()); - // Make sure that the server responded. - ASSERT_TRUE(client.getContext().response_); - Pkt4Ptr resp = client.getContext().response_; - Pkt4Ptr query = client.getContext().query_; - // Make sure that the server has responded with DHCPACK. - ASSERT_EQ(DHCPACK, static_cast(resp->getType())); - // Response should NOT have been unicast to the ciaddr. - EXPECT_NE(IOAddress("192.0.2.56"), resp->getLocalAddr()); - // Destiantion address of response should be equal to source - // address of query. - EXPECT_EQ(query->getLocalAddr(), resp->getLocalAddr()); - // The ciaddr should have been copied. - EXPECT_EQ(IOAddress("192.0.2.56"), resp->getCiaddr()); - // Response is unicast to the client, so it must not be relayed. - EXPECT_FALSE(resp->isRelayed()); - EXPECT_EQ(DHCP4_CLIENT_PORT, resp->getLocalPort()); - EXPECT_EQ(DHCP4_SERVER_PORT, resp->getRemotePort()); - // Make sure that the server id is present. - EXPECT_EQ("10.0.0.1", client.config_.serverid_.toText()); - // Make sure that the Routers option has been received. - ASSERT_EQ(2, client.config_.routers_.size()); - EXPECT_EQ("192.0.2.200", client.config_.routers_[0].toText()); - EXPECT_EQ("192.0.2.201", client.config_.routers_[1].toText()); - // Make sure that the DNS Servers option has been received. - ASSERT_EQ(2, client.config_.dns_servers_.size()); - EXPECT_EQ("192.0.2.202", client.config_.dns_servers_[0].toText()); - EXPECT_EQ("192.0.2.203", client.config_.dns_servers_[1].toText()); - // Make sure that the Quotes Servers option has been received. - ASSERT_EQ(2, client.config_.quotes_servers_.size()); - EXPECT_EQ("10.0.0.202", client.config_.quotes_servers_[0].toText()); - EXPECT_EQ("10.0.0.203", client.config_.quotes_servers_[1].toText()); - // Make sure that the Log Servers option has been received. - ASSERT_EQ(2, client.config_.log_servers_.size()); - EXPECT_EQ("10.0.0.200", client.config_.log_servers_[0].toText()); - EXPECT_EQ("10.0.0.201", client.config_.log_servers_[1].toText()); - - unsetenv("KEA_TEST_SEND_RESPONSES_TO_SOURCE"); -} - } // end of anonymous namespace diff --git a/src/bin/dhcp4/tests/shared_network_unittest.cc b/src/bin/dhcp4/tests/shared_network_unittest.cc index 47abaaf097..88a63ecdbf 100644 --- a/src/bin/dhcp4/tests/shared_network_unittest.cc +++ b/src/bin/dhcp4/tests/shared_network_unittest.cc @@ -2199,18 +2199,6 @@ TEST_F(Dhcpv4SharedNetworkTest, poolInSubnetSelectedByClass) { }); } -// Check if Kea starts with send to source mode enabled -TEST_F(Dhcpv4SharedNetworkTest, sharedNetworkCheckIfSendToSourceTestingModeEnabled) { - // Set env variable that put kea into testing mode - setenv("KEA_TEST_SEND_RESPONSES_TO_SOURCE", "ENABLED", 1); - Dhcp4Client client1(Dhcp4Client::SELECTING); - configure(NETWORKS_CONFIG[1], *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"); -} - // Shared network is selected based on giaddr value (relay specified // on shared network level, but response is send to source address. TEST_F(Dhcpv4SharedNetworkTest, sharedNetworkSendToSourceTestingModeEnabled) {