kafka-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ewe...@apache.org
Subject kafka git commit: MINOR: Use ObjectName.quote instead of URL-encoding for JMX metric tags
Date Fri, 20 Oct 2017 02:37:55 GMT
Repository: kafka
Updated Branches:
  refs/heads/trunk fdbd4d62f -> 9be71f7bd


MINOR: Use ObjectName.quote instead of URL-encoding for JMX metric tags

Author: Rajini Sivaram <rajinisivaram@googlemail.com>

Reviewers: Ewen Cheslack-Postava <ewen@confluent.io>

Closes #4099 from rajinisivaram/1.0

(cherry picked from commit 51bb83d0dce8110b941891eddedba1fe3abdf658)
Signed-off-by: Ewen Cheslack-Postava <me@ewencp.org>


Project: http://git-wip-us.apache.org/repos/asf/kafka/repo
Commit: http://git-wip-us.apache.org/repos/asf/kafka/commit/9be71f7b
Tree: http://git-wip-us.apache.org/repos/asf/kafka/tree/9be71f7b
Diff: http://git-wip-us.apache.org/repos/asf/kafka/diff/9be71f7b

Branch: refs/heads/trunk
Commit: 9be71f7bdcd147aee7a360a4ccf400acb858a056
Parents: fdbd4d6
Author: Rajini Sivaram <rajinisivaram@googlemail.com>
Authored: Thu Oct 19 19:37:38 2017 -0700
Committer: Ewen Cheslack-Postava <me@ewencp.org>
Committed: Thu Oct 19 19:37:52 2017 -0700

----------------------------------------------------------------------
 .../kafka/common/metrics/JmxReporter.java       |  2 +-
 .../kafka/common/utils/AppInfoParser.java       |  4 +-
 .../apache/kafka/common/utils/Sanitizer.java    | 37 ++++++++++++++-
 .../kafka/common/metrics/JmxReporterTest.java   | 31 +++++++------
 .../kafka/common/utils/SanitizerTest.java       | 48 ++++++++++++++++++++
 5 files changed, 104 insertions(+), 18 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kafka/blob/9be71f7b/clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java
----------------------------------------------------------------------
diff --git a/clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java b/clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java
index fda37d1..0c49224 100644
--- a/clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java
+++ b/clients/src/main/java/org/apache/kafka/common/metrics/JmxReporter.java
@@ -134,7 +134,7 @@ public class JmxReporter implements MetricsReporter {
             mBeanName.append(",");
             mBeanName.append(entry.getKey());
             mBeanName.append("=");
-            mBeanName.append(Sanitizer.sanitize(entry.getValue()));
+            mBeanName.append(Sanitizer.jmxSanitize(entry.getValue()));
         }
         return mBeanName.toString();
     }

http://git-wip-us.apache.org/repos/asf/kafka/blob/9be71f7b/clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java
----------------------------------------------------------------------
diff --git a/clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java b/clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java
index 42cf312..0a17ecd 100644
--- a/clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java
+++ b/clients/src/main/java/org/apache/kafka/common/utils/AppInfoParser.java
@@ -57,7 +57,7 @@ public class AppInfoParser {
 
     public static synchronized void registerAppInfo(String prefix, String id, Metrics metrics)
{
         try {
-            ObjectName name = new ObjectName(prefix + ":type=app-info,id=" + Sanitizer.sanitize(id));
+            ObjectName name = new ObjectName(prefix + ":type=app-info,id=" + Sanitizer.jmxSanitize(id));
             AppInfo mBean = new AppInfo();
             ManagementFactory.getPlatformMBeanServer().registerMBean(mBean, name);
 
@@ -70,7 +70,7 @@ public class AppInfoParser {
     public static synchronized void unregisterAppInfo(String prefix, String id, Metrics metrics)
{
         MBeanServer server = ManagementFactory.getPlatformMBeanServer();
         try {
-            ObjectName name = new ObjectName(prefix + ":type=app-info,id=" + Sanitizer.sanitize(id));
+            ObjectName name = new ObjectName(prefix + ":type=app-info,id=" + Sanitizer.jmxSanitize(id));
             if (server.isRegistered(name))
                 server.unregisterMBean(name);
 

http://git-wip-us.apache.org/repos/asf/kafka/blob/9be71f7b/clients/src/main/java/org/apache/kafka/common/utils/Sanitizer.java
----------------------------------------------------------------------
diff --git a/clients/src/main/java/org/apache/kafka/common/utils/Sanitizer.java b/clients/src/main/java/org/apache/kafka/common/utils/Sanitizer.java
index 0b68d0c..d35ea91 100644
--- a/clients/src/main/java/org/apache/kafka/common/utils/Sanitizer.java
+++ b/clients/src/main/java/org/apache/kafka/common/utils/Sanitizer.java
@@ -20,15 +20,35 @@ import java.io.UnsupportedEncodingException;
 import java.net.URLDecoder;
 import java.net.URLEncoder;
 import java.nio.charset.StandardCharsets;
+import java.util.regex.Pattern;
+
+import javax.management.ObjectName;
 
 import org.apache.kafka.common.KafkaException;
 
 /**
- * Utility class for sanitizing/desanitizing user principal and client-ids
- * to a safe value for use in JMX metric names and as Zookeeper node name
+ * Utility class for sanitizing/desanitizing/quoting values used in JMX metric names
+ * or as ZooKeeper node name.
+ * <p>
+ * User principals and client-ids are URL-encoded using ({@link #sanitize(String)}
+ * for use as ZooKeeper node names. User principals are URL-encoded in all metric
+ * names as well. All other metric tags including client-id are quoted if they
+ * contain special characters using {@link #jmxSanitize(String)} when
+ * registering in JMX.
  */
 public class Sanitizer {
 
+    /**
+     * Even though only a small number of characters are disallowed in JMX, quote any
+     * string containing special characteres to be safe. All characters in strings sanitized
+     * using {@link #sanitize(String)} are safe for JMX and hence included here.
+     */
+    private static final Pattern MBEAN_PATTERN = Pattern.compile("[\\w-%\\. \t]*");
+
+    /**
+     * Sanitize `name` for safe use as JMX metric name as well as ZooKeeper node name
+     * using URL-encoding.
+     */
     public static String sanitize(String name) {
         String encoded = "";
         try {
@@ -50,6 +70,10 @@ public class Sanitizer {
         }
     }
 
+    /**
+     * Desanitize name that was URL-encoded using {@link #sanitize(String)}. This
+     * is used to obtain the desanitized version of node names in ZooKeeper.
+     */
     public static String desanitize(String name) {
         try {
             return URLDecoder.decode(name, StandardCharsets.UTF_8.name());
@@ -58,4 +82,13 @@ public class Sanitizer {
         }
     }
 
+    /**
+     * Quote `name` using {@link ObjectName#quote(String)} if `name` contains
+     * characters that are not safe for use in JMX. User principals that are
+     * already sanitized using {@link #sanitize(String)} will not be quoted
+     * since they are safe for JMX.
+     */
+    public static String jmxSanitize(String name) {
+        return MBEAN_PATTERN.matcher(name).matches() ? name : ObjectName.quote(name);
+    }
 }

http://git-wip-us.apache.org/repos/asf/kafka/blob/9be71f7b/clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java
----------------------------------------------------------------------
diff --git a/clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java b/clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java
index 3b39db6..98e49f3 100644
--- a/clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java
+++ b/clients/src/test/java/org/apache/kafka/common/metrics/JmxReporterTest.java
@@ -75,25 +75,30 @@ public class JmxReporterTest {
             sensor.add(metrics.metricName("name", "group", "desc", "id", "foo+"), new Total());
             sensor.add(metrics.metricName("name", "group", "desc", "id", "foo?"), new Total());
             sensor.add(metrics.metricName("name", "group", "desc", "id", "foo:"), new Total());
-
-            assertTrue(server.isRegistered(new ObjectName(":type=group,id=foo%2A")));
-            assertEquals(0.0, server.getAttribute(new ObjectName(":type=group,id=foo%2A"),
"name"));
-            assertTrue(server.isRegistered(new ObjectName(":type=group,id=foo%2B")));
-            assertEquals(0.0, server.getAttribute(new ObjectName(":type=group,id=foo%2B"),
"name"));
-            assertTrue(server.isRegistered(new ObjectName(":type=group,id=foo%3F")));
-            assertEquals(0.0, server.getAttribute(new ObjectName(":type=group,id=foo%3F"),
"name"));
-            assertTrue(server.isRegistered(new ObjectName(":type=group,id=foo%3A")));
-            assertEquals(0.0, server.getAttribute(new ObjectName(":type=group,id=foo%3A"),
"name"));
+            sensor.add(metrics.metricName("name", "group", "desc", "id", "foo%"), new Total());
+
+            assertTrue(server.isRegistered(new ObjectName(":type=group,id=\"foo\\*\"")));
+            assertEquals(0.0, server.getAttribute(new ObjectName(":type=group,id=\"foo\\*\""),
"name"));
+            assertTrue(server.isRegistered(new ObjectName(":type=group,id=\"foo+\"")));
+            assertEquals(0.0, server.getAttribute(new ObjectName(":type=group,id=\"foo+\""),
"name"));
+            assertTrue(server.isRegistered(new ObjectName(":type=group,id=\"foo\\?\"")));
+            assertEquals(0.0, server.getAttribute(new ObjectName(":type=group,id=\"foo\\?\""),
"name"));
+            assertTrue(server.isRegistered(new ObjectName(":type=group,id=\"foo:\"")));
+            assertEquals(0.0, server.getAttribute(new ObjectName(":type=group,id=\"foo:\""),
"name"));
+            assertTrue(server.isRegistered(new ObjectName(":type=group,id=foo%")));
+            assertEquals(0.0, server.getAttribute(new ObjectName(":type=group,id=foo%"),
"name"));
 
             metrics.removeMetric(metrics.metricName("name", "group", "desc", "id", "foo*"));
             metrics.removeMetric(metrics.metricName("name", "group", "desc", "id", "foo+"));
             metrics.removeMetric(metrics.metricName("name", "group", "desc", "id", "foo?"));
             metrics.removeMetric(metrics.metricName("name", "group", "desc", "id", "foo:"));
+            metrics.removeMetric(metrics.metricName("name", "group", "desc", "id", "foo%"));
 
-            assertFalse(server.isRegistered(new ObjectName(":type=group,id=foo%2A")));
-            assertFalse(server.isRegistered(new ObjectName(":type=group,id=foo%2B")));
-            assertFalse(server.isRegistered(new ObjectName(":type=group,id=foo%3F")));
-            assertFalse(server.isRegistered(new ObjectName(":type=group,id=foo%3A")));
+            assertFalse(server.isRegistered(new ObjectName(":type=group,id=\"foo\\*\"")));
+            assertFalse(server.isRegistered(new ObjectName(":type=group,id=foo+")));
+            assertFalse(server.isRegistered(new ObjectName(":type=group,id=\"foo\\?\"")));
+            assertFalse(server.isRegistered(new ObjectName(":type=group,id=\"foo:\"")));
+            assertFalse(server.isRegistered(new ObjectName(":type=group,id=foo%")));
         } finally {
             metrics.close();
         }

http://git-wip-us.apache.org/repos/asf/kafka/blob/9be71f7b/clients/src/test/java/org/apache/kafka/common/utils/SanitizerTest.java
----------------------------------------------------------------------
diff --git a/clients/src/test/java/org/apache/kafka/common/utils/SanitizerTest.java b/clients/src/test/java/org/apache/kafka/common/utils/SanitizerTest.java
index dd384ee..59ac6c0 100644
--- a/clients/src/test/java/org/apache/kafka/common/utils/SanitizerTest.java
+++ b/clients/src/test/java/org/apache/kafka/common/utils/SanitizerTest.java
@@ -18,8 +18,16 @@ package org.apache.kafka.common.utils;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import java.io.UnsupportedEncodingException;
+import java.lang.management.ManagementFactory;
+
+import javax.management.MBeanException;
+import javax.management.MBeanServer;
+import javax.management.MalformedObjectNameException;
+import javax.management.ObjectName;
+import javax.management.OperationsException;
 
 import org.junit.Test;
 
@@ -32,4 +40,44 @@ public class SanitizerTest {
         assertTrue(sanitizedPrincipal.replace('%', '_').matches("[a-zA-Z0-9\\._\\-]+"));
         assertEquals(principal, Sanitizer.desanitize(sanitizedPrincipal));
     }
+
+    @Test
+    public void testJmxSanitize() throws MalformedObjectNameException {
+        int unquoted = 0;
+        for (int i = 0; i < 65536; i++) {
+            char c = (char) i;
+            String value = "value" + c;
+            String jmxSanitizedValue = Sanitizer.jmxSanitize(value);
+            if (jmxSanitizedValue.equals(value))
+                unquoted++;
+            verifyJmx(jmxSanitizedValue, i);
+            String encodedValue = Sanitizer.sanitize(value);
+            verifyJmx(encodedValue, i);
+            // jmxSanitize should not sanitize URL-encoded values
+            assertEquals(encodedValue, Sanitizer.jmxSanitize(encodedValue));
+        }
+        assertEquals(68, unquoted); // a-zA-Z0-9-_% space and tab
+    }
+
+    private void verifyJmx(String sanitizedValue, int c) throws MalformedObjectNameException
{
+        Object mbean = new TestStat();
+        MBeanServer server = ManagementFactory.getPlatformMBeanServer();
+        ObjectName objectName = new ObjectName("test:key=" + sanitizedValue);
+        try {
+            server.registerMBean(mbean, objectName);
+            server.unregisterMBean(objectName);
+        } catch (OperationsException | MBeanException e) {
+            fail("Could not register char=\\u" + c);
+        }
+    }
+
+    public interface TestStatMBean {
+        int getValue();
+    }
+
+    public class TestStat implements TestStatMBean {
+        public int getValue() {
+            return 1;
+        }
+    }
 }


Mime
View raw message