[3282] Addressed review comments.

Most notable change is the removeal of the remove-on-renew
configuration parameter.  Other changes were to commentary
or minor code improvements.

src/bin/dhcp4/dhcp4.spec

@@ -336,13 +336,6 @@
                 "item_description" : "Format of the update request packet"
             },
             {
-                "item_name": "remove-on-renew",
-                "item_type": "boolean",
-                "item_optional": true,
-                "item_default": false,
-                "item_description": "Enable requesting a DNS Remove, before a DNS Update on renewals"
-            },
-            {
 
                 "item_name": "always-include-fqdn",
                 "item_type": "boolean",

src/bin/dhcp4/dhcp4_srv.cc

@@ -1335,8 +1335,7 @@ Dhcpv4Srv::processRelease(Pkt4Ptr& release) {
                     .arg(client_id ? client_id->toText() : "(no client-id)")
                     .arg(release->getHWAddr()->toText());
 
-                if (CfgMgr::instance().ddnsEnabled() &&
-                    CfgMgr::instance().getD2ClientConfig()->getRemoveOnRenew()) {
+                if (CfgMgr::instance().ddnsEnabled()) {
                     // Remove existing DNS entries for the lease, if any.
                     queueNameChangeRequest(isc::dhcp_ddns::CHG_REMOVE, lease);
                 }

src/bin/dhcp4/tests/config_parser_unittest.cc

@@ -2529,7 +2529,6 @@ TEST_F(Dhcp4ParserTest, d2ClientConfig) {
         "     \"server-port\" : 777, "
         "     \"ncr-protocol\" : \"UDP\", "
         "     \"ncr-format\" : \"JSON\", "
-        "     \"remove-on-renew\" : true, "
         "     \"always-include-fqdn\" : true, "
         "     \"allow-client-update\" : true, "
         "     \"override-no-update\" : true, "
@@ -2562,7 +2561,6 @@ TEST_F(Dhcp4ParserTest, d2ClientConfig) {
     EXPECT_EQ(777, d2_client_config->getServerPort());
     EXPECT_EQ(dhcp_ddns::NCR_UDP, d2_client_config->getNcrProtocol());
     EXPECT_EQ(dhcp_ddns::FMT_JSON, d2_client_config->getNcrFormat());
-    EXPECT_TRUE(d2_client_config->getRemoveOnRenew());
     EXPECT_TRUE(d2_client_config->getAlwaysIncludeFqdn());
     EXPECT_TRUE(d2_client_config->getOverrideNoUpdate());
     EXPECT_TRUE(d2_client_config->getOverrideClientUpdate());
@@ -2589,7 +2587,6 @@ TEST_F(Dhcp4ParserTest, invalidD2ClientConfig) {
         "     \"server-port\" : 5301, "
         "     \"ncr-protocol\" : \"UDP\", "
         "     \"ncr-format\" : \"JSON\", "
-        "     \"remove-on-renew\" : true, "
         "     \"always-include-fqdn\" : true, "
         "     \"allow-client-update\" : true, "
         "     \"override-no-update\" : true, "

src/bin/dhcp4/tests/fqdn_unittest.cc

@@ -35,11 +35,10 @@ class NameDhcpv4SrvTest : public Dhcpv4SrvFakeIfaceTest {
 public:
 
     // Bit Constants for turning on and off DDNS configuration options.
-    static const uint16_t REMOVE_ON_RENEW = 1;
-    static const uint16_t ALWAYS_INCLUDE_FQDN = 2;
-    static const uint16_t OVERRIDE_NO_UPDATE = 4;
-    static const uint16_t OVERRIDE_CLIENT_UPDATE = 8;
-    static const uint16_t REPLACE_CLIENT_NAME = 16;
+    static const uint16_t ALWAYS_INCLUDE_FQDN = 1;
+    static const uint16_t OVERRIDE_NO_UPDATE = 2;
+    static const uint16_t OVERRIDE_CLIENT_UPDATE = 4;
+    static const uint16_t REPLACE_CLIENT_NAME = 8;
 
     NameDhcpv4SrvTest() : Dhcpv4SrvFakeIfaceTest() {
         srv_ = new NakedDhcpv4Srv(0);
@@ -72,7 +71,6 @@ public:
         ASSERT_NO_THROW(cfg.reset(new D2ClientConfig(true,
                                   isc::asiolink::IOAddress("192.0.2.1"), 477,
                                   dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                                  (mask & REMOVE_ON_RENEW),
                                   (mask & ALWAYS_INCLUDE_FQDN),
                                   (mask & OVERRIDE_NO_UPDATE),
                                   (mask & OVERRIDE_CLIENT_UPDATE),
@@ -922,14 +920,11 @@ TEST_F(NameDhcpv4SrvTest, processTwoRequestsHostname) {
 }
 
 // Test that when a release message is sent for a previously acquired lease,
-// DDNS updates are enabled and remove-on-renew is true that the server
-// genenerates a NameChangeRequest to remove entries corresponding to the
-// released lease.
-TEST_F(NameDhcpv4SrvTest, processRequestReleaseRemoveOn) {
-    // Enable ddns updates and remove-on-renew .
-    enableD2(REMOVE_ON_RENEW);
+// DDNS updates are enabled that the server genenerates a NameChangeRequest
+// to remove entries corresponding to the released lease.
+TEST_F(NameDhcpv4SrvTest, processRequestRelease) {
+    // Verify the updates are enabled.
     ASSERT_TRUE(CfgMgr::instance().ddnsEnabled());
-    ASSERT_TRUE(CfgMgr::instance().getD2ClientConfig()->getRemoveOnRenew());
 
     // Create and process a lease request so we have a lease to release.
     Pkt4Ptr req = generatePktWithFqdn(DHCPREQUEST, Option4ClientFqdn::FLAG_S |
@@ -966,45 +961,6 @@ TEST_F(NameDhcpv4SrvTest, processRequestReleaseRemoveOn) {
                             time(NULL), subnet_->getValid(), true);
 }
 
-// Test that when a Release message is sent for a previously acquired
-// lease and DDNS updates are enabled but remove-on-renew is off that server
-// does NOT generate NameChangeRequest to remove entries corresponding to
-// the released lease.
-TEST_F(NameDhcpv4SrvTest, processRequestReleaseRemoveOff) {
-    // Verify the config is as expected.
-    ASSERT_TRUE(CfgMgr::instance().ddnsEnabled());
-    ASSERT_FALSE(CfgMgr::instance().getD2ClientConfig()->getRemoveOnRenew());
-
-    // Create and process a lease request so we have a lease to release.
-    Pkt4Ptr req = generatePktWithFqdn(DHCPREQUEST, Option4ClientFqdn::FLAG_S |
-                                      Option4ClientFqdn::FLAG_E,
-                                      "myhost.example.com.",
-                                      Option4ClientFqdn::FULL, true);
-    Pkt4Ptr reply;
-    ASSERT_NO_THROW(reply = srv_->processRequest(req));
-    checkResponse(reply, DHCPACK, 1234);
-
-    // Verify that there is one NameChangeRequest generated for the new lease.
-    ASSERT_EQ(1, srv_->name_change_reqs_.size());
-    verifyNameChangeRequest(isc::dhcp_ddns::CHG_ADD, true, true,
-                            reply->getYiaddr().toText(), "myhost.example.com.",
-                            "00010132E91AA355CFBB753C0F0497A5A940436"
-                            "965B68B6D438D98E680BF10B09F3BCF",
-                            time(NULL), subnet_->getValid(), true);
-
-    // Create and process the Release message.
-    Pkt4Ptr rel = Pkt4Ptr(new Pkt4(DHCPRELEASE, 1234));
-    rel->setCiaddr(reply->getYiaddr());
-    rel->setRemoteAddr(IOAddress("192.0.2.3"));
-    rel->addOption(generateClientId());
-    rel->addOption(srv_->getServerID());
-    ASSERT_NO_THROW(srv_->processRelease(rel));
-
-    // With remove-on-renew off, there should be not be a NameChangeRequest
-    // for the remove.
-    ASSERT_EQ(0, srv_->name_change_reqs_.size());
-}
-
 // Test that when the Release message is sent for a previously acquired lease
 // and DDNS updates are disabled that server does NOT generate a
 // NameChangeRequest to remove entries corresponding to the released lease.

src/lib/dhcpsrv/cfgmgr.cc

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above
@@ -368,8 +368,6 @@ CfgMgr::getD2ClientMgr() {
     return (d2_client_mgr_);
 }
 
-
-
 CfgMgr::CfgMgr()
     : datadir_(DHCP_DATA_DIR),
       all_ifaces_active_(false), echo_v4_client_id_(true),

src/lib/dhcpsrv/cfgmgr.h

@@ -1,4 +1,4 @@
-// Copyright (C) 2012-2013 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2012-2014 Internet Systems Consortium, Inc. ("ISC")
 //
 // Permission to use, copy, modify, and/or distribute this software for any
 // purpose with or without fee is hereby granted, provided that the above

src/lib/dhcpsrv/d2_client.cc

@@ -29,7 +29,6 @@ D2ClientConfig::D2ClientConfig(const  bool enable_updates,
                                      NameChangeProtocol& ncr_protocol,
                                const dhcp_ddns::
                                      NameChangeFormat& ncr_format,
-                               const bool remove_on_renew,
                                const bool always_include_fqdn,
                                const bool override_no_update,
                                const bool override_client_update,
@@ -41,7 +40,6 @@ D2ClientConfig::D2ClientConfig(const  bool enable_updates,
     server_port_(server_port),
     ncr_protocol_(ncr_protocol),
     ncr_format_(ncr_format),
-    remove_on_renew_(remove_on_renew),
     always_include_fqdn_(always_include_fqdn),
     override_no_update_(override_no_update),
     override_client_update_(override_client_update),
@@ -57,7 +55,6 @@ D2ClientConfig::D2ClientConfig()
       server_port_(0),
       ncr_protocol_(dhcp_ddns::NCR_UDP),
       ncr_format_(dhcp_ddns::FMT_JSON),
-      remove_on_renew_(false),
       always_include_fqdn_(false),
       override_no_update_(false),
       override_client_update_(false),
@@ -83,9 +80,8 @@ D2ClientConfig::validateContents() {
                     << " is not yet supported");
     }
 
-    // @todo perhaps more validation we should do yet?
-    // Are there any invalid combinations of options we need to test against?
-    // Also do we care about validating contents if it's disabled?
+    /// @todo perhaps more validation we should do yet?
+    /// Are there any invalid combinations of options we need to test against?
 }
 
 bool
@@ -95,7 +91,6 @@ D2ClientConfig::operator == (const D2ClientConfig& other) const {
             (server_port_ == other.server_port_) &&
             (ncr_protocol_ == other.ncr_protocol_) &&
             (ncr_format_ == other.ncr_format_) &&
-            (remove_on_renew_ == other.remove_on_renew_) &&
             (always_include_fqdn_ == other.always_include_fqdn_) &&
             (override_no_update_ == other.override_no_update_) &&
             (override_client_update_ == other.override_client_update_) &&
@@ -119,7 +114,6 @@ D2ClientConfig::toText() const {
                << ", server_port: " << server_port_
                << ", ncr_protocol: " << ncr_protocol_
                << ", ncr_format: " << ncr_format_
-               << ", remove_on_renew: " << (remove_on_renew_ ? "yes" : "no")
                << ", always_include_fqdn: " << (always_include_fqdn_ ?
                                                 "yes" : "no")
                << ", override_no_update: " << (override_no_update_ ?
@@ -189,24 +183,15 @@ D2ClientMgr::analyzeFqdn(const bool client_s, const bool client_n,
     //
     //  N flag  S flag   Option
     // ------------------------------------------------------------------
-    //    0       0      client wants to do forward updates (secion 3.2)
-    //    0       1      client wants server to do forward updates (secion 3.3)
+    //    0       0      client wants to do forward updates (section 3.2)
+    //    0       1      client wants server to do forward updates (section 3.3)
     //    1       0      client wants no one to do updates (section 3.4)
     //    1       1      invalid combination
+    // (Note section numbers cited are for 4702, for 4704 see 5.1, 5.2, and 5.3)
     //
     // Make a bit mask from the client's flags and use it to set the response
     // flags accordingly.
-    //
-    /// @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.
-    uint8_t mask = ((client_n ? 2 : 0) + (client_s ? 1 : 0));
+    const uint8_t mask = ((client_n ? 2 : 0) + (client_s ? 1 : 0));
 
     switch (mask) {
     case 0:
@@ -228,13 +213,21 @@ D2ClientMgr::analyzeFqdn(const bool client_s, const bool client_n,
         break;
 
     default:
-        // In theory we cannot get here because the fqdn option class defends
-        // against this combination.
-        isc_throw(isc::Unexpected,
+        // RFCs declare this an invalid combination.
+        isc_throw(isc::BadValue,
                   "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;
 }
 
@@ -246,8 +239,7 @@ D2ClientMgr::generateFqdn(const asiolink::IOAddress& address) const {
 
     std::ostringstream gen_name;
     gen_name << d2_client_config_->getGeneratedPrefix() << "-" << hostname;
-    std::string tmp = gen_name.str();
-    return (qualifyName(tmp));
+    return (qualifyName(gen_name.str()));
 }
 
 std::string
@@ -256,9 +248,9 @@ D2ClientMgr::qualifyName(const std::string& partial_name) const {
     gen_name << partial_name << "." << d2_client_config_->getQualifyingSuffix();
 
     // Tack on a trailing dot in case suffix doesn't have one.
-    const char* str = gen_name.str().c_str();
-    size_t len = strlen(str);
-    if ((len > 0) && (str[len-1] != '.')) {
+    std::string str = gen_name.str();
+    size_t len = str.length();
+    if ((len > 0) && (str[len - 1] != '.')) {
         gen_name << ".";
     }
 

src/lib/dhcpsrv/d2_client.h

@@ -64,11 +64,6 @@ public:
     /// Currently only UDP is supported.
     /// @param ncr_format Format of the b10-dhcp-ddns requests.
     /// Currently only JSON format is supported.
-    /// @param remove_on_renew Enables DNS Removes when renewing a lease
-    /// If true, Kea should request an explicit DNS remove prior to requesting
-    /// a DNS update when renewing a lease.
-    /// (Note: b10-dhcp-ddns is implemented per RFC 4703 and such a remove
-    /// is unnecessary).
     /// @param always_include_fqdn Enables always including the FQDN option in
     /// DHCP responses.
     /// @param override_no_update Enables updates, even if clients request no
@@ -86,7 +81,6 @@ public:
                    const size_t server_port,
                    const dhcp_ddns::NameChangeProtocol& ncr_protocol,
                    const dhcp_ddns::NameChangeFormat& ncr_format,
-                   const bool remove_on_renew,
                    const bool always_include_fqdn,
                    const bool override_no_update,
                    const bool override_client_update,
@@ -126,11 +120,6 @@ public:
         return(ncr_format_);
     }
 
-    /// @brief Return whether or not removes should be sent for lease renewals.
-    bool getRemoveOnRenew() const {
-        return(remove_on_renew_);
-    }
-
     /// @brief Return whether or not FQDN is always included in DHCP responses.
     bool getAlwaysIncludeFqdn() const {
         return(always_include_fqdn_);
@@ -196,13 +185,6 @@ private:
     /// Currently only JSON format is supported.
     dhcp_ddns::NameChangeFormat ncr_format_;
 
-    /// @brief Should Kea request a DNS Remove when renewing a lease.
-    /// If true, Kea should request an explicit DNS remove prior to requesting
-    /// a DNS update when renewing a lease.
-    /// (Note: b10-dhcp-ddns is implemented per RFC 4703 and such a remove
-    /// is unnecessary).
-    bool remove_on_renew_;
-
     /// @brief Should Kea always include the FQDN option in its response.
     bool always_include_fqdn_;
 
@@ -269,11 +251,15 @@ 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.
     ///
     /// @param client_s  S Flag from the client's FQDN
     /// @param client_n  N Flag from the client's FQDN
-    /// @param server_s  S Flag for the server's FQDN  (output)
-    /// @param server_n  N Flag for the server's FQDN (output)
+    /// @param server_s [out] S Flag for the server's FQDN
+    /// @param server_n [out] N Flag for the server's FQDN
+    ///
+    /// @throw isc::BadValue if client_s and client_n are both 1 as this is
+    /// an invalid combination per RFCs.
     void analyzeFqdn(const bool client_s, const bool client_n, bool& server_s,
                      bool& server_n) const;
 
@@ -311,12 +297,16 @@ public:
     /// @brief Set server FQDN flags based on configuration and a given FQDN
     ///
     /// Templated wrapper around the analyzeFqdn() allowing that method to
-    /// be used for either IPv4 or IPv6 processing.
+    /// be used for either IPv4 or IPv6 processing.  This methods resets all
+    /// of the flags in the response to zero and then sets the S,N, and O
+    /// flags.  Any other flags are the responsiblity of the invoking layer.
     ///
     /// @param fqdn FQDN option from which to read client (inbound) flags
     /// @param fqdn_resp FQDN option to update with the server (outbound) flags
-    template <class OPT>
-    void adjustFqdnFlags(OPT& fqdn, OPT& fqdn_resp);
+    /// @tparam T FQDN Option class containing the FQDN data such as
+    /// dhcp::Option4ClientFqdn or dhcp::Option6ClientFqdn
+    template <class T>
+    void adjustFqdnFlags(const T& fqdn, T& fqdn_resp);
 
     /// @brief Set server FQDN name based on configuration and a given FQDN
     ///
@@ -336,47 +326,48 @@ public:
     ///
     /// @param fqdn FQDN option from which to get client (inbound) name
     /// @param fqdn_resp FQDN option to update with the adjusted name
-    template <class OPT>
-    void adjustDomainName(OPT& fqdn, OPT& fqdn_resp);
+    /// @tparam T  FQDN Option class containing the FQDN data such as
+    /// dhcp::Option4ClientFqdn or dhcp::Option6ClientFqdn
+    template <class T>
+    void adjustDomainName(const T& fqdn, T& fqdn_resp);
 
 private:
     /// @brief Container class for DHCP-DDNS configuration parameters.
     D2ClientConfigPtr d2_client_config_;
 };
 
-template <class OPT>
+template <class T>
 void
-D2ClientMgr::adjustFqdnFlags(OPT& fqdn, OPT& fqdn_resp) {
+D2ClientMgr::adjustFqdnFlags(const T& fqdn, T& fqdn_resp) {
     bool server_s = false;
     bool server_n = false;
-    analyzeFqdn(fqdn.getFlag(OPT::FLAG_S), fqdn.getFlag(OPT::FLAG_N),
+    analyzeFqdn(fqdn.getFlag(T::FLAG_S), fqdn.getFlag(T::FLAG_N),
                 server_s, server_n);
 
     // Reset the flags to zero to avoid triggering N and S both 1 check.
     fqdn_resp.resetFlags();
 
     // Set S and N flags.
-    fqdn_resp.setFlag(OPT::FLAG_S, server_s);
-    fqdn_resp.setFlag(OPT::FLAG_N, server_n);
+    fqdn_resp.setFlag(T::FLAG_S, server_s);
+    fqdn_resp.setFlag(T::FLAG_N, server_n);
 
     // Set O flag true if server S overrides client S.
-    fqdn_resp.setFlag(OPT::FLAG_O, (fqdn.getFlag(OPT::FLAG_S) != server_s));
+    fqdn_resp.setFlag(T::FLAG_O, (fqdn.getFlag(T::FLAG_S) != server_s));
 }
 
 
-template <class OPT>
+template <class T>
 void
-D2ClientMgr::adjustDomainName(OPT& fqdn, OPT& fqdn_resp) {
+D2ClientMgr::adjustDomainName(const T& fqdn, T& fqdn_resp) {
     // If we're configured to replace it or the supplied name is blank
     // set the response name to blank.
     if (d2_client_config_->getReplaceClientName() ||
         fqdn.getDomainName().empty()) {
-        fqdn_resp.setDomainName("", OPT::PARTIAL);
+        fqdn_resp.setDomainName("", T::PARTIAL);
     } else {
         // If the supplied name is partial, qualify it by adding the suffix.
-        if (fqdn.getDomainNameType() == OPT::PARTIAL) {
-            fqdn_resp.setDomainName(qualifyName(fqdn.getDomainName()),
-                                    OPT::FULL);
+        if (fqdn.getDomainNameType() == T::PARTIAL) {
+            fqdn_resp.setDomainName(qualifyName(fqdn.getDomainName()), T::FULL);
         }
     }
 }

src/lib/dhcpsrv/dhcp_parsers.cc

@@ -1208,7 +1208,6 @@ D2ClientConfigParser::build(isc::data::ConstElementPtr client_config) {
     std::string qualifying_suffix = string_values_->
                                     getParam("qualifying-suffix");
 
-    bool remove_on_renew = boolean_values_->getParam("remove-on-renew");
     bool always_include_fqdn = boolean_values_->getParam("always-include-fqdn");
     bool override_no_update = boolean_values_->getParam("override-no-update");
     bool override_client_update = boolean_values_->
@@ -1218,7 +1217,7 @@ D2ClientConfigParser::build(isc::data::ConstElementPtr client_config) {
     // Attempt to create the new client config.
     local_client_config_.reset(new D2ClientConfig(enable_updates, server_ip,
                                                   server_port, ncr_protocol,
-                                                  ncr_format, remove_on_renew,
+                                                  ncr_format,
                                                   always_include_fqdn,
                                                   override_no_update,
                                                   override_client_update,
@@ -1239,7 +1238,6 @@ D2ClientConfigParser::createConfigParser(const std::string& config_id) {
         (config_id.compare("qualifying-suffix") == 0)) {
         parser = new StringParser(config_id, string_values_);
     } else if ((config_id.compare("enable-updates") == 0) ||
-        (config_id.compare("remove-on-renew") == 0) ||
         (config_id.compare("always-include-fqdn") == 0) ||
         (config_id.compare("allow-client-update") == 0) ||
         (config_id.compare("override-no-update") == 0) ||

src/lib/dhcpsrv/tests/cfgmgr_unittest.cc

@@ -691,7 +691,7 @@ TEST_F(CfgMgrTest, d2ClientConfig) {
     ASSERT_NO_THROW(new_cfg.reset(new D2ClientConfig(true,
                                   isc::asiolink::IOAddress("127.0.0.1"), 477,
                                   dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                                  true, true, true, true, true,
+                                  true, true, true, true,
                                   "pre-fix", "suf-fix")));
 
     // Verify that we can assign a new, non-empty configuration.

src/lib/dhcpsrv/tests/d2_client_unittest.cc

@@ -43,7 +43,6 @@ TEST(D2ClientConfigTest, constructorsAndAccessors) {
     size_t server_port = 477;
     dhcp_ddns::NameChangeProtocol ncr_protocol = dhcp_ddns::NCR_UDP;
     dhcp_ddns::NameChangeFormat ncr_format = dhcp_ddns::FMT_JSON;
-    bool remove_on_renew = true;
     bool always_include_fqdn = true;
     bool override_no_update = true;
     bool override_client_update = true;
@@ -58,7 +57,6 @@ TEST(D2ClientConfigTest, constructorsAndAccessors) {
                                                           server_port,
                                                           ncr_protocol,
                                                           ncr_format,
-                                                          remove_on_renew,
                                                           always_include_fqdn,
                                                           override_no_update,
                                                          override_client_update,
@@ -75,7 +73,6 @@ TEST(D2ClientConfigTest, constructorsAndAccessors) {
     EXPECT_EQ(d2_client_config->getServerPort(), server_port);
     EXPECT_EQ(d2_client_config->getNcrProtocol(), ncr_protocol);
     EXPECT_EQ(d2_client_config->getNcrFormat(), ncr_format);
-    EXPECT_EQ(d2_client_config->getRemoveOnRenew(), remove_on_renew);
     EXPECT_EQ(d2_client_config->getAlwaysIncludeFqdn(), always_include_fqdn);
     EXPECT_EQ(d2_client_config->getOverrideNoUpdate(), override_no_update);
     EXPECT_EQ(d2_client_config->getOverrideClientUpdate(),
@@ -96,7 +93,6 @@ TEST(D2ClientConfigTest, constructorsAndAccessors) {
                                                        server_port,
                                                        dhcp_ddns::NCR_TCP,
                                                        ncr_format,
-                                                       remove_on_renew,
                                                        always_include_fqdn,
                                                        override_no_update,
                                                        override_client_update,
@@ -121,7 +117,7 @@ TEST(D2ClientConfigTest, equalityOperator) {
     ASSERT_NO_THROW(ref_config.reset(new D2ClientConfig(true,
                     ref_address, 477,
                     dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                    true, true, true, true, true,
+                    true, true, true, true,
                     "pre-fix", "suf-fix")));
     ASSERT_TRUE(ref_config);
 
@@ -129,7 +125,7 @@ TEST(D2ClientConfigTest, equalityOperator) {
     ASSERT_NO_THROW(test_config.reset(new D2ClientConfig(true,
                     ref_address, 477,
                     dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                    true, true, true, true, true,
+                    true, true, true, true,
                     "pre-fix", "suf-fix")));
     ASSERT_TRUE(test_config);
     EXPECT_TRUE(*ref_config == *test_config);
@@ -139,7 +135,7 @@ TEST(D2ClientConfigTest, equalityOperator) {
     ASSERT_NO_THROW(test_config.reset(new D2ClientConfig(false,
                     ref_address, 477,
                     dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                    true, true, true, true, true,
+                    true, true, true, true,
                     "pre-fix", "suf-fix")));
     ASSERT_TRUE(test_config);
     EXPECT_FALSE(*ref_config == *test_config);
@@ -149,7 +145,7 @@ TEST(D2ClientConfigTest, equalityOperator) {
     ASSERT_NO_THROW(test_config.reset(new D2ClientConfig(true,
                     test_address, 477,
                     dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                    true, true, true, true, true,
+                    true, true, true, true,
                     "pre-fix", "suf-fix")));
     ASSERT_TRUE(test_config);
     EXPECT_FALSE(*ref_config == *test_config);
@@ -159,17 +155,7 @@ TEST(D2ClientConfigTest, equalityOperator) {
     ASSERT_NO_THROW(test_config.reset(new D2ClientConfig(true,
                     ref_address, 333,
                     dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                    true, true, true, true, true,
-                    "pre-fix", "suf-fix")));
-    ASSERT_TRUE(test_config);
-    EXPECT_FALSE(*ref_config == *test_config);
-    EXPECT_TRUE(*ref_config != *test_config);
-
-    // Check a configuration that differs only by remove_on_renew.
-    ASSERT_NO_THROW(test_config.reset(new D2ClientConfig(true,
-                    ref_address, 477,
-                    dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                    false, true, true, true, true,
+                    true, true, true, true,
                     "pre-fix", "suf-fix")));
     ASSERT_TRUE(test_config);
     EXPECT_FALSE(*ref_config == *test_config);
@@ -179,7 +165,7 @@ TEST(D2ClientConfigTest, equalityOperator) {
     ASSERT_NO_THROW(test_config.reset(new D2ClientConfig(true,
                     ref_address, 477,
                     dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                    true, false, true, true, true,
+                    false, true, true, true,
                     "pre-fix", "suf-fix")));
     ASSERT_TRUE(test_config);
     EXPECT_FALSE(*ref_config == *test_config);
@@ -189,7 +175,7 @@ TEST(D2ClientConfigTest, equalityOperator) {
     ASSERT_NO_THROW(test_config.reset(new D2ClientConfig(true,
                     ref_address, 477,
                     dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                    true, true, false, true, true,
+                    true, false, true, true,
                     "pre-fix", "suf-fix")));
     ASSERT_TRUE(test_config);
     EXPECT_FALSE(*ref_config == *test_config);
@@ -199,7 +185,7 @@ TEST(D2ClientConfigTest, equalityOperator) {
     ASSERT_NO_THROW(test_config.reset(new D2ClientConfig(true,
                     ref_address, 477,
                     dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                    true, true, true, false, true,
+                    true, true, false, true,
                     "pre-fix", "suf-fix")));
     ASSERT_TRUE(test_config);
     EXPECT_FALSE(*ref_config == *test_config);
@@ -209,7 +195,7 @@ TEST(D2ClientConfigTest, equalityOperator) {
     ASSERT_NO_THROW(test_config.reset(new D2ClientConfig(true,
                     ref_address, 477,
                     dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                    true, true, true, true, false,
+                    true, true, true, false,
                     "pre-fix", "suf-fix")));
     ASSERT_TRUE(test_config);
     EXPECT_FALSE(*ref_config == *test_config);
@@ -219,7 +205,7 @@ TEST(D2ClientConfigTest, equalityOperator) {
     ASSERT_NO_THROW(test_config.reset(new D2ClientConfig(true,
                     ref_address, 477,
                     dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                    true, true, true, true, true,
+                    true, true, true, true,
                     "bogus", "suf-fix")));
     ASSERT_TRUE(test_config);
     EXPECT_FALSE(*ref_config == *test_config);
@@ -229,7 +215,7 @@ TEST(D2ClientConfigTest, equalityOperator) {
     ASSERT_NO_THROW(test_config.reset(new D2ClientConfig(true,
                     ref_address, 477,
                     dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                    true, true, true, true, true,
+                    true, true, true, true,
                     "pre-fix", "bogus")));
     ASSERT_TRUE(test_config);
     EXPECT_FALSE(*ref_config == *test_config);
@@ -272,7 +258,7 @@ TEST(D2ClientMgr, validConfig) {
     ASSERT_NO_THROW(new_cfg.reset(new D2ClientConfig(true,
                                   isc::asiolink::IOAddress("127.0.0.1"), 477,
                                   dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                                  true, true, true, true, true,
+                                  true, true, true, true,
                                   "pre-fix", "suf-fix")));
 
     // Verify that we can assign a new, non-empty configuration.
@@ -292,9 +278,10 @@ TEST(D2ClientMgr, validConfig) {
     EXPECT_NE(*original_config, *updated_config);
 }
 
-/// @brief Tests that analyzeFqdn generates correct server S and N flags when
-/// updates are disabled.
-TEST(D2ClientMgr, analyzeFqdnUpdatesDisabled) {
+
+/// @brief Tests that analyzeFqdn detects invalid combination of both the
+/// client S and N flags set to true.
+TEST(D2ClientMgr, analyzeFqdnInvalidCombination) {
     D2ClientMgr mgr;
     bool server_s = false;
     bool server_n = false;
@@ -304,29 +291,23 @@ TEST(D2ClientMgr, analyzeFqdnUpdatesDisabled) {
     ASSERT_NO_THROW(cfg.reset(new D2ClientConfig()));
     ASSERT_NO_THROW(mgr.setD2ClientConfig(cfg));
     ASSERT_FALSE(mgr.ddnsEnabled());
-    ASSERT_FALSE(cfg->getOverrideClientUpdate());
-    ASSERT_FALSE(cfg->getOverrideNoUpdate());
 
-    // 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)
-    mgr.analyzeFqdn(false, false, server_s, server_n);
-    EXPECT_FALSE(server_s);
-    EXPECT_TRUE(server_n);
+    // client S=1 N=1 is invalid.  analyzeFqdn should throw.
+    ASSERT_THROW(mgr.analyzeFqdn(true, true, server_s, server_n),
+                 isc::BadValue);
 
-    // client S=1 N=0 means client wants server 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)
-    mgr.analyzeFqdn(true, false, server_s, server_n);
-    EXPECT_FALSE(server_s);
-    EXPECT_TRUE(server_n);
+    // Create enabled configuration with all controls off (no overrides).
+    ASSERT_NO_THROW(cfg.reset(new D2ClientConfig(true,
+                                  isc::asiolink::IOAddress("127.0.0.1"), 477,
+                                  dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
+                                  false, false, false, false,
+                                  "pre-fix", "suf-fix")));
+    ASSERT_NO_THROW(mgr.setD2ClientConfig(cfg));
+    ASSERT_TRUE(mgr.ddnsEnabled());
 
-    // client S=0 N=1 means client wants no one to do forward updates.
-    // server S should be 0 (server is  not forward updates)
-    // and server N should be 1 (server is not doing any updates)
-    mgr.analyzeFqdn(false, true, server_s, server_n);
-    EXPECT_FALSE(server_s);
-    EXPECT_TRUE(server_n);
+    // client S=1 N=1 is invalid.  analyzeFqdn should throw.
+    ASSERT_THROW(mgr.analyzeFqdn(true, true, server_s, server_n),
+                 isc::BadValue);
 }
 
 /// @brief Tests that analyzeFqdn generates correct server S and N flags when
@@ -341,7 +322,7 @@ TEST(D2ClientMgr, analyzeFqdnEnabledNoOverrides) {
     ASSERT_NO_THROW(cfg.reset(new D2ClientConfig(true,
                                   isc::asiolink::IOAddress("127.0.0.1"), 477,
                                   dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                                  false, false, false, false, false,
+                                  false, false, false, false,
                                   "pre-fix", "suf-fix")));
     ASSERT_NO_THROW(mgr.setD2ClientConfig(cfg));
     ASSERT_TRUE(mgr.ddnsEnabled());
@@ -383,7 +364,7 @@ TEST(D2ClientMgr, analyzeFqdnEnabledOverrideNoUpdate) {
     ASSERT_NO_THROW(cfg.reset(new D2ClientConfig(true,
                                   isc::asiolink::IOAddress("127.0.0.1"), 477,
                                   dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                                  false, false, true, false, false,
+                                  false, true, false, false,
                                   "pre-fix", "suf-fix")));
     ASSERT_NO_THROW(mgr.setD2ClientConfig(cfg));
     ASSERT_TRUE(mgr.ddnsEnabled());
@@ -424,7 +405,7 @@ TEST(D2ClientMgr, analyzeFqdnEnabledOverrideClientUpdate) {
     ASSERT_NO_THROW(cfg.reset(new D2ClientConfig(true,
                                   isc::asiolink::IOAddress("127.0.0.1"), 477,
                                   dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                                  false, false, false, true, false,
+                                  false, false, true, false,
                                   "pre-fix", "suf-fix")));
     ASSERT_NO_THROW(mgr.setD2ClientConfig(cfg));
     ASSERT_TRUE(mgr.ddnsEnabled());
@@ -466,7 +447,7 @@ TEST(D2ClientMgr, adjustFqdnFlagsV4) {
     ASSERT_NO_THROW(cfg.reset(new D2ClientConfig(true,
                                   isc::asiolink::IOAddress("127.0.0.1"), 477,
                                   dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                                  false, false, true, false, false,
+                                  false, true, false, false,
                                   "pre-fix", "suf-fix")));
     ASSERT_NO_THROW(mgr.setD2ClientConfig(cfg));
     ASSERT_TRUE(mgr.ddnsEnabled());
@@ -527,7 +508,7 @@ TEST(D2ClientMgr, qualifyName) {
     ASSERT_NO_THROW(cfg.reset(new D2ClientConfig(true,
                                   isc::asiolink::IOAddress("127.0.0.1"), 477,
                                   dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                                  false, false, false, true, false,
+                                  false, false, true, false,
                                   "prefix", "suffix.com")));
     ASSERT_NO_THROW(mgr.setD2ClientConfig(cfg));
 
@@ -539,7 +520,7 @@ TEST(D2ClientMgr, qualifyName) {
     ASSERT_NO_THROW(cfg.reset(new D2ClientConfig(true,
                                   isc::asiolink::IOAddress("127.0.0.1"), 477,
                                   dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                                  false, false, false, true, false,
+                                  false, false, true, false,
                                   "prefix", "hasdot.com.")));
     ASSERT_NO_THROW(mgr.setD2ClientConfig(cfg));
 
@@ -558,7 +539,7 @@ TEST(D2ClientMgr, generateFqdn) {
     ASSERT_NO_THROW(cfg.reset(new D2ClientConfig(true,
                                   isc::asiolink::IOAddress("127.0.0.1"), 477,
                                   dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                                  false, false, false, true, false,
+                                  false, false, true, false,
                                   "prefix", "suffix.com")));
     ASSERT_NO_THROW(mgr.setD2ClientConfig(cfg));
 
@@ -590,7 +571,7 @@ TEST(D2ClientMgr, adjustDomainNameV4) {
     ASSERT_NO_THROW(cfg.reset(new D2ClientConfig(true,
                                   isc::asiolink::IOAddress("127.0.0.1"), 477,
                                   dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                                  false, false, false, false, false,
+                                  false, false, false, false,
                                   "prefix", "suffix.com")));
     ASSERT_NO_THROW(mgr.setD2ClientConfig(cfg));
     ASSERT_FALSE(cfg->getReplaceClientName());
@@ -631,7 +612,7 @@ TEST(D2ClientMgr, adjustDomainNameV4) {
     ASSERT_NO_THROW(cfg.reset(new D2ClientConfig(true,
                                   isc::asiolink::IOAddress("127.0.0.1"), 477,
                                   dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                                  false, false, false, false, true,
+                                  false, false, false, true,
                                   "prefix", "suffix.com")));
     ASSERT_NO_THROW(mgr.setD2ClientConfig(cfg));
     ASSERT_TRUE(cfg->getReplaceClientName());
@@ -679,7 +660,7 @@ TEST(D2ClientMgr, adjustDomainNameV6) {
     ASSERT_NO_THROW(cfg.reset(new D2ClientConfig(true,
                                   isc::asiolink::IOAddress("127.0.0.1"), 477,
                                   dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                                  false, false, false, false, false,
+                                  false, false, false, false,
                                   "prefix", "suffix.com")));
     ASSERT_NO_THROW(mgr.setD2ClientConfig(cfg));
     ASSERT_FALSE(cfg->getReplaceClientName());
@@ -717,7 +698,7 @@ TEST(D2ClientMgr, adjustDomainNameV6) {
     ASSERT_NO_THROW(cfg.reset(new D2ClientConfig(true,
                                   isc::asiolink::IOAddress("127.0.0.1"), 477,
                                   dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                                  false, false, false, false, true,
+                                  false, false, false, true,
                                   "prefix", "suffix.com")));
     ASSERT_NO_THROW(mgr.setD2ClientConfig(cfg));
     ASSERT_TRUE(cfg->getReplaceClientName());
@@ -765,7 +746,7 @@ TEST(D2ClientMgr, adjustFqdnFlagsV6) {
     ASSERT_NO_THROW(cfg.reset(new D2ClientConfig(true,
                                   isc::asiolink::IOAddress("127.0.0.1"), 477,
                                   dhcp_ddns::NCR_UDP, dhcp_ddns::FMT_JSON,
-                                  false, false, true, false, false,
+                                  false, true, false, false,
                                   "pre-fix", "suf-fix")));
     ASSERT_NO_THROW(mgr.setD2ClientConfig(cfg));
     ASSERT_TRUE(mgr.ddnsEnabled());

src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc

@@ -720,7 +720,6 @@ TEST_F(ParseConfigTest, validD2Config) {
         "     \"server-port\" : 3432, "
         "     \"ncr-protocol\" : \"UDP\", "
         "     \"ncr-format\" : \"JSON\", "
-        "     \"remove-on-renew\" : true, "
         "     \"always-include-fqdn\" : true, "
         "     \"override-no-update\" : true, "
         "     \"override-client-update\" : true, "
@@ -746,7 +745,6 @@ TEST_F(ParseConfigTest, validD2Config) {
     EXPECT_EQ(3432, d2_client_config->getServerPort());
     EXPECT_EQ(dhcp_ddns::NCR_UDP, d2_client_config->getNcrProtocol());
     EXPECT_EQ(dhcp_ddns::FMT_JSON, d2_client_config->getNcrFormat());
-    EXPECT_TRUE(d2_client_config->getRemoveOnRenew());
     EXPECT_TRUE(d2_client_config->getAlwaysIncludeFqdn());
     EXPECT_TRUE(d2_client_config->getOverrideNoUpdate());
     EXPECT_TRUE(d2_client_config->getOverrideClientUpdate());
@@ -765,7 +763,6 @@ TEST_F(ParseConfigTest, validD2Config) {
         "     \"server-port\" : 43567, "
         "     \"ncr-protocol\" : \"UDP\", "
         "     \"ncr-format\" : \"JSON\", "
-        "     \"remove-on-renew\" : false, "
         "     \"always-include-fqdn\" : false, "
         "     \"override-no-update\" : false, "
         "     \"override-client-update\" : false, "
@@ -790,7 +787,6 @@ TEST_F(ParseConfigTest, validD2Config) {
     EXPECT_EQ(43567, d2_client_config->getServerPort());
     EXPECT_EQ(dhcp_ddns::NCR_UDP, d2_client_config->getNcrProtocol());
     EXPECT_EQ(dhcp_ddns::FMT_JSON, d2_client_config->getNcrFormat());
-    EXPECT_FALSE(d2_client_config->getRemoveOnRenew());
     EXPECT_FALSE(d2_client_config->getAlwaysIncludeFqdn());
     EXPECT_FALSE(d2_client_config->getOverrideNoUpdate());
     EXPECT_FALSE(d2_client_config->getOverrideClientUpdate());
@@ -842,7 +838,6 @@ TEST_F(ParseConfigTest, invalidD2Config) {
         "     \"server-port\" : 53001, "
         "     \"ncr-protocol\" : \"UDP\", "
         "     \"ncr-format\" : \"JSON\", "
-        "     \"remove-on-renew\" : true, "
         "     \"always-include-fqdn\" : true, "
         "     \"override-no-update\" : true, "
         "     \"override-client-update\" : true, "
@@ -859,7 +854,6 @@ TEST_F(ParseConfigTest, invalidD2Config) {
         "     \"server-port\" : 53001, "
         "     \"ncr-protocol\" : \"UDP\", "
         "     \"ncr-format\" : \"JSON\", "
-        "     \"remove-on-renew\" : true, "
         "     \"always-include-fqdn\" : true, "
         "     \"override-no-update\" : true, "
         "     \"override-client-update\" : true, "
@@ -876,7 +870,6 @@ TEST_F(ParseConfigTest, invalidD2Config) {
         "     \"server-port\" : 53001, "
         "     \"ncr-protocol\" : \"Bogus\", "
         "     \"ncr-format\" : \"JSON\", "
-        "     \"remove-on-renew\" : true, "
         "     \"always-include-fqdn\" : true, "
         "     \"override-no-update\" : true, "
         "     \"override-client-update\" : true, "
@@ -893,7 +886,6 @@ TEST_F(ParseConfigTest, invalidD2Config) {
         "     \"server-port\" : 53001, "
         "     \"ncr-protocol\" : \"TCP\", "
         "     \"ncr-format\" : \"JSON\", "
-        "     \"remove-on-renew\" : true, "
         "     \"always-include-fqdn\" : true, "
         "     \"override-no-update\" : true, "
         "     \"override-client-update\" : true, "
@@ -910,7 +902,6 @@ TEST_F(ParseConfigTest, invalidD2Config) {
         "     \"server-port\" : 53001, "
         "     \"ncr-protocol\" : \"UDP\", "
         "     \"ncr-format\" : \"Bogus\", "
-        "     \"remove-on-renew\" : true, "
         "     \"always-include-fqdn\" : true, "
         "     \"override-no-update\" : true, "
         "     \"override-client-update\" : true, "
@@ -927,7 +918,6 @@ TEST_F(ParseConfigTest, invalidD2Config) {
         // "     \"server-port\" : 53001, "
         "     \"ncr-protocol\" : \"UDP\", "
         "     \"ncr-format\" : \"JSON\", "
-        "     \"remove-on-renew\" : true, "
         "     \"always-include-fqdn\" : true, "
         "     \"override-no-update\" : true, "
         "     \"override-client-update\" : true, "