From: Thomas Markwalder Date: Sat, 1 Mar 2014 15:37:17 +0000 (-0500) Subject: [3352] Changed calculation of server FQDN flags in D2ClientMgr X-Git-Tag: kea-eng-20140313~5^2~5 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=9e1339296c3864779691099231d36fe287c35e24;p=thirdparty%2Fkea.git [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. --- diff --git a/src/lib/dhcpsrv/d2_client_mgr.cc b/src/lib/dhcpsrv/d2_client_mgr.cc index c806bc4936..1e03d7622b 100644 --- a/src/lib/dhcpsrv/d2_client_mgr.cc +++ b/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 diff --git a/src/lib/dhcpsrv/d2_client_mgr.h b/src/lib/dhcpsrv/d2_client_mgr.h index 6222e6b5c3..fdbee1f4da 100644 --- a/src/lib/dhcpsrv/d2_client_mgr.h +++ b/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 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 + 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 +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 void diff --git a/src/lib/dhcpsrv/tests/d2_client_unittest.cc b/src/lib/dhcpsrv/tests/d2_client_unittest.cc index b935a0bc2b..68c0e5f5bd 100644 --- a/src/lib/dhcpsrv/tests/d2_client_unittest.cc +++ b/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(*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(*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