Opened 6 years ago

Closed 6 years ago

#3195 closed defect (complete)

Kea6: Relayed traffic must be supported on unicast addresses

Reported by: tomek Owned by: tomek
Priority: very high Milestone: Sprint-DHCP-20131016
Component: dhcp6 Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: Medium
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

DHCP traffic sent by relays may be sent to ALL_DHCP_SERVERS and/or to global unicast address (depending on configuration). We support neither.

Subtickets

Change History (6)

comment:1 Changed 6 years ago by tomek

  • Owner set to UnAssigned
  • Status changed from new to reviewing

The fix for this issue has been implemented. This covers the sockets creation and actual traffic handling. It does not cover responding with UseMulticast? status (see #3077).

This is a tricky code to unit-test. It was extensively tested on my VM setup, but also during dry-run.

I think the ultimate solution to deal with unit-testing this type of features will be to have a small script that has to be run on a test machine before tests are executed. Such a script will set up well known addresses on loopback. BIND9 does similar thing. The other alternative, to write mockup sockets, is less appealing, as it would not test whether the sockets are really usable for sending and receiving data (e.g. would not catch binding issues).

Please review.

comment:2 Changed 6 years ago by tmark

  • Owner changed from UnAssigned to tmark

comment:3 follow-up: Changed 6 years ago by tmark

  • Owner changed from tmark to tomek

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.

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

comment:4 in reply to: ↑ 3 Changed 6 years ago by tomek

  • Owner changed from tomek to tmark

Replying to 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.

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


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

comment:5 Changed 6 years ago by tmark

  • Owner changed from tmark to tomek

Changes and responses are acceptable. Please merge.

comment:6 Changed 6 years ago by tomek

  • Resolution set to complete
  • Status changed from reviewing to closed

Thanks. Merged. Closed.

Note: See TracTickets for help on using tickets.