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

[BIND 10 Development]

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

 * owner:  marcin => tomek


Comment:

 Replying to [comment:5 tomek]:
 > Replying to [comment:2 marcin]:
 > > Reviewed commit: 4a08e3662e7720ac6c96b7d585084bbe4033c29b

 Reviewed commit 9d3cf130a0c92c715b8ff1b7ce8f1743ab9c2bc7

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

 Similar to the comment to changelog (below)... It is worth to clarify what
 kind of !''flag!'' has been introduced to control whether the client id is
 echoed back or not. It would be clearer to call it !''configuration
 parameter!''.

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

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

 ok. I didn't mean to complicate things if we feel it is unnecessary.

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

 Ok. Except that you could do assert instead of expect, because if the
 first check is wrong, the further part of the test doesn't make sense.
 This applies to the other expect too.

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

 ok.

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

 ok.

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

 ok

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

 ok

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

 ok. Except, that you could have used assert for the first check.

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

 I think it may be worth to clarify what !''flag!'' has been added, i.e. it
 is a !''configuration parameter!'' or something like this.

 I am fine with the changes. If you concur to replace expects to asserts
 were indicated, please do so. Also, please clarify what the !''flag!'' is
 as it sounds a bit ambiguous. I don't need to see this ticket again.

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