You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kafka.apache.org by ew...@apache.org on 2017/10/20 02:37:42 UTC
kafka git commit: MINOR: Use ObjectName.quote instead of URL-encoding
for JMX metric tags
Repository: kafka
Updated Branches:
refs/heads/1.0 0e211555f -> 51bb83d0d
MINOR: Use ObjectName.quote instead of URL-encoding for JMX metric tags
Author: Rajini Sivaram <ra...@googlemail.com>
Reviewers: Ewen Cheslack-Postava <ew...@confluent.io>
Closes #4099 from rajinisivaram/1.0
Project: http://git-wip-us.apache.org/repos/asf/kafka/repo
Commit: http://git-wip-us.apache.org/repos/asf/kafka/commit/51bb83d0
Tree: http://git-wip-us.apache.org/repos/asf/kafka/tree/51bb83d0
Diff: http://git-wip-us.apache.org/repos/asf/kafka/diff/51bb83d0
Branch: refs/heads/1.0
Commit: 51bb83d0dce8110b941891eddedba1fe3abdf658
Parents: 0e21155
Author: Rajini Sivaram <ra...@googlemail.com>
Authored: Thu Oct 19 19:37:38 2017 -0700
Committer: Ewen Cheslack-Postava <me...@ewencp.org>
Committed: Thu Oct 19 19:37:38 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/51bb83d0/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/51bb83d0/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/51bb83d0/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/51bb83d0/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/51bb83d0/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;
+ }
+ }
}