BIND 10 #3322: Client classification: relay address override

[BIND 10 Development]

#3322: Client classification: relay address override
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:  tomek
                Type:  enhancement   |                       Status:
            Priority:  high          |  reviewing
           Component:  dhcp          |                    Milestone:  DHCP-
            Keywords:                |  Kea0.9-alpha
           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
-------------------------------------+-------------------------------------
Changes (by marcin):

 * owner:  marcin => tomek


Comment:

 Reviewed commit: a3db6f08b0321dcfbd7539e07764f0c124ac19af

 '''!ChangeLog'''

 Some minor editorial changes:
 {{{
 7XX.    [func]          tomek
         b10-dhcp4, b10-dhcp6: IP address of the relay agent can now be
 specified
         for both IPv4 and IPv6 subnets. That information allows the server
 to
         properly handle a case where relay agent address does not match
 *the* subnet.
         This is mostly useful *for* shared subnets and cable networks.
         (Trac #3322, git abcd)
 }}}


 '''doc/guide/bind10-guide.xml'''
 Along with this ticket the note in section 17.6 became only partly true.
 The subnet selection mechanism not only is altered by the classification
 but also by the relay override mechanism. That should be updated in the
 note.

 '''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''
 Please use the !Dhcp4SrvTest::configure method to create configuration for
 the new tests.

 '''src/bin/dhcp6/tests/dhcp6_srv_unittest.cc'''
 Typo in the relayOverride: should be !''subnet4!'', not !''subnet6!''.


 '''src/lib/dhcpsrv/cfgmgr.cc'''
 Please consider adding a debug message in getSubnet4 and getSubnet6 to
 inform that the configured relay info is used to select subnet when relay
 argument is true.

 Nit: there is a commented default value for relay in getSubnet4, but there
 isn't one for getSubnet6. Personally, I never add the default parameter in
 .cc files, partly because it may be hard to maintain consistency between
 the default value in cc file and h file. But, if you want to keep it, you
 could add the default for getSubnet4 for consistency. But, it is up to
 you.

 '''src/lib/dhcpsrv/cfgmgr.h'''
 Typo in the getSubnet6 description: !''Dhcp4/subnet6[X]/relay/ip-
 address!'' - should be Dhcp6.

 Also, irrelevant comment !''They are for giaddr only!''. There is no
 giaddr in DHCPv6.

 Nit: technically relay could be const.

 When I was going through the documentation of getSubnet6 I realized that
 the function description deserves some updates with respect to relay
 information and classification which have been introduced lately.
 Specifically, in this part:
 {{{
     /// If there are any classes specified in a subnet, that subnet
     /// will be selected only if the client belongs to appropriate class.
     ///
     /// If relay is true then relay info overrides (i.e. value the
 sysadmin
     /// can configure in Dhcp4/subnet6[X]/relay/ip-address) can be used.
     /// That is true only for relays. Those overrides must not be used
     /// for client address or for client hints. They are for giaddr only.

 }}}

 it should be mentioned that it is based on the assumption that there is
 more than one subnet configured. For a single subnet the code may never
 get to classification nor to relay overriding. This is explained in the
 function body, but should be first of all explained in the function
 description. I can imagine that someone sets client class for the sole
 subnet and expects that some clients will be rejected, some accepted. In
 fact, he will end up accepting all clients because if there is one subnet
 and the client (or relay?) sends message from link local addres, this sole
 subnet is returned regardless of classification and such. So, that makes
 me doubt that we should return the sole subnet, regardless of client class
 and relay info?

 I also think that some considerations regarding this issue should be
 included in the bind10-guide.


 There should probably be a unit test that covers getSubnet4 and getSubnet6
 behavior when the relay parameter is true.

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