Opened 7 years ago

Closed 7 years ago

#2984 closed enhancement (fixed)

Add remaining (15+) hooks into the DHCPv6 Server

Reported by: stephen Owned by: tomek
Priority: medium Milestone: Sprint-DHCP-20130821
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 (last modified by tomek)

There is a pre-work for this ticket: couple selected hooks (#2995).

Add the appropriate hook points to the BIND 10 DHCP6 server.

This ticket also includes the task of updating the relevant section of the DHCP Hooks design document with the actual API to be used (together with examples). Examples were never part of the estimates.

Subtickets

Change History (8)

comment:1 Changed 7 years ago by tomek

  • Description modified (diff)
  • Summary changed from Add Hooks into the DHCP6 Server to Add remaining (15+) hooks into the DHCPv6 Server

comment:2 Changed 7 years ago by tomek

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

comment:3 Changed 7 years ago by tomek

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

The code is ready for review.

  • Four extra hook points were implemented: buffer6_receive, lease6_renew, lease6_release, buffer6_send.
  • skip flag in pkt6_send changed meaning: it now means skip packing.
  • Pkt6::data_ is now public field and getData() method was removed (it is now safer and easier to use)

Note that dhcp6_srv_unittest.cc has been split into several files. In particular hooks-related tests are in a hooks_unittest.cc. I'm somewhat uneasy with the content of the new dhcp6_test_utils.h header. It is almost exact copy of the class that used to reside in dhcp6_srv_unittest.cc. It has too much code for a header, but I did not want to split it further into cc+h without getting extra opinion. Please advise if it is ok to keep that much code in header or not.

This is not as full featured set of hooks as we initially planned, but it does cover all the features we have now. I consider this a reasonable compromise between our plans and time restrictions.

One notable missing callout is lease6_expire, but we can't implement it until we have housekeeping routines (see #2345). I have created ticket #3066 for that task.

Please review.

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

Revieweed commit d165e1c0d34e34a24b4519106af41af74d92b197

I've made modifications to a couple of files - please "pull" before continuing.

General
Reviewing this code (and working on the generalised "getCalloutHandle()" in #3062) suggests that there is a weakness the way the stored CalloutHandle is accessed.

getCalloutHandle() uses the address of the packet to identify the CalloutHandle. The server passes a shared pointer to the packet to the hooks callout. If the callout modifies the packet in-situ, there is no problem. However, if the callout creates a new packet and updates the shared pointer, a new CalloutHandle will be obtained the next time one is needed. This is only an issue if callouts are using per-packet context but, if they are, loss of context could be surprising.

Given the current "one packet at a time" nature of the server, it might be better to modify getCalloutHandle() not to check the packet address. Instead, add an explicit clear of the CalloutHandle to the head of the "run()" loop and have getCalloutHandle() return the same handle regardless of packet passed to it. (If you agree, I'll update #3062.)

src/bin/dhcp6/dhcp6_hooks.dox
I've updated some of the text.

It occurred to me that if you wanted to drop a packet where you could determine the fact in buffer6_receive, it would be most efficient to use ycallouts on two hooks: the buffer6_receive would detect the information, set a context flag in the CalloutHandle, set the skip flag and return. The pkt6_receive callout would check the context flag and set the skip flag based on it. That way you could drop the packet without the overhead of parsing it. This would prove a useful real-world example and is perhaps one we should include somewhere.

src/bin/dhcp6/dhcp6_messages.mes
Minor corrections made to this file.

src/bin/dhcp6/dhcp6_srv.cc
Comment for the "buffer6_receive" callout: The comment here would be better if it explained in what context the hook is called, rather than just listing it's name, e.g.

The packet has just been received so contains the uninterpreted wire
data; execute callouts registered for buffer6_receive.

and, at the packet6_receive callout, something like.

At this point the information in the packet has been unpacked into
the various packet field; execte callouts registered for packet6_receive.

This applies to other hooks points as well.

Comment for the "buffer6_receive" callout (the comment prior to the call to "getSkip()"): The comment is wrong: the next processing step after receiving the packet is unpacking it, and it is this step that is skipped if the flag is set.

Would like a comment such as "Unpack the packet information unless the buffer6_receive callouts indicated they did it" before the "if (!skip_unpack)" line - it's easy to miss this significant step in the packet processing.

In the "switch" for query type, the comment in the "default" clause is now incorrect - there is no debug statement before the "switch". I suggest that a LOG_DEBUG statement be put there to output the unrecognised packet type.

"specifies if server should do the packing" - please start comments with a capital letter.

Comment is incorrect in the "packet6_send" hook code. If the callouts set the skip flag, it tells the server to skip the packing step, not to drop the packet.

renewIA_NA (packet6_send hook code): The next processing step is to update the lease, so "skip" at this stage does not mean "drop response". (Methinks there has been a fair bit of "copy and paste" with the hooks code ;-) ).

releaseIA_NA: same command as above regarding comment when the "skip" flag is set.

Comment in error: "did the removal as successful"

src/bin/dhcp6/tests/dhcp6_test_utils.h

I'm somewhat uneasy with the content of the new dhcp6_test_utils.h header. It is almost exact copy of the class that used to reside in dhcp6_srv_unittest.cc. It has too much code for a header, but I did not want to split it further into cc+h without getting extra opinion. Please advise if it is ok to keep that much code in header or not.

Create a ticket for this and leave it for now.

I agree that there is a lot of code in the header and perhaps should be separated into a .cc file, but rather than deal with an individual case I think we should look at a bit for rationalisation. For example, the code handling the fake packet sending in NakedDhcpv6Srv is more or less identical (apart from the packet type) to that in the DHCP4 NakedDhcpv6Srv class. Although it is unrealistic to combine the Naked* classes, the fake packet processing could be abstracted into a template class shared between the two tests. Some of the tests I wrote for the hooks code in #2981 are duplicated in the two servers. In the long term, we should probably create a DHCP server test directory into which we put common tests.

src/bin/dhcp6/tests/hooks_unittest.cc
"Hooks" test - could perhaps line up all the "= -1" under one another.

HooksDhcpv6SrvTest::createOption: as payload is numeric, a data type of uint8_t would be semantically more correct.

HooksDhcpv6SrvTest::buffer6_receive_change_client: spaces needed around the ">". Also, what is special about "pkt->data_[8]"?

Please start the first line of each of the callbackk headers with a capital letter.

HooksDhcpv6SrvTest: if NakedDhcpv6Srv was declared as a scoped_ptr, its deletion in the destructor would not be needed.

simple_pkt6_receive test: Please start every comment with a capital letter.

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

  • Owner changed from tomek to stephen

Replying to stephen:

Revieweed commit d165e1c0d34e34a24b4519106af41af74d92b197

I've made modifications to a couple of files - please "pull" before continuing.

General
Reviewing this code (and working on the generalised "getCalloutHandle()" in #3062) suggests that there is a weakness the way the stored CalloutHandle is accessed.

getCalloutHandle() uses the address of the packet to identify the CalloutHandle. The server passes a shared pointer to the packet to the hooks callout. If the callout modifies the packet in-situ, there is no problem. However, if the callout creates a new packet and updates the shared pointer, a new CalloutHandle will be obtained the next time one is needed. This is only an issue if callouts are using per-packet context but, if they are, loss of context could be surprising.

Given the current "one packet at a time" nature of the server, it might be better to modify getCalloutHandle() not to check the packet address. Instead, add an explicit clear of the CalloutHandle to the head of the "run()" loop and have getCalloutHandle() return the same handle regardless of packet passed to it. (If you agree, I'll update #3062.)

Agree. I haven't thought about it. I see that #3062 is ready for review. I will review it soon.

src/bin/dhcp6/dhcp6_hooks.dox
I've updated some of the text.

It occurred to me that if you wanted to drop a packet where you could determine the fact in buffer6_receive, it would be most efficient to use ycallouts on two hooks: the buffer6_receive would detect the information, set a context flag in the CalloutHandle, set the skip flag and return. The pkt6_receive callout would check the context flag and set the skip flag based on it. That way you could drop the packet without the overhead of parsing it. This would prove a useful real-world example and is perhaps one we should include somewhere.

It is possible use case, but rather infrequently used. I think that the most common place to implement access control would be pkt6_receive as the library can easily access client-id, server-id and other options. We had similar discussion before - the most useful example would be something related to access control, e.g. check if received client-id is on white/black-list and then continue processing or drop it. Easy to implement, yet sufficiently complex to showcase many hook features.

src/bin/dhcp6/dhcp6_messages.mes
Minor corrections made to this file.

Thank you.

src/bin/dhcp6/dhcp6_srv.cc
Comment for the "buffer6_receive" callout: The comment here would be better if it explained in what context the hook is called, rather than just listing it's name, e.g.

The packet has just been received so contains the uninterpreted wire
data; execute callouts registered for buffer6_receive.

and, at the packet6_receive callout, something like.

At this point the information in the packet has been unpacked into
the various packet field; execte callouts registered for packet6_receive.

This applies to other hooks points as well.

Done.

Comment for the "buffer6_receive" callout (the comment prior to the call to "getSkip()"): The comment is wrong: the next processing step after receiving the packet is unpacking it, and it is this step that is skipped if the flag is set.

Fixed.

Would like a comment such as "Unpack the packet information unless the buffer6_receive callouts indicated they did it" before the "if (!skip_unpack)" line - it's easy to miss this significant step in the packet processing.

Added.

In the "switch" for query type, the comment in the "default" clause is now incorrect - there is no debug statement before the "switch". I suggest that a LOG_DEBUG statement be put there to output the unrecognised packet type.

Added.

"specifies if server should do the packing" - please start comments with a capital letter.

Fixed that and many other comments.

Comment is incorrect in the "packet6_send" hook code. If the callouts set the skip flag, it tells the server to skip the packing step, not to drop the packet.

Fixed.

renewIA_NA (packet6_send hook code): The next processing step is to update the lease, so "skip" at this stage does not mean "drop response". (Methinks there has been a fair bit of "copy and paste" with the hooks code ;-) ).

There is. I should be more careful when copying the code.

releaseIA_NA: same command as above regarding comment when the "skip" flag is set.

Fixed.

Comment in error: "did the removal as successful"

Fixed.

src/bin/dhcp6/tests/dhcp6_test_utils.h

I'm somewhat uneasy with the content of the new dhcp6_test_utils.h header. It is almost exact copy of the class that used to reside in dhcp6_srv_unittest.cc. It has too much code for a header, but I did not want to split it further into cc+h without getting extra opinion. Please advise if it is ok to keep that much code in header or not.

Create a ticket for this and leave it for now.

Created ticket #3085.

src/bin/dhcp6/tests/hooks_unittest.cc
"Hooks" test - could perhaps line up all the "= -1" under one another.

They are now aligned.

HooksDhcpv6SrvTest::createOption: as payload is numeric, a data type of uint8_t would be semantically more correct.

Fixed.

HooksDhcpv6SrvTest::buffer6_receive_change_client: spaces needed around the ">". Also, what is special about "pkt->data_[8]"?

It's the first byte of the first option. We need to modify something and the first byte of a first option is a good choice. The code now uses constants and the purpose of that modification is explained.

Please start the first line of each of the callbackk headers with a capital letter.

Updated.

HooksDhcpv6SrvTest: if NakedDhcpv6Srv was declared as a scoped_ptr, its deletion in the destructor would not be needed.

Done.

simple_pkt6_receive test: Please start every comment with a capital letter.

Done.

Ok, the ticket is back with you. Thanks for the review.

comment:7 Changed 7 years ago by stephen

  • Owner changed from stephen to tomek

reviewed commit 540dd0449121094a56f294c500c2ed811f6016b6

Fixed a couple of typos (so please pull before proceeding). All OK, please merge.

comment:8 Changed 7 years ago by tomek

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

Pulled, merged, pushed. Closing ticket.

Note: See TracTickets for help on using tickets.