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:  tmark
                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 tomek):

 * owner:  tomek => tmark


Comment:

 Replying to [comment:3 tmark]:
 > 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.
 True. Sorry about that. The tickets were created in rush. I'll try to
 update the descriptions before the ticket hits the review.

 > 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.
 Done.

 > src/lib/dhcpsrv/cfgmgr.cc
 > I think it
 > would have been cleaner to leave the parameter as a const and save the
 > interface string to a local variable.
 Done.

 > 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?
 This is not checked on purpose. Obvious rubbish that does not form a valid
 address will cause exception to be thrown in IOAddress class. There are
 many address types that, depending on the definition, may or may not be
 considered unicasts. But this is the flexibility that we can offer. If
 someone is eager to run his DHCP server over loopback, let him have it.

 I added a warning about that to the guide though. It explains that no
 validation is done on that address and the server will attempt to bind a
 socket to it.

 > 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.
 This is much more complex than meets the eye. I added an explanation at
 the end of cfgmgr_unittest.cc.


 > 2. Calls to cfg_mgr.addActiveInterface() ought to be wrapped in
 > EXPECT_NO_THROW or ASSERT_NO_THROWs.
 I hope I fixed all of them.

 > 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?
 Because we will support multiple unicast addresses on each interface. The
 cfgmgr part is pretty easy. Extending parsers to allow it is not. So it
 was compromise. I doubt that we will encounter anyone complaining about
 not being able support second unicast on a given interface in the next
 couple years, though.


 > src/lib/dhcp/iface_mgr.h
 > There are no method comments for the new unicast methods.
 There are now.

 > 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.
 It does now. Added a test to verify it.

 > 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:
 Excellent idea. I added it to the new socket code and extended couple of
 existing
 ones.

 > Telling me you can't open socket is great without telling me why is just
 going to aggravate me ;)
 You're not the only one. There's a ticket #2765 about not our error
 messages sucking. Note that it was submitted by external user, not us! It
 is 14 months old...

 > 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())) )
 > }}}
 Good catch. Fixed. I'm wondering how it did work. I checked it that it
 correctly responds to both unicast and multicast addresses, so the code
 was working properly.

 > Regarding the disabled unit tests, should we create a ticket for this?
 Added #3206.

 > ChangeLog and guide updates are fine.
 Thanks for doing the review.

 The ticket is back with you.

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