BIND 10 #3242: Kea4: check if only remote traffic can be supported

[BIND 10 Development]

#3242: Kea4: check if only remote traffic can be supported
-------------------------------------+-------------------------------------
            Reporter:  tomek         |                        Owner:
                Type:  defect        |  marcin
            Priority:  high          |                       Status:
           Component:  dhcp4         |  reviewing
            Keywords:                |                    Milestone:  DHCP-
           Sensitive:  0             |  Kea0.9-alpha
         Sub-Project:  DHCP          |                   Resolution:
Estimated Difficulty:  24            |                 CVSS Scoring:
         Total Hours:  35            |              Defect Severity:  N/A
                                     |  Feature Depending on Ticket:
                                     |          Add Hours to Ticket:  6
                                     |                    Internal?:  0
-------------------------------------+-------------------------------------
Changes (by tomek):

 * hours:  3 => 6
 * owner:  tomek => marcin
 * totalhours:  29 => 35


Comment:

 Reviewing 1b522d63a842654c680602d3ee89eac324333d48.

 You submitted #3320 about using option 50. Do you think it should go to
 Kea-proposed? It is now in DHCP-Outstanding-tickets where it won't
 be looked at for at least the next 3 mothns.

 This is very helpful work. Thanks for taking care of this.

 '''src/bin/dhcp4/dhcpv4_srv.cc''':
 selectSubnet(): Please move the comment starting in line 1505 into the
 scope
 it comments on. The same if true for the follow up comment starting in
 1515.

 accept(): Since the accept() method became very similar to
 sanityCheck() methods, perhaps you could merge those two together?

 Also, accept() checks if the message includes message type option, but
 ignores value returned by query->getType(). Perhaps it could further
 sanity check returned value, so we drop bogus (with invalid type)
 packets early?

 acceptDirectRequest(): The last line can be simplified as:
 {{{
     return (!(pkt->getLocalAddr() == bcast && !selectSubnet(pkt)));
 }}}

 Thank you for doing the clean up in dhcp4_test_utils.

 '''src/bin/dhcp4/tests/direct_client_unittest.cc'''

 DirectClientTest::configure() seems very handy. Can you move it to
 base Dhcpv4SrvTest?

 configureSubnet, configureTwoSubnets() - please add a comment that
 the real configuration would exclude .0 (network address) and .255
 (broadcast) address, but we ignore that fact for the sake of test
 simplicity.

 !DirectClientTest.twoSubnets:
 Interesting use of ASSERT_NO_FATAL_FAILURE. I must admit that I
 haven't thought about its use before. I'm afraid there are quite a few
 tests scattered all around that may not behave properly upon failure.

 '''src/lib/dhcp/tests/iface_mgr_test_config.h'''
 It would be useful to add a pointer in the !IfaceMgrTestConfig class
 description to the place where the fake interfaces are created.
 If you don't expect it to change, perhaps even copying the interfaces
 into would be useful?

 Why did you define !FlagLoopback, !FlagUp, !FlagRunning and other flags?
 These are simple boolean flags and they will never be any more complex
 than that. Unless, there's a good reason for them, please replace
 them will booleans.

 '''src/lib/dhcp/tests/iface_mgr_test_config.cc'''
 createIfaces(): eth0 and eth1 have exactly the same link-local
 address, which implies they have the same MAC address. Is this
 intentional?

 '''src/lib/dhcp/tests/iface_mgr_unittest.cc'''
 !IfaceMgrTest.ifaceGetAddress: Tests in general should restrict
 themselves to using reserved pools specified in RFC5735 and RFC5737
 (http://en.wikipedia.org/wiki/Reserved_IP_addresses#Reserved_IPv4_addresses),
 but I understand that in many cases that is not possible.

 '''src/lib/dhcpsrv/cfgmgr.cc'''
 CfgMgr::getSubnet4(iface_name): What sort of sorcery is this? ;)
 IPv6 loopback address in IPv4 methods?

 '''src/lib/dhcpsrv/tests/cfgmgr_unittest.cc'''
 !CfgMgrTest.getSubnet4ForInterface: the checks in lines 721,722 could
 be EXPECT_FALSE. ASSERT_FALSE is not necessary.

 -----------------

 The clean build fails for me:
 make -j8

 {{{
 make[6]: Wejście do katalogu
 `/home/thomson/devel/bind10/src/lib/dhcp/tests'
   CXX      libdhcptest_la-iface_mgr_test_config.lo
   CXX      libdhcptest_la-pkt_filter_test_stub.lo
   CXX      libdhcp___unittests-run_unittests.o
   CXX      libdhcp___unittests-hwaddr_unittest.o
   CXX      libdhcp___unittests-iface_mgr_unittest.o
   CXX      libdhcp___unittests-libdhcp++_unittest.o
   CXX      libdhcp___unittests-option4_addrlst_unittest.o
   CXX      libdhcp___unittests-option4_client_fqdn_unittest.o
   CXX      libdhcp___unittests-option6_addrlst_unittest.o
   CXX      libdhcp___unittests-option6_client_fqdn_unittest.o
   CXX      libdhcp___unittests-option6_ia_unittest.o
   CXX      libdhcp___unittests-option6_iaaddr_unittest.o
   CXX      libdhcp___unittests-option6_iaprefix_unittest.o
   CXX      libdhcp___unittests-option_int_unittest.o
   CXX      libdhcp___unittests-option_int_array_unittest.o
   CXX      libdhcp___unittests-option_data_types_unittest.o
   CXX      libdhcp___unittests-option_definition_unittest.o
   CXX      libdhcp___unittests-option_custom_unittest.o
   CXX      libdhcp___unittests-option_unittest.o
   CXX      libdhcp___unittests-option_vendor_unittest.o
   CXX      libdhcp___unittests-pkt4_unittest.o
   CXX      libdhcp___unittests-pkt6_unittest.o
   CXX      libdhcp___unittests-pkt_filter_unittest.o
   CXX      libdhcp___unittests-pkt_filter_inet_unittest.o
   CXX      libdhcp___unittests-pkt_filter_inet6_unittest.o
   CXX      libdhcp___unittests-pkt_filter_test_utils.o
   CXX      libdhcp___unittests-pkt_filter6_test_utils.o
   CXX      libdhcp___unittests-pkt_filter_lpf_unittest.o
   CXX      libdhcp___unittests-protocol_util_unittest.o
 make[6]: *** Brak reguł do zrobienia obiektu
 `../../../../src/lib/dhcp/tests/libdhcptest.la', wymaganego przez
 `libdhcp++_unittests'. Stop.
 make[6]: *** Oczekiwanie na niezakończone zadania....
   CXX      libdhcp___unittests-duid_unittest.o
 make[6]: Opuszczenie katalogu
 `/home/thomson/devel/bind10/src/lib/dhcp/tests'
 }}}

 Repeated make -j8 calls managed to finally build it.
 Once they compiled, all tests passed.

 ------------------

 Please describe the subnet selection logic in the BIND10 Guide.
 Copying appropriate parts of your comment:10 would probably address
 most of that request.

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