From: Marcin Siodelski Date: Thu, 26 Nov 2015 13:00:06 +0000 (+0100) Subject: [3874] Addressed review comments. X-Git-Tag: trac4231_base~39^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=aab4794b773b06091a0530d1350f7ed2512dc32a;p=thirdparty%2Fkea.git [3874] Addressed review comments. --- diff --git a/doc/examples/kea6/duid.json b/doc/examples/kea6/duid.json index d47d52bf11..4bc6440482 100644 --- a/doc/examples/kea6/duid.json +++ b/doc/examples/kea6/duid.json @@ -70,8 +70,8 @@ "output": "/var/log/kea-debug.log" } ], - "debuglevel": 99, - "severity": "DEBUG" + "debuglevel": 0, + "severity": "INFO" } ] } diff --git a/doc/guide/dhcp6-srv.xml b/doc/guide/dhcp6-srv.xml index e613e518c0..4814c8eb31 100644 --- a/doc/guide/dhcp6-srv.xml +++ b/doc/guide/dhcp6-srv.xml @@ -2421,7 +2421,7 @@ should include options from the isc option space: "server-id": { "type": "LLT", "htype": 8, - "identifier": "A65DC7410F0568", + "identifier": "A65DC7410F05", "time": 2518920166 }, ... @@ -2443,9 +2443,9 @@ should include options from the isc option space: The hexadecimal representation of the DUID generated as a result of the configuration specified above will be: -00:01:00:08:96:23:AB:E6:A6:5D:C7:41:0F:05:68 --------------------------------------------- -|type|htype| time | identifier | +00:01:00:08:96:23:AB:E6:A6:5D:C7:41:0F:05 +------------------------------------------ +|type|htype| time | identifier | @@ -2536,7 +2536,7 @@ should include options from the isc option space: "server-id": { "type": "LL", "htype": 8, - "identifier": "A65DC7410F0568", + "identifier": "A65DC7410F05", }, ... } @@ -2548,9 +2548,9 @@ should include options from the isc option space: which will result in the following server identifier: -00:03:00:08:A6:5D:C7:41:0F:05:68 ---------------------------------- -|type|htype| identifier | +00:03:00:08:A6:5D:C7:41:0F:05 +------------------------------ +|type|htype| identifier | diff --git a/src/bin/dhcp6/dhcp6_messages.mes b/src/bin/dhcp6/dhcp6_messages.mes index 756c590476..8ee10e500d 100644 --- a/src/bin/dhcp6/dhcp6_messages.mes +++ b/src/bin/dhcp6/dhcp6_messages.mes @@ -635,20 +635,6 @@ etc. The exact reason for rejecting the packet is included in the message. % DHCP6_RESPONSE_DATA responding with packet type %1 data is %2 A debug message listing the data returned to the client. -% DHCP6_SERVERID_GENERATED server-id %1 has been generated and will be stored in %2 -This informational messages indicates that the server was not able to read -its server identifier (DUID) and has generated a new one. This server-id -will be stored in a file and will be read and used during next restart. It -is normal behavior when the server is started for the first time. If -this message is printed every start, please check that the server have -sufficient permission to write its server-id file and that the file is not -corrupt. - -Changing the server identifier in a production environment is not -recommended as existing clients will not recognize the server and may go -through a rebind phase. However, they should be able to recover without -losing their leases. - % DHCP6_USING_SERVERID server is using server-id %1 and stores in the the file %2 This info message is logged when the server reads its server-id from a file or generates it. This message is a notification to the administrator diff --git a/src/lib/dhcp/duid_factory.cc b/src/lib/dhcp/duid_factory.cc index 4345f29672..21f6267e35 100644 --- a/src/lib/dhcp/duid_factory.cc +++ b/src/lib/dhcp/duid_factory.cc @@ -52,7 +52,7 @@ DUIDFactory::DUIDFactory(const std::string& storage_location) } bool -DUIDFactory::isPersisted() const { +DUIDFactory::isStored() const { return (!storage_location_.empty()); } @@ -62,7 +62,7 @@ DUIDFactory::createLLT(const uint16_t htype, const uint32_t time_in, // We'll need DUID stored in the file to compare it against the // new configuration. If the new configuration indicates that some // bits of the DUID should be generated we'll first try to use the - // values stored in the file to prvent DUID from changing if possible. + // values stored in the file to prevent DUID from changing if possible. readFromFile(); uint16_t htype_current = 0; @@ -105,8 +105,8 @@ DUIDFactory::createLLT(const uint16_t htype, const uint32_t time_in, } else if (htype_out == 0) { // If link layer type unspecified and link layer adddress - // is specified, use HTYPE_ETHER. - htype_out = HTYPE_ETHER; + // is specified, use current type or HTYPE_ETHER. + htype_out = (htype_current != 0) ? htype_current : HTYPE_ETHER; } @@ -229,8 +229,8 @@ DUIDFactory::createLL(const uint16_t htype, } else if (htype_out == 0) { // If link layer type unspecified and link layer adddress - // is specified, use HTYPE_ETHER. - htype_out = HTYPE_ETHER; + // is specified, use current type or HTYPE_ETHER. + htype_out = (htype_current != 0) ? htype_current : HTYPE_ETHER; } @@ -263,9 +263,10 @@ DUIDFactory::createLinkLayerId(std::vector& identifier, // MAC address should be at least 6 bytes. Although there is no such // requirement in any RFC, all decent physical interfaces (Ethernet, - // WiFi, InfiniBand, etc.) have 6 bytes long MAC address. We want to - // base our DUID on real hardware address, rather than virtual - // interface that pretends that underlying IP address is its MAC. + // WiFi, InfiniBand, etc.) have at least 6 bytes long MAC address. + // We want to/ base our DUID on real hardware address, rather than + // virtual interface that pretends that underlying IP address is its + // MAC. if (iface->getMacLen() < MIN_MAC_LEN) { continue; } @@ -308,8 +309,8 @@ DUIDFactory::set(const std::vector& duid_vector) { << DUID::MIN_DUID_LEN << " bytes"); } - // Persist DUID in a file if file location specified. - if (isPersisted()) { + // Store DUID in a file if file location specified. + if (isStored()) { std::ofstream ofs; try { ofs.open(storage_location_.c_str(), std::ofstream::out | @@ -353,10 +354,11 @@ DUIDFactory::get() { } // DUID doesn't exist, so we need to create it. + const std::vector empty_vector; try { // There is no file with a DUID or the DUID stored in the file is // invalid. We need to generate a new DUID. - createLLT(0, 0, std::vector()); + createLLT(0, 0, empty_vector); } catch (...) { // It is possible that the creation of the DUID-LLT failed if there @@ -367,7 +369,7 @@ DUIDFactory::get() { // Fall back to creation of DUID enterprise. If that fails we allow // for propagating exception to indicate a fatal error. This may // be the case if we failed to write it to a file. - createEN(0, std::vector()); + createEN(0, empty_vector); } return (duid_); @@ -378,7 +380,7 @@ DUIDFactory::readFromFile() { duid_.reset(); std::ostringstream duid_str; - if (isPersisted()) { + if (isStored()) { std::ifstream ifs; ifs.open(storage_location_.c_str(), std::ifstream::in); if (ifs.good()) { diff --git a/src/lib/dhcp/duid_factory.h b/src/lib/dhcp/duid_factory.h index 5718c30d0c..de2cdc81c9 100644 --- a/src/lib/dhcp/duid_factory.h +++ b/src/lib/dhcp/duid_factory.h @@ -64,7 +64,7 @@ namespace dhcp { /// /// This class is also responsible for storing the generated DUID in a /// file. The location of this file is specified in the class constructor. -/// If this location is not specified the DUID is not persisted, i.e. is +/// If this location is not specified the DUID is not stored, i.e. is /// lost when the server or client shuts down. However, the DUID may be /// reconstructed according to the configuration of the client or server /// when they are back online. @@ -77,22 +77,22 @@ public: /// stored. DUIDFactory(const std::string& storage_location = ""); - /// @brief Checks if generated DUID will be persisted in the file. + /// @brief Checks if generated DUID will be stored in the file. /// - /// @return true if generated DUIDs are persisted in a file, false + /// @return true if generated DUIDs are stored in a file, false /// otherwise. - bool isPersisted() const; + bool isStored() const; /// @brief Generates DUID-LLT. /// - /// This method generates DUID-LLT. + /// This method generates DUID-LLT (Link Layer plus Time). /// /// @param htype Hardware type. If this is set to 0 and link layer /// address is empty a value from existing DUID or a default value /// of @c HTYPE_ETHER is used. Otherwise a link layer type of selected /// interface is used. /// @param time_in Explicit value of time for the DUID. If this is - /// set to 0 a value from existing DUOD or current time is used, + /// set to 0 a value from existing DUID or current time is used, /// otherwise a value specified is used. /// @param ll_identifier Data to be used as link layer address. If /// this is an empty vector this method will try to use link layer @@ -107,6 +107,8 @@ public: /// @brief Generates DUID-EN. /// + /// This method generates DUID-EN (DUID Enterprise). + /// /// @param enterprise_id Enterprise id. If this value is 0, a value /// from existing DUID is used or ISC's enterprise id if there is /// no DUID yet. @@ -119,6 +121,8 @@ public: /// @brief Generates DUID-LL. /// + /// This method generates DUID-LL (Link Layer). + /// /// @param htype Hardware type. If this is set to 0 and link layer /// address is empty a value from existing DUID or a default value /// of @c HTYPE_ETHER is used. Otherwise a link layer type of selected @@ -144,7 +148,7 @@ public: /// generation of the DUID-LLT may fail, e.g. when there are no interfaces /// with a suitable link layer address. In this case, this method will /// generate DUID-EN, with the ISC enterprise id. If this fails, e.g. as a - /// result of error while persisting the generated DUID-EN, exception + /// result of error while storing the generated DUID-EN, exception /// is thrown. /// /// @return Instance of the DUID read from file, or generated. @@ -166,7 +170,7 @@ private: /// @brief Sets a new DUID as current. /// - /// The generated DUID is persisted in the file, if such file is specified. + /// The generated DUID is stored in the file, if such file is specified. /// The new DUID will be returned when @c DUIDFactory::get is called. /// /// @param duid_vector New DUID represented as vector of bytes. diff --git a/src/lib/dhcp/tests/duid_factory_unittest.cc b/src/lib/dhcp/tests/duid_factory_unittest.cc index ede8bca506..c7d5643ca9 100644 --- a/src/lib/dhcp/tests/duid_factory_unittest.cc +++ b/src/lib/dhcp/tests/duid_factory_unittest.cc @@ -54,7 +54,7 @@ public: /// @brief Returns absolute path to a test DUID storage. /// /// @param duid_file_name Name of the file holding test DUID. - std::string absolutePath(const std::string& duid_file_path) const; + std::string absolutePath(const std::string& duid_file_name) const; /// @brief Removes default DUID file used in the tests. /// @@ -115,10 +115,22 @@ public: /// /// @param expected_enterprise_id Expected enterprise id as string. /// @param expected_identifier Expected variable length identifier - /// as string. + /// as string. If empty string specified the test method only checks + /// that generated identifier consists of some random values. void testEN(const std::string& expected_enterprise_id, const std::string& expected_identifier = ""); + /// @brief Tests creation of a DUID-EN. + /// + /// @param expected_enterprise_id Expected enterprise id as string. + /// @param expected_identifier Expected variable length identifier + /// as string. If empty string specified the test method only checks + /// that generated identifier consists of some random values. + /// @param factory_ref Reference to DUID factory. + void testEN(const std::string& expected_enterprise_id, + const std::string& expected_identifier, + DUIDFactory& factory_ref); + /// @brief Tests creation of a DUID-LL. /// /// @param expected_htype Expected link layer type as string. @@ -126,6 +138,15 @@ public: void testLL(const std::string& expected_htype, const std::string& expected_hwaddr); + /// @brief Tests creation of a DUID-LL. + /// + /// @param expected_htype Expected link layer type as string. + /// @param expected_hwaddr Expected link layer type as string. + /// @param factory_ref Reference to DUID factory. + void testLL(const std::string& expected_htype, + const std::string& expected_hwaddr, + DUIDFactory& factory_ref); + /// @brief Returns reference to a default factory. DUIDFactory& factory() { return (factory_); @@ -153,9 +174,9 @@ DUIDFactoryTest::~DUIDFactoryTest() { } std::string -DUIDFactoryTest::absolutePath(const std::string& duid_file_path) const { +DUIDFactoryTest::absolutePath(const std::string& duid_file_name) const { std::ostringstream s; - s << TEST_DATA_BUILDDIR << "/" << duid_file_path; + s << TEST_DATA_BUILDDIR << "/" << duid_file_name; return (s.str()); } @@ -255,12 +276,17 @@ DUIDFactoryTest::testLLT(const std::string& expected_htype, EXPECT_EQ(duid->toText(), readDefaultFile()); } - - void DUIDFactoryTest::testEN(const std::string& expected_enterprise_id, const std::string& expected_identifier) { - DuidPtr duid = factory().get(); + testEN(expected_enterprise_id, expected_identifier, factory()); +} + +void +DUIDFactoryTest::testEN(const std::string& expected_enterprise_id, + const std::string& expected_identifier, + DUIDFactory& factory_ref) { + DuidPtr duid = factory_ref.get(); ASSERT_TRUE(duid); ASSERT_GE(duid->getDuid().size(), 8); std::string duid_text = toString(duid->getDuid()); @@ -270,7 +296,7 @@ DUIDFactoryTest::testEN(const std::string& expected_enterprise_id, // Verify enterprise ID. EXPECT_EQ(expected_enterprise_id, duid_text.substr(4, 8)); - // If no expecyed identifier, we should at least check that the + // If no expected identifier, we should at least check that the // generated identifier contains some random non-zero digits. if (expected_identifier.empty()) { EXPECT_FALSE(isRangeZero(duid->getDuid().begin(), @@ -287,14 +313,21 @@ DUIDFactoryTest::testEN(const std::string& expected_enterprise_id, void DUIDFactoryTest::testLL(const std::string& expected_htype, const std::string& expected_hwaddr) { - DuidPtr duid = factory().get(); + testLL(expected_htype, expected_hwaddr, factory()); +} + +void +DUIDFactoryTest::testLL(const std::string& expected_htype, + const std::string& expected_hwaddr, + DUIDFactory& factory_ref) { + DuidPtr duid = factory_ref.get(); ASSERT_TRUE(duid); ASSERT_GE(duid->getDuid().size(), 8); std::string duid_text = toString(duid->getDuid()); // DUID type LL EXPECT_EQ("0003", duid_text.substr(0, 4)); - // Link layer type HTYPE_ETHER + // Link layer type. EXPECT_EQ(expected_htype, duid_text.substr(4, 4)); // MAC address of the interface. @@ -314,7 +347,7 @@ TEST_F(DUIDFactoryTest, createLLT) { // use current time, HTYPE_ETHER and MAC address of one of the // interfaces. ASSERT_NO_THROW(factory().createLLT(0, 0, std::vector())); - testLLT("0001", timeAsHexString(), false, "010101010101"); + testLLT("0001", timeAsHexString(), false, "080808080808"); } // This test verifies that the factory class creates a DUID-LLT from @@ -322,7 +355,7 @@ TEST_F(DUIDFactoryTest, createLLT) { // generated. TEST_F(DUIDFactoryTest, createLLTExplicitTime) { ASSERT_NO_THROW(factory().createLLT(0, 0xABCDEF, std::vector())); - testLLT("0001", "00ABCDEF", true, "010101010101"); + testLLT("0001", "00ABCDEF", true, "080808080808"); } // This test verifies that the factory class creates DUID-LLT with @@ -330,7 +363,7 @@ TEST_F(DUIDFactoryTest, createLLTExplicitTime) { // is used to generate the DUID. TEST_F(DUIDFactoryTest, createLLTExplicitHtype) { ASSERT_NO_THROW(factory().createLLT(HTYPE_FDDI, 0, std::vector())); - testLLT("0001", timeAsHexString(), false, "010101010101"); + testLLT("0001", timeAsHexString(), false, "080808080808"); } // This test verifies that the factory class creates DUID-LLT from @@ -361,7 +394,27 @@ TEST_F(DUIDFactoryTest, createLLTReuse) { // link layer address. The factory function should use the // values in the existing DUID. ASSERT_NO_THROW(factory2.createLLT(0, 0, std::vector())); - testLLT("0008", "FAFAFAFA", true, "242424242424"); + testLLT("0008", "FAFAFAFA", true, "242424242424", factory2); + + // Try to reuse only a time value. + DUIDFactory factory3(absolutePath(DEFAULT_DUID_FILE)); + ASSERT_NO_THROW(factory3.createLLT(HTYPE_ETHER, 0, + toVector("121212121212"))); + testLLT("0001", "FAFAFAFA", true, "121212121212", factory3); + + // Reuse only a hardware type. + DUIDFactory factory4(absolutePath(DEFAULT_DUID_FILE)); + ASSERT_NO_THROW(factory4.createLLT(0, 0x23432343, + toVector("455445544554"))); + testLLT("0001", "23432343", true, "455445544554", factory4); + + // Reuse link layer address. Note that in this case the hardware + // type is set to the type of the interface from which hardware + // address is obtained and the explicit value is ignored. + DUIDFactory factory5(absolutePath(DEFAULT_DUID_FILE)); + ASSERT_NO_THROW(factory5.createLLT(HTYPE_FDDI, 0x11111111, + std::vector())); + testLLT("0001", "11111111", true, "455445544554", factory5); } // This test verifies that the DUID-EN can be generated entirely. Such @@ -399,21 +452,31 @@ TEST_F(DUIDFactoryTest, createENReuse) { // Create another factory class, which uses the same file. DUIDFactory factory2(absolutePath(DEFAULT_DUID_FILE)); ASSERT_NO_THROW(factory2.createEN(0, std::vector())); - testEN("FAFAFAFA", "242424242424"); + testEN("FAFAFAFA", "242424242424", factory2); + + // Reuse only enterprise id. + DUIDFactory factory3(absolutePath(DEFAULT_DUID_FILE)); + ASSERT_NO_THROW(factory3.createEN(0, toVector("121212121212"))); + testEN("FAFAFAFA", "121212121212", factory3); + + // Reuse only variable length identifier. + DUIDFactory factory4(absolutePath(DEFAULT_DUID_FILE)); + ASSERT_NO_THROW(factory4.createEN(0x1234, std::vector())); + testEN("00001234", "121212121212", factory4); } // This test verifies that the DUID-LL is generated when neither link layer // type nor address is specified. TEST_F(DUIDFactoryTest, createLL) { ASSERT_NO_THROW(factory().createLL(0, std::vector())); - testLL("0001", "010101010101"); + testLL("0001", "080808080808"); } // This test verifies that the DUID-LL is generated and the link layer type // used is taken from the interface used to generate link layer address. TEST_F(DUIDFactoryTest, createLLExplicitHtype) { ASSERT_NO_THROW(factory().createLL(HTYPE_FDDI, std::vector())); - testLL("0001", "010101010101"); + testLL("0001", "080808080808"); } // This test verifies that DUID-LL is created from explicitly provided @@ -458,14 +521,26 @@ TEST_F(DUIDFactoryTest, createLLReuse) { // link layer address. The factory function should use the // values in the existing DUID. ASSERT_NO_THROW(factory2.createLL(0, std::vector())); - testLL("0008", "242424242424"); + testLL("0008", "242424242424", factory2); + + // Reuse only hardware type + DUIDFactory factory3(absolutePath(DEFAULT_DUID_FILE)); + ASSERT_NO_THROW(factory3.createLL(0, toVector("121212121212"))); + testLL("0008", "121212121212", factory3); + + // Reuse link layer address. Note that when the link layer address is + // reused, the explicit value of hardware type is reused too and the + // explicit value of hardware type is ignored. + DUIDFactory factory4(absolutePath(DEFAULT_DUID_FILE)); + ASSERT_NO_THROW(factory4.createLL(HTYPE_ETHER, std::vector())); + testLL("0008", "121212121212", factory4); } // This test verifies that it is possible to override a DUID. TEST_F(DUIDFactoryTest, override) { // Create default DUID-LLT. ASSERT_NO_THROW(static_cast(factory().get())); - testLLT("0001", timeAsHexString(), false, "010101010101"); + testLLT("0001", timeAsHexString(), false, "080808080808"); ASSERT_NO_THROW(factory().createEN(0, toVector("12131415"))); testEN("000009BF", "12131415"); diff --git a/src/lib/dhcp/tests/iface_mgr_test_config.cc b/src/lib/dhcp/tests/iface_mgr_test_config.cc index 333e11c1ca..2bef7e65bf 100644 --- a/src/lib/dhcp/tests/iface_mgr_test_config.cc +++ b/src/lib/dhcp/tests/iface_mgr_test_config.cc @@ -96,8 +96,8 @@ IfaceMgrTestConfig::createIface(const std::string &name, const int ifindex) { iface->flag_up_ = true; iface->flag_running_ = true; - // Set MAC address to 01:01:01:01:01:01. - std::vector mac_vec(6, 1); + // Set MAC address to 08:08:08:08:08:08. + std::vector mac_vec(6, 8); iface->setMac(&mac_vec[0], mac_vec.size()); iface->setHWType(HTYPE_ETHER); diff --git a/src/lib/dhcpsrv/parsers/duid_config_parser.h b/src/lib/dhcpsrv/parsers/duid_config_parser.h index 05639c50d5..99c8c8fc31 100644 --- a/src/lib/dhcpsrv/parsers/duid_config_parser.h +++ b/src/lib/dhcpsrv/parsers/duid_config_parser.h @@ -23,7 +23,14 @@ namespace isc { namespace dhcp { -/// @brief Parser for a single host reservation entry. +/// @brief Parser for server DUID configuration. +/// +/// This parser currently supports the following DUID types: +/// - DUID-LLT, +/// - DUID-EN +/// - DUID-LL +/// +/// @todo Add support for DUID-UUID in the parser. class DUIDConfigParser : public DhcpConfigParser { public: