Repository: kafka Updated Branches: refs/heads/trunk fdbd4d62f -> 9be71f7bd
MINOR: Use ObjectName.quote instead of URL-encoding for JMX metric tags Author: Rajini Sivaram <rajinisiva...@googlemail.com> Reviewers: Ewen Cheslack-Postava <e...@confluent.io> Closes #4099 from rajinisivaram/1.0 (cherry picked from commit 51bb83d0dce8110b941891eddedba1fe3abdf658) Signed-off-by: Ewen Cheslack-Postava <m...@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 <rajinisiva...@googlemail.com> Authored: Thu Oct 19 19:37:38 2017 -0700 Committer: Ewen Cheslack-Postava <m...@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; + } + } }