[3352] Changed calculation of server FQDN flags in D2ClientMgr

D2ClientMgr::analyzeFqdn template method was changed to correctly
set server S and N flags to properly support client delegation.
Added convenience method D2ClientMgr::getUpdateDirections.
Adjusted and added unit tests as appropriate.

src/lib/dhcpsrv/d2_client_mgr.cc

@@ -144,14 +144,20 @@ D2ClientMgr::analyzeFqdn(const bool client_s, const bool client_n,
 
     switch (mask) {
     case 0:
-        // If updates are enabled and we are overriding client delegation
-        // then S flag should be true.
-        server_s = (d2_client_config_->getEnableUpdates() &&
-                    d2_client_config_->getOverrideClientUpdate());
+        if (!d2_client_config_->getEnableUpdates()) {
+            server_s = false;
+            server_n = true;
+        } else {
+            // If updates are enabled and we are overriding client delegation
+            // then S flag should be true.  N-flag should be false.
+            server_s = d2_client_config_->getOverrideClientUpdate();
+            server_n = false;
+        }
         break;
 
     case 1:
         server_s = d2_client_config_->getEnableUpdates();
+        server_n = !server_s;
         break;
 
     case 2:
@@ -159,6 +165,7 @@ D2ClientMgr::analyzeFqdn(const bool client_s, const bool client_n,
         // S flag should be true.
         server_s = (d2_client_config_->getEnableUpdates() &&
                     d2_client_config_->getOverrideNoUpdate());
+        server_n = !server_s;
         break;
 
     default:
@@ -167,17 +174,6 @@ D2ClientMgr::analyzeFqdn(const bool client_s, const bool client_n,
                   "Invalid client FQDN - N and S cannot both be 1");
         break;
     }
-
-    /// @todo Currently we are operating under the premise that N should be 1
-    /// if the server is not doing updates nor do we have configuration
-    /// controls to govern forward and reverse updates independently.
-    /// In addition, the client FQDN flags cannot explicitly suggest what to
-    /// do with reverse updates. They request either forward updates or no
-    /// updates.  In other words, the client cannot request the server do or
-    /// not do reverse updates.  For now, we are either going to do updates in
-    /// both directions or none at all.  If and when additional configuration
-    /// parameters are added this logic will have to be reassessed.
-    server_n = !server_s;
 }
 
 std::string

src/lib/dhcpsrv/d2_client_mgr.h

@@ -115,7 +115,39 @@ public:
     /// conjunction with the configuration parameters updates-enabled, override-
     /// no-updates, and override-client-updates to determine the values that
     /// should be used for the server's FQDN S and N flags.
-    /// The logic in this method is based upon RFCs 4702 and 4704.
+    /// The logic in this method is based upon RFCs 4702 and 4704, and is
+    /// shown in the following truth table:
+    ///
+    /// @code
+    ///
+    /// When Updates are enabled:
+    ///
+    ///  ON = Override No Updates, OC = Override Client Updates
+    ///
+    ///  | Client |--------   Server Response Flags   ------------|
+    ///  | Flags  | ON=F,OC=F | ON=F,OC=T | ON=T,OC=F | ON=T,OC=T |
+    ///  |  N-S   |  N-S-O    |   N-S-O   |   N-S-O   |   N-S-O   |
+    ///  ----------------------------------------------------------
+    ///  |  0-0   |  0-0-0    |   0-1-1   |   0-0-0   |   0-1-1   |
+    ///  |  0-1   |  0-1-0    |   0-1-0   |   0-1-0   |   0-1-0   |
+    ///  |  1-0   |  1-0-0    |   1-0-0   |   0-1-1   |   0-1-1   |
+    ///
+    /// One can then use the server response flags to know when forward and
+    /// reverse updates should be performed:
+    ///
+    ///  - Forward updates should be done when the Server S-Flag is true.
+    ///  - Reverse updates should be done when the Server N-Flag is false.
+    ///
+    /// When Updates are disabled:
+    ///
+    /// | Client  | Server |
+    /// |  N-S    |  N-S-O |
+    /// --------------------
+    /// |  0-0    | 1-0-0  |
+    /// |  0-1    | 1-0-1  |
+    /// |  1-0    | 1-0-0  |
+    ///
+    /// @endcode
     ///
     /// @param client_s  S Flag from the client's FQDN
     /// @param client_n  N Flag from the client's FQDN
@@ -172,6 +204,26 @@ public:
     template <class T>
     void adjustFqdnFlags(const T& fqdn, T& fqdn_resp);
 
+    /// @brief Get direcional update flags based on server FQDN flags
+    ///
+    /// Templated convenience method which determines whether forward and
+    /// reverse updates should be performed based on a server response version
+    /// of the FQDN flags. The logic is straight forward and currently not
+    /// dependent upon configuration specific values:
+    ///
+    /// * forward will be true if S_FLAG is true
+    /// * reverse will be true if N_FLAG is false
+    ///
+    /// @param fqdn FQDN option from which to read server (outbound) flags
+    /// @param forward[out] bool value will be set to true if forward udpates
+    /// should be done, false if not.
+    /// @param reverse[out] bool value will be set to true if reverse udpates
+    /// should be done, false if not.
+    /// @tparam T FQDN Option class containing the FQDN data such as
+    /// dhcp::Option4ClientFqdn or dhcp::Option6ClientFqdn
+    template <class T>
+    void getUpdateDirections(const T& fqdn_resp, bool& forward, bool& reverse);
+
     /// @brief Set server FQDN name based on configuration and a given FQDN
     ///
     /// Templated method which adjusts the domain name value and type in
@@ -394,6 +446,13 @@ D2ClientMgr::adjustFqdnFlags(const T& fqdn, T& fqdn_resp) {
     fqdn_resp.setFlag(T::FLAG_O, (fqdn.getFlag(T::FLAG_S) != server_s));
 }
 
+template <class T>
+void
+D2ClientMgr::getUpdateDirections(const T& fqdn_resp,
+                                 bool& forward, bool& reverse) {
+    forward = fqdn_resp.getFlag(T::FLAG_S);
+    reverse = !(fqdn_resp.getFlag(T::FLAG_N));
+}
 
 template <class T>
 void

src/lib/dhcpsrv/tests/d2_client_unittest.cc

@@ -337,10 +337,10 @@ TEST(D2ClientMgr, analyzeFqdnEnabledNoOverrides) {
 
     // client S=0 N=0 means client wants to do forward update.
     // server S should be 0 (server is not doing forward updates)
-    // and server N should be 1 (server doing no updates)
+    // and server N should be 0 (server doing reverse updates)
     mgr.analyzeFqdn(false, false, server_s, server_n);
     EXPECT_FALSE(server_s);
-    EXPECT_TRUE(server_n);
+    EXPECT_FALSE(server_n);
 
     // client S=1 N=0 means client wants server to do forward update.
     // server S should be 1 (server is doing forward updates)
@@ -379,10 +379,10 @@ TEST(D2ClientMgr, analyzeFqdnEnabledOverrideNoUpdate) {
 
     // client S=0 N=0 means client wants to do forward update.
     // server S should be 0 (server is not doing forward updates)
-    // and server N should be 1 (server is not doing any updates)
+    // and server N should be 0 (server is doing reverse updates)
     mgr.analyzeFqdn(false, false, server_s, server_n);
     EXPECT_FALSE(server_s);
-    EXPECT_TRUE(server_n);
+    EXPECT_FALSE(server_n);
 
     // client S=1 N=0 means client wants server to do forward update.
     // server S should be 1 (server is doing forward updates)
@@ -462,7 +462,7 @@ TEST(D2ClientMgr, adjustFqdnFlagsV4) {
 
     // client S=0 N=0 means client wants to do forward update.
     // server S should be 0 (server is not doing forward updates)
-    // and server N should be 1 (server doing no updates)
+    // and server N should be 0 (server is doing reverse updates)
     // and server O should be 0
     request.reset(new Option4ClientFqdn(0, Option4ClientFqdn::RCODE_CLIENT(),
                                         "", Option4ClientFqdn::PARTIAL));
@@ -471,7 +471,7 @@ TEST(D2ClientMgr, adjustFqdnFlagsV4) {
 
     mgr.adjustFqdnFlags<Option4ClientFqdn>(*request, *response);
     EXPECT_FALSE(response->getFlag(Option4ClientFqdn::FLAG_S));
-    EXPECT_TRUE(response->getFlag(Option4ClientFqdn::FLAG_N));
+    EXPECT_FALSE(response->getFlag(Option4ClientFqdn::FLAG_N));
     EXPECT_FALSE(response->getFlag(Option4ClientFqdn::FLAG_O));
 
     // client S=1 N=0 means client wants server to do forward update.
@@ -505,6 +505,42 @@ TEST(D2ClientMgr, adjustFqdnFlagsV4) {
     EXPECT_TRUE(response->getFlag(Option4ClientFqdn::FLAG_O));
 }
 
+/// @brief Verified the getUpdateDirections template method with
+/// Option4ClientFqdn objects.
+TEST(D2ClientMgr, updateDirectionsV4) {
+    D2ClientMgr mgr;
+    Option4ClientFqdnPtr response;
+
+    bool do_forward = false;
+    bool do_reverse = false;
+
+    // Response S=0, N=0 should mean do reverse only.
+    response.reset(new Option4ClientFqdn(0,
+                                         Option4ClientFqdn::RCODE_CLIENT(),
+                                         "", Option4ClientFqdn::PARTIAL));
+    mgr.getUpdateDirections(*response, do_forward, do_reverse);
+    EXPECT_FALSE(do_forward);
+    EXPECT_TRUE(do_reverse);
+
+    // Response S=0, N=1 should mean don't do either.
+    response.reset(new Option4ClientFqdn(Option4ClientFqdn::FLAG_N,
+                                         Option4ClientFqdn::RCODE_CLIENT(),
+                                         "", Option4ClientFqdn::PARTIAL));
+    mgr.getUpdateDirections(*response, do_forward, do_reverse);
+    EXPECT_FALSE(do_forward);
+    EXPECT_FALSE(do_reverse);
+
+    // Response S=1, N=0 should mean do both.
+    response.reset(new Option4ClientFqdn(Option4ClientFqdn::FLAG_S,
+                                         Option4ClientFqdn::RCODE_CLIENT(),
+                                         "", Option4ClientFqdn::PARTIAL));
+    mgr.getUpdateDirections(*response, do_forward, do_reverse);
+    EXPECT_TRUE(do_forward);
+    EXPECT_TRUE(do_reverse);
+
+    // Response S=1, N=1 isn't possible.
+}
+
 /// @brief Tests the qualifyName method's ability to construct FQDNs
 TEST(D2ClientMgr, qualifyName) {
     D2ClientMgr mgr;
@@ -761,7 +797,7 @@ TEST(D2ClientMgr, adjustFqdnFlagsV6) {
 
     // client S=0 N=0 means client wants to do forward update.
     // server S should be 0 (server is not doing forward updates)
-    // and server N should be 1 (server doing no updates)
+    // and server N should be 0 (server doing reverse updates)
     // and server O should be 0
     request.reset(new Option6ClientFqdn(0, "", Option6ClientFqdn::PARTIAL));
     response.reset(new Option6ClientFqdn(*request));
@@ -769,7 +805,7 @@ TEST(D2ClientMgr, adjustFqdnFlagsV6) {
 
     mgr.adjustFqdnFlags<Option6ClientFqdn>(*request, *response);
     EXPECT_FALSE(response->getFlag(Option6ClientFqdn::FLAG_S));
-    EXPECT_TRUE(response->getFlag(Option6ClientFqdn::FLAG_N));
+    EXPECT_FALSE(response->getFlag(Option6ClientFqdn::FLAG_N));
     EXPECT_FALSE(response->getFlag(Option6ClientFqdn::FLAG_O));
 
     // client S=1 N=0 means client wants server to do forward update.
@@ -801,4 +837,37 @@ TEST(D2ClientMgr, adjustFqdnFlagsV6) {
     EXPECT_TRUE(response->getFlag(Option6ClientFqdn::FLAG_O));
 }
 
+/// @brief Verified the getUpdateDirections template method with
+/// Option6ClientFqdn objects.
+TEST(D2ClientMgr, updateDirectionsV6) {
+    D2ClientMgr mgr;
+    Option6ClientFqdnPtr response;
+
+    bool do_forward = false;
+    bool do_reverse = false;
+
+    // Response S=0, N=0 should mean do reverse only.
+    response.reset(new Option6ClientFqdn(0,
+                                         "", Option6ClientFqdn::PARTIAL));
+    mgr.getUpdateDirections(*response, do_forward, do_reverse);
+    EXPECT_FALSE(do_forward);
+    EXPECT_TRUE(do_reverse);
+
+    // Response S=0, N=1 should mean don't do either.
+    response.reset(new Option6ClientFqdn(Option6ClientFqdn::FLAG_N,
+                                         "", Option6ClientFqdn::PARTIAL));
+    mgr.getUpdateDirections(*response, do_forward, do_reverse);
+    EXPECT_FALSE(do_forward);
+    EXPECT_FALSE(do_reverse);
+
+    // Response S=1, N=0 should mean do both.
+    response.reset(new Option6ClientFqdn(Option6ClientFqdn::FLAG_S,
+                                         "", Option6ClientFqdn::PARTIAL));
+    mgr.getUpdateDirections(*response, do_forward, do_reverse);
+    EXPECT_TRUE(do_forward);
+    EXPECT_TRUE(do_reverse);
+
+    // Response S=1, N=1 isn't possible.
+}
+
 } // end of anonymous namespace