diff --git a/ChangeLog b/ChangeLog index 89011a0b7a..7f8ae861b8 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,9 +1,8 @@ 4XX. [func] tomek - A new library (libb10-dhcpsrv) has been create. Currently its - functionality is limited to a Configuration Manager. CfgMgr - currently only supports basic configuration storage for DHCPv6 - server, but that capability is expected to be expanded in a near - future. + A new library (libb10-dhcpsrv) has been created. At present, it + only holds the code for the DHCP Configuration Manager. Currently + this object only supports basic configuration storage for the DHCPv6 + server, but that capability will be expanded. (Trac #2238, git TBD) 475. [func] naokikambe diff --git a/src/lib/asiolink/io_address.h b/src/lib/asiolink/io_address.h index 2e09260066..a3bb61ac29 100644 --- a/src/lib/asiolink/io_address.h +++ b/src/lib/asiolink/io_address.h @@ -150,18 +150,15 @@ public: /// Operations within one protocol family are obvious. /// Comparisons between v4 and v6 will allways return v4 /// being smaller. This follows boost::asio::ip implementation - bool smallerThan(const IOAddress& other) const { - if (this->getFamily() < other.getFamily()) { - return (true); - } - if (this->getFamily() > other.getFamily()) { - return (false); - } - if (this->getFamily() == AF_INET6) { - return (this->asio_address_.to_v6() < other.asio_address_.to_v6()); - } else { - return (this->asio_address_.to_v4() < other.asio_address_.to_v4()); + bool lessThan(const IOAddress& other) const { + if (this->getFamily() == other.getFamily()) { + if (this->getFamily() == AF_INET6) { + return (this->asio_address_.to_v6() < other.asio_address_.to_v6()); + } else { + return (this->asio_address_.to_v4() < other.asio_address_.to_v4()); + } } + return (this->getFamily() < other.getFamily()); } /// \brief Checks if one address is smaller or equal than the other @@ -173,7 +170,7 @@ public: if (equals(other)) { return (true); } - return (smallerThan(other)); + return (lessThan(other)); } /// \brief Checks if one address is smaller than the other @@ -182,7 +179,7 @@ public: /// /// See \ref smaller_than method for details. bool operator<(const IOAddress& other) const { - return (smallerThan(other)); + return (lessThan(other)); } /// \brief Checks if one address is smaller or equal than the other diff --git a/src/lib/asiolink/tests/io_address_unittest.cc b/src/lib/asiolink/tests/io_address_unittest.cc index 2530ca5e87..a1a90760eb 100644 --- a/src/lib/asiolink/tests/io_address_unittest.cc +++ b/src/lib/asiolink/tests/io_address_unittest.cc @@ -100,7 +100,7 @@ TEST(IOAddressTest, uint32) { EXPECT_EQ(addr3.toText(), "192.0.2.5"); } -TEST(IOAddressTest, compare) { +TEST(IOAddressTest, lessThanEqual) { IOAddress addr1("192.0.2.5"); IOAddress addr2("192.0.2.6"); IOAddress addr3("0.0.0.0"); diff --git a/src/lib/dhcp/addr_utilities.cc b/src/lib/dhcp/addr_utilities.cc index ddd761a00a..15999d496c 100644 --- a/src/lib/dhcp/addr_utilities.cc +++ b/src/lib/dhcp/addr_utilities.cc @@ -14,46 +14,80 @@ #include +using namespace isc::asiolink; + namespace isc { namespace dhcp { isc::asiolink::IOAddress firstAddrInPrefix(const isc::asiolink::IOAddress& prefix, - uint8_t len) { + uint8_t len) { static char bitMask[]= { 0, 0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe, 0xff }; - uint8_t packed[16]; + uint8_t packed[V6ADDRESS_LEN]; + // First we copy the whole address as 16 bytes. memcpy(packed, prefix.getAddress().to_v6().to_bytes().data(), 16); + // If the length is divisible by 8, it is simple. We just zero out the host + // part. Otherwise we need to handle the byte that has to be partially + // zeroed. if (len % 8 != 0) { + + // Get the appropriate mask. It has relevant bits (those that should + // stay) set and irrelevant (those that should be wiped) cleared. uint8_t mask = bitMask[len % 8]; + + // Let's leave only whatever the mask says should not be cleared. packed[len / 8] = packed[len / 8] & mask; - len = (len/8 + 1) * 8; + + // Since we have just dealt with this byte, let's move the prefix length + // to the beginning of the next byte (len is expressed in bits). + len = (len / 8 + 1) * 8; } - for (int i = len / 8; i < 16; ++i) { + + // Clear out the remaining bits. + for (int i = len / 8; i < sizeof(packed); ++i) { packed[i] = 0x0; } + // Finally, let's wrap this into nice and easy IOAddress object. return (isc::asiolink::IOAddress::from_bytes(AF_INET6, packed)); } isc::asiolink::IOAddress lastAddrInPrefix(const isc::asiolink::IOAddress& prefix, - uint8_t len) { + uint8_t len) { static char bitMask[]= { 0, 0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe, 0xff }; - uint8_t packed[16]; + uint8_t packed[V6ADDRESS_LEN]; + // First we copy the whole address as 16 bytes. memcpy(packed, prefix.getAddress().to_v6().to_bytes().data(), 16); + // if the length is divisible by 8, it is simple. We just fill the host part + // with ones. Otherwise we need to handle the byte that has to be partially + // zeroed. if (len % 8 != 0) { + // Get the appropriate mask. It has relevant bits (those that should + // stay) set and irrelevant (those that should be set to 1) cleared. uint8_t mask = bitMask[len % 8]; + + // Let's set those irrelevant bits with 1. It would be perhaps + // easier to not use negation here and invert bitMask content. However, + // with this approach, we can use the same mask in first and last + // address calculations. packed[len / 8] = packed[len / 8] | ~mask; - len = (len/8 + 1) * 8; + + // Since we have just dealt with this byte, let's move the prefix length + // to the beginning of the next byte (len is expressed in bits). + len = (len / 8 + 1) * 8; } - for (int i = len / 8; i < 16; ++i) { + + // Finally set remaining bits to 1. + for (int i = len / 8; i < sizeof(packed); ++i) { packed[i] = 0xff; } + // Finally, let's wrap this into nice and easy IOAddress object. return (isc::asiolink::IOAddress::from_bytes(AF_INET6, packed)); } diff --git a/src/lib/dhcp/addr_utilities.h b/src/lib/dhcp/addr_utilities.h index d82a8a693d..15532d0543 100644 --- a/src/lib/dhcp/addr_utilities.h +++ b/src/lib/dhcp/addr_utilities.h @@ -18,32 +18,36 @@ namespace isc { namespace dhcp { /// This code is based on similar code from the Dibbler project. I, Tomasz Mrugalski, -/// as a sole creater of that code hereby release it under BSD license for the benefit +/// as a sole creator of that code hereby release it under BSD license for the benefit /// of the BIND10 project. /// @brief returns a first address in a given prefix /// /// Example: For 2001:db8:1::deaf:beef and length /120 the function will return -/// 2001:db8:1::dead:bee0. See also @ref lastAddrInPrefix. +/// 2001:db8:1::dead:be00. See also @ref lastAddrInPrefix. +/// +/// @todo It currently works for v6 only and will throw if v4 address is passed. /// /// @param prefix and address that belongs to a prefix /// @param len prefix length /// /// @return first address from a prefix isc::asiolink::IOAddress firstAddrInPrefix(const isc::asiolink::IOAddress& prefix, - uint8_t len); + uint8_t len); /// @brief returns a last address in a given prefix /// /// Example: For 2001:db8:1::deaf:beef and length /112 the function will return /// 2001:db8:1::dead:ffff. See also @ref firstAddrInPrefix. /// +/// @todo It currently works for v6 only and will throw if v4 address is passed. +/// /// @param prefix and address that belongs to a prefix /// @param len prefix length /// /// @return first address from a prefix isc::asiolink::IOAddress lastAddrInPrefix(const isc::asiolink::IOAddress& prefix, - uint8_t len); + uint8_t len); }; }; diff --git a/src/lib/dhcp/cfgmgr.cc b/src/lib/dhcp/cfgmgr.cc index 1bb76c8085..ff55a90ccf 100644 --- a/src/lib/dhcp/cfgmgr.cc +++ b/src/lib/dhcp/cfgmgr.cc @@ -27,8 +27,8 @@ Pool::Pool(const isc::asiolink::IOAddress& first, :id_(getNextID()), first_(first), last_(last) { } -bool Pool::inRange(const isc::asiolink::IOAddress& addr) { - return ( first_.smallerEqual(addr) && addr.smallerEqual(last_) ); +bool Pool::inRange(const isc::asiolink::IOAddress& addr) const { + return (first_.smallerEqual(addr) && addr.smallerEqual(last_)); } Pool6::Pool6(Pool6Type type, const isc::asiolink::IOAddress& first, @@ -46,12 +46,14 @@ Pool6::Pool6(Pool6Type type, const isc::asiolink::IOAddress& first, // we need to comment it and uncomment lines below. // On one hand, letting the user specify 2001::f - 2001::1 is nice, but // on the other hand, 2001::1 may be a typo and the user really meant - // 2001::1:0 (or 1something), so a at least a warning would be useful. + // 2001::1:0 (or 1 followed by some hex digit), so a at least a warning + // would be useful. // first_ = last; // last_ = first; } + // TYPE_PD is not supported by this constructor. first-last style // parameters are for IA and TA only. There is another dedicated // constructor for that (it uses prefix/length) @@ -75,6 +77,9 @@ Pool6::Pool6(Pool6Type type, const isc::asiolink::IOAddress& prefix, isc_throw(BadValue, "Invalid prefix length"); } + /// @todo: We should probably implement checks against weird addresses + /// here, like ::, starting with fe80, starting with ff etc. . + // Let's now calculate the last address in defined pool last_ = lastAddrInPrefix(prefix, prefix_len); } @@ -91,11 +96,11 @@ Subnet::Subnet(const isc::asiolink::IOAddress& prefix, uint8_t len, } } -bool Subnet::inRange(const isc::asiolink::IOAddress& addr) { +bool Subnet::inRange(const isc::asiolink::IOAddress& addr) const { IOAddress first = firstAddrInPrefix(prefix_, prefix_len_); IOAddress last = lastAddrInPrefix(prefix_, prefix_len_); - return ( (first <= addr) && (addr <= last) ); + return ((first <= addr) && (addr <= last)); } Subnet6::Subnet6(const isc::asiolink::IOAddress& prefix, uint8_t length, @@ -106,7 +111,7 @@ Subnet6::Subnet6(const isc::asiolink::IOAddress& prefix, uint8_t length, :Subnet(prefix, length, t1, t2, valid_lifetime), preferred_(preferred_lifetime){ if (prefix.getFamily() != AF_INET6) { - isc_throw(BadValue, "Invalid prefix " << prefix.toText() + isc_throw(BadValue, "Non IPv6 prefix " << prefix.toText() << " specified in subnet6"); } } @@ -121,6 +126,8 @@ void Subnet6::addPool6(const Pool6Ptr& pool) { << ") subnet6"); } + /// @todo: Check that pools do not overlap + pools_.push_back(pool); } @@ -153,6 +160,13 @@ Subnet6Ptr CfgMgr::getSubnet6(const isc::asiolink::IOAddress& hint) { // If there's only one subnet configured, let's just use it + // The idea is to keep small deployments easy. In a small network - one + // router that also runs DHCPv6 server. Users specifies a single pool and + // expects it to just work. Without this, the server would complain that it + // doesn't have IP address on its interfaces that matches that + // configuration. Such requirement makes sense in IPv4, but not in IPv6. + // The server does not need to have a global address (using just link-local + // is ok for DHCPv6 server) from the pool it serves. if (subnets6_.size() == 1) { return (subnets6_[0]); } diff --git a/src/lib/dhcp/cfgmgr.h b/src/lib/dhcp/cfgmgr.h index f3ffe98b27..48cb847fe2 100644 --- a/src/lib/dhcp/cfgmgr.h +++ b/src/lib/dhcp/cfgmgr.h @@ -31,7 +31,7 @@ class Pool6; class Subnet6; -/// @brief this template specifes a parameter value +/// @brief this template specifies a parameter value /// /// This template class is used to store configuration parameters, like lifetime or T1. /// It defines 3 parameters: min, default, and max value. There are 2 constructors: @@ -48,15 +48,18 @@ public: /// /// Typically: uint32_t to Triplet assignment. It is very convenient /// to be able to simply write Triplet x = 7; - Triplet& operator = (T base_type) { - return Triplet(base_type); + Triplet operator=(T other) { + min_ = other; + default_ = other; + max_ = other; + return *this; } /// @brief triplet to base type conversion /// /// Typically: Triplet to uint32_t assignment. It is very convenient /// to be able to simply write uint32_t z = x; (where x is a Triplet) - operator T () const { + operator T() const { return (default_); } @@ -73,7 +76,7 @@ public: /// @throw BadValue if min <= def <= max rule is violated Triplet(T min, T def, T max) :min_(min), default_(def), max_(max) { - if ( (min_>def) || (def > max_) ) { + if ( (min_ > def) || (def > max_) ) { isc_throw(BadValue, "Invalid triplet values."); } } @@ -127,6 +130,7 @@ public: /// @brief returns Pool-id /// + /// @return pool-id value /// Pool-id is an unique value that can be used to identify a pool. uint32_t getId() const { return (id_); @@ -148,7 +152,7 @@ public: /// @brief Checks if a given address is in the range. /// /// @return true, if the address is in pool - bool inRange(const isc::asiolink::IOAddress& addr); + bool inRange(const isc::asiolink::IOAddress& addr) const; protected: @@ -170,7 +174,7 @@ protected: /// @brief pool-id /// - /// This ID is used to indentify this specific pool. + /// This ID is used to identify this specific pool. uint32_t id_; /// @brief The first address in a pool @@ -227,7 +231,7 @@ public: return (type_); } -protected: +private: /// @brief defines a pool type Pool6Type type_; @@ -255,7 +259,7 @@ typedef std::vector Pool6Collection; class Subnet { public: /// @brief checks if specified address is in range - bool inRange(const isc::asiolink::IOAddress& addr); + bool inRange(const isc::asiolink::IOAddress& addr) const; /// @brief return valid-lifetime for addresses in that prefix Triplet getValid() const { @@ -317,29 +321,54 @@ protected: /// This class represents an IPv6 subnet. class Subnet6 : public Subnet { public: + + /// @brief Constructor with all parameters + /// + /// @param prefix Subnet6 prefix + /// @param length prefix length + /// @param t1 renewal timer (in seconds) + /// @param t2 rebind timer (in seconds) + /// @param preferred_lifetime preferred lifetime of leases (in seconds) + /// @param valid_lifetime preferred lifetime of leases (in seconds) Subnet6(const isc::asiolink::IOAddress& prefix, uint8_t length, const Triplet& t1, const Triplet& t2, const Triplet& preferred_lifetime, const Triplet& valid_lifetime); + /// @brief Returns preverred lifetime (in seconds) + /// + /// @return a triplet with preferred lifetime Triplet getPreferred() const { return (preferred_); } + /// @brief Returns a pool that specified address belongs to + /// + /// @param hint address that the returned pool should cover (optional) + /// @return Pointer to found pool6 (or NULL) Pool6Ptr getPool6(const isc::asiolink::IOAddress& hint = isc::asiolink::IOAddress("::")); + /// @brief Adds a new pool. + /// @param pool pool to be added void addPool6(const Pool6Ptr& pool); + /// @brief returns all pools + /// + /// The reference is only valid as long as the object that + /// returned it. + /// + /// @return a collection of all pools const Pool6Collection& getPools() const { return pools_; } protected: - /// collection of pools in that list + /// @brief collection of pools in that list Pool6Collection pools_; + /// @brief a triplet with preferred lifetime (in seconds) Triplet preferred_; }; diff --git a/src/lib/dhcp/tests/Makefile.am b/src/lib/dhcp/tests/Makefile.am index 42abcb65bc..70c87c8c77 100644 --- a/src/lib/dhcp/tests/Makefile.am +++ b/src/lib/dhcp/tests/Makefile.am @@ -51,6 +51,7 @@ libdhcpsrv_unittests_LDADD = $(GTEST_LDADD) libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/exceptions/libb10-exceptions.la libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/asiolink/libb10-asiolink.la libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/dhcp/libb10-dhcpsrv.la +libdhcpsrv_unittests_LDADD += $(top_builddir)/src/lib/log/libb10-log.la if USE_CLANGPP @@ -66,6 +67,7 @@ libdhcp___unittests_LDADD += $(top_builddir)/src/lib/log/libb10-log.la libdhcp___unittests_LDADD += $(top_builddir)/src/lib/util/libb10-util.la libdhcp___unittests_LDADD += $(top_builddir)/src/lib/asiolink/libb10-asiolink.la libdhcp___unittests_LDADD += $(top_builddir)/src/lib/exceptions/libb10-exceptions.la +libdhcp___unittests_LDADD += $(top_builddir)/src/lib/log/libb10-log.la libdhcp___unittests_LDADD += $(GTEST_LDADD) endif diff --git a/src/lib/dhcp/tests/addr_utilities_unittest.cc b/src/lib/dhcp/tests/addr_utilities_unittest.cc index c9daa9ba49..46207b29db 100644 --- a/src/lib/dhcp/tests/addr_utilities_unittest.cc +++ b/src/lib/dhcp/tests/addr_utilities_unittest.cc @@ -53,6 +53,14 @@ TEST(Pool6Test, lastAddrInPrefix) { EXPECT_EQ("2001::3f", lastAddrInPrefix(addr2, 122).toText()); EXPECT_EQ("2001::7f", lastAddrInPrefix(addr2, 121).toText()); EXPECT_EQ("2001::ff", lastAddrInPrefix(addr2, 120).toText()); + + // Let's check extreme cases + IOAddress anyAddr("::"); + EXPECT_EQ("7fff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", + lastAddrInPrefix(anyAddr, 1).toText()); + EXPECT_EQ("ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff", + lastAddrInPrefix(anyAddr, 0).toText()); + EXPECT_EQ("::", lastAddrInPrefix(anyAddr, 128).toText()); } TEST(Pool6Test, firstAddrInPrefix) { diff --git a/src/lib/dhcp/tests/cfgmgr_unittest.cc b/src/lib/dhcp/tests/cfgmgr_unittest.cc index 6592803c3b..7f287eaf80 100644 --- a/src/lib/dhcp/tests/cfgmgr_unittest.cc +++ b/src/lib/dhcp/tests/cfgmgr_unittest.cc @@ -35,28 +35,28 @@ namespace { // constructor validation TEST(TripletTest, constructor) { - uint32_t min = 10; - uint32_t value = 20; - uint32_t max = 30; + const uint32_t min = 10; + const uint32_t value = 20; + const uint32_t max = 30; Triplet x(min, value, max); - EXPECT_EQ(10, x.getMin()); - EXPECT_EQ(20, x.get()); - EXPECT_EQ(30, x.getMax()); + EXPECT_EQ(min, x.getMin()); + EXPECT_EQ(value, x.get()); + EXPECT_EQ(max, x.getMax()); // requested values below min should return allowed min value - EXPECT_EQ(10, x.get(5)); + EXPECT_EQ(min, x.get(min - 5)); - EXPECT_EQ(10, x.get(10)); + EXPECT_EQ(min, x.get(min)); // requesting a value from within the range (min < x < max) should // return the requested value EXPECT_EQ(17, x.get(17)); - EXPECT_EQ(30, x.get(30)); + EXPECT_EQ(max, x.get(max)); - EXPECT_EQ(30, x.get(35)); + EXPECT_EQ(max, x.get(max + 5)); // this will be boring. It is expected to return 42 no matter what Triplet y(42); @@ -77,15 +77,26 @@ TEST(TripletTest, operator) { uint32_t x = 47; - // assignment operator: uint32_t => triplet - Triplet y = x; + Triplet foo(1,2,3); + Triplet bar(4,5,6); - EXPECT_EQ(47, y.get()); + foo = bar; + + EXPECT_EQ(4, foo.getMin()); + EXPECT_EQ(5, foo.get()); + EXPECT_EQ(6, foo.getMax()); + + // assignment operator: uint32_t => triplet + Triplet y(0); + y = x; + + EXPECT_EQ(x, y.get()); // let's try the other way around: triplet => uint32_t - uint32_t z = y; + uint32_t z = 0; + z = y; - EXPECT_EQ(47, z); + EXPECT_EQ(x, z); } // check if specified values are sane @@ -131,6 +142,14 @@ TEST(Pool6Test, constructor_prefix_len) { EXPECT_EQ("2001:db8:1::", pool1.getFirstAddress().toText()); EXPECT_EQ("2001:db8:1::ffff:ffff", pool1.getLastAddress().toText()); + // No such thing as /130 prefix + EXPECT_THROW(Pool6(Pool6::TYPE_IA, IOAddress("2001:db8::"), 130), + BadValue); + + // /0 prefix does not make sense + EXPECT_THROW(Pool6(Pool6::TYPE_IA, IOAddress("2001:db8::"), 0), + BadValue); + // This is Pool6, IPv4 addresses do not belong here EXPECT_THROW(Pool6(Pool6::TYPE_IA, IOAddress("192.168.0.2"), 96), BadValue); diff --git a/src/lib/dhcp/tests/run_unittests.cc b/src/lib/dhcp/tests/run_unittests.cc index d3dd12e867..0126645f62 100644 --- a/src/lib/dhcp/tests/run_unittests.cc +++ b/src/lib/dhcp/tests/run_unittests.cc @@ -13,10 +13,12 @@ // PERFORMANCE OF THIS SOFTWARE. #include +#include int main(int argc, char* argv[]) { ::testing::InitGoogleTest(&argc, argv); + isc::log::initLogger(); int result = RUN_ALL_TESTS();