BIND 10 #2238: Pool/Lease Configuration - IPv6

[BIND 10 Development]

#2238: Pool/Lease Configuration - IPv6
-------------------------------------+-------------------------------------
                   Reporter:         |                 Owner:  tomek
  stephen                            |                Status:  reviewing
                       Type:  task   |             Milestone:  Sprint-
                   Priority:         |  DHCP-20121004
  medium                             |            Resolution:
                  Component:         |             Sensitive:  0
  dhcpconf                           |           Sub-Project:  DHCP
                   Keywords:         |  Estimated Difficulty:  0
            Defect Severity:  N/A    |           Total Hours:  0
Feature Depending on Ticket:         |
        Add Hours to Ticket:  0      |
                  Internal?:  0      |
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:7 stephen]:
 > Reviewed commit b8f538b9fcf70394e5737269d4bed660721c245d
 >
 > '''!ChangeLog'''
 > Suggest change to the entry:
 > {{{
 > 4XX.  [func]          tomek
 >       A new library (libb10-dhcpsrv) has been created. At present, it
 >       only holds the code for the DHCP Configuration Manager. Currently
 >       this object only supports basic configuration storage for the
 DHCPv6
 >       server, but that capability will be expanded.
 >       (Trac #2238, git TBD)
 > }}}
 Done.


 > '''src/lib/asiolink/io_address.h'''[[BR]]
 > smallerThan(): correct name, but "lessThan()" strikes me as a better
 one. (Many languages use "lt" or "le" for "<" or "<=" comparisons.)
 Done.

 > smallerThan(): OK, although a small restructuring of the code can reduce
 the number of comparisons in the (probably more common) case of address
 families being the same:
 > {{{
 > if (this->getFamily() == other.getFamily()) {
 >    if (this->getFamily() == AF_INET6) {
 >       return (<V6 expression>);
 >    } else {
 >       return (<V4 expression>);
 >    }
 > }
 > // Family equality has already been considered
 > return (this->getFamily() < other.getFamily());
 > }}}
 > (... and the code is slightly shorter.)
 Done.


 >
 > '''src/lib/asiolink/tests/io_address_unittest.cc'''[[BR]]
 > Would suggest that the new test be named "lessThanEqual" (or something
 like that): there is already test for addresses being equal, and "compare"
 implies that it includes an equality check.
 >
 > '''src/lib/dhcp/addr_utilities.cc'''[[BR]]
 > Some comments would be useful. I worked out that the logic is to copy
 the entire prefix, if the mask is not a multiple of 8 bits, use the number
 of bits modulo 8 as an index into the bitMask array to get the mask at
 that point, then zero out the rest of the array.  But it did take me a few
 minutes to work that out.
 That method is slightly convoluted, but it is very fast. I have commented
 both methods and fixed spaces around some operators.

 > Trivial point: when doing operations on the "packed" array, using
 "sizeof(packed)" as the maximum index  always seems clearer.  Also, I know
 that 16 is the length of a V6 address, but it is a "magic number" and a
 comment when "packed" is declared might be appropriate.
 I just used isc::asiolink::V6ADDRESS_LEN.

 > Would it be useful to name these functions to include the protocol
 family?  There might be a requirement in the future to work out the first
 and last addresses in a V4 address given in CIDR notation.
 I don't think it is necessary. I was planning on extending those functions
 to cover v4. Currently it will throw BadValue as to_v6() method will fail
 for v4 addresses. Added appropriate comment and todo. I don't have strong
 preference here, so if you still think it should be 2 separate functions
 one for each family, I'll do that.

 > '''src/lib/dhcp/addr_utilities.h'''[[BR]]
 > In the comment for firstAddrInPrefix, there is a typo: the first address
 for a prefix length of 120 in 2001:db8:1:dead:beef ends be00, not bee0.
 Fixed.

 > '''src/lib/dhcp/cfgmgr.cc'''[[BR]]
 > Since the address utilities compare two addresses of any family, it
 strikes me that the "first > last" test can be done just once in the
 "Pool" constructor, and removed from the Pool4/Pool6 constructors. (Tests
 on the address family etc. would have to stay in Pool4/6.) OTOH, having
 all the tests in one place does have merits.  I'll leave the decision up
 to you.
 Left as they are now. In case of prefix/len constructor, the last_ is
 initially set to :: and only later calculated, after family checks are
 made. I want to check family first and only if the family matches, then
 start other checks. BTW there may be other checks in the future (like
 forbidding crazy addresses like ::, starting with fe80 or ff, etc.). Added
 a comment about it.

 >
 > Pool6 constructor (first one): typo in comment: "1something"
 It's not a typo. I actually meant "1 followed by some hex digit". :-)
 Updated the comment.

 > Subnet::inRange(): see comments for cfgmgr.h.
 >
 > Subnet::inRange(): Not usual to have space after/before opening/closing
 parentheses (e.g. "((first <= addr) ..." is preferred to "( (first <=
 addr)".
 >
 > Subnet6 constructor: message associated with !BadValue exception could
 more accurately reflect the reason it was thrown, e.g. "non IPv6 prefix
 specified in subnet6".
 >
 > Subnet6::addPool6: as this is being done during configuration, run-time
 is not too important.  Suggest keeping the pools sorted by order of first
 address, and in getPool6(), using a binary_search to locate the relevant
 pool.
 >
 > CfgMgr::getSubnet6: not clear about the logic: if there is only one
 subnet, we use it regardless of the hint.  If there is more that one, we
 use the hint to locate it and if we can't find it,
 The idea is to keep small deployments easy. Imagine small home network -
 one router that also runs DHCPv6 server. Users specifies a single pool and
 expects it to just work. Unfortunately, it is not the case for ISC DHCP4.
 The server will complain that it doesn't have IP address on its interfaces
 that matches that configuration. Such requirement makes sense in IPv4, but
 not in IPv6. The server does not need to have a global address from the
 pool it serves. This optimization overcomes that problem. Added clarifying
 text that explains it.

 >
 >
 > '''src/lib/dhcp/cfgmgr.h'''[[BR]]
 > Typo: "specifes"
 > More common to use "operator=()" than "operator = ()". Similarly
 "operator T()" (if the template parsing allows it).
 > Space between operators in Triplet constructor ("min_>def")
 > Pool::getId() needs an "@return" in the header.
 > Can Pool::inRange() be declared "const"?
 > Typo "indentify".
 Fixed them all.

 > Pool6: presume that the attributes are declared protected for testing.
 If so, there should be a comment to that effect.
 Eee, no. Good point. Fixed that. It seems that we can use private access
 in BIND10 after all.
 >
 > Although all these classes are related to the configuration, I suggest
 separating out the "Pool" classes into a separate file on the grounds that
 it will be easier to find and edit them as the code grows.  Similarly for
 the Subnet classes.
 >
 > Can Subnet::inRange() be declared "const"?
 Fixed.

 > Subnet6 methods should be documented with Doxygen headers.
 >
 > !CfgMgr Header: referring to the example you give, an alternative idea
 for inheritance (and I'm not saying change your idea, I'm just raising the
 possibility of an alternative) is to implement global parameters in a
 separate singleton class, and subnet-specific parameters in the Subnet6/4
 class.  When a parameter is requested, the Subnet4/6 method calls the
 singleton-class method if it doesn't have the data.  The idea here is to
 simplify the
 configuration processing - global parameters go into the singleton class,
 subnet-specific parameters into instances of the Subnet4/6 classes.  So,
 for example, if a global parameter is changed, only the singleton class
 needs to be updated: there is no need to check all instances of the
 Subnet4/6 classes to see if they have values that need updating.
 There are couple problems with that approach. First, there's no such thing
 as "subnet4/6 doesn't have the data". Sure, such a thing could be
 implemented, but that would kind of spoil the whole idea. That is a
 performance feature. Configuration is done rarely, so it is ok for it to
 take a bit longer (i.e. sort out the inheritance stuff here). During
 normal operation, the path should be quick. Also, it will be more
 convenient to implement and debug.


 > !CfgMgr: as we are having separate processes for IPv4 and IPV6, does it
 make sense to have separate configuration managers for these processes (in
 which case, should the class be CfgMgr6)?
 I would prefer to keep them unified. I'm not sure how much commonality we
 will have, but it seems reasonable that at least some of them will be
 common, e.g. domain name, logging levels, perhaps hooks etc. We will
 separate them if we have to. Also, this follows the approach in iface_mgr
 that is a single one for both families.

 > subnets6_: regarding the comment on this attributes, a simple vector is
 fine.  However, if we alter inRange() to return -1/0/1 depending whether
 an address is below the start of range, in the range, or above the range
 (or create another comparison function that does it) and keep the subnets
 vector sorted by order of start address, we could use the STL
 binary_search algorithm when locating a subnet.
 Ok, but that is big enough to have a separate ticket. Created ticket
 #2280.

 > '''src/lib/dhcp/tests/addr_utilities_unittest.cc'''
 > I think that limit cases (e.g. "::/1", "::/128") should also have tests.
 Done.

 > 280     0       src/lib/dhcp/tests/cfgmgr_unittest.cc
 > "min", "max" and "value" can be declared as "const uint32_t".
 Done/

 > If you've used symbolic names for the limits in Triplet, suggest that
 you use the same symbols for the expected results in the appropriate part
 of the tests (e.g. expected value for x.getMin() is "min").
 >
 > operator test - these tests doesn't actually check operator=() - they
 check the constructor.  The construct:
 > {{{
 > Triplet<uint32_t> y = x
 > }}}
 > ... is another way of writing
 > {{{
 > Triplet<uint32_t> y(x);
 > }}}
 > To test the operator=() function, you need something like:
 > {{{
 > Triplet<uint32_t> y(0);
 > y = x;
 > }}}
 Good catch.

 > constructor_prefix_len test: Should also check for invalid prefix
 lengths.
 It checks it now.

 > '''src/lib/dhcp/tests/run_unittests.cc'''[[BR]]
 > Do you really need to remove logger support?  Although libdhcp++ will
 not including logging (as it is intended to be used by external programs),
 libb10-dhcpsrv doesn't have that restriction as it is for used by the
 server images.  At some point we may want to add logging to it.
 Ok. Reverted it.

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