BIND 10 #3274: Kea4/6: use client classification in subnet selection (classification, part 2)

[BIND 10 Development]

#3274: Kea4/6: use client classification in subnet selection (classification, part
2)
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:
                Type:  enhancement   |  marcin
            Priority:  high          |                       Status:
           Component:  dhcp          |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea0.9-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  50            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  1
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by marcin):

 * hours:  6 => 1
 * totalhours:  49 => 50


Comment:

 Replying to [comment:10 tomek]:
 > > '''src/lib/dhcpsrv/dhcp_parsers.cc'''
 > > Line 814: It should be !RelayInfoParser, not !PoolParser.
 > >
 > > The parser should validate whether the IP address is v4 or v6. If I
 understand correctly, the v6 relay address doesn't make sense for DHCPv4
 server and vice versa. The global_context_->universe carries necessary
 information to check whether we are in the v4 or v6 context. In the
 future, the data structure may be extended so as more sophisticated
 validation may be required and we should probably have some methods in
 Subnet to handle that.
 > Added validation. I decided to follow the same path as ParserContext -
 pass Option::Universe and then make decisions based on that.


 Although, normally I am picky about use of consts. Adding a const in front
 of Option::Universe may cost you cppcheck warnings on some systems. The
 cppcheck for some reason wants const enums be passed by reference, like
 any other class.

 I added a space in one of the exception strings to make exception string
 more readable.

 > >
 > > '''src/lib/dhcpsrv/dhcp_parsers.h'''
 > > A couple nits:
 > > - RelayInfoParser::build: typo in!''strcture!''.

 This was not addressed, so I now fixed it myself.

 > > '''src/lib/dhcpsrv/subnet.h'''
 > > The member which holds relay information is public and I believe it
 should be private or protected. One can argue that it is simpler to expose
 a public field instead of providing getters and setters for each member.
 But, I think there is a lot of sense in providing a setter if a parameter
 being set is a structure which doesn't validate its fields. Currently the
 !RelayInfo structure holds an IP address but it is already questionable
 whether it should be possible to set the IPv6 address as a relay address
 for IPv4 subnet. It is possible with the current code because neither
 !RelayInfo nor setRelay validate it. If we extend !RelayInfo structure
 there will be more cases to validate. So, if we want to stick to having
 !RelayInfo as a structure, we probably need to extend setRelay to validate
 that the structure is acceptable. But, if we have setRelay function it
 doesn't make much sense to expose relay_ in a public scope because
 everybody can bypass the validation checks.
 > You are right regarding the setters, but I'm more concerned with getters
 that are much more frequently used. What do you think the getter should
 return? Shared pointer? Then you're complicating both the structure access
 method and making it slower.

 Why not return the const reference to relay_? It is sufficiently fast and
 precludes modification of the object. And I don't really see a big problem
 in the fact that the reference invalidates when the object is destroyed.
 We do it in many places and it has never been a real problem. It's been
 rather theoretical.

 >
 > I think the parameter verification is sufficiently checked at the
 parsing time. If you disagree, I can then turn RelayInfo into a class,
 make the addr_field protected, and implement getters/setters for it.

 It is checked, I agree. However, if we start adding new members to this
 structure it would be better to do validation on the structure side, on
 the parser's side. The parser's code is already overly complex and any
 more sophisticated validation would make it more complex. This is opposed
 to calling a single method of RelayInfo like: RelayInfo::validateData().
 So, I think that turning this structure into a class that has some
 validation capability makes sense but I don't consider it critical now.

 > >
 > > allowClientClass: The documentation says that this function '''adds'''
 a class to the list of supported classes. It doesn't add. It replaces the
 current class name with a new class name.
 > >
 > > The following member:
 > > {{{
 > > ClientClass white_list_;
 > > }}}
 > > has confusing name, i.e. everything that is called a list sounds like
 it contains multiple elements in it. In fact, it merely contains a single
 name.
 > Because for now it is a single class, but it will grow into two lists
 (list of allowed classes,
 > list of forbidden classes) eventually. So I decided to keep the
 interface forward compatible,
 > only the implementation will have to change. But I agree that it looks
 confusing, so added
 > an explanatory note to white_list_ and a reference to it in
 allowClientClass().

 I still don't get that. Per documentation of !ClientClass, it represents a
 single class. There is a !ClientClasses type which holds a list of
 classes. So, are you saying that ClientClass will evolve to multiple
 classes and !ClientClasses will hold a list of list of classes? If
 !ClientClass is going to evolve to collection of client's classes, we may
 need to rename it now to indicate that it will be a list and add a note
 that even though it is a list, there may be just one class on the list at
 the moment and the rest is ignored, or whatever.

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