[3210] Changes after review:

 - tests/methods descriptions improved
 - CfgMgrTest.echoClientId improved
 - Redundant checks removed from Dhcpv4SrvTest.discoverEchoClientId
 - BIND10 Guide clarified
 - ChangeLog added

ChangeLog

+XXX.	[func]		tomek
+	b10-dhcp4: Support for sending back client-id (RFC6842) has been
+	added now. Also a flag (echo-client-id) has been added, so it is
+	possible to enable backward compatibility (echo-client-id false).
+	(Trac #3210, git abcd)
+
 698.	[bug]		muks
 	A bug was fixed in the interaction between b10-init and b10-msgq
 	that caused BIND 10 failures after repeated start/stop of

doc/guide/bind10-guide.xml

@@ -4416,19 +4416,22 @@ Dhcp4/subnet4	[]	list	(default)
 
     <section id="dhcp4-echo-client-id">
       <title>Echoing client-id (RFC6842)</title>
-      <para>Original DHCPv4 spec (RFC2131) states that the DHCPv4 server must not
-      send back client-id options when responding to clients. However, in some cases
-      that confused clients that did not have MAC address or client-id. See RFC6842
-      for details. That behavior has changed with the publication of RFC6842 which
-      updated RFC2131. That update now states that the server must send client-id.
-      That is the default behaviour that Kea offers. However, in some cases older
-      devices that do not support RFC6842 may refuse to accept responses that include
-      client-id options. To enable backward compatibility, an optional flag has been
-      introduced. To configure it, use the following commands:</para>
+      <para>Original DHCPv4 spec (RFC2131) states that the DHCPv4
+      server must not send back client-id options when responding to
+      clients. However, in some cases that confused clients that did
+      not have MAC address or client-id. See RFC6842 for details. That
+      behavior has changed with the publication of RFC6842 which
+      updated RFC2131. That update now states that the server must
+      send client-id if client sent it. That is the default behaviour
+      that Kea offers. However, in some cases older devices that do
+      not support RFC6842 may refuse to accept responses that include
+      client-id option. To enable backward compatibility, an optional
+      flag has been introduced. To configure it, use the following
+      commands:</para>
 
 <screen>
 &gt; <userinput>config add Dhcp4/echo-client-id</userinput>
-&gt; <userinput>config set Dhcp4/echo-client-id True"</userinput>
+&gt; <userinput>config set Dhcp4/echo-client-id False</userinput>
 &gt; <userinput>config commit</userinput>
 </screen>
 

src/bin/dhcp4/config_parser.cc

@@ -399,6 +399,8 @@ DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id) {
 }
 
 void commitGlobalOptions() {
+    // Although the function is modest for now, it is certain that the number
+    // of global switches will increase over time, hence the name.
 
     // Set whether v4 server is supposed to echo back client-id (yes = RFC6842
     // compatible, no = backward compatibility)

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -582,9 +582,14 @@ TEST_F(Dhcp4ParserTest, echoClientId) {
     ElementPtr json_false = Element::fromJSON(config_false);
     ElementPtr json_true = Element::fromJSON(config_true);
 
+    // Let's check the default. It should be true
+    EXPECT_TRUE(CfgMgr::instance().echoClientId());
+
+    // Now check that "false" configuration is really applied.
     EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json_false));
     EXPECT_FALSE(CfgMgr::instance().echoClientId());
 
+    // Now check that "true" configuration is really applied.
     EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json_true));
     EXPECT_TRUE(CfgMgr::instance().echoClientId());
 

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -687,7 +687,9 @@ TEST_F(Dhcpv4SrvTest, ManyDiscovers) {
     cout << "Offered address to client3=" << addr3.toText() << endl;
 }
 
-// Checks whether echoing back client-id is controllable
+// Checks whether echoing back client-id is controllable, i.e.
+// whether the server obeys echo-client-id and sends (or not)
+// client-id
 TEST_F(Dhcpv4SrvTest, discoverEchoClientId) {
     NakedDhcpv4Srv srv(0);
 
@@ -702,7 +704,6 @@ TEST_F(Dhcpv4SrvTest, discoverEchoClientId) {
     // Check if we get response at all
     checkResponse(offer, DHCPOFFER, 1234);
     checkClientId(offer, clientid);
-    EXPECT_TRUE(offer->getOption(DHO_DHCP_CLIENT_IDENTIFIER));
 
     CfgMgr::instance().echoClientId(false);
     offer = srv.processDiscover(dis);
@@ -710,7 +711,6 @@ TEST_F(Dhcpv4SrvTest, discoverEchoClientId) {
     // Check if we get response at all
     checkResponse(offer, DHCPOFFER, 1234);
     checkClientId(offer, clientid);
-    EXPECT_FALSE(offer->getOption(DHO_DHCP_CLIENT_IDENTIFIER));
 }
 
 // This test verifies that incoming REQUEST can be handled properly, that an

src/bin/dhcp4/tests/dhcp4_test_utils.h

@@ -186,6 +186,11 @@ public:
     void checkServerId(const Pkt4Ptr& rsp, const OptionPtr& expected_srvid);
 
     /// @brief Checks if server response (OFFER, ACK, NAK) includes proper client-id
+    ///
+    /// This method follows values reported by CfgMgr in echoClientId() method.
+    /// Depending on its configuration, the client-id is either mandatory or
+    /// forbidden to appear in the response.
+    ///
     /// @param rsp response packet to be validated
     /// @param expected_clientid expected value of client-id
     void checkClientId(const Pkt4Ptr& rsp, const OptionPtr& expected_clientid);

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc

@@ -658,11 +658,16 @@ TEST_F(CfgMgrTest, activateAllIfaces) {
 TEST_F(CfgMgrTest, echoClientId) {
     CfgMgr& cfg_mgr = CfgMgr::instance();
 
+    // Check that the default is true
     EXPECT_TRUE(cfg_mgr.echoClientId());
 
+    // Check that it can be modified to false
     cfg_mgr.echoClientId(false);
-
     EXPECT_FALSE(cfg_mgr.echoClientId());
+
+    // Check that the default value can be restored
+    cfg_mgr.echoClientId(true);
+    EXPECT_TRUE(cfg_mgr.echoClientId());
 }
 
 /// @todo Add unit-tests for testing: