[#2536] addressed review comments

6 files changed, 341 insertions, 140 deletions

diff --git a/src/lib/dhcp/option4_dnr.cc b/src/lib/dhcp/option4_dnr.cc
index 9a9470232e..4a625e8cc2 100644
--- a/src/lib/dhcp/option4_dnr.cc
+++ b/src/lib/dhcp/option4_dnr.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2018-2023 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2023 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -10,8 +10,6 @@
 #include <dns/labelsequence.h>
 #include <util/strutil.h>
 
-#include <set>
-
 using namespace isc::asiolink;
 using namespace isc::util;
 
@@ -39,6 +37,7 @@ Option4Dnr::pack(OutputBuffer& buf, bool check) const {
         if (dnr_instance.isAdnOnlyMode()) {
             continue;
         }
+
         buf.writeUint8(dnr_instance.getAddrLength());
         dnr_instance.packAddresses(buf);
         dnr_instance.packSvcParams(buf);
@@ -52,7 +51,7 @@ Option4Dnr::unpack(OptionBufferConstIter begin, OptionBufferConstIter end) {
         DnrInstance dnr_instance(V4);
         if (std::distance(begin, end) < dnr_instance.getMinimalLength()) {
             isc_throw(OutOfRange, dnr_instance.getLogPrefix()
-                                      << " malformed: DNR instance data truncated to size "
+                                      << "DNR instance data truncated to size "
                                       << std::distance(begin, end));
         }
 
@@ -73,6 +72,7 @@ Option4Dnr::unpack(OptionBufferConstIter begin, OptionBufferConstIter end) {
             addDnrInstance(dnr_instance);
             continue;
         }
+
         dnr_instance.setAdnOnlyMode(false);
 
         // Unpack Addr Len + IPv4 Address(es).
@@ -97,6 +97,7 @@ Option4Dnr::toText(int indent) const {
                << "(Instance len=" << dnr_instance.getDnrInstanceDataLength() << ", "
                << dnr_instance.getDnrInstanceAsText() << ")";
     }
+
     return (stream.str());
 }
 
@@ -107,6 +108,7 @@ Option4Dnr::len() const {
         len += dnr_instance.getDnrInstanceDataLength() +
                dnr_instance.getDnrInstanceDataLengthSize();
     }
+
     return (len);
 }
 
@@ -115,6 +117,12 @@ Option4Dnr::addDnrInstance(DnrInstance& dnr_instance) {
     dnr_instances_.push_back(dnr_instance);
 }
 
+const std::unordered_set<std::string> DnrInstance::FORBIDDEN_SVC_PARAMS = {"ipv4hint", "ipv6hint"};
+
+DnrInstance::DnrInstance(Option::Universe universe) : universe_(universe) {
+    initMembers();
+}
+
 DnrInstance::DnrInstance(Option::Universe universe,
                          const uint16_t service_priority,
                          const std::string& adn,
@@ -122,6 +130,7 @@ DnrInstance::DnrInstance(Option::Universe universe,
                          const std::string& svc_params)
     : universe_(universe), service_priority_(service_priority), ip_addresses_(ip_addresses),
       svc_params_(svc_params) {
+    initMembers();
     setAdn(adn);
     checkFields();
 }
@@ -130,6 +139,7 @@ DnrInstance::DnrInstance(Option::Universe universe,
                          const uint16_t service_priority,
                          const std::string& adn)
     : universe_(universe), service_priority_(service_priority) {
+    initMembers();
     setAdn(adn);
 }
 
@@ -138,9 +148,11 @@ DnrInstance::packAdn(OutputBuffer& buf) const {
     if (!adn_) {
         // This should not happen since Encrypted DNS options are designed
         // to always include an authentication domain name.
-        isc_throw(InvalidOptionDnrDomainName, "Mandatory Authentication Domain Name fully "
-                                              "qualified domain-name is missing");
+        isc_throw(InvalidOptionDnrDomainName, getLogPrefix()
+                                                  << "Mandatory Authentication Domain Name fully "
+                                                     "qualified domain-name is missing");
     }
+
     isc::dns::LabelSequence label_sequence(*adn_);
     if (label_sequence.getDataLength() > 0) {
         size_t data_length = 0;
@@ -167,32 +179,33 @@ DnrInstance::packSvcParams(OutputBuffer& buf) const {
 
 std::string
 DnrInstance::getAdnAsText() const {
-    if (adn_) {
-        return (adn_->toText());
-    }
-    return ("");
+    return (adn_) ? (adn_->toText()) : ("");
 }
 
 void
 DnrInstance::setAdn(const std::string& adn) {
     std::string trimmed_adn = str::trim(adn);
     if (trimmed_adn.empty()) {
-        isc_throw(InvalidOptionDnrDomainName, "Mandatory Authentication Domain Name fully "
-                                              "qualified domain-name must not be empty");
+        isc_throw(InvalidOptionDnrDomainName, getLogPrefix()
+                                                  << "Mandatory Authentication Domain Name fully "
+                                                     "qualified domain-name must not be empty");
     }
+
     try {
         adn_.reset(new isc::dns::Name(trimmed_adn, true));
     } catch (const Exception& ex) {
-        isc_throw(InvalidOptionDnrDomainName, "Failed to parse "
-                                              "fully qualified domain-name from string "
-                                              "- " << ex.what());
+        isc_throw(InvalidOptionDnrDomainName, getLogPrefix()
+                                                  << "Failed to parse "
+                                                     "fully qualified domain-name from string - "
+                                                  << ex.what());
     }
+
     size_t adn_len = 0;
     isc::dns::LabelSequence label_sequence(*adn_);
     label_sequence.getData(&adn_len);
     if (adn_len > std::numeric_limits<uint16_t>::max()) {
-        isc_throw(InvalidOptionDnrDomainName,
-                  "Given ADN FQDN length " << adn_len << " is bigger than uint_16 MAX");
+        isc_throw(InvalidOptionDnrDomainName, getLogPrefix() << "Given ADN FQDN length " << adn_len
+                                                             << " is bigger than uint_16 MAX");
     }
 
     adn_length_ = adn_len;
@@ -204,23 +217,34 @@ DnrInstance::setAdn(const std::string& adn) {
 void
 DnrInstance::unpackAdn(OptionBufferConstIter& begin, OptionBufferConstIter end) {
     OpaqueDataTuple::LengthFieldType lft = OptionDataTypeUtil::getTupleLenFieldType(universe_);
-    OpaqueDataTuple adn_tuple(lft, begin, end);
+    OpaqueDataTuple adn_tuple(lft);
+    try {
+        adn_tuple.unpack(begin, end);
+    } catch (const Exception& ex) {
+        isc_throw(BadValue, getLogPrefix() << "failed to unpack ADN data"
+                                           << " - " << ex.what());
+    }
+
     adn_length_ = adn_tuple.getLength();
 
     // Encrypted DNS options are designed to always include an authentication domain name,
     // so when there is no FQDN included, we shall throw an exception.
     if (adn_length_ == 0) {
-        isc_throw(InvalidOptionDnrDomainName, "Mandatory Authentication Domain Name fully "
-                                              "qualified domain-name is missing");
+        isc_throw(InvalidOptionDnrDomainName, getLogPrefix()
+                                                  << "Mandatory Authentication Domain Name fully "
+                                                     "qualified domain-name is missing");
     }
-    InputBuffer name_buf(&*adn_tuple.getData().begin(), adn_length_);
+
+    InputBuffer name_buf(adn_tuple.getData().data(), adn_length_);
     try {
         adn_.reset(new isc::dns::Name(name_buf, true));
     } catch (const Exception& ex) {
-        isc_throw(InvalidOptionDnrDomainName, "Failed to parse "
-                                              "fully qualified domain-name from wire format "
-                                              "- " << ex.what());
+        isc_throw(InvalidOptionDnrDomainName, getLogPrefix()
+                                                  << "Failed to parse "
+                                                     "fully qualified domain-name from wire format "
+                                                     "- " << ex.what());
     }
+
     begin += adn_length_ + getAdnLengthSize();
 }
 
@@ -228,16 +252,20 @@ void
 DnrInstance::checkSvcParams(bool from_wire_data) {
     std::string svc_params = str::trim(svc_params_);
     if (svc_params.empty()) {
-        isc_throw(InvalidOptionDnrSvcParams, "Provided Svc Params field is empty");
+        isc_throw(InvalidOptionDnrSvcParams, getLogPrefix()
+                                                 << "Provided Svc Params field is empty");
     }
+
     if (!from_wire_data) {
         // If Service Params field was not parsed from on-wire data,
         // but actually was provided with ctor, let's calculate svc_params_length_.
         auto svc_params_len = svc_params.length();
         if (svc_params_len > std::numeric_limits<uint16_t>::max()) {
-            isc_throw(OutOfRange, "Given Svc Params length " << svc_params_len
-                                                             << " is bigger than uint_16 MAX");
+            isc_throw(InvalidOptionDnrSvcParams, getLogPrefix()
+                                                     << "Given Svc Params length " << svc_params_len
+                                                     << " is bigger than uint_16 MAX");
         }
+
         svc_params_length_ = svc_params_len;
         // If Service Params field was not parsed from on-wire data,
         // but actually was provided with ctor, let's replace it with trimmed value.
@@ -252,57 +280,47 @@ DnrInstance::checkSvcParams(bool from_wire_data) {
     std::vector<std::string> tokens = str::tokens(svc_params, " ");
 
     // Set of keys used to check if a key is not repeated.
-    std::set<std::string> keys;
+    std::unordered_set<std::string> keys;
     // String sanitizer is used to check keys syntax.
     str::StringSanitizerPtr sanitizer;
     // SvcParamKeys are lower-case alphanumeric strings. Key names
     // contain 1-63 characters from the ranges "a"-"z", "0"-"9", and "-".
     std::string regex = "[^a-z0-9-]";
     sanitizer.reset(new str::StringSanitizer(regex, ""));
-    // The service parameters MUST NOT include
-    // "ipv4hint" or "ipv6hint" SvcParams as they are superseded by the
-    // included IP addresses.
-    std::set<std::string> forbidden_keys = {"ipv4hint", "ipv6hint"};
 
     // Now let's check each SvcParamKey=SvcParamValue pair.
     for (const std::string& token : tokens) {
         std::vector<std::string> key_val = str::tokens(token, "=");
         if (key_val.size() > 2) {
-            isc_throw(InvalidOptionDnrSvcParams, "Wrong Svc Params syntax - more than one "
-                                                 "equals sign found in SvcParamKey=SvcParamValue "
-                                                 "pair");
+            isc_throw(InvalidOptionDnrSvcParams,
+                      getLogPrefix() << "Wrong Svc Params syntax - more than one "
+                                        "equals sign found in SvcParamKey=SvcParamValue pair");
         }
 
         // SvcParam Key related checks come below.
         std::string key = key_val[0];
-        if (forbidden_keys.find(key) != forbidden_keys.end()) {
+        if (key.length() > 63) {
             isc_throw(InvalidOptionDnrSvcParams,
-                      "Wrong Svc Params syntax - key " << key << " must not be used");
+                      getLogPrefix() << "Wrong Svc Params syntax - key had more than 63 "
+                                        "characters - " << key);
         }
 
-        auto insert_res = keys.insert(key);
-        if (!insert_res.second) {
-            isc_throw(InvalidOptionDnrSvcParams,
-                      "Wrong Svc Params syntax - key " << key << " was duplicated");
+        if (FORBIDDEN_SVC_PARAMS.find(key) != FORBIDDEN_SVC_PARAMS.end()) {
+            isc_throw(InvalidOptionDnrSvcParams, getLogPrefix() << "Wrong Svc Params syntax - key "
+                                                                << key << " must not be used");
         }
 
-        if (key.length() > 63) {
-            isc_throw(InvalidOptionDnrSvcParams, "Wrong Svc Params syntax - key had more than 63 "
-                                                 "characters - "
-                                                     << key);
+        auto insert_res = keys.insert(key);
+        if (!insert_res.second) {
+            isc_throw(InvalidOptionDnrSvcParams, getLogPrefix() << "Wrong Svc Params syntax - key "
+                                                                << key << " was duplicated");
         }
 
         std::string sanitized_key = sanitizer->scrub(key);
         if (sanitized_key.size() < key.size()) {
-            isc_throw(InvalidOptionDnrSvcParams, "Wrong Svc Params syntax - invalid character "
-                                                 "used in key - "
-                                                     << key);
-        }
-
-        if (key_val.size() == 2) {
-            // For now we skip Check of value syntax.
-            // This is up to customer to configure this correctly.
-            std::string value = key_val[1];
+            isc_throw(InvalidOptionDnrSvcParams,
+                      getLogPrefix()
+                          << "Wrong Svc Params syntax - invalid character used in key - " << key);
         }
     }
 }
@@ -313,27 +331,30 @@ DnrInstance::checkFields() {
         // ADN only mode, nothing more to do.
         return;
     }
+
     if (!svc_params_.empty() && ip_addresses_.empty()) {
         // As per draft-ietf-add-dnr 3.1.8:
         // If additional data is supplied (i.e. not ADN only mode),
         // the option includes at least one valid IP address.
-        isc_throw(OutOfRange,
-                  getLogPrefix()
-                      << " malformed: No IP address given. Since this is not "
-                         "ADN only mode, at least one valid IP address must be included");
+        isc_throw(OutOfRange, getLogPrefix() << "No IP address given. Since this is not ADN only "
+                                                "mode, at least one valid IP address must "
+                                                "be included");
     }
+
     if (!svc_params_.empty()) {
         checkSvcParams(false);
     }
+
     adn_only_mode_ = false;
     const uint8_t addr_field_len = (universe_ == Option::V4) ? V4ADDRESS_LEN : V6ADDRESS_LEN;
     const uint16_t max_addr_len = (universe_ == Option::V4) ? std::numeric_limits<uint8_t>::max() :
                                                               std::numeric_limits<uint16_t>::max();
     auto addr_len = ip_addresses_.size() * addr_field_len;
     if (addr_len > max_addr_len) {
-        isc_throw(OutOfRange, "Given IP addresses length " << addr_len << " is bigger than MAX "
-                                                           << max_addr_len);
+        isc_throw(OutOfRange, getLogPrefix() << "Given IP addresses length " << addr_len
+                                             << " is bigger than MAX " << max_addr_len);
     }
+
     addr_length_ = addr_len;
     if (universe_ == Option::V4) {
         dnr_instance_data_length_ = dnrInstanceLen();
@@ -341,27 +362,22 @@ DnrInstance::checkFields() {
 }
 
 std::string
-DnrInstance::getLogPrefix() const {
-    return (universe_ == Option::V4) ?
-               ("DHCPv4 Encrypted DNS Option (" + std::to_string(DHO_V4_DNR) + ")") :
-               ("DHCPv6 Encrypted DNS Option (" + std::to_string(D6O_V6_DNR) + ")");
-}
-
-std::string
 DnrInstance::getDnrInstanceAsText() const {
-    std::string text = "service_priority=" + std::to_string(service_priority_) + ", " +
-                       "adn_length=" + std::to_string(adn_length_) + ", " + "adn='" +
-                       getAdnAsText() + "'";
+    std::ostringstream stream;
+    stream << "service_priority=" << service_priority_ << ", adn_length=" << adn_length_ << ", "
+           << "adn='" << getAdnAsText() << "'";
     if (!adn_only_mode_) {
-        text += ", addr_length=" + std::to_string(addr_length_) + ", address(es):";
+        stream << ", addr_length=" << addr_length_ << ", address(es):";
         for (const auto& address : ip_addresses_) {
-            text += " " + address.toText();
+            stream << " " << address.toText();
         }
+
         if (svc_params_length_ > 0) {
-            text += ", svc_params='" + svc_params_ + "'";
+            stream << ", svc_params='" + svc_params_ + "'";
         }
     }
-    return (text);
+
+    return (stream.str());
 }
 
 uint16_t
@@ -370,36 +386,8 @@ DnrInstance::dnrInstanceLen() const {
     if (!adn_only_mode_) {
         len += addr_length_ + getAddrLengthSize() + svc_params_length_;
     }
-    return (len);
-}
-
-uint8_t
-DnrInstance::getDnrInstanceDataLengthSize() const {
-    if (universe_ == Option::V6) {
-        return (0);
-    }
-    return (2);
-}
-
-uint8_t
-DnrInstance::getAdnLengthSize() const {
-    if (universe_ == Option::V6) {
-        return (2);
-    }
-    return (1);
-}
 
-uint8_t
-DnrInstance::getAddrLengthSize() const {
-    if (universe_ == Option::V6) {
-        return (2);
-    }
-    return (1);
-}
-
-uint8_t
-DnrInstance::getMinimalLength() const {
-    return (getDnrInstanceDataLengthSize() + SERVICE_PRIORITY_SIZE + getAdnLengthSize());
+    return (len);
 }
 
 void
@@ -413,7 +401,7 @@ DnrInstance::unpackDnrInstanceDataLength(OptionBufferConstIter& begin, OptionBuf
     begin += getDnrInstanceDataLengthSize();
     if (std::distance(begin, end) < dnr_instance_data_length_) {
         isc_throw(OutOfRange, getLogPrefix()
-                                  << " malformed: DNR instance data truncated to size "
+                                  << "DNR instance data truncated to size "
                                   << std::distance(begin, end) << " but it was supposed to be "
                                   << dnr_instance_data_length_);
     }
@@ -427,12 +415,19 @@ DnrInstance::unpackServicePriority(OptionBufferConstIter& begin) {
 
 void
 DnrInstance::unpackAddresses(OptionBufferConstIter& begin, const OptionBufferConstIter end) {
-    OpaqueDataTuple addr_tuple(OpaqueDataTuple::LENGTH_1_BYTE, begin, end);
+    OpaqueDataTuple addr_tuple(OpaqueDataTuple::LENGTH_1_BYTE);
+    try {
+        addr_tuple.unpack(begin, end);
+    } catch (const Exception& ex) {
+        isc_throw(BadValue, getLogPrefix() << "failed to unpack IP Addresses data"
+                                           << " - " << ex.what());
+    }
+
     addr_length_ = addr_tuple.getLength();
     // It MUST be a multiple of 4.
     if ((addr_length_ % V4ADDRESS_LEN) != 0) {
-        isc_throw(OutOfRange, getLogPrefix() << " malformed: Addr Len=" << addr_length_
-                                             << " is not divisible by 4");
+        isc_throw(OutOfRange, getLogPrefix()
+                                  << "Addr Len=" << addr_length_ << " is not divisible by 4");
     }
 
     // As per draft-ietf-add-dnr 3.1.8:
@@ -440,7 +435,7 @@ DnrInstance::unpackAddresses(OptionBufferConstIter& begin, const OptionBufferCon
     // the option includes at least one valid IP address.
     if (addr_length_ == 0) {
         isc_throw(OutOfRange, getLogPrefix()
-                                  << " malformed: Addr Len=" << addr_length_
+                                  << "Addr Len=" << addr_length_
                                   << " but it must contain at least one valid IP address");
     }
 
@@ -466,5 +461,17 @@ DnrInstance::unpackSvcParams(OptionBufferConstIter& begin, OptionBufferConstIter
     }
 }
 
+void
+DnrInstance::initMembers() {
+    dnr_instance_data_length_size_ = (universe_ == Option::V6) ? 0 : 2;
+    adn_length_size_ = (universe_ == Option::V6) ? 2 : 1;
+    addr_length_size_ = (universe_ == Option::V6) ? 2 : 1;
+    minimal_length_ = dnr_instance_data_length_size_ + SERVICE_PRIORITY_SIZE + adn_length_size_;
+    log_prefix_ =
+        (universe_ == Option::V4) ?
+            ("DHCPv4 Encrypted DNS Option (" + std::to_string(DHO_V4_DNR) + ") malformed: ") :
+            ("DHCPv6 Encrypted DNS Option (" + std::to_string(D6O_V6_DNR) + ") malformed: ");
+}
+
 }  // namespace dhcp
 }  // namespace isc
diff --git a/src/lib/dhcp/option4_dnr.h b/src/lib/dhcp/option4_dnr.h
index 3062f69c23..ae8e43c620 100644
--- a/src/lib/dhcp/option4_dnr.h
+++ b/src/lib/dhcp/option4_dnr.h
@@ -1,4 +1,4 @@
-// Copyright (C) 2018-2023 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2023 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -14,6 +14,8 @@
 #include <dhcp/option_data_types.h>
 #include <dns/name.h>
 
+#include <unordered_set>
+
 namespace isc {
 namespace dhcp {
 
@@ -53,10 +55,17 @@ public:
     /// @brief Size in octets of Service Priority field.
     static const uint8_t SERVICE_PRIORITY_SIZE = 2;
 
+    /// @brief Set of forbidden SvcParams.
+    ///
+    /// The service parameters MUST NOT include
+    /// "ipv4hint" or "ipv6hint" SvcParams as they are superseded by the
+    /// included IP addresses.
+    static const std::unordered_set<std::string> FORBIDDEN_SVC_PARAMS;
+
     /// @brief Constructor of the empty DNR Instance.
     ///
     /// @param universe either V4 or V6 Option universe
-    explicit DnrInstance(Option::Universe universe) : universe_(universe) {}
+    explicit DnrInstance(Option::Universe universe);
 
     /// @brief Constructor of the DNR Instance with all fields from params.
     ///
@@ -157,27 +166,32 @@ public:
 
     /// @brief Returns minimal length of the DNR instance data (without headers) in octets.
     ///
-    /// If the ADN-only mode is used, then "Addr Length", "ip(v4/v6)-address(es)",
-    /// and "Service Parameters (SvcParams)" fields are not present.
-    /// So minimal length of data is calculated by adding 2 octets for Service Priority,
-    /// octets needed for ADN Length and octets needed for DNR Instance Data Length
-    /// (only in case of DHCPv4).
-    ///
     /// @return Minimal length of the DNR instance data (without headers) in octets.
-    uint8_t getMinimalLength() const;
+    uint8_t getMinimalLength() const {
+        return (minimal_length_);
+    }
 
     /// @brief Returns size in octets of Addr Length field.
-    uint8_t getAddrLengthSize() const;
+    uint8_t getAddrLengthSize() const {
+        return (addr_length_size_);
+    }
 
     /// @brief Returns size in octets of DNR Instance Data Length field.
-    uint8_t getDnrInstanceDataLengthSize() const;
+    uint8_t getDnrInstanceDataLengthSize() const {
+        return (dnr_instance_data_length_size_);
+    }
 
     /// @brief Returns size in octets of ADN Length field.
-    uint8_t getAdnLengthSize() const;
+    uint8_t getAdnLengthSize() const {
+        return (adn_length_size_);
+    }
 
-    /// @brief Constructs Log prefix depending on V4/V6 Option universe.
+    /// @brief Returns Log prefix depending on V4/V6 Option universe.
+    ///
     /// @return Log prefix as a string which can be used for prints when throwing an exception.
-    std::string getLogPrefix() const;
+    std::string getLogPrefix() const {
+        return (log_prefix_);
+    }
 
     /// @brief Returns whether ADN only mode is enabled or disabled.
     bool isAdnOnlyMode() const {
@@ -192,9 +206,13 @@ public:
     /// It also calculates and sets value of Addr length field.
     ///
     /// @param adn string representation of ADN FQDN
+    ///
+    /// @throw InvalidOptionDnrDomainName Thrown in case of any issue with parsing ADN
+    /// from given string.
     void setAdn(const std::string& adn);
 
     /// @brief Setter of the @c adn_only_mode_ field.
+    ///
     /// @param adn_only_mode enabled/disabled setting
     void setAdnOnlyMode(bool adn_only_mode) {
         adn_only_mode_ = adn_only_mode;
@@ -206,6 +224,8 @@ public:
     /// DNS resolver data is appended at the end of the buffer.
     ///
     /// @param [out] buf buffer where ADN FQDN will be written.
+    ///
+    /// @throw InvalidOptionDnrDomainName Thrown when mandatory field ADN is empty.
     void packAdn(isc::util::OutputBuffer& buf) const;
 
     /// @brief Writes the IP address(es) in the wire format into a buffer.
@@ -231,9 +251,12 @@ public:
     ///
     /// @param begin beginning of the buffer from which the field will be read
     /// @param end end of the buffer from which the field will be read
+    ///
+    /// @throw OutOfRange Thrown in case of truncated data detected.
     void unpackDnrInstanceDataLength(OptionBufferConstIter& begin, OptionBufferConstIter end);
 
     /// @brief Unpacks Service Priority from wire data buffer and stores it in @c service_priority_.
+    ///
     /// @param begin beginning of the buffer from which the field will be read
     void unpackServicePriority(OptionBufferConstIter& begin);
 
@@ -243,6 +266,10 @@ public:
     ///
     /// @param begin beginning of the buffer from which the ADN will be read
     /// @param end end of the buffer from which the ADN will be read
+    ///
+    /// @throw BadValue Thrown in case of any issue with unpacking opaque data of the ADN.
+    /// @throw InvalidOptionDnrDomainName Thrown in case of any issue with parsing ADN
+    /// from given wire data.
     void unpackAdn(OptionBufferConstIter& begin, OptionBufferConstIter end);
 
     /// @brief Unpacks IP address(es) from wire data and stores it/them in @c ip_addresses_.
@@ -251,6 +278,10 @@ public:
     ///
     /// @param begin beginning of the buffer from which the field will be read
     /// @param end end of the buffer from which the field will be read
+    ///
+    /// @throw BadValue Thrown in case of any issue with unpacking opaque data of the IP addresses.
+    /// @throw OutOfRange Thrown in case of malformed data detected during parsing e.g.
+    /// Addr Len not divisible by 4, Addr Len is 0.
     virtual void unpackAddresses(OptionBufferConstIter& begin, OptionBufferConstIter end);
 
     /// @brief Unpacks Service Parameters from wire data buffer and stores it in @c svc_params_.
@@ -264,15 +295,33 @@ public:
     /// @brief Checks SvcParams field if encoded correctly and throws in case of issue found.
     ///
     /// The field should be encoded following the rules in
-    /// Section 2.1 of [I-D.ietf-dnsop-svcb-https].
+    /// Section 2.1 of [I-D.ietf-dnsop-svcb-https]. SvcParams are
+    /// a whitespace-separated list, with each SvcParam consisting of
+    /// a SvcParamKey=SvcParamValue pair or a standalone SvcParamKey.
+    ///
+    /// @note It is user's responsibility to provide correct configuration
+    /// of @c SvcParams as described in Section 2.1 of [I-D.ietf-dnsop-svcb-https].
+    /// Currently, SvcParamValue is not verified. Proper syntax of SvcParamValue
+    /// is described in Appendix A of [I-D.ietf-dnsop-svcb-https].
+    ///
+    /// @param from_wire_data used to determine whether SvcParams data comes
+    /// from unpacked wire data or from ctor param
+    ///
+    /// @throw InvalidOptionDnrSvcParams Thrown in case of any issue found when checking
+    /// @c ServiceParams field syntax
     void checkSvcParams(bool from_wire_data = true);
 
     /// @brief Checks IP address(es) field if data is correct and throws in case of issue found.
     ///
     /// Fields lengths are also calculated and saved to member variables.
+    ///
+    /// @throw OutOfRange Thrown in case of no IP addresses found or when IP addresses length
+    /// is too big
+    /// @throw InvalidOptionDnrSvcParams Thrown when @c checkSvcParams(from_wire_data) throws
     void checkFields();
 
     /// @brief Adds IP address to @c ip_addresses_ container.
+    ///
     /// @param ip_address IP address to be added
     void addIpAddress(const asiolink::IOAddress& ip_address);
 
@@ -327,6 +376,35 @@ protected:
     /// @brief Calculates and returns length of DNR Instance data in octets.
     /// @return length of DNR Instance data in octets.
     uint16_t dnrInstanceLen() const;
+
+private:
+    /// @brief Size in octets of DNR Instance Data Length field.
+    ///
+    /// @note This field is used only in case of V4 DNR option.
+    uint8_t dnr_instance_data_length_size_;
+
+    /// @brief Size in octets of ADN Length field.
+    uint8_t adn_length_size_;
+
+    /// @brief Size in octets of Addr Length field.
+    uint8_t addr_length_size_;
+
+    /// @brief Minimal length of the DNR instance data (without headers) in octets.
+    ///
+    /// @note If the ADN-only mode is used, then "Addr Length", "ip(v4/v6)-address(es)",
+    /// and "Service Parameters (SvcParams)" fields are not present.
+    /// So minimal length of data is calculated by adding 2 octets for Service Priority,
+    /// octets needed for ADN Length and octets needed for DNR Instance Data Length
+    /// (only in case of DHCPv4).
+    uint8_t minimal_length_;
+
+    /// @brief Log prefix as a string which can be used for prints when throwing an exception.
+    std::string log_prefix_;
+
+    /// @brief Initializes private member variables basing on option's V4/V6 Universe.
+    ///
+    /// @note It must be called in all types of constructors of class @c DnrInstance .
+    void initMembers();
 };
 
 /// @brief Represents DHCPv4 Encrypted DNS %Option (code 162).
@@ -378,10 +456,48 @@ public:
         return (dnr_instances_);
     }
 
+    /// @brief Copies this option and returns a pointer to the copy.
+    ///
+    /// @return Pointer to the copy of the option.
     OptionPtr clone() const override;
+
+    /// @brief Writes option in wire-format to a buffer.
+    ///
+    /// Writes option in wire-format to buffer, returns pointer to first unused
+    /// byte after stored option (that is useful for writing options one after
+    /// another).
+    ///
+    /// @param buf pointer to a buffer
+    /// @param check flag which indicates if checking the option length is
+    /// required (used only in V4)
+    ///
+    /// @throw InvalidOptionDnrDomainName Thrown when Option's mandatory field ADN is empty.
+    /// @throw OutOfRange Thrown when @c check param set to @c true and
+    /// @c Option::packHeader(buf,check) throws.
     void pack(util::OutputBuffer& buf, bool check = true) const override;
+
+    /// @brief Parses received wire data buffer.
+    ///
+    /// @param begin iterator to first byte of option data
+    /// @param end iterator to end of option data (first byte after option end)
+    ///
+    /// @throw OutOfRange Thrown in case of truncated data. May be also thrown when
+    /// @c DnrInstance::unpackDnrInstanceDataLength(begin,end) throws.
+    /// @throw BadValue Thrown when @c DnrInstance::unpackAdn(begin,end) throws.
+    /// @throw InvalidOptionDnrDomainName Thrown when @c DnrInstance::unpackAdn(begin,end) throws.
     void unpack(OptionBufferConstIter begin, OptionBufferConstIter end) override;
+
+    /// @brief Returns string representation of the option.
+    ///
+    /// @param indent number of spaces before printing text
+    ///
+    /// @return string with text representation.
     std::string toText(int indent = 0) const override;
+
+    /// @brief Returns length of the complete option (data length + DHCPv4/DHCPv6
+    /// option header)
+    ///
+    /// @return length of the option
     uint16_t len() const override;
 
 protected:
diff --git a/src/lib/dhcp/option6_dnr.cc b/src/lib/dhcp/option6_dnr.cc
index 4893fbf322..3d7bb0fcd8 100644
--- a/src/lib/dhcp/option6_dnr.cc
+++ b/src/lib/dhcp/option6_dnr.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2018-2023 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2023 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -33,6 +33,7 @@ Option6Dnr::pack(util::OutputBuffer& buf, bool check) const {
     if (adn_only_mode_) {
         return;
     }
+
     buf.writeUint16(addr_length_);
     packAddresses(buf);
     packSvcParams(buf);
@@ -42,8 +43,10 @@ void
 Option6Dnr::packAddresses(util::OutputBuffer& buf) const {
     for (const auto& address : ip_addresses_) {
         if (!address.isV6()) {
-            isc_throw(isc::BadValue, address.toText() << " is not an IPv6 address");
+            isc_throw(isc::BadValue, getLogPrefix()
+                                         << address.toText() << " is not an IPv6 address");
         }
+
         buf.writeData(&address.toBytes()[0], V6ADDRESS_LEN);
     }
 }
@@ -51,9 +54,10 @@ Option6Dnr::packAddresses(util::OutputBuffer& buf) const {
 void
 Option6Dnr::unpack(OptionBufferConstIter begin, OptionBufferConstIter end) {
     if (std::distance(begin, end) < getMinimalLength()) {
-        isc_throw(OutOfRange, getLogPrefix() << " malformed: data truncated to size "
-                                             << std::distance(begin, end));
+        isc_throw(OutOfRange, getLogPrefix()
+                                  << "data truncated to size " << std::distance(begin, end));
     }
+
     setData(begin, end);
 
     // First two octets of Option data is Service Priority - this is mandatory field.
@@ -67,6 +71,7 @@ Option6Dnr::unpack(OptionBufferConstIter begin, OptionBufferConstIter end) {
         // ADN only mode, other fields are not included.
         return;
     }
+
     adn_only_mode_ = false;
 
     unpackAddresses(begin, end);
@@ -92,17 +97,18 @@ Option6Dnr::len() const {
 void
 Option6Dnr::unpackAddresses(OptionBufferConstIter& begin, OptionBufferConstIter end) {
     if (std::distance(begin, end) < getAddrLengthSize()) {
-        isc_throw(OutOfRange, getLogPrefix() << " malformed: after"
+        isc_throw(OutOfRange, getLogPrefix() << "after"
                                                 " ADN field, there should be at least "
                                                 "2 bytes long Addr Length field");
     }
+
     // Next come two octets of Addr Length.
     addr_length_ = isc::util::readUint16(&(*begin), getAddrLengthSize());
     begin += getAddrLengthSize();
     // It MUST be a multiple of 16.
     if ((addr_length_ % V6ADDRESS_LEN) != 0) {
-        isc_throw(OutOfRange, getLogPrefix() << " malformed: Addr Len=" << addr_length_
-                                             << " is not divisible by 16");
+        isc_throw(OutOfRange, getLogPrefix()
+                                  << "Addr Len=" << addr_length_ << " is not divisible by 16");
     }
 
     // As per draft-ietf-add-dnr 3.1.8:
@@ -110,13 +116,13 @@ Option6Dnr::unpackAddresses(OptionBufferConstIter& begin, OptionBufferConstIter
     // the option includes at least one valid IP address.
     if (addr_length_ == 0) {
         isc_throw(OutOfRange, getLogPrefix()
-                                  << " malformed: Addr Len=" << addr_length_
+                                  << "Addr Len=" << addr_length_
                                   << " but it must contain at least one valid IP address");
     }
 
     // Check if IPv6 Address(es) field is not truncated.
     if (std::distance(begin, end) < addr_length_) {
-        isc_throw(OutOfRange, getLogPrefix() << " malformed: Addr Len=" << addr_length_
+        isc_throw(OutOfRange, getLogPrefix() << "Addr Len=" << addr_length_
                                              << " but IPv6 address(es) are truncated to len="
                                              << std::distance(begin, end));
     }
@@ -124,7 +130,13 @@ Option6Dnr::unpackAddresses(OptionBufferConstIter& begin, OptionBufferConstIter
     // Let's unpack the ipv6-address(es).
     auto addr_end = begin + addr_length_;
     while (begin != addr_end) {
-        ip_addresses_.push_back(IOAddress::fromBytes(AF_INET6, &(*begin)));
+        try {
+            ip_addresses_.push_back(IOAddress::fromBytes(AF_INET6, &(*begin)));
+        } catch (const Exception& ex) {
+            isc_throw(BadValue, getLogPrefix() << "failed to parse IPv6 address"
+                                               << " - " << ex.what());
+        }
+
         begin += V6ADDRESS_LEN;
     }
 }
diff --git a/src/lib/dhcp/option6_dnr.h b/src/lib/dhcp/option6_dnr.h
index f76b401829..b157adafca 100644
--- a/src/lib/dhcp/option6_dnr.h
+++ b/src/lib/dhcp/option6_dnr.h
@@ -1,4 +1,4 @@
-// Copyright (C) 2018-2023 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2023 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -64,13 +64,66 @@ public:
     Option6Dnr(const uint16_t service_priority, const std::string& adn)
         : Option(V6, D6O_V6_DNR), DnrInstance(V6, service_priority, adn) {}
 
+    /// @brief Copies this option and returns a pointer to the copy.
+    ///
+    /// @return Pointer to the copy of the option.
     OptionPtr clone() const override;
+
+    /// @brief Writes option in wire-format to a buffer.
+    ///
+    /// Writes option in wire-format to buffer, returns pointer to first unused
+    /// byte after stored option (that is useful for writing options one after
+    /// another).
+    ///
+    /// @param buf pointer to a buffer
+    /// @param check flag which indicates if checking the option length is
+    /// required (used only in V4)
+    ///
+    /// @throw InvalidOptionDnrDomainName Thrown when Option's mandatory field ADN is empty.
     void pack(util::OutputBuffer& buf, bool check = false) const override;
+
+    /// @brief Parses received wire data buffer.
+    ///
+    /// @param begin iterator to first byte of option data
+    /// @param end iterator to end of option data (first byte after option end)
+    ///
+    /// @throw OutOfRange Thrown in case of truncated data.
+    /// @throw BadValue Thrown when @c DnrInstance::unpackAdn(begin,end) throws.
+    /// @throw InvalidOptionDnrDomainName Thrown when @c DnrInstance::unpackAdn(begin,end) throws.
     void unpack(OptionBufferConstIter begin, OptionBufferConstIter end) override;
+
+    /// @brief Returns string representation of the option.
+    ///
+    /// @param indent number of spaces before printing text
+    ///
+    /// @return string with text representation.
     std::string toText(int indent = 0) const override;
+
+    /// @brief Returns length of the complete option (data length + DHCPv4/DHCPv6
+    /// option header)
+    ///
+    /// @return length of the option
     uint16_t len() const override;
 
+    /// @brief Writes the IP address(es) in the wire format into a buffer.
+    ///
+    /// The IP address(es) (@c ip_addresses_) data is appended at the end
+    /// of the buffer.
+    ///
+    /// @param [out] buf buffer where IP address(es) will be written.
+    ///
+    /// @throw BadValue Thrown when trying to pack address which is not an IPv6 address
     void packAddresses(isc::util::OutputBuffer& buf) const override;
+
+    /// @brief Unpacks IP address(es) from wire data and stores it/them in @c ip_addresses_.
+    ///
+    /// It may throw in case of malformed data detected during parsing.
+    ///
+    /// @param begin beginning of the buffer from which the field will be read
+    /// @param end end of the buffer from which the field will be read
+    ///
+    /// @throw OutOfRange Thrown in case of malformed data detected during parsing e.g.
+    /// Addr Len not divisible by 16, Addr Len is 0, addresses data truncated etc.
     void unpackAddresses(OptionBufferConstIter& begin, OptionBufferConstIter end) override;
 };
 
diff --git a/src/lib/dhcp/tests/option4_dnr_unittest.cc b/src/lib/dhcp/tests/option4_dnr_unittest.cc
index 612ffecd9e..60f8cf31c2 100644
--- a/src/lib/dhcp/tests/option4_dnr_unittest.cc
+++ b/src/lib/dhcp/tests/option4_dnr_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2015-2023 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2023 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -360,6 +360,7 @@ TEST(Option4DnrTest, onWireDataCtor) {
         0x07, 0x65, 0x78, 0x61, 0x6D, 0x70, 0x6C, 0x65,  // example.
         0x03, 0x63, 0x6F, 0x6D, 0x00                     // com.
     };
+
     OptionBuffer buf(buf_data, buf_data + sizeof(buf_data));
     // Create option instance. Check that constructor doesn't throw.
     Option4DnrPtr option;
@@ -381,6 +382,7 @@ TEST(Option4DnrTest, unpackOneAdnOnly) {
         0x07, 0x65, 0x78, 0x61, 0x6D, 0x70, 0x6C, 0x65,  // example.
         0x03, 0x63, 0x6F, 0x6D, 0x00                     // com.
     };
+
     OptionBuffer buf(buf_data, buf_data + sizeof(buf_data));
     // Create option instance. Check that constructor doesn't throw.
     Option4DnrPtr option;
@@ -437,6 +439,7 @@ TEST(Option4DnrTest, unpackOneDnrInstance) {
         '4',  '=',  'v',  'a',  'l',  '2',  ' ',  'k',   // Svc Params
         'e',  'y',  '3',  '4',  '5'                      // Svc Params
     };
+
     OptionBuffer buf(buf_data, buf_data + sizeof(buf_data));
     // Create option instance. Check that constructor doesn't throw.
     Option4DnrPtr option;
@@ -490,6 +493,7 @@ TEST(Option4DnrTest, unpackMixedDnrInstances) {
         '4',  '=',  'v',  'a',  'l',  '2',  ' ',  'k',   // Svc Params
         'e',  'y',  '3',  '4',  '5'                      // Svc Params
     };
+
     OptionBuffer buf(buf_data, buf_data + sizeof(buf_data));
     // Create option instance. Check that constructor doesn't throw.
     Option4DnrPtr option;
@@ -537,6 +541,7 @@ TEST(Option4DnrTest, unpackTruncatedDnrInstanceDataLen) {
         0x03, 0x63, 0x6F, 0x6D, 0x00,                    // com.
         0x00, 62                                         // DNR Instance Data Len truncated
     };
+
     OptionBuffer buf(buf_data, buf_data + sizeof(buf_data));
 
     // Create option instance. Check that constructor throws an exception while doing unpack.
@@ -561,6 +566,7 @@ TEST(Option4DnrTest, unpackTruncatedDnrInstanceData) {
         21                                               // ADN Length is 21 dec
         // the rest of DNR instance data is truncated
     };
+
     OptionBuffer buf(buf_data, buf_data + sizeof(buf_data));
 
     // Create option instance. Check that constructor throws an exception while doing unpack.
@@ -585,11 +591,12 @@ TEST(Option4DnrTest, unpackTruncatedAdn) {
         21                                               // ADN Length is 21 dec
         // ADN data is missing.
     };
+
     OptionBuffer buf(buf_data, buf_data + sizeof(buf_data));
 
     // Create option instance. Check that constructor throws an exception while doing unpack.
     Option4DnrPtr option;
-    EXPECT_THROW(option.reset(new Option4Dnr(buf.begin(), buf.end())), OpaqueDataTupleError);
+    EXPECT_THROW(option.reset(new Option4Dnr(buf.begin(), buf.end())), BadValue);
     ASSERT_FALSE(option);
 }
 
@@ -609,6 +616,7 @@ TEST(Option4DnrTest, unpackInvalidFqdnAdn) {
         1,                                               // ADN Length is 1 dec
         ' '                                              // ADN contains only whitespace
     };
+
     OptionBuffer buf(buf_data, buf_data + sizeof(buf_data));
 
     // Create option instance. Check that constructor throws an exception while doing unpack.
@@ -632,6 +640,7 @@ TEST(Option4DnrTest, unpackNoFqdnAdn) {
         0x00, 0x02,                                      // Service priority is 2 dec
         0                                                // ADN Length is 0 dec
     };
+
     OptionBuffer buf(buf_data, buf_data + sizeof(buf_data));
 
     // Create option instance. Check that constructor throws an exception while doing unpack.
@@ -660,11 +669,12 @@ TEST(Option4DnrTest, unpackTruncatedIpAddress) {
         8                                                // Addr Len
         // the rest of DNR instance data is truncated.
     };
+
     OptionBuffer buf(buf_data, buf_data + sizeof(buf_data));
 
     // Create option instance. Check that constructor throws an exception while doing unpack.
     Option4DnrPtr option;
-    EXPECT_THROW(option.reset(new Option4Dnr(buf.begin(), buf.end())), OpaqueDataTupleError);
+    EXPECT_THROW(option.reset(new Option4Dnr(buf.begin(), buf.end())), BadValue);
     ASSERT_FALSE(option);
 }
 
@@ -687,6 +697,7 @@ TEST(Option4DnrTest, unpackNoIpAddress) {
         0x03, 0x63, 0x6F, 0x6D, 0x00,                    // com.
         0                                                // Addr Len = 0
     };
+
     OptionBuffer buf(buf_data, buf_data + sizeof(buf_data));
 
     // Create option instance. Check that constructor throws an exception while doing unpack.
@@ -716,6 +727,7 @@ TEST(Option4DnrTest, unpackIpAddressNon4Modulo) {
         192,  168,  0,    1,                             // IP address 1
         192,  168,  0                                    // IP address 2 truncated
     };
+
     OptionBuffer buf(buf_data, buf_data + sizeof(buf_data));
 
     // Create option instance. Check that constructor throws an exception while doing unpack.
@@ -746,6 +758,7 @@ TEST(Option4DnrTest, unpackvcParamsInvalidCharKey) {
         192,  168,  0,    2,                             // IP address 2 truncated
         'k',  'e',  'y',  '+',  '2',  '3'                // Svc Params key has forbidden char +
     };
+
     OptionBuffer buf(buf_data, buf_data + sizeof(buf_data));
 
     // Create option instance. Check that constructor throws an exception while doing unpack.
diff --git a/src/lib/dhcp/tests/option6_dnr_unittest.cc b/src/lib/dhcp/tests/option6_dnr_unittest.cc
index a6e616ffb1..29c082da68 100644
--- a/src/lib/dhcp/tests/option6_dnr_unittest.cc
+++ b/src/lib/dhcp/tests/option6_dnr_unittest.cc
@@ -1,4 +1,4 @@
-// Copyright (C) 2015-2023 Internet Systems Consortium, Inc. ("ISC")
+// Copyright (C) 2023 Internet Systems Consortium, Inc. ("ISC")
 //
 // This Source Code Form is subject to the terms of the Mozilla Public
 // License, v. 2.0. If a copy of the MPL was not distributed with this
@@ -122,9 +122,9 @@ TEST(Option6DnrTest, onWireCtorTruncatedFqdn) {
     };
 
     OptionBuffer buf(buf_data, buf_data + sizeof(buf_data));
-    // Create option instance. Check that constructor throws OpaqueDataTupleError exception.
+    // Create option instance. Check that constructor throws BadValue exception.
     Option6DnrPtr option;
-    EXPECT_THROW(option.reset(new Option6Dnr(buf.begin(), buf.end())), OpaqueDataTupleError);
+    EXPECT_THROW(option.reset(new Option6Dnr(buf.begin(), buf.end())), BadValue);
     ASSERT_FALSE(option);
 }