[2898] Second part of changes after review

doc/guide/bind10-guide.xml

@@ -4871,43 +4871,54 @@ should include options from the isc option space:
       <section id="dhcp6-relays">
         <title>DHCPv6 Relays</title>
         <para>
-          DHCPv6 server supports remote clients connected via relays. Server
-          that has many subnets defined and receives a request from client, must
-          select appropriate subnet for it. Remote clients there are two
-          mechanisms that can be used here.  The first one is based on
-          interface-id options. While forwarding client's message, relays may
-          insert interface-id option that identifies the interface on which the
-          client message was received on. Some relays allow configuration of
-          that parameter, but it is sometimes hardcoded. This may range from
-          very simple (e.g. "vlan100") to very cryptic. One example used by real
-          hardware was "ISAM144|299|ipv6|nt:vp:1:110"). This may seem
-          meaningless, but that information is sufficient for its
-          purpose. Server may use it to select appropriate subnet and the relay
-          will know which interface to use for response transmission when it
-          gets that value back. This value must be unique in the whole
-          network. Server configuration must match whatever values are inserted
-          by the relays.
+          A DHCPv6 server with multiple subnets defined must select the
+          appropriate subnet when it receives a request from client.  For clients
+          connected via relays, two mechanisms are used.
         </para>
         <para>
-          The second way in which server may pick the proper subnet is by using
-          linkaddr field in the RELAY_FORW message. Name of this field is somewhat
-          misleading. It does not contain link-layer address, but an address that
-          is used to identify a link. This typically is a global address. Kea
-          server checks if that address belongs to any defined subnet6. If it does,
-          that subnet is selected for client's request.
+          The first uses the linkaddr field in the RELAY_FORW message. The name
+          of this field is somewhat misleading in that it does not contain link-layer
+          address: instead, it holds an address (typically a global address) that is
+          used to identify a link. The DHCPv6 server checks if the address belongs
+          to a defined subnet and, if it does, that subnet is selected for the client's
+          request.
         </para>
         <para>
-          It should be noted that "interface" that defines which local network
-          interface can be used to access a given subnet and "interface-id" that
-          specify the content of the interface-id option used by relays are mutually
-          exclusive. A subnet cannot be both reachable locally (direct traffic) and 
-          via relays (remote traffic). Specifying both is a configuration error and
-          Kea server will refuse such configuration.
+          The second mechanism is based on interface-id options. While forwarding client's
+          message, relays may insert an interface-id option into the message that
+          identifies the interface on the relay that received client message. (Some
+          relays allow configuration of that parameter, but it is sometimes
+          hardcoded and may range from very simple (e.g. "vlan100") to very cryptic:
+          one example seen on real hardware was "ISAM144|299|ipv6|nt:vp:1:110"). The
+          server can use this information to select the appropriate subnet.
+          The information is also returned to the relay which then knows which
+          interface to use to transmit the response to the client. In order to
+          use this successfully, the relay interface IDs must be unique within
+          the network, and the server configuration must match those values.
         </para>
         <para>
+          When configuring the DHCPv6 server, it should be noted that two
+          similarly-named parameters can be configured for a subnet:
+          <itemizedlist>
+            <listitem><simpara>
+              "interface" defines which local network interface can be used
+              to access a given subnet.
+            </simpara></listitem>
+            <listitem><simpara>
+              "interface-id" specifies the content of the interface-id option
+              used by relays to identify the interface on the relay to which
+              the response packet is sent.
+            </simpara></listitem>
+          </itemizedlist>
+          The two are mutually exclusive: a subnet cannot be both reachable locally
+          (direct traffic) and via relays (remote traffic). Specifying both is a
+          configuration error and the DHCPv6 server will refuse such a configuration.
+        </para>
+
+        <para>
           To specify interface-id with value "vlan123", the following commands can
           be used:
-<screen>
+          <screen>
 &gt; <userinput>config add Dhcp6/subnet6</userinput>
 &gt; <userinput>config set Dhcp6/subnet6[0]/subnet "2001:db8:beef::/48"</userinput>
 &gt; <userinput>config set Dhcp6/subnet6[0]/pool [ "2001:db8:beef::/48" ]</userinput>

src/lib/dhcp/pkt6.cc

@@ -73,56 +73,58 @@ uint16_t Pkt6::len() {
 }
 
 OptionPtr Pkt6::getAnyRelayOption(uint16_t opt_type, RelaySearchOrder order) {
-    int start = 0;
-    int end = 0;
-    int direction = 0;
 
     if (relay_info_.empty()) {
-        // there's no relay info, this is a direct message
+        // There's no relay info, this is a direct message
         return (OptionPtr());
     }
 
+    int start = 0; // First relay to check
+    int end = 0;   // Last relay to check
+    int direction = 0; // How we going to iterate: forward or backward?
+
     switch (order) {
     case RELAY_SEARCH_FROM_CLIENT:
-        // search backwards
+        // Search backwards
         start = relay_info_.size() - 1;
         end = 0;
         direction = -1;
         break;
     case RELAY_SEARCH_FROM_SERVER:
-        // search forward
+        // Search forward
         start = 0;
         end = relay_info_.size() - 1;
         direction = 1;
         break;
-    case RELAY_SEARCH_FIRST:
-        // look at the innermost relay only
+    case RELAY_GET_FIRST:
+        // Look at the innermost relay only
         start = relay_info_.size() - 1;
         end = start;
-        direction = 0;
+        direction = 1;
         break;
-    case RELAY_SEARCH_LAST:
-        // look at the outermost relay only
+    case RELAY_GET_LAST:
+        // Look at the outermost relay only
         start = 0;
         end = 0;
-        direction = 0;
+        direction = 1;
     }
 
-    // this is a tricky loop. It must go from start to end, but it must work in
+    // This is a tricky loop. It must go from start to end, but it must work in
     // both directions (start > end; or start < end). We can't use regular
-    // exit condition, because we don't know whether to use i <= end or i >= end
-    for (int i = start; ; i += direction) {
+    // exit condition, because we don't know whether to use i <= end or i >= end.
+    // That's why we check if in the next iteration we would go past the
+    // list (end + direction). It is similar to STL concept of end pointing
+    // to a place after the last element
+    for (int i = start; i != end + direction; i += direction) {
         OptionPtr opt = getRelayOption(opt_type, i);
         if (opt) {
             return (opt);
         }
-
-        if (i == end) {
-            break;
-        }
     }
 
-    return getRelayOption(opt_type, end);
+    // We iterated over specified relays and haven't found what we were
+    // looking for
+    return (OptionPtr());
 }
 
 
@@ -539,27 +541,28 @@ const char* Pkt6::getName() const {
 
 void Pkt6::copyRelayInfo(const Pkt6Ptr& question) {
 
-    // we use index rather than iterator, because we need that as a parameter
+    // We use index rather than iterator, because we need that as a parameter
     // passed to getRelayOption()
     for (int i = 0; i < question->relay_info_.size(); ++i) {
-        RelayInfo x;
-        x.msg_type_ = DHCPV6_RELAY_REPL;
-        x.hop_count_ = question->relay_info_[i].hop_count_;
-        x.linkaddr_ = question->relay_info_[i].linkaddr_;
-        x.peeraddr_ = question->relay_info_[i].peeraddr_;
-
-        // Is there interface-id option in this nesting level if there is,
-        // we need to echo it back
+        RelayInfo info;
+        info.msg_type_ = DHCPV6_RELAY_REPL;
+        info.hop_count_ = question->relay_info_[i].hop_count_;
+        info.linkaddr_ = question->relay_info_[i].linkaddr_;
+        info.peeraddr_ = question->relay_info_[i].peeraddr_;
+
+        // Is there an interface-id option in this nesting level?
+        // If there is, we need to echo it back
         OptionPtr opt = question->getRelayOption(D6O_INTERFACE_ID, i);
         // taken from question->RelayInfo_[i].options_
         if (opt) {
-            x.options_.insert(pair<int, boost::shared_ptr<Option> >(opt->getType(), opt));
+            info.options_.insert(make_pair(opt->getType(), opt));
         }
 
-        /// @todo: implement support for ERO (Echo Request Option, RFC4994)
+        /// @todo: Implement support for ERO (Echo Request Option, RFC4994)
 
-        // add this relay-repl info to our message
-        relay_info_.push_back(x);
+        // Add this relay-forw info (client's message) to our relay-repl
+        // message (server's response)
+        relay_info_.push_back(info);
     }
 }
 

src/lib/dhcp/pkt6.h

@@ -60,14 +60,14 @@ public:
     /// relay closest to the server (i.e. outermost in the encapsulated
     /// message, which also means it was the last relay that relayed
     /// the received message and will be the first one to process
-    /// server's response). RELAY_SEARCH_FIRST is option from the first
-    /// relay (closest to the client), RELAY_SEARCH_LAST is the
-    /// last relay (closest to the server).
+    /// server's response). RELAY_GET_FIRST will try to get option from
+    /// the first relay only (closest to the client), RELAY_GET_LAST will
+    /// try to get option form the the last relay (closest to the server).
     enum RelaySearchOrder {
         RELAY_SEARCH_FROM_CLIENT = 1,
         RELAY_SEARCH_FROM_SERVER = 2,
-        RELAY_SEARCH_FIRST = 3,
-        RELAY_SEARCH_LAST = 4
+        RELAY_GET_FIRST = 3,
+        RELAY_GET_LAST = 4
     };
 
     /// @brief structure that describes a single relay information
@@ -226,12 +226,12 @@ public:
     /// @return pointer to the option (or NULL if there is no such option)
     OptionPtr getRelayOption(uint16_t option_code, uint8_t nesting_level);
 
-    /// @brief returns first instance of a specified option
+    /// @brief Return first instance of a specified option
     ///
-    /// When client's packet traverses multiple relays, each passing relay
-    /// may insert extra options. This method allows getting specific instance
-    /// of a given option (closest to the client, closest to the server, etc.)
-    /// See @ref RelaySearchOrder for detailed description.
+    /// When a client's packet traverses multiple relays, each passing relay may
+    /// insert extra options. This method allows the specific instance of a given
+    /// option to be obtained (e.g. closest to the client, closest to the server,
+    /// etc.) See @ref RelaySearchOrder for a detailed description.
     ///
     /// @param option_code searched option
     /// @param order option search order (see @ref RelaySearchOrder)

src/lib/dhcp/tests/pkt6_unittest.cc

@@ -609,8 +609,14 @@ TEST_F(Pkt6Test, getAnyRelayOption) {
 
     // First check that the getAnyRelayOption does not confuse client options
     // and relay options
-    OptionPtr opt = msg->getAnyRelayOption(300, Pkt6::RELAY_SEARCH_FROM_CLIENT);
     // 300 is a client option, present in the message itself.
+    OptionPtr opt = msg->getAnyRelayOption(300, Pkt6::RELAY_SEARCH_FROM_CLIENT);
+    EXPECT_FALSE(opt);
+    opt = msg->getAnyRelayOption(300, Pkt6::RELAY_SEARCH_FROM_SERVER);
+    EXPECT_FALSE(opt);
+    opt = msg->getAnyRelayOption(300, Pkt6::RELAY_GET_FIRST);
+    EXPECT_FALSE(opt);
+    opt = msg->getAnyRelayOption(300, Pkt6::RELAY_GET_LAST);
     EXPECT_FALSE(opt);
 
     // Option 200 is added in every relay.
@@ -628,12 +634,12 @@ TEST_F(Pkt6Test, getAnyRelayOption) {
     EXPECT_TRUE(opt->equal(relay1_opt1));
 
     // We just want option from the first relay (closest to the client)
-    opt = msg->getAnyRelayOption(200, Pkt6::RELAY_SEARCH_FIRST);
+    opt = msg->getAnyRelayOption(200, Pkt6::RELAY_GET_FIRST);
     ASSERT_TRUE(opt);
     EXPECT_TRUE(opt->equal(relay3_opt1));
 
     // We just want option from the last relay (closest to the server)
-    opt = msg->getAnyRelayOption(200, Pkt6::RELAY_SEARCH_LAST);
+    opt = msg->getAnyRelayOption(200, Pkt6::RELAY_GET_LAST);
     ASSERT_TRUE(opt);
     EXPECT_TRUE(opt->equal(relay1_opt1));
 
@@ -647,12 +653,24 @@ TEST_F(Pkt6Test, getAnyRelayOption) {
     ASSERT_TRUE(opt);
     EXPECT_TRUE(opt->equal(relay2_opt1));
 
-    opt = msg->getAnyRelayOption(100, Pkt6::RELAY_SEARCH_FIRST);
+    opt = msg->getAnyRelayOption(100, Pkt6::RELAY_GET_FIRST);
+    EXPECT_FALSE(opt);
+
+    opt = msg->getAnyRelayOption(100, Pkt6::RELAY_GET_LAST);
+    EXPECT_FALSE(opt);
+
+    // Finally, try to get an option that does not exist
+    opt = msg->getAnyRelayOption(500, Pkt6::RELAY_GET_FIRST);
     EXPECT_FALSE(opt);
 
-    opt = msg->getAnyRelayOption(100, Pkt6::RELAY_SEARCH_LAST);
+    opt = msg->getAnyRelayOption(500, Pkt6::RELAY_GET_LAST);
     EXPECT_FALSE(opt);
 
+    opt = msg->getAnyRelayOption(500, Pkt6::RELAY_SEARCH_FROM_SERVER);
+    EXPECT_FALSE(opt);
+
+    opt = msg->getAnyRelayOption(500, Pkt6::RELAY_SEARCH_FROM_CLIENT);
+    EXPECT_FALSE(opt);
 }
 
 }

src/lib/dhcpsrv/cfgmgr.cc

@@ -183,7 +183,8 @@ Subnet6Ptr CfgMgr::getSubnet6(OptionPtr iface_id_option) {
         return (Subnet6Ptr());
     }
 
-    // If there is more than one, we need to choose the proper one
+    // Let's iterate over all subnets and for those that have interface-id
+    // defined, check if the interface-id is equal to what we are looking for
     for (Subnet6Collection::iterator subnet = subnets6_.begin();
          subnet != subnets6_.end(); ++subnet) {
         if ( (*subnet)->getInterfaceId() &&

src/lib/dhcpsrv/dhcpsrv_messages.mes

@@ -105,14 +105,14 @@ This is a debug message reporting that the DHCP configuration manager
 has returned the specified IPv6 subnet for a packet received over
 given interface.  This particular subnet was selected, because it
 was specified as being directly reachable over given interface. (see
-'interface' parameter in subnet6 definition).
+'interface' parameter in the subnet6 definition).
 
 % DHCPSRV_CFGMGR_SUBNET6_IFACE_ID selected subnet %1 (interface-id match) for incoming packet
 This is a debug message reporting that the DHCP configuration manager
 has returned the specified IPv6 subnet for a received packet. This particular
 subnet was selected, because value of interface-id option matched what was
 configured in server's interface-id option for that selected subnet6.
-(see 'interface-id' parameter in subnet6 definition).
+(see 'interface-id' parameter in the subnet6 definition).
 
 % DHCPSRV_CLOSE_DB closing currently open %1 database
 This is a debug message, issued when the DHCP server closes the currently

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc

@@ -49,7 +49,7 @@ TEST(ValueStorageTest, BooleanTesting) {
     EXPECT_FALSE(testStore.getParam("firstBool"));
     EXPECT_TRUE(testStore.getParam("secondBool"));
 
-    // Verify that we can update paramaters.
+    // Verify that we can update parameters.
     testStore.setParam("firstBool", true);
     testStore.setParam("secondBool", false);
 
@@ -437,6 +437,10 @@ TEST_F(CfgMgrTest, subnet6Interface) {
     // Now we have only one subnet, any request will be served from it
     EXPECT_EQ(subnet1, cfg_mgr.getSubnet6("foo"));
 
+    // Check that the interface name is checked even when there is
+    // only one subnet defined.
+    EXPECT_FALSE(cfg_mgr.getSubnet6("bar"));
+
     // If we have only a single subnet and the request came from a local
     // address, let's use that subnet
     EXPECT_EQ(subnet1, cfg_mgr.getSubnet6(IOAddress("fe80::dead:beef")));