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

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

mumrah commented on code in PR #13645:
URL: https://github.com/apache/kafka/pull/13645#discussion_r1179202301


##########
clients/src/main/java/org/apache/kafka/common/config/internals/QuotaConfigs.java:
##########
@@ -71,7 +71,13 @@ public static boolean isClientOrUserConfig(String name) {
         return userClientConfigNames.contains(name);
     }
 
-    public static ConfigDef userConfigs() {
+    public static ConfigDef clientQuotaConfigs() {

Review Comment:
   Can we name this "userOrClientQuotaConfigs" since it's used for either?



##########
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:
   Unrelated to this patch, but it seems like we would accept a hostname here. Is that expected?



-- 
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