BIND 10 #3234: Kea4/6: reconfigure increases subnet_id

[BIND 10 Development]

#3234: Kea4/6: reconfigure increases subnet_id
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:
                Type:  defect        |  marcin
            Priority:  high          |                       Status:
           Component:  dhcpconf      |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea1.0-alpha
         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 => marcin


Comment:

 Replying to [comment:6 marcin]:
 > Reviewed commit 09cfa792bffe8ffa9106c915334ef539577fb62d
 >
 > General comment. The static subnet_id is a nice trick to keep having
 globally unique IDs accross all Subnet objects. However, I foresee some
 problems with that approach when it comes to keeping the configuration
 consistent with the lease database!
 Agree. This is not an ultimate solution, just the remedy for the most
 obvious issue. Without this fix, every time anything was changed in the
 config, server kept forgetting every lease (because of mis-matched subnet-
 id).

 A follow up work is needed. I created a ticket #3281 for that.


 > I have in mind the case, where there are multiple subnets configured,
 each having distinct subnet id. When adding a new subnet, the
 configuration manager will put it on the very end of the list, and I
 presume it will be processed as the last getting the highest subnet id.
 Other Subnets will get their usual subnet ids. However, I am concerned
 that when you remove the subnet in the middle (say second subnet id out of
 3), the IDs for remaining two will be renumbered, such that the subnet id
 with the number of 3 will now get subnet id = 2. If there are any leases
 in the database for the subnet id 3, they will not match with the subnet
 they belong to.
 In general case, it will break if you remove any subnet that is not the
 last. We also need to cover another case - where you remove one subnet and
 add different one.

 This also brings in a question whether we want to get rid of all leases
 from the database when we remove the subnet configuration. There are pros
 and cons for keeping and removing it. We may also consider a feature for
 gentle removal of a subnet. The subnet is marked for deletion, but the
 server keeps it until the least active lease is expired or released.
 Server does not extend existing leases.

 > Nevertheless, this problem is a little more complex, because by removing
 all subnets from !CfgMgr on reconfiguration we actually loose all
 information about the matching between subnets held in !CfgMgr and in
 bind10 configuration. I don't see the easy solution for that, other than
 manually assign subnet id with the bindctl and check for invalid or
 duplicate IDs.
 True. I'm afraid that ultimately we'll have to implement some sort of
 logic that will do comparison between old subnets and new subnets and then
 deal with the leases that belong to subnets being removed.

 > '''!ChangeLog'''
 > Please consider adding a space between comma sign and !''b10-dhcp6!''
 like this:
 > {{{
 > b10-dhcp4, b10-dhcp6: ...
 > }}}
 Updadated.

 > '''src/bin/dhcp4/tests/config_parser_unittest.cc'''
 > '''src/bin/dhcp6/tests/config_parser_unittest.cc'''
 >
 > Apparently, there are no tests that check removal of the existing subnet
 or addition of the new subnet if there are existing subnets. In the case
 of removal, there is a problem of subnet id rennumbering which I described
 above.
 Added new test. It has 2 cases. The first one removes the last subnet. We
 can support that now and it passes. The second case (had 4 subnets,
 removing 2nd) is implemented, but currently fails. It is ifdefed with the
 @todo comment referencing #3281.

 > '''src/lib/dhcpsrv/subnet.h'''
 > Subnet: not strictly related to this ticket, but still...
 > {{{
 > /// ...... Pool4 and Pool6 should be used instead.
 > }}}
 >
 > The comment seems irrelevant to the class. Perhaps it should be Subnet4
 and Subnet6?
 Good catch.

 > Also, I was going to say that this constructor generates unique subnet
 id by itself and that it modifies the global value which affects id of all
 Subnet objects created in the future. That should be mentioned in this
 constructor or maybe it is even more important to put it in the Subnet4
 and Subnet6 constructor documentation.
 It is described in Subnet constructor, because this is where the subnet-id
 is assigned. Also
 added brief mention in Subnet4 and Subnet6 constructor docs.

 > getNextID: There should be much more text about the intended use of this
 function. In particular this function modifies the value of the subnet id,
 contrary to its name, which suggests that it returns the next available
 value rather than modify it. The caller should be urged in the
 documentation that the function modifies the subnet id value for all
 Subnet objects.
 Added extra text in Subnet constructor, in getNextID and renamed it to
 generateNextID. I think the new names is more appropriate as it is now
 obvious that the thing is generated, not merely returned.

 Also added a text in BIND10 Guide that explains the current
 reconfiguration limitations.

-- 
Ticket URL: <https://bind10.isc.org/ticket/3234#comment:7>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development