KAFKA-5638: Improve the Required ACL of ListGroups API (KIP-231) #5352

vahidhashemian · 2018-07-10T18:46:05Z

Committer Checklist (excluded from commit message)

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

vahidhashemian · 2018-07-10T22:10:51Z

cc @hachikuji @ewencp for review. Thanks!

omkreddy · 2018-07-11T10:19:52Z

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

+        sendResponseMaybeThrottle(request, requestThrottleMs =>
+          request.body[ListGroupsRequest].getErrorResponse(requestThrottleMs, Errors.CLUSTER_AUTHORIZATION_FAILED.exception))
+      else {
+        val filteredGroups = groups.filter(group => authorize(request.session, Describe, new Resource(Group, group.groupId, LITERAL)))


Can we just return CLUSTER_AUTHORIZATION_FAILED error, if filteredGroups is empty? This may avoid getAcls().filter iteration above.

Yes, I think that works, and should simply the error condition. Updated the PR. Thanks!

vahidhashemian · 2018-08-08T22:07:42Z

retest this please

hachikuji

Thanks for the PR. Left a few minor comments.

hachikuji · 2018-08-10T00:46:48Z

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

+      val filteredGroups = groups.filter(group => authorize(request.session, Describe, new Resource(Group, group.groupId, LITERAL)))
+      if (filteredGroups.isEmpty) {
+        sendResponseMaybeThrottle(request, requestThrottleMs =>
+          request.body[ListGroupsRequest].getErrorResponse(requestThrottleMs, Errors.CLUSTER_AUTHORIZATION_FAILED.exception))


Could we return an empty list of groups and no error instead?

Yes, we could return an empty list instead of throwing an error. I guess one reason I went with the error was for compatibility too. Prior to this, if the user did not have describe cluster permission they received this error. With your suggestion of returning an empty we never return an error. I thought this might confuse existing users.

In the latest commit I went with your suggestion.

hachikuji · 2018-08-10T00:47:54Z

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

@@ -1238,14 +1238,20 @@ class KafkaApis(val requestChannel: RequestChannel,
  }

  def handleListGroupsRequest(request: RequestChannel.Request) {
-    if (!authorize(request.session, Describe, Resource.ClusterResource)) {
+    val (error, groups) = groupCoordinator.handleListGroups()
+    // the alternative where at least some Describe Group ACLs for current principal is needed for running ListGroups


Might be worth mentioning in this comment that the cluster permission is retained for compatibility.

hachikuji · 2018-08-10T00:48:10Z

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

+    val otherConsumer = TestUtils.createConsumer(TestUtils.getBrokerListStrFromServers(servers), groupId = group2, securityProtocol = SecurityProtocol.PLAINTEXT)
+    otherConsumer.subscribe(Collections.singleton(topic))
+    consumeRecords(otherConsumer)
+


nit: unneeded newline

hachikuji · 2018-08-10T15:36:07Z

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

+    removeAllAcls()
+    addAndVerifyAcls(Set(new Acl(userPrincipal, Allow, Acl.WildCardHost, Describe)), Resource.ClusterResource)
+    // it should list both groups (due to cluster describe permission)
+    assertEquals(2, adminClient.listConsumerGroups().all().get().size())


nit: we could replace this with an explicit comparison using a set perhaps? Same below.

hachikuji · 2018-08-10T15:37:42Z

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

+    // write some record to the topic
+    addAndVerifyAcls(Set(new Acl(userPrincipal, Allow, Acl.WildCardHost, Write)), topicResource)
+    sendRecords(1, tp)
+    removeAllAcls()


nit: seems we could skip this since we remove all acls before describing the group anyway

vahidhashemian

@hachikuji thanks for reviewing the PR. I submitted a patch to address your feedback. Also, there is a response inline.

vahidhashemian · 2018-08-10T18:29:33Z

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

+      val filteredGroups = groups.filter(group => authorize(request.session, Describe, new Resource(Group, group.groupId, LITERAL)))
+      if (filteredGroups.isEmpty) {
+        sendResponseMaybeThrottle(request, requestThrottleMs =>
+          request.body[ListGroupsRequest].getErrorResponse(requestThrottleMs, Errors.CLUSTER_AUTHORIZATION_FAILED.exception))


Yes, we could return an empty list instead of throwing an error. I guess one reason I went with the error was for compatibility too. Prior to this, if the user did not have describe cluster permission they received this error. With your suggestion of returning an empty we never return an error. I thought this might confuse existing users.

In the latest commit I went with your suggestion.

hachikuji

Thanks for the updates. Left a couple additional comments.

hachikuji · 2018-08-11T20:59:16Z

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

+        new ListGroupsResponse(requestThrottleMs, error, groups.map { group => new ListGroupsResponse.Group(group.groupId, group.protocolType) }.asJava))
+    else {
+      val filteredGroups = groups.filter(group => authorize(request.session, Describe, new Resource(Group, group.groupId, LITERAL)))
+      if (filteredGroups.isEmpty)


Do we need the check? I think the else case works if filteredGroups is empty.

That makes sense, will update. Thanks.

hachikuji · 2018-08-11T21:01:00Z

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

+    addAndVerifyAcls(Set(new Acl(userPrincipal, Allow, Acl.WildCardHost, Read)), groupResource)
+    // it should list only one group now
+    val groupList = adminClient.listConsumerGroups().all().get().asScala.toList
+    assertTrue(groupList.length == 1 && groupList.head.groupId == group)


nit: It's generally preferred to use assertEquals. The advantage is that this allows junit to provide a more helpful message upon failure (we can see the value that failed the comparison). Similarly for the couple assertTrue uses below, we can compare with an empty list.

Sounds good. Will update in the next patch.

vahidhashemian

@hachikuji Thanks for taking another look. Submitted a patch to address your feedback.

vahidhashemian · 2018-08-12T17:03:23Z

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

+        new ListGroupsResponse(requestThrottleMs, error, groups.map { group => new ListGroupsResponse.Group(group.groupId, group.protocolType) }.asJava))
+    else {
+      val filteredGroups = groups.filter(group => authorize(request.session, Describe, new Resource(Group, group.groupId, LITERAL)))
+      if (filteredGroups.isEmpty)


That makes sense, will update. Thanks.

vahidhashemian · 2018-08-12T17:09:40Z

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

+    addAndVerifyAcls(Set(new Acl(userPrincipal, Allow, Acl.WildCardHost, Read)), groupResource)
+    // it should list only one group now
+    val groupList = adminClient.listConsumerGroups().all().get().asScala.toList
+    assertTrue(groupList.length == 1 && groupList.head.groupId == group)


Sounds good. Will update in the next patch.

vahidhashemian · 2018-08-12T19:44:24Z

retest this please

hachikuji

LGTM. Thanks for the patch.

…che#5352) Reviewers: Manikumar Reddy O <manikumar.reddy@gmail.com>, Jason Gustafson <jason@confluent.io>

vahidhashemian force-pushed the KAFKA-5638_KIP-231 branch 2 times, most recently from 47406fc to deb839b Compare July 10, 2018 19:03

omkreddy reviewed Jul 11, 2018

View reviewed changes

vahidhashemian force-pushed the KAFKA-5638_KIP-231 branch from 8321349 to 8cee70f Compare August 1, 2018 23:45

hachikuji self-assigned this Aug 8, 2018

hachikuji reviewed Aug 10, 2018

View reviewed changes

vahidhashemian force-pushed the KAFKA-5638_KIP-231 branch 2 times, most recently from adf1075 to 93ef0d2 Compare August 10, 2018 19:14

vahidhashemian commented Aug 10, 2018

View reviewed changes

hachikuji reviewed Aug 11, 2018

View reviewed changes

vahidhashemian added 4 commits August 12, 2018 09:58

KAFKA-5638: Improve the Required ACL of ListGroups API (KIP-231)

ecfe819

Simplified the error condition

6ab0c00

Minor updates to address feedback

8c930c4

Minor improvements

6fb99d3

vahidhashemian force-pushed the KAFKA-5638_KIP-231 branch from 93ef0d2 to 6fb99d3 Compare August 12, 2018 17:11

vahidhashemian commented Aug 12, 2018

View reviewed changes

hachikuji approved these changes Aug 13, 2018

View reviewed changes

hachikuji merged commit e6fd99d into apache:trunk Aug 13, 2018

pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019

KAFKA-5638; Improve the Required ACL of ListGroups API (KIP-231) (apa…

7827b23

…che#5352) Reviewers: Manikumar Reddy O <manikumar.reddy@gmail.com>, Jason Gustafson <jason@confluent.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-5638: Improve the Required ACL of ListGroups API (KIP-231) #5352

KAFKA-5638: Improve the Required ACL of ListGroups API (KIP-231) #5352

vahidhashemian commented Jul 10, 2018

vahidhashemian commented Jul 10, 2018

omkreddy Jul 11, 2018

vahidhashemian Jul 11, 2018

vahidhashemian commented Aug 8, 2018

hachikuji left a comment

hachikuji Aug 10, 2018

vahidhashemian Aug 10, 2018

hachikuji Aug 10, 2018

hachikuji Aug 10, 2018

hachikuji Aug 10, 2018

hachikuji Aug 10, 2018

vahidhashemian left a comment

vahidhashemian Aug 10, 2018

hachikuji left a comment

hachikuji Aug 11, 2018

vahidhashemian Aug 12, 2018

hachikuji Aug 11, 2018 •

edited

Loading

vahidhashemian Aug 12, 2018

vahidhashemian left a comment

vahidhashemian Aug 12, 2018

vahidhashemian Aug 12, 2018

vahidhashemian commented Aug 12, 2018

hachikuji left a comment

KAFKA-5638: Improve the Required ACL of ListGroups API (KIP-231) #5352

KAFKA-5638: Improve the Required ACL of ListGroups API (KIP-231) #5352

Conversation

vahidhashemian commented Jul 10, 2018

Committer Checklist (excluded from commit message)

vahidhashemian commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vahidhashemian commented Aug 8, 2018

hachikuji 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

vahidhashemian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hachikuji left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hachikuji Aug 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vahidhashemian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vahidhashemian commented Aug 12, 2018

hachikuji left a comment

Choose a reason for hiding this comment

hachikuji Aug 11, 2018 •

edited

Loading