Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFCs 6334, 6603, 7598] Additional native option support #24

Closed
wants to merge 22 commits into from

Conversation

Projects
None yet
3 participants
@andreipavelQ
Copy link
Contributor

andreipavelQ commented Jun 17, 2016

Hello,

I work for Qualitance and I've been working on implementing RFCs 6334, 6603, 7598 and refactored the option engine to natively support option definitions previously specified in kea.conf.

I've also added support for recursive encapsulations of options. I've stumbled upon a problem with encapsulated options created with default global option spaces ( "dhcp4", "dhcp6" ). It seems like Option::getEncapsulatedSpace()) is used to reference the option's own space and this causes it to try to encapsulate all global options. This was not an issue before because there was only one level of encapsulation. I've provided a temporary workaround in CfgOption::encapsulateOptions(), but the actual fix should be a separation between option space and encapsulated space.

@andreipavelQ

This comment has been minimized.

Copy link
Contributor Author

andreipavelQ commented Jun 17, 2016

Commited e9e3c1d with updated licenses and authors.

@tomaszmrugalski

This comment has been minimized.

Copy link
Collaborator

tomaszmrugalski commented Jul 4, 2016

Hi Andrei! Thanks for PR. Apologies for not getting back to this earlier, but our schedule for upcoming Kea 1.1 is very tight and we have only limited time available for any unplanned features. Having said that, I'd love to see this patch included in 1.1.

As I said, I have not reviewed it extensively, but looking at the code I have 3 comments so far:

  1. The license. The new files you added are licensed under Apache license. Would you reconsider releasing them under MPL? We recently accepted Cassandra backend, but it's a different situation. Cassandra is an optional code that is not compiled by default. If a user wants to use Kea under MPL, he can simply choose to not compile Cassandra. With lw4o6 and pd-exclude he wouldn't have such luxury and would have to accept dual MPL+Apache license. This may be a problem for some users. Having said that, I'm not a lawyer. I will need to discuss this with my management whether this would be a problem or not.
  2. There are no unit-tests. This is a showshopper, am afraid. We can't accept non-trivial patches without unit-tests. At least some unit-tests are needed: testing that Option6PDExclude can be built and parsed, that the encapsulated options are working, that lw4o6 options are working. Ideally, this should cover a test that allows lw4o6 configuration (so we're sure the parser is able to handle the lw4o6 configuration).
  3. We need documentation. 6334 is trivial, so simply updating table 8.1 in the User's Guide will do the trick. For 6603 it's slightly more complicated, so a short section will be useful. For 7598 it's definitely a complex topic, so it requires a section with examples. A sample config in doc/examples/kea6 would also be very useful.

As for the separation of option-space and encapsulated-space, I need to talk with Marcin and will get back to you.

@tomaszmrugalski

This comment has been minimized.

Copy link
Collaborator

tomaszmrugalski commented Jul 14, 2016

Thanks for the update. I just downloaded the code, compiled it and tried to load an example config file with custom options. It does not work on this code, but works on master. I used ea-cfg3-options-custom.json, available here: https://github.com/sysrepo/sysrepo-plugin-kea/blob/master/kea-configs/kea-cfg3-options-custom.json.

Sadly, I'm preparing for upcoming hackathon and IETF meeting, so will not be able to debug the issue anytime soon :(

@msiodelski msiodelski self-assigned this Jul 26, 2016

@msiodelski

This comment has been minimized.

Copy link
Collaborator

msiodelski commented Jul 26, 2016

Just to let everyone know... I am currently working on this pull request. There seem to be three changes: 1) replacing "dhcpX" with DHCPX_OPTION_SPACE, 2) Implementation of the Exclude Prefix option, 3) Significant rework of the option spaces etc.

I am porting those changes separately into internal branch in that order 1), 2), 3). The 1) and 2) seem to be straight forward and I don't see why we shouldn't apply them, except that they still require a lot of work, i.e. lack of doxygen comments or invalid comments, lack of unit tests or insufficient unit tests, some of the virtual methods not implemented (and required) for Option6PDExclude etc. I am addressing most of those issues for 1) and 2) on my own, because it is often easier to fix them than to point them out.

The 3) change is a significant rework of libdhcp++. To some extent it is a revolution. I realize that there are some things with respect to encapsulated options that may not work correctly but I am also not sure that the selected approach isn't going to far. Once I address 1) and 2) I will make careful review of that approach and we may have some further discussions about it.

For the time being, please be informed that this pull request is being worked on and refrain from additional updates and reviews.

@andreipavelQ

This comment has been minimized.

Copy link
Contributor Author

andreipavelQ commented Jul 27, 2016

Please consider applying the following patch. Explanations are in the comment.
options.patch

@andreipavelQ

This comment has been minimized.

Copy link
Contributor Author

andreipavelQ commented Jul 27, 2016

Updated the mentioned patch.

@msiodelski

This comment has been minimized.

Copy link
Collaborator

msiodelski commented Sep 29, 2016

Folks,
I have created a new branch 'github24' in the isc-projects/kea github repository. This branch includes your proposed changes, which I later significantly modified. First of all: I added a bunch of unit tests (required for any new code). As I said before, I wasn't terribly pleased with the amount of changes affecting libdhcp++. I realize that you guys wanted to make it more generic, but this should not really be a part of this effort. I reduced the number of changes affecting libdhcp++ to the reasonable minimum. I also modified the code changes pertaining to Prefix Exclude stuff in the DHCPv6 server. Rather than process ORO option multiple times, it is better to record the fact that the client has requested the Prefix Exclude option when we're processing ORO option for the first time. The same pertains to the pool search (which is quite expensive operation). The Allocation Engine records the pool during prefix allocation so it doesn't need to be looked up again to place the Prefix Exclude Option.

There is one outstanding issue with the Prefix Exclude processing, i.e. it doesn't quite work with the host reservations, because host reservations can be created outside of any pool. We'll need a separate effort/ticket to cover that.

The proposed documentation seems to have many todos (is incomplete). The patch that you have sent includes some changes to the documentation that don't apply. So, it seems that there may be a gap between the HEAD of your repo and the patch, which introduced the documentation I can't see. Can you please send me a diff for doc/guide/* files based on the latest Kea code (taken from master of isc-projects/kea repo)?

Finally, I have a couple of comments regarding the scale of your changes. I spent a lot of time going through this code to review it and fix it. Partly, the problem was that you had combined (at least) 3 different features in a single pull request. I had to decouple those changes, and sometimes there is no clear way to decouple them, because two features can affect a single file. If I like the feature A, but not B, I have to manually remove/modify the parts of the file that pertain to feature B. This is really tedious work. It would be much more efficient if this effort was split into 3 or 4 pull requests. In the future, please try to make sure the code changes are small enough that you'd not feel the pain yourself trying to review it. ;-)

But, many thanks for your efforts and I am looking forward to your response regarding the documentation.

Marcin

@msiodelski

This comment has been minimized.

Copy link
Collaborator

msiodelski commented Sep 29, 2016

I forgot to also ask you to system test the code on 'github24' branch to make sure that it still does what it should after my changes.

@msiodelski

This comment has been minimized.

Copy link
Collaborator

msiodelski commented Oct 14, 2016

As there was no followup on this, I have created documentation and also resolved some bugs in the code. The code is now waiting for the review from one of the engineers at ISC. The code is available on branch 'github24' of our Kea repo.

@msiodelski

This comment has been minimized.

Copy link
Collaborator

msiodelski commented Nov 4, 2016

Guys,
Once again thanks for submitting the patch. Your changes and my further changes have now been merged to the master branch. Closing the pull request. Please give it a try.
Marcin

@msiodelski msiodelski closed this Nov 4, 2016

isc-projects pushed a commit that referenced this pull request Nov 4, 2016

[5016] Merged pull request #24 from github24 and squashed it.
This change includes the code implemented by the pull request
submitter as well as Marcin's fixes/changes on top of it, but
without a review.
@andreipavelQ

This comment has been minimized.

Copy link
Contributor Author

andreipavelQ commented Dec 15, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.