DaanHoogland commented on code in PR #6808:
URL: https://github.com/apache/cloudstack/pull/6808#discussion_r992039424
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -1149,20 +1151,20 @@ private String validateConfigurationValue(final String
name, String value, final
value = value.trim();
try {
- if (overprovisioningFactorsForValidation.contains(name) &&
Float.parseFloat(value) <= 0f) {
- final String msg = name + " should be greater than 0";
+ if (overprovisioningFactorsForValidation.contains(name) &&
Float.parseFloat(value) < 1f) {
+ final String msg = String.format("[%s] should be greater than
or equal to 1.", name);
s_logger.error(msg);
throw new InvalidParameterValueException(msg);
}
} catch (final NumberFormatException e) {
- final String msg = "There was an error trying to parse the float
value for: " + name;
+ final String msg = String.format("There was an error trying to
parse the float value for: [%s].", name);
Review Comment:
this format string is used at 1222 and 1223 as well. let's define a constant?
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -1171,147 +1173,205 @@ private String validateConfigurationValue(final
String name, String value, final
if (type.equals(Integer.class) &&
NetworkModel.MACIdentifier.key().equalsIgnoreCase(name)) {
try {
final int val = Integer.parseInt(value);
- //The value need to be between 0 to 255 because the mac
generation needs a value of 8 bit
+ //The value needs 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");
+ throw new
InvalidParameterValueException(String.format("[%s] value should be between 0
and 255. 0 value will disable this feature.", name));
}
} catch (final NumberFormatException e) {
- s_logger.error("There was an error trying to parse the integer
value for:" + name);
- throw new InvalidParameterValueException("There was an error
trying to parse the integer value for:" + name);
+ s_logger.error(String.format("There was an error trying to
parse the integer value for: [%s].", name));
+ throw new InvalidParameterValueException(String.format("There
was an error trying to parse the integer value for: [%s].", name));
}
}
if (type.equals(Integer.class) &&
configValuesForValidation.contains(name)) {
try {
final int val = Integer.parseInt(value);
if (val <= 0) {
- throw new InvalidParameterValueException("Please enter a
positive value for the configuration parameter:" + name);
+ throw new
InvalidParameterValueException(String.format("Please enter a positive value for
the configuration parameter: [%s].", name));
}
if ("vm.password.length".equalsIgnoreCase(name) && val < 6) {
- throw new InvalidParameterValueException("Please enter a
value greater than 5 for the configuration parameter:" + name);
+ throw new
InvalidParameterValueException(String.format("Please enter a value greater than
5 for the configuration parameter: [%s].", 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);
+ throw new
InvalidParameterValueException(String.format("Please enter a value greater than
7 for the configuration parameter: [%s].", name));
}
if (val > 256) {
- throw new InvalidParameterValueException("Please enter
a value less than 257 for the configuration parameter:" + name);
+ throw new
InvalidParameterValueException(String.format("Please enter a value less than
257 for the configuration parameter: [%s].", name));
}
}
if (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);
+ throw new
InvalidParameterValueException(String.format("Please enter a value less than
1048576 for the configuration parameter: [%s].", name));
}
}
} catch (final NumberFormatException e) {
- s_logger.error("There was an error trying to parse the integer
value for:" + name);
- throw new InvalidParameterValueException("There was an error
trying to parse the integer value for:" + name);
+ s_logger.error(String.format("There was an error trying to
parse the integer value for: [%s].", name));
+ throw new InvalidParameterValueException(String.format("There
was an error trying to parse the integer value for: [%s].", 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);
+ throw new
InvalidParameterValueException(String.format("Please enter a value between 0
and 1 for the configuration parameter: [%s].", name));
}
} catch (final NumberFormatException e) {
- s_logger.error("There was an error trying to parse the float
value for:" + name);
- throw new InvalidParameterValueException("There was an error
trying to parse the float value for:" + name);
+ s_logger.error(String.format("There was an error trying to
parse the float value for: [%s].", name));
+ throw new InvalidParameterValueException(String.format("There
was an error trying to parse the float value for: [%s].", name));
}
}
- if(c == null ) {
+ if(configuration == null ) {
//range validation has to be done per case basis, for now
//return in case of Configkey parameters
return null;
}
- final String range = c.getRange();
+ final String[] range = configuration.getRange();
if (range == null) {
return null;
}
if (type.equals(String.class)) {
- if (range.equals("privateip")) {
- try {
- if (!NetUtils.isSiteLocalAddress(value)) {
- s_logger.error("privateip range " + value + " is not a
site local address for configuration variable " + name);
- return "Please enter a site local IP address.";
- }
- } catch (final NullPointerException e) {
- s_logger.error("Error parsing ip address for " + name);
- throw new InvalidParameterValueException("Error parsing ip
address");
- }
- } else if (range.equals("netmask")) {
- if (!NetUtils.isValidIp4Netmask(value)) {
- s_logger.error("netmask " + value + " is not a valid net
mask for configuration variable " + name);
- return "Please enter a valid netmask.";
- }
- } else if (range.equals("hypervisorList")) {
- final String[] hypervisors = value.split(",");
- if (hypervisors == null) {
- return "Please enter hypervisor list, separated by comma";
- }
- for (final String hypervisor : hypervisors) {
- if (HypervisorType.getType(hypervisor) ==
HypervisorType.Any || HypervisorType.getType(hypervisor) ==
HypervisorType.None) {
- return "Please enter a valid hypervisor type";
- }
- }
- } else if (range.equalsIgnoreCase("instanceName")) {
- if (!NetUtils.verifyInstanceName(value)) {
- return "Instance name can not contain hyphen, space or
plus sign";
- }
- } else if (range.equalsIgnoreCase("domainName")) {
- String domainName = value;
- if (value.startsWith("*")) {
- domainName = value.substring(2); //skip the "*."
- }
- //max length for FQDN is 253 + 2, code adds xxx-xxx-xxx-xxx to
domain name when creating URL
- if (domainName.length() >= 238 ||
!domainName.matches(DOMAIN_NAME_PATTERN)) {
- return "Please enter a valid string for domain name,
prefixed with '*.' if applicable";
- }
- } else if (range.equals("routes")) {
- final String[] routes = value.split(",");
- for (final String route : routes) {
- if (route != null) {
- final String routeToVerify = route.trim();
- if (!NetUtils.isValidIp4Cidr(routeToVerify)) {
- throw new InvalidParameterValueException("Invalid
value for route: " + route + " in deny list. Valid format is list"
- + " of cidrs separated by coma. Example:
10.1.1.0/24,192.168.0.0/24");
- }
- }
- }
- } else {
- final String[] options = range.split(",");
- for (final String option : options) {
- if (option.trim().equalsIgnoreCase(value)) {
- return null;
- }
- }
- s_logger.error("configuration value for " + name + " is
invalid");
- return "Please enter : " + range;
+ return validateIfStringValueIsInRange(name, value, range);
+ } else if (type.equals(Integer.class)) {
+ return validateIfIntValueIsInRange(name, value, range[0]);
+ }
+ return String.format("Invalid value for configuration [%s].", name);
+ }
+ protected String validateIfIntValueIsInRange(String name, String value,
String range) {
+ final String[] options = range.split("-");
+ if (options.length != 2) {
+ final String msg = String.format("Configuration range [%1$s] for
[%2$s] is invalid.", range, name);
+ s_logger.error(msg);
+ return msg;
+ }
+ final int min = Integer.parseInt(options[0]);
+ final int max = Integer.parseInt(options[1]);
+ final int val = Integer.parseInt(value);
+ if (val < min || val > max) {
+ s_logger.error(String.format("Configuration value for [%s] is
invalid. Please enter a value in the range: [%s].", name, range));
+ return String.format("Please enter a value in the range: [%s].",
range);
+ }
+ return null;
+ }
+
+
+ private String validateIfStringValueIsInRange(String name, String value,
String... range) {
+ String message = "";
+ String errMessage = "";
+ for (String rangeOption : range) {
+ if (StringUtils.isNotBlank(message)) {
+ message = String.format("%s|| ", message);
+ }
+ switch (rangeOption) {
+ case "privateip":
+ errMessage = validateRangePrivateIp(name, value);
+ break;
+ case "netmask":
+ errMessage = validateRangeNetmask(name, value);
+ break;
+ case "hypervisorList":
+ errMessage = validateRangeHypervisorList(value);
+ break;
+ case "instanceName":
+ errMessage = validateRangeInstanceName(value);
+ break;
+ case "domainName":
+ errMessage = validateRangeDomainName(value);
+ break;
+ case "routes":
+ errMessage = validateRangeRoutes(value);
+ break;
+ default:
+ errMessage = validateRangeOther(name, value, rangeOption);
+ }
+ if (errMessage == null) {
+ return null;
}
- } else if (type.equals(Integer.class)) {
- final String[] options = range.split("-");
- if (options.length != 2) {
- final String msg = "configuration range " + range + " for " +
name + " is invalid";
- s_logger.error(msg);
- return msg;
+ message = String.format("%1$s%2$s", message, errMessage);
+ }
+ return message;
+ }
+
+ protected String validateRangePrivateIp(String name, String value) {
+ try {
+ if (NetUtils.isSiteLocalAddress(value)) {
+ return null;
}
- final int min = Integer.parseInt(options[0]);
- final int max = Integer.parseInt(options[1]);
- final int val = Integer.parseInt(value);
- if (val < min || val > max) {
- s_logger.error("configuration value for " + name + " is
invalid");
- return "Please enter : " + range;
+ s_logger.error(String.format("The private IP range [%1$s] is not a
site local address for the configuration variable [%2$s].", value, name));
+ } catch (final NullPointerException e) {
+ s_logger.error(String.format("Error while parsing IP address for
[%s].", name));
+ }
+ return "Please enter a site local IP address.";
+ }
+
+ protected String validateRangeNetmask(String name, String value) {
+ if (NetUtils.isValidIp4Netmask(value)) {
+ return null;
+ }
+ s_logger.error(String.format("Netmask [%1$s] is not a valid netmask
for configuration variable [%2$s].", value, name));
+ return "Please enter a valid netmask.";
+ }
+
+ protected String validateRangeHypervisorList(String value) {
+ final String[] hypervisors = value.split(",");
+ for (final String hypervisor : hypervisors) {
+ if (HypervisorType.getType(hypervisor) == HypervisorType.Any ||
HypervisorType.getType(hypervisor) == HypervisorType.None) {
+ return String.format("Please enter a valid hypervisor type.");
Review Comment:
why the `String.format()` here? did you intent to add values (like
`hypervisor` or `HypervisorType`)?
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -1098,35 +1099,36 @@ private String validateConfigurationValue(final String
name, String value, final
if (!configScope.contains(scope) &&
!(ENABLE_ACCOUNT_SETTINGS_FOR_DOMAIN.value() &&
configScope.contains(ConfigKey.Scope.Account.toString()) &&
scope.equals(ConfigKey.Scope.Domain.toString()))) {
- s_logger.error("Invalid scope id provided for the parameter "
+ name);
- return "Invalid scope id provided for the parameter " + name;
+ String msg = String.format("Invalid scope id provided for the
parameter [%s].", name);
+ s_logger.error(msg);
+ return msg;
}
}
Class<?> type = null;
- final Config c = Config.getConfig(name);
- if (c == null) {
- s_logger.warn("Did not find configuration " + name + " in
Config.java. Perhaps moved to ConfigDepot");
+ final Config configuration = Config.getConfig(name);
+ if (configuration == null) {
+ s_logger.warn(String.format("Configuration [%s] could not be found
in Config.java. Perhaps moved to ConfigDepot.", name));
final ConfigKey<?> configKey = _configDepot.get(name);
if(configKey == null) {
- s_logger.warn("Did not find configuration " + name + " in
ConfigDepot too.");
+ s_logger.warn(String.format("Configuration [%s] could not be
found in ConfigDepot."));
Review Comment:
```suggestion
s_logger.warn(String.format("Configuration [%s] could not be
found in ConfigDepot.", configKey));
```
##########
server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java:
##########
@@ -1171,147 +1173,205 @@ private String validateConfigurationValue(final
String name, String value, final
if (type.equals(Integer.class) &&
NetworkModel.MACIdentifier.key().equalsIgnoreCase(name)) {
try {
final int val = Integer.parseInt(value);
- //The value need to be between 0 to 255 because the mac
generation needs a value of 8 bit
+ //The value needs 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");
+ throw new
InvalidParameterValueException(String.format("[%s] value should be between 0
and 255. 0 value will disable this feature.", name));
}
} catch (final NumberFormatException e) {
- s_logger.error("There was an error trying to parse the integer
value for:" + name);
- throw new InvalidParameterValueException("There was an error
trying to parse the integer value for:" + name);
+ s_logger.error(String.format("There was an error trying to
parse the integer value for: [%s].", name));
Review Comment:
let's extract this format string in a constant. it is used in four places.
Also let's construct the message one time for both the log and the exception
below.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
