cmccabe commented on code in PR #13645:
URL: https://github.com/apache/kafka/pull/13645#discussion_r1179448569
##########
metadata/src/main/java/org/apache/kafka/controller/ClientQuotaControlManager.java:
##########
@@ -214,46 +214,64 @@ private ApiError configKeysForEntityType(Map<String,
String> entity, Map<String,
return ApiError.NONE;
}
- private ApiError validateQuotaKeyValue(Map<String, ConfigDef.ConfigKey>
validKeys, String key, Double value) {
- // TODO can this validation be shared with alter configs?
+ static ApiError validateQuotaKeyValue(
+ Map<String, ConfigDef.ConfigKey> validKeys,
+ String key,
+ double value
+ ) {
// Ensure we have an allowed quota key
ConfigDef.ConfigKey configKey = validKeys.get(key);
if (configKey == null) {
return new ApiError(Errors.INVALID_REQUEST, "Invalid configuration
key " + key);
}
+ if (value <= 0.0) {
+ return new ApiError(Errors.INVALID_REQUEST, "Quota " + key + "
must be greater than 0");
+ }
// Ensure the quota value is valid
switch (configKey.type()) {
case DOUBLE:
- break;
+ return ApiError.NONE;
case SHORT:
+ if (value > Short.MAX_VALUE) {
+ return new ApiError(Errors.INVALID_REQUEST,
+ "Proposed value for " + key + " is too large for a
SHORT.");
+ }
+ return getErrorForIntegralQuotaValue(value, key);
case INT:
- case LONG:
- Double epsilon = 1e-6;
- Long longValue = Double.valueOf(value + epsilon).longValue();
- if (Math.abs(longValue.doubleValue() - value) > epsilon) {
+ if (value > Integer.MAX_VALUE) {
+ return new ApiError(Errors.INVALID_REQUEST,
+ "Proposed value for " + key + " is too large for an
INT.");
+ }
+ return getErrorForIntegralQuotaValue(value, key);
+ case LONG: {
+ if (value > Long.MAX_VALUE) {
return new ApiError(Errors.INVALID_REQUEST,
- "Configuration " + key + " must be a Long value");
+ "Proposed value for " + key + " is too large for a
LONG.");
}
- break;
+ return getErrorForIntegralQuotaValue(value, key);
+ }
default:
return new ApiError(Errors.UNKNOWN_SERVER_ERROR,
"Unexpected config type " + configKey.type() + "
should be Long or Double");
}
+ }
+
+ static ApiError getErrorForIntegralQuotaValue(double value, String key) {
+ double remainder = Math.abs(value % 1.0);
+ if (remainder > 1e-6) {
+ return new ApiError(Errors.INVALID_REQUEST, key + " cannot be a
fractional value.");
+ }
return ApiError.NONE;
}
- // TODO move this somewhere common?
- private boolean isValidIpEntity(String ip) {
- if (Objects.nonNull(ip)) {
- try {
- InetAddress.getByName(ip);
- return true;
- } catch (UnknownHostException e) {
- return false;
- }
- } else {
+ static boolean isValidIpEntity(String ip) {
Review Comment:
yes, we accept hostnames here.
this is a questionable design decision, especially since it means if the
hostname isn't resolving for whatever reason, validation will fail, but maybe
succeed later if the hostname can be resolved at that point.
unfortunately there's no changing it without a KIP
--
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]
