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

 * owner:  tomek => marcin


Comment:

 > '''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.

 > Nit: When the parser is given a bogus IP address the IOAddress
 constructor will throw an exception. Typically, parsers throw
 Dhcp4ConfigError exception but the IOAddress constructor will throw an
 exception of a different type. This shouldn't be a problem because (I
 think) all exceptions are caught. But for consistency the !RelayInfoParser
 could throw an exception. It is up to you.
 Implemented. Extended tests. They now check if specifying address of the
 wrong type will throw exception. Also check if bogus address will throw.

 >
 > '''src/lib/dhcpsrv/dhcp_parsers.h'''
 > A couple nits:
 > - RelayInfoParser::build: typo in!''strcture!''.
 > - instead of !''dummy!'' you could name the unused parameter
 !''unused!'' or !''ignored!'' - up to you.
 > - createConfigParser has no documentation
 Added.

 > '''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.

 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.

 > On the related matter... should !RelayInfo be the same for !Subnet4 and
 !Subnet6? Even if it should, maybe Subnet4 and Subnet6 should provide
 different implementations of setRelay function to validate that the relay
 information is valid for a particular subnet type?
 Yes. This can be the same structure with the same parser. Even if we
 decide to extend it, we still can have a single RelayInfo object. That's
 was the huge parser refactor was about, isn't it? So let's try to keep
 things unified as long as we can.

 >
 > 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().

 > A few nits:
 > - brief description of setRelay: Currently it is true, but if you extend
 the structure I doubt that it will be updated to say: !''Sets relay
 information!'', instead of !''Sets address of the relay!''.
 > - The same comment as above relates to @params tag.
 > - It should be @param, not @params.
 Updated.
 >
 > '''src/lib/dhcpsrv/tests/cfgmgr_unittest.cc'''
 > Not really related to this work because there is a lot of those in the
 code:
 > {{{
 >     EXPECT_EQ(subnet2, cfg_mgr.getSubnet6(ifaceid2, classify_));
 > }}}
 >
 > This will check that the pointers match. I wonder if we should rather
 have a == operator for subnet and check that subnets are equal, not
 necessarily that pointers are the same.
 We can create a ticket for it, but I don't expect anyone would look at it
 anytime soon. Excessive number of such tickets is one of the reasons that
 doomed recursive resolver project.

 > '''src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc'''
 > There should be a test for invalid ip address too.
 Added.

 Thanks for thorough review.
 Back to you, Marcin.

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