KAFKA-10747: Implement APIs for altering and describing IP connection rate quotas by splett2 · Pull Request #9628 · apache/kafka

splett2 · 2020-11-20T07:34:54Z

This PR adds support for IP entities to the DescribeClientQuotas and AlterClientQuotas APIs. This change does not require any protocol version bumps.

This PR also adds support for describing/altering IP quotas via kafka-configs tooling.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

bdbyrne

LG - only minor comments, thanks for the change!

dajac

@splett2 Thanks for the PR. It looks pretty good. I have left some comments. I will need to do another pass over it, especially over the tests.

apovzner

LGTM! Left couple of minor comments.

dajac

@splett2 Thanks for the updates. I just made another pass and left few more comments.

dajac · 2020-12-04T14:41:39Z

+    val ipDefaults = parser.accepts("ip-defaults", "The config defaults for all IPs.")
+    val ip = parser.accepts("ip", "The IP address.")


It would be good to add these two flags in the KIP as well for completeness.

will do, along with the change to lower

done. while I was updating the KIP, i noticed that the previous config documentation was inconsistent with existing configs.

Default connection rate quotas for an IP address can be configured by omitting entity name. bin/kafka-configs --bootstrap-server localhost:9091 --alter --add-config 'connection_creation_rate=100' --entity-type IPs

This isn't the case for any of the existing configs, you get an error:
an entity-name or default entity must be specified with --alter of users, clients, brokers
so I also updated the KIP for default IP to specify --entity-default.

splett2 · 2020-12-08T00:28:47Z

Thanks for the feedback @dajac
I went ahead and also refactored some tests in ConfigCommandTest that were a bit repetitive.
Let me know what you think

dajac

@splett2 Thanks for the updates. I have left few minor comments.

dajac · 2020-12-08T15:07:11Z

  def validateIpConfig(ip: String, configs: Properties): Unit = {
-    if (ip != ConfigEntityName.Default && !Utils.validHostPattern(ip))
-      throw new AdminOperationException(s"IP $ip is not a valid address.")
+    DynamicConfig.Ip.validateIpOrHost(ip)


It is a little weird to throw InvalidRequestException here because there is not request involved. Users of AdminZkClient may not expect this. I think that it would be better to throw a IllegalArgumentException. Then in the AdminManager#alterClientQuotas, we can catch it and transform it into InvalidRequestException by copying the internal message.

dajac · 2020-12-08T15:08:49Z

      }

+      if (hasEntityName && entityTypeVals.contains(ConfigType.Ip)) {
+        Seq(entityName, ip).filter(options.has(_)).map(options.valueOf(_)).foreach(DynamicConfig.Ip.validateIpOrHost)


ditto here. It is a bit weird to have an InvalidRequestException thrown here.

dajac · 2020-12-08T15:10:00Z

+        if (!Utils.validHostPattern(ip))
+          throw new InvalidRequestException(s"$ip is not a valid hostname")


I wonder if this step is really necessary as InetAddress.getByName also ensures that the hostname is valid. What do you think?

dajac · 2020-12-08T15:10:17Z

+        try {
+          InetAddress.getByName(ip)
+        } catch {
+          case _ :UnknownHostException => throw new InvalidRequestException(s"$ip is not a valid IP or resolvable hostname")


nit: _ :UnknownHostException => _: UnknownHostException

dajac

@splett2 Thanks for the updates. I have left two small suggestions. Besides this, the PR looks good. One last iteration and we can merge it.

dajac · 2020-12-09T09:16:20Z

+          try {
+            DynamicConfig.Ip.validateIpOrHost(ip)
+          } catch {
+            case e: IllegalArgumentException => throw new InvalidRequestException(e.getMessage)
+          }


For my understanding, have you chosen to do the validation here to ensure that requests with validateOnly = true are fully validated?

Initially, I thought that we could just catch the IllegalArgumentException here

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

Line 1043 in 1b0ec2a

} catch {

. Doing the conversion here seems reasonable as well. Alternatively, we could also change validateIpOrHost to return a boolean and let the caller decides which exception to throw.

@dajac
yes, I thought that catching in that block would work, but after taking a closer look, it seems like entity validation is only done when altering, so we wouldn't hit validation in the validateOnly = true case.

I think changing validateIpOrHost to return a boolean is reasonable. I originally wanted to have it throw exception instead of boolean since we had two validation steps (validHostPattern and then resolvable IP), but since we simplified that code block to only do host resolution, it makes more sense to have it just return a boolean.

dajac · 2020-12-09T09:18:55Z

  }

+  @Test
+  def testAlterClientQuotasInvalidEntityCombination(): Unit = {


For these three tests, would it make sense to also verify that the error message? InvalidRequestException are thrown all over the place so we may get one but not for the expected cause. Your last commit suggests that you got caught by this already, is it? 1b0ec2a#diff-f02b619b5cd14e83f7e21dbe211b3f88336f825e1ee5c630fee32b8a0fbe3d20R294

dajac

LGTM.

dajac · 2020-12-10T08:47:44Z

Failed test is not related:

JDK 11 - kafka.api.PlaintextAdminIntegrationTest.testAlterReplicaLogDirs

dajac · 2020-12-10T08:53:56Z

@splett2 Thanks for your contribution!

dajac self-requested a review November 20, 2020 12:30

splett2 added 2 commits November 30, 2020 11:59

Alter and describe IP entity with client quota API

296330b

Adds IP entity to config command

cf888a9

splett2 force-pushed the KAFKA-10747 branch from 41864e3 to cf888a9 Compare November 30, 2020 18:04

bdbyrne approved these changes Nov 30, 2020

View reviewed changes

Address feedback, fix test

cdd2364

dajac reviewed Dec 2, 2020

View reviewed changes

splett2 added 2 commits December 2, 2020 18:10

more verbose error message

1d5d69a

IP => ip

f9e14cc

apovzner reviewed Dec 4, 2020

View reviewed changes

Comment thread core/src/test/scala/integration/kafka/network/DynamicConnectionQuotaTest.scala

apovzner reviewed Dec 4, 2020

View reviewed changes

Comment thread core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala Outdated

apovzner approved these changes Dec 4, 2020

View reviewed changes

dajac reviewed Dec 4, 2020

View reviewed changes

splett2 added 2 commits December 7, 2020 17:00

address feedback, refactor ConfigCommandTest

a59b1c8

More thorough verification of IP entity input

6bee345

dajac reviewed Dec 8, 2020

View reviewed changes

address feedback

1b0ec2a

dajac reviewed Dec 9, 2020

View reviewed changes

address feedback

a781ee7

dajac approved these changes Dec 10, 2020

View reviewed changes

dajac merged commit 404062d into apache:trunk Dec 10, 2020

		val ipDefaults = parser.accepts("ip-defaults", "The config defaults for all IPs.")
		val ip = parser.accepts("ip", "The IP address.")

		if (!Utils.validHostPattern(ip))
		throw new InvalidRequestException(s"$ip is not a valid hostname")

Conversation

splett2 commented Nov 20, 2020

Committer Checklist (excluded from commit message)

Uh oh!

bdbyrne left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

apovzner left a comment

Choose a reason for hiding this comment

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

splett2 Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

splett2 commented Dec 8, 2020

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dajac left a comment

Choose a reason for hiding this comment

Uh oh!

dajac commented Dec 10, 2020

Uh oh!

dajac commented Dec 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

splett2 Dec 8, 2020 •

edited

Loading