Opened 7 years ago

Closed 5 years ago

#2949 closed enhancement (complete)

DHCPv6: implement InformationRequest message support.

Reported by: marcin Owned by: tomek
Priority: low Milestone: Kea0.9.1beta
Component: dhcp6 Version: git
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

Currently, Kea has a stub implementation for INORMATION_REQUEST messages. It creates empty Reply messages (with no options). The logic to process InformationRequest? messages should be added.

Subtickets

Change History (21)

comment:1 Changed 6 years ago by tomek

  • Milestone changed from DHCP Outstanding Tasks to DHCP-Kea-proposed

comment:2 Changed 6 years ago by tomek

  • Milestone changed from DHCP-Kea-proposed to DHCP-Kea0.9-alpha
  • Priority changed from medium to low

MOving to 0.9-alpha as low priority (as discussed at 2014-02-05).

comment:3 Changed 6 years ago by stephen

  • Milestone changed from DHCP-Kea0.9-alpha to DHCP-Kea0.9-beta

comment:4 Changed 6 years ago by tomek

  • Milestone changed from DHCP-Kea0.9 to DHCP-Kea-proposed

comment:5 Changed 6 years ago by tomek

  • Priority changed from low to medium

comment:6 Changed 6 years ago by tomek

  • Milestone changed from Kea-proposed to Kea0.9
  • Priority changed from medium to low

comment:7 Changed 5 years ago by tomek

  • Priority changed from low to medium

comment:8 Changed 5 years ago by tomek

  • Milestone changed from Kea0.9 to Kea-proposed

Swapping #3390 with #2949 in Kea 0.9.

comment:9 Changed 5 years ago by tomek

As discussed on Kea meeting (2014-07-16), moving to 1.0.

comment:10 Changed 5 years ago by tomek

  • Milestone changed from Kea-proposed to Kea1.0

comment:11 Changed 5 years ago by tomek

  • Milestone changed from Kea1.0 to Kea0.9.1
  • Version set to git

comment:12 Changed 5 years ago by tomek

We support DHCPINFORM in v4, should support it in v6 to keep feature parity.

comment:13 Changed 5 years ago by tomek

I thought it may be useful if a bit of extra info is provided.

The information-request processing is described in RFC3315. It's very simple essentially. Server needs to assign any options requested by a client in ORO. One thing worth noting is that client is not required to send client-id (they usually do, though).

There's already a boilerplate method: Dhcpv6Srv::processInfRequest() in src/bin/dhcp6/dhcp6_srv.cc. You may take a look at Dhcpv6Srv::processRequest(), which does similar things, just omit unnecessary calls to anything dealing with leases and fqdn (neither is allowed in inf-request).

As for unit-tests, you may want to take a look at Dhcpv6SrvTest.advertiseOptions in dhcp6_srv_unittest.cc. It does similar checks for solicit/advertise.

One tricky part would be to provide options if there are no subnets defined (Kea allows definition of global options that apply to all subnets). In principle, it should be possible to just define options and not care about any subnets at all. Marcin recently updated configuration storage and he specifically did some improvements to cover that scenario. You may want to ask him about the details. In any case, it is fine to push this particular piece into separate ticket.

comment:14 Changed 5 years ago by hschempf

  • Priority changed from medium to low

comment:15 Changed 5 years ago by tomek

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

comment:16 Changed 5 years ago by tomek

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

Implemented support for inf-request. The code is now ready for review.

Please note the disparity between actual code and unit-tests (roughly 10 times more tests than production code).

Proposed ChangeLog?:

8xx.	[func]		tomek
	Information-Request (stateless mode) in DHCPv6 is now supported.
	(Trac #2949, git tbd)
Last edited 5 years ago by tomek (previous) (diff)

comment:17 Changed 5 years ago by stephen

  • Owner changed from UnAssigned to stephen

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

  • Owner changed from stephen to tomek

Reviewed commit 2632fbb0c54b49f9a21035a63ac7b7fd07965c71

doc/examples/kea6/stateless.json
In the first line: s/DNS servers information/the DNS server's information/

doc/guide/dhcp6-srv.xml
I've done some editing of the text. Please pull and review.

src/bin/dhcp6/dhcp6_srv.cc
processInfRequest: Add some comments please. Just looking at the code, I'm not sure why we both copy and append default options while only appending requested options.

src/bin/dhcp6/tests/dhcp6_client.cc
doInfRequest: the comments appear to suggest that the method is for testing and takes actions that are against the specification (a suspicion reinforced by the use of the magic numbers 1234 and 5670. Please clarify.

src/bin/dhcp6/tests/dhcp6_client.h
Copyright date needs updating to 2015.

sendClientId/sendORO: The methods only flag the client ID/ORO for sending, it does not actually send it (which is what the method names imply). How about calling them something like "setClientIdSend" or "enableSendClientId" (and similar).

doInfRequest: the header comments are incomplete.

sendOro: the single parameter ("send") is not defined in the header comments.

The list of options requested (oro_) appears to be a public member. It should be a private member with a getter/setter method.

The definitions of the send_oro_ and send_client_id_ member variables should be Doxygen commented.

src/bin/dhcp6/tests/dhcp6_test_utils.cc
Copyright date needs updating to 2015.

compareOptions: As memcmp is used, "string.h" should be #included.

Is compareOptions something that could be usefully included in dhcpsrv?

src/bin/dhcp6/tests/dhcp6_test_utils.h
Copyright date needs updating to 2015.

src/bin/dhcp6/tests/infrequest_unittest.cc
Trivial point: having the closing brace of the sample configurations on a separate line and at the same indent as the opening brace would make them easier to read.

ChangeLog
This entry looks OK.

comment:19 in reply to: ↑ 18 Changed 5 years ago by tomek

  • Owner changed from tomek to stephen

Replying to stephen:

Reviewed commit 2632fbb0c54b49f9a21035a63ac7b7fd07965c71

doc/examples/kea6/stateless.json
In the first line: s/DNS servers information/the DNS server's information/

This is information about 2 DNS servers, so the plural form is correct. Updated the text. Hopefully it is more correct now.

doc/guide/dhcp6-srv.xml
I've done some editing of the text. Please pull and review.

Thanks. Your changes are ok.

src/bin/dhcp6/dhcp6_srv.cc
processInfRequest: Add some comments please. Just looking at the code, I'm not sure why we both copy and append default options while only appending requested options.

Added. There are several other methods that lack such comments. If you feel it is a productive use of engineering time, please submit a ticket to address that issue.

src/bin/dhcp6/tests/dhcp6_client.cc
doInfRequest: the comments appear to suggest that the method is for testing and takes actions that are against the specification (a suspicion reinforced by the use of the magic numbers 1234 and 5670. Please clarify.

Yes. Added more comments. As the directory suggests, this is testing code. Hopefully, it will one day be used to also test negative scenarios, like client sending options not allowed in inf-request.

src/bin/dhcp6/tests/dhcp6_client.h
sendClientId/sendORO: The methods only flag the client ID/ORO for sending, it does not actually send it (which is what the method names imply). How about calling them something like "setClientIdSend" or "enableSendClientId" (and similar).

Your proposal is too long. I changed it to useClientId() and useORO(), which are both short and meaningful.

doInfRequest: the header comments are incomplete.

Oops. Corrected.

sendOro: the single parameter ("send") is not defined in the header comments.

It is now.

The list of options requested (oro_) appears to be a public member. It should be a private member with a getter/setter method.

I must say no. This patch is already 799 lines long, only 90 of those changes are actual code changes. This means that there's almost 9 times more testing code than the production code. Don't you think it's a getting a bit out of proportion? This is not an exercise in C++ correctness.

If this argument does not convince you, here's another. Having the oro vector exposed is more useful, as tests can tweak the content easier. You can argue that the getter could return const reference, but then I would counter-argue that the reference may be used beyond the live of the object that returned it. We could continue that discussion, but no useful thing would come out of it.

The definitions of the send_oro_ and send_client_id_ member variables should be Doxygen commented.

They are documented now.

src/bin/dhcp6/tests/dhcp6_test_utils.cc
Copyright date needs updating to 2015.

compareOptions: As memcmp is used, "string.h" should be #included.

Added missing include.

Is compareOptions something that could be usefully included in dhcpsrv?

In theory yes, in practice no. This is a convenient method, but it is very inefficent. That's ok for testing, but not for production code. If you can point to a specific use case that would benefit from having this available outside of src/bin/dhcp6, I'll move it to src/lib/testutils/.

src/bin/dhcp6/tests/dhcp6_test_utils.h
Copyright date needs updating to 2015.

src/bin/dhcp6/tests/infrequest_unittest.cc
Trivial point: having the closing brace of the sample configurations on a separate line and at the same indent as the opening brace would make them easier to read.

I just followed the convenition used in sarr_unittest.cc, dhcp6_srv_unittest.cc, rebind_unittest.cc, confirm_unittest.cc, hooks_unittest.cc, ctrl_dhcp6_srv_unittest.cc, d2_unittest.cc, config_parser_unitest.cc, kea_controller_unittest.cc and 7 other files in src/bin/dhcp4/tests.

Thanks for your review.

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

  • Owner changed from stephen to tomek

Reviewed commit 4e5bf865e14bc51cbdb090f2c11cca24bf647cb4

src/bin/dhcp6/dhcp6_srv.cc
processInfRequest: Add some comments please. Just looking at the code, I'm not sure why we both copy and append default options while only appending requested options.

Added. There are several other methods that lack such comments. If you feel it is a productive use of engineering time, please submit a ticket to address that issue.

I think you misunderstood me. Rephrasing the question, we are copying the default options to the reply packet, and then appending them. Does that mean we have two copies of the default options in that packet and, if so, why? (In other words, are the "default options" the same in both cases? And if not, why not?)

The list of options requested (oro_) appears to be a public member. It should be a private member with a getter/setter method.

I must say no. This patch is already 799 lines long, only 90 of those changes are actual code changes. This means that there's almost 9 times more testing code than the production code. Don't you think it's a getting a bit out of proportion? This is not an exercise in C++ correctness.

The cost of fixing a bug once the code has been released is far more than the cost of getting the code right the first time. If a patch requires 10x the number of lines in test code to get it properly tested, so be it. It's still cheaper than the cost to us of receiving a bug report, reproducing and tracking down the bug, creating a patch and (possibly) sending out an updated release out of the normal cycle.

As to C++ correctness, the issue is more one of good practice. In general, accessing the internals of objects directly makes it more difficult to maintain that code - it can become expensive to change something because it has implications all over the program. This is in contrast to a class providing a defined interface - providing the interface is maintained, you can change the internals as much as you want.

In this particular case, I won't push the issue as it is test code, but there is a reason for C++ correctness.

src/bin/dhcp6/tests/infrequest_unittest.cc
Trivial point: having the closing brace of the sample configurations on a separate line and at the same indent as the opening brace would make them easier to read.

I just followed the convenition used in sarr_unittest.cc, dhcp6_srv_unittest.cc, rebind_unittest.cc, confirm_unittest.cc, hooks_unittest.cc, ctrl_dhcp6_srv_unittest.cc, d2_unittest.cc, config_parser_unitest.cc, kea_controller_unittest.cc and 7 other files in src/bin/dhcp4/tests.

Sigh! Leave it for now then. I would point out that the current usage is inconsistent as within the JSON, if a clause is split across multiple lines, the trailing brace is on a new line.

You can merge now, but I think the comments in dhcp_srv.cc should be updated first to address the point I raised above. I don't need to see this ticket again.

comment:21 in reply to: ↑ 20 Changed 5 years ago by tomek

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

Replying to stephen:

Reviewed commit 4e5bf865e14bc51cbdb090f2c11cca24bf647cb4

src/bin/dhcp6/dhcp6_srv.cc
processInfRequest: Add some comments please. Just looking at the code, I'm not sure why we both copy and append default options while only appending requested options.

Added. There are several other methods that lack such comments. If you feel it is a productive use of engineering time, please submit a ticket to address that issue.

I think you misunderstood me. Rephrasing the question, we are copying the default options to the reply packet, and then appending them. Does that mean we have two copies of the default options in that packet and, if so, why? (In other words, are the "default options" the same in both cases? And if not, why not?)

Ok, I get it now. Yes, "default" meant different things in those contexts. I renamed copyDefaultOptions to copyClientOptions. This should do the trick.

Also, we can consider renaming appendDefaultOptions to appendServerOptions. Let me know if that's something you'd like to see. This ticket is about to be merged, but I can easily do it in other tickets that touch this area.

The list of options requested (oro_) appears to be a public member. It should be a private member with a getter/setter method.

I must say no. This patch is already 799 lines long, only 90 of those changes are actual code changes. This means that there's almost 9 times more testing code than the production code. Don't you think it's a getting a bit out of proportion? This is not an exercise in C++ correctness.

The cost of fixing a bug once the code has been released is far more than the cost of getting the code right the first time. If a patch requires 10x the number of lines in test code to get it properly tested, so be it. It's still cheaper than the cost to us of receiving a bug report, reproducing and tracking down the bug, creating a patch and (possibly) sending out an updated release out of the normal cycle.

As to C++ correctness, the issue is more one of good practice. In general, accessing the internals of objects directly makes it more difficult to maintain that code - it can become expensive to change something because it has implications all over the program. This is in contrast to a class providing a defined interface - providing the interface is maintained, you can change the internals as much as you want.

In this particular case, I won't push the issue as it is test code, but there is a reason for C++ correctness.

We have a difference on opinions here. It's essentially a judgement call how far we want to go with the unit-tests vs. return of investment of that effort. There's no "right" or "wrong" answer here.

Anyway, oro_ vector is now private. Since it is not used directly (tests call requestOption()), there was no need to implement classic getters/setters yet.

src/bin/dhcp6/tests/infrequest_unittest.cc
Trivial point: having the closing brace of the sample configurations on a separate line and at the same indent as the opening brace would make them easier to read.

I just followed the convenition used in sarr_unittest.cc, dhcp6_srv_unittest.cc, rebind_unittest.cc, confirm_unittest.cc, hooks_unittest.cc, ctrl_dhcp6_srv_unittest.cc, d2_unittest.cc, config_parser_unitest.cc, kea_controller_unittest.cc and 7 other files in src/bin/dhcp4/tests.

Sigh! Leave it for now then. I would point out that the current usage is inconsistent as within the JSON, if a clause is split across multiple lines, the trailing brace is on a new line.

Some people prefer more compact notation. I'm not sure if the representation of JSON notation is codified anywhere, and if it is, whether we're bound by it. In any case, I'm keeping the code as it is now for consistency reasons. We may create a ticket for this, but I would think that it would be very low priority.

You can merge now, but I think the comments in dhcp_srv.cc should be updated first to address the point I raised above. I don't need to see this ticket again.

Thanks. Merging now.

Note: See TracTickets for help on using tickets.