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

[BIND 10 Development]

#3234: Kea4/6: reconfigure increases subnet_id
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  defect        |                       Status:
            Priority:  high          |  reviewing
           Component:  dhcpconf      |                    Milestone:  DHCP-
            Keywords:                |  Kea1.0-alpha
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:  N/A
         Total Hours:  0             |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tomek


Comment:

 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!

 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.

 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.

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


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


 '''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?

 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.

 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.

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