Opened 7 years ago

Closed 7 years ago

#2238 closed task (fixed)

Pool/Lease Configuration - IPv6

Reported by: stephen Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20121004
Component: ~dhcpconf(obsolete) Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

All the tasks in ticket #2237, but for IPv6.

Subtickets

Change History (11)

comment:1 Changed 7 years ago by tomek

  • Owner set to tomek
  • Status changed from new to accepted

comment:2 Changed 7 years ago by tomek

  • Owner changed from tomek to UnAssigned
  • Status changed from accepted to reviewing

This ticket is ready for a review. Things to consider:

  • this ticket covers configuration storage only. It is intended to store v6 server configuration. The actual configuration parser (that translates user's configuration into structures defined) is part of a separate ticket, see #2269.
  • Currently classes Pool, Pool6, Subnet, Subnet6 are stored in the same files as CfgMgr?. As they are part of the CfgMgr?, they may be possibly defined as internal classes (e.g. CfgMgr::Pool, rather than just a Pool). Alternatively, they could be moved to separate files. Finally, they can be left as they are now.
  • If would be nice if Pool and Subnet were abstract classes. Unfortunately, I couldn't find any method that would be required in both derived versions (Pool4 and Pool6) and take a parameter that is common for both derived classes.
  • I'm not sure if pointers to Pool and Subnet are really needed. I would like to hear reviewer's opinion on that.
  • Parameter inheritance is planned to be implemented as part of the configuration parser logic and the inheritance will be done at configuration phase. That means that Subnet6 objects will be instantiated with their "ultimate" values with inheritance already applied if necessary. That will make configuration phase slightly slower, but will make the actual operation faster.
  • Feel free to post any suggestions regarding Subnet4 and Pool4 here as well. Unless there's a reason to do otherwise, I plan to implement them using exactly the same approach.
Last edited 7 years ago by tomek (previous) (diff)

comment:3 Changed 7 years ago by tomek

  • Owner changed from UnAssigned to tomek
  • Status changed from reviewing to accepted

comment:4 Changed 7 years ago by tomek

I just discovered that this change does not build cleanly from scratch. There's a dependency problem in src/lib. I'm working on it now.

comment:5 Changed 7 years ago by tomek

  • Owner changed from tomek to UnAssigned
  • Status changed from accepted to reviewing

Ok, the circular dependency (asiolink->log->util->asiolink) is now fixed. Util no longer depends on asiolink. That code that required it is now in libdhcpsrv.

Please review.

comment:6 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:7 follow-ups: Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

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)

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

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

src/lib/asiolink/tests/io_address_unittest.cc
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
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.

Note that the code has "len / 8" at some points and "len/8" at others.

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.

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.

src/lib/dhcp/addr_utilities.h
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.

src/lib/dhcp/cfgmgr.cc
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.

Pool6 constructor (first one): typo in comment: "1something"

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,

src/lib/dhcp/cfgmgr.h
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".

Pool6: presume that the attributes are declared protected for testing. If so, there should be a comment to that effect.

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

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.

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

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.

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

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

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;

constructor_prefix_len test: Should also check for invalid prefix lengths.

src/lib/dhcp/tests/run_unittests.cc
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.

Response to 'Things to consider' in comment:2

this ticket covers configuration storage only. It is intended to store v6 server configuration. The actual configuration parser (that translates user's configuration into structures defined) is part of a separate ticket, see #2269.

One of the comments above suggests separate CfgMgr4 and CfgMgr6 classes.

Currently classes Pool, Pool6, Subnet, Subnet6 are stored in the same files as CfgMgr?. As they are part of the CfgMgr?, they may be possibly defined as internal classes (e.g. CfgMgr::Pool, rather than just a Pool). Alternatively, they could be moved to separate files. Finally, they can be left as they are now.

As noted above, I suggest moving them to separate files. The files are smaller to edit, and the name of the file clearly indicates the contents.

If would be nice if Pool and Subnet were abstract classes. Unfortunately, I couldn't find any method that would be required in both derived versions (Pool4 and Pool6) and take a parameter that is common for both derived classes.

The identification of the pool/subnet could be handled in a base class. Also, utility functions such as an address being in a given range, checking that "first < last", etc.

I'm not sure if pointers to Pool and Subnet are really needed. I would like to hear reviewer's opinion on that.

Leave the definitions in for now. If it turns out that they are not used, they can always be removed (or moved to the test files where they are used).

Parameter inheritance is planned to be implemented as part of the configuration parser logic and the inheritance will be done at configuration phase. That means that Subnet6 objects will be instantiated with their "ultimate" values with inheritance already applied if necessary. That will make configuration phase slightly slower, but will make the actual operation faster.

See comments above. The overhead of splitting the global/specific parameters when accessing the values is only a few instructions (probably just one "if" if the accessor functions are inline). Opposed to this is more complicated parser logic (and reconfiguration logic) if objects are instantiated with their "ultimate" values.

comment:8 in reply to: ↑ 7 Changed 7 years ago by tomek

Replying to 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
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
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
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
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
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
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
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.

comment:9 in reply to: ↑ 7 Changed 7 years ago by tomek

  • Owner changed from tomek to stephen

Replying to stephen:

Response to 'Things to consider' in comment:2

this ticket covers configuration storage only. It is intended to store v6 server configuration. The actual configuration parser (that translates user's configuration into structures defined) is part of a separate ticket, see #2269.

One of the comments above suggests separate CfgMgr4 and CfgMgr6 classes.

Let's keep them unified for now. If the code becomes too big, we will split them.

Currently classes Pool, Pool6, Subnet, Subnet6 are stored in the same files as CfgMgr?. As they are part of the CfgMgr?, they may be possibly defined as internal classes (e.g. CfgMgr::Pool, rather than just a Pool). Alternatively, they could be moved to separate files. Finally, they can be left as they are now.

As noted above, I suggest moving them to separate files. The files are smaller to edit, and the name of the file clearly indicates the contents.

Done. pool.{cc|h}, subnet.{cc|h} and triplet.h were created.

If would be nice if Pool and Subnet were abstract classes. Unfortunately, I couldn't find any method that would be required in both derived versions (Pool4 and Pool6) and take a parameter that is common for both derived classes.

The identification of the pool/subnet could be handled in a base class. Also, utility functions such as an address being in a given range, checking that "first < last", etc.

By identification you mean getID() method? It is already in the base class (Pool). I will probably move some methods to base class once I'll work on #2237 (v4 pool/subnet storage).

I'm not sure if pointers to Pool and Subnet are really needed. I would like to hear reviewer's opinion on that.

Leave the definitions in for now. If it turns out that they are not used, they can always be removed (or moved to the test files where they are used).

Ok.

Parameter inheritance is planned to be implemented as part of the configuration parser logic and the inheritance will be done at configuration phase. That means that Subnet6 objects will be instantiated with their "ultimate" values with inheritance already applied if necessary. That will make configuration phase slightly slower, but will make the actual operation faster.

See comments above. The overhead of splitting the global/specific parameters when accessing the values is only a few instructions (probably just one "if" if the accessor functions are inline). Opposed to this is more complicated parser logic (and reconfiguration logic) if objects are instantiated with their "ultimate" values.

Ok. I will see how this unfolds when I implement configuration parser. There's another complication here - the default values defined in spec files. I'm not sure how they affect the problem. I will give it more thought during #2269 work.

That hopefully answers all comments. Please review.

comment:10 Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

Reviewed commit 3690baaf0660af752b00a370fadcd60859c50ca4

All OK. The only comment I would make is that the subnet/pool/triplet tests be moved out of cfgmgr_unittest.cc into separate files. That makes it clearer where the unit tests reside by creating a correspondence between source file and test files; it also matches the current way the unit tests are organised.

comment:11 Changed 7 years ago by tomek

  • Resolution set to fixed
  • Status changed from reviewing to closed

Tests split into separate files, changes merged and pushed.

Thanks for the review.

Closing ticket.

Note: See TracTickets for help on using tickets.