From: Thomas Markwalder Date: Wed, 29 Jan 2014 14:51:59 +0000 (-0500) Subject: [3282] Addressed review comments. X-Git-Tag: bind10-1.2.0beta1-release~91^2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=587c50528ab9c42467fc03327dfdbadd8933e360;p=thirdparty%2Fkea.git [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. --- diff --git a/src/bin/dhcp4/dhcp4.spec b/src/bin/dhcp4/dhcp4.spec index c6d2c32b80..04bcc7ac61 100644 --- a/src/bin/dhcp4/dhcp4.spec +++ b/src/bin/dhcp4/dhcp4.spec @@ -335,13 +335,6 @@ "item_default": "JSON", "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", diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 4c061fa889..49231d4e1c 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/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); } diff --git a/src/bin/dhcp4/tests/config_parser_unittest.cc b/src/bin/dhcp4/tests/config_parser_unittest.cc index 14c4f8065f..55fabd4f70 100644 --- a/src/bin/dhcp4/tests/config_parser_unittest.cc +++ b/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, " diff --git a/src/bin/dhcp4/tests/fqdn_unittest.cc b/src/bin/dhcp4/tests/fqdn_unittest.cc index 73f15a760d..e17a7e3ac6 100644 --- a/src/bin/dhcp4/tests/fqdn_unittest.cc +++ b/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. diff --git a/src/lib/dhcpsrv/cfgmgr.cc b/src/lib/dhcpsrv/cfgmgr.cc index b9e48c6e51..8d41a15320 100644 --- a/src/lib/dhcpsrv/cfgmgr.cc +++ b/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), diff --git a/src/lib/dhcpsrv/cfgmgr.h b/src/lib/dhcpsrv/cfgmgr.h index 0867826045..18455d08f2 100644 --- a/src/lib/dhcpsrv/cfgmgr.h +++ b/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 diff --git a/src/lib/dhcpsrv/d2_client.cc b/src/lib/dhcpsrv/d2_client.cc index 031066d1a8..419c5ce935 100644 --- a/src/lib/dhcpsrv/d2_client.cc +++ b/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 << "."; } diff --git a/src/lib/dhcpsrv/d2_client.h b/src/lib/dhcpsrv/d2_client.h index d179387db3..0a6faa4713 100644 --- a/src/lib/dhcpsrv/d2_client.h +++ b/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 - void adjustFqdnFlags(OPT& fqdn, OPT& fqdn_resp); + /// @tparam T FQDN Option class containing the FQDN data such as + /// dhcp::Option4ClientFqdn or dhcp::Option6ClientFqdn + template + 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 - void adjustDomainName(OPT& fqdn, OPT& fqdn_resp); + /// @tparam T FQDN Option class containing the FQDN data such as + /// dhcp::Option4ClientFqdn or dhcp::Option6ClientFqdn + template + void adjustDomainName(const T& fqdn, T& fqdn_resp); private: /// @brief Container class for DHCP-DDNS configuration parameters. D2ClientConfigPtr d2_client_config_; }; -template +template 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 +template 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); } } } diff --git a/src/lib/dhcpsrv/dhcp_parsers.cc b/src/lib/dhcpsrv/dhcp_parsers.cc index 3525185f90..8acc10b527 100644 --- a/src/lib/dhcpsrv/dhcp_parsers.cc +++ b/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) || diff --git a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc b/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc index 4a0ccc447f..e13cb320aa 100644 --- a/src/lib/dhcpsrv/tests/cfgmgr_unittest.cc +++ b/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. diff --git a/src/lib/dhcpsrv/tests/d2_client_unittest.cc b/src/lib/dhcpsrv/tests/d2_client_unittest.cc index e33dc8e8ec..8a8550fbe2 100644 --- a/src/lib/dhcpsrv/tests/d2_client_unittest.cc +++ b/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()); diff --git a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc b/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc index b615cb6fab..78e3442273 100644 --- a/src/lib/dhcpsrv/tests/dhcp_parsers_unittest.cc +++ b/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, "