Skip to content

Commit

Permalink
Refactor type and range validation in configuration update process (#…
Browse files Browse the repository at this point in the history 
…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 <[email protected]>

* Remove duplicate imports

* Fix tests

* Address Joao's reviews

---------

Co-authored-by: Henrique Sato <[email protected]>
  • Loading branch information
@winterhazel @hsato03
winterhazel and hsato03 authored Sep 6, 2024
1 parent b11897c commit f156c4e
Show file tree
Hide file tree
Showing 2 changed files with 402 additions and 150 deletions.
259 changes: 140 additions & 119 deletions server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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());
Expand All @@ -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());
Expand Down Expand Up @@ -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.");
}
}

Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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()) &&
Expand All @@ -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;
Expand All @@ -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;
}

/**
Expand Down
Loading

0 comments on commit f156c4e

Please sign in to comment.