[5377] DHCPv4 server now respects the outbound-interface setting.

src/bin/dhcp4/dhcp4_srv.cc

@@ -26,6 +26,7 @@
 #include <dhcpsrv/callout_handle_store.h>
 #include <dhcpsrv/cfgmgr.h>
 #include <dhcpsrv/cfg_host_operations.h>
+#include <dhcpsrv/cfg_iface.h>
 #include <dhcpsrv/cfg_subnets4.h>
 #include <dhcpsrv/lease_mgr.h>
 #include <dhcpsrv/lease_mgr_factory.h>
@@ -1139,13 +1140,20 @@ Dhcpv4Srv::srvidToString(const OptionPtr& srvid) {
 
 void
 Dhcpv4Srv::appendServerID(Dhcpv4Exchange& ex) {
-    // The source address for the outbound message should have been set already.
-    // This is the address that to the best of the server's knowledge will be
-    // available from the client.
-    /// @todo: perhaps we should consider some more sophisticated server id
-    /// generation, but for the current use cases, it should be ok.
+
+    // Use local address on which the packet has been received as a
+    // server identifier. In some cases it may be a different address,
+    // e.g. broadcast packet or DHCPv4o6 packet.
+    IOAddress local_addr = ex.getQuery()->getLocalAddr();
+    Pkt4Ptr query = ex.getQuery();
+
+    if (local_addr.isV4Bcast() || query->isDhcp4o6()) {
+        SocketInfo sock_info = IfaceMgr::instance().getSocket(*query);
+        local_addr = sock_info.addr_;
+    }
+
     OptionPtr opt_srvid(new Option4AddrLst(DHO_DHCP_SERVER_IDENTIFIER,
-                                           ex.getResponse()->getLocalAddr()));
+                                           local_addr));
     ex.getResponse()->addOption(opt_srvid);
 }
 
@@ -2027,38 +2035,52 @@ Dhcpv4Srv::adjustIfaceData(Dhcpv4Exchange& ex) {
         response->setRemotePort(DHCP4_SERVER_PORT);
     }
 
-    IOAddress local_addr = query->getLocalAddr();
+    CfgIfacePtr cfg_iface = CfgMgr::instance().getCurrentCfg()->getCfgIface();
+    if (query->isRelayed() &&
+        (cfg_iface->getSocketType() == CfgIface::SOCKET_UDP) &&
+        (cfg_iface->getOutboundIface() == CfgIface::USE_ROUTING)) {
 
-    // In many cases the query is sent to a broadcast address. This address
-    // appears as a local address in the query message. We can't simply copy
-    // this address to a response message and use it as a source address.
-    // Instead we will need to use the address assigned to the interface
-    // on which the query has been received. In other cases, we will just
-    // use this address as a source address for the response.
-    // Do the same for DHCPv4-over-DHCPv6 exchanges.
-    if (local_addr.isV4Bcast() || query->isDhcp4o6()) {
-        SocketInfo sock_info = IfaceMgr::instance().getSocket(*query);
-        local_addr = sock_info.addr_;
+        response->setLocalAddr(IOAddress::IPV4_ZERO_ADDRESS());
+        response->setIface("");
+        response->resetIndex();
+
+    } else {
+
+        IOAddress local_addr = query->getLocalAddr();
+
+        // In many cases the query is sent to a broadcast address. This address
+        // appears as a local address in the query message. We can't simply copy
+        // this address to a response message and use it as a source address.
+        // Instead we will need to use the address assigned to the interface
+        // on which the query has been received. In other cases, we will just
+        // use this address as a source address for the response.
+        // Do the same for DHCPv4-over-DHCPv6 exchanges.
+        if (local_addr.isV4Bcast() || query->isDhcp4o6()) {
+            SocketInfo sock_info = IfaceMgr::instance().getSocket(*query);
+            local_addr = sock_info.addr_;
+        }
+
+        // We assume that there is an appropriate socket bound to this address
+        // and that the address is correct. This is safe assumption because
+        // the local address of the query is set when the query is received.
+        // The query sent to an incorrect address wouldn't have been received.
+        // However, if socket is closed for this address between the reception
+        // of the query and sending a response, the IfaceMgr should detect it
+        // and return an error.
+        response->setLocalAddr(local_addr);
+        // In many cases the query is sent to a broadcast address. This address
+        // appears as a local address in the query message. Therefore we can't
+        // simply copy local address from the query and use it as a source
+        // address for the response. Instead, we have to check what address our
+        // socket is bound to and use it as a source address. This operation
+        // may throw if for some reason the socket is closed.
+        /// @todo Consider an optimization that we use local address from
+        /// the query if this address is not broadcast.
+        response->setIface(query->getIface());
+        response->setIndex(query->getIndex());
     }
-    // We assume that there is an appropriate socket bound to this address
-    // and that the address is correct. This is safe assumption because
-    // the local address of the query is set when the query is received.
-    // The query sent to an incorrect address wouldn't have been received.
-    // However, if socket is closed for this address between the reception
-    // of the query and sending a response, the IfaceMgr should detect it
-    // and return an error.
-    response->setLocalAddr(local_addr);
-    // In many cases the query is sent to a broadcast address. This address
-    // appears as a local address in the query message. Therefore we can't
-    // simply copy local address from the query and use it as a source
-    // address for the response. Instead, we have to check what address our
-    // socket is bound to and use it as a source address. This operation
-    // may throw if for some reason the socket is closed.
-    /// @todo Consider an optimization that we use local address from
-    /// the query if this address is not broadcast.
+
     response->setLocalPort(DHCP4_SERVER_PORT);
-    response->setIface(query->getIface());
-    response->setIndex(query->getIndex());
 }
 
 void

src/bin/dhcp4/dhcp4_srv.h

@@ -681,12 +681,10 @@ protected:
 
     /// @brief Adds server identifier option to the server's response.
     ///
-    /// This method adds a server identifier to the DHCPv4 message. It expects
-    /// that the local (source) address is set for this message. If address is
-    /// not set, it will throw an exception. This method also expects that the
-    /// server identifier option is not present in the specified message.
-    /// Otherwise, it will throw an exception on attempt to add a duplicate
-    /// server identifier option.
+    /// This method adds a server identifier to the DHCPv4 message. This is set
+    /// to the local address on which the client's query has been received with
+    /// the exception of broadcast traffic and DHCPv4o6 query for which a socket
+    /// on the particular interface is found and its address is used as server id.
     ///
     /// @note This method doesn't throw exceptions by itself but the underlying
     /// classes being used my throw. The reason for this method to not sanity

src/bin/dhcp4/tests/dhcp4_srv_unittest.cc

@@ -204,6 +204,88 @@ TEST_F(Dhcpv4SrvTest, adjustIfaceDataRelay) {
     EXPECT_EQ("192.0.1.50", resp->getRemoteAddr().toText());
 }
 
+// This test verifies that it is possible to configure the server to use
+// routing information to determine the right outbound interface to sent
+// responses to a relayed client.
+TEST_F(Dhcpv4SrvTest, adjustIfaceDataUseRouting) {
+    IfaceMgrTestConfig test_config(true);
+    IfaceMgr::instance().openSockets4();
+
+    // Create configuration for interfaces. It includes the outbound-interface
+    // setting which indicates that the responses aren't neccessarily sent
+    // over the same interface via which a request has been received, but routing
+    // information is used to determine this interface.
+    CfgMgr::instance().clear();
+    CfgIfacePtr cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();
+    cfg_iface->useSocketType(AF_INET, CfgIface::SOCKET_UDP);
+    cfg_iface->use(AF_INET, "eth0");
+    cfg_iface->use(AF_INET, "eth1");
+    cfg_iface->setOutboundIface(CfgIface::USE_ROUTING);
+    CfgMgr::instance().commit();;
+
+    // Create the instance of the incoming packet.
+    boost::shared_ptr<Pkt4> req(new Pkt4(DHCPDISCOVER, 1234));
+    // Set the giaddr to non-zero address and hops to non-zero value
+    // as if it was relayed.
+    req->setGiaddr(IOAddress("192.0.1.1"));
+    req->setHops(2);
+    // Set ciaddr to zero. This simulates the client which applies
+    // for the new lease.
+    req->setCiaddr(IOAddress("0.0.0.0"));
+    // Clear broadcast flag.
+    req->setFlags(0x0000);
+
+    // Set local address, port and interface.
+    req->setLocalAddr(IOAddress("192.0.2.5"));
+    req->setLocalPort(1001);
+    req->setIface("eth1");
+    req->setIndex(1);
+
+    // Create the exchange using the req.
+    Dhcpv4Exchange ex = createExchange(req);
+
+    Pkt4Ptr resp = ex.getResponse();
+    resp->setYiaddr(IOAddress("192.0.1.100"));
+    // Clear the remote address.
+    resp->setRemoteAddr(IOAddress("0.0.0.0"));
+    // Set hops value for the response.
+    resp->setHops(req->getHops());
+
+    // This function never throws.
+    ASSERT_NO_THROW(NakedDhcpv4Srv::adjustIfaceData(ex));
+
+    // Now the destination address should be relay's address.
+    EXPECT_EQ("192.0.1.1", resp->getRemoteAddr().toText());
+    // The query has been relayed, so the response must be sent to the port 67.
+    EXPECT_EQ(DHCP4_SERVER_PORT, resp->getRemotePort());
+
+    // The local port is always DHCPv4 server port 67.
+    EXPECT_EQ(DHCP4_SERVER_PORT, resp->getLocalPort());
+
+
+    // No specific interface is selected as outbound interface and no specific
+    // local address is provided. The IfaceMgr will figure out which interface to use.
+    EXPECT_TRUE(resp->getLocalAddr().isV4Zero());
+    EXPECT_TRUE(resp->getIface().empty());
+    EXPECT_FALSE(resp->indexSet());
+
+    // Another test verifies that setting outbound interface to same as inbound will
+    // cause the server to set interface and local address as expected.
+
+    cfg_iface = CfgMgr::instance().getStagingCfg()->getCfgIface();
+    cfg_iface->useSocketType(AF_INET, CfgIface::SOCKET_UDP);
+    cfg_iface->use(AF_INET, "eth0");
+    cfg_iface->use(AF_INET, "eth1");
+    cfg_iface->setOutboundIface(CfgIface::SAME_AS_INBOUND);
+    CfgMgr::instance().commit();
+
+    ASSERT_NO_THROW(NakedDhcpv4Srv::adjustIfaceData(ex));
+
+    EXPECT_EQ("192.0.2.5", resp->getLocalAddr().toText());
+    EXPECT_EQ("eth1", resp->getIface());
+    EXPECT_EQ(1, resp->getIndex());
+}
+
 // This test verifies that the destination address of the response message
 // is set to ciaddr when giaddr is set to zero and the ciaddr is set to
 // non-zero address in the received message. This is the case when the
@@ -487,7 +569,7 @@ TEST_F(Dhcpv4SrvTest, appendServerID) {
 
     // Set a local address. It is required by the function under test
     // to create the Server Identifier option.
-    response->setLocalAddr(IOAddress("192.0.3.1"));
+    query->setLocalAddr(IOAddress("192.0.3.1"));
 
     // Append the Server Identifier.
     ASSERT_NO_THROW(NakedDhcpv4Srv::appendServerID(ex));

src/lib/dhcp/pkt.h

@@ -468,6 +468,11 @@ public:
         ifindex_ = ifindex;
     };
 
+    /// @brief Resets interface index to negative value.
+    void resetIndex() {
+        ifindex_ = -1;
+    }
+
     /// @brief Returns interface index.
     ///
     /// @return interface index
@@ -475,6 +480,13 @@ public:
         return (ifindex_);
     };
 
+    /// @brief Checks if interface index has been set.
+    ///
+    /// @return true if interface index set, false otherwise.
+    bool indexSet() const {
+        return (ifindex_ >= 0);
+    }
+
     /// @brief Returns interface name.
     ///
     /// Returns interface name over which packet was received or is
@@ -697,7 +709,7 @@ protected:
     /// Each network interface has assigned an unique ifindex.
     /// It is a functional equivalent of a name, but sometimes more useful, e.g.
     /// when using odd systems that allow spaces in interface names.
-    int ifindex_;
+    int64_t ifindex_;
 
     /// @brief Local IP (v4 or v6) address.
     ///

src/lib/dhcp/pkt_filter_inet.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2013-2016 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2013-2017 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -255,8 +255,21 @@ PktFilterInet::send(const Iface&, uint16_t sockfd,
     cmsg->cmsg_len = CMSG_LEN(sizeof(struct in_pktinfo));
     struct in_pktinfo* pktinfo =(struct in_pktinfo *)CMSG_DATA(cmsg);
     memset(pktinfo, 0, sizeof(struct in_pktinfo));
-    pktinfo->ipi_ifindex = pkt->getIndex();
-    pktinfo->ipi_spec_dst.s_addr = htonl(pkt->getLocalAddr().toUint32()); // set the source IP address
+
+    // In some cases the index of the outbound interface is not set. This
+    // is a matter of configuration. When the server is configured to
+    // determine the outbound interface based on routing information,
+    // the index is left unset (negative).
+    if (pkt->indexSet()) {
+        pktinfo->ipi_ifindex = pkt->getIndex();
+    }
+
+    // When the DHCP server is using routing to determine the outbound
+    // interface, the local address is also left unset.
+    if (!pkt->getLocalAddr().isV4Zero()) {
+        pktinfo->ipi_spec_dst.s_addr = htonl(pkt->getLocalAddr().toUint32());
+    }
+
     m.msg_controllen = CMSG_SPACE(sizeof(struct in_pktinfo));
 #endif
 

src/lib/dhcpsrv/cfg_iface.cc

@@ -261,7 +261,7 @@ CfgIface::textToOutboundIface(const std::string& txt) {
 
 void
 CfgIface::setOutboundIface(const OutboundIface& outbound_iface) {
-    outbound_iface_ = traffic_type;
+    outbound_iface_ = outbound_iface;
 }
 
 void

src/lib/dhcpsrv/cfg_iface.h

@@ -233,6 +233,11 @@ public:
     void useSocketType(const uint16_t family,
                        const std::string& socket_type_name);
 
+    /// @brief Returns DHCP socket type used by the server.
+    SocketType getSocketType() const {
+        return (socket_type_);
+    }
+
     /// @brief Returns the socket type in the textual format.
     std::string socketTypeToText() const;