[#1456] Checkpoint: code done, UTs to do

src/bin/dhcp4/json_config_parser.cc

@@ -182,6 +182,9 @@ public:
     void
     sanityChecks(const SrvConfigPtr& cfg, const ConstElementPtr& global) {
 
+        /// Global lifetime sanity checks
+        cfg->sanityChecksLifetime("valid-lifetime");
+
         /// Shared network sanity checks
         const SharedNetwork4Collection* networks = cfg->getCfgSharedNetworks4()->getAll();
         if (networks) {

src/bin/dhcp6/json_config_parser.cc

@@ -269,6 +269,10 @@ public:
     void
     sanityChecks(const SrvConfigPtr& cfg, const ConstElementPtr& global) {
 
+        /// Global lifetime sanity checks
+        cfg->sanityChecksLifetime("preferred-lifetime");
+        cfg->sanityChecksLifetime("valid-lifetime");
+
         /// Shared network sanity checks
         const SharedNetwork6Collection* networks = cfg->getCfgSharedNetworks6()->getAll();
         if (networks) {

src/bin/dhcp6/tests/config_parser_unittest.cc

@@ -737,6 +737,7 @@ public:
             "\"dhcp-ddns\": { \"enable-updates\" : false }, "
             "\"option-def\": [ ], "
             "\"option-data\": [ ] }";
+        CfgMgr::instance().rollback();
         static_cast<void>(executeConfiguration(config,
                                                "reset configuration database"));
         // The default setting is to listen on all interfaces. In order to

src/lib/dhcpsrv/cb_ctl_dhcp4.cc

@@ -79,6 +79,9 @@ CBControlDHCPv4::databaseConfigApply(const BackendSelector& backend_selector,
             // Add defaults.
             external_cfg->applyDefaultsConfiguredGlobals(SimpleParser4::GLOBAL4_DEFAULTS);
 
+            // Sanity check it.
+            external_cfg->sanityChecksLifetime("valid-lifetime");
+
             // Now that we successfully fetched the new global parameters, let's
             // remove existing ones and merge them into the current configuration.
             cfg->clearConfiguredGlobals();
@@ -234,6 +237,8 @@ CBControlDHCPv4::databaseConfigApply(const BackendSelector& backend_selector,
         // If we're configuring the server after startup, we do not apply the
         // ip-reservations-unique setting here. It will be applied when the
         // configuration is committed.
+        auto const& cfg = CfgMgr::instance().getStagingCfg();
+        external_cfg->sanityChecksLifetime(*cfg, "valid-lifetime");
         CfgMgr::instance().mergeIntoStagingCfg(external_cfg->getSequence());
     } else {
         if (globals_fetched) {
@@ -253,6 +258,8 @@ CBControlDHCPv4::databaseConfigApply(const BackendSelector& backend_selector,
                 external_cfg->addConfiguredGlobal("ip-reservations-unique", Element::create(true));
             }
         }
+        auto const& cfg = CfgMgr::instance().getCurrentCfg();
+        external_cfg->sanityChecksLifetime(*cfg, "valid-lifetime");
         CfgMgr::instance().mergeIntoCurrentCfg(external_cfg->getSequence());
     }
     LOG_INFO(dhcpsrv_logger, DHCPSRV_CFGMGR_CONFIG4_MERGED);

src/lib/dhcpsrv/cb_ctl_dhcp6.cc

@@ -78,6 +78,10 @@ CBControlDHCPv6::databaseConfigApply(const db::BackendSelector& backend_selector
             // Add defaults.
             external_cfg->applyDefaultsConfiguredGlobals(SimpleParser6::GLOBAL6_DEFAULTS);
 
+            // Sanity check it.
+            external_cfg->sanityChecksLifetime("preferred-lifetime");
+            external_cfg->sanityChecksLifetime("valid-lifetime");
+
             // Now that we successfully fetched the new global parameters, let's
             // remove existing ones and merge them into the current configuration.
             cfg->clearConfiguredGlobals();
@@ -233,6 +237,9 @@ CBControlDHCPv6::databaseConfigApply(const db::BackendSelector& backend_selector
         // If we're configuring the server after startup, we do not apply the
         // ip-reservations-unique setting here. It will be applied when the
         // configuration is committed.
+        auto const& cfg = CfgMgr::instance().getStagingCfg();
+        external_cfg->sanityChecksLifetime(*cfg, "preferred-lifetime");
+        external_cfg->sanityChecksLifetime(*cfg, "valid-lifetime");
         CfgMgr::instance().mergeIntoStagingCfg(external_cfg->getSequence());
     } else {
         if (globals_fetched) {
@@ -252,6 +259,9 @@ CBControlDHCPv6::databaseConfigApply(const db::BackendSelector& backend_selector
                 external_cfg->addConfiguredGlobal("ip-reservations-unique", Element::create(true));
             }
         }
+        auto const& cfg = CfgMgr::instance().getCurrentCfg();
+        external_cfg->sanityChecksLifetime(*cfg, "preferred-lifetime");
+        external_cfg->sanityChecksLifetime(*cfg, "valid-lifetime");
         CfgMgr::instance().mergeIntoCurrentCfg(external_cfg->getSequence());
     }
     LOG_INFO(dhcpsrv_logger, DHCPSRV_CFGMGR_CONFIG6_MERGED);

src/lib/dhcpsrv/network.cc

@@ -101,7 +101,8 @@ Network::getRequiredClasses() const {
 
 Optional<IOAddress>
 Network::getGlobalProperty(Optional<IOAddress> property,
-                           const std::string& global_name) const {
+                           const std::string& global_name,
+                           bool /*triplet*/) const {
     if (!global_name.empty() && fetch_globals_fn_) {
         ConstElementPtr globals = fetch_globals_fn_();
         if (globals && (globals->getType() == Element::map)) {

src/lib/dhcpsrv/network.h

@@ -333,7 +333,7 @@ public:
     /// @param inheritance inheritance mode to be used.
     Triplet<uint32_t> getValid(const Inheritance& inheritance = Inheritance::ALL) const {
         return (getProperty<Network>(&Network::getValid, valid_, inheritance,
-                                     "valid-lifetime"));
+                                     "valid-lifetime", true));
     }
 
     /// @brief Sets new valid lifetime for a network.
@@ -556,7 +556,7 @@ public:
             (inheritance != Inheritance::PARENT_NETWORK)) {
             // Get global mode.
             util::Optional<std::string> mode_label;
-            mode_label = getGlobalProperty(mode_label, "ddns-replace-client-name");
+            mode_label = getGlobalProperty(mode_label, "ddns-replace-client-name", false);
             if (!mode_label.unspecified()) {
                 try {
                     // If the mode is globally configured, convert it to an enum.
@@ -769,7 +769,8 @@ protected:
     /// of @c property.
     template<typename ReturnType>
     ReturnType getGlobalProperty(ReturnType property,
-                                 const std::string& global_name) const {
+                                 const std::string& global_name,
+                                 bool /*triplet*/) const {
         if (!global_name.empty() && fetch_globals_fn_) {
             data::ConstElementPtr globals = fetch_globals_fn_();
             if (globals && (globals->getType() == data::Element::map)) {
@@ -784,6 +785,59 @@ protected:
         return (property);
     }
 
+    /// @brief The @c getGlobalProperty specialization for Triplet<T>.
+    ///
+    /// The behavior depends on the triplet argument:
+    ///  - if true it tries to get min and max values too
+    ///  - if false the property is implemented as an Optional
+    ///
+    /// @note: use overloading vs specialization because full specialization
+    /// is not allowed in this scope.
+    ///
+    /// @tparam IntType Type of the encapsulated value(s).
+    ///
+    /// @param property Value to be returned when it is specified or when
+    /// no global value is found.
+    /// @param global_name Name of the global parameter which value should
+    /// be returned
+    /// @param triplet False (default) when has no min and max value so
+    /// is in fact only an @c OptionalValue, true otherwise.
+    ///
+    /// @return Optional value fetched from the global level or the value
+    /// of @c property.
+    template<typename IntType>
+    Triplet<IntType> getGlobalProperty(Triplet<IntType> property,
+                                       const std::string& global_name,
+                                       bool triplet) const {
+        if (!global_name.empty() && fetch_globals_fn_) {
+            data::ConstElementPtr globals = fetch_globals_fn_();
+            if (globals && (globals->getType() == data::Element::map)) {
+                data::ConstElementPtr param = globals->get(global_name);
+                if (param) {
+                    IntType def_value = static_cast<IntType>(param->intValue());
+                    if (!triplet) {
+                        return (def_value);
+                    } else {
+                        IntType min_value = def_value;
+                        IntType max_value = def_value;
+                        const std::string& min_name = "min-" + global_name;
+                        data::ConstElementPtr min_param = globals->get(min_name);
+                        if (min_param) {
+                            min_value = static_cast<IntType>(min_param->intValue());
+                        }
+                        const std::string& max_name = "max-" + global_name;
+                        data::ConstElementPtr max_param = globals->get(max_name);
+                        if (min_param) {
+                            min_value = static_cast<IntType>(min_param->intValue());
+                        }
+                        return (Triplet<IntType>(min_value, def_value, max_value));
+                    }
+                }
+            }
+        }
+        return (property);
+    }
+
     /// @brief The @c getGlobalProperty specialization for Optional<IOAddress>.
     ///
     /// This does two things:
@@ -802,7 +856,8 @@ protected:
     /// of @c property.
     util::Optional<asiolink::IOAddress>
     getGlobalProperty(util::Optional<asiolink::IOAddress> property,
-                      const std::string& global_name) const;
+                      const std::string& global_name,
+                      bool /*triplet*/) const;
 
     /// @brief Returns a value associated with a network using inheritance.
     ///
@@ -827,6 +882,8 @@ protected:
     /// level. This value is empty by default, which indicates that the
     /// global value for the given parameter is not supported and shouldn't
     /// be fetched.
+    /// @param triplet False (default) when has no min and max value so
+    /// is in fact only an @c OptionalValue, true otherwise.
     ///
     /// @return Optional value fetched from this instance level, parent
     /// network level or global level
@@ -834,7 +891,8 @@ protected:
     ReturnType getProperty(ReturnType(BaseType::*MethodPointer)(const Inheritance&) const,
                            ReturnType property,
                            const Inheritance& inheritance,
-                           const std::string& global_name = "") const {
+                           const std::string& global_name = "",
+                           bool triplet = false) const {
 
         // If no inheritance is to be used, return the value for this
         // network regardless if it is specified or not.
@@ -853,7 +911,7 @@ protected:
 
         // If global value requested, return it.
         } else if (inheritance == Inheritance::GLOBAL) {
-            return (getGlobalProperty(ReturnType(), global_name));
+            return (getGlobalProperty(ReturnType(), global_name, triplet));
         }
 
         // We use inheritance and the value is not specified on the network level.
@@ -876,7 +934,7 @@ protected:
             // can be specified on global level and there is a callback
             // that returns the global values, try to find this parameter
             // at the global scope.
-            return (getGlobalProperty(property, global_name));
+            return (getGlobalProperty(property, global_name, triplet));
         }
 
         // We haven't found the value at any level, so return the unspecified.

src/lib/dhcpsrv/srv_config.cc

@@ -248,7 +248,7 @@ SrvConfig::mergeGlobals(SrvConfig& other) {
             }
         } catch(const std::exception& ex) {
             isc_throw (BadValue, "Invalid value:" << element->str()
-                       << " explict global:" << name);
+                       << " explicit global:" << name);
         }
     }
 }
@@ -400,6 +400,179 @@ SrvConfig::extractConfiguredGlobals(isc::data::ConstElementPtr config) {
     }
 }
 
+void
+SrvConfig::sanityChecksLifetime(const std::string& name) const {
+    // Initialize as some compilers complain otherwise.
+    uint32_t value = 0;
+    ConstElementPtr has_value = getConfiguredGlobal(name);
+    if (has_value) {
+        value = has_value->intValue();
+    }
+
+    uint32_t min_value = 0;
+    ConstElementPtr has_min = getConfiguredGlobal("min-" + name);
+    if (has_min) {
+        min_value = has_min->intValue();
+    }
+
+    uint32_t max_value = 0;
+    ConstElementPtr has_max = getConfiguredGlobal("max-" + name);
+    if (has_max) {
+        max_value = has_max->intValue();
+    }
+
+    if (!has_value && !has_min && !has_max) {
+        return;
+    }
+    if (has_value) {
+        if (!has_min && !has_max) {
+            // default only.
+            min_value = value;
+            max_value = value;
+        } else if (!has_min) {
+            // default and max.
+            min_value = value;
+        } else if (!has_max) {
+            // default and min.
+            max_value = value;
+        }
+    } else if (has_min) {
+        // min only.
+        if (!has_max) {
+            value = min_value;
+            max_value = min_value;
+        } else {
+            // min and max.
+            isc_throw(BadValue, "have min-" << name << " and max-"
+                      << name << " but no " << name << " (default)");
+        }
+    } else {
+        // max only.
+        min_value = max_value;
+        value = max_value;
+    }
+
+    // Check that min <= max.
+    if (min_value > max_value) {
+        if (has_min && has_max) {
+            isc_throw(BadValue, "the value of min-" << name << " ("
+                      << min_value << ") is not less than max-" << name << " ("
+                      << max_value << ")");
+        } else if (has_min) {
+            // Only min and default so min > default.
+            isc_throw(BadValue, "the value of min-" << name << " ("
+                      << min_value << ") is not less than (default) " << name
+                      << " (" << value << ")");
+        } else {
+            // Only default and max so default > max.
+            isc_throw(BadValue, "the value of (default) " << name
+                      << " (" << value << ") is not less than max-" << name
+                      << " (" << max_value << ")");
+        }
+    }
+
+    // Check that value is between min and max.
+    if ((value < min_value) || (value > max_value)) {
+        isc_throw(BadValue, "the value of (default) " << name << " ("
+                  << value << ") is not between min-" << name << " ("
+                  << min_value << ") and max-" << name << " ("
+                  << max_value << ")");
+    }
+}
+
+void
+SrvConfig::sanityChecksLifetime(const SrvConfig& target_config,
+                                const std::string& name) const {
+    // Three cases:
+    //  - the external/source config has the parameter: use it.
+    //  - only the target config has the parameter: use this one.
+    //  - no config has the parameter.
+    uint32_t value = 0;
+    ConstElementPtr has_value = getConfiguredGlobal(name);
+    if (!has_value) {
+        has_value = target_config.getConfiguredGlobal(name);
+    }
+    if (has_value) {
+        value = has_value->intValue();
+    }
+
+    uint32_t min_value = 0;
+    ConstElementPtr has_min = getConfiguredGlobal("min-" + name);
+    if (!has_min) {
+        has_min = target_config.getConfiguredGlobal("min-" + name);
+    }
+    if (has_min) {
+        min_value = has_min->intValue();
+    }
+
+    uint32_t max_value = 0;
+    ConstElementPtr has_max = getConfiguredGlobal("max-" + name);
+    if (!has_max) {
+        has_max = target_config.getConfiguredGlobal("max-" + name);
+    }
+    if (has_max) {
+        max_value = has_max->intValue();
+    }
+
+    if (!has_value && !has_min && !has_max) {
+        return;
+    }
+    if (has_value) {
+        if (!has_min && !has_max) {
+            // default only.
+            min_value = value;
+            max_value = value;
+        } else if (!has_min) {
+            // default and max.
+            min_value = value;
+        } else if (!has_max) {
+            // default and min.
+            max_value = value;
+        }
+    } else if (has_min) {
+        // min only.
+        if (!has_max) {
+            value = min_value;
+            max_value = min_value;
+        } else {
+            // min and max.
+            isc_throw(BadValue, "have min-" << name << " and max-"
+                      << name << " but no " << name << " (default)");
+        }
+    } else {
+        // max only.
+        min_value = max_value;
+        value = max_value;
+    }
+
+    // Check that min <= max.
+    if (min_value > max_value) {
+        if (has_min && has_max) {
+            isc_throw(BadValue, "the value of min-" << name << " ("
+                      << min_value << ") is not less than max-" << name << " ("
+                      << max_value << ")");
+        } else if (has_min) {
+            // Only min and default so min > default.
+            isc_throw(BadValue, "the value of min-" << name << " ("
+                      << min_value << ") is not less than (default) " << name
+                      << " (" << value << ")");
+        } else {
+            // Only default and max so default > max.
+            isc_throw(BadValue, "the value of (default) " << name
+                      << " (" << value << ") is not less than max-" << name
+                      << " (" << max_value << ")");
+        }
+    }
+
+    // Check that value is between min and max.
+    if ((value < min_value) || (value > max_value)) {
+        isc_throw(BadValue, "the value of (default) " << name << " ("
+                  << value << ") is not between min-" << name << " ("
+                  << min_value << ") and max-" << name << " ("
+                  << max_value << ")");
+    }
+}
+
 ElementPtr
 SrvConfig::toElement() const {
     // Toplevel map

src/lib/dhcpsrv/srv_config.h

@@ -793,6 +793,19 @@ public:
         configured_globals_->set(name, value);
     }
 
+    /// @brief Conducts sanity checks on global lifetime parameters.
+    ///
+    /// @param name Base name of the lifetime parameter.
+    void sanityChecksLifetime(const std::string& name) const;
+
+    /// @brief Conducts sanity checks on global lifetime parameters
+    /// before merge from the external configuration to the target one.
+    ///
+    /// @param target_config Target configuration.
+    /// @param name Base name of the lifetime parameter.
+    void sanityChecksLifetime(const SrvConfig& target_config,
+                              const std::string& name) const;
+
     /// @brief Moves deprecated parameters from dhcp-ddns element to global element
     ///
     /// Given a server configuration element map, the following parameters are moved