KAFKA-5735: KIP-190: Handle client-ids consistently #3906

mimaison · 2017-09-19T18:00:44Z

Developed with @edoardocomar

@edoardocomar

Developed with @edoardocomar

mimaison · 2017-09-19T18:01:32Z

@ijuma @rajinisivaram Can you take a look ?
Thanks

rajinisivaram

@mimaison @edoardocomar Thank you for the PR. I have left some minor comments.

rajinisivaram · 2017-09-20T09:29:49Z

clients/src/main/java/org/apache/kafka/clients/admin/KafkaAdminClient.java

@@ -298,7 +299,7 @@ static KafkaAdminClient createInternal(AdminClientConfig config, TimeoutProcesso
                    config.getLong(AdminClientConfig.METADATA_MAX_AGE_CONFIG), true);
            List<MetricsReporter> reporters = config.getConfiguredInstances(AdminClientConfig.METRIC_REPORTER_CLASSES_CONFIG,
                MetricsReporter.class);
-            Map<String, String> metricTags = Collections.singletonMap("client-id", clientId);
+            Map<String, String> metricTags = Collections.singletonMap("client-id", Sanitizer.sanitize(clientId));


Since there are two calls to sanitize here, perhaps worth doing once and reusing?

Here to avoid the 2nd call, we'd have to pass sanitizedClientId as an argument to the private constructor. Is it what you prefer ?

Personally, yes, I prefer one call, but I leave it up to you.

rajinisivaram · 2017-09-20T09:30:15Z

clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java

@@ -657,7 +658,7 @@ private KafkaConsumer(ConsumerConfig config,
                throw new ConfigException(ConsumerConfig.REQUEST_TIMEOUT_MS_CONFIG + " should be greater than " + ConsumerConfig.SESSION_TIMEOUT_MS_CONFIG + " and " + ConsumerConfig.FETCH_MAX_WAIT_MS_CONFIG);
            this.time = Time.SYSTEM;

-            Map<String, String> metricsTags = Collections.singletonMap("client-id", clientId);
+            Map<String, String> metricsTags = Collections.singletonMap("client-id", Sanitizer.sanitize(this.clientId));


Same as before, do once and reuse?

rajinisivaram · 2017-09-20T09:31:22Z

clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java

            this.ioThread = new KafkaThread(ioThreadName, this.sender, true);
            this.ioThread.start();
            this.errors = this.metrics.sensor("errors");
            config.logUnused();
-            AppInfoParser.registerAppInfo(JMX_PREFIX, clientId);
+            AppInfoParser.registerAppInfo(JMX_PREFIX, Sanitizer.sanitize(clientId));


As before, do once and reuse?

rajinisivaram · 2017-09-20T09:31:57Z

clients/src/main/java/org/apache/kafka/common/metrics/Sanitizer.java

+
+public class Sanitizer {
+
+    public static String sanitize(String name) {


Can you add a class comment?

rajinisivaram · 2017-09-20T09:32:13Z

clients/src/main/java/org/apache/kafka/common/metrics/Sanitizer.java

+            for (int i = 0; i < encoded.length(); i++) {
+                char c = encoded.charAt(i);
+                if (c == '*') {
+                    builder.append("%2A");


why? there was a comment in the original code

rajinisivaram · 2017-09-20T09:32:34Z

clients/src/main/java/org/apache/kafka/common/metrics/Sanitizer.java

+                if (c == '*') {
+                    builder.append("%2A");
+                } else if (c == '+') {
+                    builder.append("%20");


again comment removed from original code

rajinisivaram · 2017-09-20T09:37:04Z

core/src/test/scala/integration/kafka/api/BaseQuotaTest.scala

@@ -234,7 +234,7 @@ abstract class BaseQuotaTest extends IntegrationTestHarness {
                                  quotaType.toString,
                                  "Tracking throttle-time per user/client-id",
                                  "user", quotaId.sanitizedUser.getOrElse(""),
-                                  "client-id", quotaId.clientId.getOrElse(""))
+                                  "client-id", quotaId.sanitizedClientId.getOrElse(""))


Can we change one of the client-ids in this test to contain special characters?

rajinisivaram · 2017-09-20T09:40:13Z

core/src/test/scala/integration/kafka/api/UserClientIdQuotaTest.scala

  }

  private def updateQuotaOverride(userPrincipal: String, clientId: String, properties: Properties) {
-    AdminUtils.changeUserOrUserClientIdConfig(zkUtils, QuotaId.sanitize(userPrincipal) + "/clients/" + clientId, properties)
+    AdminUtils.changeUserOrUserClientIdConfig(zkUtils, Sanitizer.sanitize(userPrincipal) + "/clients/" + clientId, properties)


Can you update the equivalent code in ClientIdQuotaTest to do sanitize as well for consistency? Also, AdminUtils.changeClientIdConfig now uses sanitized client-ids, so the parameters and javadoc need updating.

rajinisivaram · 2017-09-20T09:43:42Z

core/src/main/scala/kafka/server/ClientQuotaManager.scala

@@ -387,14 +360,15 @@ class ClientQuotaManager(private val config: ClientQuotaManagerConfig,
   * First sensor of the tuple is the quota enforcement sensor. Second one is the throttle time sensor
   */
  def getOrCreateQuotaSensors(sanitizedUser: String, clientId: String): ClientSensors = {
-    val clientQuotaEntity = quotaEntity(sanitizedUser, clientId)
+    val sanitizedClientId = Sanitizer.sanitize(clientId)


Can we cache sanitized client-ids somewhere so that we dont have to sanitize for every request? Obviously they could change, but perhaps check and re-sanitize if required?

I think we should also move sanitizedUser from Session to KafkaChannel perhaps since Session objects are now created for every request.

@rajinisivaram I think the optimization of moving the sanitisedUser should be done in a separate PR, maybe for a point release.

Currently once a connection is established , Sanitize is called once for the user and - with this PR - once per client-id.

So I'd say about the suggested optimization of caching the sanitize(client-id) in CilentQuotaManager.getOrCreate... that as well should go in a separate PR.
At that time we could measure if caching is worth doing !

@edoardocomar Hmm.. We used to sanitize user principal once per connection, but there was a recent PR (not this one) which changed it to once per request. That should be easy to fix though, but a separate PR is fine. For client-id caching, there is a bit more work involved since client-ids can change within a connection. Again I am ok with doing that later.

guozhangwang · 2017-09-21T12:55:37Z

@rajinisivaram how do you think about the status of @mimaison's patch? And is the KIP itself low risk for introducing regression? The feature freeze date has passed, so if you are not confident to merge it in the next day or two, I'll have to push it to 1.1.0 release.

ijuma · 2017-09-21T12:57:00Z

This fixes an important bug with potential security implications, so we should get this in. It's close and low risk. @mimaison, will you be able to address the comments today?

mimaison · 2017-09-21T13:19:11Z

Yes I should be able to update the PR in the afternoon.

mimaison · 2017-09-21T16:07:37Z

Should I add a line in the upgrade notes ? Do we want to mention the quotas issues ?

ijuma · 2017-09-21T16:16:19Z

Yes, please add a line in the upgrade notes.

…ient

mimaison · 2017-09-21T16:50:02Z

I believe we addressed all comments so far. The few larger optimizations suggested will be done later in another PR.

rajinisivaram

LGTM, merging to trunk

KAFKA-5735: KIP-190: Handle client-ids consistently

274e52d

Developed with @edoardocomar

rajinisivaram reviewed Sep 20, 2017

View reviewed changes

Updates following review

b726b31

Updated upgrade notes, call Sanitizer.sanitize() once in KafkaAdminCl…

d34f813

…ient

rajinisivaram approved these changes Sep 21, 2017

View reviewed changes

asfgit closed this in c8c6ab6 Sep 21, 2017

mimaison deleted the KAFKA-5735 branch April 18, 2018 13:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-5735: KIP-190: Handle client-ids consistently #3906

KAFKA-5735: KIP-190: Handle client-ids consistently #3906

mimaison commented Sep 19, 2017

mimaison commented Sep 19, 2017

rajinisivaram left a comment

rajinisivaram Sep 20, 2017

mimaison Sep 21, 2017

rajinisivaram Sep 21, 2017

rajinisivaram Sep 20, 2017

rajinisivaram Sep 20, 2017

rajinisivaram Sep 20, 2017

rajinisivaram Sep 20, 2017

rajinisivaram Sep 20, 2017

rajinisivaram Sep 20, 2017

rajinisivaram Sep 20, 2017

rajinisivaram Sep 20, 2017

rajinisivaram Sep 20, 2017

edoardocomar Sep 21, 2017 •

edited

Loading

edoardocomar Sep 21, 2017

rajinisivaram Sep 21, 2017

guozhangwang commented Sep 21, 2017

ijuma commented Sep 21, 2017

mimaison commented Sep 21, 2017

mimaison commented Sep 21, 2017

ijuma commented Sep 21, 2017

mimaison commented Sep 21, 2017

rajinisivaram left a comment


		public class Sanitizer {

		public static String sanitize(String name) {

KAFKA-5735: KIP-190: Handle client-ids consistently #3906

KAFKA-5735: KIP-190: Handle client-ids consistently #3906

Conversation

mimaison commented Sep 19, 2017

mimaison commented Sep 19, 2017

rajinisivaram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edoardocomar Sep 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guozhangwang commented Sep 21, 2017

ijuma commented Sep 21, 2017

mimaison commented Sep 21, 2017

mimaison commented Sep 21, 2017

ijuma commented Sep 21, 2017

mimaison commented Sep 21, 2017

rajinisivaram left a comment

Choose a reason for hiding this comment

edoardocomar Sep 21, 2017 •

edited

Loading