BIND 10 #2414: Use AllocationEngine to actually handle v6 clients' messages

[BIND 10 Development]

#2414: Use AllocationEngine to actually handle v6 clients' messages
-------------------------------------+-------------------------------------
                   Reporter:  tomek  |                 Owner:  stephen
                       Type:  task   |                Status:  reviewing
                   Priority:         |             Milestone:  Sprint-
  medium                             |  DHCP-20121101
                  Component:  dhcp6  |            Resolution:
                   Keywords:         |             Sensitive:  0
            Defect Severity:  N/A    |           Sub-Project:  DHCP
Feature Depending on Ticket:         |  Estimated Difficulty:  0
        Add Hours to Ticket:  0      |           Total Hours:  0
                  Internal?:  0      |
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => stephen


Comment:

 Replying to [comment:5 stephen]:
 > '''src/bin/dhcp6/dhcp6_messages.mes'''[[BR]]
 > DHCP_DB_BACKEND_STARTED: Second sentence would probably be better
 expressed as "It indicates what database backend is being used to store
 lease and other information."
 Done.

 > DHCP_LEASE_ALLOC(_FAIL): is it possible to distinguish between the
 advertised or allocated message - either by a different code or by means
 of another field in the message?
 Done. There are 4 messages now: DHCP6_LEASE_ADVERT(_FAIL) and
 DHCP6_LEASE_ALLOC(_FAIL).
 >
 > DHCP6_SUBNET*: the message should start with a lower-case character.
 I thought the the message is supposed to be a full English sentence
 (starting with upper-case and ending with a dot). Anyway, did as you
 asked. There were couple more messages that started with upper-case, so I
 fixed them as well.

 > DHCP6_SUBNET_SELECTION_FAILED: suggest:
 > "This warning message is output when a packet was received from a subnet
 for which the DHCPv6 server has not been configured.  The cause is most
 likely due to a misconfiguration of the server.  The packet has been
 ignored."
 > (''Note - Is the last sentence true?'')
 No. Server will include status-code option with code NoAddrsAvail and
 continue processing the message. That behavior is enforced by RFC3315. It
 is possible (but unlikely) that the same message may contain other options
 that will be served (e.g. IA_PD or another IA_NA option).

 On a side note, the getSubnet() method is very simple for now, but it will
 become much more complex once we start supporting relays. Also, I have an
 idea for very simple way of differentiating between interfaces (so the
 user will be able to specify that subnet A should be served over eth0 and
 subnet B over eth1).

 > '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 > Dhcp6Srv constructor: Not clear about the comment "used for testing
 purposes": that looks as if it should apply to the check of
 "countIfaces()", not the check of "port".
 Port 0 has special meaning - to not open sockets at all. Clarified the
 comment.

 > Dhcp6Srv constructor: "used for testing..." should be "Used for
 testing".
 Fixed.

 > "if (port) {".  As port is numeric, would prefer "if (port > 0) {".
 Done.
 >
 > The comments should make it clear that the test usage is with the port
 equal to zero.
 Done.

 > The "todo" indicates that the lease manager should be implemented at the
 point of the "todo".  But the code instantiates the memfile lease manage
 later on - where is the correct position?
 Removed that old @todo.

 > run(): what is the timeout measured in?
 I seconds. Expanded that comment.
 >
 > setServerID(): comment: the logical place to store this information is
 the database.
 In theory yes, but practice may suggest something else. A server is a
 server, regardless of the database it is using. Server-Id should not
 change even if you change backends. Storing the server duid in the backend
 will cause the server to change identity if you change backends. There's
 very specific text in RFC3315 that we will break with this behavior.

 We could provide a way to kind of keep the data in sync, but that will get
 us into a completely new topic of data migration (and even worse, possible
 synchronization) between backends. So yes - I agree with you in principle,
 but that is not as simple as it looks.

 > setServerID(): comments should start with a capital letter.
 Fixed.
 > setServerID(): "&srvid[0]+8" should be "&srvid[0] + 8".
 Done.
 >
 > setServerID(): call to fillrandom() should have a space after the comma.
 Done.
 >
 > copyDefaultOptions: use the "!OptionPtr" typedef instead of the explicit
 type of boost::shared_ptr<Option>. (Incidentally, both addOption() methods
 expect the argument to be passed by value, not reference: this will incur
 the overhead of incrementing and decrementing the reference count of the
 pointed-to object.)
 >
 > addRequestedOptions: use the "!OptionPtr" typedef instead of the
 explicit type of boost::shared_ptr<Option>.
 This code predates the OptionPtr definition. Updated.
 >
 > handleIA_NA: need more comments to explain what is happening.
 Added more comments.
 >
 > assignLeases: need more comments to explain what is happening.
 Added more comments.

 > processXXX: a copy why "copyDefaultOptions" is immediately followed by
 "appendDefaultOptions" (which from their names, appear to add two sets of
 default options to the created packet) would be helpful.

 The server's response contains many options. Some of them are copied from
 client request (copyDefaultOptions, e.g. client-id) and some are always
 inserted by the server (appendDefaultOptions, e.g. server-id or
 preference). In both cases, client does not have to request it in any way
 (on ORO), so in that sense they are default (options that must be sent by
 default). There are also other options that are sent back only if
 requested (appendRequestedOptions method).

 That's the theory. In practice, the lines between them will be blurred, as
 Marcin implements mechanism for sending options even if they are not
 requested by clients. Once we add relay support, there will be other
 things like ERO (echo request option), where relay asks server to send a
 relay option back to the client. I think it should be rethinked, but not
 as part of this ticket.

 > '''src/bin/dhcp6/dhcp6_srv.h'''[[BR]]
 > Trivial point: start all "@brief" descriptions with a capital letter.
 Done.
 >
 > selectSubnet: need "@param" describing the "question" argument.
 Done.

 > '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''[[BR]]
 > Would appreciate some documentation about the methods of the
 Dhcp6SrvTest class (i.e. they should include a header, although perhaps
 not a Doxygen one.)
 >
 > A number of the tests have identical function headers.
 Most of them were similar (no hint sent, valid hint, invalid hint), but in
 one instance it was an identical copy. Fixed.

 > Solicit tests: is it worth checking that a sequence of solicit messages
 result in advertise responses with different addresses?
 It makes perfect sense. It also makes sense to implement similar test for
 request. See new tests: ManySolicits and ManyRequests.

 > '''src/lib/dhcp/Makefile.am'''[[BR]]
 > OK.  The memfile backend is probably better put in the main dhcp
 directory (with appropriate "TODO"s about not being finished.)
 Done. Added a text to memfile_LeaseMgr constructor that warns about
 memfile being incomplete.

 I have noted that there is some dependency between b10-dhcp++ and
 b10-dhcpsrv libraries. It fails to build occasionally with: make -j8 (I
 use 8 cores). After couple retries it finally builds. We should move
 libdhcpsrv to separate directory.

 > '''src/lib/dhcp/addr_utilities.cc'''[[BR]]
 > {first,last}AddrInPrefix4() already have a check for prefix length.
 (Although it should precede the assignment of the "addr" variable - which
 is redundant if the exception is thrown - and there should be a space
 around the ">" in lastAddrInPrefix4.) For this reason, I suggest the
 prefix lengths checks be removed from {first,last}!AddrInPrefix(), and
 checks added to the V6 versions (remembering to put spaces around the ">"
 in the comparison).
 Done.

 > '''src/lib/dhcp/cfgmgr.cc'''[[BR]]
 > getSubnet6(): Spurious space "if ( (subnets6_.size()...": there should
 be no space between the two opening parentheses.
 Fixed.

 > getSubnet6(): I think we've discussed this before, but what happens if
 the user has a configured subnet that does not match the configured single
 pool?  The logic here is that the single subnet is always chosen.  Also,
 suppose that there is a single V6 subnet that is not link local: it is
 rejected by the first check but could still be chosen in the subsequent
 iteration over the (single) subnet.
 The current code in this regard is very simplified. Currently we support
 only direct (not relayed) traffic, we don't differentiate between
 interfaces and we don't support shared subnets. That means that we always
 handle only a single subnet and the client's source address is always
 link-local (fe80::/10).


 > '''src/lib/dhcp/duid.cc'''[[BR]]
 > toText(): For consistency, either set the string stream's fill
 character, field width and output base with calls to "fill()", "width()",
 and "setf()" before the loop, or set them on a per-output call inside the
 loop with the manipulators hex, setfill and setw.
 They cannot be set before the loop, as width() and setfill() operators
 affect only the next token, not the whole stream. Hex manipulator on the
 other hand, affects the whole stream, so it is now done before the loop
 (no need to repeat it).


 > '''src/lib/dhcp/duid.h'''[[BR]]
 > It is not usual to write "operator==()" as "operator == ()" (same goes
 for the "not equals" operator.)
 Fixed.

 > ''' src/lib/dhcp/subnet.h'''[[BR]]
 > get(): it is probably clearer to use the "make_pair" call:
 > {{{
 > std::pair<isc::asiolink::IOAddress, uint8_t> get() const {
 >     return (std::make_pair(prefix_, prefix_len_));
 > }
 > }}}
 > (Also note that "get()" can be declared "const").
 Done.

 > toText(): this can be declared "const".
 Done.

 > '''src/lib/dhcp/tests/duid_unittest.cc'''[[BR]]
 > toText() test: there is no need to create a DUID via "new" or used a
 scoped pointer to store it: it could be created as an automatic variable:
 > {{{
 > DUID duid1(data1, sizeof(data1));
 > EXPECT_EQ("00:01:02:03:04:ff:fe", duid1.toText());
 > }}}
 Done.

 > '''src/lib/dhcp/tests/lease_mgr_unittest.cc'''[[BR]]
 > OK. (Looks as if I need to complete all the xxx6() methods in the MySQL
 lease manager code for it to be useful.)
 I'm not sure about that. Originally memfile was planned as way to test
 LeaseMgr interface, so those tests may continue to use memfile backend.
 IIRC you have developed dedicated tests to MySQL backend.

 > '''src/lib/dhcp/tests/subnet_unittest.cc'''[[BR]]
 > toText() test: there is no need to create a Subnet4 via "new" or used a
 scoped pointer to store it: it could be created as an automatic variable.
 (See comment for duid_unittest.cc.)
 I've converted some of the tests. Tests don't have to be consistent. In
 fact, variety of approaches should be preferred to test them. You can
 consider some of the tests as implicit test for Subnet6Ptr type. If the
 shared_ptr fails to release the object, we will fail valgrind.

 Changes pushed. The ticket is back with you. I will have some spare cycles
 during IETF, but I would very much prefer to do non-critical things, as
 there may be folks hunting for me for IETF things.

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