[master] Merge branch 'trac3191' (next-server field in DHCPv4)

Conflicts:
	ChangeLog

ChangeLog

@@ -1,3 +1,9 @@
+696.	[func]		tomek
+	b10-dhcp4: It is now possible to specify value of siaddr field
+	in DHCPv4 responses. It is used to point out to the next
+	server in the boot process (that typically is TFTP server).
+	(Trac #3191, git 541922b5300904a5de2eaeddc3666fc4b654ffba)
+
 695.	[func]		tomek
 	b10-dhcp6 is now able to listen on global IPv6 unicast addresses.
 	(Trac #3195, git 72e601f2a57ab70b25d50877c8e49242739d1c9f)

doc/guide/bind10-guide.xml

@@ -4390,6 +4390,30 @@ Dhcp4/subnet4	[]	list	(default)
       </para>
     </section>
 
+    <section id="dhcp4-next-server">
+      <title>Next server (siaddr)</title>
+      <para>In some cases, clients want to obtain configuration from the TFTP server.
+      Although there is a dedicated option for it, some devices may use siaddr field
+      in the DHCPv4 packet for that purpose. That specific field can be configured
+      using next-server directive. It is possible to define it in global scope or
+      for a given subnet only. If both are defined, subnet value takes precedence.
+      The value in subnet can be set to 0.0.0.0, which means that next-server should
+      not be sent. It may also be set to empty string, which means the same as if
+      it was not defined at all - use global value.
+      </para>
+
+<screen>
+&gt; <userinput>config add Dhcp4/next-server</userinput>
+&gt; <userinput>config set Dhcp4/next-server "192.0.2.123"</userinput>
+&gt; <userinput>config commit</userinput>
+<userinput></userinput>
+&gt; <userinput>config add Dhcp4/subnet[0]/next-server</userinput>
+&gt; <userinput>config set Dhcp4/subnet[0]/next-server "192.0.2.234"</userinput>
+&gt; <userinput>config commit</userinput>
+</screen>
+
+    </section>
+
     <section id="dhcp4-std">
       <title>Supported Standards</title>
       <para>The following standards and draft standards are currently

src/bin/dhcp4/config_parser.cc

@@ -192,7 +192,8 @@ protected:
             (config_id.compare("rebind-timer") == 0))  {
             parser = new Uint32Parser(config_id, uint32_values_);
         } else if ((config_id.compare("subnet") == 0) ||
-                   (config_id.compare("interface") == 0)) {
+                   (config_id.compare("interface") == 0) ||
+                   (config_id.compare("next-server") == 0)) {
             parser = new StringParser(config_id, string_values_);
         } else if (config_id.compare("pool") == 0) {
             parser = new Pool4Parser(config_id, pools_);
@@ -257,14 +258,34 @@ protected:
         Triplet<uint32_t> t2 = getParam("rebind-timer");
         Triplet<uint32_t> valid = getParam("valid-lifetime");
 
-        /// @todo: Convert this to logger once the parser is working reliably
         stringstream tmp;
         tmp << addr.toText() << "/" << (int)len
             << " with params t1=" << t1 << ", t2=" << t2 << ", valid=" << valid;
 
         LOG_INFO(dhcp4_logger, DHCP4_CONFIG_NEW_SUBNET).arg(tmp.str());
 
-        subnet_.reset(new Subnet4(addr, len, t1, t2, valid));
+        Subnet4Ptr subnet4(new Subnet4(addr, len, t1, t2, valid));
+        subnet_ = subnet4;
+
+        // Try global value first
+        try {
+            string next_server = globalContext()->string_values_->getParam("next-server");
+            if (!next_server.empty()) {
+                subnet4->setSiaddr(IOAddress(next_server));
+            }
+        } catch (const DhcpConfigError&) {
+            // Don't care. next_server is optional. We can live without it
+        }
+
+        // Try subnet specific value if it's available
+        try {
+            string next_server = string_values_->getParam("next-server");
+            if (!next_server.empty()) {
+                subnet4->setSiaddr(IOAddress(next_server));
+            }
+        } catch (const DhcpConfigError&) {
+            // Don't care. next_server is optional. We can live without it
+        }
     }
 };
 
@@ -359,7 +380,8 @@ DhcpConfigParser* createGlobalDhcp4ConfigParser(const std::string& config_id) {
     } else if (config_id.compare("option-def") == 0) {
         parser  = new OptionDefListParser(config_id,
                                           globalContext()->option_defs_);
-    } else if (config_id.compare("version") == 0) {
+    } else if ((config_id.compare("version") == 0) ||
+               (config_id.compare("next-server") == 0)) {
         parser  = new StringParser(config_id,
                                     globalContext()->string_values_);
     } else if (config_id.compare("lease-database") == 0) {

src/bin/dhcp4/dhcp4.spec

@@ -16,7 +16,7 @@
           "item_default": ""
         }
       },
- 
+
       { "item_name": "interfaces",
         "item_type": "list",
         "item_optional": false,
@@ -48,6 +48,12 @@
         "item_default": 4000
       },
 
+      { "item_name": "next-server",
+        "item_type": "string",
+        "item_optional": true,
+        "item_default": ""
+      },
+
       { "item_name": "option-def",
         "item_type": "list",
         "item_optional": false,
@@ -218,6 +224,13 @@
                   "item_optional": false,
                   "item_default": 7200
                 },
+
+                { "item_name": "next-server",
+                  "item_type": "string",
+                  "item_optional": true,
+                  "item_default": "0.0.0.0"
+                },
+
                 { "item_name": "pool",
                   "item_type": "list",
                   "item_optional": false,
@@ -290,7 +303,7 @@
 
         {
             "command_name": "libreload",
-            "command_description": "Reloads the current hooks libraries.", 
+            "command_description": "Reloads the current hooks libraries.",
             "command_args": []
         }
 

src/bin/dhcp4/dhcp4_srv.cc

@@ -701,6 +701,13 @@ Dhcpv4Srv::assignLease(const Pkt4Ptr& question, Pkt4Ptr& answer) {
         return;
     }
 
+    // Set up siaddr. Perhaps assignLease is not the best place to call this
+    // as siaddr has nothing to do with a lease, but otherwise we would have
+    // to select subnet twice (performance hit) or update too many functions
+    // at once.
+    // @todo: move subnet selection to a common code
+    answer->setSiaddr(subnet->getSiaddr());
+
     LOG_DEBUG(dhcp4_logger, DBG_DHCP4_DETAIL_DATA, DHCP4_SUBNET_SELECTED)
         .arg(subnet->toText());
 

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -411,6 +411,151 @@ TEST_F(Dhcp4ParserTest, subnetGlobalDefaults) {
     EXPECT_EQ(4000, subnet->getValid());
 }
 
+// Checks if the next-server defined as global parameter is taken into
+// consideration.
+TEST_F(Dhcp4ParserTest, nextServerGlobal) {
+
+    ConstElementPtr status;
+
+    string config = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"next-server\": \"1.2.3.4\", "
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"subnet\": \"192.0.2.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    ElementPtr json = Element::fromJSON(config);
+
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json));
+
+    // check if returned status is OK
+    checkResult(status, 0);
+
+    // Now check if the configuration was indeed handled and we have
+    // expected pool configured.
+    Subnet4Ptr subnet = CfgMgr::instance().getSubnet4(IOAddress("192.0.2.200"));
+    ASSERT_TRUE(subnet);
+    EXPECT_EQ("1.2.3.4", subnet->getSiaddr().toText());
+}
+
+// Checks if the next-server defined as subnet parameter is taken into
+// consideration.
+TEST_F(Dhcp4ParserTest, nextServerSubnet) {
+
+    ConstElementPtr status;
+
+    string config = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"next-server\": \"1.2.3.4\", "
+        "    \"subnet\": \"192.0.2.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    ElementPtr json = Element::fromJSON(config);
+
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json));
+
+    // check if returned status is OK
+    checkResult(status, 0);
+
+    // Now check if the configuration was indeed handled and we have
+    // expected pool configured.
+    Subnet4Ptr subnet = CfgMgr::instance().getSubnet4(IOAddress("192.0.2.200"));
+    ASSERT_TRUE(subnet);
+    EXPECT_EQ("1.2.3.4", subnet->getSiaddr().toText());
+}
+
+// Test checks several negative scenarios for next-server configuration: bogus
+// address, IPv6 adddress and empty string.
+TEST_F(Dhcp4ParserTest, nextServerNegative) {
+
+    ConstElementPtr status;
+
+    // Config with junk instead of next-server address
+    string config_bogus1 = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"rebind-timer\": 2000, "
+        "    \"renew-timer\": 1000, "
+        "    \"next-server\": \"a.b.c.d\", "
+        "    \"subnet\": \"192.0.2.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    // Config with IPv6 next server address
+    string config_bogus2 = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"rebind-timer\": 2000, "
+        "    \"renew-timer\": 1000, "
+        "    \"next-server\": \"2001:db8::1\", "
+        "    \"subnet\": \"192.0.2.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    // Config with empty next server address
+    string config_bogus3 = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"rebind-timer\": 2000, "
+        "    \"renew-timer\": 1000, "
+        "    \"next-server\": \"\", "
+        "    \"subnet\": \"192.0.2.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    ElementPtr json1 = Element::fromJSON(config_bogus1);
+    ElementPtr json2 = Element::fromJSON(config_bogus2);
+    ElementPtr json3 = Element::fromJSON(config_bogus3);
+
+    // check if returned status is always a failure
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json1));
+    checkResult(status, 1);
+
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json2));
+    checkResult(status, 1);
+
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json3));
+    checkResult(status, 0);
+}
+
+// Checks if the next-server defined as global value is overridden by subnet
+// specific value.
+TEST_F(Dhcp4ParserTest, nextServerOverride) {
+
+    ConstElementPtr status;
+
+    string config = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"next-server\": \"192.0.0.1\", "
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"next-server\": \"1.2.3.4\", "
+        "    \"subnet\": \"192.0.2.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    ElementPtr json = Element::fromJSON(config);
+
+    EXPECT_NO_THROW(status = configureDhcp4Server(*srv_, json));
+
+    // check if returned status is OK
+    checkResult(status, 0);
+
+    // Now check if the configuration was indeed handled and we have
+    // expected pool configured.
+    Subnet4Ptr subnet = CfgMgr::instance().getSubnet4(IOAddress("192.0.2.200"));
+    ASSERT_TRUE(subnet);
+    EXPECT_EQ("1.2.3.4", subnet->getSiaddr().toText());
+}
+
 // 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

@@ -1293,6 +1293,136 @@ TEST_F(Dhcpv4SrvTest, unpackOptions) {
     EXPECT_EQ(0x0, option_bar->getValue());
 }
 
+// Checks whether the server uses default (0.0.0.0) siaddr value, unless
+// explicitly specified
+TEST_F(Dhcpv4SrvTest, siaddrDefault) {
+    boost::scoped_ptr<NakedDhcpv4Srv> srv;
+    ASSERT_NO_THROW(srv.reset(new NakedDhcpv4Srv(0)));
+    IOAddress hint("192.0.2.107");
+
+    Pkt4Ptr dis = Pkt4Ptr(new Pkt4(DHCPDISCOVER, 1234));
+    dis->setRemoteAddr(IOAddress("192.0.2.1"));
+    OptionPtr clientid = generateClientId();
+    dis->addOption(clientid);
+    dis->setYiaddr(hint);
+
+    // Pass it to the server and get an offer
+    Pkt4Ptr offer = srv->processDiscover(dis);
+    ASSERT_TRUE(offer);
+
+    // Check if we get response at all
+    checkResponse(offer, DHCPOFFER, 1234);
+
+    // Verify that it is 0.0.0.0
+    EXPECT_EQ("0.0.0.0", offer->getSiaddr().toText());
+}
+
+// Checks whether the server uses specified siaddr value
+TEST_F(Dhcpv4SrvTest, siaddr) {
+    boost::scoped_ptr<NakedDhcpv4Srv> srv;
+    ASSERT_NO_THROW(srv.reset(new NakedDhcpv4Srv(0)));
+    subnet_->setSiaddr(IOAddress("192.0.2.123"));
+
+    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);
+    ASSERT_TRUE(offer);
+
+    // Check if we get response at all
+    checkResponse(offer, DHCPOFFER, 1234);
+
+    // Verify that its value is proper
+    EXPECT_EQ("192.0.2.123", offer->getSiaddr().toText());
+}
+
+// Checks if the next-server defined as global value is overridden by subnet
+// specific value and returned in server messages. There's also similar test for
+// checking parser only configuration, see Dhcp4ParserTest.nextServerOverride in
+// config_parser_unittest.cc.
+TEST_F(Dhcpv4SrvTest, nextServerOverride) {
+
+    NakedDhcpv4Srv srv(0);
+
+    ConstElementPtr status;
+
+    string config = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"next-server\": \"192.0.0.1\", "
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"next-server\": \"1.2.3.4\", "
+        "    \"subnet\": \"192.0.2.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    ElementPtr json = Element::fromJSON(config);
+
+    EXPECT_NO_THROW(status = configureDhcp4Server(srv, json));
+
+    // check if returned status is OK
+    ASSERT_TRUE(status);
+    comment_ = config::parseAnswer(rcode_, status);
+    ASSERT_EQ(0, rcode_);
+
+    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);
+    ASSERT_TRUE(offer);
+    EXPECT_EQ(DHCPOFFER, offer->getType());
+
+    EXPECT_EQ("1.2.3.4", offer->getSiaddr().toText());
+}
+
+// Checks if the next-server defined as global value is used in responses
+// when there is no specific value defined in subnet and returned to the client
+// properly. There's also similar test for checking parser only configuration,
+// see Dhcp4ParserTest.nextServerGlobal in config_parser_unittest.cc.
+TEST_F(Dhcpv4SrvTest, nextServerGlobal) {
+
+    NakedDhcpv4Srv srv(0);
+
+    ConstElementPtr status;
+
+    string config = "{ \"interfaces\": [ \"*\" ],"
+        "\"rebind-timer\": 2000, "
+        "\"renew-timer\": 1000, "
+        "\"next-server\": \"192.0.0.1\", "
+        "\"subnet4\": [ { "
+        "    \"pool\": [ \"192.0.2.1 - 192.0.2.100\" ],"
+        "    \"subnet\": \"192.0.2.0/24\" } ],"
+        "\"valid-lifetime\": 4000 }";
+
+    ElementPtr json = Element::fromJSON(config);
+
+    EXPECT_NO_THROW(status = configureDhcp4Server(srv, json));
+
+    // check if returned status is OK
+    ASSERT_TRUE(status);
+    comment_ = config::parseAnswer(rcode_, status);
+    ASSERT_EQ(0, rcode_);
+
+    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);
+    ASSERT_TRUE(offer);
+    EXPECT_EQ(DHCPOFFER, offer->getType());
+
+    EXPECT_EQ("192.0.0.1", offer->getSiaddr().toText());
+}
+
+
 // a dummy MAC address
 const uint8_t dummyMacAddr[] = {0, 1, 2, 3, 4, 5};
 

src/lib/dhcpsrv/subnet.cc

@@ -144,13 +144,26 @@ Subnet4::Subnet4(const isc::asiolink::IOAddress& prefix, uint8_t length,
                  const Triplet<uint32_t>& t1,
                  const Triplet<uint32_t>& t2,
                  const Triplet<uint32_t>& valid_lifetime)
-    :Subnet(prefix, length, t1, t2, valid_lifetime) {
+    :Subnet(prefix, length, t1, t2, valid_lifetime),
+    siaddr_(IOAddress("0.0.0.0")) {
     if (!prefix.isV4()) {
         isc_throw(BadValue, "Non IPv4 prefix " << prefix.toText()
                   << " specified in subnet4");
     }
 }
 
+void Subnet4::setSiaddr(const isc::asiolink::IOAddress& siaddr) {
+    if (!siaddr.isV4()) {
+        isc_throw(BadValue, "Can't set siaddr to non-IPv4 address "
+                  << siaddr.toText());
+    }
+    siaddr_ = siaddr;
+}
+
+isc::asiolink::IOAddress Subnet4::getSiaddr() const {
+    return (siaddr_);
+}
+
 const PoolCollection& Subnet::getPools(Lease::Type type) const {
     // check if the type is valid (and throw if it isn't)
     checkType(type);

src/lib/dhcpsrv/subnet.h

@@ -466,6 +466,18 @@ public:
             const Triplet<uint32_t>& t2,
             const Triplet<uint32_t>& valid_lifetime);
 
+    /// @brief Sets siaddr for the Subnet4
+    ///
+    /// Will be used for siaddr field (the next server) that typically is used
+    /// as TFTP server. If not specified, the default value of 0.0.0.0 is
+    /// used.
+    void setSiaddr(const isc::asiolink::IOAddress& siaddr);
+
+    /// @brief Returns siaddr for this subnet
+    ///
+    /// @return siaddr value
+    isc::asiolink::IOAddress getSiaddr() const;
+
 protected:
 
     /// @brief Check if option is valid and can be added to a subnet.
@@ -488,6 +500,9 @@ protected:
     /// @param type type to be checked
     /// @throw BadValue if invalid value is used
     virtual void checkType(Lease::Type type) const;
+
+    /// @brief siaddr value for this subnet
+    isc::asiolink::IOAddress siaddr_;
 };
 
 /// @brief A pointer to a Subnet4 object

src/lib/dhcpsrv/tests/subnet_unittest.cc

@@ -58,6 +58,24 @@ TEST(Subnet4Test, in_range) {
     EXPECT_FALSE(subnet.inRange(IOAddress("255.255.255.255")));
 }
 
+// Checks whether siaddr field can be set and retrieved correctly.
+TEST(Subnet4Test, siaddr) {
+    Subnet4 subnet(IOAddress("192.0.2.1"), 24, 1000, 2000, 3000);
+
+    // Check if the default is 0.0.0.0
+    EXPECT_EQ("0.0.0.0", subnet.getSiaddr().toText());
+
+    // Check that we can set it up
+    EXPECT_NO_THROW(subnet.setSiaddr(IOAddress("1.2.3.4")));
+
+    // Check that we can get it back
+    EXPECT_EQ("1.2.3.4", subnet.getSiaddr().toText());
+
+    // Check that only v4 addresses are supported
+    EXPECT_THROW(subnet.setSiaddr(IOAddress("2001:db8::1")),
+        BadValue);
+}
+
 TEST(Subnet4Test, Pool4InSubnet4) {
 
     Subnet4Ptr subnet(new Subnet4(IOAddress("192.1.2.0"), 24, 1, 2, 3));