Skip to content

Refactor type and range validation in configuration update process #9107

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JoaoJandre merged 9 commits into from
Sep 6, 2024
Merged

Refactor type and range validation in configuration update process #9107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
91ca46c
Refactor type and range validation in configuration update process
winterhazel Mar 6, 2024
dbcb349
Update server/src/main/java/com/cloud/configuration/ConfigurationMana…
winterhazel Jun 14, 2024
46e6be3
Merge main
winterhazel Jun 27, 2024
e7a8fd6
Merge main
winterhazel Jul 1, 2024
bc4e12d
Remove duplicate imports
winterhazel Jul 1, 2024
229178a
Fix tests
winterhazel Jul 1, 2024
efd2c26
Address Joao's reviews
winterhazel Aug 22, 2024
1cb6a99
Merge branch 'main' into updateconfig-refactor
winterhazel Aug 22, 2024
ef851d8
Merge main
winterhazel Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
259 changes: 140 additions & 119 deletions server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,9 +476,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 @@ -575,7 +575,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 @@ -597,7 +597,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 @@ -647,7 +647,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 @@ -689,9 +689,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 @@ -951,8 +950,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 @@ -1184,14 +1181,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 @@ -1200,11 +1204,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 @@ -1213,144 +1217,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