Refactor type and range validation in configuration update process (#9107)

* Refactor type and range validation in configuration update process

* Update server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

Co-authored-by: Henrique Sato <henriquesato2003@gmail.com>

* Remove duplicate imports

* Fix tests

* Address Joao's reviews

---------

Co-authored-by: Henrique Sato <henriquesato2003@gmail.com>

Author: winterhazel

server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

@@ -477,9 +477,9 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati
 
     private long _defaultPageSize = Long.parseLong(Config.DefaultPageSize.getDefaultValue());
     private static final String DOMAIN_NAME_PATTERN = "^((?!-)[A-Za-z0-9-]{1,63}(?<!-)\\.)+[A-Za-z]{1,63}$";
-    private Set<String> configValuesForValidation = new HashSet<String>();
-    private Set<String> weightBasedParametersForValidation = new HashSet<String>();
-    private Set<String> overprovisioningFactorsForValidation = new HashSet<String>();
+    private Set<String> configValuesForValidation = new HashSet<>();
+    private Set<String> weightBasedParametersForValidation = new HashSet<>();
+    private Set<String> overprovisioningFactorsForValidation = new HashSet<>();
 
     public static final ConfigKey<Boolean> SystemVMUseLocalStorage = new ConfigKey<Boolean>(Boolean.class, "system.vm.use.local.storage", "Advanced", "false",
             "Indicates whether to use local storage pools or shared storage pools for system VMs.", false, ConfigKey.Scope.Zone, null);
@@ -577,7 +577,7 @@ protected void populateConfigValuesForValidationSet() {
         configValuesForValidation.add(UnmanagedVMsManager.ConvertVmwareInstanceToKvmTimeout.key());
     }
 
-    private void weightBasedParametersForValidation() {
+    protected void weightBasedParametersForValidation() {
         weightBasedParametersForValidation.add(AlertManager.CPUCapacityThreshold.key());
         weightBasedParametersForValidation.add(AlertManager.StorageAllocatedCapacityThreshold.key());
         weightBasedParametersForValidation.add(AlertManager.StorageCapacityThreshold.key());
@@ -599,7 +599,7 @@ private void weightBasedParametersForValidation() {
         weightBasedParametersForValidation.add(ClusterDrsService.ClusterDrsImbalanceSkipThreshold.key());
     }
 
-    private void overProvisioningFactorsForValidation() {
+    protected void overProvisioningFactorsForValidation() {
         overprovisioningFactorsForValidation.add(CapacityManager.MemOverprovisioningFactor.key());
         overprovisioningFactorsForValidation.add(CapacityManager.CpuOverprovisioningFactor.key());
         overprovisioningFactorsForValidation.add(CapacityManager.StorageOverprovisioningFactor.key());
@@ -649,7 +649,7 @@ protected void validateIpAddressRelatedConfigValues(final String configName, fin
             }
         }
         if (err) {
-            throw new InvalidParameterValueException("Invalid IP address value(s) specified for the config value");
+            throw new InvalidParameterValueException("Invalid IP address value(s) specified for the config value.");
         }
     }
 
@@ -691,9 +691,8 @@ public boolean stop() {
     @DB
     public String updateConfiguration(final long userId, final String name, final String category, String value, final String scope, final Long resourceId) {
         final String validationMsg = validateConfigurationValue(name, value, scope);
-
         if (validationMsg != null) {
-            logger.error("Invalid configuration option, name: " + name + ", value:" + value);
+            logger.error("Invalid value [{}] for configuration [{}] due to [{}].", value, name, validationMsg);
             throw new InvalidParameterValueException(validationMsg);
         }
 
@@ -956,8 +955,6 @@ public Configuration updateConfiguration(final UpdateCfgCmd cmd) throws InvalidP
             category = config.getCategory();
         }
 
-        validateIpAddressRelatedConfigValues(name, value);
-
         if (value == null) {
             return _configDao.findByName(name);
         }
@@ -1192,14 +1189,21 @@ public Pair<Configuration, String> resetConfiguration(final ResetCfgCmd cmd) thr
         return new Pair<Configuration, String>(_configDao.findByName(name), newValue);
     }
 
-    protected String validateConfigurationValue(final String name, String value, final String scope) {
+    /**
+     * Validates whether a value is valid for the specified configuration. This includes type and range validation.
+     * @param name name of the configuration.
+     * @param value value to validate.
+     * @param scope scope of the configuration.
+     * @return null if the value is valid; otherwise, returns an error message.
+     */
+    protected String validateConfigurationValue(String name, String value, String scope) {
         final ConfigurationVO cfg = _configDao.findByName(name);
         if (cfg == null) {
             logger.error("Missing configuration variable " + name + " in configuration table");
             return "Invalid configuration variable.";
         }
 
-        final String configScope = cfg.getScope();
+        String configScope = cfg.getScope();
         if (scope != null) {
             if (!configScope.contains(scope) &&
                     !(ENABLE_ACCOUNT_SETTINGS_FOR_DOMAIN.value() && configScope.contains(ConfigKey.Scope.Account.toString()) &&
@@ -1208,11 +1212,11 @@ protected String validateConfigurationValue(final String name, String value, fin
                 return "Invalid scope id provided for the parameter " + name;
             }
         }
-        Class<?> type = null;
-        final Config configuration = Config.getConfig(name);
+        Class<?> type;
+        Config configuration = Config.getConfig(name);
         if (configuration == null) {
             logger.warn("Did not find configuration " + name + " in Config.java. Perhaps moved to ConfigDepot");
-            final ConfigKey<?> configKey = _configDepot.get(name);
+            ConfigKey<?> configKey = _configDepot.get(name);
             if(configKey == null) {
                 logger.warn("Did not find configuration " + name + " in ConfigDepot too.");
                 return null;
@@ -1221,144 +1225,161 @@ protected String validateConfigurationValue(final String name, String value, fin
         } else {
             type = configuration.getType();
         }
-        //no need to validate further if a
-        //config can have null value.
-        String errMsg = null;
-        try {
-            if (type.equals(Integer.class)) {
-                errMsg = "There was error in trying to parse value: " + value + ". Please enter a valid integer value for parameter " + name;
-                Integer.parseInt(value);
-            } else if (type.equals(Float.class)) {
-                errMsg = "There was error in trying to parse value: " + value + ". Please enter a valid float value for parameter " + name;
-                Float.parseFloat(value);
-            } else if (type.equals(Long.class)) {
-                errMsg = "There was error in trying to parse value: " + value + ". Please enter a valid long value for parameter " + name;
-                Long.parseLong(value);
-            }
-        } catch (final Exception e) {
-            // catching generic exception as some throws NullPointerException and some throws NumberFormatExcpeion
-            logger.error(errMsg);
-            return errMsg;
+
+        boolean isTypeValid = validateValueType(value, type);
+        if (!isTypeValid) {
+            return String.format("Value [%s] is not a valid [%s].", value, type);
         }
 
-        if (value == null) {
-            if (type.equals(Boolean.class)) {
-                return "Please enter either 'true' or 'false'.";
-            }
-            if (overprovisioningFactorsForValidation.contains(name)) {
-                final String msg = "value cannot be null for the parameter " + name;
-                logger.error(msg);
-                return msg;
-            }
-            return null;
+        return validateValueRange(name, value, type, configuration);
+    }
+
+    /**
+     * Returns whether a value is valid for a configuration of the provided type.
+     * Valid configuration values are:
+     *
+     * <ul>
+     *     <li>String: any value, including null;</li>
+     *     <li>Character: any value, including null;</li>
+     *     <li>Boolean: strings that equal "true" or "false" (case-sensitive);</li>
+     *     <li>Integer, Short, Long: strings that contain a valid int/short/long;</li>
+     *     <li>Float, Double: strings that contain a valid float/double, except infinity.</li>
+     * </ul>
+     *
+     * If a type isn't listed here, then the value will be considered invalid.
+     * @param value value to validate.
+     * @param type type of the configuration.
+     * @return boolean indicating whether the value is valid.
+     */
+    protected boolean validateValueType(String value, Class<?> type) {
+        if (type == String.class || type == Character.class) {
+            return true;
         }
 
-        value = value.trim();
         try {
-            if (overprovisioningFactorsForValidation.contains(name) && Float.parseFloat(value) <= 0f) {
-                final String msg = name + " should be greater than 0";
-                logger.error(msg);
-                throw new InvalidParameterValueException(msg);
+            if (type == Boolean.class) {
+                return value.equals("true") || value.equals("false");
+            } else if (type == Integer.class) {
+                Integer.parseInt(value);
+            } else if (type == Long.class) {
+                Long.parseLong(value);
+            } else if (type == Short.class) {
+                Short.parseShort(value);
+            } else if (type == Float.class) {
+                float floatValue = Float.parseFloat(value);
+                return !Float.isInfinite(floatValue);
+            } else if (type == Double.class) {
+                double doubleValue = Double.parseDouble(value);
+                return !Double.isInfinite(doubleValue);
+            } else {
+                return false;
             }
-        } catch (final NumberFormatException e) {
-            final String msg = "There was an error trying to parse the float value for: " + name;
-            logger.error(msg);
-            throw new InvalidParameterValueException(msg);
+            return true;
+        } catch (NullPointerException | NumberFormatException e) {
+            return false;
         }
+    }
 
-        if (type.equals(Boolean.class)) {
-            if (!(value.equals("true") || value.equals("false"))) {
-                logger.error("Configuration variable " + name + " is expecting true or false instead of " + value);
-                return "Please enter either 'true' or 'false'.";
+    /**
+     * If the specified configuration contains a range, validates if the value is in that range. If it doesn't contain
+     * a range, any value is considered valid.
+     * The value must be previously checked by `validateValueType` so there aren't casting exceptions here.
+     * @param name name of the configuration.
+     * @param value value to validate.
+     * @param type type of the value.
+     * @param configuration if the configuration uses Config instead of ConfigKey, the Config object; null otherwise.
+     * @return if the value is valid, returns null; if not, returns an error message.
+     */
+    protected String validateValueRange(String name, String value, Class<?> type, Config configuration) {
+        if (type.equals(Float.class)) {
+            Float val = Float.parseFloat(value);
+            if (overprovisioningFactorsForValidation.contains(name) && val <= 0f) {
+                return String.format("Value for configuration [%s] should be greater than 0.", name);
+            } else if (weightBasedParametersForValidation.contains(name) && (val < 0f || val > 1f)) {
+                return String.format("Please enter a value between 0 and 1 for the configuration parameter: [%s].", name);
             }
-            return null;
         }
 
         if (type.equals(Integer.class)) {
-            try {
-                final int val = Integer.parseInt(value);
-                if (NetworkModel.MACIdentifier.key().equalsIgnoreCase(name)) {
-                    //The value need to be between 0 to 255 because the mac generation needs a value of 8 bit
-                    //0 value is considered as disable.
-                    if(val < 0 || val > 255){
-                        throw new InvalidParameterValueException(name + " value should be between 0 and 255. 0 value will disable this feature");
-                    }
+            int val = Integer.parseInt(value);
+            if (NetworkModel.MACIdentifier.key().equalsIgnoreCase(name)) {
+                // The value needs to be between 0 to 255 because the MAC generation needs a value of 8 bits
+                // 0 is considered as disabled.
+                if (val < 0 || val > 255){
+                    return String.format("[%s] value should be between 0 and 255. 0 value will disable this feature.", name);
                 }
-
-                if (UnmanagedVMsManager.ThreadsOnMSToImportVMwareVMFiles.key().equalsIgnoreCase(name) || UnmanagedVMsManager.ThreadsOnKVMHostToImportVMwareVMFiles.key().equalsIgnoreCase(name)) {
-                    if (val > 10) {
-                        throw new InvalidParameterValueException("Please enter a value between 0 and 10 for the configuration parameter: " + name + ", -1 will disable it");
-                    }
+            }
+            if (UnmanagedVMsManager.ThreadsOnMSToImportVMwareVMFiles.key().equalsIgnoreCase(name) ||
+                    UnmanagedVMsManager.ThreadsOnKVMHostToImportVMwareVMFiles.key().equalsIgnoreCase(name)) {
+                if (val < -1 || val > 10) {
+                    return String.format("Please enter a value between -1 and 10 for the configuration parameter: [%s]. -1 will disable it.", name);
                 }
-
-                if (configValuesForValidation.contains(name)) {
-                    if (val <= 0) {
-                        throw new InvalidParameterValueException("Please enter a positive value for the configuration parameter:" + name);
-                    }
-                    if ("vm.password.length".equalsIgnoreCase(name) && val < 6) {
-                        throw new InvalidParameterValueException("Please enter a value greater than 5 for the configuration parameter:" + name);
-                    }
-                    if ("remote.access.vpn.psk.length".equalsIgnoreCase(name)) {
-                        if (val < 8) {
-                            throw new InvalidParameterValueException("Please enter a value greater than 7 for the configuration parameter:" + name);
-                        }
-                        if (val > 256) {
-                            throw new InvalidParameterValueException("Please enter a value less than 257 for the configuration parameter:" + name);
-                        }
-                    }
-                    if (UserDataManager.VM_USERDATA_MAX_LENGTH_STRING.equalsIgnoreCase(name)) {
-                        if (val > 1048576) {
-                            throw new InvalidParameterValueException("Please enter a value less than 1048576 for the configuration parameter:" + name);
-                        }
-                    }
+            } else if (configValuesForValidation.contains(name)) {
+                if (val <= 0) {
+                    return String.format("Please enter a positive value for the configuration parameter: [%s].", name);
                 }
-            } catch (final NumberFormatException e) {
-                logger.error("There was an error trying to parse the integer value for configuration parameter: " + name);
-                throw new InvalidParameterValueException("There was an error trying to parse the integer value for configuration parameter: " + name);
-            }
-        }
-
-        if (type.equals(Float.class)) {
-            try {
-                final Float val = Float.parseFloat(value);
-                if (weightBasedParametersForValidation.contains(name) && (val < 0f || val > 1f)) {
-                    throw new InvalidParameterValueException("Please enter a value between 0 and 1 for the configuration parameter: " + name);
+                if ("vm.password.length".equalsIgnoreCase(name) && val < 6) {
+                    return String.format("Please enter a value greater than 5 for the configuration parameter: [%s].",  name);
+                }
+                if ("remote.access.vpn.psk.length".equalsIgnoreCase(name) && (val < 8 || val > 256)) {
+                    return String.format("Please enter a value greater than 7 and less than 257 for the configuration parameter: [%s].", name);
+                }
+                if (UserDataManager.VM_USERDATA_MAX_LENGTH_STRING.equalsIgnoreCase(name) && val > 1048576) {
+                    return String.format("Please enter a value less than 1048577 for the configuration parameter: [%s].", name);
                 }
-            } catch (final NumberFormatException e) {
-                logger.error("There was an error trying to parse the float value for configuration parameter: " + name);
-                throw new InvalidParameterValueException("There was an error trying to parse the float value for configuration parameter: " + name);
             }
         }
 
         if (type.equals(String.class)) {
-            if (name.equalsIgnoreCase(SecStorageAllowedInternalDownloadSites.key()) && StringUtils.isNotEmpty(value)) {
+            if (SecStorageAllowedInternalDownloadSites.key().equalsIgnoreCase(name) && StringUtils.isNotEmpty(value)) {
                 final String[] cidrs = value.split(",");
                 for (final String cidr : cidrs) {
                     if (!NetUtils.isValidIp4(cidr) && !NetUtils.isValidIp6(cidr) && !NetUtils.getCleanIp4Cidr(cidr).equals(cidr)) {
-                        logger.error(String.format("Invalid CIDR %s value specified for the config %s", cidr, name));
-                        throw new InvalidParameterValueException(String.format("Invalid CIDR %s value specified for the config %s", cidr, name));
+                        return String.format("Invalid CIDR %s value specified for the config %s.", cidr, name);
                     }
                 }
             }
         }
 
-        if (configuration == null ) {
-            //range validation has to be done per case basis, for now
-            //return in case of Configkey parameters
-            return null;
-        }
+        validateIpAddressRelatedConfigValues(name, value);
 
-        if (configuration.getRange() == null) {
+        if (!shouldValidateConfigRange(name, value, configuration)) {
             return null;
         }
-        String[] range = configuration.getRange().split(",");
 
-        if (type.equals(String.class)) {
-            return validateIfStringValueIsInRange(name, value, range);
-        } else if (type.equals(Integer.class)) {
+        String[] range = configuration.getRange().split(",");
+        if (type.equals(Integer.class)) {
             return validateIfIntValueIsInRange(name, value, range[0]);
         }
-        return String.format("Invalid value for configuration [%s].", name);
+        return validateIfStringValueIsInRange(name, value, range);
+    }
+
+    /**
+     * Returns a boolean indicating whether a Config's range should be validated. It should not be validated when:</br>
+     * <ul>
+     *  <li>The value is null;</li>
+     *  <li>The configuration uses ConfigKey instead of Config;</li>
+     *  <li>The Config does not have a specified range.</li>
+     * </ul>
+     */
+    protected boolean shouldValidateConfigRange(String name, String value, Config configuration) {
+        if (value == null) {
+            logger.debug("Not proceeding with configuration [{}]'s range validation, as its provided value is null.", name);
+            return false;
+        }
+
+        if (configuration == null) {
+            logger.debug("Not proceeding with configuration [{}]'s range validation, as it uses ConfigKey instead of Config.", name);
+            return false;
+        }
+
+        if (configuration.getRange() == null) {
+            logger.debug("Not proceeding with configuration [{}]'s range validation, as it does not have a specified range.", name);
+            return false;
+        }
+
+        logger.debug("Proceeding with configuration [{}]'s range validation.", name);
+        return true;
     }
 
     /**