Opened 4 years ago

Closed 4 years ago

#4497 closed defect (complete)

Provide API for hook libraries to manipulate options without affecting server configuration

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Kea1.1
Component: hooks 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: 3
Total Hours: 50 Internal?: no

Description

Hooks libraries can freely modify the contents of the DHCP messages. This includes manipulating options. Kea servers don't copy options from the configuration into the packets, but it rather copies pointers. This means that any modification to an option within a packet affects the option within configuration. We have been doing it to gain some performance but from the hooks standpoint is not acceptable.

Different approaches have been discussed here: https://lists.isc.org/pipermail/kea-dev/2016-April/000647.html

and this thread is a good start point. This may also require some mini-design.

Subtickets

Change History (11)

comment:1 Changed 4 years ago by hschempf

  • Milestone changed from Kea-proposed to Kea1.1

per team meeting 5/5/, accept 1.1. No estimate.

comment:2 Changed 4 years ago by marcin

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

comment:3 follow-up: Changed 4 years ago by marcin

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

Several approaches have been discussed prior to implementing this ticket:

1) explicit option checkout/checkin within hooks libraries by calling special functions
This approach was rejected on the grounds that it relies on the hook implementors to remember to checkout an option prior to modifying it. Almost certainly some of the implementors would neglect to do it and possibly report issues that modification of an option caused weird behavior of the server. A better approach to copy by default and provide a way to disable copying when the hook library implementor desires that, because it ensures that the hook library implementor knows what he is doing.

2) Copy-on-write options using shared pointers,
This was a very interesting idea brought by Francis. I even implemented a little PoC but it ended up being too complex and too intrusive change. Implementing an CoW with shared pointers would require creating a new class which implements almost all functions and operators that the boost::shared_ptr implements to guarantee backward compatibility of the new class with the current use of boost::shared_ptr<Option>. This also includes overriding boost cast operators for the new class. Finally, we also extensively use boost::shared_ptr<Option> rather than OptionPtr?, which implies that we'd need to update many places in the current code.

The most important argument against it was that Option instances contain other Option instances (option encapsulation) and implementing CoW on the Option level gets weak when you already deal with a copy of entire option (retrieved from a packet) and you want to retrieve sub-option. In this case copying is not desired (because sub-option has been already copied) but CoW makes it hard to avoid a copy of sub-option.

All in all, I don't think that implementing CoW is not possible but it would be too heavy and too risky change. Having some more control over copying, i.e. what is copied and when is probably better and less error prone.

3) Derive class from Pkt4 and Pkt6 which would encapsulate an instance of original Pkt4 or Pkt6 object and always copy options on retrieval.
This was proposed by Stephen. A derived class would relay calls to a Pkt4/Pkt6 class and for some calls (like getOption) it would make a copy of an option. At the same time, the derived class would be convertible to Pkt4 and Pkt6. This was a fine idea, but the problem I found with it was that the derived class would need to implement all functions of the Pkt4/Pkt6 class. This seemed like too much overhead and the major problem is that if you update Pkt4/Pkt6 by adding new method you have to remember to also implement this method in the derived class. I think it is better to continue using Pkt4 and Pkt6 classes but provide them with some internal flag that controls whether options are copied or not.

4) Always copy options.
This was proposed by Thomas... I thought it was cool, but what if majority of hook libraries don't change anything in options within a packet? This would have serious performance implications. In fact, you wouldn't even have to have a hook library to hit the performance problem. Any call to getOption by the Kea server would trigger a copy.

What I finally implemented is this.....

A Pkt4/Pkt6 class has an internal boolean flag which indicates if a retrieved option should be copied or not. By default options are not copied which ensures that the server still operates on original instances, rather than copies. This is important from the performance standpoint.

The flag is turned ON for a packet at each hook point where an instance of the packet is passed to a hook library. This means that the hook library calling Pkt4::getOption, Pkt6::getOption, Pkt6::getOptions, Pkt6::getRelayOption or Pkt6::getAnyRelayOption will (by default) get a copy of an option. The copied option replaces the original option in the packet. As a result, if the callout modifies the option it doesn't need to make any additional calls to commit the change or anything. From the standpoint of existing hook libraries this is a transparent change.

The libraries that require good performance and when the library doesn't retrieve options to modify them (but to read them only, for example) the hook library can disable copying options by explicitly calling a function on an instance of Pkt4/Pkt6.

To ensure that option copying is disabled when the callout returns control to the server I used a simple RAII object which keeps the flag ON on the packet only within a scope of a hook point, and automatically sets it to OFF when the hook point gets to an end.

This approach (and any other approach also would) required implementing deep copying options of various types. In fact, for most of the classes it was sufficient to rely on default copy constructors and assignment operators. There were some options where custom copy constructors and assignment operators were needed.

Finally, this change required changes in many files: each class derived from Option class (to implement deep copying), changes to Pkt4/Pkt6 classes, updates to the unit tests for hooks, new tests that check deep copy of options, updates to some files to fix constness of functions etc.

Thus, the diff is around 5k lines, but most of the changes are straightforward, like adding "const" to a declaration of a function or repeated additions to the Option-derived classes. The essential changes are really not that big.

Proposed ChangeLog entry:

11XX.	[func]		marcin
	Pkt4 and Pkt6 objects passed to hooks libraries return copies
	of DHCP options to avoid unintended modification of the
	options stored in the server configuration.
	(Trac #4497, git abcd)

comment:4 Changed 4 years ago by marcin

  • Add Hours to Ticket changed from 0 to 43
  • Total Hours changed from 0 to 43

comment:5 in reply to: ↑ 3 Changed 4 years ago by fdupont

Replying to marcin:

What I finally implemented is this.....

A Pkt4/Pkt6 class has an internal boolean flag which indicates if a retrieved option should be copied or not. By default options are not copied which ensures that the server still operates on original instances, rather than copies. This is important from the performance standpoint.

=> this is copy-on-reference, a variant of copy-on-write used by some OS kernels. So in theory it should work...

comment:6 Changed 4 years ago by fdupont

  • Owner changed from UnAssigned to fdupont

comment:7 Changed 4 years ago by fdupont

Fixed a spelling error so please pull.

Why the buffer6_send callout at the end of Dhcpv6Srv::run_one() doesn't set the copy flag to the response?

Same question about lease6_release callout in Dhcpv6Srv::releaseIA_PD()

It seems the trac4497 base lacks the last DHCPv4-over-DHCPv6 code which includes some hooks too.

To continue.

comment:8 Changed 4 years ago by fdupont

  • Owner changed from fdupont to marcin
  • Total Hours changed from 43 to 47

Pkt4o6 is not handled: I propose to make setCopyRetrievedOptions virtual so it can be redefined for Pk4to6 to set the flag to both the Pkt4 part (super) and the Pkt6 part

or (simpler) wait for a call to getPkt6() to copy the flag to pkt6_ (private member)?

Of course there should be some unit tests for that...

comment:9 follow-up: Changed 4 years ago by marcin

  • Add Hours to Ticket changed from 43 to 3
  • Owner changed from marcin to fdupont
  • Total Hours changed from 47 to 50

Because the trac4497 didn't include DHCPv4o6 stuff, I created new branch trac4497_rebase and rebased the code against latest master. Please switch to this new trac4497_rebase branch for further review.

I made the setCopyRetrievedOptions virtual and redefined it in the Pkt4o6 class. I think it is safer, because it guarantees that the flag is always correctly set for the Pkt6.

I updated the unit tests for DHCPv4o6 IPC to record within the hooks if the flag was correctly set when the respective callouts where invoked.

I added new unit test for the lease6_release hook PD case and then I am setting the flag for this hook point. Also fixed the buffer6_send case.

comment:10 in reply to: ↑ 9 Changed 4 years ago by fdupont

  • Owner changed from fdupont to marcin

Replying to marcin:

Because the trac4497 didn't include DHCPv4o6 stuff, I created new branch trac4497_rebase and rebased the code against latest master. Please switch to this new trac4497_rebase branch for further review.

=> I added a REBASED file in trac4497 to make the rebase very clear.

I made the setCopyRetrievedOptions virtual and redefined it in the Pkt4o6 class. I think it is safer, because it guarantees that the flag is always correctly set for the Pkt6.

=> why:
pkt6_->setCopyRetrievedOptions(isCopyRetrievedOptions());
vs
pkt6_->setCopyRetrievedOptions(bool);
If there is no reason to call isCopyRetrievedOptions the second is far simpler and efficient.

I updated the unit tests for DHCPv4o6 IPC to record within the hooks if the flag was correctly set when the respective callouts where invoked.

I added new unit test for the lease6_release hook PD case and then I am setting the flag for this hook point. Also fixed the buffer6_send case.

=> it should be fine now. Please merge after a pull for 2 spelling errors.

comment:11 Changed 4 years ago by marcin

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

Merged with commit e50d2ebe998ec3faad8ade22b6971d6584c81044

Also fixed inefficient flag setting.

Note: See TracTickets for help on using tickets.