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:  53            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  3
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * hours:  1 => 3
 * owner:  tomek => marcin
 * totalhours:  50 => 53


Comment:

 Replying to [comment:13 marcin]:
 > 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.
 Changed to reference.

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

 > > > '''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.
 Ok. Added getRelayInfo() that returns const reference and the relay_
 member is now protected. Also renamed getRelay to getRelayInfo().

 > 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.
 We will turn it into a class when more parameters appear in it.

 > > > 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.
 Further clarified the @todo text. ClientClass is and will remain a single
 class. ClientClasses is a collection of classes with possibly extra
 quantifiers added later.

 The intention here is to eventually change white_list_ into ClientClasses.
 There is a reason to avoid it for now, though. Right now the algorithm
 checks whether a collection of classes (that client's message belongs to)
 meet a single class. This is simple: iterate over classes and find a
 match. If we promote white_list_ to ClientClasses, we will have to
 implement a inclusion relationship check of two sets. (To check whether
 each element from set A also belongs to set B). This is not overly
 complex, but has some corner cases (what if the expected classes list is
 empty? It means accept for black_list, but means reject for white_list,
 unless there's nothing on black_list, which means that no access control
 is configured, hence means accept). So this starts to be a bigger task,
 like a separate ticket that can stand on its own. Hence white_list_ is a
 ClientClass now, but will be promoted to ClientClasses eventually.

 I hope that explanation is satisfactory.

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