BIND 10 #3210: Kea4 must not echo back client-id

[BIND 10 Development]

#3210: Kea4 must not echo back client-id
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:
                Type:  defect        |  marcin
            Priority:  very high     |                       Status:
           Component:  dhcp4         |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea1.0-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  0             |                 CVSS Scoring:
         Total Hours:  0             |              Defect Severity:  Low
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  0
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * owner:  tomek => marcin


Comment:

 Replying to [comment:2 marcin]:
 > Reviewed commit: 4a08e3662e7720ac6c96b7d585084bbe4033c29b
 >
 > '''doc/guide/bind10-guide.xml'''
 > Apparently, the RFC6842 says that the client id must be echoed back to
 the client IF client id has been included in the client's message. I
 suggest that the bind10-guide is updated accordingly. Otherwise, it gives
 an impression that client-id must be always included.
 Clarified.

 > '''src/bin/dhcp4/config_parser.cc'''
 > commitGlobalOptions: why is this function called like this? It merely
 stores the new !''echo-client-id!'' option in the !CfgMgr.
 Because it is the first global parameter. The first, but definitely not
 the last one. For example, dibbler-server has at least 24 global scoped
 parameters (work-dir, log parameters, auth info, cache sizes, performance-
 mode and tons of other stuff). Added a comment that clarifies the name.


 > Question: do you think it makes sense to have a per-subnet configuration
 of this new option?
 No. Let's try to follow the latest spec. If users complain about that,
 there's the (global) switch. If users complain about that, then we'll ask
 for detailed explanation why that is not sufficient. If they convince us,
 then we will invest time in fancy migration procedures with per-subnet
 information.


 > '''src/bin/dhcp4/tests/config_parser_unittest.cc'''
 > The echoClientId test is insufficient because it doesn't check the value
 of !''echo-client-id!'' flag before applying the configuration.
 The test has been extended. It checks the default (before any
 configuration tricks), then "echo-client-id false" configuration and
 finally a reverse back to the default.

 > '''src/bin/dhcp4/dhcp4_test_utils.h'''
 > checkClientId: After recent changes to this function you should extend
 the description of this function to say that the function checks presence
 of the client-id if specified so in the !CfgMgr and checks the absence in
 other case. Otherwise it looks like you always expect that client-id is
 present.
 Done.

 > '''src/bin/dhcp4/tests/dhcp4_srv_unittest.cc'''
 > Dhcpv4SrvTest::discoverEchoClientId: The test description is a bit
 ambiguous. It may be unclear what it means that the !''echoing back
 client-id!'' is controllable.
 Clarified.

 > The checks in line 705 and 713 look like duplicates of what the
 !''checkClientId!'' function is already doing.
 Removed.

 > The same comments apply to Dhcpv4SrvTest::requestEchoClientId
 Removed.

 > '''src/lib/dhcpsrv/cfgmgr.h'''
 > I fixed a few trivial issues and pushed changes to the branch.
 Thanks.

 > '''src/lib/dhcpsrv/tests/cfgmgr_unittest.cc'''
 > CfgMgrTest::echoClientId: Just a thought. The test could also check that
 the flag can be set to TRUE after setting it to FALSE. However, I admit
 that it is not very likely to fail :-)
 Implemented anyway. Somewhat not surprisingly, the test is passing.
 >

 > We need a !ChangeLog for this because it has a user impact.
 Added proposed ChangeLog.

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