Opened 7 years ago

Closed 7 years ago

#2414 closed task (complete)

Use AllocationEngine to actually handle v6 clients' messages

Reported by: tomek Owned by: stephen
Priority: medium Milestone: Sprint-DHCP-20121115
Component: dhcp6 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

#2324 introduced AllocationEgine?. This ticket uses it in the actual message processing.

With #2342 (MySQL backend), this ticket will make DHCPv6 assign leases.

Subtickets

Change History (13)

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

Ticket is ready for review. This ticket lacks update of Developer's Guide description, but that will be handled in a separate ticket.

I hope to have this ticket merged by Friday. Note that tests has been scheduled on trac2414 (scheduled it twice, the second run includes fix for bus error/segfault in RequestBasic? test).

comment:3 Changed 7 years ago by tomek

The following fixes were pushed:

  • {first,last}AddrInPrefix? now support invalid prefix lengths (segfault in Subnet6.constructor)
  • DHCP6_SUBNET_SELECTED log message is now fixed
  • compilation fix for Solaris in alloc_engine.cc (missing header)
  • checkLease now returns lease properly (crash in Dhcpv6SrvTest.RequestBasic?)

comment:4 Changed 7 years ago by stephen

  • Owner changed from UnAssigned to stephen

comment:5 follow-up: Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

src/bin/dhcp6/dhcp6_messages.mes
DHCP_DB_BACKEND_STARTED: Second sentence would probably be better expressed as "It indicates what database backend is being used to store lease and other information."

DHCP_LEASE_ALLOC(_FAIL): is it possible to distinguish between the advertised or allocated message - either by a different code or by means of another field in the message?

DHCP6_SUBNET*: the message should start with a lower-case character.

DHCP6_SUBNET_SELECTION_FAILED: suggest:
"This warning message is output when a packet was received from a subnet for which the DHCPv6 server has not been configured. The cause is most likely due to a misconfiguration of the server. The packet has been ignored."
(Note - Is the last sentence true?)

src/bin/dhcp6/dhcp6_srv.cc
Dhcp6Srv constructor: Not clear about the comment "used for testing purposes": that looks as if it should apply to the check of "countIfaces()", not the check of "port".

Dhcp6Srv constructor: "used for testing..." should be "Used for testing".

"if (port) {". As port is numeric, would prefer "if (port > 0) {".

The comments should make it clear that the test usage is with the port equal to zero.

The "todo" indicates that the lease manager should be implemented at the point of the "todo". But the code instantiates the memfile lease manage later on - where is the correct position?

run(): what is the timeout measured in?

setServerID(): comment: the logical place to store this information is the database.

setServerID(): comments should start with a capital letter.

setServerID(): "&srvid[0]+8" should be "&srvid[0] + 8".

setServerID(): call to fillrandom() should have a space after the comma.

copyDefaultOptions: use the "OptionPtr" typedef instead of the explicit type of boost::shared_ptr<Option>. (Incidentally, both addOption() methods expect the argument to be passed by value, not reference: this will incur the overhead of incrementing and decrementing the reference count of the pointed-to object.)

addRequestedOptions: use the "OptionPtr" typedef instead of the explicit type of boost::shared_ptr<Option>.

handleIA_NA: need more comments to explain what is happening.

assignLeases: need more comments to explain what is happening.

processXXX: a copy why "copyDefaultOptions" is immediately followed by "appendDefaultOptions" (which from their names, appear to add two sets of default options to the created packet) would be helpful.

src/bin/dhcp6/dhcp6_srv.h
Trivial point: start all "@brief" descriptions with a capital letter.

selectSubnet: need "@param" describing the "question" argument.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
Would appreciate some documentation about the methods of the Dhcp6SrvTest class (i.e. they should include a header, although perhaps not a Doxygen one.)

A number of the tests have identical function headers.

Solicit tests: is it worth checking that a sequence of solicit messages result in advertise responses with different addresses?

src/lib/dhcp/Makefile.am
OK. The memfile backend is probably better put in the main dhcp directory (with appropriate "TODO"s about not being finished.)

src/lib/dhcp/addr_utilities.cc
{first,last}AddrInPrefix4() already have a check for prefix length. (Although it should precede the assignment of the "addr" variable - which is redundant if the exception is thrown - and there should be a space around the ">" in lastAddrInPrefix4.) For this reason, I suggest the prefix lengths checks be removed from {first,last}AddrInPrefix(), and checks added to the V6 versions (remembering to put spaces around the ">" in the comparison).

(As an aside, the checks are more logical in the xxxPrefix{4,6}() functions: all the {first,last}AddrInPrefix() functions should be doing is routing to the protocol-specific versions based on address family.)

src/lib/dhcp/cfgmgr.cc
getSubnet6(): Spurious space "if ( (subnets6_.size()...": there should be no space between the two opening parentheses.

getSubnet6(): I think we've discussed this before, but what happens if the user has a configured subnet that does not match the configured single pool? The logic here is that the single subnet is always chosen. Also, suppose that there is a single V6 subnet that is not link local: it is rejected by the first check but could still be chosen in the subsequent iteration over the (single) subnet.

src/lib/dhcp/duid.cc
toText(): For consistency, either set the string stream's fill character, field width and output base with calls to "fill()", "width()", and "setf()" before the loop, or set them on a per-output call inside the loop with the manipulators hex, setfill and setw.

src/lib/dhcp/duid.h
It is not usual to write "operator==()" as "operator == ()" (same goes for the "not equals" operator.)

src/lib/dhcp/subnet.h
get(): it is probably clearer to use the "make_pair" call:

std::pair<isc::asiolink::IOAddress, uint8_t> get() const {
    return (std::make_pair(prefix_, prefix_len_));
}

(Also note that "get()" can be declared "const").

toText(): this can be declared "const".

src/lib/dhcp/tests/duid_unittest.cc
toText() test: there is no need to create a DUID via "new" or used a scoped pointer to store it: it could be created as an automatic variable:

DUID duid1(data1, sizeof(data1));
EXPECT_EQ("00:01:02:03:04:ff:fe", duid1.toText());

src/lib/dhcp/tests/lease_mgr_unittest.cc
OK. (Looks as if I need to complete all the xxx6() methods in the MySQL lease manager code for it to be useful.)

src/lib/dhcp/tests/subnet_unittest.cc
toText() test: there is no need to create a Subnet4 via "new" or used a scoped pointer to store it: it could be created as an automatic variable. (See comment for duid_unittest.cc.)

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

  • Owner changed from tomek to stephen

Replying to stephen:

src/bin/dhcp6/dhcp6_messages.mes
DHCP_DB_BACKEND_STARTED: Second sentence would probably be better expressed as "It indicates what database backend is being used to store lease and other information."

Done.

DHCP_LEASE_ALLOC(_FAIL): is it possible to distinguish between the advertised or allocated message - either by a different code or by means of another field in the message?

Done. There are 4 messages now: DHCP6_LEASE_ADVERT(_FAIL) and DHCP6_LEASE_ALLOC(_FAIL).

DHCP6_SUBNET*: the message should start with a lower-case character.

I thought the the message is supposed to be a full English sentence (starting with upper-case and ending with a dot). Anyway, did as you asked. There were couple more messages that started with upper-case, so I fixed them as well.

DHCP6_SUBNET_SELECTION_FAILED: suggest:
"This warning message is output when a packet was received from a subnet for which the DHCPv6 server has not been configured. The cause is most likely due to a misconfiguration of the server. The packet has been ignored."
(Note - Is the last sentence true?)

No. Server will include status-code option with code NoAddrsAvail? and continue processing the message. That behavior is enforced by RFC3315. It is possible (but unlikely) that the same message may contain other options that will be served (e.g. IA_PD or another IA_NA option).

On a side note, the getSubnet() method is very simple for now, but it will become much more complex once we start supporting relays. Also, I have an idea for very simple way of differentiating between interfaces (so the user will be able to specify that subnet A should be served over eth0 and subnet B over eth1).

src/bin/dhcp6/dhcp6_srv.cc
Dhcp6Srv constructor: Not clear about the comment "used for testing purposes": that looks as if it should apply to the check of "countIfaces()", not the check of "port".

Port 0 has special meaning - to not open sockets at all. Clarified the comment.

Dhcp6Srv constructor: "used for testing..." should be "Used for testing".

Fixed.

"if (port) {". As port is numeric, would prefer "if (port > 0) {".

Done.

The comments should make it clear that the test usage is with the port equal to zero.

Done.

The "todo" indicates that the lease manager should be implemented at the point of the "todo". But the code instantiates the memfile lease manage later on - where is the correct position?

Removed that old @todo.

run(): what is the timeout measured in?

I seconds. Expanded that comment.

setServerID(): comment: the logical place to store this information is the database.

In theory yes, but practice may suggest something else. A server is a server, regardless of the database it is using. Server-Id should not change even if you change backends. Storing the server duid in the backend will cause the server to change identity if you change backends. There's very specific text in RFC3315 that we will break with this behavior.

We could provide a way to kind of keep the data in sync, but that will get us into a completely new topic of data migration (and even worse, possible synchronization) between backends. So yes - I agree with you in principle, but that is not as simple as it looks.

setServerID(): comments should start with a capital letter.

Fixed.

setServerID(): "&srvid[0]+8" should be "&srvid[0] + 8".

Done.

setServerID(): call to fillrandom() should have a space after the comma.

Done.

copyDefaultOptions: use the "OptionPtr" typedef instead of the explicit type of boost::shared_ptr<Option>. (Incidentally, both addOption() methods expect the argument to be passed by value, not reference: this will incur the overhead of incrementing and decrementing the reference count of the pointed-to object.)

addRequestedOptions: use the "OptionPtr" typedef instead of the explicit type of boost::shared_ptr<Option>.

This code predates the OptionPtr? definition. Updated.

handleIA_NA: need more comments to explain what is happening.

Added more comments.

assignLeases: need more comments to explain what is happening.

Added more comments.

processXXX: a copy why "copyDefaultOptions" is immediately followed by "appendDefaultOptions" (which from their names, appear to add two sets of default options to the created packet) would be helpful.

The server's response contains many options. Some of them are copied from client request (copyDefaultOptions, e.g. client-id) and some are always inserted by the server (appendDefaultOptions, e.g. server-id or preference). In both cases, client does not have to request it in any way (on ORO), so in that sense they are default (options that must be sent by default). There are also other options that are sent back only if requested (appendRequestedOptions method).

That's the theory. In practice, the lines between them will be blurred, as Marcin implements mechanism for sending options even if they are not requested by clients. Once we add relay support, there will be other things like ERO (echo request option), where relay asks server to send a relay option back to the client. I think it should be rethinked, but not as part of this ticket.

src/bin/dhcp6/dhcp6_srv.h
Trivial point: start all "@brief" descriptions with a capital letter.

Done.

selectSubnet: need "@param" describing the "question" argument.

Done.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
Would appreciate some documentation about the methods of the Dhcp6SrvTest class (i.e. they should include a header, although perhaps not a Doxygen one.)

A number of the tests have identical function headers.

Most of them were similar (no hint sent, valid hint, invalid hint), but in one instance it was an identical copy. Fixed.

Solicit tests: is it worth checking that a sequence of solicit messages result in advertise responses with different addresses?

It makes perfect sense. It also makes sense to implement similar test for request. See new tests: ManySolicits? and ManyRequests?.

src/lib/dhcp/Makefile.am
OK. The memfile backend is probably better put in the main dhcp directory (with appropriate "TODO"s about not being finished.)

Done. Added a text to memfile_LeaseMgr constructor that warns about memfile being incomplete.

I have noted that there is some dependency between b10-dhcp++ and b10-dhcpsrv libraries. It fails to build occasionally with: make -j8 (I use 8 cores). After couple retries it finally builds. We should move libdhcpsrv to separate directory.

src/lib/dhcp/addr_utilities.cc
{first,last}AddrInPrefix4() already have a check for prefix length. (Although it should precede the assignment of the "addr" variable - which is redundant if the exception is thrown - and there should be a space around the ">" in lastAddrInPrefix4.) For this reason, I suggest the prefix lengths checks be removed from {first,last}AddrInPrefix(), and checks added to the V6 versions (remembering to put spaces around the ">" in the comparison).

Done.

src/lib/dhcp/cfgmgr.cc
getSubnet6(): Spurious space "if ( (subnets6_.size()...": there should be no space between the two opening parentheses.

Fixed.

getSubnet6(): I think we've discussed this before, but what happens if the user has a configured subnet that does not match the configured single pool? The logic here is that the single subnet is always chosen. Also, suppose that there is a single V6 subnet that is not link local: it is rejected by the first check but could still be chosen in the subsequent iteration over the (single) subnet.

The current code in this regard is very simplified. Currently we support only direct (not relayed) traffic, we don't differentiate between interfaces and we don't support shared subnets. That means that we always handle only a single subnet and the client's source address is always link-local (fe80::/10).


src/lib/dhcp/duid.cc
toText(): For consistency, either set the string stream's fill character, field width and output base with calls to "fill()", "width()", and "setf()" before the loop, or set them on a per-output call inside the loop with the manipulators hex, setfill and setw.

They cannot be set before the loop, as width() and setfill() operators affect only the next token, not the whole stream. Hex manipulator on the other hand, affects the whole stream, so it is now done before the loop (no need to repeat it).


src/lib/dhcp/duid.h
It is not usual to write "operator==()" as "operator == ()" (same goes for the "not equals" operator.)

Fixed.

src/lib/dhcp/subnet.h
get(): it is probably clearer to use the "make_pair" call:

std::pair<isc::asiolink::IOAddress, uint8_t> get() const {
    return (std::make_pair(prefix_, prefix_len_));
}

(Also note that "get()" can be declared "const").

Done.

toText(): this can be declared "const".

Done.

src/lib/dhcp/tests/duid_unittest.cc
toText() test: there is no need to create a DUID via "new" or used a scoped pointer to store it: it could be created as an automatic variable:

DUID duid1(data1, sizeof(data1));
EXPECT_EQ("00:01:02:03:04:ff:fe", duid1.toText());

Done.

src/lib/dhcp/tests/lease_mgr_unittest.cc
OK. (Looks as if I need to complete all the xxx6() methods in the MySQL lease manager code for it to be useful.)

I'm not sure about that. Originally memfile was planned as way to test LeaseMgr? interface, so those tests may continue to use memfile backend. IIRC you have developed dedicated tests to MySQL backend.

src/lib/dhcp/tests/subnet_unittest.cc
toText() test: there is no need to create a Subnet4 via "new" or used a scoped pointer to store it: it could be created as an automatic variable. (See comment for duid_unittest.cc.)

I've converted some of the tests. Tests don't have to be consistent. In fact, variety of approaches should be preferred to test them. You can consider some of the tests as implicit test for Subnet6Ptr type. If the shared_ptr fails to release the object, we will fail valgrind.

Changes pushed. The ticket is back with you. I will have some spare cycles during IETF, but I would very much prefer to do non-critical things, as there may be folks hunting for me for IETF things.

comment:7 Changed 7 years ago by stephen

Review commit 50c262d64042f0350cc887b1f51000294bc74b5c

src/bin/dhcp6/dhcp6_messages.mes
Corrected a typo and removed a redundant paragraph.

src/bin/dhcp6/tests/dhcp6_srv_unittest.cc
Corrected typos in a couple of comments.

src/lib/dhcp/addr_utilities.cc
Added space around operators, and put prefix length test as the first operation in the methods.

I think a ChangeLog entry of the form:

496.	[func]		tomek
	DHCPv6 Allocation Engine now used in message processing.
	(Trac #2414, git xxx)

... is appropriate.

comment:8 Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

comment:9 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

Reviewed commit f25544eeedb66a4f5bb0e0c41387e2e58555daa9.

Changes are ok. Please merge.

comment:10 Changed 7 years ago by marcin

Since there have been a couple of changes since f255... commit I am reviewing it since that commit up to 4067c8....

dhcp6_srv_unittest.cc
advertiseOptions: There are some comments needed in the new code that sets up the Solicit message. In particular: why do we have to set the link local address: fe80::abcd? What are the magic numbers 234, 1500, 3000?
Also extra space before

Pkt6Ptr sol = Pkt6Ptr(...);

dhcp6_srv.cc
Dhcp6Srv: Is there any reason for not initializing the Allocation Engine in the constructor's initialization list but in the constructor's body? Does it have to be initialized after other objects, i.e. logger, option defs etc?

dhcp6_srv.h
Dhcpv6Srv header: There is nothing in Dhcpv6Srv that makes it singleton class. Doxygen comment should be changed.

dhcp_srv_unittest.cc
Dhcp6SrvTest constructor: Technically the subnet_ and pool_ could be initialized with constructor's initialization list not in the body.

checkIAAddr: It would be good to comment why we compare:

EXPECT_EQ(expected_addr.toText(), addr->getAddress().toText());

instead of

EXPECT_EQ(expected_addr, addr->getAddress());

I presume this is because the == operator is not working properly for IOAddress because of the presence of uint32 operator?

checkIAAddr: the addr parameter could be passed by const reference.

comment:11 follow-up: Changed 7 years ago by stephen

  • Owner changed from stephen to marcin

dhcp6_srv_unittest.cc

advertiseOptions: There are some comments needed in the new code that sets up the Solicit message. In particular: why do we have to set the link local address: fe80::abcd? What are the magic numbers 234, 1500, 3000?

I don't think they are anything special: the fact that the server is started with a port of 0 means that no sockets are opened, so fe80::abcd is just an arbitrary address. And the others are just random numbers of the test. However, Tomek wrote the test, so he is best placed to give the reasons. Added #2474 to cover this.

Also extra space before
Pkt6Ptr sol = Pkt6Ptr(...);

Removed.

dhcp6_srv.cc

Dhcp6Srv: Is there any reason for not initializing the Allocation Engine in the constructor's initialization list but in the constructor's body?

Comments in the header file indicate that it should be a pointer with the intention that it can be changed at runtime. I've interpreted this to mean that at some point, a function call will be needed to extract allocation engine parameters, something that will have to be done in the constructor body. For that reason, I left the initialization there, rather than move it to the initialization list.

Does it have to be initialized after other objects, i.e. logger, option defs etc?

I think the order is not relevant.

dhcp6_srv.h

Dhcpv6Srv header: There is nothing in Dhcpv6Srv that makes it singleton class. Doxygen comment should be changed.

Comment updated.

dhcp_srv_unittest.cc

Dhcp6SrvTest constructor: Technically the subnet_ and pool_ could be initialized with constructor's initialization list not in the body.

Agreed, although as the initialization statements are a bit long, I think it would look a bit clumsy. I suggest we leave it for now.

checkIAAddr: It would be good to comment why we compare:
EXPECT_EQ(expected_addr.toText(), addr->getAddress().toText());
instead of
EXPECT_EQ(expected_addr, addr->getAddress());
I presume this is because the == operator is not working properly for IOAddress because of the presence of uint32 operator?

It is because IOAddress can't be streamed to an ostream using operator<<() (used by EXPECT_EQ to report an error). It is possible to use EXPECT_TRUE(address1 == address2), as OPAddress does support operator==(): however, this gives less information in the case of a failure. A comment explaining this has been added.

checkIAAddr: the addr parameter could be passed by const reference.

Changed.

comment:12 in reply to: ↑ 11 Changed 7 years ago by marcin

  • Owner changed from marcin to stephen

Replying to stephen:

dhcp6_srv.cc

Dhcp6Srv: Is there any reason for not initializing the Allocation Engine in the constructor's initialization list but in the constructor's body?

Comments in the header file indicate that it should be a pointer with the intention that it can be changed at runtime. I've interpreted this to mean that at some point, a function call will be needed to extract allocation engine parameters, something that will have to be done in the constructor body. For that reason, I left the initialization there, rather than move it to the initialization list.

That's fair enough. I think that comment would be useful but I leave it up to you.

dhcp_srv_unittest.cc

Dhcp6SrvTest constructor: Technically the subnet_ and pool_ could be initialized with constructor's initialization list not in the body.

Agreed, although as the initialization statements are a bit long, I think it would look a bit clumsy. I suggest we leave it for now.

Let's play the game, find 2 differences in the code sections below:

    Dhcpv6SrvTest()
        : rcode_(-1),
          subnet_(new Subnet6(IOAddress("2001:db8:1::"), 48, 1000,
                              2000, 3000, 4000)),
          pool_(new Pool6(Pool6::TYPE_IA, IOAddress("2001:db8:1:1::"), 64)) {

        subnet_->addPool6(pool_);

        CfgMgr::instance().addSubnet6(subnet_);
    }

vs

    Dhcpv6SrvTest() : rcode_(-1) {
        subnet_ = Subnet6Ptr(new Subnet6(IOAddress("2001:db8:1::"), 48, 1000,
                                         2000, 3000, 4000));
        pool_ = Pool6Ptr(new Pool6(Pool6::TYPE_IA, IOAddress("2001:db8:1:1::"), 64));
        subnet_->addPool6(pool_);

        CfgMgr::instance().addSubnet6(subnet_);
    }

Seriously speaking, I don't think it makes the constructor ''clumsy''. They look pretty similar.

comment:13 Changed 7 years ago by stephen

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

Merged to master in commit b3526430f02aa3dc3273612524d23137b8f1fe87

Note: See TracTickets for help on using tickets.