From: Razvan Becheriu Date: Wed, 18 Jan 2023 16:07:52 +0000 (+0200) Subject: [#1518] addressed review comments X-Git-Tag: Kea-2.3.4~16 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=cb6340cefb54851e7678727158ad37dbd0d7fd18;p=thirdparty%2Fkea.git [#1518] addressed review comments --- diff --git a/doc/sphinx/arm/dhcp4-srv.rst b/doc/sphinx/arm/dhcp4-srv.rst index b6852eb473..217289ecfb 100644 --- a/doc/sphinx/arm/dhcp4-srv.rst +++ b/doc/sphinx/arm/dhcp4-srv.rst @@ -2495,54 +2495,56 @@ following: :: - { + "Dhcp4": { // First, we need to define that the sub-option 2 in vivso option for // vendor-id 25167 has a specific format (it's a plain string in this example). // After this definition, we can specify values for option tftp. "option-def": [ - { - // We define a short name, so the option can be referenced by name. - // The option has code 2 and resides within vendor space 25167. - // Its data is a plain string. - "name": "tftp", - "code": 2, - "space": "vendor-25167", - "type": "string" - } ], + { + // We define a short name, so the option can be referenced by name. + // The option has code 2 and resides within vendor space 25167. + // Its data is a plain string. + "name": "tftp", + "code": 2, + "space": "vendor-25167", + "type": "string" + } + ], "client-classes": [ - { - // We now need to tell Kea how to recognize when to use vendor space 25167. - // Usually we can use a simple expression, such as checking if the device - // sent a vivso option with specific vendor-id, e.g. "vendor[4491].exists". - // Unfortunately, Genexis is a bit unusual in this aspect, because it - // doesn't send vivso. In this case we need to look into the vendor class - // (option code 60) and see if there's a specific string that identifies - // the device. - "name": "cpe_genexis", - "test": "substring(option[60].hex,0,7) == 'HMC1000'", - - // Once the device is recognized, we want to send two options: - // the vivso option with vendor-id set to 25167, and a sub-option 2. - "option-data": [ - { - "name": "vivso-suboptions", - "data": "25167" - }, + { + // We now need to tell Kea how to recognize when to use vendor space 25167. + // Usually we can use a simple expression, such as checking if the device + // sent a vivso option with specific vendor-id, e.g. "vendor[4491].exists". + // Unfortunately, Genexis is a bit unusual in this aspect, because it + // doesn't send vivso. In this case we need to look into the vendor class + // (option code 60) and see if there's a specific string that identifies + // the device. + "name": "cpe_genexis", + "test": "substring(option[60].hex,0,7) == 'HMC1000'", + + // Once the device is recognized, we want to send two options: + // the vivso option with vendor-id set to 25167, and a sub-option 2. + "option-data": [ + { + "name": "vivso-suboptions", + "data": "25167" + }, - // The sub-option 2 value is defined as any other option. However, - // we want to send this sub-option 2, even when the client didn't - // explicitly request it (often there is no way to do that for - // vendor options). Therefore we use always-send to force Kea - // to always send this option when 25167 vendor space is involved. - { - "name": "tftp", - "space": "vendor-25167", - "data": "tftp://192.0.2.1/genexis/HMC1000.v1.3.0-R.img", - "always-send": true - } - ] - } ] + // The sub-option 2 value is defined as any other option. However, + // we want to send this sub-option 2, even when the client didn't + // explicitly request it (often there is no way to do that for + // vendor options). Therefore we use always-send to force Kea + // to always send this option when 25167 vendor space is involved. + { + "name": "tftp", + "space": "vendor-25167", + "data": "tftp://192.0.2.1/genexis/HMC1000.v1.3.0-R.img", + "always-send": true + } + ] + } + ] } By default, Kea sends back only those options that are requested by a client, @@ -2565,12 +2567,60 @@ that in this particular case an option is defined in vendor space 25167. With with vendor space 25167. This is also how the Kea server can be configured to send multiple vendor enterprise numbers and multiple options, specific for each vendor. +If these options need to be sent by the server regardless if the client +specified any enterprise number, the ``"always-send": true`` must be configured +for the option with code 125 (``vivso-suboptions``) for each enterprise number. + +:: + + "Dhcp4": { + "option-data": [ + { + "always-send": true, + "name": "vivso-suboptions", + "space": "dhcp4", + "data": "2234" + }, + { + "always-send": true, + "name": "vivso-suboptions", + "space": "dhcp4", + "data": "3561" + }, + { + "always-send": true, + "data": "tagged", + "name": "tag", + "space": "vendor-2234" + }, + { + "always-send": true, + "name": "url", + "space": "vendor-3561", + "data": "https://example.com:1234/path" + } + ], + "option-def": [ + { + "code": 22, + "name": "tag", + "space": "vendor-2234", + "type": "string" + }, + { + "code": 11, + "name": "url", + "space": "vendor-3561", + "type": "string" + } + ] + } Another possibility is to redefine the option; see :ref:`dhcp4-private-opts`. Kea comes with several example configuration files. Some of them showcase how to configure options 60 and 43. See ``doc/examples/kea4/vendor-specific.json`` -and ``doc/examples/kea6/vivso.json`` in the Kea sources. +and ``doc/examples/kea4/vivso.json`` in the Kea sources. .. note:: diff --git a/doc/sphinx/arm/dhcp6-srv.rst b/doc/sphinx/arm/dhcp6-srv.rst index 68c6123d7b..c06e68bf0e 100644 --- a/doc/sphinx/arm/dhcp6-srv.rst +++ b/doc/sphinx/arm/dhcp6-srv.rst @@ -6678,7 +6678,7 @@ option is actually needed. An example configuration looks as follows: "subnet6": [ { "pd-pools": [ { - "prefix": "2001:db8::", + "prefix": "2001:db8::", "prefix-len": 56, "delegated-len": 64, diff --git a/src/lib/dhcp/libdhcp++.cc b/src/lib/dhcp/libdhcp++.cc index 20c1c00cf2..c6dd3de7ef 100644 --- a/src/lib/dhcp/libdhcp++.cc +++ b/src/lib/dhcp/libdhcp++.cc @@ -688,39 +688,44 @@ LibDHCP::fuseOptions4(OptionCollection& options) { void LibDHCP::extendVendorOptions4(isc::dhcp::OptionCollection& options) { map vendors_data; - for (auto const& option : options) { - if (option.second->getType() == DHO_VIVSO_SUBOPTIONS) { - uint32_t offset = 0; - auto const& data = option.second->getData(); - auto const& begin = data.begin(); - auto const& end = data.end(); - size_t size; - while ((size = distance(begin + offset, end)) != 0) { - if (size < sizeof(uint32_t)) { - options.erase(DHO_VIVSO_SUBOPTIONS); - isc_throw(SkipRemainingOptionsError, - "Truncated vendor-specific information option" - << ", length=" << size); - } - uint32_t vendor_id = isc::util::readUint32(&(*begin) + offset, distance(begin, end)); - offset += 4; - OptionBuffer vendor_buffer(begin + offset, end); - try { - offset += LibDHCP::unpackVendorOptions4(vendor_id, vendor_buffer, vendors_data[vendor_id]); - } catch (const SkipThisOptionError&) { - } catch (const Exception&) { - options.erase(DHO_VIVSO_SUBOPTIONS); - throw; - } + auto const& it = options.find(DHO_VIVSO_SUBOPTIONS); + // After calling fuseOptions4 there should be at most one option of + // DHO_VIVSO_SUBOPTIONS type. + if (it != options.end()) { + uint32_t offset = 0; + auto const& data = it->second->getData(); + auto const& begin = data.begin(); + size_t size; + while ((size = data.size() - offset) != 0) { + if (size < sizeof(uint32_t)) { + options.erase(DHO_VIVSO_SUBOPTIONS); + isc_throw(SkipRemainingOptionsError, + "Truncated vendor-specific information option" + << ", length=" << size); + } + uint32_t vendor_id = isc::util::readUint32(&(*begin) + offset, data.size()); + offset += 4; + const OptionBuffer vendor_buffer(begin + offset, data.end()); + try { + offset += LibDHCP::unpackVendorOptions4(vendor_id, vendor_buffer, vendors_data[vendor_id]); + } catch (const SkipThisOptionError&) { + // Ignore this kind of error and continue. + } catch (const Exception&) { + options.erase(DHO_VIVSO_SUBOPTIONS); + throw; } } } + // Delete the initial option. options.erase(DHO_VIVSO_SUBOPTIONS); + // Create a new instance of OptionVendor for each enterprise ID. for (auto const& vendor : vendors_data) { OptionVendorPtr vendor_opt(new OptionVendor(Option::V4, vendor.first)); for (auto const& option : vendor.second) { vendor_opt->addOption(option.second); } + // Add the new instance of VendorOption with respective sub-options for + // this enterprise ID. options.insert(std::make_pair(DHO_VIVSO_SUBOPTIONS, vendor_opt)); } } diff --git a/src/lib/dhcp/option_vendor.cc b/src/lib/dhcp/option_vendor.cc index d4b9b9baee..8ab9b4649b 100644 --- a/src/lib/dhcp/option_vendor.cc +++ b/src/lib/dhcp/option_vendor.cc @@ -27,11 +27,6 @@ OptionVendor::OptionVendor(Option::Universe u, OptionBufferConstIter begin, unpack(begin, end); } -OptionVendor::OptionVendor(const OptionVendor& option) : Option(option), - vendor_id_(option.vendor_id_) { - Option::operator=(option); -} - OptionPtr OptionVendor::clone() const { return (cloneInternal()); @@ -69,8 +64,6 @@ void OptionVendor::unpack(OptionBufferConstIter begin, << ", length=" << distance(begin, end)); } - options_.clear(); - vendor_id_ = isc::util::readUint32(&(*begin), distance(begin, end)); OptionBuffer vendor_buffer(begin + 4, end); @@ -120,11 +113,3 @@ OptionVendor::toText(int indent) const { return (output.str()); } - -OptionVendor& OptionVendor::operator=(const OptionVendor& rhs) { - if (&rhs != this) { - Option::operator=(rhs); - vendor_id_ = rhs.vendor_id_; - } - return (*this); -} diff --git a/src/lib/dhcp/option_vendor.h b/src/lib/dhcp/option_vendor.h index b3f81ff20f..73b1b4784e 100644 --- a/src/lib/dhcp/option_vendor.h +++ b/src/lib/dhcp/option_vendor.h @@ -50,14 +50,6 @@ public: OptionVendor(Option::Universe u, OptionBufferConstIter begin, OptionBufferConstIter end); - /// @brief Copy constructor. - /// - /// This constructor makes a deep copy of the option and all of the - /// suboptions. It calls @ref getOptionsCopy to deep copy suboptions. - /// - /// @param source Option to be copied. - OptionVendor(const OptionVendor& option); - /// @brief Copies this option and returns a pointer to the copy. OptionPtr clone() const; @@ -110,15 +102,6 @@ public: /// @return Vendor option in the textual format. virtual std::string toText(int indent = 0) const; - /// @brief Assignment operator. - /// - /// The assignment operator performs a deep copy of the option and - /// its suboptions. It calls @ref getOptionsCopy to deep copy - /// suboptions. - /// - /// @param rhs Option to be assigned. - OptionVendor& operator=(const OptionVendor& rhs); - private: /// @brief Enterprise-id diff --git a/src/lib/dhcp/tests/libdhcp++_unittest.cc b/src/lib/dhcp/tests/libdhcp++_unittest.cc index 1896b772e0..7e83ce15b9 100644 --- a/src/lib/dhcp/tests/libdhcp++_unittest.cc +++ b/src/lib/dhcp/tests/libdhcp++_unittest.cc @@ -1107,20 +1107,21 @@ TEST_F(LibDhcpTest, extentVendorOptions4) { } ASSERT_NO_THROW(LibDHCP::fuseOptions4(options)); ASSERT_EQ(options.size(), 1); + options.insert(make_pair(2, opt3)); + options.insert(make_pair(1, opt4)); + ASSERT_EQ(options.size(), 3); container.clear(); - container.insert(make_pair(1, options.begin()->second)); - container.insert(make_pair(2, opt3)); - container.insert(make_pair(1, opt4)); - ASSERT_EQ(container.size(), 3); - options.clear(); - for (auto const& option : container) { + for (auto const& option : options) { const OptionBuffer& buffer = option.second->toBinary(); - options.insert(make_pair(option.second->getType(), + container.insert(make_pair(option.second->getType(), OptionPtr(new Option(Option::V4, option.second->getType(), buffer)))); } - ASSERT_EQ(options.size(), 3); + ASSERT_EQ(container.size(), 3); + options = container; + ASSERT_NO_THROW(LibDHCP::fuseOptions4(options)); + ASSERT_EQ(options.size(), 1); LibDHCP::extendVendorOptions4(options); ASSERT_EQ(options.size(), 2); for (auto const& option : options) {