BIND 10 #2325: Allocation engine v6: renewal

[BIND 10 Development]

#2325: Allocation engine v6: renewal
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:
                Type:  task          |  stephen
            Priority:  medium        |                       Status:
           Component:  dhcp6         |  reviewing
            Keywords:                |                    Milestone:
           Sensitive:  0             |  Sprint-DHCP-20121213
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => stephen


Comment:

 Replying to [comment:5 stephen]:
 > Reviewed commit d6fb00a3198fe327cdbf4ebfe14f47eb23086325
 >
 > '''src/bin/dhcp6/dhcp6_messages.mes'''[[BR]]
 > The text of DHCP6_REQUIRE_OPTIONS_CHECK_FAIL might be better phrased as:
 > {{{
 > This message indicates that received DHCPv6 packet is invalid.  This may
 be due
 > to a number of reasons, e.g. the mandatory client-id option is missing,
 > the server-id forbidden in that particular type of message is present,
 > there is more than one instance of client-id or server-id present,
 > etc. The exact reason for rejecting the packet is included in the
 message.
 > }}}
 Done.

 > '''src/bin/dhcp6/dhcp6_srv.h'''[[BR]]
 > renewIA_NA(): typo s/IA_NA response is generates with/an IA_NA response
 is generated with an/
 Fixed.

 > "Assigns leases" header.  Is the restriction that only IA_NA leases are
 supported permanent or temporary?  If the latter, please add a "@todo".
 Added @todo. I have not yet decided if there will be separate (similar)
 method for handling IA_PD or not. That is likely to be decided sometime in
 2013.

 > '''src/bin/dhcp6/dhcp6_srv.cc'''[[BR]]
 > assignLeases()/renewLeases() (comment for "if (!subnet)"): if the
 problem is a misconfiguration of something under the control of the server
 administrator, I would suggest that it be logged as an error instead of as
 a debug message.
 It is now logged as error. For the time being, we do support only a single
 subnet, because we do not have relay support. Also, we don't differentiate
 between interfaces, so there is no way to say that subnet[0] is accessible
 over eth0 and subnet[1] over eth1. I think that in many cases that will be
 more of a limitation of current code rather than misconfiguration. I don't
 plan to spend much time on doing any fancy error reporting as it will go
 away as soon as relay support is added.

 > Throwing an RFC violation - suggest that a debug message is logged
 before the "throw".
 Added.

 > renewIA_NA: if a client is trying to renew a lease we know nothing
 about, that might be worth a debug message being logged.
 Added DHCP6_UNKNOWN_RENEW with a substantial explanation. This may either
 indicate mild confusion of a single client that just resumed from sleep
 and is confused about time or may indicate a massive problem (that the
 server lost its database content). To not be too alarmistic, I've decided
 to log on debug level. If you think this should be on different level,
 please let me know.

 > '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''[[BR]]
 > !RenewBasic test: can "addr" be made "const"?
 Added.

 > !RenewBasic test: comment looks unfinished: "Check that T1, T2,
 preferred, valid really".
 Finished it.

 > !RenewBasic test: In the part that starts "Let's create a RENEW", it's
 not clear why the ASSERT_TRUE is present at that point.  As subnet_ is a
 test class member, and addr is not altered in the test, presumably addr
 was created as an address in the subnet.  The logical place to put the
 check is therefore immediately after the creation of addr.
 It's a copy-and-paste artifact. This assert should be checked, but not at
 this stage. Moved it earlier in the code, together with a comment.

 > !RenewBasic test: Please start one-line comments with a capital letter.
 Ok. There may be other tests that do not adhere to this rule, but I didn't
 want this ticket to be turned into large scale comment cleanup operation.

 > !RenewBasic test: "check that lease is really in the database".  That
 should be an ASSERT_TRUE check: if the lease is not in the database, the
 following EXPECT_EQ checks will attempt to dereference a null pointer.
 Fixed.

 > !RenewReject test: the comment "check that the lease is really in the
 database" is wrong - the check is that the lease is not in the database.
 Fixed.

 > !RenewReject test: see comment above about the ASSERT_TRUE check for the
 address.
 Fixed.

 > '''src/lib/dhcp/tests/option6_ia_unittest.cc'''[[BR]]
 > "simple" test: s/Option6IA * ia/Option6IA* ia/
 Fixed.

 > "simple" test: Should check that T1 and T2 are not the values set by
 setT1/setT2 before calling these methods. As it is, the test would pass if
 setT1/setT2 did nothing, but t1 and t2 were initialized to 2345 and 3456.
 Fixed.

 Fixed checked in. The ticket is back with you.

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