From 88da7d9cee5025fdbe7003312c2aec4c6c021dad Mon Sep 17 00:00:00 2001 From: Marcin Siodelski Date: Wed, 27 Apr 2016 09:22:29 +0200 Subject: [PATCH] [4303] Addressed remaining review comments. --- src/bin/dhcp4/dhcp4_srv.cc | 97 ++++++++++++------- src/bin/dhcp4/dhcp4_srv.h | 6 ++ src/bin/dhcp6/dhcp6_srv.cc | 40 +++++--- src/lib/dhcpsrv/alloc_engine.cc | 40 +++----- src/lib/dhcpsrv/alloc_engine.h | 36 +++++-- src/lib/dhcpsrv/cfg_host_operations.cc | 2 +- src/lib/dhcpsrv/cfg_host_operations.h | 4 +- .../parsers/host_reservation_parser.cc | 4 +- .../dhcpsrv/tests/alloc_engine4_unittest.cc | 4 +- src/lib/dhcpsrv/tests/alloc_engine_utils.cc | 2 +- .../tests/cfg_host_operations_unittest.cc | 59 +++++++---- 11 files changed, 185 insertions(+), 109 deletions(-) diff --git a/src/bin/dhcp4/dhcp4_srv.cc b/src/bin/dhcp4/dhcp4_srv.cc index 85bf972f85..0cc74195f8 100644 --- a/src/bin/dhcp4/dhcp4_srv.cc +++ b/src/bin/dhcp4/dhcp4_srv.cc @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -137,47 +138,17 @@ Dhcpv4Exchange::Dhcpv4Exchange(const AllocEnginePtr& alloc_engine, .arg(query->getLabel()) .arg(subnet->getID()); } - } - - // Before we can check for static reservations, we need to prepare a set - // of identifiers to be used for this. - // HW address. - if (context_->hwaddr_ && !context_->hwaddr_->hwaddr_.empty()) { - context_->host_identifiers_[Host::IDENT_HWADDR] = context_->hwaddr_->hwaddr_; - } + // Find static reservations if not disabled for our subnet. + if (subnet->getHostReservationMode() != Subnet::HR_DISABLED) { + // Before we can check for static reservations, we need to prepare a set + // of identifiers to be used for this. + setHostIdentifiers(); - // Client identifier - if (context_->clientid_) { - const std::vector& vec = context_->clientid_->getDuid(); - if (!vec.empty()) { - // Client identifier type = DUID? Client identifier holding a DUID - // comprises Type (1 byte), IAID (4 bytes), followed by the actual - // DUID. Thus, the minimal length is 6. - if ((vec[0] == CLIENT_ID_OPTION_TYPE_DUID) && (vec.size() > 5)) { - // Extract DUID, skip IAID. - context_->host_identifiers_.insert( - AllocEngine::IdentifierPair(Host::IDENT_DUID, - std::vector(vec.begin() + 5, - vec.end()))); - } - /// @todo Add support for other client identifiers (see #4317). + // Check for static reservations. + alloc_engine->findReservation(*context_); } } - // Circuit id - OptionPtr rai = query_->getOption(DHO_DHCP_AGENT_OPTIONS); - if (rai) { - OptionPtr circuit_id_opt = rai->getOption(RAI_OPTION_AGENT_CIRCUIT_ID); - if (circuit_id_opt) { - const OptionBuffer& circuit_id_vec = circuit_id_opt->getData(); - if (!circuit_id_vec.empty()) { - context_->host_identifiers_[Host::IDENT_CIRCUIT_ID] = circuit_id_vec; - } - } - } - - // Check for static reservations. - alloc_engine->findReservation(*context_); }; void @@ -270,6 +241,58 @@ Dhcpv4Exchange::copyDefaultOptions() { } } +void +Dhcpv4Exchange::setHostIdentifiers() { + const ConstCfgHostOperationsPtr cfg = + CfgMgr::instance().getCurrentCfg()->getCfgHostOperations4(); + BOOST_FOREACH(const Host::IdentifierType& id_type, + cfg->getIdentifierTypes()) { + switch (id_type) { + case Host::IDENT_HWADDR: + if (context_->hwaddr_ && !context_->hwaddr_->hwaddr_.empty()) { + context_->addHostIdentifier(id_type, context_->hwaddr_->hwaddr_); + } + break; + + case Host::IDENT_DUID: + if (context_->clientid_) { + const std::vector& vec = context_->clientid_->getDuid(); + if (!vec.empty()) { + // Client identifier type = DUID? Client identifier holding a DUID + // comprises Type (1 byte), IAID (4 bytes), followed by the actual + // DUID. Thus, the minimal length is 6. + if ((vec[0] == CLIENT_ID_OPTION_TYPE_DUID) && (vec.size() > 5)) { + // Extract DUID, skip IAID. + context_->addHostIdentifier(id_type, + std::vector(vec.begin() + 5, + vec.end())); + } + /// @todo Add support for other client identifiers (see #4317). + } + } + break; + + case Host::IDENT_CIRCUIT_ID: + { + OptionPtr rai = query_->getOption(DHO_DHCP_AGENT_OPTIONS); + if (rai) { + OptionPtr circuit_id_opt = rai->getOption(RAI_OPTION_AGENT_CIRCUIT_ID); + if (circuit_id_opt) { + const OptionBuffer& circuit_id_vec = circuit_id_opt->getData(); + if (!circuit_id_vec.empty()) { + context_->addHostIdentifier(id_type, circuit_id_vec); + } + } + } + } + break; + + default: + ; + } + } +} + const std::string Dhcpv4Srv::VENDOR_CLASS_PREFIX("VENDOR_CLASS_"); Dhcpv4Srv::Dhcpv4Srv(uint16_t port, const bool use_bcast, diff --git a/src/bin/dhcp4/dhcp4_srv.h b/src/bin/dhcp4/dhcp4_srv.h index 17b08245b0..477da8cf49 100644 --- a/src/bin/dhcp4/dhcp4_srv.h +++ b/src/bin/dhcp4/dhcp4_srv.h @@ -137,6 +137,12 @@ private: /// not NULL. void copyDefaultOptions(); + /// @brief Set host identifiers within a context. + /// + /// This method sets an order list of host identifier types and + /// values which the server should use to find host reservations. + void setHostIdentifiers(); + /// @brief Pointer to the allocation engine used by the server. AllocEnginePtr alloc_engine_; /// @brief Pointer to the DHCPv4 message sent by the client. diff --git a/src/bin/dhcp6/dhcp6_srv.cc b/src/bin/dhcp6/dhcp6_srv.cc index caa288b968..0fa5a8ea04 100644 --- a/src/bin/dhcp6/dhcp6_srv.cc +++ b/src/bin/dhcp6/dhcp6_srv.cc @@ -28,6 +28,7 @@ #include #include #include +#include #include #include #include @@ -277,20 +278,37 @@ Dhcpv6Srv::createContext(const Pkt6Ptr& pkt) { AllocEngine::ClientContext6 ctx; ctx.subnet_ = selectSubnet(pkt); ctx.query_ = pkt; - - // Collect host identifiers. - // DUID ctx.duid_ = pkt->getClientId(); - if (ctx.duid_) { - ctx.host_identifiers_[Host::IDENT_DUID] = ctx.duid_->getDuid(); - } - // HW Address. ctx.hwaddr_ = getMAC(pkt); - if (ctx.hwaddr_) { - ctx.host_identifiers_[Host::IDENT_HWADDR] = ctx.hwaddr_->hwaddr_; + + // Collect host identifiers if host reservations enabled. + if (ctx.subnet_ && + (ctx.subnet_->getHostReservationMode() != Subnet::HR_DISABLED)) { + const ConstCfgHostOperationsPtr cfg = + CfgMgr::instance().getCurrentCfg()->getCfgHostOperations6(); + BOOST_FOREACH(const Host::IdentifierType& id_type, + cfg->getIdentifierTypes()) { + switch (id_type) { + case Host::IDENT_DUID: + if (ctx.duid_) { + ctx.addHostIdentifier(id_type, ctx.duid_->getDuid()); + } + break; + + case Host::IDENT_HWADDR: + if (ctx.hwaddr_) { + ctx.addHostIdentifier(id_type, ctx.hwaddr_->hwaddr_); + } + break; + + default: + ; + } + } + + // Find host reservations using specified identifiers. + alloc_engine_->findReservation(ctx); } - // And find a host reservation using those identifiers. - alloc_engine_->findReservation(ctx); return (ctx); } diff --git a/src/lib/dhcpsrv/alloc_engine.cc b/src/lib/dhcpsrv/alloc_engine.cc index 4058b26a99..fdf298bce8 100644 --- a/src/lib/dhcpsrv/alloc_engine.cc +++ b/src/lib/dhcpsrv/alloc_engine.cc @@ -306,7 +306,6 @@ AllocEngine::AllocatorPtr AllocEngine::getAllocator(Lease::Type type) { template void AllocEngine::findReservationInternal(ContextType& ctx, - const ConstCfgHostOperationsPtr& cfg, const AllocEngine::HostGetFunc& host_get) { ctx.host_.reset(); @@ -320,20 +319,17 @@ AllocEngine::findReservationInternal(ContextType& ctx, if (hr_mode != Subnet::HR_DISABLED) { // Iterate over configured identifiers in the order of preference // and try to use each of them to search for the reservations. - BOOST_FOREACH(const Host::IdentifierType& id, cfg->getIdentifierTypes()) { - IdentifierMap::const_iterator id_ctx = ctx.host_identifiers_.find(id); - if (id_ctx != ctx.host_identifiers_.end()) { - // Attempt to find a host using a specified identifier. - ctx.host_ = host_get(ctx.subnet_->getID(), id, - &id_ctx->second[0], id_ctx->second.size()); - // If we found matching host, return. - if (ctx.host_) { - return; - } + BOOST_FOREACH(const IdentifierPair& id_pair, ctx.host_identifiers_) { + // Attempt to find a host using a specified identifier. + ctx.host_ = host_get(ctx.subnet_->getID(), id_pair.first, + &id_pair.second[0], id_pair.second.size()); + // If we found matching host, return. + if (ctx.host_) { + return; } } } - } + } } @@ -370,17 +366,15 @@ AllocEngine::ClientContext6::ClientContext6(const Subnet6Ptr& subnet, const Duid // Initialize host identifiers. if (duid) { - host_identifiers_[Host::IDENT_DUID] = duid->getDuid(); + addHostIdentifier(Host::IDENT_DUID, duid->getDuid()); } } void AllocEngine::findReservation(ClientContext6& ctx) { - ConstCfgHostOperationsPtr cfg = - CfgMgr::instance().getCurrentCfg()->getCfgHostOperations6(); - findReservationInternal(ctx, cfg, boost::bind(&HostMgr::get6, - &HostMgr::instance(), - _1, _2, _3, _4)); + findReservationInternal(ctx, boost::bind(&HostMgr::get6, + &HostMgr::instance(), + _1, _2, _3, _4)); } Lease6Collection @@ -2099,7 +2093,7 @@ AllocEngine::ClientContext4::ClientContext4(const Subnet4Ptr& subnet, // Initialize host identifiers. if (hwaddr) { - host_identifiers_[Host::IDENT_HWADDR] = hwaddr->hwaddr_; + addHostIdentifier(Host::IDENT_HWADDR, hwaddr->hwaddr_); } } @@ -2135,11 +2129,9 @@ AllocEngine::allocateLease4(ClientContext4& ctx) { void AllocEngine::findReservation(ClientContext4& ctx) { - ConstCfgHostOperationsPtr cfg = - CfgMgr::instance().getCurrentCfg()->getCfgHostOperations4(); - findReservationInternal(ctx, cfg, boost::bind(&HostMgr::get4, - &HostMgr::instance(), - _1, _2, _3, _4)); + findReservationInternal(ctx, boost::bind(&HostMgr::get4, + &HostMgr::instance(), + _1, _2, _3, _4)); } Lease4Ptr diff --git a/src/lib/dhcpsrv/alloc_engine.h b/src/lib/dhcpsrv/alloc_engine.h index d1557e141f..f7d9d225fc 100644 --- a/src/lib/dhcpsrv/alloc_engine.h +++ b/src/lib/dhcpsrv/alloc_engine.h @@ -13,7 +13,6 @@ #include #include #include -#include #include #include #include @@ -23,6 +22,7 @@ #include #include +#include #include #include @@ -257,7 +257,7 @@ public: typedef std::pair > IdentifierPair; /// @brief Map holding values to be used as host identifiers. - typedef std::map > IdentifierMap; + typedef std::list IdentifierList; /// @brief Context information for the DHCPv6 leases allocation. /// @@ -358,9 +358,19 @@ public: /// @brief A pointer to the IA_NA/IA_PD option to be sent in response Option6IAPtr ia_rsp_; - /// @brief A map holding host identifiers extracted from a message + /// @brief A list holding host identifiers extracted from a message /// received by the server. - IdentifierMap host_identifiers_; + IdentifierList host_identifiers_; + + /// @brief Conveniece function adding host identifier into + /// @ref host_identifiers_ list. + /// + /// @param id_type Identifier type. + /// @param identifier Identifier value. + void addHostIdentifier(const Host::IdentifierType& id_type, + const std::vector& identifier) { + host_identifiers_.push_back(IdentifierPair(id_type, identifier)); + } /// @brief Default constructor. ClientContext6(); @@ -634,15 +644,11 @@ private: /// functions. /// /// @param ctx Reference to a @ref ClientContext6 or @ref ClientContext4. - /// @param cfg Pointer to object holding general configuration for host - /// reservations. It uses this object to read the desired order of - /// host identifiers to be used to search for reservations. /// @param host_get Pointer to the @ref HostMgr functions to be used /// to retrieve reservation by subnet identifier and host identifier. /// @tparam ContextType Either @ref ClientContext6 or @ref ClientContext4. template static void findReservationInternal(ContextType& ctx, - const ConstCfgHostOperationsPtr& cfg, const HostGetFunc& host_get); /// @brief creates a lease and inserts it in LeaseMgr if necessary @@ -1006,9 +1012,19 @@ public: /// transaction identification information. Pkt4Ptr query_; - /// @brief A map holding host identifiers extracted from a message + /// @brief A list holding host identifiers extracted from a message /// received by the server. - IdentifierMap host_identifiers_; + IdentifierList host_identifiers_; + + /// @brief Conveniece function adding host identifier into + /// @ref host_identifiers_ list. + /// + /// @param id_type Identifier type. + /// @param identifier Identifier value. + void addHostIdentifier(const Host::IdentifierType& id_type, + const std::vector& identifier) { + host_identifiers_.push_back(IdentifierPair(id_type, identifier)); + } /// @brief Default constructor. ClientContext4(); diff --git a/src/lib/dhcpsrv/cfg_host_operations.cc b/src/lib/dhcpsrv/cfg_host_operations.cc index 7eb233b21f..c5bf4ea0cd 100644 --- a/src/lib/dhcpsrv/cfg_host_operations.cc +++ b/src/lib/dhcpsrv/cfg_host_operations.cc @@ -44,7 +44,7 @@ CfgHostOperations::addIdentifierType(const std::string& identifier_name) { } void -CfgHostOperations::clear() { +CfgHostOperations::clearIdentifierTypes() { identifier_types_.clear(); } diff --git a/src/lib/dhcpsrv/cfg_host_operations.h b/src/lib/dhcpsrv/cfg_host_operations.h index fcb3a577ae..50ebbd1dc2 100644 --- a/src/lib/dhcpsrv/cfg_host_operations.h +++ b/src/lib/dhcpsrv/cfg_host_operations.h @@ -74,8 +74,8 @@ public: return (identifier_types_); } - /// @brief Restores default configuration. - void clear(); + /// @brief Removes existing identifier types. + void clearIdentifierTypes(); private: diff --git a/src/lib/dhcpsrv/parsers/host_reservation_parser.cc b/src/lib/dhcpsrv/parsers/host_reservation_parser.cc index 8443c079d8..35dafd359a 100644 --- a/src/lib/dhcpsrv/parsers/host_reservation_parser.cc +++ b/src/lib/dhcpsrv/parsers/host_reservation_parser.cc @@ -324,8 +324,8 @@ HostReservationIdsParser::HostReservationIdsParser() void HostReservationIdsParser::build(isc::data::ConstElementPtr ids_list) { - // Remove any default configuration. - staging_cfg_->clear(); + // Remove existing identifier types. + staging_cfg_->clearIdentifierTypes(); BOOST_FOREACH(ConstElementPtr element, ids_list->listValue()) { std::string id_name = element->stringValue(); diff --git a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc index 2b09144771..c5068500b2 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine4_unittest.cc @@ -1649,8 +1649,8 @@ TEST_F(AllocEngine4Test, findReservation) { AllocEngine::ClientContext4 ctx(subnet_, clientid_, hwaddr_, IOAddress("0.0.0.0"), false, false, "", false); - ctx.host_identifiers_[Host::IDENT_HWADDR] = hwaddr_->hwaddr_; - ctx.host_identifiers_[Host::IDENT_DUID] = clientid_->getDuid(); + ctx.addHostIdentifier(Host::IDENT_HWADDR, hwaddr_->hwaddr_); + ctx.addHostIdentifier(Host::IDENT_DUID, clientid_->getDuid()); // There is no reservation in the database so no host should be // retruned. diff --git a/src/lib/dhcpsrv/tests/alloc_engine_utils.cc b/src/lib/dhcpsrv/tests/alloc_engine_utils.cc index 9198658fc5..3237d2afd6 100644 --- a/src/lib/dhcpsrv/tests/alloc_engine_utils.cc +++ b/src/lib/dhcpsrv/tests/alloc_engine_utils.cc @@ -270,7 +270,7 @@ AllocEngine6Test::simpleAlloc6Test(const Pool6Ptr& pool, const IOAddress& hint, AllocEngine::ClientContext6 ctx(subnet_, duid_, iaid_, hint, type, false, false, "", fake); ctx.hwaddr_ = hwaddr_; - ctx.host_identifiers_[Host::IDENT_HWADDR] = hwaddr_->hwaddr_; + ctx.addHostIdentifier(Host::IDENT_HWADDR, hwaddr_->hwaddr_); ctx.query_.reset(new Pkt6(fake ? DHCPV6_SOLICIT : DHCPV6_REQUEST, 1234)); findReservation(*engine, ctx); diff --git a/src/lib/dhcpsrv/tests/cfg_host_operations_unittest.cc b/src/lib/dhcpsrv/tests/cfg_host_operations_unittest.cc index 0a6e718e46..b1c51ecdf6 100644 --- a/src/lib/dhcpsrv/tests/cfg_host_operations_unittest.cc +++ b/src/lib/dhcpsrv/tests/cfg_host_operations_unittest.cc @@ -10,23 +10,44 @@ #include #include #include +#include using namespace isc; using namespace isc::dhcp; namespace { -/// @brief Checks if specified identifier has been added. +/// @brief Checks if specified identifier is present. /// /// @param cfg Object holding current configuration. /// @param id Identifier type which presence should be checked. +/// @return true if specified identifier is present. bool -identifierAdded(const CfgHostOperations& cfg, const Host::IdentifierType& id) { +identifierPresent(const CfgHostOperations& cfg, const Host::IdentifierType& id) { CfgHostOperations::IdentifierTypes types = cfg.getIdentifierTypes(); return (std::find(types.begin(), types.end(), id) != types.end()); } -// This test checks default configuration. +/// @brief Checks if specified identifier is at specified position. +/// +/// @param cfg Object holding current configuration. +/// @param id Identifier type which presence should be checked. +/// @param pos Position at which the identifier is expected on the list. +/// @return true if specified identifier exists at specified position. +bool +identifierAtPosition(const CfgHostOperations& cfg, const Host::IdentifierType& id, + const size_t pos) { + CfgHostOperations::IdentifierTypes types = cfg.getIdentifierTypes(); + if (types.size() > pos) { + CfgHostOperations::IdentifierTypes::const_iterator type = types.begin(); + std::advance(type, pos); + return (*type == id); + } + return (false); +} + +// This test checks that the list of identifiers is initially +// empty. TEST(CfgHostOperationsTest, defaults) { CfgHostOperations cfg; EXPECT_TRUE(cfg.getIdentifierTypes().empty()); @@ -39,24 +60,24 @@ TEST(CfgHostOperationsTest, addIdentifier) { // Only HW address added. ASSERT_NO_THROW(cfg.addIdentifierType("hw-address")); - EXPECT_TRUE(identifierAdded(cfg, Host::IDENT_HWADDR)); - EXPECT_FALSE(identifierAdded(cfg, Host::IDENT_DUID)); - EXPECT_FALSE(identifierAdded(cfg, Host::IDENT_CIRCUIT_ID)); + EXPECT_TRUE(identifierAtPosition(cfg, Host::IDENT_HWADDR, 0)); + EXPECT_FALSE(identifierPresent(cfg, Host::IDENT_DUID)); + EXPECT_FALSE(identifierPresent(cfg, Host::IDENT_CIRCUIT_ID)); // HW address and DUID should be present. ASSERT_NO_THROW(cfg.addIdentifierType("duid")); - EXPECT_TRUE(identifierAdded(cfg, Host::IDENT_HWADDR)); - EXPECT_TRUE(identifierAdded(cfg, Host::IDENT_DUID)); - EXPECT_FALSE(identifierAdded(cfg, Host::IDENT_CIRCUIT_ID)); + EXPECT_TRUE(identifierAtPosition(cfg, Host::IDENT_HWADDR, 0)); + EXPECT_TRUE(identifierAtPosition(cfg, Host::IDENT_DUID, 1)); + EXPECT_FALSE(identifierPresent(cfg, Host::IDENT_CIRCUIT_ID)); // All three identifiers should be present now. ASSERT_NO_THROW(cfg.addIdentifierType("circuit-id")); - EXPECT_TRUE(identifierAdded(cfg, Host::IDENT_HWADDR)); - EXPECT_TRUE(identifierAdded(cfg, Host::IDENT_DUID)); - EXPECT_TRUE(identifierAdded(cfg, Host::IDENT_CIRCUIT_ID)); + EXPECT_TRUE(identifierAtPosition(cfg, Host::IDENT_HWADDR, 0)); + EXPECT_TRUE(identifierAtPosition(cfg, Host::IDENT_DUID, 1)); + EXPECT_TRUE(identifierAtPosition(cfg, Host::IDENT_CIRCUIT_ID, 2)); // Let's clear and make sure no identifiers are present. - ASSERT_NO_THROW(cfg.clear()); + ASSERT_NO_THROW(cfg.clearIdentifierTypes()); EXPECT_TRUE(cfg.getIdentifierTypes().empty()); } @@ -65,9 +86,9 @@ TEST(CfgHostOperationsTest, addIdentifier) { TEST(CfgHostOperationsTest, createConfig4) { CfgHostOperationsPtr cfg = CfgHostOperations::createConfig4(); - EXPECT_TRUE(identifierAdded(*cfg, Host::IDENT_HWADDR)); - EXPECT_TRUE(identifierAdded(*cfg, Host::IDENT_DUID)); - EXPECT_TRUE(identifierAdded(*cfg, Host::IDENT_CIRCUIT_ID)); + EXPECT_TRUE(identifierAtPosition(*cfg, Host::IDENT_HWADDR, 0)); + EXPECT_TRUE(identifierAtPosition(*cfg, Host::IDENT_DUID, 1)); + EXPECT_TRUE(identifierAtPosition(*cfg, Host::IDENT_CIRCUIT_ID, 2)); } // This test verfies that the default DHCPv6 configuration is created @@ -75,9 +96,9 @@ TEST(CfgHostOperationsTest, createConfig4) { TEST(CfgHostOperationsTest, createConfig6) { CfgHostOperationsPtr cfg = CfgHostOperations::createConfig6(); - EXPECT_TRUE(identifierAdded(*cfg, Host::IDENT_HWADDR)); - EXPECT_TRUE(identifierAdded(*cfg, Host::IDENT_DUID)); - EXPECT_FALSE(identifierAdded(*cfg, Host::IDENT_CIRCUIT_ID)); + EXPECT_TRUE(identifierAtPosition(*cfg, Host::IDENT_HWADDR, 0)); + EXPECT_TRUE(identifierAtPosition(*cfg, Host::IDENT_DUID, 1)); + EXPECT_FALSE(identifierPresent(*cfg, Host::IDENT_CIRCUIT_ID)); } } // end of anonymous namespace -- 2.47.2