BIND 10 #3195: Kea6: Relayed traffic must be supported on unicast addresses

[BIND 10 Development]

#3195: Kea6: Relayed traffic must be supported on unicast addresses
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  defect        |                       Status:
            Priority:  very high     |  reviewing
           Component:  dhcp6         |                    Milestone:
            Keywords:                |  Sprint-DHCP-20131016
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:
         Total Hours:  0             |  Medium
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tmark):

 * owner:  tmark => tomek


Comment:

 General:

 This really applies to issues in general. It would be helpful to others if
 the issue contained an overal description of how the issue was resolved.
 Something akin to a design description so readers at least of a road map
 of what was done.
 It can be time consuming to unravel it from looking at the diffs and git
 log.

 -------------------------------------------------------------------------

 src/lib/dhcpsrv/cfgmgr.h

 Suggestion, maybe you may want to mention that the pointer returned by

 CfgMgr::getUnicast(const std::string& iface)

 could go out of scope if the underlying IOAddress were deleted.
 I leave this to you.

 -------------------------------------------------------------------------

 src/lib/dhcpsrv/cfgmgr.cc

 CfgMgr::addActiveIface(std::string iface)

 1. You changed the input parameter from a const string reference to a
 non-const string so you could alter it within the method on:

   iface = iface.substr(0,pos);

 But I think this just ends up making a copy of iface anyway.  I think it
 would have been cleaner to leave the parameter as a const and save the
 interface string to a local variable.

 2. This method assumes that the address is a unicast address if it
 converts when
 passed to IOAddress.  Is this a safe assumption? Are there no values
 that would convert into an IOAddress but that would not be a valid unicast
 address?

 -------------------------------------------------------------------------

 src/lib/dhcpsrv/tests/cfgmgr_unittest.cc

 1. I do not see tests for attempts to add invalid interfaces with invalid
  address strings or duplicates.

 2. Calls to cfg_mgr.addActiveInterface() ought to be wrapped in
 EXPECT_NO_THROW or ASSERT_NO_THROWs.

 -------------------------------------------------------------------------

 src/bin/dhcp6/dhcp6_srv.cc

 Dhcpv6Srv::openActiveSockets(const uint16_t port)

 This method calls clearUnicasts() on the each interface, yet it can only
 ever
 add a single interface.  Why does the interface support more than one
 unicast
 even though we do not allow more than one per interface to be configured?

 -------------------------------------------------------------------------

 src/lib/dhcp/iface_mgr.h

 There are no method comments for the new unicast methods.

 -------------------------------------------------------------------------

 src/lib/dhcp/iface_mgr.h

 Should addUnicast() defend against duplicate entries?  If not, maybe a
 warning
 in its commentary (after you add it) is warranted.

 -------------------------------------------------------------------------

 src/lib/dhcp/iface_mgr.cc

 It would be even better if the socket operation exceptions included the
 error
 message returned by strerror(), like this:

 {{{
     if (sock < 0) {
         // get it first before anything resets errno...
         const char* errstr = strerror(errno);
         isc_throw(SocketConfigError, "failed to open unicast socket on "
             << addr->toText() << " on interface " << iface->getName()
             << " reason: " << errstr);
              }

 }}}

 You should get a meaningful error on most if not all OSs.  Telling me you
 can't
 open socket is great without telling me why is just going to aggravate me
 ;)


 -------------------------------------------------------------------------

 src/lib/dhcp/iface_mgr.cc

    IfaceMgr::getSocket(const isc::dhcp::Pkt6& pkt)

 Your link-local/global test doesn't seem right. The second half of the
 expression:

 {{{
   (!pkt.getRemoteAddr().getAddress().to_v6().is_link_local() &&
                   s->addr_.getAddress().to_v6().is_link_local()) )
 }}}

 Is true whent the packet is not local but the socket IS local. Shouldn't
 it be
 this?

 {{{
   (!pkt.getRemoteAddr().getAddress().to_v6().is_link_local() &&
                  !(s->addr_.getAddress().to_v6().is_link_local())) )
 }}}

 --------------------------------------------------------------------------

 Regarding the disabled unit tests, should we create a ticket for this?



 ChangeLog and guide updates are fine.

-- 
Ticket URL: <http://bind10.isc.org/ticket/3195#comment:3>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development