[3874] Addressed review comments.

doc/examples/kea6/duid.json

@@ -70,8 +70,8 @@
                     "output": "/var/log/kea-debug.log"
                 }
             ], 
-            "debuglevel": 99,
+            "debuglevel": 0,
-            "severity": "DEBUG"
+            "severity": "INFO"
         }
     ]
 }

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:
       <para>The hexadecimal representation of the DUID generated as a result
       of the configuration specified above will be:
 <screen>
-00:01:00:08:96:23:AB:E6:A6:5D:C7:41:0F:05:68
+00:01:00:08:96:23:AB:E6:A6:5D:C7:41:0F:05
---------------------------------------------
+------------------------------------------
-|type|htype|   time    |    identifier     |
+|type|htype|   time    |   identifier    |
 </screen>
       </para>
 
@@ -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:
 
 <screen>
-00:03:00:08:A6:5D:C7:41:0F:05:68
+00:03:00:08:A6:5D:C7:41:0F:05
----------------------------------
+------------------------------
-|type|htype|     identifier     |
+|type|htype|   identifier   |
 </screen>
       </para>
 

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

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.
+        // is specified, use current type or HTYPE_ETHER.
-        htype_out = 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.
+        // is specified, use current type or HTYPE_ETHER.
-        htype_out = HTYPE_ETHER;
+        htype_out = (htype_current != 0) ? htype_current : HTYPE_ETHER;
 
     }
 
@@ -263,9 +263,10 @@ DUIDFactory::createLinkLayerId(std::vector<uint8_t>& 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
+        // WiFi, InfiniBand, etc.) have at least 6 bytes long MAC address.
-        // base our DUID on real hardware address, rather than virtual
+        // We want to/ base our DUID on real hardware address, rather than
-        // interface that pretends that underlying IP address is its MAC.
+        // 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<uint8_t>& duid_vector) {
                   << DUID::MIN_DUID_LEN << " bytes");
     }
 
-    // Persist DUID in a file if file location specified.
+    // Store DUID in a file if file location specified.
-    if (isPersisted()) {
+    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<uint8_t> 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<uint8_t>());
+        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<uint8_t>());
+        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()) {

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.

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<uint8_t>()));
-    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<uint8_t>()));
-    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<uint8_t>()));
-    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<uint8_t>()));
-    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<uint8_t>()));
+    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<uint8_t>()));
-    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<uint8_t>()));
+    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<uint8_t>()));
-    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<uint8_t>()));
-    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<uint8_t>()));
-    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<uint8_t>()));
+    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<void>(factory().get()));
-    testLLT("0001", timeAsHexString(), false, "010101010101");
+    testLLT("0001", timeAsHexString(), false, "080808080808");
 
     ASSERT_NO_THROW(factory().createEN(0, toVector("12131415")));
     testEN("000009BF", "12131415");

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.
+    // Set MAC address to 08:08:08:08:08:08.
-    std::vector<uint8_t> mac_vec(6, 1);
+    std::vector<uint8_t> mac_vec(6, 8);
     iface->setMac(&mac_vec[0], mac_vec.size());
     iface->setHWType(HTYPE_ETHER);
 

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: