[#1781] Addressed initial review comments

doc/sphinx/arm/hooks.rst
    Added paragraph about hook library ordering

src/hooks/dhcp/high_availability/query_filter.cc
    Added comment

src/hooks/dhcp/high_availability/tests/query_filter_unittest.cc
    Test beyond enum EOF.

diff --git a/doc/sphinx/arm/hooks.rst b/doc/sphinx/arm/hooks.rst
index 6d4b5a253eb2ea20b4c6319c2d75511da086eb94..4b2d32656bf9e8191e1a18306c56a966d8d0e7f0 100644 (file)
--- a/doc/sphinx/arm/hooks.rst
+++ b/doc/sphinx/arm/hooks.rst
@@ -252,6 +252,22 @@ Notes:
 At the moment, only the ``kea-dhcp4`` and ``kea-dhcp6`` processes support
 hook libraries.
 
+.. _order-of-configuation-hooks:
+
+Order of Configuration:
+~~~~~~~~~~~~~~~~~~~~~~~
+
+It is important to recognize that the order in which hook libraries are
+configured determines the order in which their callouts will be executed,
+in cases where more than one hook library implements the same callout.  For
+example, if you wish to use the flex-id hook library to formulate the client
+IDs in conjunction with HA hook library for load-balanced HA, it is essential
+that the flex-id library be specified first in your server's ``hooks-libraries``
+section.  This ensures that the client ID is formualted by the flex-id library
+before the HA library uses it for load-balancing. Similarly it would be best to
+specify forensic logging last, to ensure any other install hooks have made
+their contributions to the packet processing.
+
 .. _user-context-hooks:
 
 User Contexts in Hooks

diff --git a/src/hooks/dhcp/high_availability/query_filter.cc b/src/hooks/dhcp/high_availability/query_filter.cc
index c434e0e0f0366892913994d8b3e1d7fea4961158..4a74197fbe44a8ed67ecb2ef7b5c0807bbe1897c 100644 (file)
--- a/src/hooks/dhcp/high_availability/query_filter.cc
+++ b/src/hooks/dhcp/high_availability/query_filter.cc
@@ -92,6 +92,9 @@ std::array<bool, DHCPV6_TYPES_EOF> v6_ha_types = {
     false,  // DHCPV6_LEASEQUERY_DATA      = 17
     false,  // DHCPV6_RECONFIGURE_REQUEST  = 18
     false,  // DHCPV6_RECONFIGURE_REPLY    = 19
+    /// @todo There is some question as to whether DHCPV4_QUERY should get load-balanced
+    /// or not. It may not provide sufficient information to properly scope it. For now
+    /// we will not load-balance them.
     false,  // DHCPV6_DHCPV4_QUERY         = 20
     false,  // DHCPV6_DHCPV4_RESPONSE      = 21
     false,  // DHCPV6_ACTIVELEASEQUERY     = 22

diff --git a/src/hooks/dhcp/high_availability/tests/query_filter_unittest.cc b/src/hooks/dhcp/high_availability/tests/query_filter_unittest.cc
index ae797ed4b3d3fe4aded880f9fd440e517da4cc97..a9c4a69f51cfc8e568c496445d79ce8b73647d9c 100644 (file)
--- a/src/hooks/dhcp/high_availability/tests/query_filter_unittest.cc
+++ b/src/hooks/dhcp/high_availability/tests/query_filter_unittest.cc
@@ -768,7 +768,8 @@ QueryFilterTest::loadBalancingHaTypes4() {
     ASSERT_TRUE(server1_pkt && server2_pkt) << "do not have both scopes in "
                 << max_scope_tries << ", load balance broken?";
 
-    for (uint8_t msg_type = DHCP_NOTYPE; msg_type < DHCP_TYPES_EOF; ++msg_type) {
+    // We exceed DHCP_TYPES_EOF just to be sure.
+    for (uint8_t msg_type = 0; msg_type < DHCP_TYPES_EOF + 2; ++msg_type) {
         // All message types should be in scope for server1.
         server1_pkt->setType(msg_type);
 
@@ -778,7 +779,7 @@ QueryFilterTest::loadBalancingHaTypes4() {
         EXPECT_TRUE(is_in_scope);
 
         server2_pkt->setType(msg_type);
-        scope_class = "";
+        scope_class.clear();
         is_in_scope = filter.inScope(server2_pkt, scope_class);
         switch (msg_type) {
             case DHCPDISCOVER:
@@ -843,7 +844,8 @@ QueryFilterTest::loadBalancingHaTypes6() {
     ASSERT_TRUE(server1_pkt && server2_pkt) << "do not have both scopes in "
                 << max_scope_tries << ", load balance broken?";
 
-    for (uint8_t msg_type = DHCPV6_NOTYPE; msg_type < DHCPV6_TYPES_EOF; ++msg_type) {
+    // We exceed DHCPV6_TYPES_EOF just to be sure.
+    for (uint8_t msg_type = 0; msg_type < DHCPV6_TYPES_EOF + 2; ++msg_type) {
         // All message types should be in scope for server1.
         server1_pkt->setType(msg_type);
 
@@ -853,7 +855,7 @@ QueryFilterTest::loadBalancingHaTypes6() {
         EXPECT_TRUE(is_in_scope);
 
         server2_pkt->setType(msg_type);
-        scope_class = "";
+        scope_class.clear();
         is_in_scope = filter.inScope(server2_pkt, scope_class);
         switch (msg_type) {
             case DHCPV6_SOLICIT: