[GitHub] [kafka] cmccabe commented on a diff in pull request #13645: KAFKA-14943: Fix ClientQuotaControlManager validation

["cmccabe (via GitHub)" <gi...@apache.org>]

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: jira-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org