BIND 10 #3316: Kea not able to parse vendor class option

[BIND 10 Development]

#3316: Kea not able to parse vendor class option
-------------------------------------+-------------------------------------
            Reporter:  wlodekwencel  |                        Owner:
                Type:  defect        |  marcin
            Priority:  medium        |                       Status:
           Component:  dhcp6         |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea0.9-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  32            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  3
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * hours:  4 => 3
 * owner:  tomek => marcin
 * totalhours:  29 => 32


Comment:

 Replying to [comment:10 marcin]:
 > Replying to [comment:9 tomek]:
 > Added. As we agreed on jabber, you'll need to provide some text about
 why we're doing it.
 There's slight complication. The section about client classification is on
 master, but not on trac3316. We'll decide what to do with it over jabber.

 > > Do we need OpaqueDataTuple::Buffer type? Can't you just use
 !OptionBuffer?
 > > If you don't have strong opinion on this one, I'd prefer to keep the
 > > number of new types to minimum.
 >
 > We could, but the !OptionBuffer is defined in Option class and I would
 need to include the option.h in the opaque_data_tuple.h which would cause
 a circular dependency. Or, I could do forward declaration which I dislike,
 as it causes troubles. Alternatively, I could move the OptionBuffer
 definition to a separate header file for this typedef. I think it is too
 much burden. And in fact, the !OpaqueDataTuple is independent from any
 !Option class and I prefer it to remain that way.
 Ok. Let the code stay as it is now.

 > > append() instead of saying that the behavior is undefined if there is
 > > too much data added, it would be better to just throw an exception.
 >
 > I made it a template function so as I can provide any type of iterator
 as function, including a raw pointer. For raw pointers I have no means to
 check whether the length is correct or not. That is the down side of using
 non-explicit data type but I think it has more benefits.
 I see. I never hid the fact that I'm not a great enthusiast of
 templates...
 Let the code stay as it is now.

 > > Shouldn't the !TuplesCollection type be moved to opaque_data_tuple.h?
 > > There may be other places that will operate on tuple collections.
 >
 > It could. But why should I mandate the use of the vector as a collection
 of tuples? If there is some other class that needs a different type of
 container (e.g. a list or other proprietary container) I should let it
 have it. Also, at the moment we have just one class using tuples so there
 is no urgency in having it in the common place.
 If I did the code, I would define a type in opaque_data_tuple.h and
 encourage others to use that type (and change it a different container, if
 needed). But that's a matter of personal preference. So let's keep the
 code as it is now.

 > It seems that modifying the !OptionVendorClass::toText() is not enough
 as we will need to have similar capability for other options. Also, the
 toText() function for other options presents data fields using data=value
 notation, so I don't really see a point to make it differently for
 !OptionVendorClass. If we want to have something returning the text in the
 parsable format we should add a different method.
 I'm afraid that is true. So we will probably need to add a string
 getTextValue() or something similar to all classes. But that is definitely
 not something we should consider now. We'll discuss that during F2F.

 > > Finally, a general comment. This bug (two bugs, actually) was
 discovered
 > > by the fact that the tests send short vendor-class option. The first
 > > bug was that our code was not able to parse specific option. You fixed
 > > that bug. There is another bug - if for whatever reason we can't parse
 > > an option, we should ignore just that option, not the whole message.
 > >
 > > This has to be fixed. I don't have any preference if this is done in
 > > this ticket or as a separate ticket.
 >
 > Note that it is not a problem specific to !OptionVendorClass. We have
 plenty of other options - for some of them we should drop the packet, for
 some of them just an option. Also, some of the errors in parsing options
 may result in inability to parse the rest of the packet. There is a
 question of what we should do in such case.
 >
 > All in all, the problem is much more complex in my opinion and we should
 not be trying to solve it here. One of the possible ways would be to
 define different types of exceptions for options, e.g. !OptionFatalError,
 !OptionNonFatalError or so, to inform the caller whether it should drop a
 packet or just an option.
 Fair enough. Let's discuss this on a meeting tomorrow. I think the fact
 that we can receive a single option that we can't parse causes us to drop
 the whole packet is an issue. One way or the other, it is definitely a
 matter for separate ticket.
 > {{{
 > XXX.  [bug]           marcin
 >       b10-dhcp6: server parses DHCPv6 Vendor Class option. Previously
 >       the server failed to parse Vendor Class option having empty opaque
 >       data field because of the invalid definition in libdhcp++. The
 >       DHCPv6 Vendor Class option and DHCPv4 V-I Vendor Class option is
 >       now represented by the new OptionVendorClass. The b10-dhcp4 is
 >       affected by this change such that it uses new class to parse the
 >       DHCPv4 V-I Vendor Class option.
 >       (Trac #3316, git abcd)
 > }}}
 That looks fine.

 I've committed a change regarding VENDOR_CLASS prefix. It is now added to
 all classes, in v4 and v6. Please review. If you think it is ok, please
 merge. If not, pass the ticket back to me.

 I think we can add the client classification doc update as a separate,
 super-small ticket.

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