BIND 10 #3194: Vendor options support is needed in Kea

[BIND 10 Development]

#3194: Vendor options support is needed in Kea
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  enhancement   |                       Status:
            Priority:  high          |  reviewing
           Component:  dhcp          |                    Milestone:
            Keywords:                |  Sprint-DHCP-20131016
           Sensitive:  0             |                   Resolution:
         Sub-Project:  DHCP          |                 CVSS Scoring:
Estimated Difficulty:  0             |              Defect Severity:
         Total Hours:  0             |  Medium
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------

Comment (by tomek):

 Replying to [comment:1 marcin]:
 > The commits on branch trac3194 had wrong descritpions (starting with the
 wrong branch name). Therefore I moved all the commits from the trac3194 to
 the new branch trac3194_1 and I made just a single commit out of them. I
 reviewed this commit on the trac3194_1 branch:
 >
 > 0764a74e461d71c8d3dcb25dc2399a4d18d553f8
 I think this was a bit unnecessary move. But since you did it anyway, it
 is fine.

 > Then, I made some changes on the branch which I am hoping you'll review.
 I fill. I just first plan to address your comments about my code.

 > Please note that there is one unit test is now failing because I am
 expecting ORO to be returned as OptionUint8Array, not Option. This however
 will not work until we merge the master to this branch. I haven't merged
 it yet to avoid messing this up further.
 I've added couple more dhcpv4 vendor tests. Two of them are failing as
 well.

 > '''src/bin/dhcp4/config_parser.cc'''
 > '''src/bin/dhcp6/config_parser.cc'''
 > findServerSpaceOptionDefinition: There is not much sense to call
 SubnetConfigParser::findServerSpaceOptionDefinition if option space is
 "dhcp4". Therefore, the function could look like this:
 > {{{
 >     virtual OptionDefinitionPtr findServerSpaceOptionDefinition (
 >                 std::string& option_space, uint32_t option_code) {
 >         OptionDefinitionPtr def;
 >         if (option_space == "dhcp4" &&
 >             LibDHCP::isStandardOption(Option::V4, option_code)) {
 >             def = LibDHCP::getOptionDef(Option::V4, option_code);
 >         } else if (option_space == "dhcp6") {
 >             isc_throw(DhcpConfigError, "'dhcp6' option space name is
 reserved"
 >                      << " for DHCPv6 server");
 >         } else {
 >             // Check if this is a vendor-option. If it is, get vendor-
 specific
 >             // definition.
 >             uint32_t vendor_id =
 SubnetConfigParser::optionSpaceToVendorId(option_space);
 >             if (vendor_id) {
 >                 def = LibDHCP::getVendorOptionDef(Option::V4, vendor_id,
 option_code);
 >             }
 >         }
 >         return (def);
 >     }
 >
 > }}}
 Updated as suggested.

 > It seems to me that there are no tests for parsing vendor options for
 DHCPv4.
 There are now. Two of them are failing due to invalid vendor suboption 1
 type.

 > '''src/bin/dhcp4/dhcp4_srv.cc'''
 > There is nothing to do here.
 Excellent!

 > I did some modifications to the appendRequestedVendorOptions to use
 OptionUint8Array instead of generic option and some other trivial changes.
 > Also, I removed the bits that parsed vendor options from the callback
 function (unpackOptions) because this logic had been moved to option
 definition.
 Thanks.

 > '''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''
 > The Dhcpv4Srv::vendorOptionsDocsis is a good and simple test. But, we
 should probably consider extending it to test multiple options, not just
 one. It could also be extended to cover attempts to get options from non-
 existent option spaces etc.
 > I wonder if vendor options tests shouldn't be made a separate class of
 tests (perhaps in a separate file) as I presume there will be many
 different tests that we may create here.
 >
 > For the sake of this ticket though, todo is sufficient.
 Added @todo for it.


 > '''src/bin/dhcp6/dhcp6_srv.cc'''
 > A really minor thing: Please consider swapping the following code:
 > {{{
 > uint32_t vendor_id = vendor_req->getVendorId();
 > }}}
 >
 > with the check if option ORO is present. It is likely that the ORO is
 absent and in such case you don't need to initialize vendor_id. But, since
 it is a tiny performace penalty I leave it up to you.
 Done.

 > Also, I removed the bits of code in the callback function
 (unpackOptions) which are responsible for parsing a vendor option as this
 logic had been moved to OptionDefinition.
 Thanks.


 > '''src/bin/dhcp6/tests/config_parser_unittest.cc'''
 > Do you think it makes sense to do similar tests for options defined with
 a subnet, rather than global only?
 Yes, but not now. Added @todo for it.

 > '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''
 > Dhcpv6SrvTest::docsisVendorOptionsParse: It would be a great help if you
 used option names instead of raw codes: 1, 36, 35...
 It would be, I agree. The problem is that we have codes defined for only a
 small number of options. I just defined the absolute minimum that we need
 to send back.

 Added new ticket for all that work: #3208.

 > Dhcpv6SrvTest::docsisVendorORO: A comment !''Check if the packet
 contain!'' seems to be missing the second part (what does it contain?).
 Also, it should be !''contains!''.
 Updated.


 > Please consider using ASSERT_NO_THROW instead of EXPECT_NO_THROW in this
 statement:
 > {{{
 > EXPECT_NO_THROW(sol->unpack());
 > }}}
 > because further test doesn't make sense if unpack fails.
 Done.

 > '''src/lib/dhcp/docsis3_option_defs.h'''
 > Error in last line: STD_OPTION_DEFS_H. It should be
 DOSCIS3_OPTION_DEFS_H
 Fixed.

 > '''src/lib/dhcp/libdhcp++.cc'''
 > Can initStdOptionDefs4 use initOptionSpace just like other functions of
 this kind?
 Sure.

 >
 > unpackVendorOptions6 and unpackVendorOptions4: The comment near:
 > {{{
 > if (idx) {
 > }}}
 > is wrong. You're checking if index exists, not if there is an option
 definition. Also, I am not sure if there is any need to make index a
 pointer. Index is always present - this is guaranteed by the declaration
 of the multi index container.
 Actually, it is correct. If there is no such vendor space, then
 option_defs is NULL. We can't obtain index on non-existing container, so
 the index may be NULL. And that happens only when there is no definition
 for such a vendor option. Hence the comment.

 > '''src/lib/dhcp/libdhcp++.h'''
 > All new functions and class members deserve doxygen comments.
 Defined.

 > getVendorOptionXDefs: Why do you have to return a pointer, instead of a
 reference?
 Because there may be no definitions for a given vendor. Returning NULL is
 a way to say "no such definitions".


 > '''src/lib/dhcp/option_vendor.cc'''
 > pack: The format of the data fields following enterprise id is different
 for V4 and V6 (if I understand that correctly). When calling packOptions()
 for V4 you don't take into account the data-len field in V4 VIVSO option.
 The same applies for len(). I may be wrong....
 I'm afraid you are correct and the code is buggy. I hope I fixed it now.

 But it is embarrassing that it was not picked up by unit-tests.
 [30 seconds later] There are no unit-tests for OptionVendor...

 > unpack: Please consider including the length of the provided buffer in
 the exception string.
 Done.

 > '''src/lib/dhcp/option_vendor.h'''
 > I would extend the comment for the class to indicate that the same class
 represents an option for V4 and V6 and that the format of the option is a
 bit different because the data-len field is present in V4.
 >
 > Constructor: it is not true that you throw InvalidDataType exception.
 The same applies for pack() and unpack().
 Fixed.

 > Please correct the descriptions for setVendorId and getVendorId.
 Fixed.

 > The last line of the header is invalid.
 Fixed.

 > '''src/lib/dhcpsrv/dhcp_parsers.cc'''
 > Unrecognized comment !''thomson!'' in dhcp_parsers.cc (line 1055).
 Removed.

 > optionSpaceToVendorId:
 > Why enterprise numbers are limited to four digits minimum? (line 1108)
 They aren't. vendor id is 32 bit integer. So the shortest is "vendor-X",
 where is 0..9. The "vendor-" string has 7 characters.

 > Apparently, there are no tests for this function.
 It is indirectly tested by tests in
 src/bin/dhcp{4,6}/tests/config_parser_unittests.cc.
 Added one new test.

 >
 > '''src/lib/dhcpsrv/dhcp_parsers.h'''
 > The doxygen description of the optionSpaceToVendorId lacks description
 of the option_space parameter. And again, it would be good to explain why
 the format is !''vendor-1234!'' or what is the minimum value (and why) of
 !''1234!''.
 It doesn't have to be exactly 4 digits. Extended the comment.

 > '''src/lib/dhcpsrv/option_space_container.h'''
 > Comments like:
 > {{{
 >     /// @param option_space name of the option space.
 > }}}
 >
 > or
 > {{{
 >     /// A map holding container (option space name is the key).
 > }}}
 >
 > no longer apply because you may use different selector.
 Updated.

 > '''src/lib/dhcpsrv/subnet.h'''
 > The new functions deserve doxygen descriptions.
 Added.

 That's it for tonight. Tomorrow morning I'll do the review of your
 changes. Can you check if my responses and fixes addressing all your
 concerns?

 Whoever finishes first, may claim a prize - the privilege of writing unit-
 tests for OptionVendor class ;)

 I'm not reassigning the ticket to you, because I still need to review your
 changes.

-- 
Ticket URL: <http://bind10.isc.org/ticket/3194#comment:2>
BIND 10 Development <http://bind10.isc.org>
BIND 10 Development