BIND 10 #3232: Kea6: Develop REBIND support

[BIND 10 Development]

#3232: Kea6: Develop REBIND support
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  medium        |                       Status:
           Component:  Unclassified  |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea0.9-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  51            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  1.5
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * hours:  5.5 => 1.5
 * owner:  tomek => marcin
 * totalhours:  49.5 => 51


Comment:

 Replying to [comment:8 marcin]:
 > > '''src/bin/dhcp6/tests/dhcp6_client.cc'''
 > > Dhcp6Client::applyConfiguration() the condition in line 75
 > Yes. This function became a bit ugly as a result of my attempt to not
 copy-paste too much for each processed IA. I restructured this function a
 bit and it now looks nicer.
 The new code looks good. I would prefer the other option types (default
 label) to throw exception rather than silently ignore, but I won't insist
 on it.

 > > When creating the lease later in the same function, please add a brief
 > > comment that the lease is assumed to belong to a subnet with subnet-id
 > > 0. That's a reasonable assumption for now, but it may fail once we
 > > start testing fancier scenarios with that class.
 > Sorry, I didn't get that point.
 In line 92 you pass 0 as the last argument. That value is subnet-id. 0 is
 correct value if there is only one subnet configured.
 It may be invalid if there are more subnets (so it could potentially be 1
 instead of 0).

 That's ok for now. It was just a comment.

 > > The code should eventually have a clause that removes leases if
 > > received with lifetimes set to 0. It's probably not needed for this
 > > ticket, so a simple @todo will be sufficient for now.
 > From the unit test standpoint it is good that leases with zero-lifetimes
 are not immediately removed because the unit test want to check that
 lifetimes are 0.
 Good point.

 > Perhaps there will need to be a public function that can be explicitly
 called to remove leases of this kind. This class will need to be extended
 anyway, so I am not sure we need todo for evernything?
 Nope. I hadn't thought about test reasons to keep the leases with
 lifetimes 0.

 > > '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''
 > > testUnicast: It's a good test, but it has a flaw. The unicast is
 > > allowed only if the server is configured to send server-unicast
 > > option. If it's not configured, it should be reject/status-code with
 > > UseMulticast for every client originated packet.
 >
 > Well, when talking about the Rebind message it must always be discarded
 if sent to a unicast and not relayed, I think:
 >
 > {{{
 >    A server MUST discard any Solicit, Confirm, Rebind or
 >    Information-request messages it receives with a unicast destination
 >    address.
 > }}}
 >
 > This is from section 15 of RFC3315.
 Uh oh. I was sure it was UseMulticast here as well. I stand corrected.

 > > REBIND_CONFIGS structure: Can you add a one-line comment for each
 > > configuration? It's a bit difficult to compare 2 configurations that
 > > are 2 screens away and pick up the subtle differences. I was thinking
 > > about something like
 > > {{{
 > > // Configuration 0: 2 subnets, address only (2001:db8:1/64,
 2001:db8:2::/64)
 > > // Configuration 1: same as 0, but different subnets
 ((2001:db8:4::/64, 2001:db8:4::/64)
 > > // Configuration 2: ...
 > > }}}
 >
 > Added. Also removed Config4 which appeared to be unused.
 Thanks.

 > > RebindTest::configure(): if you agree with my proposal for extending
 > > client constructor to accept Dhcpv6Srv pointer, you could get rid of
 > > this method and use Dhcpv6SrvTest::configure() method directly.
 > > Which is not on this branch yet, because it was branched before
 > > configure() was merged to master. So it makes my comment somewhat
 > > useless, unless you want to do master->trac3232 merge, then update
 > > then trac3232->master merge.
 > I don't want to merge that back and forth. I will remove it once I have
 a chance in another ticket.
 Sure.


 > > General comment for all tests in rebind_unittest.cc:
 > > Please add one line explanation for each test purpose.
 > Ooops. Why did I forget about it? Added.
 Thanks.

 > It is not supporting the stateful issues yet. The key change in stateful
 issues is that server may allocate leases upon Renew or Rebind, which we
 don't support here. I decided to write this up in stateful issues before
 implementing it.
 Oh well. I was hoping we could extend our list of supported RFCs and
 drafts a bit...

 You may consider adding an exception for other option types (the default
 label discussed above). No matter what your decision will be, the code
 looks ready to go.  Please merge.

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