Skip to content

GitLab

Commit ad0f44ba authored by Marcin Siodelski's avatar Marcin Siodelski
Browse files

[3628] Addressed review comments. 

One of the major changes was to eliminate possibility of defining the dead
reservation: neither hostname nor IP address reserved and covering it with
suitable unit tests.
parent f9c52694
Hide whitespace changes
Inline Side-by-side
src/bin/dhcp4/dhcp4.spec
View file @ ad0f44ba
......@@ -339,13 +339,13 @@
{
"item_name": "hw-address",
"item_type": "string",
"item_optional": false,
"item_optional": true,
"item_default": ""
},
{
"item_name": "duid",
"item_type": "string",
"item_optional": false,
"item_optional": true,
"item_default": ""
},
{
......
src/bin/dhcp4/tests/config_parser_unittest.cc
View file @ ad0f44ba
......@@ -3245,7 +3245,8 @@ TEST_F(Dhcp4ParserTest, reservations) {
string config = "{ \"interfaces\": [ \"*\" ],"
"\"rebind-timer\": 2000, "
"\"renew-timer\": 1000, "
"\"subnet4\": [ { "
"\"subnet4\": [ "
" { "
" \"pools\": [ { \"pool\": \"192.0.2.1 - 192.0.2.100\" } ],"
" \"subnet\": \"192.0.2.0/24\", "
" \"id\": 123,"
......@@ -3357,14 +3358,15 @@ TEST_F(Dhcp4ParserTest, reservations) {
}
// This test verfies that the bogus host reservation would trigger a
// server configuration error. In this case the "hw-address" parameter in the
// reservation is misspelled.
// server configuration error.
TEST_F(Dhcp4ParserTest, reservationBogus) {
// Case 1: misspelled hw-address parameter.
ConstElementPtr x;
string config = "{ \"interfaces\": [ \"*\" ],"
"\"rebind-timer\": 2000, "
"\"renew-timer\": 1000, "
"\"subnet4\": [ { "
"\"subnet4\": [ "
" { "
" \"pools\": [ { \"pool\": \"192.0.4.101 - 192.0.4.150\" } ],"
" \"subnet\": \"192.0.4.0/24\","
" \"id\": 542,"
......@@ -3380,8 +3382,65 @@ TEST_F(Dhcp4ParserTest, reservationBogus) {
ElementPtr json = Element::fromJSON(config);
CfgMgr::instance().clear();
EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
checkResult(x, 1);
// Case 2: DUID and HW Address both specified.
config = "{ \"interfaces\": [ \"*\" ],"
"\"rebind-timer\": 2000, "
"\"renew-timer\": 1000, "
"\"subnet4\": [ "
" { "
" \"pools\": [ { \"pool\": \"192.0.4.101 - 192.0.4.150\" } ],"
" \"subnet\": \"192.0.4.0/24\","
" \"id\": 542,"
" \"reservations\": ["
" {"
" \"duid\": \"01:02:03:04:05:06\","
" \"hw-address\": \"06:05:04:03:02:01\","
" \"ip-address\": \"192.0.4.102\","
" \"hostname\": \"\""
" }"
" ]"
" } ],"
"\"valid-lifetime\": 4000 }";
json = Element::fromJSON(config);
// Remove existing configuration, if any.
CfgMgr::instance().clear();
EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
checkResult(x, 1);
// Case 3: Neither ip address nor hostname specified.
config = "{ \"interfaces\": [ \"*\" ],"
"\"rebind-timer\": 2000, "
"\"renew-timer\": 1000, "
"\"subnet4\": [ "
" { "
" \"pools\": [ { \"pool\": \"192.0.4.101 - 192.0.4.150\" } ],"
" \"subnet\": \"192.0.4.0/24\","
" \"id\": 542,"
" \"reservations\": ["
" {"
" \"hw-address\": \"06:05:04:03:02:01\""
" }"
" ]"
" } ],"
"\"valid-lifetime\": 4000 }";
json = Element::fromJSON(config);
// Remove existing configuration, if any.
CfgMgr::instance().clear();
EXPECT_NO_THROW(x = configureDhcp4Server(*srv_, json));
checkResult(x, 1);
}
}
src/bin/dhcp6/dhcp6.spec
View file @ ad0f44ba
......@@ -377,13 +377,13 @@
{
"item_name": "hw-address",
"item_type": "string",
"item_optional": false,
"item_optional": true,
"item_default": ""
},
{
"item_name": "duid",
"item_type": "string",
"item_optional": false,
"item_optional": true,
"item_default": ""
},
{
......
src/bin/dhcp6/tests/config_parser_unittest.cc
View file @ ad0f44ba
......@@ -3407,7 +3407,8 @@ TEST_F(Dhcp6ParserTest, reservations) {
string config = "{ \"interfaces\": [ \"*\" ],"
"\"rebind-timer\": 2000, "
"\"renew-timer\": 1000, "
"\"subnet6\": [ { "
"\"subnet6\": [ "
" { "
" \"pools\": [ { \"pool\": \"2001:db8:1::/80\" } ],"
" \"subnet\": \"2001:db8:1::/64\", "
" \"id\": 123,"
......@@ -3447,7 +3448,8 @@ TEST_F(Dhcp6ParserTest, reservations) {
" \"hostname\": \"\""
" }"
" ]"
" } ],"
" } "
"], "
"\"preferred-lifetime\": 3000,"
"\"valid-lifetime\": 4000 }";
......@@ -3537,14 +3539,15 @@ TEST_F(Dhcp6ParserTest, reservations) {
}
// This test verfies that the bogus host reservation would trigger a
// server configuration error. In this case the "duid" parameter in the
// reservation is misspelled.
// server configuration error.
TEST_F(Dhcp6ParserTest, reservationBogus) {
// Case 1: misspelled "duid" parameter.
ConstElementPtr x;
string config = "{ \"interfaces\": [ \"*\" ],"
"\"rebind-timer\": 2000, "
"\"renew-timer\": 1000, "
"\"subnet6\": [ { "
"\"subnet6\": [ "
" { "
" \"pools\": [ ],"
" \"subnet\": \"2001:db8:3::/64\", "
" \"id\": 542,"
......@@ -3555,7 +3558,8 @@ TEST_F(Dhcp6ParserTest, reservationBogus) {
" \"hostname\": \"\""
" }"
" ]"
" } ],"
" } "
"], "
"\"preferred-lifetime\": 3000,"
"\"valid-lifetime\": 4000 }";
......@@ -3563,6 +3567,64 @@ TEST_F(Dhcp6ParserTest, reservationBogus) {
EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
checkResult(x, 1);
// Case 2: DUID and HW Address both specified.
config = "{ \"interfaces\": [ \"*\" ],"
"\"rebind-timer\": 2000, "
"\"renew-timer\": 1000, "
"\"subnet6\": [ "
" { "
" \"pools\": [ ],"
" \"subnet\": \"2001:db8:3::/64\", "
" \"id\": 542,"
" \"reservations\": ["
" {"
" \"hw-address\": \"01:02:03:04:05:06\","
" \"duid\": \"0A:09:08:07:06:05:04:03:02:01\","
" \"prefixes\": [ \"2001:db8:3:2::/96\" ],"
" \"hostname\": \"\""
" }"
" ]"
" } "
"], "
"\"preferred-lifetime\": 3000,"
"\"valid-lifetime\": 4000 }";
json = Element::fromJSON(config);
// Remove existing configuration, if any.
CfgMgr::instance().clear();
EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
checkResult(x, 1);
// Case 3: Neither ip address nor hostname specified.
config = "{ \"interfaces\": [ \"*\" ],"
"\"rebind-timer\": 2000, "
"\"renew-timer\": 1000, "
"\"subnet6\": [ "
" { "
" \"pools\": [ ],"
" \"subnet\": \"2001:db8:3::/64\", "
" \"id\": 542,"
" \"reservations\": ["
" {"
" \"duid\": \"0A:09:08:07:06:05:04:03:02:01\""
" }"
" ]"
" } "
"], "
"\"preferred-lifetime\": 3000,"
"\"valid-lifetime\": 4000 }";
json = Element::fromJSON(config);
// Remove existing configuration, if any.
CfgMgr::instance().clear();
EXPECT_NO_THROW(x = configureDhcp6Server(srv_, json));
checkResult(x, 1);
}
};
src/lib/dhcpsrv/cfg_hosts.cc
View file @ ad0f44ba
......@@ -14,6 +14,7 @@
#include <dhcpsrv/cfg_hosts.h>
#include <exceptions/exceptions.h>
#include <ostream>
using namespace isc::asiolink;
......@@ -165,10 +166,26 @@ CfgHosts::add(const HostPtr& host) {
isc_throw(BadValue, "specified host object must not be NULL when it"
" is added to the configuration");
}
/// @todo This may need further sanity checks. For example, a duplicate
/// should be rejected.
HWAddrPtr hwaddr = host->getHWAddress();
DuidPtr duid = host->getDuid();
// There should be at least one resource reserved: hostname, IPv4
// address, IPv6 address or prefix.
if (host->getHostname().empty() &&
(host->getIPv4Reservation() == IOAddress("0.0.0.0")) &&
(!host->hasIPv6Reservation())) {
std::ostringstream s;
if (hwaddr) {
s << "for DUID: " << hwaddr->toText();
} else if (duid) {
s << "for HW address: " << duid->toText();
}
isc_throw(BadValue, "specified reservation " << s
<< " must include at least one resource, i.e. "
"hostname, IPv4 address or IPv6 address/prefix");
}
// Check for duplicates for the specified IPv4 subnet.
if (get4(host->getIPv4SubnetID(), hwaddr, duid)) {
isc_throw(DuplicateHost, "failed to add new host using the HW"
......@@ -177,7 +194,7 @@ CfgHosts::add(const HostPtr& host) {
<< "' to the IPv4 subnet id '" << host->getIPv4SubnetID()
<< "' as this host has already been added");
// Checek for duplicates for the specified IPv6 subnet.
// Check for duplicates for the specified IPv6 subnet.
} else if (get6(host->getIPv6SubnetID(), duid, hwaddr)) {
isc_throw(DuplicateHost, "failed to add new host using the HW"
" address '" << (hwaddr ? hwaddr->toText(false) : "(null)")
......@@ -186,6 +203,8 @@ CfgHosts::add(const HostPtr& host) {
<< "' as this host has already been added");
}
/// @todo This may need further sanity checks.
// This is a new instance - add it.
hosts_.insert(host);
}
......
src/lib/dhcpsrv/host.cc
View file @ ad0f44ba
......@@ -194,6 +194,11 @@ Host::getIPv6Reservations(const IPv6Resrv::Type& type) const {
return (ipv6_reservations_.equal_range(type));
}
bool
Host::hasIPv6Reservation() const {
return (!ipv6_reservations_.empty());
}
bool
Host::hasReservation(const IPv6Resrv& reservation) const {
IPv6ResrvRange reservations = getIPv6Reservations(reservation.getType());
......
src/lib/dhcpsrv/host.h
View file @ ad0f44ba
......@@ -352,6 +352,11 @@ public:
/// the specified type.
IPv6ResrvRange getIPv6Reservations(const IPv6Resrv::Type& type) const;
/// @brief Checks if there is at least one IPv6 reservation for this host.
///
/// @return true if there is a reservation for the host, false otherwise.
bool hasIPv6Reservation() const;
/// @brief Checks if specified IPv6 reservation exists for the host.
///
/// @param reservation A reservation to be checked for the host.
......
src/lib/dhcpsrv/parsers/dhcp_parsers.cc
View file @ ad0f44ba
......@@ -966,6 +966,9 @@ void
SubnetConfigParser::build(ConstElementPtr subnet) {
BOOST_FOREACH(ConfigPair param, subnet->mapValue()) {
// Host reservations must be parsed after subnet specific parameters.
// Note that the reservation parsing will be invoked by the build()
// in the derived classes, i.e. Subnet4ConfigParser and
// Subnet6ConfigParser.
if (param.first == "reservations") {
continue;
}
......
src/lib/dhcpsrv/tests/Makefile.am
View file @ ad0f44ba
......@@ -70,6 +70,7 @@ libdhcpsrv_unittests_SOURCES += daemon_unittest.cc
libdhcpsrv_unittests_SOURCES += dbaccess_parser_unittest.cc
libdhcpsrv_unittests_SOURCES += host_unittest.cc
libdhcpsrv_unittests_SOURCES += host_reservation_parser_unittest.cc
libdhcpsrv_unittests_SOURCES += host_reservations_list_parser_unittest.cc
libdhcpsrv_unittests_SOURCES += lease_file_io.cc lease_file_io.h
libdhcpsrv_unittests_SOURCES += lease_unittest.cc
libdhcpsrv_unittests_SOURCES += lease_mgr_factory_unittest.cc
......
src/lib/dhcpsrv/tests/cfg_hosts_unittest.cc
View file @ ad0f44ba
......@@ -343,53 +343,59 @@ TEST_F(CfgHostsTest, duplicatesSubnet4DUID) {
}
// This test verifies that it is not possible to add the same Host to the
// same IPv4 subnet twice.
// same IPv6 subnet twice.
TEST_F(CfgHostsTest, duplicatesSubnet6HWAddr) {
CfgHosts cfg;
// Add a host.
ASSERT_NO_THROW(cfg.add(HostPtr(new Host(hwaddrs_[0]->toText(false),
"hw-address",
SubnetID(10), SubnetID(1),
IOAddress("0.0.0.0")))));
IOAddress("0.0.0.0"),
"foo.example.com"))));
// Try to add the host with the same HW address to the same subnet. The fact
// that the IP address is different here shouldn't really matter.
EXPECT_THROW(cfg.add(HostPtr(new Host(hwaddrs_[0]->toText(false),
"hw-address",
SubnetID(11), SubnetID(1),
IOAddress("0.0.0.0")))),
IOAddress("0.0.0.0"),
"foo.example.com"))),
isc::dhcp::DuplicateHost);
// Now try to add it to a different subnet. It should go through.
EXPECT_NO_THROW(cfg.add(HostPtr(new Host(hwaddrs_[0]->toText(false),
"hw-address",
SubnetID(11), SubnetID(2),
IOAddress("0.0.0.0")))));
IOAddress("0.0.0.0"),
"foo.example.com"))));
}
// This test verifies that it is not possible to add the same Host to the
// same IPv4 subnet twice.
// same IPv6 subnet twice.
TEST_F(CfgHostsTest, duplicatesSubnet6DUID) {
CfgHosts cfg;
// Add a host.
ASSERT_NO_THROW(cfg.add(HostPtr(new Host(duids_[0]->toText(),
"duid",
SubnetID(10), SubnetID(1),
IOAddress("0.0.0.0")))));
IOAddress("0.0.0.0"),
"foo.example.com"))));
// Try to add the host with the same DUID to the same subnet. The fact
// that the IP address is different here shouldn't really matter.
EXPECT_THROW(cfg.add(HostPtr(new Host(duids_[0]->toText(),
"duid",
SubnetID(11), SubnetID(1),
IOAddress("0.0.0.0")))),
IOAddress("0.0.0.0"),
"foo.example.com"))),
isc::dhcp::DuplicateHost);
// Now try to add it to a different subnet. It should go through.
EXPECT_NO_THROW(cfg.add(HostPtr(new Host(duids_[0]->toText(),
"duid",
SubnetID(11), SubnetID(2),
IOAddress("0.0.0.0")))));
IOAddress("0.0.0.0"),
"foo.example.com"))));
}
......
src/lib/dhcpsrv/tests/host_reservation_parser_unittest.cc
View file @ ad0f44ba
......@@ -141,6 +141,30 @@ TEST_F(HostReservationParserTest, dhcp4DUID) {
EXPECT_TRUE(hosts[0]->getHostname().empty());
}
// This test verifies that the parser can parse the reservation entry
// when IPv4 address is specified, but hostname is not.
TEST_F(HostReservationParserTest, dhcp4NoHostname) {
std::string config = "{ \"duid\": \"01:02:03:04:05:06:07:08:09:0A\","
"\"ip-address\": \"192.0.2.10\" }";
ElementPtr config_element = Element::fromJSON(config);
HostReservationParser4 parser(SubnetID(10));
ASSERT_NO_THROW(parser.build(config_element));
CfgHostsPtr cfg_hosts = CfgMgr::instance().getStagingCfg()->getCfgHosts();
HostCollection hosts;
ASSERT_NO_THROW(hosts = cfg_hosts->getAll(HWAddrPtr(), duid_));
ASSERT_EQ(1, hosts.size());
EXPECT_EQ(10, hosts[0]->getIPv4SubnetID());
EXPECT_EQ(0, hosts[0]->getIPv6SubnetID());
EXPECT_EQ("192.0.2.10", hosts[0]->getIPv4Reservation().toText());
EXPECT_TRUE(hosts[0]->getHostname().empty());
}
// This test verifies that the configuration parser for host reservations
// throws an exception when IPv6 address is specified for IPv4 address
// reservation.
......@@ -166,6 +190,53 @@ TEST_F(HostReservationParserTest, noIdentifier) {
EXPECT_THROW(parser.build(config_element), DhcpConfigError);
}
// This test verifies that the configuration parser for host reservations
// throws an exception when neither ip address nor hostname is specified.
TEST_F(HostReservationParserTest, noResource) {
std::string config = "{ \"hw-address\": \"01:02:03:04:05:06\" }";
ElementPtr config_element = Element::fromJSON(config);
HostReservationParser4 parser(SubnetID(10));
EXPECT_THROW(parser.build(config_element), DhcpConfigError);
}
// This test verifies that the parser can parse the reservation entry
// when IP address is not specified, but hostname is specified.
TEST_F(HostReservationParserTest, noIPAddress) {
std::string config = "{ \"duid\": \"01:02:03:04:05:06:07:08:09:0A\","
"\"hostname\": \"foo.example.com\" }";
ElementPtr config_element = Element::fromJSON(config);
HostReservationParser4 parser(SubnetID(10));
ASSERT_NO_THROW(parser.build(config_element));
CfgHostsPtr cfg_hosts = CfgMgr::instance().getStagingCfg()->getCfgHosts();
HostCollection hosts;
ASSERT_NO_THROW(hosts = cfg_hosts->getAll(HWAddrPtr(), duid_));
ASSERT_EQ(1, hosts.size());
EXPECT_EQ(10, hosts[0]->getIPv4SubnetID());
EXPECT_EQ(0, hosts[0]->getIPv6SubnetID());
EXPECT_EQ("0.0.0.0", hosts[0]->getIPv4Reservation().toText());
EXPECT_EQ("foo.example.com", hosts[0]->getHostname());
}
// This test verifies that the configuration parser for host reservations
// throws an exception when hostname is empty, and IP address is not
// specified.
TEST_F(HostReservationParserTest, emptyHostname) {
std::string config = "{ \"hw-address\": \"01:02:03:04:05:06\","
"\"hostname\": \"\" }";
ElementPtr config_element = Element::fromJSON(config);
HostReservationParser4 parser(SubnetID(10));
EXPECT_THROW(parser.build(config_element), DhcpConfigError);
}
// This test verifies that the configuration parser for host reservations
// throws an exception when invalid IP address is specified.
TEST_F(HostReservationParserTest, malformedAddress) {
......@@ -264,6 +335,44 @@ TEST_F(HostReservationParserTest, dhcp6DUID) {
ASSERT_EQ(0, std::distance(prefixes.first, prefixes.second));
}
// This test verfies that the parser can parse the IPv6 reservation entry
// which lacks hostname parameter.
TEST_F(HostReservationParserTest, dhcp6NoHostname) {
std::string config = "{ \"duid\": \"01:02:03:04:05:06:07:08:09:0A\","
"\"ip-addresses\": [ \"2001:db8:1::100\", \"2001:db8:1::200\" ],"
"\"prefixes\": [ ] }";
ElementPtr config_element = Element::fromJSON(config);
HostReservationParser6 parser(SubnetID(12));
ASSERT_NO_THROW(parser.build(config_element));
CfgHostsPtr cfg_hosts = CfgMgr::instance().getStagingCfg()->getCfgHosts();
HostCollection hosts;
ASSERT_NO_THROW(hosts = cfg_hosts->getAll(HWAddrPtr(), duid_));
ASSERT_EQ(1, hosts.size());
EXPECT_EQ(0, hosts[0]->getIPv4SubnetID());
EXPECT_EQ(12, hosts[0]->getIPv6SubnetID());
EXPECT_TRUE(hosts[0]->getHostname().empty());
IPv6ResrvRange addresses = hosts[0]->
getIPv6Reservations(IPv6Resrv::TYPE_NA);
ASSERT_EQ(2, std::distance(addresses.first, addresses.second));
EXPECT_TRUE(reservationExists(IPv6Resrv(IPv6Resrv::TYPE_NA,
IOAddress("2001:db8:1::100")),
addresses));
EXPECT_TRUE(reservationExists(IPv6Resrv(IPv6Resrv::TYPE_NA,
IOAddress("2001:db8:1::200")),
addresses));
IPv6ResrvRange prefixes = hosts[0]->getIPv6Reservations(IPv6Resrv::TYPE_PD);
ASSERT_EQ(0, std::distance(prefixes.first, prefixes.second));
}
// This test verifies that the configuration parser throws an exception
// when IPv4 address is specified for IPv6 reservation.
TEST_F(HostReservationParserTest, dhcp6IPv4Address) {
......
src/lib/dhcpsrv/tests/host_reservations_list_parser_unittest.cc 0 → 100644
View file @ ad0f44ba
// Copyright (C) 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
// copyright notice and this permission notice appear in all copies.
//
// THE SOFTWARE IS PROVIDED "AS IS" AND ISC DISCLAIMS ALL WARRANTIES WITH
// REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY
// AND FITNESS. IN NO EVENT SHALL ISC BE LIABLE FOR ANY SPECIAL, DIRECT,
// INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM
// LOSS OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE
// OR OTHER TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
// PERFORMANCE OF THIS SOFTWARE.
#include <config.h>
#include <cc/data.h>
#include <dhcp/duid.h>
#include <dhcp/hwaddr.h>
#include <dhcpsrv/cfgmgr.h>
#include <dhcpsrv/cfg_hosts.h>
#include <dhcpsrv/host.h>
#include <dhcpsrv/subnet_id.h>
#include <dhcpsrv/parsers/host_reservation_parser.h>
#include <dhcpsrv/parsers/host_reservations_list_parser.h>
#include <gtest/gtest.h>
using namespace isc::data;
using namespace isc::dhcp;
namespace {
/// @brief Test fixture class for @c HostReservationsListParser.
class HostReservationsListParserTest : public ::testing::Test {
protected:
/// @brief Setup for each test.
///
/// Clears the configuration in the @c CfgMgr. It alse initializes
/// hwaddr_ and duid_ class members.
virtual void SetUp();
/// @brief Clean up after each test.
///
/// Clears the configuration in the @c CfgMgr.
virtual void TearDown();
/// @brief HW Address object used by tests.
HWAddrPtr hwaddr_;
/// @brief DUID object used by tests.
DuidPtr duid_;
};
void
HostReservationsListParserTest::SetUp() {
CfgMgr::instance().clear();
// Initialize HW address used by tests.
const uint8_t hwaddr_data[] = { 0x01, 0x02, 0x03, 0x04, 0x05, 0x06 };
hwaddr_ = HWAddrPtr(new HWAddr(hwaddr_data, sizeof(hwaddr_data),
HTYPE_ETHER));