Add Kafka quota enforcer #53

danielfritsch · 2021-12-07T00:10:43Z

Enables programmatic configuration of Kafka quotas to restrict monopolization of Kafka broker resources caused by bad acting clients.

https://kafka.apache.org/documentation/#design_quotas

shrijeet-tesla

Looking great, made some minor comments.

shrijeet-tesla · 2021-12-08T19:17:01Z

kafka_quota_enforcer/src/main/java/com/tesla/data/quota/enforcer/QuotaEnforceCommand.java

+        Time.SYSTEM,
+        "kafka.server", // default used in the client
+        "SessionExpireListener" // default used in the client
+    );


A general comment, its a good practice to declare the variable as final as much as possible in many cases it doesn't have an obvious benefit, but in multi-threaded code, it can protect you from bugs

shrijeet-tesla · 2021-12-08T19:17:21Z

kafka_quota_enforcer/src/main/java/com/tesla/data/quota/enforcer/QuotaEnforceCommand.java

+
+  @Override
+  protected Enforcer<ConfiguredQuota> initEnforcer() {
+    Map<String, Object> zookeeperConfig = zookeeperConfig();


nit: rename this variable, it conflicts with the method name

shrijeet-tesla · 2021-12-08T19:17:42Z

kafka_quota_enforcer/src/main/java/com/tesla/data/quota/enforcer/QuotaService.java

+    this.adminClient = adminClient;
+  }
+
+  enum Type {


can you add some docs here explaining what each of these mean?

shrijeet-tesla

+1. Get Jesse's 👀 too.

jyates

Some nits and testing improvements, but overall looks great!

jyates · 2021-12-09T11:30:01Z

kafka_quota_enforcer/README.md

+bazel build //kafka_quota_enforcer/src/main/java/com/tesla/data/quota/enforcer:Main_deploy.jar
+```
+
+### Verify


Its not clear what to expect from these commands

jyates · 2021-12-09T11:32:01Z

kafka_quota_enforcer/README.md

+
+* A quota applies to a cluster if that cluster is present under quota's `clusters`
+attribute. If no changes to base config are desired, specify empty dict `{}`
+* Any quota value can be overridden. Quota 'principal' or 'client' properties should not be overridden.


i would expect the enforcer to fail in that case (and thus the comment would read "If the 'principal' or 'client' properties are overridden, enforcing will fail")... but i assume this is not possible? In which case how about "'principal' or 'client' are not recommended to be overridden.

yeah, don't think it's currently supported to restrict specific fields from being overridden. Will add in "not recommended"

jyates · 2021-12-09T11:32:58Z

kafka_quota_enforcer/README.md

+* Map (dictionary) properties are merged in a way that preserves base settings
+
+
+Note: `quotasFile` is supported for multi-cluster configuration too.


Note: quotasFile supports multi-cluster configuration as well.

jyates · 2021-12-09T11:35:16Z

kafka_quota_enforcer/README.md

+You can read more about it here: https://kafka.apache.org/documentation/#design_quotas
+
+To specify a default quota, simply use `<default>` as the value in the quota's `principal` or `client` config fields.
+For example, to set a default quota for clients groups that have unique principals, here's a sample `quotasFile.yaml`:


For example, to set the default quota for each unique principal - regardless of the client - you could use:

jyates · 2021-12-09T11:39:31Z

kafka_quota_enforcer/src/test/java/com/tesla/data/quota/enforcer/QuotaEnforcerTest.java

+  public void setup() {
+    quotaService = mock(QuotaService.class);
+    enforcer = new QuotaEnforcer(emptyList(), quotaService, true, true);
+    quotas = Arrays.asList(


why not just make static, final and set above?

jyates · 2021-12-09T11:54:02Z

kafka_quota_enforcer/README.md

+            configuration file is expected
+```
+
+## Configuration


There is no discussion of precedence of quotas for client/principal combinations, or at least a pointer to the kafka docs describing the hierarchy. Can lead to unexpected outcomes and/or confusion for users

thanks, forgot about that

jyates · 2021-12-09T11:54:46Z

kafka_quota_enforcer/README.md

+  * cluster default
+  * default 
+
+# Setting 'default' quotas


How about calling a "low water mark"... really anything besides default :)

changed to "entity-default" - it seems that referencing 'default' in the title is important as that's what users will be looking for in the docs, and "entity-default" would help to relate that back to the kafka-configs command usage

jyates · 2021-12-09T11:58:02Z

kafka_quota_enforcer/src/main/java/com/tesla/data/quota/enforcer/QuotaService.java

+    Integer requestPercentage = quota.get(REQUEST_PERCENTAGE) != null ?
+        Integer.parseInt(quota.getProperty(REQUEST_PERCENTAGE)) : null;
+
+    switch (quotaType) {


isn't this an entityType?

jyates · 2021-12-09T11:58:37Z

kafka_quota_enforcer/src/main/java/com/tesla/data/quota/enforcer/QuotaService.java

+    this.adminClient = adminClient;
+  }
+
+  enum Type {


Doesn't EntityType more closely describe what 'type' means here?

jyates · 2021-12-09T12:03:19Z

kafka_quota_enforcer/src/main/java/com/tesla/data/quota/enforcer/ConfiguredQuota.java

+  }
+
+  @Override
+  public boolean equals(Object o) {


minor style nit: generally equals/hashcode/toString are at the bottom of the class

generally hashcode goes next to equals - they should be logically the same

jyates

Some more nits, but lgtm otherwise

jyates · 2021-12-10T15:25:09Z

kafka_quota_enforcer/src/main/java/com/tesla/data/quota/enforcer/ConfiguredQuota.java

+  }
+
+  @Override
+  public boolean equals(Object o) {


generally hashcode goes next to equals - they should be logically the same

jyates · 2021-12-10T15:27:06Z

kafka_quota_enforcer/src/main/java/com/tesla/data/quota/enforcer/QuotaService.java

+
+  enum Type {
+    // quota applied solely on a per principal/user basis, ex: principal=user1
+    USER,


Everything in ZK and Kafka says principal - it is strictly different than a 'user' https://stackoverflow.com/questions/4989063/what-is-the-meaning-and-difference-between-subject-user-and-principal

jyates · 2021-12-10T15:27:18Z

kafka_quota_enforcer/src/main/java/com/tesla/data/quota/enforcer/QuotaService.java

+    Integer requestPercentage = quota.get(REQUEST_PERCENTAGE) != null ?
+        Integer.parseInt(quota.getProperty(REQUEST_PERCENTAGE)) : null;
+
+    switch (quotaType) {


jyates · 2021-12-10T15:29:16Z

kafka_quota_enforcer/src/main/java/com/tesla/data/quota/enforcer/QuotaService.java

+      case CLIENT:
+        return new ConfiguredQuota(null, quotaEntry.getKey(), producerByteRate, consumerByteRate, requestPercentage);
+      case COMBINATION:
+        // when both user + client are configured, the full name looks like {configuredUser}/clients/{configuredClient}


You can name the groups as well and then reference them by name, for instance "(<principal>.*)/clients/(<client>.*)" then you can do new ConfiguredQuota(matcher.group("principal"), matcher.group("client") which is very explicit

jyates · 2021-12-10T15:29:56Z

kafka_quota_enforcer/src/test/java/com/tesla/data/quota/enforcer/QuotaEnforcerTest.java

+  public void testCreate() {
+    QuotaEnforcer enforcer = new QuotaEnforcer(emptyList(), quotaService, true, false);
+    enforcer.create(quotas);
+    verify(quotaService, times(1)).create(quotas);


nit: times(1) is the default with verify(), you can just drop the times parameter

… quotas to restrict monopolization of Kafka broker resources caused by bad acting clients.

danielfritsch force-pushed the feature/kafka_quota_enforcer branch from 7bb1b1b to 8ffa4e5 Compare December 7, 2021 02:31

shrijeet-tesla reviewed Dec 8, 2021

View reviewed changes

danielfritsch force-pushed the feature/kafka_quota_enforcer branch from 8ffa4e5 to c1a7f20 Compare December 8, 2021 23:32

shrijeet-tesla requested review from shrijeet-tesla and jyates December 9, 2021 01:40

shrijeet-tesla approved these changes Dec 9, 2021

View reviewed changes

danielfritsch force-pushed the feature/kafka_quota_enforcer branch from c1a7f20 to 9831504 Compare December 9, 2021 02:28

jyates reviewed Dec 9, 2021

View reviewed changes

danielfritsch force-pushed the feature/kafka_quota_enforcer branch from 9831504 to 152fa0f Compare December 10, 2021 00:53

jyates approved these changes Dec 10, 2021

View reviewed changes

Add Kafka quota enforcer. Enables programmatic configuration of Kafka…

696bfb5

… quotas to restrict monopolization of Kafka broker resources caused by bad acting clients.

danielfritsch force-pushed the feature/kafka_quota_enforcer branch from 152fa0f to 696bfb5 Compare December 10, 2021 18:19

shrijeet-tesla merged commit 155dfd5 into teslamotors:master Dec 10, 2021

danielfritsch mentioned this pull request Dec 17, 2021

Use KafkaAdminClient for quota enforcement + upgrade Kafka libs to 2.6 #55

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Kafka quota enforcer #53

Add Kafka quota enforcer #53

danielfritsch commented Dec 7, 2021

shrijeet-tesla left a comment

shrijeet-tesla Dec 8, 2021

shrijeet-tesla Dec 8, 2021

shrijeet-tesla Dec 8, 2021

shrijeet-tesla left a comment

jyates left a comment

jyates Dec 9, 2021

jyates Dec 9, 2021

danielfritsch Dec 9, 2021

jyates Dec 9, 2021

jyates Dec 9, 2021

jyates Dec 9, 2021

jyates Dec 9, 2021

danielfritsch Dec 9, 2021

jyates Dec 9, 2021

danielfritsch Dec 9, 2021

jyates Dec 9, 2021

jyates Dec 10, 2021

jyates Dec 9, 2021

jyates Dec 9, 2021

jyates Dec 10, 2021

jyates left a comment

jyates Dec 10, 2021

jyates Dec 10, 2021

jyates Dec 10, 2021

jyates Dec 10, 2021

jyates Dec 10, 2021

		* Map (dictionary) properties are merged in a way that preserves base settings


		Note: `quotasFile` is supported for multi-cluster configuration too.

Add Kafka quota enforcer #53

Add Kafka quota enforcer #53

Conversation

danielfritsch commented Dec 7, 2021

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

shrijeet-tesla left a comment

Choose a reason for hiding this comment

jyates 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyates 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