[3210] client-id echo in DHCPv4 server is now configurable

src/bin/dhcp4/config_parser.cc

@@ -209,7 +209,6 @@ protected:
         return (parser);
     }
 
-
     /// @brief Determines if the given option space name and code describe
     /// a standard option for the DCHP4 server.
     ///
@@ -388,6 +387,8 @@ DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id) {
         parser = new DbAccessParser(config_id);
     } else if (config_id.compare("hooks-libraries") == 0) {
         parser = new HooksLibrariesParser(config_id);
+    } else if (config_id.compare("echo-client-id") == 0) {
+        parser = new BooleanParser(config_id, globalContext()->boolean_values_);
     } else {
         isc_throw(NotImplemented,
                 "Parser error: Global configuration parameter not supported: "
@@ -397,6 +398,18 @@ DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id) {
     return (parser);
 }
 
+void commitGlobalOptions() {
+
+    // Set whether v4 server is supposed to echo back client-id (yes = RFC6842
+    // compatible, no = backward compatibility)
+    try {
+        bool echo_client_id = globalContext()->boolean_values_->getParam("echo-client-id");
+        CfgMgr::instance().echoClientId(echo_client_id);
+    } catch (...) {
+        // Ignore errors. This flag is optional
+    }
+}
+
 isc::data::ConstElementPtr
 configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
     if (!config_set) {
@@ -529,6 +542,9 @@ configureDhcp4Server(Dhcpv4Srv&, isc::data::ConstElementPtr config_set) {
                 iface_parser->commit();
             }
 
+            // Apply global options
+            commitGlobalOptions();
+
             // This occurs last as if it succeeds, there is no easy way
             // revert it.  As a result, the failure to commit a subsequent
             // change causes problems when trying to roll back.
@@ -572,4 +588,3 @@ ParserContextPtr& globalContext() {
 
 }; // end of isc::dhcp namespace
 }; // end of isc namespace
-

src/bin/dhcp4/dhcp4.spec

@@ -54,6 +54,12 @@
         "item_default": ""
       },
 
+      { "item_name": "echo-client-id",
+        "item_type": "boolean",
+        "item_optional": true,
+        "item_default": true
+      },
+
       { "item_name": "option-def",
         "item_type": "list",
         "item_optional": false,

src/bin/dhcp4/dhcp4_srv.cc

@@ -564,8 +564,10 @@ Dhcpv4Srv::copyDefaultFields(const Pkt4Ptr& question, Pkt4Ptr& answer) {
     answer->setGiaddr(question->getGiaddr());
 
     // Let's copy client-id to response. See RFC6842.
+    // It is possible to disable RFC6842 to keep backward compatibility
+    bool echo = CfgMgr::instance().echoClientId();
     OptionPtr client_id = question->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
-    if (client_id) {
+    if (client_id && echo) {
         answer->addOption(client_id);
     }
 

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -556,6 +556,42 @@ TEST_F(Dhcp4ParserTest, nextServerOverride) {
     EXPECT_EQ("1.2.3.4", subnet->getSiaddr().toText());
 }
 
+// Check whether it is possible to configure echo-client-id
+TEST_F(Dhcp4ParserTest, echoClientId) {
+
+    ConstElementPtr status;
+
+    string config_false = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"echo-client-id\": false,"
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"subnet\": \"192.0.2.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    string config_true = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"echo-client-id\": true,"
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"subnet\": \"192.0.2.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    ElementPtr json_false = Element::fromJSON(config_false);
+    ElementPtr json_true = Element::fromJSON(config_true);
+
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json_false));
+    EXPECT_FALSE(CfgMgr::instance().echoClientId());
+
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json_true));
+    EXPECT_TRUE(CfgMgr::instance().echoClientId());
+
+    // In any case revert back to the default value (true)
+    CfgMgr::instance().echoClientId(true);
+}
+
 // This test checks if it is possible to override global values
 // on a per subnet basis.
 TEST_F(Dhcp4ParserTest, subnetLocal) {

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -687,6 +687,32 @@ TEST_F(Dhcpv4SrvTest, ManyDiscovers) {
     cout << "Offered address to client3=" << addr3.toText() << endl;
 }
 
+// Checks whether echoing back client-id is controllable
+TEST_F(Dhcpv4SrvTest, discoverEchoClientId) {
+    NakedDhcpv4Srv srv(0);
+
+    Pkt4Ptr dis = Pkt4Ptr(new Pkt4(DHCPDISCOVER, 1234));
+    dis->setRemoteAddr(IOAddress("192.0.2.1"));
+    OptionPtr clientid = generateClientId();
+    dis->addOption(clientid);
+
+    // Pass it to the server and get an offer
+    Pkt4Ptr offer = srv.processDiscover(dis);
+
+    // 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);
+
+    // 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
 // ACK is generated, that the response has an address and that address
 // really belongs to the configured pool.
@@ -829,6 +855,32 @@ TEST_F(Dhcpv4SrvTest, ManyRequests) {
     cout << "Offered address to client3=" << addr3.toText() << endl;
 }
 
+// Checks whether echoing back client-id is controllable
+TEST_F(Dhcpv4SrvTest, requestEchoClientId) {
+    NakedDhcpv4Srv srv(0);
+
+    Pkt4Ptr dis = Pkt4Ptr(new Pkt4(DHCPREQUEST, 1234));
+    dis->setRemoteAddr(IOAddress("192.0.2.1"));
+    OptionPtr clientid = generateClientId();
+    dis->addOption(clientid);
+
+    // Pass it to the server and get ACK
+    Pkt4Ptr ack = srv.processRequest(dis);
+
+    // Check if we get response at all
+    checkResponse(ack, DHCPACK, 1234);
+    checkClientId(ack, clientid);
+    EXPECT_TRUE(ack->getOption(DHO_DHCP_CLIENT_IDENTIFIER));
+
+    CfgMgr::instance().echoClientId(false);
+    ack = srv.processDiscover(dis);
+
+    // Check if we get response at all
+    checkResponse(ack, DHCPOFFER, 1234);
+    checkClientId(ack, clientid);
+    EXPECT_FALSE(ack->getOption(DHO_DHCP_CLIENT_IDENTIFIER));
+}
+
 
 // This test verifies that incoming (positive) REQUEST/Renewing can be handled properly, that a
 // REPLY is generated, that the response has an address and that address

src/bin/dhcp4/tests/dhcp4_test_utils.cc

@@ -66,6 +66,12 @@ Dhcpv4SrvTest::Dhcpv4SrvTest()
     valid_iface_ = ifaces.begin()->getName();
 }
 
+Dhcpv4SrvTest::~Dhcpv4SrvTest() {
+
+    // Make sure that we revert to default value
+    CfgMgr::instance().echoClientId(true);
+}
+
 void Dhcpv4SrvTest::addPrlOption(Pkt4Ptr& pkt) {
 
     OptionUint8ArrayPtr option_prl =
@@ -331,12 +337,21 @@ void Dhcpv4SrvTest::checkServerId(const Pkt4Ptr& rsp, const OptionPtr& expected_
 }
 
 void Dhcpv4SrvTest::checkClientId(const Pkt4Ptr& rsp, const OptionPtr& expected_clientid) {
+
+    bool include_clientid = CfgMgr::instance().echoClientId();
+
     // check that server included our own client-id
     OptionPtr opt = rsp->getOption(DHO_DHCP_CLIENT_IDENTIFIER);
-    ASSERT_TRUE(opt);
-    EXPECT_EQ(expected_clientid->getType(), opt->getType());
-    EXPECT_EQ(expected_clientid->len(), opt->len());
-    EXPECT_TRUE(expected_clientid->getData() == opt->getData());
+    if (include_clientid) {
+        // Normal mode: echo back (see RFC6842)
+        ASSERT_TRUE(opt);
+        EXPECT_EQ(expected_clientid->getType(), opt->getType());
+        EXPECT_EQ(expected_clientid->len(), opt->len());
+        EXPECT_TRUE(expected_clientid->getData() == opt->getData());
+    } else {
+        // Backward compatibility mode for pre-RFC6842 devices
+        ASSERT_FALSE(opt);
+    }
 }
 
 ::testing::AssertionResult

src/bin/dhcp4/tests/dhcp4_test_utils.h

@@ -79,8 +79,7 @@ public:
     Dhcpv4SrvTest();
 
     /// @brief destructor
-    virtual ~Dhcpv4SrvTest() {
-    }
+    virtual ~Dhcpv4SrvTest();
 
     /// @brief Add 'Parameter Request List' option to the packet.
     ///