BIND 10 #3322: Client classification: relay address override

[BIND 10 Development]

#3322: Client classification: relay address override
-------------------------------------+-------------------------------------
            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:  11            |              Defect Severity:
                                     |  Medium
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  8
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * hours:  3 => 8
 * owner:  tomek => marcin
 * totalhours:  3 => 11


Comment:

 Replying to [comment:5 marcin]:
 > 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)
 > }}}
 Thanks.

 > '''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.
 Updated section 17.6 and some v6 sections.

 > '''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''
 > Please use the !Dhcp4SrvTest::configure method to create configuration
 for the new tests.
 Done. Cool method. Implemented similar one for v6.

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

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

 > 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.
 Good point. I haven't thought about the cc/h consistency over time.
 Removed comment.
 >
 > '''src/lib/dhcpsrv/cfgmgr.h'''
 > Typo in the getSubnet6 description: !''Dhcp4/subnet6[X]/relay/ip-
 address!'' - should be Dhcp6.
 Fixed.

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

 > Nit: technically relay could be const.
 Added.

 > 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?
 This comment is no longer relevant. After our discussion on Jabber I
 decided to get rid of the single subnet exception. There were surprisingly
 wide repercussions of that step:
 - over 20 tests started to fail
 - they all worked under the assumption that the subnet will be selected if
 there's only one and source is link-local
 - I adjusted them to use interface name instead, but that brought the
 issue of having known interface configuration
 - So PktFilter6TestStub was implemented.
 - Dhcpv6SrvTest was refactored slightly, so there is no configure() method
 similar to the one in Dhcpv4SrvTest
 - Many tests updated to match mentioned changes

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

 > There should probably be a unit test that covers getSubnet4 and
 getSubnet6 behavior when the relay parameter is true.
 There are two new tests for this now.

 Thanks for your review, but I'm afraid this is not over yet...

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